huntingclubroster/PRODUCTION-REVIEW.md

# Production readiness review — October 18, 2026

Follow-up implementation: see [PRODUCTION-FIXES.md](PRODUCTION-FIXES.md) for the
October 19 code changes, new migration, tests, and remaining host acceptance work.
The findings below describe the reviewed version before that implementation.

Recommendation: hold production launch until the security defects and financial
recordkeeping gaps below are resolved and the production deployment checks pass.
The recent successful workflow tests remain useful, but do not cover these gaps.

This review inspected the current source, schema, server rules, configuration,
dependencies, and deployment documentation. Composer's locked dependency audit
returned no published advisories or abandoned packages. One security defect was
reproduced using a disposable local database; that database was removed and its
server stopped. No development database records or application code were changed
during this review. The earlier 231-check suite and 54 development checks were
not rerun as part of this review.

## 1. P1 — Revoke reset links when account security details change

Evidence: `classes/MaintenanceService.php:110–137` revokes sessions on account
changes but leaves password reset tokens intact. `classes/AuthService.php:150–181`
accepts a token based on its user ID and expiry without binding it to the account
version or email that existed when it was issued.

Confirmed reproduction: issue a valid token for `old@example.test`, change the
account email to `new@example.test` through MaintenanceService, then submit the
old token. The reset succeeds. Someone holding a link delivered to the old mailbox
can therefore regain access while the token remains valid, up to 30 minutes.

Fix: invalidate existing tokens transactionally on email and other security changes;
bind issued tokens to the relevant account version so concurrent issuance cannot
leave an old token valid. Cover email changes, disable/reactivate, role changes,
and concurrent reset issuance with regression tests.

## 2. P1 — Replace the vulnerable browser dependency

Evidence: `xsl/content.xsl:41`, `xsl/login.xsl:36`, and
`xsl/createAccount.xsl:37` load `js/jquery-3.2.0.min.js`.

This version predates fixes for published cross-site scripting vulnerabilities in
HTML manipulation. The upstream advisories describe vulnerable operations and
affected versions; this review does not claim a demonstrated application exploit.
Composer audit does not cover bundled JavaScript files.

Fix: select a patched, supported jQuery version, review the bundled plugins for
compatibility, and rerun browser tests. Track browser dependencies in a manifest
and include them in dependency checks.

Primary sources:
- https://github.com/jquery/jquery/security/advisories/GHSA-gxr4-xjj5-5px2
- https://github.com/jquery/jquery/security/advisories/GHSA-jpcq-cgw6-v4j6
- https://jquery.com/upgrade-guide/3.5/

## 3. P1 — Complete financial history and corrections before real dues use

Evidence: `classes/Roster.php:66–71` displays charges, summed payments, and balances,
but no itemized payment history. `classes/RosterService.php:62–79` supports inserting
charges and payments, without a correction, void, or reversal workflow.
`migrations/002_roster.sql:30–49` has no recorded-by actor, correction reason, or
payment reference fields.

A payment or charge entered incorrectly currently requires direct database work.
The user also cannot inspect each payment's date and details in the application.

Fix: add itemized payment history and financial export/reconciliation; record who
entered each item and a reference/method where appropriate; implement permissioned,
audited adjustments or reversals that preserve the original entry. Test balance
recalculation, repeated requests, and concurrent corrections. This concerns the
manual ledger already in scope; online payment processing is a separate feature.

## 4. P1/P2 — Preserve and complete the audit trail

Evidence: `classes/Journal.php:38–44` lets an administrator truncate the complete
journal. `classes/RosterService.php:44–61` does not audit membership or season
changes. Financial messages at lines 67 and 77 omit amounts and the new financial
record's ID. `classes/Audit.php:3–7` catches write failures; financial writes commit
before their audit messages are attempted. Administrative account changes also
lack corresponding detailed audit entries.

Consequences: changing club access may leave no journal event, and the journal
can be erased. A financial save can succeed while its audit record is lost.

Fix: remove unrestricted journal clearing, define retention and protected archival,
record actor/entity/before-and-after details for privileged changes, and write
financial audit events in the same transaction or a transactional outbox. Keep
authentication available if its logging backend fails, with failure monitoring.

## 5. P2 — Verify email ownership before relying on it for club access

Evidence: `classes/AuthService.php:86–113` immediately activates a self-registered
account without verifying its mailbox. `classes/RosterService.php:44–55` assigns
membership to an existing username or email with no verified-email check.

An account can be registered using someone else's email. Signup alone does not
grant membership, but an administrator assigning that expected email could select
an account controlled by the wrong person.

Fix: implement mailbox verification or an invitation acceptance flow, and require
verified identity before assignment by email. Email changes should require fresh
verification and invalidate outstanding reset links. Until then, an explicit
independent identity-check procedure is needed for assignments.

## 6. Release gate — Verify production isolation, transport, and secrets

These are deployment checks, not claims that a public leak currently exists.

- `classes/Base.php:145–153` has no database TLS certificate/verification options.
  The approved non-TLS development connection does not establish a production
  transport design. If production uses a remote database, configure verified TLS
  or an explicitly secured private transport; use a restricted runtime database
  account and a separate migration account.
- `classes/Base.php:432–440` automatically prefers `xml/settings.local.xml` before
  environment overrides. Build a clean production artifact that excludes local
  secrets, `.tools` backups and control files, `.git`, tests, and IDE files.
  Current workspace changes include untracked application classes and migrations;
  ensure the release captures every required file.
- Apache private-directory rules depend on rewrite configuration. IIS and the
  development router have different rules; no Nginx configuration is provided.
  Verify private paths and backup filenames return a denial on the actual host.
  Prefer placing private code, secrets, and backups outside the document root.
- `main.php:20–30` detects HTTPS only through `$_SERVER['HTTPS']`. If TLS terminates
  at a proxy, configure a trusted proxy/server arrangement; otherwise HTTPS
  visitors can be rejected or cookies can be misconfigured. Do not trust arbitrary
  client-supplied forwarding headers.
- Confirm production environment mode, secure cookies, PHP error-display settings,
  HTTPS enforcement, and secret provisioning. Add HSTS at the HTTPS server and a
  tested Content Security Policy; the latter needs work on inline scripts and
  existing JavaScript links.
- Perform a production-host invitation/reset test using the configured application
  URL and sender. Earlier SMTP acceptance and local tests do not establish inbox
  delivery or the final hosting setup. This review did not test production DNS.

## 7. Release gate — Make backup, monitoring, and deployment repeatable

The verified manual backups prove a backup can be restored; the repository does
not demonstrate an automated production backup/restore policy, alerting setup, or
release pipeline. These may exist outside the project and should be verified.

Before launch:
- Schedule protected backups outside the application document root, define
  retention and recovery objectives, and periodically restore-test them.
- Monitor application failures, audit failures, SMTP delivery, database health,
  disk capacity, and failed backup jobs.
- Schedule `bin/cleanup-auth.php` as documented; ensure scheduled jobs use the
  correct environment and credentials.
- Create a reproducible release with platform-requirement checks, dependency
  audits, isolated regression tests, migration verification, and a recovery plan.
- Test representative roster sizes. Club rosters/dues and exports currently load
  complete result sets; large clubs need paging, query/performance testing, and
  export limits or background generation as appropriate.

## Product decisions and lower-priority improvements

- All club management currently belongs to global site administrators, who can
  access every club. This matches the implemented scope. If each club will manage
  itself, scoped club-administrator roles and cross-club authorization tests are
  needed before that operating model launches.
- Active members can see their club's other active members' email addresses
  (`RosterService.php:22–24`). Confirm that this is the intended directory/privacy
  policy; consider restricted visibility or opt-in contacts.
- Add MFA and recent-password confirmation for sensitive administrator actions.
  These are additional protections, not a claim that current session checks fail.
- Add season editing with history, clearer payment error recovery, keyboard/mobile
  usability checks, and repair the garbled username validation text in
  `AuthService.php:95`. These should follow the security and ledger work.

Recommended order: reset-token invalidation; browser dependency update; durable
financial history/corrections and audit; verified identity; production deployment
and operational verification. Resolve the club-administration and directory-privacy
decisions before enabling real club users.