133 lines
7.0 KiB
Markdown
133 lines
7.0 KiB
Markdown
# Codebase Concerns
|
||
|
||
**Analysis Date:** 2026-04-27
|
||
|
||
## Security Considerations
|
||
|
||
**Credentials committed to repository:**
|
||
- Risk: DB credentials and SMTP credentials checked into version control
|
||
- Files: `_rejestracja/core/config/db.config.ini`, `_rejestracja/core/config/smtp.config.ini`
|
||
- Current mitigation: None — production secrets in plaintext in repo
|
||
- Recommendations: Move to gitignored config files; add `.gitignore` entry; rotate credentials if repo is/was public
|
||
|
||
**Web-accessible migration runners:**
|
||
- Risk: Anyone who knows the URL can trigger `ALTER TABLE` operations on production DB
|
||
- Files: `_rejestracja/sql/apply-*.php` (all runner files)
|
||
- Current mitigation: Requires `?run=YYYYMMDD` param to prevent accidental execution
|
||
- Recommendations: Move runners outside web root, or add IP/auth check before execution
|
||
|
||
**`eval()` usage in core:**
|
||
- Risk: Remote code execution if user input reaches eval
|
||
- Files: Detected in `_rejestracja/core/class/` (exact file TBD — needs verification)
|
||
- Current mitigation: Unknown — depends on what is eval'd
|
||
- Recommendations: Audit eval usage; replace with explicit logic where possible
|
||
|
||
**No input sanitization layer:**
|
||
- Risk: SQL injection if user input bypasses DAL parameterization; XSS if template escaping is skipped
|
||
- Files: `_rejestracja/controller/IndexController.php` — uses `Request::GetPost()` directly
|
||
- Current mitigation: Smarty auto-escaping in templates; DAL may use parameterized queries (verify)
|
||
- Recommendations: Audit DAL for raw query concatenation; confirm all template output uses `|escape`
|
||
|
||
## Tech Debt
|
||
|
||
**`core/_model/` directory duplication:**
|
||
- Issue: 78 files in `_rejestracja/core/_model/` mirror `_rejestracja/core/model/` but are NOT loaded by the autoloader
|
||
- Files: All files in `_rejestracja/core/_model/`
|
||
- Why: Historical backup that was never deleted
|
||
- Impact: Confusing when navigating; edits to `_model/` are silently ignored; risk of editing wrong file
|
||
- Fix approach: Delete `_rejestracja/core/_model/` entirely — it serves no runtime purpose
|
||
|
||
**Commented-out debug calls throughout controllers:**
|
||
- Issue: `//Utils::ArrayDisplay(Request::GetAllPost());` and similar calls left in code
|
||
- Files: `_rejestracja/Admin/controller/CalcController.php` (lines 54, 118), others
|
||
- Why: Dev debugging not cleaned up
|
||
- Impact: Clutter; risk of accidentally uncommenting in production
|
||
- Fix approach: Remove all commented-out debug calls during any refactor pass
|
||
|
||
**`TODO` about disc field in IndexController:**
|
||
- Issue: Unresolved logic around the `disc` field in fee calculation
|
||
- Files: `_rejestracja/controller/IndexController.php`
|
||
- Why: Deferred during earlier development
|
||
- Impact: Potentially incorrect discount pricing in edge cases
|
||
- Fix approach: Review `disc` field handling; either implement or document the intended behavior
|
||
|
||
**`setSortBy()` raw string injection:**
|
||
- Issue: `DalData::setSortBy()` injects value directly into `ORDER BY` SQL clause with no escaping
|
||
- Files: `_rejestracja/core/class/DefaultDAL.class.php`, all callers
|
||
- Why: Design choice for simplicity
|
||
- Impact: SQL injection risk if sort value ever comes from user input (currently hardcoded — low risk)
|
||
- Fix approach: Whitelist allowed sort columns; never pass user input to `setSortBy()`
|
||
|
||
## Fragile Areas
|
||
|
||
**`core/core.php` autoloader:**
|
||
- Files: `_rejestracja/core/core.php`
|
||
- Why fragile: `spl_autoload_register` loads from `core/model/` — if class naming deviates from `Mf{Entity}.class.php` convention, autoload silently fails
|
||
- Common failures: Editing `core/_model/` instead of `core/model/` (no error, just stale behavior)
|
||
- Safe modification: Always verify you're editing `core/model/` not `core/_model/`
|
||
|
||
**`$fields` array in model classes:**
|
||
- Files: All `_rejestracja/core/model/Mf*.class.php`
|
||
- Why fragile: `$fields` array must exactly match DB column names; mismatch means property reads `null` silently
|
||
- Common failures: Adding a DB column without updating `$fields`; typo in column name
|
||
- Safe modification: After adding to `$fields`, verify getter returns expected value from DB
|
||
|
||
**Smarty template variable assignment:**
|
||
- Files: All `_rejestracja/template/` and `_rejestracja/Admin/template/` `.tpl` files
|
||
- Why fragile: If controller doesn't `assign()` a variable, Smarty silently outputs empty string — no error
|
||
- Common failures: Renaming a controller variable without updating template references
|
||
- Safe modification: Search for the variable name in both controller and all related templates before renaming
|
||
|
||
## Known Bugs
|
||
|
||
**`RegDeleteAction` deletes wrong record:**
|
||
- Symptoms: `CalcController::RegDeleteAction` calls `MfParametersDAL::Delete($dalData)` instead of `MfParticipantDAL::Delete($dalData)` — DAL class mismatch
|
||
- Files: `_rejestracja/Admin/controller/CalcController.php` lines 187–206
|
||
- Workaround: Unknown — may delete a pricing parameter row instead of the participant
|
||
- Root cause: Copy-paste error in controller method; wrong DAL class referenced
|
||
|
||
## Performance Bottlenecks
|
||
|
||
**No query caching:**
|
||
- Problem: Every request hits MySQL — `mf_dictionary` is read on every page load for every `{translate}` call
|
||
- Files: `_rejestracja/core/model/MfDictionaryDAL.class.php`, all templates using `{translate}`
|
||
- Cause: No caching layer; MySQL query per translation key
|
||
- Improvement path: Cache dictionary in PHP session or static array per request
|
||
|
||
**No pagination on admin registration list:**
|
||
- Problem: `CalcController::RegAction` loads all `mf_participant` rows into memory with no LIMIT
|
||
- Files: `_rejestracja/Admin/controller/CalcController.php` (RegAction), `_rejestracja/Admin/template/partial/Calc/Reg.tpl`
|
||
- Cause: `DalData` has no pagination set; template has commented-out pager HTML
|
||
- Impact: Will degrade significantly as registration count grows past a few hundred rows
|
||
|
||
## Dependencies at Risk
|
||
|
||
**PHP 5.x:**
|
||
- Risk: PHP 5.x is end-of-life since December 2018; no security patches
|
||
- Impact: Known PHP 5.x vulnerabilities unpatched on production server
|
||
- Migration plan: Requires testing all code for PHP 7.x/8.x compatibility (significant effort)
|
||
|
||
**jQuery 1.x:**
|
||
- Risk: jQuery 1.x is unmaintained; known XSS vulnerabilities in older versions
|
||
- Impact: Frontend JS security issues
|
||
- Migration plan: Upgrade to jQuery 3.x or replace with vanilla JS
|
||
|
||
**Bundled Smarty / PHPMailer:**
|
||
- Risk: Vendor-bundled libraries in `core/lib/` may be outdated versions with unpatched CVEs
|
||
- Impact: Template injection (Smarty), email header injection (PHPMailer)
|
||
- Migration plan: Replace with Composer-managed dependencies and current versions
|
||
|
||
## Test Coverage Gaps
|
||
|
||
**Everything — no tests exist:**
|
||
- What's not tested: All application logic
|
||
- Files: All of `_rejestracja/`
|
||
- Risk: Any code change could introduce silent regressions; verified by manual testing only
|
||
- Priority: High for `IndexController.php` (primary user-facing flow) and `DefaultDAL.class.php` (affects all DB operations)
|
||
- Difficulty to test: Medium — PHP 5.x compatible test framework required; DAL tests need DB fixture
|
||
|
||
---
|
||
|
||
*Concerns audit: 2026-04-27*
|
||
*Update as issues are fixed or new ones discovered*
|