generated from jric11/baseProject
123 lines
7.0 KiB
Markdown
123 lines
7.0 KiB
Markdown
# Production readiness review — October 20, 2026
|
||
|
||
Follow-up: the three code findings below are addressed in
|
||
[maintenance and backup repairs](MAINTENANCE-REPAIRS.md). The findings describe
|
||
the state at review time; production-host acceptance work remains applicable.
|
||
|
||
The production-safeguards migration is applied to development. Reset invalidation,
|
||
verified-email access, financial corrections/reversals, and transactional audit
|
||
records are already implemented. Their earlier findings are not being reopened.
|
||
|
||
There are three remaining code fixes and production-host acceptance work. This
|
||
review changed no application code or development records. It used a disposable
|
||
database for focused validation checks, then removed it and stopped its server.
|
||
It did not rerun the complete regression suite or contact the future production
|
||
hostname. The current Composer audit reported no advisories or abandoned packages.
|
||
|
||
## Code fixes, in recommended order
|
||
|
||
### 1. Update the separate CKEditor dependency
|
||
|
||
`js/ckeditor.js:5` contains the version constant `24.0.0`. This editor is loaded by
|
||
the alert and popover forms. It was not replaced by the jQuery update and is not
|
||
included in browser-dependencies.json or its integrity checks.
|
||
|
||
The upstream advisory GHSA-3rh3-wfr4-76mj affects CKEditor packages through 26.0.0,
|
||
including engine, font, and list packages present in this build. It describes a
|
||
low-severity regular-expression denial of service that can freeze a browser tab.
|
||
This review did not demonstrate an application exploit or establish that every
|
||
newer advisory applies to this build.
|
||
|
||
Fix: rebuild/replace the editor with an appropriately licensed, patched version
|
||
or use a simpler maintained editor. Record all bundled browser libraries and
|
||
their provenance, extend advisory/integrity checks, and test paste, save, edit,
|
||
formatting, and CSP behavior. Passing editor-initialization tests alone does not
|
||
establish security coverage.
|
||
|
||
Primary source:
|
||
https://github.com/ckeditor/ckeditor5/security/advisories/GHSA-3rh3-wfr4-76mj
|
||
|
||
### 2. Finish server validation for older maintenance forms
|
||
|
||
`classes/Alerts.php:70–80` accepts title/message fields directly and converts
|
||
dates with strtotime without validating the ordering. The edit path at lines
|
||
163–174 repeats this pattern. Browser JavaScript checks some of these values,
|
||
but a direct authenticated request bypasses those checks.
|
||
|
||
Confirmed in a disposable database: posting a blank alert title and an expiration
|
||
before its start returned HTTP 200 and persisted both invalid values.
|
||
|
||
`classes/Popovers.php:52–56` and its edit path also lack equivalent server-side
|
||
required-field validation. Posting a blank popover name/title returned HTTP 200
|
||
and stored the blank name.
|
||
|
||
Fix: centralize required fields, length limits, strict date/time parsing and date
|
||
ordering for create/edit operations. Return field-specific validation errors and
|
||
test direct HTTP submissions. These routes require an administrator; this is a
|
||
data-integrity gap, not a demonstrated unauthenticated write vulnerability.
|
||
|
||
### 3. Make backup completion checking bounded in memory
|
||
|
||
`bin/backup-database.php:23` uses file_get_contents on the entire SQL dump merely
|
||
to locate the completion marker. Memory use grows with the database backup size.
|
||
Under a configured memory limit, a valid large dump can therefore cause the
|
||
backup job to fail during validation. Current tests exercise a small database.
|
||
|
||
Fix: inspect a bounded tail of the dump or stream it, retaining the existing
|
||
exit-code and checksum checks. Test large-file behavior and failed/interrupted
|
||
jobs, including temporary credential/partial-file cleanup and scheduler alerts.
|
||
The resource concern was identified from source; a large-file failure was not
|
||
reproduced during this review.
|
||
|
||
## Production acceptance work still required
|
||
|
||
These are checks on the selected production environment, not evidence of a public
|
||
leak or an instruction to change the approved development connection.
|
||
|
||
1. **Complete and preserve the release.** Essential new classes, migrations,
|
||
browser assets, deployment files, and the CI workflow are still untracked in
|
||
the working tree. Include them in the reviewed source-control release; a
|
||
GitHub checkout cannot run files that have not been committed. Run CI on the
|
||
actual repository and retain a versioned release and recovery plan.
|
||
2. **Validate the actual hosting configuration.** Configure production secrets,
|
||
HTTPS/proxy trust, secure cookies/headers, protected sessions/logs, and private
|
||
directory denial. Run the production check script against the real HTTPS
|
||
deployment when it exists. Apache/IIS/Nginx examples have not been exercised
|
||
on the selected production host.
|
||
3. **Verify production database transport and privileges.** Use verified TLS for
|
||
a remote production database and separate runtime/migration/backup roles.
|
||
Run complete workflows using the actual restricted runtime account, rather
|
||
than validating only connectivity or individual denied operations. The
|
||
development non-TLS approval does not establish this production configuration.
|
||
4. **Test real email delivery.** The most recent development acceptance run
|
||
captured links locally. Send actual verification, invitation, and reset emails
|
||
from production, confirm inbox delivery and links, and configure sender-domain
|
||
authentication and delivery-failure monitoring. Generated URLs and a prior
|
||
SMTP acceptance result do not prove the final inbox experience.
|
||
5. **Install operational jobs and alerts.** Backup/cleanup/health entry points
|
||
exist, but the repository does not prove their production schedules or alert
|
||
routing. Configure protected encrypted off-host backups, retention, restore
|
||
drills, log rotation, and monitoring for failed jobs, audit writes, SMTP,
|
||
database health, and disk space. Verify any existing external setup rather
|
||
than assuming it is absent.
|
||
6. **Test realistic volumes.** Roster, dues, history, and export code loads whole
|
||
collections. Test the expected club sizes and concurrent use. Introduce paging,
|
||
streaming, or background exports where measured limits require them; small
|
||
workflow tests do not establish large-club performance.
|
||
|
||
## Recommendations and product scope
|
||
|
||
- MFA and recent-password confirmation for sensitive administrator actions remain
|
||
recommended protections. They are not implemented, and this review does not
|
||
represent them as newly discovered authorization bypasses.
|
||
- Global administrators still manage all clubs. Delegated club administrators
|
||
are needed only if clubs will administer themselves.
|
||
- Active members still see fellow club members' email addresses. Confirm that
|
||
directory policy before onboarding real members.
|
||
- Online payment collection/refunds are outside the current manual-ledger scope;
|
||
their absence is not a defect in recording received payments and reversals.
|
||
|
||
Recommended next step: address the three code items, then complete one documented
|
||
production acceptance run. Additional features should be driven by the intended
|
||
operating model rather than treated as automatic launch blockers.
|