amlhc/.planning/codebase/CONCERNS.md

# Codebase Concerns

**Analysis Date:** 2026-04-21

---

## Security

### S1. Password Hashing Uses Weak Double-MD5 (HIGH)

**Files:**
- `application/common/library/Auth.php` line 490-492
- `application/admin/library/Auth.php` line 146-148

**Issue:** Both frontend and admin password hashing use double-MD5 with salt:
```php
// application/common/library/Auth.php:490-492
public function getEncryptPassword($password, $salt = '')
{
    return md5(md5($password) . $salt);
}
```
MD5 is cryptographically broken. The inner hash is unsalted, making rainbow table attacks viable. MD5 can be brute-forced at billions of hashes per second on commodity hardware. The salt is only 6 characters (`Random::alnum()` typically produces short strings).

**Impact:** If the database is compromised, all user passwords can be cracked rapidly.

**Fix approach:** Migrate to `password_hash()` with `PASSWORD_BCRYPT` or `PASSWORD_ARGON2ID`. Add a migration script to re-hash passwords on next successful login.

---

### S2. SQL Injection via Raw Queries in Admin Controllers (HIGH)

**Files:**
- `application/admin/command/Crud.php` lines 440, 444, 462, 466
- `application/admin/controller/general/Config.php` line 293
- `application/admin/controller/Dashboard.php` line 47
- `application/admin/controller/Command.php` line 39
- `extend/fast/Auth.php` line 160

**Issue:** Multiple raw SQL queries interpolate variables without parameterization:
```php
// application/admin/command/Crud.php:440
$modelTableInfo = $dbconnect->query("SHOW TABLE STATUS LIKE '{$modelTableName}'", [], true);

// application/admin/controller/general/Config.php:293
$tableList = \think\Db::query("SELECT `TABLE_NAME` AS `name`,`TABLE_COMMENT` AS `title` FROM `information_schema`.`TABLES` where `TABLE_SCHEMA` = '{$dbname}';");

// extend/fast/Auth.php:160
->where("aga.uid='{$uid}' and ag.status='normal'")
```
The `Crud.php` command receives table names via CLI arguments (`--table=`) which could be manipulated. The `Auth.php` interpolates `$uid` directly into the WHERE clause.

**Impact:** An attacker with admin access could inject malicious SQL through crafted table names in CRUD generation. The Auth SQL injection is more concerning if `$uid` can ever be user-controlled.

**Fix approach:** Use parameterized queries. Validate table names against `/^[a-z0-9_]+$/i` before embedding in SQL. Replace string interpolation in Auth.php with `->where('aga.uid', $uid)->where('ag.status', 'normal')`.

---

### S3. HTTP Method Spoofing Enabled (MEDIUM)

**Files:**
- `application/config.php` line 109

**Issue:** The config `'var_method' => '_method'` allows clients to spoof HTTP methods via POST parameter. An attacker can send `_method=DELETE` in a POST request to bypass CSRF protections that only check POST requests.

**Impact:** CSRF bypass via method override on state-changing operations.

**Fix approach:** Set `'var_method' => ''` to disable method spoofing, or ensure all state-changing operations require explicit CSRF token validation regardless of HTTP method.

---

### S4. Cookie Security Flags Not Set (MEDIUM)

**Files:**
- `application/config.php` lines 216-231

**Issue:** Session cookies lack security flags:
```php
'cookie' => [
    'secure'   => false,  // Not enforced
    'httponly' => '',     // Empty string = disabled
],
```

**Impact:** Session cookies vulnerable to interception on HTTP connections and theft via XSS attacks.

**Fix approach:** Set `'secure' => true` (when HTTPS is available) and `'httponly' => true`.

---

### S5. Token Key Hardcoded in Config (MEDIUM)

**Files:**
- `application/config.php` line 264

**Issue:** The token encryption key is hardcoded directly in the config file:
```php
'token' => [
    'key' => '3byNV4KupeZAvl60sdr2COjDYUmqwPJW',  // line 264
],
```
This value is committed to git and visible to anyone with repository access. If leaked, an attacker can forge valid tokens.

**Impact:** Token forgery if the key is exposed.

**Fix approach:** Move to environment variable via `Env::get('token.key')` and regenerate the key.

---

### S6. External API Scraping Without Data Validation (HIGH)

**Files:**
- `application/index/controller/Index.php` lines 20-58

**Issue:** The `get_history()` method fetches lottery data from `https://history.macaumarksix.com` and writes directly to the database with zero validation:
```php
public function get_history()
{
    $client = new \GuzzleHttp\Client();
    $res = $client->request('GET', 'https://history.macaumarksix.com/history/macaujc2/y/2026');
    // ...
    foreach ($data as $item) {
        $insert_data['expect'] = $item['expect'];
        $insert_data['num1'] = $openCode[0];
        // ... direct DB insert without any validation
        Db::name('history')->insert($insert_data);
    }
}
```
No validation of: response data structure, value ranges (lottery numbers should be 1-49), data types, or expected format of `openCode`. The year `2026` is hardcoded.

**Impact:** If the external API is compromised or returns malformed data, the database gets corrupted with invalid lottery records. The hardcoded year requires annual manual updates.

**Fix approach:** Add data validation (type checks, range validation, schema verification), make the year parameter configurable, and add error handling for API structure changes.

---

### S7. External Scraping Has No Reliability Safeguards (MEDIUM)

**Files:**
- `application/index/controller/Index.php`

**Issue:** No retry logic, timeout configuration, or circuit breaker for the external API call. No rate limiting on the `get_history` endpoint - any user can trigger the scrape repeatedly. No Guzzle timeout is configured.

**Impact:** External API downtime causes unhandled exceptions. Repeated calls could trigger rate limiting or IP bans on the source API.

**Fix approach:** Add Guzzle timeout config, implement retry logic with exponential backoff, add rate limiting to the endpoint, and cache results.

---

### S8. Login CAPTCHA Disabled by Default (MEDIUM)

**Files:**
- `application/config.php` line 277

**Issue:** `'login_captcha' => false` disables login CAPTCHA.

**Impact:** Without login CAPTCHA, the application is vulnerable to brute-force password attacks and credential stuffing. The 10-attempts-per-day lockout is the only defense.

**Fix approach:** Set `'login_captcha' => true` in production.

---

### S9. Upload File Type Check Relies on Client-Supplied MIME (LOW-MEDIUM)

**Files:**
- `application/common/library/Upload.php` lines 87-98, 106-120

**Issue:** The upload check uses `$this->fileInfo['type']` which comes from the client's `Content-Type` header. PHP's `$_FILES['type']` is set by the browser and can be easily spoofed.

**Impact:** An attacker could potentially upload files with disguised MIME types if the suffix check has gaps.

**Fix approach:** Use `finfo_open()` / `finfo_file()` to detect actual file type from content bytes.

---

### S10. Suspicious PHP File in Public Directory (HIGH)

**Files:**
- `public/ByZjtVrKok.php` (1250 bytes, untracked in git)

**Issue:** A PHP file with a random-looking name exists in the public web root. Any PHP file in `public/` is directly executable via HTTP request.

**Impact:** This could be an unauthorized backdoor, test file, or admin script. If it contains administrative functionality, anyone who discovers the URL could execute it.

**Fix approach:** Audit the file contents immediately. If not needed, delete it. If it serves a purpose, move it outside the public directory.

---

### S11. No CSRF Protection on API Endpoints (MEDIUM)

**Files:**
- `application/api/controller/User.php`
- `application/api/controller/Sms.php`
- `application/api/controller/Ems.php`

**Issue:** API controller methods do not use CSRF token validation. While APIs typically use token-based auth, browser-initiated API calls from authenticated sessions are vulnerable to CSRF.

**Impact:** An authenticated user visiting a malicious page could have API calls triggered on their behalf (e.g., sending SMS codes, changing account settings).

**Fix approach:** For browser-initiated API calls that modify state, require CSRF token validation. For pure API usage, ensure token-based auth is mandatory.

---

### S12. 0777 File Permissions (MEDIUM)

**Files:**
- `application/common.php` line 121
- `application/admin/command/Addon.php` line 271

**Issue:** `@chmod($file, 0777)` sets world-writable permissions on created files.

**Impact:** On shared hosting, any user/process can modify these files, potentially injecting malicious code.

**Fix approach:** Use `0755` for directories and `0644` for files.

---

### S13. Deprecated mcrypt Fallback (LOW)

**Files:**
- `application/common/library/Security.php` lines 438-439

**Issue:** References `mcrypt_create_iv()` and `MCRYPT_DEV_URANDOM` — the mcrypt extension was removed in PHP 7.2. The project requires PHP >= 7.4.

**Impact:** Dead code that will never execute but creates confusion and maintenance debt.

**Fix approach:** Remove the mcrypt fallback. `random_bytes()` is sufficient for PHP 7.4+.

---

### S14. exec() Calls with User-Influenced Input (MEDIUM)

**Files:**
- `application/admin/command/Crud.php` lines 580, 1042, 1232

**Issue:** Shell commands are constructed with interpolated variables:
```php
exec("php think menu -c {$controllerUrl} -d 1 -f 1");
exec("php think crud -t {$relation['relationTableName']} ...");
```

**Impact:** If input variables can be controlled by non-admin users, command injection is possible.

**Fix approach:** Use `escapeshellarg()` on all variables interpolated into shell commands.

---

## Maintainability

### M1. Massive CRUD Command File (1795 lines)

**Files:**
- `application/admin/command/Crud.php` (1795 lines)

**Issue:** This single file handles table analysis, code generation for models, controllers, views, validation, language packs, menu generation, and relation handling. It violates the Single Responsibility Principle.

**Impact:** Difficult to maintain, test, and extend.

**Fix approach:** Split into separate classes: TableAnalyzer, ModelGenerator, ControllerGenerator, ViewGenerator, MenuGenerator.

---

### M2. Duplicated Code Across Base Controllers

**Files:**
- `application/common/controller/Api.php` lines 318-329, 153-160
- `application/common/controller/Backend.php` lines 600-613, 237-244
- `application/common/controller/Frontend.php` lines 149-161, 128-135

**Issue:** The `token()` and `loadlang()` methods are identically duplicated across all three base controllers.

**Impact:** Changes must be applied in three places.

**Fix approach:** Move to a shared trait.

---

### M3. Tightly Coupled Backend Traits

**Files:**
- `application/admin/library/traits/Backend.php` (481 lines)
- `application/common/controller/Backend.php` (614 lines)

**Issue:** The Backend trait injects massive functionality (index, add, edit, del, recyclebin, restore, destroy, multi, import, selectpage) into every admin controller. The trait and base controller are deeply intertwined.

**Impact:** All admin controllers carry full CRUD weight. Customizing one method requires overriding the entire trait method.

**Fix approach:** Split into smaller, focused traits (CrudTrait, ImportTrait, RecycleBinTrait).

---

### M4. No Service Layer

**Files:** All controllers

**Issue:** Business logic is embedded directly in controllers. `application/index/controller/Index.php` handles external API fetching, data parsing, and database insertion all in one method.

**Impact:** Controllers are difficult to test. Logic cannot be reused across entry points.

**Fix approach:** Introduce a service layer (e.g., `LotteryDataService`, `UserAuthService`).

---

### M5. composer.lock Ignored in Version Control

**Files:**
- `.gitignore` line 11

**Issue:** `composer.lock` is gitignored, meaning dependency versions are not pinned.

**Impact:** Different environments install different dependency versions, leading to inconsistent behavior and potential security vulnerabilities.

**Fix approach:** Remove `composer.lock` from `.gitignore` and commit it.

---

## Performance

### P1. N+1 Query Pattern in Data Scraping (HIGH)

**Files:**
- `application/index/controller/Index.php` lines 28-45

**Issue:** For each item from the external API, a separate SELECT + INSERT/UPDATE is executed:
```php
foreach ($data as $item) {
    $exist = Db::name('history')->where('expect', $item['expect'])->find();
    if (!$exist) {
        Db::name('history')->insert($insert_data);
    } else {
        Db::name('history')->where('expect', $item['expect'])->update($insert_data);
    }
}
```
This generates 2N queries for N records.

**Impact:** For large datasets, significant database load and slow execution.

**Fix approach:** Use `INSERT ... ON DUPLICATE KEY UPDATE` for single-query upsert, or batch operations.

---

### P2. File-Based Cache Only

**Files:**
- `application/config.php` lines 187-197

**Issue:** Only file-based caching configured. Token storage defaults to MySQL.

**Impact:** File I/O is slower than memory-based caching, especially under concurrent access. Token validation on every request adds database load.

**Fix approach:** Configure Redis for caching and token storage in production.

---

### P3. Dashboard Runs Heavy Queries Without Caching

**Files:**
- `application/admin/controller/Dashboard.php` lines 24-82

**Issue:** The dashboard executes ~10+ aggregate queries on every page load with no caching:
```php
$totaluser = User::count();
$todayusersignup = User::whereTime('jointime', 'today')->count();
$sevendau = User::whereTime('jointime|logintime|prevtime', '-7 days')->count();
$dbTableList = Db::query("SHOW TABLE STATUS");
```

**Impact:** Dashboard becomes slower as user base grows.

**Fix approach:** Cache dashboard statistics with a short TTL (e.g., 5 minutes).

---

### P4. No Frontend Asset Build Pipeline

**Files:**
- `Gruntfile.js`
- `public/assets/js/backend/command.js`
- `public/assets/js/backend/history.js`

**Issue:** While a Gruntfile.js exists, there is no evidence of automated build pipeline. JavaScript files are loaded individually per controller.

**Impact:** Slow page load times due to multiple HTTP requests.

**Fix approach:** Implement asset bundling and minification in the deployment pipeline.

---

## Reliability

### R1. Silent Exception Swallowing in Financial Operations (HIGH)

**Files:**
- `application/common/model/User.php` lines 93-111, 119-137

**Issue:** The `money()` and `score()` methods catch exceptions but only roll back without logging or returning error:
```php
public static function money($money, $user_id, $memo)
{
    Db::startTrans();
    try {
        // ... money operation
        Db::commit();
    } catch (\Exception $e) {
        Db::rollback();
        // No logging, no return value - silent failure
    }
}
```

**Impact:** Failed money/score operations silently fail. Financial data integrity is at risk.

**Fix approach:** Log the exception, throw or return error indicator. Add audit trail for financial operations.

---

### R2. Empty Catch Block in Dashboard

**Files:**
- `application/admin/controller/Dashboard.php` lines 26-30

**Issue:** Exception caught and completely ignored:
```php
try {
    \think\Db::execute("SET @@sql_mode='';");
} catch (\Exception $e) {
    // Empty
}
```

**Fix approach:** Log the exception at minimum.

---

### R3. No Structured Logging

**Files:**
- `application/config.php` lines 168-177

**Issue:** Logging configured minimally with empty level array. No log rotation, no error tracking service.

**Impact:** Difficult to debug production issues. Log files can grow unbounded.

**Fix approach:** Configure log levels, add log rotation, integrate with error tracking (e.g., Sentry).

---

### R4. No Validation on History Model

**Files:**
- `application/admin/controller/History.php`
- `application/admin/model/History.php`
- `application/admin/validate/History.php` (exists but not referenced)

**Issue:** The History controller has no custom validation. The model has no validation rules. A validate file exists (`application/admin/validate/History.php`) but is not referenced in the controller.

**Impact:** Malformed lottery data can be stored through the admin panel.

**Fix approach:** Enable model validation in the controller and define rules for lottery data fields.

---

## Architecture

### A1. ThinkPHP 5.x is Outdated

**Files:**
- `composer.json` line 19
- `thinkphp/` directory

**Issue:** Uses `topthink/framework: dev-master` (ThinkPHP 5.x fork). ThinkPHP 5 is EOL; current stable is 8.x. The dev-master branch from Gitee is a maintained fork but diverges from the official framework.

**Impact:** No official security patches. Dependency compatibility issues.

**Fix approach:** Monitor the FastAdmin fork for security updates. Plan migration to ThinkPHP 6+ when feasible.

---

### A2. Dependency Version Risks

**Files:**
- `composer.json`

**Issue:** Unstable and outdated dependencies:
```json
"topthink/framework": "dev-master",       // Unstable branch
"topthink/think-queue": "1.1.6",          // Very old
"topthink/think-captcha": "^1.0.9",       // TP5-era
```

**Impact:** `dev-master` can introduce breaking changes. Old versions may contain known vulnerabilities.

**Fix approach:** Pin stable versions. Run `composer audit` regularly.

---

### A3. Framework Code Committed to Repository

**Files:**
- `thinkphp/` directory
- `.gitignore` line 2

**Issue:** The entire ThinkPHP framework source is committed to the repository instead of being managed purely through Composer.

**Impact:** Repository bloat. Difficulty tracking framework upgrades.

**Fix approach:** Remove `thinkphp/` from the repository and manage solely through Composer.

---

### A4. No API Versioning

**Files:**
- `application/api/controller/` (all files)

**Issue:** API endpoints have no version prefix. Any breaking change affects all clients immediately.

**Impact:** Inability to make breaking API changes without disrupting existing clients.

**Fix approach:** Add API versioning (e.g., `/api/v1/`) using ThinkPHP routes.

---

## Domain-Specific

### D1. Lottery Data Accuracy Not Guaranteed (HIGH)

**Files:**
- `application/index/controller/Index.php` lines 20-58

**Issue:** Lottery data sourced from a single third-party API with no verification, backup source, or data integrity checks. Raw `openCode` string is split by comma and mapped directly without range validation.

**Impact:** If the external source provides incorrect data, the system propagates errors silently.

**Fix approach:** Add range validation (numbers 1-49), implement backup data sources, and maintain an audit log.

---

### D2. Data Validation Gaps for Lottery Records

**Files:**
- `sql/macaujc_history.sql`
- `application/admin/model/History.php`
- `application/admin/validate/History.php`

**Issue:** The History model has no validation rules. The SQL schema has loose VARCHAR constraints without format validation.

**Impact:** Invalid lottery records can be stored (negative numbers, out-of-range values, malformed dates).

**Fix approach:** Add validation rules to the History model. Implement data normalization accessors.

---

### D3. Hardcoded Year in Scraping Endpoint

**Files:**
- `application/index/controller/Index.php` line 23

**Issue:** The API URL hardcodes the year `2026`:
```php
$res = $client->request('GET', 'https://history.macaumarksix.com/history/macaujc2/y/2026');
```

**Impact:** Requires annual manual update. After January 1st of a new year, new data won't be fetched.

**Fix approach:** Make the year dynamic with `date('Y')` or accept it as a parameter.

---

## Test Coverage Gaps

### T1. Zero Automated Tests

**Files:** Entire application codebase

**Issue:** No test files exist anywhere in the application directory. No `phpunit.xml` configuration.

**Risk:** High

**Impact:** No automated regression testing. Code changes carry high risk of introducing bugs. Cannot safely refactor.

**Fix approach:** Set up PHPUnit. Start with unit tests for Auth library, then API endpoints, then lottery data handling.

---

## Additional Concerns

### AC1. No .env.example Template

**Files:**
- `.gitignore` line 15
- `.env` (present)

**Issue:** `.env` is gitignored but there is no `.env.example` template.

**Impact:** New developers cannot set up the environment without guessing variable names.

**Fix approach:** Create `.env.example` with all required variables documented.

---

### AC2. Debug Mode Default Risk

**Files:**
- `application/config.php` line 21

**Issue:** `'app_debug' => Env::get('app.debug', false)` defaults to `false`, but if `.env` is misconfigured in production with `debug = true`, full stack traces are exposed.

**Impact:** Detailed error messages with file paths and SQL queries exposed to end users.

**Fix approach:** Explicitly set `app_debug=false` in production config. Never rely on defaults.

---

### AC3. Install Script Redirect Logic Still Present

**Files:**
- `public/index.php` lines 16-19

**Issue:** The entry point checks for `install.lock` and redirects to `./install.php` if missing. Although `public/install.php` has been deleted, the redirect logic remains.

**Impact:** If someone restores `install.php`, the installation wizard becomes publicly accessible.

**Fix approach:** Remove the redirect logic from `public/index.php` after installation is confirmed complete.

---

*Concerns audit: 2026-04-21*