From c80ff2d4ffa97276e21e7d5ebd24808d78d21704 Mon Sep 17 00:00:00 2001 From: DoRmAmMu1997 Date: Mon, 31 Aug 2026 15:18:11 +0530 Subject: [PATCH 1/2] SEC-003: fix polynomial ReDoS in the admin email shape check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub code scanning (CodeQL default setup) reported two open high-severity alerts on main. This resolves both: one is a genuine bug, one is heuristic noise that is now documented rather than "fixed". py/polynomial-redos (genuine) — the SEC-001 address check used `^[^@\s]+@[^@\s]+\.[^@\s]+$`, which is ambiguous because `[^@\s]` matches "." as well: the domain half can split at any dot, so a rejecting address makes the engine retry every split and rescan to the end each time. Taint path is ui/roles_page.py (st.text_input) -> assign_role -> _normalize_email -> match(). Reproduced with `"a@" + "a."*n + "@"` (the trailing "@" survives the strip in _normalize_email, the same shape as the existing two@@example.com case): 16 KB took 1.97s, 32 KB took 12.3s, 40 KB took 17.8s of blocked server thread. Reachability is admin-only, but Streamlit runs the script in the server process, so one paste stalls a thread. Two independent defences: 1. Every atom now excludes "." (`^[^@\s]+@[^@\s.]+(?:\.[^@\s.]+)+$`), forcing each split at a literal dot and matching in linear time — the same inputs now take 1.9ms / 5.5ms / 8ms. 2. An RFC 5321 254-character cap is checked before the regex, so the work stays bounded however the pattern is edited later. 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. All pre-existing accept/reject cases are unchanged. Note on the tests: the timing guard asserts on _EMAIL_SHAPE directly rather than through assign_role. Code review caught that routing it through assign_role made it vacuous — the length cap short-circuits the `or`, so the regex is never reached (0 invocations) and the test passed even with the ambiguous pattern restored. Testing the pattern directly keeps defence 1 guarded independently of defence 2; a second test pins the short-circuit ordering that makes the cap a real bound. Both were mutation-checked: reverting either protection fails the corresponding test. py/clear-text-storage-sensitive-data (false positive) — the SARIF flow names normalize_secret_safe_json() as the sensitive-data source. CodeQL's classifier is name-based, so the substring "secret" makes it treat the return value as a secret; that function is the redactor, so the alert flags the sanitizer's own output. What is written is the HMAC-signed envelope with already-redacted provenance, carrying the signature and never the signing key. Renaming a public boundary function with 55 references across 16 files to satisfy a substring match was considered and rejected; the analysis is recorded in the audit register and the alert is dismissed on GitHub as a false positive. Gates: pytest 1935 passed / 1 skipped, coverage 89.79% (floor 89), ruff clean, mypy clean over 257 files, bandit clean, pip-audit clean, compileall clean. Co-Authored-By: Claude Opus 5 --- backend/admin/roles_service.py | 18 +++++++- docs/architecture/audit-2026-06.md | 73 ++++++++++++++++++++++++++++++ tests/test_admin_roles_service.py | 65 ++++++++++++++++++++++++++ 3 files changed, 154 insertions(+), 2 deletions(-) diff --git a/backend/admin/roles_service.py b/backend/admin/roles_service.py index 27699ce..7676cb3 100644 --- a/backend/admin/roles_service.py +++ b/backend/admin/roles_service.py @@ -81,7 +81,21 @@ 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.]+)+$") + +# RFC 5321 caps a full address at 254 characters. Checking the length before the +# regex bounds the work no matter how the pattern is edited later — cheap belt +# and braces so a future loosening cannot quietly reintroduce a ReDoS. +_MAX_EMAIL_LENGTH = 254 def _guard_last_admin(locked_admins: list[Any]) -> None: @@ -116,7 +130,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/docs/architecture/audit-2026-06.md b/docs/architecture/audit-2026-06.md index 287e44e..eecb8dd 100644 --- a/docs/architecture/audit-2026-06.md +++ b/docs/architecture/audit-2026-06.md @@ -275,3 +275,76 @@ 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 an RFC 5321 254-character cap checked *before* +the regex so the work stays bounded however the pattern is edited later. + +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_assign_email_check_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. + + What is actually written is the HMAC-signed envelope + `{schema_version, prompt_version, verdict, provenance}` where `provenance` is + already redacted, and the only crypto material present is the **signature** + (`integrity_hmac_sha256`), never the signing key — see + `backend/ai_cache_integrity.py`. No secret is persisted. + + **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..9c9cc38 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 RFC 5321 254-character cap. + "a" * 250 + "@example.com", ], ) def test_assign_rejects_invalid_email(file_session_factory, bad_email): @@ -90,6 +105,56 @@ def test_assign_rejects_invalid_email(file_session_factory, bad_email): ) +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-character cap there 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] = [] + + class _RecordingPattern: + def match(self, value: str): + calls.append(value) + return roles_service._EMAIL_SHAPE.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. From 8ec23fbe055fb8503cbb1fce29ee69700e8cdf64 Mon Sep 17 00:00:00 2001 From: DoRmAmMu1997 Date: Tue, 1 Sep 2026 08:44:01 +0530 Subject: [PATCH 2/2] SEC-003: harden regression coverage and audit accuracy Capture the original email matcher before replacing it with the ordering test double so a regressed guard produces the intended assertion instead of recursive test-helper failure. Pin the inclusive 254-code-point boundary with realistic shape-valid addresses on both sides. Clarify that the bound is an application work limit rather than full RFC/octet validation, align the storage-capacity comment, correct the audit's test name, and describe the CodeQL false-positive path without overstating best-effort redaction. Co-authored-by: Codex --- backend/admin/roles_service.py | 8 +++-- backend/storage/models.py | 6 ++-- docs/architecture/audit-2026-06.md | 19 +++++++---- tests/test_admin_roles_service.py | 55 ++++++++++++++++++++++++++++-- 4 files changed, 73 insertions(+), 15 deletions(-) diff --git a/backend/admin/roles_service.py b/backend/admin/roles_service.py index 7676cb3..7a5de6a 100644 --- a/backend/admin/roles_service.py +++ b/backend/admin/roles_service.py @@ -92,9 +92,11 @@ def _normalize_email(email: str) -> str: # "a@b.c.") are now rejected too, exactly the typo'd grants SEC-001 targets. _EMAIL_SHAPE = re.compile(r"^[^@\s]+@[^@\s.]+(?:\.[^@\s.]+)+$") -# RFC 5321 caps a full address at 254 characters. Checking the length before the -# regex bounds the work no matter how the pattern is edited later — cheap belt -# and braces so a future loosening cannot quietly reintroduce a ReDoS. +# 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 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 eecb8dd..1d58910 100644 --- a/docs/architecture/audit-2026-06.md +++ b/docs/architecture/audit-2026-06.md @@ -307,13 +307,15 @@ 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 an RFC 5321 254-character cap checked *before* -the regex so the work stays bounded however the pattern is edited later. +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_assign_email_check_stays_linear_on_pathological_input`. +`test_email_shape_pattern_stays_linear_on_pathological_input`. ### Findings verified FALSE (do not re-flag) @@ -329,11 +331,14 @@ cases are unchanged, and the timing property is pinned by persistence boundary that masks credential-named keys and pushes strings through `redact_text`. CodeQL is flagging the sanitizer's own output. - What is actually written is the HMAC-signed envelope - `{schema_version, prompt_version, verdict, provenance}` where `provenance` is - already redacted, and the only crypto material present is the **signature** + 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`. No secret is persisted. + `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 diff --git a/tests/test_admin_roles_service.py b/tests/test_admin_roles_service.py index 9c9cc38..5c09791 100644 --- a/tests/test_admin_roles_service.py +++ b/tests/test_admin_roles_service.py @@ -93,7 +93,7 @@ def test_assign_same_role_is_noop_without_audit(file_session_factory, monkeypatc # 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 RFC 5321 254-character cap. + # SEC-003: longer than the 254-code-point application cap. "a" * 250 + "@example.com", ], ) @@ -105,6 +105,51 @@ 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. @@ -114,7 +159,7 @@ def test_email_shape_pattern_stays_linear_on_pathological_input(): 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-character cap there rejects any input long enough + ``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 @@ -138,11 +183,15 @@ def test_assign_rejects_over_length_email_before_running_the_pattern( 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) - return roles_service._EMAIL_SHAPE.match(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())