SEC-003: fix polynomial ReDoS in the admin email shape check

[DoRmAmMu1997]

Why

GitHub code scanning reported two open high-severity CodeQL alerts on main.
Both are resolved here: one is a genuine bug and is fixed, the other is a
name-heuristic false positive and is documented rather than "fixed".

Alert	Rule	Verdict
#2	py/polynomial-redos	Genuine — fixed below
#1	py/clear-text-storage-sensitive-data	False positive — analysed and recorded

Alert #2 — polynomial ReDoS (genuine)

The SEC-001 address check used ^[^@\s]+@[^@\s]+\.[^@\s]+$. That pattern is
ambiguous: [^@\s] matches . as well, so [^@\s]+\.[^@\s]+ can split at
any dot in the domain. On a rejecting address the engine retries every split
and rescans to the end each time — quadratic.

SARIF taint path: ui/roles_page.py (st.text_input) → assign_role →
_normalize_email → .match().

Reproduced with the witness "a@" + "a."*n + "@" — the trailing @ survives the
.strip() in _normalize_email, and is the same shape as the long-standing
two@@example.com test case:

input	before	after
16 KB	1.97 s	1.9 ms
32 KB	12.3 s	5.5 ms
40 KB	17.8 s	8 ms

Reachability is admin-only (ui/roles_page.py returns early without the admin
role), so the realistic impact is a careless or compromised admin session rather
than anonymous DoS — but Streamlit runs the script in the server process, so one
paste blocks a server thread for ~12 s.

Two independent defences

1. Unambiguous pattern — every atom now excludes .
   (^[^@\s]+@[^@\s.]+(?:\.[^@\s.]+)+$), forcing each split at a literal dot and
   matching in linear time.
2. 254-code-point application cap, checked before the regex, so the work
   stays bounded however the pattern is edited later. SMTP wire limits use
   octets, so this is deliberately not presented as full RFC validation.

revoke_role is deliberately left alone: it never runs the regex, and its
non-empty-only check is what lets an admin clean up badly-shaped rows that
predate SEC-001.

Deliberate side effect, consistent with SEC-001's intent: empty DNS labels
(a@a..b, a@.b.c, a@b.c.) are now rejected. Every pre-existing accept/reject
case is unchanged, and the new pattern was proven to be a strict subset of
the old one (exhaustive enumeration to length 6 over a.@␠-, plus 300k random
strings, found zero addresses newly accepted) — so this cannot widen who may be
granted a role.

A note on the tests, because review changed them

The first version of the timing guard ran through assign_role and was
vacuous: the new length cap short-circuits the or, so the regex was never
reached (0 invocations) and the test passed even with the ambiguous pattern
restored. It now asserts on _EMAIL_SHAPE directly, which keeps defence 1
guarded independently of defence 2, and a second test pins the short-circuit
ordering that makes the cap a real bound.

The matcher and guard were mutation-checked — reverting either protection fails
the corresponding test. The follow-up also pins a shape-valid 254-character
address as accepted and its 255-character counterpart as rejected:

mutation	test
restore the ambiguous pattern	test_email_shape_pattern_stays_linear_on_pathological_input FAILS
reorder the cap after the regex	test_assign_rejects_over_length_email_before_running_the_pattern FAILS
change the cap to 253 or 255	the corresponding 254/255 boundary test FAILS

Alert #1 — clear-text storage of a secret (false positive)

The SARIF flow names the sensitive-data source as normalize_secret_safe_json()
(backend/scanning/result_contract.py), reached via _provenance_json. CodeQL's
SensitiveDataSource classifier is name-based — the substring "secret" in
the callee name makes it treat the return value as classification: secret. But
that function is the redactor, so the alert is flagging the sanitizer's own
output.

In the cited path, the HMAC-signed envelope
{schema_version, prompt_version, verdict, provenance} contains provenance that
passes through the application's best-effort persistence redactor and carries
the signature (integrity_hmac_sha256), never the signing key — see
backend/ai_cache_integrity.py. This disproves the alert's claimed source
without overstating the broader redaction safety net as proof about arbitrary
values.

Considered and rejected: renaming normalize_secret_safe_json to break the
substring heuristic — an accurate, well-documented public boundary name with 55
references across 16 files (including AGENTS.md and six architecture docs).
The analysis is recorded in docs/architecture/audit-2026-06.md under Findings
verified FALSE (do not re-flag). Alert #1 was dismissed on GitHub as a
false positive on 2026-08-31 with the same rationale and verified by API
readback.

Codex follow-up review

Commit 8ec23fb implements the bounded findings from the comment-first review:

• fixes the self-recursing recording matcher by delegating to the captured real pattern;
• adds exact, mutation-checked 254/255 boundary tests;
• distinguishes application code-point bounds from SMTP octet limits;
• aligns the storage-capacity comment and audit test reference; and
• narrows the false-positive rationale to the verified redaction/HMAC guarantees.

Normalized AST comparison confirms the follow-up changes no production Python
behavior relative to the original PR head c80ff2d4. A second Codex Security
scan of the final origin/main...8ec23fb range found zero reportable findings.
The follow-up commit is co-authored by Codex.

Gates

All green on this branch:

• hosted Python 3.11 and 3.12: pytest — 1938 passed on each; coverage 89.83% (floor 89)
• ruff clean · mypy clean over 257 files · bandit clean · pip-audit clean · compileall clean
• pre-commit validate-config clean · CodeQL clean
• hosted Docker build + Compose health smoke test clean
• git diff origin/main HEAD -- constraints.txt pyproject.toml empty (no dependency drift)

No schema/ORM behavior, CI-command, dependency, or deterministic-screener changes,
so no migration, supply-chain policy update, or golden regeneration is required.
The migration suite was nevertheless re-run in the focused follow-up verification.

/code-review and /security-review were run on the finished diff. The original
and final Codex Security scans returned zero reportable findings, and the
published PR-head CodeQL analyses returned zero results.

🤖 Original implementation generated with Claude Code; follow-up review and hardening co-authored by Codex.

[DoRmAmMu1997]

Codex follow-up review (COMMENT): the production ReDoS fix is sound. I reviewed the exact a154f248...c80ff2d4 range, followed the admin-input path through role persistence, reproduced the old/new timing difference, and completed a Codex Security diff scan with no reportable vulnerabilities. GitHub's PR-head CodeQL analyses also report zero results.

Before merge I found four bounded follow-ups:

1. tests/test_admin_roles_service.py:145 resolves _EMAIL_SHAPE through the module after monkeypatching it, so the recording double recursively calls itself if the length guard regresses. The double should capture and delegate to the original compiled pattern.
2. The exact 254/255 application boundary is not pinned; add valid boundary cases without changing the production regex.
3. docs/architecture/audit-2026-06.md:316 names a nonexistent timing test, and the RFC wording should distinguish this code-point work bound from full RFC/octet validation.
4. The audit note says CodeQL alert #1 was dismissed and that no secret is persisted. The alert is currently still open, and the latter wording is broader than the documented best-effort redaction contract. I independently confirmed the CodeQL name heuristic and the HMAC/redaction path; I will narrow the prose and dismiss/read back alert #1 as an authorized false positive.

I am applying these as a surgical follow-up commit, then will rerun the repository gates and the diff security scan before updating this PR.

[DoRmAmMu1997]

Codex follow-up complete at 8ec23fbe055fb8503cbb1fce29ee69700e8cdf64.

Implemented:

• repaired the self-recursing regression-test matcher by delegating to the captured compiled pattern;
• added mutation-checked 254/255 boundary coverage with shape-valid addresses;
• corrected the application-bound versus SMTP-octet wording, storage-capacity comment, audit test reference, and best-effort-redaction rationale;
• dismissed CodeQL alert Switch charts to TradingView Lightweight Charts v5 #1 as false positive and verified the GitHub API readback; genuine alert Switch charts to Lightweight Charts v5 and polish the UI #2 remains open until this PR merges; and
• retained production behavior: normalized ASTs for roles_service.py and models.py match the original PR head c80ff2d4.

Verification:

• focused role/UI/migration batch: 51 passed;
• hosted Python 3.11: 1938 passed, coverage 89.83%;
• hosted Python 3.12: 1938 passed, coverage 89.83%;
• Ruff, mypy over 257 files, compileall, Bandit, pip-audit, pre-commit config, and dependency-drift checks: clean;
• hosted Docker image plus Compose health smoke test: green;
• PR-head CodeQL: zero results / all checks green;
• final Codex Security scan 90c63003-d990-45dc-a60e-932f157041e9: zero reportable findings; and
• PR state: cleanly mergeable at the follow-up SHA.

Local-only note: this machine has Python 3.13 rather than the supported CI 3.11/3.12 and no Docker. Its full run reached 1936 passed, 1 skipped, coverage 89.79%, plus the pre-existing order-dependent LogRecord.message test failure; that test passed alone. The supported hosted matrices and Docker gate are fully green.

The follow-up commit includes Co-authored-by: Codex <codex@openai.com>.