147 lines
6.8 KiB
Markdown
147 lines
6.8 KiB
Markdown
# Areas of Concern
|
||
|
||
Prioritized: **HIGH** = security/data risk, **MEDIUM** = significant tech debt, **LOW** = polish/quality.
|
||
|
||
---
|
||
|
||
## Security — HIGH
|
||
|
||
### Hardcoded credentials in [config.php](config.php)
|
||
|
||
DB password, email password, and remote DB host stored in plaintext. If the repo leaks (or is on a shared dev machine), full DB compromise is immediate.
|
||
|
||
**Fix:** move to `.env` + `getenv()`; add `config.php` to `.gitignore`; rotate the leaked secret.
|
||
|
||
### Unsafe `unserialize()`
|
||
|
||
PHP object-injection surface:
|
||
|
||
- [autoload/class.Cache.php](autoload/class.Cache.php) line 29 — `@unserialize($data)` (cache files in `temp/`)
|
||
- [libraries/grid/grid.php](libraries/grid/grid.php) lines 95, 122
|
||
- [libraries/medoo/medoo.php](libraries/medoo/medoo.php) line 1264
|
||
|
||
**Fix:** switch to `json_encode/decode`, or pass the `['allowed_classes' => false]` option.
|
||
|
||
### Insecure persistent-login cookie
|
||
|
||
[index.php](index.php) lines 92-102 sets a cookie containing JSON `{email, hash}` with no `HttpOnly`, no `Secure`, no `SameSite` (also see `setcookie()` calls in [autoload/controls/class.Users.php](autoload/controls/class.Users.php) lines 43, 561).
|
||
|
||
**Fix:** issue an opaque random token, store hash server-side, set `HttpOnly; Secure; SameSite=Strict`.
|
||
|
||
### `eval()` in vendored grid
|
||
|
||
[libraries/grid/templates/results.php](libraries/grid/templates/results.php) line 289 and [libraries/grid/templates/print.php](libraries/grid/templates/print.php) line 73 evaluate strings drawn from `$_SESSION`. Session takeover ⇒ RCE.
|
||
|
||
**Fix:** replace with a safe expression evaluator or simple template helpers.
|
||
|
||
### Debug scripts shipped with credentials
|
||
|
||
`tmp/debug_*.php` (7 files) embed live DB creds. If `tmp/` is web-accessible, they're a one-shot console.
|
||
|
||
**Fix:** delete from repo; add `tmp/` to `.gitignore`; verify `tmp/` is not served (check [.htaccess](.htaccess)).
|
||
|
||
---
|
||
|
||
## Security — MEDIUM
|
||
|
||
### No CSRF protection
|
||
|
||
POST endpoints (controllers, [ajax.php](ajax.php), [api.php](api.php)) accept requests with no token validation. **Fix:** generate per-session token, embed in forms, verify in mutating actions.
|
||
|
||
### Path traversal in `\Tpl::render()`
|
||
|
||
[autoload/class.Tpl.php](autoload/class.Tpl.php) lines 31-62 builds `include` paths from `$file` without whitelisting. If `$file` ever flows from request data, `../../config` is reachable. Currently template names are hardcoded in controllers, so risk is latent — keep it that way.
|
||
|
||
### Inconsistent XSS escaping in templates
|
||
|
||
[templates/products/main_view.php](templates/products/main_view.php) defines a local `escape_html()` but applies it inconsistently. Polish content with apostrophes / quoted JSON in `<script>` blocks needs `htmlspecialchars($v, ENT_QUOTES, 'UTF-8')` consistently.
|
||
|
||
### IP-bound sessions break legitimate users
|
||
|
||
[ajax.php](ajax.php) lines 28-33 destroys the session on any `REMOTE_ADDR` change. NAT, mobile networks, and VPN switches will log users out frequently. Consider fingerprinting (UA + IP `/24`) instead of strict IP equality.
|
||
|
||
### File upload in Allegro controller
|
||
|
||
[autoload/controls/class.Allegro.php](autoload/controls/class.Allegro.php) lines 47-62 validates extension via `pathinfo`, not MIME. **Fix:** `finfo_file()`; store outside webroot.
|
||
|
||
### `exec()` in vendored upload handler
|
||
|
||
[libraries/filemanager-9.14.1/UploadHandler.php](libraries/filemanager-9.14.1/UploadHandler.php) lines 1006, 1032. **Fix:** prefer the `imagick`/`gd` extension; if `exec` stays, validate every argument with `escapeshellarg`.
|
||
|
||
---
|
||
|
||
## Tech debt — MEDIUM
|
||
|
||
### Dual ORM (Medoo + RedBeanPHP)
|
||
|
||
Medoo (`$mdb`) is the documented standard, but [api.php](api.php) (~1350 lines) and [cron.php](cron.php) use RedBeanPHP (`\R::`). Two query styles, two error paths, two sets of edge cases. Estimated 2-3 weeks to consolidate to Medoo.
|
||
|
||
### Monolithic controllers / factories
|
||
|
||
| File | Lines |
|
||
|---|---|
|
||
| [autoload/controls/class.Cron.php](autoload/controls/class.Cron.php) | ~5,200 |
|
||
| [autoload/factory/class.Products.php](autoload/factory/class.Products.php) | ~1,540 |
|
||
| [api.php](api.php) | ~1,350 |
|
||
| [autoload/services/class.GoogleAdsApi.php](autoload/services/class.GoogleAdsApi.php) | ~3,200 |
|
||
|
||
`Cron` mixes campaign sync, product sync, search-term aggregation, and Facebook ingest. Extract per-pipeline service classes.
|
||
|
||
### N+1 queries in cron paths
|
||
|
||
[autoload/controls/class.Cron.php](autoload/controls/class.Cron.php) lines 96 (`task_user`), 155 (`users.email` inside nested loop). For 10 tasks × multiple users per task → ~100 round-trips. **Fix:** pre-fetch with `WHERE IN (...)`.
|
||
|
||
### Migration safety
|
||
|
||
[migrations/](migrations/) files use `IF NOT EXISTS` (good) and `schema_migrations` tracks application, but there's no rollback mechanism and no recovery path if a migration partially fails mid-run.
|
||
|
||
### Cache strategy gaps
|
||
|
||
[autoload/class.Cache.php](autoload/class.Cache.php) is file-based with MD5 keys, no invalidation hooks, no namespacing, no TTL audit. Suppression (`@unserialize`) hides corruption. Multi-server deployments would silently desync.
|
||
|
||
---
|
||
|
||
## Operational — MEDIUM
|
||
|
||
### No retry / backoff on external APIs
|
||
|
||
[autoload/services/class.GoogleAdsApi.php](autoload/services/class.GoogleAdsApi.php) issues 50+ direct `curl_exec` calls. No exponential backoff, no rate-limit detection. Same in [class.FacebookAdsApi.php](autoload/services/class.FacebookAdsApi.php). A throttle response just becomes a logged error.
|
||
|
||
### Global warning suppression
|
||
|
||
`error_reporting(E_ALL ^ E_NOTICE ^ E_STRICT ^ E_WARNING)` in [ajax.php](ajax.php) line 1 hides production bugs. Log warnings to a file, even if not displayed.
|
||
|
||
### No secrets rotation
|
||
|
||
API keys live in `settings` indefinitely; no expiry, no rotation reminder. Consider a 90-day reminder + log entry on access.
|
||
|
||
---
|
||
|
||
## Frontend — LOW
|
||
|
||
- jQuery 3.6 is fine; Bootstrap 5 (active) coexists with Bootstrap 4.1.3 (legacy) in [libraries/bootstrap-4.1.3/](libraries/bootstrap-4.1.3/) — verify nothing in production still loads it.
|
||
- Two Font Awesome versions (4.7.0 + 6.5.1) — same audit needed.
|
||
- SCSS is compiled manually. A 5-line `package.json` + `sass` watcher would prevent stale CSS commits.
|
||
|
||
---
|
||
|
||
## Compliance / privacy — LOW
|
||
|
||
- No documented GDPR data-export or deletion flow despite serving EU/PL users.
|
||
- Persistent-cookie design embeds user identifier; not strictly PII-violating but worth re-architecting alongside the cookie-security fix.
|
||
- No retention policy on `logs` or product/campaign history tables.
|
||
|
||
---
|
||
|
||
## Suggested remediation order
|
||
|
||
1. **Rotate and remove [config.php](config.php) credentials** — 1 day.
|
||
2. **Cookie hardening + opaque token** — 2-3 days.
|
||
3. **Replace `unserialize` with JSON in cache + grid** — 3-5 days.
|
||
4. **CSRF tokens on mutating endpoints** — 1 week.
|
||
5. **Consolidate ORMs (drop RedBeanPHP)** — 2-3 weeks.
|
||
6. **Split `Cron` and `Products` factory** — 2 weeks.
|
||
7. **Introduce PHPUnit + minimal CI** — 1 week.
|
||
|
||
Total realistic remediation: **~13 weeks** at one full-time engineer.
|