docs: map existing codebase
- stack.md - Technologies and dependencies - architecture.md - System design and patterns - structure.md - Directory layout - conventions.md - Code style and patterns - testing.md - Test structure (none) - integrations.md - External services - concerns.md - Technical debt and issues - db_schema.md - Database schema and relationships Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
131
.paul/codebase/concerns.md
Normal file
131
.paul/codebase/concerns.md
Normal file
@@ -0,0 +1,131 @@
|
||||
# Codebase Concerns
|
||||
|
||||
**Analysis Date:** 2026-04-26
|
||||
|
||||
## Critical Security Issues
|
||||
|
||||
**SQL Injection — Admin Panel:**
|
||||
- `autoload/controls/class.Apanel.php` (line ~34)
|
||||
- `$_GET['id']` concatenated directly into raw SQL: `'SELECT * FROM order_tickets WHERE order_id =' . $clientId`
|
||||
- Any authenticated admin (or CSRF attacker) can inject arbitrary SQL
|
||||
- Fix: Use Medoo parameterized query: `$mdb->select('order_tickets', '*', ['order_id' => (int)$clientId])`
|
||||
|
||||
**Hardcoded Credentials in Source Control:**
|
||||
- `config.php` — DB password, SMTP password (`biletyonline`), admin password (`Admin2022!`), Przelewy24 CRC key, fakturowo.pl API key, test mode secret
|
||||
- Any repo access exposes production credentials
|
||||
- Fix: Move to environment variables or a non-committed config file
|
||||
|
||||
**Weak Password Hashing (MD5):**
|
||||
- `autoload/controls/class.Users.php` and `autoload/factory/class.Users.php`
|
||||
- User passwords stored and compared as plain MD5 — trivially broken by rainbow tables
|
||||
- Fix: Replace with `password_hash()` / `password_verify()` (bcrypt)
|
||||
|
||||
**Payment Callback Not Fully Verified:**
|
||||
- `autoload/controls/class.Tickets.php::przelewy24_response()`
|
||||
- CRC check exists but cURL verification call return value may not be checked before marking order paid
|
||||
- Fix: Ensure verification response is confirmed successful before updating `payment_status`
|
||||
|
||||
## High Priority
|
||||
|
||||
**Missing Input Validation on Checkout Form:**
|
||||
- `autoload/controls/class.Tickets.php::basketFormHandler()`
|
||||
- `$_POST` fields (email, name, NIP, ZIP) trimmed but not format-validated before DB insert
|
||||
- Email not validated with `filter_var($email, FILTER_VALIDATE_EMAIL)`
|
||||
- NIP, ZIP not validated for Polish format
|
||||
- Fix: Add validation layer before processing order
|
||||
|
||||
**888-Line Monolithic Controller:**
|
||||
- `autoload/controls/class.Tickets.php` — 888 lines, handles 15+ distinct operations
|
||||
- Mixes: ticket display, cart management, payment processing, email sending, invoice generation, QR generation, GA data layer
|
||||
- Makes changes risky (large surface area, no tests)
|
||||
- Fix: Extract into `TicketsCart`, `TicketsCheckout`, `TicketsPayment`, `TicketsNotification`
|
||||
|
||||
**No CSRF Protection on Admin Actions:**
|
||||
- `autoload/controls/class.Apanel.php` — all POST actions (login, order edits, deletes)
|
||||
- No CSRF tokens on any form
|
||||
- Fix: Add token generation + validation middleware
|
||||
|
||||
**cURL Error Handling Missing:**
|
||||
- `autoload/controls/class.Tickets.php` — Przelewy24 and fakturowo.pl cURL calls
|
||||
- `curl_exec()` return value not consistently checked; no timeout set
|
||||
- Silent failures possible: order could be marked paid without invoice, or invoice not generated
|
||||
- Fix: Check return value, add `CURLOPT_TIMEOUT`, log failures
|
||||
|
||||
**No Session Fixation Protection on Login:**
|
||||
- `autoload/controls/class.Users.php::login()` — no `session_regenerate_id(true)` on successful auth
|
||||
- Session regeneration only on first visit (new session), not on privilege escalation
|
||||
- Fix: Call `session_regenerate_id(true)` after successful login
|
||||
|
||||
## Medium Priority
|
||||
|
||||
**Hardcoded Domain URLs:**
|
||||
- `autoload/controls/class.Tickets.php` — `https://bilety.brzezovka.pl` hardcoded in ~5 places
|
||||
- Breaks if domain changes; makes staging difficult
|
||||
- Fix: Store base URL in `config.php` as `$settings['base_url']`
|
||||
|
||||
**No Application Logging:**
|
||||
- No error logging system (PSR-3 or equivalent)
|
||||
- No audit trail for payments, admin actions, or errors
|
||||
- Only optional `mail_debug.log` for email
|
||||
- Fix: Add basic file logger for critical operations (payment events, admin changes)
|
||||
|
||||
**Weak Order Hash Generation:**
|
||||
- `autoload/controls/class.Tickets.php` — hash based on MD5 of email + city + timestamp
|
||||
- 1-second collision window for same customer; predictable construction
|
||||
- Fix: Use `bin2hex(random_bytes(16))` for cryptographically random order hashes
|
||||
|
||||
**Global Variables Without DI:**
|
||||
- All controllers: `global $settings, $mdb, $user` at top of every method
|
||||
- Untestable, hidden dependencies, risk of undefined global
|
||||
- Fix: Long-term — dependency injection; short-term — document as known pattern
|
||||
|
||||
**QR Codes in Web-Accessible Directory:**
|
||||
- `orders/{hash[0]}/{hash[1]}/{hash}.png` — served directly by Apache
|
||||
- Hash is predictable (see hash concern above)
|
||||
- Fix: Move QR storage outside web root; serve via PHP controller with auth check
|
||||
|
||||
**Large Template Files:**
|
||||
- `templates/tickets/main-view.php` — 401 lines
|
||||
- `templates/admin-panel/order-data.php` — 373 lines
|
||||
- `templates/tickets/basket-view.php` — 316 lines
|
||||
- Mixed PHP logic and HTML; difficult to maintain
|
||||
- Fix: Extract repeated logic to view helper classes
|
||||
|
||||
**Duplicate Bootstrap Code:**
|
||||
- `index.php`, `ajax.php`, `api.php`, `cron.php` — each contains near-identical DB init, session, autoloader
|
||||
- Fix: Extract to `bootstrap.php` included by all entry points
|
||||
|
||||
**No Error Handling on DB Writes:**
|
||||
- `autoload/controls/class.Tickets.php` — `$mdb->insert()` / `$mdb->update()` calls without checking return value
|
||||
- Silent failure if DB write fails (order not saved but user gets confirmation)
|
||||
- Fix: Check return value; wrap in try/catch
|
||||
|
||||
## Low Priority / Documentation
|
||||
|
||||
**Magic Numbers in Pricing Logic:**
|
||||
- `autoload/controls/class.Tickets.php` — Day ranges `0`, `2`, `7` for dynamic pricing; product ID `999999` for delivery
|
||||
- Fix: Define as named constants (e.g., `const DELIVERY_PRODUCT_ID = 999999`)
|
||||
|
||||
**Commented Debug Code:**
|
||||
- `autoload/controls/class.Tickets.php` — `// file_put_contents('sandbox_przelewy24_response.txt', ...)` left in code
|
||||
- Fix: Remove
|
||||
|
||||
**Short PHP Tags in Templates:**
|
||||
- `templates/` — `<? if ...` (deprecated, requires `short_open_tag = On`)
|
||||
- Fix: Replace with `<?php` in templates over time
|
||||
|
||||
**MD5 Session CSRF Token:**
|
||||
- `index.php` — `$_SESSION['check'] = md5(...)` used as CSRF-adjacent token
|
||||
- MD5 for security tokens is considered weak
|
||||
- Fix: Use `bin2hex(random_bytes(32))` for session tokens
|
||||
|
||||
**No README / Setup Guide:**
|
||||
- No documentation on how to set up dev environment or database
|
||||
- No `.env.example` or credential template
|
||||
- Fix: Add `README.md` with setup steps and required config values
|
||||
|
||||
---
|
||||
|
||||
*Concerns analysis: 2026-04-26*
|
||||
*Priority order: Critical → High → Medium → Low*
|
||||
*Address Critical items before any new feature development*
|
||||
Reference in New Issue
Block a user