138 lines
8.7 KiB
Markdown
138 lines
8.7 KiB
Markdown
# Codebase Concerns
|
||
|
||
**Analysis Date:** 2026-05-10
|
||
|
||
> Severity legend: **CRITICAL** (immediate fix), **HIGH** (next sprint), **MEDIUM** (planned), **LOW** (track).
|
||
|
||
## Critical Security
|
||
|
||
**Hardcoded credentials committed to repository — CRITICAL**
|
||
- Files: `buy-by-phone.php` (lines ~11–28: reCAPTCHA secret `6LeJAUUsAAAAAIyCrwFMrsw9yQLPgnCWCOHPcjc8`, SMTP passwords `8njIZnAb`, `kM4BU_F_`), `app/config/parameters.php` (DB password `eRbZ]Ioh-0-2]fM+`, app secret `LStbzv3bfsMaq5dvsVR5wXt8dEpe63IAx2uOHffpbK9gy9x4EZsjWeRM`, cookie keys)
|
||
- Why: convenience during development — never moved to env vars
|
||
- Impact: full DB compromise, mail account takeover, session forgery if repo or backups leak; secrets remain valid in git history
|
||
- Fix: rotate ALL exposed credentials immediately → move to environment variables (or Docker secrets) → load via `getenv()` → purge from git history (`git filter-repo` / BFG) → add the file to `.gitignore`
|
||
|
||
**Diagnostic / phpinfo files exposed in webroot — CRITICAL**
|
||
- Files: `diag_20792_tmp.php` (2.6 KB), `diag_upload_tmp.php` (4.4 KB), `diag_fix_tmp.php` (14 B), `info.php` (14 B — almost certainly `<?php phpinfo();`)
|
||
- Why: ad-hoc debug scripts left after a 2026-03-05 incident (`feat: update diagnostic scripts and configuration for improved product data handling`, commit `194405bc`)
|
||
- Impact: `info.php` discloses full PHP environment to anyone; diag scripts run raw SQL with weak `$_GET['key'] !== 'diag2026'` gate (security through obscurity, easily bypassed)
|
||
- Fix: delete these 4 files now; if production deploy mirrors repo, also delete on server; consider reverting commit `194405bc`
|
||
|
||
**Unsanitized `$_POST` / `$_GET` in custom scripts — HIGH**
|
||
- Files: `buy-by-phone.php` lines 29–58 (17 direct `$_POST[...]` accesses) and lines 83–94 (POST values injected into HTML email body — XSS via mail / SMTP header injection); `category-description.php` line 7 (`$url = $_GET['url']`)
|
||
- Why: written outside PrestaShop conventions
|
||
- Impact: header injection, mail-relay abuse, stored XSS in support inboxes
|
||
- Fix: replace with `Tools::getValue('field', '')` (PrestaShop sanitizer) and escape on output (`htmlspecialchars`, `Tools::safeOutput`); validate email with `Validate::isEmail()`; add reCAPTCHA verification result check before send
|
||
|
||
**SQL string concatenation in custom scripts — HIGH**
|
||
- Files: `diag_20792_tmp.php` lines 23–49, `diag_upload_tmp.php` lines 22–50, `import-product.php` lines 63–93
|
||
- Why: legacy PrestaShop pattern (`pSQL()` instead of bound parameters)
|
||
- Impact: any unsanitized input flowing into these queries is an injection vector
|
||
- Fix: delete diag scripts; refactor `import-product.php` to use `Db::getInstance()` with proper escaping or Doctrine DBAL parameterized queries; never concatenate user input into SQL
|
||
|
||
**Database backups + 5 MB error log inside webroot — HIGH**
|
||
- Files: `iadmin/backups/` (e.g. `1698930967-7683b7af.sql.bz2`), `iadmin/errors.log` (5 MB, 26 months of PHP errors), `errors.log` (root)
|
||
- Why: standard PrestaShop default location; `iadmin/` is the renamed admin folder (still under DocumentRoot)
|
||
- Impact: SQL dumps directly downloadable if URL guessed; error log discloses paths and module internals
|
||
- Fix: move backups outside DocumentRoot or `Deny from all` via `.htaccess`; rotate logs to `/var/log/...`; add `errors.log`, `*.log`, `iadmin/backups/`, `iadmin/errors.log` to `.gitignore`
|
||
|
||
## Tech Debt
|
||
|
||
**Backup and obsolete files committed — HIGH**
|
||
- Files: `backup_before_patch/As4SearchEngine.php.backup` (160 KB), `backup_before_patch/As4SearchEngineDb.php.backup` (3.4 KB), `backup_before_patch/pm_advancedsearch4.php.backup` (279 KB)
|
||
- Why: manual snapshot taken before patching the search engine module
|
||
- Impact: bloats repo, confuses code search, may shadow current implementation in IDEs
|
||
- Fix: delete the directory; rely on `git log` for history; add `*.backup`, `backup_before_patch/` to `.gitignore`
|
||
|
||
**Oversized root-level scripts — MEDIUM**
|
||
- Files: `import-product.php` (813 lines / 32 KB — fetches and applies external XML feed), `buy-by-phone.php` (122 lines — manual PHPMailer + reCAPTCHA)
|
||
- Why: written as single-file utilities
|
||
- Impact: hard to test, no transaction safety in importer, no logging of import results
|
||
- Fix approach: extract `import-product.php` into a proper PrestaShop module or CLI command (`bin/console`-style), wrap DB writes in transactions, add structured logging; convert `buy-by-phone.php` into a `ModuleFrontController`
|
||
|
||
**429-line `.htaccess` with hardcoded SEO redirects — MEDIUM**
|
||
- File: `.htaccess` (~100 manual `RewriteRule ^...$ /s/.../...?` redirects, lines 14–370+)
|
||
- Impact: not maintainable, regex conflicts likely, no audit trail
|
||
- Fix: migrate to a database-driven redirect table + module that emits 301s in `hookActionDispatcher` (or use `modules/x13linkrewrite/`)
|
||
|
||
**107 undocumented core overrides — MEDIUM**
|
||
- Files: `override/classes/` (31 files — `Cart.php`, `CartRule.php`, `Product.php`, `Hook.php`, `Dispatcher.php`, `controller/FrontController.php`, `controller/ProductListingFrontController.php`, …) plus 76 more under `override/`
|
||
- Why: each accumulated over time to extend core behavior
|
||
- Impact: every PrestaShop upgrade requires manual override review; no inline doc states why each override exists
|
||
- Fix: add a header comment to each override stating purpose + ticket; consider moving simple cases to module hooks
|
||
|
||
**Stale stack — PrestaShop 1.7.8.11 / PHP 7.4 — MEDIUM**
|
||
- File: `app/AppKernel.php` line 35 (`const VERSION = '1.7.8.11'`)
|
||
- Why: production stability
|
||
- Impact: missing security patch 1.7.8.12; PHP 7.4 reached EOL 2022-11-28 (no upstream security fixes); core 1.7.x EOL since 2021-10
|
||
- Fix: apply 1.7.8.12 patch first (minor), then plan migration to PrestaShop 8.x or 9.x; PHP 8.2 already in runtime — verify all modules under PHP 8.x compat-checker
|
||
|
||
**`iadmin/autoupgrade/` contains `prestashop_1.7.5.1.zip` left in webroot — MEDIUM**
|
||
- Why: artifact of a past upgrade attempt
|
||
- Impact: downloadable archive exposing core source paths
|
||
- Fix: delete
|
||
|
||
## Performance
|
||
|
||
**Custom cross-sell module clean — no concerns**
|
||
- File: `modules/crosssellpro/crosssellpro.php`
|
||
- Uses `Product::getAccessoriesLight()` (efficient) + `Product::getProductsProperties()`; filters with `Shop::addSqlAssociation`; respects `DEFAULT_LIMIT = 12`
|
||
- No N+1 detected
|
||
|
||
**No application-level caching active**
|
||
- Memcached configured but disabled (`app/config/parameters.php`)
|
||
- Page cache module installed (`modules/pagecache/`) — verify it is enabled in production
|
||
- Fix: enable Memcached + verify pagecache configuration; add OPcache validation if not on
|
||
|
||
## Fragile Areas
|
||
|
||
**Override chain in `override/classes/`**
|
||
- Why fragile: `Cart::getOrderTotal()` extension depends on `ets_promotion` module presence (`override/classes/Cart.php` line ~55); `Hook::exec` extension depends on `pagecache` (`override/classes/Hook.php` line ~42)
|
||
- Common failures: removing one of those modules without cleaning up the override leaves dead branches
|
||
- Safe modification: read the override + the referenced module's hook surface together; never edit core `classes/` directly
|
||
|
||
**Recurring header-modification errors in admin**
|
||
- Source: `iadmin/errors.log` shows repeated errors at `AdminCartRulesController.php:121` (output before `header()` call)
|
||
- Trigger: editing cart rules in back-office
|
||
- Likely cause: a module emitting whitespace before `<?php` or echoing in `init()`
|
||
|
||
**MySQL "server has gone away" entries**
|
||
- Source: `iadmin/errors.log`
|
||
- Likely cause: long-running scripts (`import-product.php`?) exceeding `wait_timeout`
|
||
- Fix: increase `wait_timeout`/`mysql.connect_timeout` in PHP config or call `Db::getInstance()->disconnect()` + reconnect for long-running batch jobs
|
||
|
||
## Test Coverage Gaps
|
||
|
||
- **Custom modules have NO tests** — `modules/crosssellpro/`, `modules/caraty/`
|
||
- **No tests for `import-product.php`** (highest-risk script — bulk DB writes, no rollback)
|
||
- **No tests for `buy-by-phone.php`** (handles user input + sends mail)
|
||
- Risk: regressions land silently; manual browser testing is the only safety net
|
||
|
||
## Documentation Gaps
|
||
|
||
- No `README.md` at repo root (only PrestaShop's stock files)
|
||
- 31 core overrides — none have purpose comments
|
||
- `iadmin/errors.log` retains 26 months of error history but no rotation policy
|
||
|
||
## Deletion Candidates (Quick Wins)
|
||
|
||
```
|
||
diag_20792_tmp.php
|
||
diag_fix_tmp.php
|
||
diag_upload_tmp.php
|
||
info.php
|
||
errors.log
|
||
backup_before_patch/
|
||
iadmin/errors.log
|
||
iadmin/autoupgrade/prestashop_1.7.5.1.zip
|
||
themes/classic.zip # (verify needed)
|
||
themes/leo_gstore.zip
|
||
```
|
||
|
||
Add to `.gitignore`: `errors.log`, `*.log`, `*.backup`, `backup_before_patch/`, `iadmin/backups/`, `iadmin/errors.log`, `iadmin/autoupgrade/*.zip`.
|
||
|
||
---
|
||
|
||
*Concerns audit: 2026-05-10*
|
||
*Update as issues are fixed or new ones discovered*
|