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

backend/admin/roles_service.py

@@ -81,7 +81,23 @@ def _normalize_email(email: str) -> str:
 # The check exists because a typo'd grant ("analyst@" or "user@gmailcom") is
 # silent dead weight in ``user_roles`` — the auth gate can never match it, and
 # nobody notices until the intended user reports being locked out.
-_EMAIL_SHAPE = re.compile(r"^[^@\s]+@[^@\s]+\.[^@\s]+$")
+#
+# SEC-003: every atom here excludes "." so each split point is forced by a
+# literal dot — the pattern is unambiguous and matches in linear time. The
+# earlier ``[^@\s]+\.[^@\s]+`` domain half let ``[^@\s]`` match dots too, so a
+# rejecting address made the engine retry every dot in the domain and rescan to
+# the end each time: quadratic (CodeQL py/polynomial-redos). A 32 KB paste into
+# the admin Roles form took ~12s of blocked server thread; it now takes ~5ms.
+# Side effect of the rewrite, and desirable: empty DNS labels ("a@a..b",
+# "a@b.c.") are now rejected too, exactly the typo'd grants SEC-001 targets.
+_EMAIL_SHAPE = re.compile(r"^[^@\s]+@[^@\s.]+(?:\.[^@\s.]+)+$")
+
+# This service uses 254 Unicode code points as a conservative application-level
+# work bound. SMTP's wire limits are measured in octets, so this is deliberately
+# not a claim of full RFC validation; it is a simple upper limit for form input.
+# Checking it before the regex means a future pattern edit cannot quietly make
+# an arbitrarily large address consume unbounded matching work.
+_MAX_EMAIL_LENGTH = 254
 
 
 def _guard_last_admin(locked_admins: list[Any]) -> None:
@@ -116,7 +132,7 @@ def assign_role(
     role is a no-op that records nothing.
     """
     normalized = _normalize_email(email)
-    if not _EMAIL_SHAPE.match(normalized):
+    if len(normalized) > _MAX_EMAIL_LENGTH or not _EMAIL_SHAPE.match(normalized):
         raise RoleAssignmentError("A valid email address is required.")
     parsed = Role.parse(role)
     if parsed is None:

backend/storage/models.py

@@ -700,8 +700,10 @@ class UserRole(Base):
     )
 
     # The normalized, lower-cased identity email — the exact form the auth gate
-    # compares against ALLOWED_EMAILS/ADMIN_EMAILS. 320 is the maximum email
-    # length, matching ``audit_logs.user_email``.
+    # compares against ALLOWED_EMAILS/ADMIN_EMAILS. The column keeps its existing
+    # 320-character storage capacity to match ``audit_logs.user_email`` and older
+    # rows; ``assign_role`` applies the stricter 254-code-point input-work bound
+    # before new assignments reach persistence.
     email: Mapped[str] = mapped_column(
         String(320), primary_key=True, comment="Normalized lowercase user email"
     )

docs/architecture/audit-2026-06.md

@@ -275,3 +275,81 @@ QUAL-004 mypy on tests/ phase 1 (#96) · REF-002 app.py extraction of
 | Postgres worked deployment guide | **Landed** (#104), including the SQLite→Postgres data-migration recipe and pool guidance. |
 | Redaction single-pass regex | Still deferred — remains a micro-optimization. |
 | `_parse_company_page(session=None)` crash path | Still deferred — production-unreachable; tidy when the scraper next changes. |
+
+## August 2026 — GitHub code-scanning triage (SEC-003, appended 2026-08-31)
+
+CodeQL **default setup** (no `.github/workflows/codeql.yml`; alerts carry
+`analysis_key: dynamic/github-code-scanning/codeql:analyze`) raised two open
+**high**-severity alerts on `main`. The SARIF taint paths were pulled and traced
+before either was actioned. One was real, one was heuristic noise.
+
+### The genuine one — `py/polynomial-redos` (fixed in SEC-003)
+
+`backend/admin/roles_service.py` validated admin-entered addresses with
+`^[^@\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. Taint path: `ui/roles_page.py` (`st.text_input`) →
+`assign_role` → `_normalize_email` → `.match()`.
+
+Reproduced with the witness `"a@" + "a."*n + "@"` (a 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. Fixed by making every atom exclude `.`
+(`^[^@\s]+@[^@\s.]+(?:\.[^@\s.]+)+$`), which forces each split at a literal dot
+and matches in linear time, plus a 254-code-point application cap checked
+*before* the regex so the work stays bounded however the pattern is edited
+later. SMTP wire limits are measured in octets, so this is an input-work bound,
+not a claim that this pragmatic shape check performs full RFC validation.
+
+Deliberate side effect, consistent with SEC-001's intent: empty DNS labels
+(`a@a..b`, `a@.b.c`, `a@b.c.`) are now rejected. All pre-existing accept/reject
+cases are unchanged, and the timing property is pinned by
+`test_email_shape_pattern_stays_linear_on_pathological_input`.
+
+### Findings verified FALSE (do not re-flag)
+
+1. **"`backend/fundamentals/fundamentals_cache.py:155` stores a secret in clear
+   text" (CodeQL `py/clear-text-storage-sensitive-data`, alert #1).** False, and
+   it is worth knowing *why* so it is not re-litigated. The SARIF flow names the
+   sensitive-data **source** as `normalize_secret_safe_json()`
+   (`backend/scanning/result_contract.py`), reached via `_provenance_json` in
+   `sixty_seven/agent.py` and `technical/technical_agent.py`. 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** — the
+   persistence boundary that masks credential-named keys and pushes strings
+   through `redact_text`. CodeQL is flagging the sanitizer's own output.
+
+   In the cited path, what is actually written is the HMAC-signed envelope
+   `{schema_version, prompt_version, verdict, provenance}`. `provenance` passes
+   through the application's best-effort persistence redactor first, and the
+   only cryptographic material in the envelope is the **signature**
+   (`integrity_hmac_sha256`), never the signing key — see
+   `backend/ai_cache_integrity.py`. That evidence disproves this alert's claim
+   that the sanitizer's return value is itself a secret without overstating the
+   broader redaction safety net as proof about every possible arbitrary value.
+
+   **Considered and rejected:** renaming `normalize_secret_safe_json` to break
+   the substring heuristic. It is an accurate, well-documented public boundary
+   name with 55 references across 16 files (including `AGENTS.md` and six
+   architecture docs); churning all of that to satisfy a name match would trade
+   real clarity for a quiet scanner. The alert was **dismissed on GitHub as a
+   false positive** with this rationale instead.
+
+Lesson recorded: CodeQL's sensitive-data classification keys off identifier
+*names*, so any helper with `secret`, `token`, `password`, or `key` in its name
+will mark its return value tainted — including sanitizers, whose whole job is to
+make that value safe. Read the SARIF `codeFlows` before acting on a
+`clear-text-*` alert; the source node, not the sink, is what tells you whether
+the finding is real.

tests/test_admin_roles_service.py

@@ -2,6 +2,8 @@
 
 from __future__ import annotations
 
+import time
+
 import pytest
 
 from backend.admin import roles_service
@@ -80,6 +82,19 @@ def test_assign_same_role_is_noop_without_audit(file_session_factory, monkeypatc
         "spa ce@example.com",
         "user@nodot",
         "two@@example.com",
+        # SEC-003: empty DNS labels. The pre-SEC-003 pattern accepted these
+        # because its `[^@\s]` domain atoms matched dots as well; an address
+        # with an empty label is exactly the kind of typo'd grant SEC-001 wants
+        # kept out of user_roles.
+        "a@a..b",
+        "a@.b.c",
+        "a@b.c.",
+        # SEC-003: the ReDoS witness shape. Note this one is rejected by the
+        # length cap, not the pattern — the cap is the first line of defence.
+        # The pattern's own linear-time property is pinned separately below.
+        "a@" + "a." * 200 + "@",
+        # SEC-003: longer than the 254-code-point application cap.
+        "a" * 250 + "@example.com",
     ],
 )
 def test_assign_rejects_invalid_email(file_session_factory, bad_email):
@@ -90,6 +105,105 @@ def test_assign_rejects_invalid_email(file_session_factory, bad_email):
         )
 
 
+def test_assign_accepts_email_at_application_length_limit(
+    file_session_factory, monkeypatch
+):
+    """SEC-003: a structurally valid 254-character address remains assignable.
+
+    Beginner note: the length guard is inclusive. This fixture uses a 64-character
+    local part and domain labels no longer than 63 characters, so a rejection here
+    would reveal an off-by-one error in the application-level work bound rather than
+    a different shape-validation failure.
+    """
+    monkeypatch.setenv("ADMIN_EMAILS", "boss@example.com")
+    email = "a" * 64 + "@" + "b" * 63 + "." + "c" * 63 + "." + "d" * 61
+    assert len(email) == 254
+
+    result = assign_role(
+        email=email,
+        role="viewer",
+        assigned_by="boss@example.com",
+        session_factory=file_session_factory,
+    )
+
+    assert result.changed is True
+    assert result.email == email
+
+
+def test_assign_rejects_email_above_application_length_limit(file_session_factory):
+    """SEC-003: the first structurally valid address above the cap is rejected.
+
+    Beginner note: this is the 255-character counterpart to the accepted boundary
+    case above. Keeping every individual label valid isolates the total-length rule,
+    so the pair pins ``254 accepted / 255 rejected`` without depending on a malformed
+    email shape.
+    """
+    email = "a" * 64 + "@" + "b" * 63 + "." + "c" * 63 + "." + "d" * 62
+    assert len(email) == 255
+
+    with pytest.raises(RoleAssignmentError):
+        assign_role(
+            email=email,
+            role="viewer",
+            assigned_by="boss@example.com",
+            session_factory=file_session_factory,
+        )
+
+
+def test_email_shape_pattern_stays_linear_on_pathological_input():
+    r"""SEC-003: the pattern itself must reject a ReDoS shape in milliseconds.
+
+    Beginner note: the old `^[^@\s]+@[^@\s]+\.[^@\s]+$` was *ambiguous* — its
+    domain half could split at any dot, so a rejecting address forced the regex
+    engine to retry every dot and rescan to the end each time (quadratic work).
+    This input took ~18 seconds on that pattern and ~8ms on the current one.
+
+    This asserts on ``_EMAIL_SHAPE`` **directly**, and deliberately not through
+    ``assign_role``: the 254-code-point application cap rejects any input long enough
+    to be pathological before the regex is reached, so a test routed through
+    ``assign_role`` would pass even with the ambiguous pattern restored and
+    would guard nothing. The cap is the first line of defence; this test is the
+    second, so raising or removing the cap later cannot silently re-expose the
+    quadratic blow-up. The generous one-second bound cannot flake on a loaded CI
+    runner while still failing loudly if the ambiguity ever returns.
+    """
+    pathological = "a@" + "a." * 20000 + "@"
+
+    started = time.perf_counter()
+    assert roles_service._EMAIL_SHAPE.match(pathological) is None
+    assert time.perf_counter() - started < 1.0
+
+
+def test_assign_rejects_over_length_email_before_running_the_pattern(
+    file_session_factory, monkeypatch
+):
+    """SEC-003: the length cap short-circuits, so the regex never sees huge input.
+
+    Pins the ordering that makes the cap meaningful as a ReDoS bound: if the
+    guard were ever reordered to match first, this fails.
+    """
+    calls: list[str] = []
+    original_pattern = roles_service._EMAIL_SHAPE
+
+    class _RecordingPattern:
+        def match(self, value: str):
+            calls.append(value)
+            # Beginner note: capture the real pattern before monkeypatching the
+            # module. Looking it up through ``roles_service`` here would find this
+            # recording double again and recurse instead of exercising the matcher.
+            return original_pattern.match(value)
+
+    monkeypatch.setattr(roles_service, "_EMAIL_SHAPE", _RecordingPattern())
+
+    with pytest.raises(RoleAssignmentError):
+        assign_role(
+            email="a@" + "a." * 20000 + "@", role="viewer",
+            assigned_by="boss@example.com", session_factory=file_session_factory,
+        )
+
+    assert calls == []
+
+
 def test_assign_accepts_and_normalizes_realistic_email(file_session_factory, monkeypatch):
     # The stricter SEC-001 shape check must not reject legitimate addresses;
     # mixed case and padding still normalize to the canonical lowercase key.