fix: review fixes — infinite recursion, SQL injection, FK prefix, DB importer #49

2026-06-18T17:09:39Z

jmiller commented

2026-06-18 17:09:39 +00:00

Summary

Post-merge review fixes from PR #46.

Critical Fixes

Fix infinite recursion in getValidatedPrefix() — was calling itself instead of reading from $data
Fix SQL injection in actionResetAdmin() — prefix not validated, now uses getValidatedPrefix()
Fix FK REFERENCES not abstracted in prefix replacement — str_replace now targets backtick+prefix pattern

Error Handling

Wrap cleanupOldBackups() in try-catch (prevents admin panel crash on DB errors)
Failure notification record includes all required fields (total_size, files_count, etc.)
deleteBackupRecord() logs unlink failures, wraps DB delete in try-catch
runPreActionBackup catches \\Throwable instead of \\Exception
sendNtfy() checks for ext-curl before curl_init()
Encryption warns on skipped files
Truncated ntfy response in error logs
Security gate file write check

Other Fixes

DatabaseImporter: apply #__ prefix replacement on trailing SQL statement
SteppedBackupEngine: remove unused DatabaseDumper instantiation, fix indentation
Stepped notification catch \\Throwable instead of \\Exception

Test plan

MokoRestore: DB import works with different prefix
MokoRestore: security gate displays and verifies code
Cleanup: runs without crashing admin panel
Notifications: failure notifications include all fields

## Summary Post-merge review fixes from PR #46. ### Critical Fixes - Fix infinite recursion in `getValidatedPrefix()` — was calling itself instead of reading from `$data` - Fix SQL injection in `actionResetAdmin()` — prefix not validated, now uses `getValidatedPrefix()` - Fix FK REFERENCES not abstracted in prefix replacement — `str_replace` now targets backtick+prefix pattern ### Error Handling - Wrap `cleanupOldBackups()` in try-catch (prevents admin panel crash on DB errors) - Failure notification record includes all required fields (total_size, files_count, etc.) - `deleteBackupRecord()` logs unlink failures, wraps DB delete in try-catch - `runPreActionBackup` catches `\\Throwable` instead of `\\Exception` - `sendNtfy()` checks for ext-curl before `curl_init()` - Encryption warns on skipped files - Truncated ntfy response in error logs - Security gate file write check ### Other Fixes - `DatabaseImporter`: apply `#__` prefix replacement on trailing SQL statement - `SteppedBackupEngine`: remove unused `DatabaseDumper` instantiation, fix indentation - Stepped notification catch `\\Throwable` instead of `\\Exception` ## Test plan - [ ] MokoRestore: DB import works with different prefix - [ ] MokoRestore: security gate displays and verifies code - [ ] Cleanup: runs without crashing admin panel - [ ] Notifications: failure notifications include all fields

jmiller closed this pull request

2026-06-18 17:13:08 +00:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

Priority -

Type -

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: MokoConsulting/MokoSuiteBackup#49