v2.2.3 — Audit remediation: login hardening, financial race fixes, scheduler and attendance load

Released as 5d29abb41 on 2026-10-12 at 05:27 UTC by release run 38207971990. Migration 6a01dc7a606b. Record: DEPLOY_2026-10-12_5d29abb41.md. The F1–F4 payment entries below (#1534) were first filed under [2.2.2], but #1534 merged after the v2.2.2 tag; they shipped in this release.

Fixed

• Rent and insurance premium payments no longer overdraw checking (financial-correctness audit F1–F4, 2026-10-10; DOM-OBL-001 3.3 §II.C / SPEC-ECON-003 §4.5.1.1, owner ruling 2026-10-07). Three live payment paths accepted a payment the seat could not cover and posted it anyway:

  • Rent (pay_rent, FEAT-OBL-001) applied its resolved Ledger plan with no affordability check; a student could pay rent into a negative checking balance, and the obligation was marked paid regardless. pay_rent and pay_rent_bill now refuse with INSUFFICIENT_FUNDS — leaving the obligation unpaid, owed, and subject to late fees — when checking, together with any full overdraft-protection transfer, does not cover the payment. A bill payment (rent + its late fees) is checked against its full combined amount before any slice posts, so a bill is never partially charged.
  • Insurance first premium (execute_purchase_insurance, FEAT-OBL-004) debited the premium with no balance read at all. It now refuses INSUFFICIENT_FUNDS before any mutation — no entitlement grant, bill cycle, assessment, or ledger row — the same "no agreement was formed" outcome as a refused Store purchase.
  • Manual insurance premium payment (execute_insurance_premium_payment, FEAT-OBL-003) had no affordability check at all, unlike the automatic-renewal path (_attempt_autopay), which already refused. Both now share one premium_is_affordable check in insurance_premium_payment_feat.py.

  FEAT-OBL-003 §1.a and FEAT-OBL-004 §VII/§VIII/§XII are amended (1.2 and 0.4) to withdraw their stale "checking-only affordability rejection does not apply" wording, which predated the 2026-10-07 owner ruling.

• Rent payment amounts are validated before any mutation (same audit, F4). A NaN, Infinity, negative, or sub-cent (e.g. $0.001) payment amount raised mid-write or silently rounded to an amount different from the one reported paid. pay_rent/pay_rent_bill now refuse any amount that isn't a positive, finite, whole-cent value with INVALID_AMOUNT before touching the ledger. An extreme-but-finite amount (e.g. 1e999999) is also refused rather than raising decimal.InvalidOperation out of the whole-cent check (PR #1534 review).

• A double-click or back-button retry of a payment this command already partly or fully posted no longer sees INSUFFICIENT_FUNDS (PR #1534 review, same audit). The F1–F4 affordability checks above re-evaluated the request against the current balance on every call, including a replay — so a retry with the same idempotency key, arriving after its own first call already spent the money it needed, was wrongly refused instead of cleanly replaying its already-accepted charge. pay_rent_bill and settle_insurance_premium now check for an existing command reservation first and only screen a genuinely fresh command's affordability; a true replay reaches the existing idempotent Ledger write path unconditionally. Fixing this also surfaced and corrected a related bug in pay_rent_bill's remaining_after: it was computed as group_remaining - total_paid, which double-counted an already-applied amount on replay and under-reported what was still owed.

• A bulk manual payroll credit could pay the same students twice on retry (audit finding F5). Each selected student's credit committed in its own FEAT transaction, keyed by a submission-wide nonce that was freshly randomized on every POST. A mid-batch failure (a lock timeout on student 15 of 30, say) left the first 14 already paid, and the teacher's retry — the only recovery the UI offered — minted new idempotency keys for every student and paid those 14 a second time. The nonce is now derived from the submission's own content (teacher, class, selected students, description, amount) within a 5-minute window, so an immediate retry of the identical batch replays the same per-seat ledger effect instead of creating a new one; the same batch run again outside that window (a genuinely new payment) still pays normally.

• Two overlapping rent-reconciliation runs could double-assess the same obligation (audit finding F8). assess_obligation()'s idempotency check is a plain read before the insert, with no row lock, and the one guard that used to exist — a unique index on assessment_events.correlation_id — was deliberately dropped in 8a7b6c5d4e3f because one correlation_id legitimately carries several event types (ASSESSMENT, PAYMENT, WAIVED). Migration 6a01dc7a606b adds a partial unique index on (internal_ref, correlation_id) WHERE event_type = 'ASSESSMENT', which protects exactly FEAT-OBLI-001 §IV.3's promise — one ASSESSMENT per lineage — without resurrecting the blanket uniqueness the prior migration correctly removed. (A full (internal_ref, correlation_id, event_type) constraint was rejected: it would make a second partial PAYMENT on the same obligation an IntegrityError.) The index build fails if duplicate ASSESSMENT rows already exist; production was checked read-only on 2026-10-12 at revision 35a9a0d3ab93 and has none. assess_obligation() now recovers the winning row on a constraint collision instead of surfacing an error to the scheduler.

• A scheduled job still running could be double-run by a new scheduler owner (audit finding O5). app/scheduler_ownership.py::_release_ownership() called scheduler.shutdown(wait=False), which returns immediately, then closed the advisory-lock connection on the very next line — releasing the lock for a waiting successor while the old job's worker thread could still be executing. Release now waits for the scheduler's executor threads to actually finish (wait=True) before the connection, and the lock it holds, are released.

• Two concurrent hall-pass requests from the same seat could reserve the same pass twice (audit finding F6). submit_hall_pass_request only locked a seat's existing pending-request row before clearing it and checking entitlement availability; a seat with no pending row yet has nothing for that to lock, so two near-simultaneous submissions could both read "nothing pending," both find the same available grant, and both insert — two pending rows reserving one pass instead of the second genuinely replacing the first. app/feats/hall_pass_request_feat.py::submit_hall_pass_request now takes an unconditional with_for_update() lock on the seat row as its first statement, serializing the pair the same way transfer_feat.py and store_purchase_feat.py already serialize theirs. Proven against the emitted SQL (statement order), not a hold-and-timeout race — pending_actions carries a foreign key to seats, so an INSERT into it already takes an implicit FOR KEY SHARE lock on the parent regardless of whether the explicit fix is present, which made a first draft of the regression test pass unchanged with the fix reverted.

• The "max students out" hall-pass limit could be exceeded (audit finding F7), two ways. First, a concurrency race: _enforce_hall_pass_settings counted today's approved/out hall-pass logs for the class with no class-wide lock, so two concurrent approvals for two different students could both read the same "currently out" count before either had inserted its row, and both pass the check. app/feats/prod.py now locks the ClassEconomy row (with_for_update()) before counting, as the first statement of the approval path. Second, a logic gap, independent of any race: the count only included passes already in the "left" state, so a student approved but not yet tapped "Left Class" didn't count against the limit at all — rapid-fire approvals with nobody yet walked out always read zero and all succeeded, defeating the limit entirely. The count now includes both "approved" and "left" states. Both proven in real PostgreSQL: the lock by statement order (same false-positive trap and fix as F6, since hall_pass_logs also carries a class foreign key), the logic gap by a plain behavioral test confirmed to fail with the fix reverted.

• The teacher roster's balance figures could read a transaction's money as settled by neither "posted" nor "pending" (audit finding F9), and "earnings" overcounted non-earned credits. get_batch_balances_by_class_seat (the only caller is roster_view_model.py, behind the teacher's Student Management roster) read the posted snapshot and the pending-transaction sum as two separate statements — exactly the race _available_balance_expression's own code comment already documents for the single-seat read (bug #1364): a settlement committing between the two round trips moves a transaction out of "pending" after the first statement already read the stale posted figure, so the amount is counted in neither. The batch read now computes both in one statement, the same model _available_balance_expression already uses. Separately, "earnings" only excluded "Transfer"-prefixed deposits, so the automatic overdraft-protection funding leg (type Deposit) and reversal refund credits (type REVERSAL) both inflated a student's reported earnings; the exclusion list now covers both. Proven in real PostgreSQL: the statement-count fix by counting emitted SQL statements (the only reliable way to prove "one round trip," since no table-level locking trick distinguishes it the way F6/F7's did), the earnings fix with a real purchase-and-refund via the production reversal path (a hand-built REVERSAL row is rejected by the enforce_ledger_recovery_cap() trigger's exact-negation check).

• An unrecognized account name on a student transfer returned a 500 instead of a graceful error (audit finding F10). /student/transfer passed from_account/to_account straight from the form to get_available_balance() with no server-side validation — only the <select> in templates/student_transfer.html constrained the choices client-side, so a crafted POST with any other value hit _require_balance_scope()'s ValueError("FATAL: Unsupported account_type...") uncaught. app/routes/student.py::transfer() now rejects any from_account/to_account outside {"checking", "savings"} with a normal flash/JSON 400 response before any balance lookup.

• Attendance taps are budgeted per seat, and student login's shared per-address budget is narrower (audit finding O1, #1551). A school NAT puts every student behind a few public addresses, so /student/login's 60 per minute (which also counted GET page loads) and /api/tap's 100 per minute were spent by the whole building at bell time. Student login's address limit now counts POSTs only, plus a new 10 per minute limit per submitted username (student_login_identity_limit_key(), keyed by the username lookup digest, never plaintext) that bounds guessing against one account. The address-keyed POST limit itself remains, so more than 60 login attempts a minute from one school address can still be refused; O1 is narrowed for login, not closed. /api/tap is keyed by (class_id, seat_id) when a canonical context is attached (attendance_tap_seat_limit_key()), falling back to the address otherwise. New tests: tests/test_bell_time_rate_limits.py.

• The hourly scheduled jobs no longer all fire in the same minute, and the daily-limit job no longer loads every attendance row (audit findings O2, O3, #1552). The nine hourly jobs used interval, hours=1, which anchors every job to process start, so all nine came due together every hour and drifted on each deploy; they now run on fixed, distinct cron minutes (order preserved: ledger settlement before savings interest). enforce_daily_limits_job() loaded the full AttendanceSession history to find open seats; a window function now returns only seats whose latest row is active. New tests: tests/test_scheduled_job_stagger.py, tests/dom/prod/test_stale_session_sweep.py.

• A student status poll no longer re-scans the seat's whole attendance history three or four times (audit finding O4, #1553). The readers in app/services/attendance_service.py each ran the same unbounded per-seat query; it is now memoized for one request (_canonical_seat_rows()), invalidated by the attendance writer after it flushes, by an after_flush listener for any other write path, and cleared on rollback. Nothing is cached outside a request (scheduled jobs). New tests: tests/test_attendance_status_request_cache.py.

• The 404 log de-duplication cache is capped and no longer rescanned on every 404 (audit finding O8, #1554). wsgi.py's not_found_error rebuilt the whole in-memory cache on each 404 and had no size limit, so a scanner probing many unique paths cost O(n²) and grew memory for an hour. Pruning now runs at most once a minute or when the cache passes 10,000 entries, trimming oldest-first to 9,000. New tests: tests/test_404_dedupe_cache.py.

Security

• The sysadmin login now verifies Turnstile server-side, and both teacher and sysadmin logins have a per-account attempt budget and TOTP replay protection (audit finding S5). The sysadmin login's Turnstile widget rendered on the page but was never checked server-side, unlike the teacher login (S3); it now calls verify_turnstile_token() the same way. Both logins previously budgeted TOTP attempts only per IP (10 per minute), so attempts against one account could be spread across addresses or simply outlast a budget shared with other legitimate traffic; login_account_limit_key() (app/extensions.py) adds an independent 5 per 5 minutes limit keyed by an unsalted lookup digest of the submitted username. valid_window=1 also let a captured code sign in again for up to ~90 seconds of drift tolerance; login_totp_replay_limit_key() adds a 1 per 2 minutes limit keyed by (account, exact code), so resubmitting the same code is refused while a different code (a genuine retry) is not. New digest: hash_login_totp_replay_key() in app/hash_utils.py. New tests: tests/test_login_hardening_s5.py.

• Student usernames no longer draw their two vocabulary words from a non-cryptographic PRNG (audit finding S9). build_username() used random.sample(), a Mersenne Twister draw whose internal state is recoverable from enough output and would then make future draws predictable; it now uses secrets.SystemRandom().sample(). Output format is unchanged. New tests: tests/test_username_generation_rng.py.

• A missing TURNSTILE_SECRET_KEY no longer disables the bot check in production (audit finding S3, 2026-10-10). verify_turnstile_token() skipped verification whenever the secret key was absent, and the key was not in create_app()'s required-environment list, so a production deploy that dropped it started normally with student login, seat claim, student recovery and teacher recovery all missing their only Turnstile check. Production now requires TURNSTILE_SECRET_KEY/TURNSTILE_SITE_KEY at startup, and the verifier fails closed (returns False) if the key is ever missing while ENV=production. Development and testing are unchanged.

• Forwarded-IP headers (CF-Connecting-IP, X-Forwarded-For) are now trusted only from our own reverse-proxy peer (audit finding S7). The rate limiter's key function and get_real_ip() read these headers from any TCP peer with no provenance check, so a request that reached the application directly (bypassing nginx/Cloudflare) could set them to rotate every per-address rate limit and the audit-logged address. app/utils/ip_handler.peer_is_trusted_proxy() now checks the connecting peer against loopback (or TRUSTED_PROXY_CIDRS, for a proxy on another address) before either header is read; an untrusted peer is keyed and logged by its own remote_addr.

• Student PIN format is now enforced server-side (audit finding S8). FEAT-IDEN-002's own failure table requires a 4-6 digit PIN that is neither all one digit nor sequential, but activate_student_credentials() hashed whatever arrived; only a client-side regex in student_pin_setup.html checked the shape, and a crafted POST bypassed it entirely. app/feats/identity_feat.py::_pin_is_well_formed() is now the one gate every setup and recovery path funnels through before hashing.

• Decrypted display names are no longer cached in the Flask session (audit finding S10). DisplayMetadata (app/utils/display_metadata.py) carries decrypted PII (actor_first_name and its siblings) by design, and a prior revision of SPEC-DISPLAY-001 allowed caching the whole object in the session — a signed but unencrypted cookie, putting that PII at rest in plaintext on the client, in conflict with INV-CORE-000 section 2 / INV-ARC-018 section VII.1. The admin layout's separate teacher-name cache (app/utils/display_name_session.py) did the same for the teacher's name shown before a class is selected. Operator ruling (Tim, 2026-10-12): resolve both fresh from the database on every call rather than narrow the caching allowance — decrypting a short display string per request is cheap, so there was no tradeoff worth making. SPEC-DISPLAY-001 bumped to v1.1, removing the session-cache contract (section VII) entirely. get_admin_display_name_cache() now also purges any plaintext name a pre-fix session cookie may still carry. New tests: tests/test_display_name_cache_s10.py; tests/test_identity_teacher_note_surface.py and tests/dom/identity/test_identity_establishment_requires_no_principal.py updated for the removed cache.

• Aikido removed; CodeQL's query suite switched to security-extended (follow-up to S1/S4, see docs/ops/audits/INCIDENT_2026-10-11_SEC-SAST-GAP-001.md). Aikido's free-tier PR check is diff-scoped only: it had actually flagged the S1 client-side sink as a High "likely exploitable" finding, but because no PR after the one that introduced it touched that line again, every later PR's check reported clean and the finding sat unreviewed in Aikido's dashboard. A scanner whose gate reports green regardless of its own backlog is a worse signal than none, so it was removed. CodeQL's default query suite had no coverage at all for either S1 sink or S4 (a value reaching Jinja output via safe or a custom Markup()-wrapping filter isn't modeled by the default suite); the repository owner switched code-scanning/default-setup to security-extended in Settings, which immediately surfaced a larger alert backlog now being triaged separately. That switch only partly closes the original gap: security-extended's Jinja query (py/jinja2/autoescape-false) still does not model the safe-filter/Markup() pattern either sink actually used, so a custom query or guard test for that pattern is now a required (not optional) follow-up — see the incident record.

• security-extended CodeQL backlog: actionable findings fixed, the rest classified per alert (follow-up to the suite switch above; see docs/ops/audits/SECURITY_TRIAGE_2026-10-11_CODEQL_BASELINE_CLASSIFICATION.md for the full 98-alert breakdown with per-alert evidence). Fixed:

  • Polynomial ReDoS (py/polynomial-redos, 7 alerts) in scripts/verify_chain.py, scripts/validate-migrations.py, and scripts/analyze_migrations.py — each extracted Alembic's down_revision/branch_labels/depends_on assignments with an ambiguous ^name\s*=\s*(.+)$ regex over migration-file text. A new _extract_assignment_value() helper in each script replaces the regex with a single-pass line scan; output is unchanged on every migration file in migrations/versions/. scripts/localize_font_links.py's STALE_COMMENT_RE (an overlapping [^\n]*(alt)[^\n]* shape) is replaced by a keyword-based strip_stale_comments() with no regex.

  • Log injection (py/log-injection, 8 alerts) — the 404/403/401/400/429/503 error handlers in wsgi.py and the Cloudflare-mismatch warning in app/__init__.py interpolated request.url/request.path/request.remote_addr into a log line unsanitized. A new sanitize_for_log() (app/utils/helpers.py) escapes CR/LF and other control characters before every one of those sites. Fixing this also surfaced a materially wider, CodeQL-missed instance: RequestIdFilter's fallback to raw request.path for every log record's endpoint field (any unmatched route), and endpoints supplied on the record, now go through the same sanitizer. New tests: tests/test_no_log_injection.py.

  • Code injection (py/code-injection, 1 alert) — scripts/dev-utilities/diagnose_migrations.py used eval() to parse a migration's tuple-form down_revision; replaced with ast.literal_eval().

  • Command-line injection (py/command-line-injection, 4 alerts) — scripts/add-github-actions-to-firewall.py and scripts/create-github-actions-firewall.py ran doctl/firewall-rule commands via subprocess.run(cmd, shell=True) with f-string commands built from a CLI-supplied id and GitHub's /meta API response. Both now build argv lists, which need no shell quoting regardless of what either value contains.

  • Path injection (py/path-injection, 5 alerts) — scripts/lint_migrations.py now rejects symlinked baselines, changed-file lists, migration sources, and parent directories before reading them. Git checkout paths alone did not prevent symlink targets outside the repository from being read and printed. Targeted tests cover direct, parent-directory, and broken symlinks.

  The remaining 73 baseline alerts are classified, not blanket-dismissed: 63 are documented false-positive/mitigated findings with per-alert code-level evidence (an internal path-injection source, a sysadmin-gated proxy with a configured origin and an ineffective path-prefix check, hardcoded denial messages from both reversal exception paths, argv-list subprocess calls CodeQL's taint analysis didn't resolve, and two autoescape-false blocks that render no request-derived content), and 10 (actions/unpinned-tag, third-party GitHub Actions pinned to a mutable tag) are confirmed real and deliberately deferred to their own follow-up PR, since several of the affected workflows hold production deploy credentials and pinning them deserves its own review rather than being rushed into this one. Classification alone changes nothing on GitHub's Security tab; dismissing the 63 false-positive/mitigated alerts needs security_events-scoped API access this change doesn't have.

• Custom regression guard for the Markup()/|safe taint shape behind S1 and S4 (the incident record's follow-up item 3, "required, not optional" — neither CodeQL's default suite nor security-extended has a query for it). tests/guards/unsafe_jinja_filter_markup.py statically checks every Jinja filter function registered via jinja_env.filters[...] for a Markup() call on text that isn't a literal, hasn't passed through markupsafe.escape()/a bleach .clean(), or isn't built only from such values; and every template {{ ... }} expression ending in |safe for a filter chain not yet proven to escape first, plus any {% autoescape false %} block. Nine pre-existing |safe usages that are safe for reasons the guard can't see from the template alone (HTML sanitized server-side before reaching the template, or hardcoded non-request content) are recorded in tests/guards/unsafe_safe_filter_baseline.txt, following the same discipline as migrations/lint_baseline.txt: the baseline only shrinks, and is not a place to silence a new finding without review. Enforced by tests/test_no_unsafe_jinja_markup.py, which also proves the detector against synthetic reproductions of the exact S1/S4 shapes. A deliberate reinstatement of the exact S1/S4 sinks on a never-merged draft PR (#1547) confirms this: the guard fails on both as designed, and CodeQL remains silent on both as predicted.

• docs/STANDARD_OPERATING_PROCEDURES/SECURITY/SOP-SEC-002_SAST_FINDINGS_REVIEW_AND_DISPOSITION.md (the incident record's remaining two follow-up items). Documents the code_scanning branch-ruleset rule's security_alerts_threshold: high_or_higher configuration and why it was chosen over a stricter or absent threshold, including the experimentally confirmed fail-closed-on-missing-analysis behavior and the known gap that the ruleset has no required status check for this repository's own guard tests; formalizes the evidence-based, never-blanket per-alert disposition process used for the security-extended triage above; and establishes a recurring (not purely PR-triggered) backlog review, at minimum after every query-suite change and at least quarterly, so a pre-existing finding in untouched code gets a scheduled look.

• Release workflow actions pinned to commit SHAs, and the backup tool quotes table identifiers (#1546). Six workflows (release-v2.yml, tailscale-ssh-smoke-test.yml, deploy-status.yml, login-notice.yml, docs-links.yml, actionlint.yml) now reference their third-party actions (tailscale/github-action, webfactory/ssh-agent, google-github-actions/auth and setup-gcloud, lycheeverse/lychee-action, reviewdog/action-actionlint) by full commit SHA rather than a movable tag. infra/db-backup/cth_db_backup.py's table_counts() builds its query with psycopg2.sql.Identifier instead of an f-string (not exploitable: table names come from pg_tables). The adversarial proof harness under scripts/adversarial/ was repaired alongside; see docs/ops/audits/SECURITY_TRIAGE_2026-10-11_CODEQL_FOLLOWUP.md.

Investigated, no code change

• Teacher-recovery resume PIN (audit finding S2): a six-digit PIN looked up across every open recovery request, not cleared after use for the request's 5-day life, matches DOM-IDEN-003's own design (a "separate-purpose verifier" explicitly meant to persist across a resume) — not a defect in that contract. Its real exposure depended on whether Turnstile was actually configured in production, which S3 above now guarantees rather than silently bypassing. Measuring the live count of open, PIN-bearing recovery requests needs production access this review didn't have; see the PR for the read-only query to run.
• Seat-claim failure responses distinguish invalid credentials from an already-claimed seat (audit finding S6): FEAT-IDEN-001's own failure-scenario table (section VII) requires these distinct responses for a workable claim UX, while SPEC-SEC-001 section V.4's generic-response rule is written for authentication/recovery flows. Operator ruling (Tim, 2026-10-12): probably intentional, subject to enumeration controls — keep the distinguishing responses, but /student/claim-account had no rate limit at all (the route's own comment said so). It now carries @limiter.limit("10 per minute", methods=["POST"]), the same per-IP budgeting pattern used elsewhere. New tests: tests/test_claim_account_rate_limit_s6.py.