fix(dashboard): audit pre-audit refusals by middleware position - #7545
Conversation
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSDesign-level review of The claims verify: five raise sites in the dashboard chain, Design-Verdict: PASS Converts a remembered-helper-call convention into a positional guarantee at the right layer, with the fail direction pointing toward duplicate records, not lost ones. WatchThe positional guarantee covers only raised refusals — "Returned responses are NOT inspected" means a future barrier that returns a 403 without auditing recreates today's hole in the other shape, guarded only by the same class of source pin this PR retired. Disclosed in the docstring, and extending to returned responses is an audit-surface ruling (handler-level GET 403s) that rightly stays a follow-up — but the issue owner deciding #2588 should weigh it explicitly. [DESIGN-REVIEWED] daefef5 |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All counts verified: exactly five First-Principles-Verdict: PASS A reported deny-or-audit defect (#2539/#2588) fixed at cause level — the guarantee moves from a per-site convention to middleware position — with honest, verifiable counts. What this change shipsIntent: make every refused request leave an audit record without depending on each deny site remembering a helper call — a FIX (item 2 of #2588).
The deeper alternative (registering Watch
[FIRST-PRINCIPLES-REVIEWED] daefef5 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
9c1223e to
1708cee
Compare
|
Dispositions for round 1 on GPT 5.6 (BLOCKING) - fixed at the cause"GET permission refusals are claimed without an audit event." Correct, and the Adopted the prescribed fix - the claim now sits inside the audited branch on both Net behaviour change vs the previous revision: Pinned so it cannot regress: the wiring pin now asserts the claim appears AFTER Design Review, Watch 1 (the return-idiom half) - accepted and deferred, with the countReal, and the reasoning about The raise-shaped half is closed differently, and this is why the asymmetry is The cheaper alternative offered - pin that barriers raise rather than return - is Design Review, Watch 2 (private claim key) - fixedThis is the double-log hole above, and the fix is the one suggested: the marker is First Principles, Watch (three WS-origin refusals stay unaudited) - now closedThe grep and the three sites are accurate for Opus 4.8 - no findings, nothing to dispositionAlso on this head
|
1708cee to
2580926
Compare
|
Round 2 on First Principles, Subtraction (drop the
|
2580926 to
9f26d19
Compare
|
Round 3 on GPT 5.6 (BLOCKING) - fixed, remedy widenedThe mechanism holds. Adopted with one change:
Shipped: Pinned by 87 targeted tests pass; flake8, isort and the black gate clean. |
|
Round 4 on First Principles, Watch (attribution absent from the description) - fixedCorrect: the caller-attribution change landed in round 3 and the body still Design Review, Suggestion (pin that returned deny statuses come only from
|
|
The two red backend shards on Failing test, identically on Evidence it is not mine:
The main-side fix is already in flight: #7554 ("fix(ci): route pip stderr logs So this PR waits rather than folding the fix in - the census belongs to that |
sel_audit_middleware is registered inner to the Host, CSRF and token barriers, so a refusal one of them raises produces a 403 the audit middleware never observes. The three known sites each call _audit_denied themselves and a source-string test pins that they keep doing so, but a pin only catches what someone remembers to run and the omission is invisible in production: the refusal appears in no log at all. Add a deny-audit boundary built by a shared factory and registered outer to every barrier on both entrypoints. It catches a raised 401/403 on the way out and records it through the same off-loop, best-effort helper unless a layer already claimed the request, so the guarantee is positional and a future deny site that forgets everything is still audited. A layer claims exactly when it wrote the specific record itself: _audit_denied for the two barriers, sel_audit_middleware for the mutating /api/ requests it actually logs, and the two WebSocket-origin handlers that log their own denial. The claim marker lives in origin.py because those handlers cannot import server.py without a cycle. Claiming a request the audit middleware logs nowhere would promise an audit nobody writes, so the claim sits inside its method/path branch; the one refusal that now reaches the boundary unclaimed is ws.py's cross-origin WebSocket 403, which audited nothing of its own and was recorded nowhere before. token_auth_middleware returns its 401/403 rather than raising and audits each itself, and returned responses are not inspected. Only 401 and 403 are refusals: a 302 from host canonicalization and a 404 from routing pass through untouched. Refs #2588
9f26d19 to
daefef5
Compare
|
Final state on Design Review, Watch (returned refusals stay convention-held) - accepted, deferred to the #2588 rulingFourth framing of one residue, and the verdict text itself says extending to First Principles, Watch (401 in
|
bolichen97
left a comment
There was a problem hiding this comment.
Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.
Problem / Motivation
sel_audit_middlewareis registered INNER to the Host, CSRF and token barriers onboth entrypoints, so a refusal one of those barriers raises never reaches it: the
client gets a 403 that appears in no audit record. Three deny sites paper over
that today by each calling the shared
_audit_denied()helper, and asource-string test (
test_every_middleware_denial_is_audited_off_the_loop) failsif one of them drops the call.
That pin only catches what someone remembers to run, and the omission it guards
is invisible in production: a fourth barrier that raises a bare 403 loses the
record silently. This is item 2 of #2588, the deny-before-audit remainder. Item 1
(re-resolving
sel_hmac_key_path()) shipped as #7036 and is untouched here.Why it matters
An unaudited refusal is the deny-or-audit violation #2539 reported, and the
refusals in question are the security-relevant ones: a DNS-rebinding attempt with
a forged
Host, a cross-origin mutating request, a cross-origin WebSocketupgrade. If a future barrier, or a future arm of an existing one, lands without
the helper call, the gateway starts refusing requests that leave no trace and
nothing in production says so.
What changed (motivation, approach, change)
Symptom: a pre-audit refusal is recorded only because the deny site remembered to
record it. Root cause: the guarantee is a convention held by a source-string pin,
in a position where the audit middleware cannot observe the refusal at all.
Change: put a middleware where it can.
_make_deny_audit_middleware(caller)is a shared factory (like the Host and CSRFbarriers, so the two entrypoints cannot drift) registered OUTER to every barrier
that can refuse. It catches a raised
HTTPExceptionand, when the status is arefusal and no layer has CLAIMED the request, records it through the same
_audit_denied()helper: off the loop, best-effort, refusal re-raised unchanged.Forgetting the per-site call now costs the record's reason DETAIL, not the record.
A layer claims exactly when it wrote the specific record itself, and claiming is
the only thing that suppresses the boundary:
_audit_denied();sel_audit_middleware, for the mutating/api/requests it actually logs, soits
outcome="error"entry for a handler's 403 is not doubled. The claim sitsINSIDE that method/path branch on purpose: claiming a request it logs nowhere
would promise an audit nobody writes (GPT round 1 caught exactly this - the
first revision claimed unconditionally, which would have kept a cross-origin
WebSocket GET refused in its handler silently unaudited);
(
stt_stream.py:1050,handlers/terminal.py:586), via the neworigin.mark_audit_claimed(request). The marker lives inorigin.pyratherthan
server.pybecause those two are handlers and importingserverfrom ahandler is a cycle - the same reason
check_originitself lives there.Not claiming is the safe direction. There are exactly five
raise web.HTTPForbidden/HTTPUnauthorizedsites insrc/kiro_crew/: the twobarriers and the three WebSocket-origin refusals. Two of the three audit
themselves and now claim; the third,
ws.py's_check_ws_origin, audits nothingof its own, so it is the ONE record class this PR adds - and it is the class the
issue is about, a cross-origin WebSocket upgrade refused with no trace. No new
volume from pollers:
token_auth_middlewareRETURNS its 401/403 rather thanraising and audits each with its own reason code, and returned responses are not
inspected. Only 401 and 403 count; a 302 from host canonicalization and a 404 from
routing pass through untouched. The boundary is inner to the latency middleware
only, so that one's "times the FULL in-gateway handling" contract still holds.
The record is attributed to whoever was refused, not to the factory's label.
token_auth_middlewareruns inner to the boundary and setsrequest["user"]/request["app"]on every authenticated path, so a refusal raised below it arriveswith an identity: the boundary passes
request.get("app") or request.get("user") or caller.request["app"]is""forthe dashboard user and
token_authtreats that present-empty claim as POSITIVEproof of them, so an empty app falls through to the user rather than to the label,
and the static
callerremains only the pre-auth fallback. Without this an app'sor an operator's WebSocket-origin refusal would be filed under
dashboard_user-the attribution
handlers/terminal.pyalready avoids at its own deny site byreading
request.get("user").The issue proposed registering
sel_audit_middlewareitself outermost. I did not,and the reason is on the issue: that variant double-logs every refusal the three
sites already record, and the two entrypoints do not audit the same method sets,
so it is a change to the audit surface that needs a ruling (which is why the issue
carries
needs-human). The boundary reaches the same guarantee with asingle, enumerable record class added instead.
Both specs that state the chain move with the code:
dashboard-token-auth.mdhadthe old order in a diagram, a code snippet and two prose chains, and
security.mdclaimed the Host barrier is registered "second".Tests
Targeted, in
test/test_api_health.py(where the existing pin and the real-chainHost tests live) and
test/test_dashboard_server_startup_coverage.py:test_a_forgetful_pre_audit_refusal_is_still_audited_by_position- the fourthdeny site written the way the pin cannot catch: a barrier that raises a bare
403 and audits nothing. Asserts the record appears, names the method and path,
and was written OFF the event loop (not
MainThread), and that the 403 stillreaches the client.
test_a_barrier_that_audits_itself_is_not_recorded_twice- the real Hostbarrier through the boundary: exactly one record, and it is the site's own
(naming the offending header), not the generic one.
test_a_refusal_the_audit_middleware_logs_itself_is_not_doubled- a mutating/api/handler 403 under a claiming audit layer yields no boundary record.test_a_refusal_the_audit_middleware_does_not_log_is_recorded_here- the otherhalf: a GET refused in its handler, which that middleware logs nowhere, IS
recorded by the boundary. These two together are the contract GPT's finding was
about.
test_every_self_auditing_raised_refusal_claims_the_request- walks everyraise web.HTTPForbidden/HTTPUnauthorizedinsrc/kiro_crew/and assertseach site that writes its own denial audit also claims, so a future
self-auditing deny site cannot silently double-log. Enumeration, not a list.
test_the_record_names_the_authenticated_caller_not_the_static_label- allthree attribution cases through a real chain: app token files under the app,
dashboard session under the user, no identity under the static label.
test_the_boundary_ignores_outcomes_that_are_not_refusals- a 302 and a 404audit nothing.
test_an_audit_failure_never_turns_the_refusal_into_a_500-sel()raisingstill returns the 403.
test_every_middleware_denial_is_audited_off_the_loop: both auditmiddlewares must claim, and the claim must appear AFTER the method/path guard
(an index comparison on the source, so re-widening the claim fails the pin).
Added
test_both_servers_install_the_shared_deny_audit_boundary(bothentrypoints build AND register it), plus the real-chain order assertion in
test_the_middleware_chain_is_ordered_outermost_first.Mutation-verified rather than asserted. With base
server.pyrestored under thenew tests, exactly 7 fail and all 25 pre-existing tests pass. With the PREVIOUS
revision's
server.py+stt_stream.pyrestored, the two new pins fail withtheir own messages (claim-before-guard, and stt_stream not claiming) while the
rest pass. Removing the boundary from the chain list alone fails the order
assertion. 381 passed across
test_api_health.py,test_dashboard_server_startup_coverage.py,test_teams_webhook_hardening.py,test_stt_stream.pyandtest_terminal_handler.py. flake8, isort and the blackbaseline gate are clean on the diff.
Manual verification
N/A - unit coverage sufficient: the change is a middleware-chain property, and
every claim above is driven through a real aiohttp chain over the wire, including
the off-loop assertion and the client-visible status.
Related Issues
Refs #2588
Refsrather thanCloses, deliberately. Item 1 shipped in #7036 and this isitem 2's goal, so the issue's substance is addressed - but item 2 was labelled
needs-humanfor a specific ruling ("shouldsel_audit_middlewarebe registeredoutermost"), and this PR argues that ruling is no longer needed rather than
making it. Whoever owns that label should close the issue, or say they still want
the outermost variant along with its double-log retirement and the unification of
the two audit method sets. One thing this deliberately does NOT change, so it is
not silently claimed as done: the dashboard audits POST/PUT/DELETE/PATCH while the
headless server also audits GET. That divergence is not a deny-before-audit gap,
and unifying the two sets is still its own decision.
Pattern harvest
Rule candidate: review-prompt
Pattern: a security guarantee held by a source-string test rather than by
structure. The pin proved the three known sites still comply and could say
nothing about the fourth, because the thing it guards is an ABSENCE at a site
that does not exist yet. Review prompt: when a test's failure message reads "X no
longer does Y", ask whether position, a type, or a chokepoint could make Y
unnecessary instead of pinned. Not a semgrep rule for the same reason the pin was
weak: no pattern over current source can match a site nobody has written. The
second-order lesson from this PR's own review: when a positional guarantee is
suppressed by a marker, the marker must be set by whoever actually wrote a record