test: Deep dive audit — verify and fix all findings #81

New Issue

2026-06-21T23:04:13Z

jmiller commented

2026-06-21 23:04:13 +00:00

Summary

Master tracking issue for all bugs, security issues, and code quality findings from the full codebase audit (2026-06-21). Each sub-issue has a fix described — verify the fix works, then close.

Critical (must fix before release)

#71 — bug: CLI RestoreCommand passes wrong arguments to RestoreEngine (completely broken)
#72 — security: Path traversal in JpaUnarchiver — no filename sanitization
#73 — bug: S3Uploader loads entire file into RAM via file_get_contents — OOM
#74 — bug: DatabaseDumper loads entire SQL dump into RAM — OOM on large sites

High (fix before next minor release)

#75 — security: AkeebaImporter uses unserialize() on untrusted filter data
#76 — bug: BackupTable deletes archive file before confirming DB row deletion
#77 — security: RestoreEngine staging path uses unsanitized $record->tag
#78 — security: API profiles endpoint leaks FTP/S3/GDrive credentials
#79 — bug: Webcron handler missing return after sendJsonResponse on auth failure
#80 — bug: BackupModel/ProfileModel loadFormData() returns array when object declared — TypeError

Medium (not yet tracked as issues — fix opportunistically)

BackupEngine.php:324 — $archiveName may be undefined in catch block
SteppedSession.php:68 — session directory 0755, encryption password readable by co-tenants
SteppedBackupEngine.php:397 — $session->profileId not cast to int after JSON deserialization
DatabaseDumper.php:142 — no ORDER BY in chunked dump, duplicates/gaps during concurrent writes
SteppedBackupEngine.php:478 — backupstart empty string in notifications from stepped engine
BackupEngine.php:547 — sanitizeConfiguration() regex misses values with embedded newlines
BackupEngine.php:207 — plaintext archive left on disk when AES encryption fails
BackupsModel.php:74 — sort column uses escape() instead of quoteName()
TarGzArchiver.php:51 — intermediate .tar not cleaned up if compress() throws
CleanupCommand.php:79 — dry-run double-counts records in max-count output

Low (cosmetic / defense-in-depth)

api/BackupsController.php:124 — profiles() leaks credentials (covered by #78)
SteppedSession.php:113 — session properties not type-validated on load
BackupEngine.php:468 — setPassword() is a no-op for ZIP write (misleading)

Testing Strategy

Fix each critical/high issue on its own branch
PR review each fix
Test on waas.dev (see testing reference)
Check the box and close the sub-issue when verified
Close this master issue when all critical + high are resolved

## Summary Master tracking issue for all bugs, security issues, and code quality findings from the full codebase audit (2026-06-21). Each sub-issue has a fix described — verify the fix works, then close. --- ## Critical (must fix before release) - [ ] #71 — **bug:** CLI RestoreCommand passes wrong arguments to RestoreEngine (completely broken) - [ ] #72 — **security:** Path traversal in JpaUnarchiver — no filename sanitization - [ ] #73 — **bug:** S3Uploader loads entire file into RAM via file_get_contents — OOM - [ ] #74 — **bug:** DatabaseDumper loads entire SQL dump into RAM — OOM on large sites ## High (fix before next minor release) - [ ] #75 — **security:** AkeebaImporter uses unserialize() on untrusted filter data - [ ] #76 — **bug:** BackupTable deletes archive file before confirming DB row deletion - [ ] #77 — **security:** RestoreEngine staging path uses unsanitized $record->tag - [ ] #78 — **security:** API profiles endpoint leaks FTP/S3/GDrive credentials - [ ] #79 — **bug:** Webcron handler missing return after sendJsonResponse on auth failure - [ ] #80 — **bug:** BackupModel/ProfileModel loadFormData() returns array when object declared — TypeError ## Medium (not yet tracked as issues — fix opportunistically) - `BackupEngine.php:324` — `$archiveName` may be undefined in catch block - `SteppedSession.php:68` — session directory 0755, encryption password readable by co-tenants - `SteppedBackupEngine.php:397` — `$session->profileId` not cast to int after JSON deserialization - `DatabaseDumper.php:142` — no ORDER BY in chunked dump, duplicates/gaps during concurrent writes - `SteppedBackupEngine.php:478` — `backupstart` empty string in notifications from stepped engine - `BackupEngine.php:547` — sanitizeConfiguration() regex misses values with embedded newlines - `BackupEngine.php:207` — plaintext archive left on disk when AES encryption fails - `BackupsModel.php:74` — sort column uses escape() instead of quoteName() - `TarGzArchiver.php:51` — intermediate .tar not cleaned up if compress() throws - `CleanupCommand.php:79` — dry-run double-counts records in max-count output ## Low (cosmetic / defense-in-depth) - `api/BackupsController.php:124` — profiles() leaks credentials (covered by #78) - `SteppedSession.php:113` — session properties not type-validated on load - `BackupEngine.php:468` — setPassword() is a no-op for ZIP write (misleading) --- ## Testing Strategy 1. Fix each critical/high issue on its own branch 2. PR review each fix 3. Test on waas.dev (see [testing reference](reference_mokodev_testing.md)) 4. Check the box and close the sub-issue when verified 5. Close this master issue when all critical + high are resolved

jmiller added the component: engine component: admin labels 2026-06-21 23:04:13 +00:00

jmiller commented

2026-06-21 23:05:02 +00:00

Additional findings: PreflightCheck itself has issues

The silent failure hunter reviewed PR #70's PreflightCheck and found issues with the preflight system itself:

Critical

PreflightCheck::run() and all sub-methods have unguarded database queries — if the DB is down, the preflight throws an uncaught exception that cascades into BackupEngine (where $recordId is undefined), crashing worse than having no preflight at all
Fix: wrap run() body in try-catch, convert DB exceptions to errors

High

@mkdir() at line 120 suppresses the actual failure reason (permissions? open_basedir? quota?)
disk_free_space() calculation uses (int) cast — integer overflow on 32-bit PHP with >1.7GB backups
Dead checkRequiredExtensions() still in BackupEngine + silent ZIP fallback in createArchiver() default case

Medium

JS warnings overwritten: preflight warnings set on mb-phase element, then immediately overwritten by updateProgress() — warnings are invisible to users
Silent return on unresolved placeholders — no warning when [HOME] can't be resolved
Missing remote credentials are warnings not errors — allows 30-min backup to run then fail at upload

These should be fixed before the preflight feature is considered production-ready.

## Additional findings: PreflightCheck itself has issues The silent failure hunter reviewed PR #70's PreflightCheck and found issues with the preflight system itself: ### Critical - `PreflightCheck::run()` and all sub-methods have **unguarded database queries** — if the DB is down, the preflight throws an uncaught exception that cascades into BackupEngine (where `$recordId` is undefined), crashing worse than having no preflight at all - Fix: wrap `run()` body in try-catch, convert DB exceptions to errors ### High - `@mkdir()` at line 120 suppresses the actual failure reason (permissions? open_basedir? quota?) - `disk_free_space()` calculation uses `(int)` cast — integer overflow on 32-bit PHP with >1.7GB backups - Dead `checkRequiredExtensions()` still in BackupEngine + silent ZIP fallback in `createArchiver()` default case ### Medium - **JS warnings overwritten**: preflight warnings set on `mb-phase` element, then immediately overwritten by `updateProgress()` — warnings are invisible to users - Silent return on unresolved placeholders — no warning when `[HOME]` can't be resolved - Missing remote credentials are warnings not errors — allows 30-min backup to run then fail at upload These should be fixed before the preflight feature is considered production-ready.

jmiller referenced this issue from a commit

2026-06-21 23:09:43 +00:00

fix: critical and high audit findings (#81)

jmiller referenced this issue

2026-06-21 23:09:56 +00:00

fix: Critical and high audit findings (#81) #83

jmiller referenced this issue from a commit

2026-06-21 23:10:24 +00:00

Merge pull request 'fix: Critical and high audit findings (#81)' (#83) from fix/audit-critical-high into main

jmiller referenced this issue from a commit

2026-06-21 23:17:16 +00:00

fix: remaining audit findings — OOM, security, error handling (#81)