diff --git a/backend/admin/roles_service.py b/backend/admin/roles_service.py index 27699ce..7a5de6a 100644 --- a/backend/admin/roles_service.py +++ b/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: diff --git a/backend/storage/models.py b/backend/storage/models.py index ed97f27..60d1520 100644 --- a/backend/storage/models.py +++ b/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" ) diff --git a/docs/architecture/audit-2026-06.md b/docs/architecture/audit-2026-06.md index 287e44e..1d58910 100644 --- a/docs/architecture/audit-2026-06.md +++ b/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. diff --git a/tests/test_admin_roles_service.py b/tests/test_admin_roles_service.py index 923d7cd..5c09791 100644 --- a/tests/test_admin_roles_service.py +++ b/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.