Skip to content

fix(dashboard): audit pre-audit refusals by middleware position - #7545

Merged
bolichen97 merged 1 commit into
mainfrom
fix/deny-before-audit
Sep 1, 2026
Merged

fix(dashboard): audit pre-audit refusals by middleware position#7545
bolichen97 merged 1 commit into
mainfrom
fix/deny-before-audit

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

sel_audit_middleware is registered INNER to the Host, CSRF and token barriers on
both 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 a
source-string test (test_every_middleware_denial_is_audited_off_the_loop) fails
if 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 WebSocket
upgrade. 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 CSRF
barriers, so the two entrypoints cannot drift) registered OUTER to every barrier
that can refuse. It catches a raised HTTPException and, when the status is a
refusal 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:

  • the two barriers, through _audit_denied();
  • sel_audit_middleware, for the mutating /api/ requests it actually logs, so
    its outcome="error" entry for a handler's 403 is not doubled. The claim sits
    INSIDE 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);
  • the two WebSocket-origin handlers that log their own denial
    (stt_stream.py:1050, handlers/terminal.py:586), via the new
    origin.mark_audit_claimed(request). The marker lives in origin.py rather
    than server.py because those two are handlers and importing server from a
    handler is a cycle - the same reason check_origin itself lives there.

Not claiming is the safe direction. There are exactly five
raise web.HTTPForbidden/HTTPUnauthorized sites in src/kiro_crew/: the two
barriers and the three WebSocket-origin refusals. Two of the three audit
themselves and now claim; the third, ws.py's _check_ws_origin, audits nothing
of 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_middleware RETURNS its 401/403 rather than
raising 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_middleware runs inner to the boundary and sets request["user"] /
request["app"] on every authenticated path, so a refusal raised below it arrives
with an identity: the boundary passes
request.get("app") or request.get("user") or caller. request["app"] is "" for
the dashboard user and token_auth treats that present-empty claim as POSITIVE
proof of them, so an empty app falls through to the user rather than to the label,
and the static caller remains only the pre-auth fallback. Without this an app's
or an operator's WebSocket-origin refusal would be filed under dashboard_user -
the attribution handlers/terminal.py already avoids at its own deny site by
reading request.get("user").

The issue proposed registering sel_audit_middleware itself 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 a
single, enumerable record class added instead.

Both specs that state the chain move with the code: dashboard-token-auth.md had
the old order in a diagram, a code snippet and two prose chains, and
security.md claimed the Host barrier is registered "second".

Tests

Targeted, in test/test_api_health.py (where the existing pin and the real-chain
Host tests live) and test/test_dashboard_server_startup_coverage.py:

  • test_a_forgetful_pre_audit_refusal_is_still_audited_by_position - the fourth
    deny 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 still
    reaches the client.
  • test_a_barrier_that_audits_itself_is_not_recorded_twice - the real Host
    barrier 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 other
    half: 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 every
    raise web.HTTPForbidden/HTTPUnauthorized in src/kiro_crew/ and asserts
    each 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 - all
    three 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 404
    audit nothing.
  • test_an_audit_failure_never_turns_the_refusal_into_a_500 - sel() raising
    still returns the 403.
  • Extended test_every_middleware_denial_is_audited_off_the_loop: both audit
    middlewares 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 (both
    entrypoints 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.py restored under the
new tests, exactly 7 fail and all 25 pre-existing tests pass. With the PREVIOUS
revision's server.py + stt_stream.py restored, the two new pins fail with
their 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.py and test_terminal_handler.py. flake8, isort and the black
baseline 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

Refs rather than Closes, deliberately. Item 1 shipped in #7036 and this is
item 2's goal, so the issue's substance is addressed - but item 2 was labelled
needs-human for a specific ruling ("should sel_audit_middleware be registered
outermost"), 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

  • setting it to preserve a scope boundary makes it a promise nobody keeps.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of daefef51c11902d908a69eb7f34d3a466ceddbed and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] daefef5

False positive or not applicable? A repository writer can comment:
/ai-review override gpt daefef51c11902d908a69eb7f34d3a466ceddbed: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of daefef51c11902d908a69eb7f34d3a466ceddbed — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The claims verify: five raise sites in the dashboard chain, sel_audit_middleware logs both arms of the branch it claims, and md_notebook's exceptions live in its own web.Application with its own middleware, outside these chains. The design gate checks out — position replaces convention at the root cause, claim-marker failure direction is duplicate records (safe), both entrypoints share one factory, specs move in the same commit, and the rejected alternative (registering sel_audit_middleware outermost) is argued with the double-log consequence on the issue.

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.

Watch

The 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

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of daefef51c11902d908a69eb7f34d3a466ceddbed — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All counts verified: exactly five raise web.HTTPForbidden/HTTPUnauthorized sites in src/kiro_crew/ (server.py:609,669; ws.py:316; terminal.py:589; stt_stream.py:1053), ws.py's site audits nothing of its own, and token_auth returns its 403s rather than raising. The description's claims match the diff. Final review:

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 ships

Intent: make every refused request leave an audit record without depending on each deny site remembering a helper call — a FIX (item 2 of #2588).

  1. A refused cross-origin WebSocket upgrade now appears in the audit log — justified; this is the reported defect.
  2. Any barrier's raised 401/403 is audited by position, even a forgetful future one — the fix, cause-level.
  3. Refusal records name the authenticated app/user, not a static label — declared, justified.
  4. Self-auditing deny sites claim their request so no refusal is recorded twice — justified; audit surface widens by exactly one record class.
  5. Both entrypoint chains gain the outermost deny-audit layer, specs updated same commit — mandated by AGENTS.md.
  6. New origin.mark_audit_claimed/AUDIT_CLAIMED_KEY callable by other code — declared; counted 5 callers, 1 reader (the boundary).

The deeper alternative (registering sel_audit_middleware itself outermost) was considered and declined with a reason recorded on the issue; the description names the level this fix sits at. The "exactly five raise sites" claim and the "token_auth returns, never raises" claim both survive an independent grep. Zero unaudited returned-403 middleware sites exist today, so no unfixed siblings.

Watch

_PRE_AUDIT_DENY_STATUSES includes 401 with zero producers today (grep: no raise web.HTTPUnauthorized( in src/kiro_crew/) — the diff declares this ("Nothing raises 401 today"). Cost is one set element and it closes the exact class this PR exists for, so it stays; noting only so the declaration is on record.

[FIRST-PRINCIPLES-REVIEWED] daefef5

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed daefef51c11902d908a69eb7f34d3a466ceddbed — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] daefef5

Verdict parsed from the review's SHA-scoped output markers for commit daefef51c11902d908a69eb7f34d3a466ceddbed.

False positive or not applicable? A repository writer can comment:
/ai-review override fable daefef51c11902d908a69eb7f34d3a466ceddbed: <one-sentence reason>

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Dispositions for round 1 on 9c1223eed, all addressed in 1708cee5c.

GPT 5.6 (BLOCKING) - fixed at the cause

"GET permission refusals are claimed without an audit event." Correct, and the
mechanism is exactly as described: request[_AUDIT_CLAIMED_KEY] = True ran before
the method/path guard, so a cross-origin WebSocket GET refused in its handler was
claimed by a middleware that logs nothing for GET, and the boundary then stood
down. The claim was a promise nobody kept.

Adopted the prescribed fix - the claim now sits inside the audited branch on both
entrypoints - and closed the double-log hole that narrowing opens, which is the
half the prescription did not cover. Three of the tree's five raised refusals are
WebSocket-origin handlers; two of them (stt_stream.py:1050,
handlers/terminal.py:586) already write their own denial record, so with the
claim narrowed they would have been logged twice. They now claim through a new
origin.mark_audit_claimed(request). The marker moved from server.py to
origin.py because those two are handlers and importing server from a handler
is a cycle - the same reason check_origin lives there.

Net behaviour change vs the previous revision: ws.py's _check_ws_origin 403,
which audits nothing of its own, is now recorded. It was recorded nowhere before,
on this branch or on main.

Pinned so it cannot regress: the wiring pin now asserts the claim appears AFTER
the method/path guard (source index comparison, so re-widening fails), and a new
test walks every raise web.HTTPForbidden/HTTPUnauthorized in src/kiro_crew/
asserting each site that writes its own denial audit also claims. Both were
mutation-verified against the previous revision's files and fail there with their
own messages.

Design Review, Watch 1 (the return-idiom half) - accepted and deferred, with the count

Real, and the reasoning about token_auth being the in-repo template a copier
reaches for is right. Not folded in, and here is the measurement rather than an
assertion: src/kiro_crew/dashboard/ has 246 sites that RETURN a 401/403
response. Inspecting returned responses puts all 246 under the boundary, and only
the token_auth subset audits itself, so the claim-based dedupe that works for
the raise-shaped population does not transfer - the rest would become new audit
records. That is precisely the audit-surface change #2588 reserves for a human
ruling, so folding it in here would smuggle the ruling into a scoped fix.

The raise-shaped half is closed differently, and this is why the asymmetry is
defensible rather than arbitrary: that population is FIVE sites and the new test
enumerates it by walking the tree, so it cannot drift silently. The return-shaped
population cannot be enumerated the same way.

The cheaper alternative offered - pin that barriers raise rather than return - is
worth doing and I have deliberately not done it in this push: this PR just
converged on a blocking finding and every push re-rolls five non-deterministic
lanes. The contract is now stated in _make_deny_audit_middleware's docstring and
in security.md. Say the word and I will add the pin here; otherwise it belongs
with the ruling on the return-shaped population, since a pin without that decision
only records a convention rather than closing anything.

Design Review, Watch 2 (private claim key) - fixed

This is the double-log hole above, and the fix is the one suggested: the marker is
no longer private to server.py. origin.AUDIT_CLAIMED_KEY +
origin.mark_audit_claimed(request) are importable by any barrier, and the two
out-of-module deny sites that audit themselves now use them.

First Principles, Watch (three WS-origin refusals stay unaudited) - now closed

The grep and the three sites are accurate for 9c1223eed. As of 1708cee5c all
three are covered: stt_stream.py and handlers/terminal.py keep their own
specific record (and claim, so it stays single), and ws.py's refusal - the one
that audited nothing - is recorded by the boundary. So the concrete refusals the
review flagged for whoever rules on #2588 no longer need that ruling; what remains
deferred is only the method-set divergence, which is not a deny-before-audit gap.

Opus 4.8 - no findings, nothing to disposition

Also on this head

Gateway Tests (macOS) failed on 9c1223eed in
test_pod_e2e_harness_paths.py::test_health_refuses_a_foreign_port_holder_and_names_the_conflict
(expected HEALTHY=0 FOREIGN=1, got FOREIGN=0). Not caused by this diff: that
test deliberately parks a foreign process on the pod's port so the pod never
starts, which means the dashboard app - the only thing this PR touches - never
serves a request in it; the assertion that failed is the harness's foreign-holder
detection. The same job is green on the other open PR heads I checked. Watching it
on the new head.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 2 on 1708cee5c - all four lanes PASS. Both advisory items dispositioned;
one is fixed in 25809268b.

First Principles, Subtraction (drop the _AUDIT_CLAIMED_KEY alias) - fixed

Right, and taken further in the same direction: the key now has ONE name and the
write has ONE spelling.

  • deleted the _AUDIT_CLAIMED_KEY = AUDIT_CLAIMED_KEY alias and its 10-line
    comment (that comment restated what origin.AUDIT_CLAIMED_KEY's own docstring
    already says);
  • the boundary READS AUDIT_CLAIMED_KEY directly, imported at server.py:124;
  • all four in-module claim sites now WRITE through origin.mark_audit_claimed()
    rather than assigning the key, so there is no raw-key write left anywhere
    outside origin.py. mark_audit_claimed goes from 2 consumers to 6, which is
    the shape that made the helper worth having;
  • the wiring pin and the two test stand-ins follow the same spelling, so the
    guard-order assertion (claim must appear AFTER the method/path guard) still
    fails if the claim is re-widened.

381 targeted tests pass; flake8, isort and the black gate clean.

Design Review, Suggestion (returned refusals / token_auth claiming) - accepted, deferred to the ruling on #2588

Same residue as round 1's Watch 1, and the answer is unchanged, so rather than
repeat it: src/kiro_crew/dashboard/ holds 246 sites that RETURN a 401/403, only
the token_auth subset audits itself, and auditing the rest is the audit-surface
change #2588's needs-human label owns. Having token_auth claim is the easy
half; the hard half is that every OTHER returned 403 then becomes a new audit
record, which is a decision and not a refactor.

Recorded on the issue so it travels with that ruling rather than living only in
this thread, and captured locally for re-judgement on a clock. If the maintainer
wants the cheap partial now - token_auth claims, boundary still ignores returns -
say so and it is a two-line change; on its own it buys nothing, since the boundary
would still not be looking at returned responses.

The asymmetry is deliberate and measurable, which is why I am comfortable shipping
it: the RAISE-shaped population is five sites and a test walks the tree to keep it
that way, so it cannot drift silently. The return-shaped population cannot be
enumerated the same way.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 3 on 25809268b - GPT's attribution finding is real. Fixed in 9f26d191b,
with one correction to the prescribed remedy.

GPT 5.6 (BLOCKING) - fixed, remedy widened

The mechanism holds. token_auth_middleware runs INNER to the boundary and sets
request["user"] / request["app"] on every authenticated path, so a refusal
raised below it - ws.py's cross-origin WebSocket 403, the one refusal that
reaches the boundary unclaimed - arrives with an identity on the request while the
boundary was filing it under the factory's static caller. handlers/terminal.py
already avoids exactly this by reading request.get("user") at its own deny site,
so the boundary was the odd one out.

Adopted with one change: request.get("app") or caller loses the dashboard user.
request["app"] is "" for them, and token_auth documents that a PRESENT empty
claim is POSITIVE proof of the dashboard user - several call sites read it that way

  • so an empty app must fall through to request["user"], not to the label. A
    normal operator's cross-origin WebSocket refusal would otherwise still have been
    recorded as dashboard_user, which is the same loss one rung down.

Shipped: request.get("app") or request.get("user") or caller. App token files
under the app, dashboard session under the user, and a refusal that happened before
any identity existed (the Host / CSRF barriers, if they ever stopped claiming)
still falls back to the static label.

Pinned by test_the_record_names_the_authenticated_caller_not_the_static_label,
which drives all three cases through a real chain and asserts the recorded
caller. Mutation-verified: with the previous revision's server.py it fails on
the first case with recorded as dashboard_user.

87 targeted tests pass; flake8, isort and the black gate clean.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Round 4 on 9f26d191b - all four lanes PASS. Both advisory items dispositioned.

First Principles, Watch (attribution absent from the description) - fixed

Correct: the caller-attribution change landed in round 3 and the body still
described the revision before it. Body updated - "What changed" now carries the
request.get("app") or request.get("user") or caller paragraph and the reason an
empty app claim falls through to the user rather than to the label, and "Tests"
names test_the_record_names_the_authenticated_caller_not_the_static_label. No
code change; the shipped behaviour was already what round 3's disposition
described.

Design Review, Suggestion (pin that returned deny statuses come only from token_auth) - accepted, deferred with the ruling

Third framing of the same residue, and this one names a cheaper closure than
"inspect returned responses": a source pin instead of a behaviour change. Worth
having, and still deferred, for one reason - a pin asserting returned 401/403 in
middleware come only from token_auth records the convention; it does not audit
anything. The refusal it would forbid is currently the shape 246 sites in
src/kiro_crew/dashboard/ use, so what the pin is really worth depends on whether
those returned refusals should be audited at all, which is the needs-human
decision on #2588 item 2. Pinning first would freeze one answer to a question
nobody has ruled on.

Recorded on #2588 (comment 5490540450) with the three options the maintainer now
has, and locally as f-20260901-07 for re-judgement on a clock, with this pin
shape added as the cheapest of the three. If the Command Center or a maintainer
wants the pin in this PR instead, it is a small test and I will add it.

GPT 5.6 and Opus 4.8 - no findings on this head, nothing to disposition

CI shards and Opus are still running on 9f26d191b; will report when they conclude.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

The two red backend shards on 9f26d191b are MAIN-OWNED, not from this diff.
Recording the proof so nobody re-derives it.

Failing test, identically on Backend Tests (3.10, 3) and
Backend Tests (Windows) (3):

test_security_posture.py::TestGateSideLogRedactorSpelling::test_no_new_gate_side_log_line_reads_the_baseline_redactor
AssertionError: New gate-side log/audit line(s) reading the BASELINE redactor
dashboard/handlers/memory.py: 2 sites, census says 0.

Evidence it is not mine:

  1. dashboard/handlers/memory.py and test_security_posture.py are not in this
    PR's diff. The eight files it touches are server.py, origin.py,
    stt_stream.py, handlers/terminal.py, two specs and two test modules.
  2. Reverting this PR's ENTIRE diff to its base (671528203) and running that one
    test reproduces the same failure, so the ratchet is red on main's own tree.
  3. It is also red at 671528203's parent (c0dca4a8b), so the base commit did not
    introduce it either. The 2 uncounted sites came in with
    2e627dc10 fix(memory): redact pip stderr before logging install failures (#7283).
  4. Other open PR heads show the same failing shards.

The main-side fix is already in flight: #7554 ("fix(ci): route pip stderr logs
through the context redactor") touches exactly
src/kiro_crew/dashboard/handlers/memory.py.

So this PR waits rather than folding the fix in - the census belongs to that
change, and raising the baseline number here would claim a decision about the
memory handler that is not mine to make. Once #7554 lands I will rebase onto
settled main to cut a fresh merge ref, which also re-rolls the review lanes; all
five are PASS on the current head.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 1, 2026
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
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 1, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Final state on daefef51c: 62 checks green, 8 skipped, 0 red, PR Readiness passed
("Eligible automated validation passed for this revision"), all four review lanes
PASS with their markers naming this head. Closing out the two Watch items that
came with those PASS verdicts.

Design Review, Watch (returned refusals stay convention-held) - accepted, deferred to the #2588 ruling

Fourth framing of one residue, and the verdict text itself says 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." That is exactly what was done: issue comment 5490540450 states the
residue, the measurement that decides its cost (246 sites in
src/kiro_crew/dashboard/ RETURN a 401/403, of which only the token_auth subset
audits itself), and the three options a maintainer now has - accept
positional-for-raised and close, extend to returned refusals, or pin structurally
that a barrier must raise. Also held locally as f-20260901-07 for re-judgement
on a clock. No code change here, deliberately: closing it either way is the
decision the needs-human label exists for.

First Principles, Watch (401 in _PRE_AUDIT_DENY_STATUSES has no producers today) - no change, concurred

Accurate, and the review reaches the same conclusion it would ask for: "Cost is one
set element and it closes the exact class this PR exists for, so it stays; noting
only so the declaration is on record." The declaration it refers to is in the
module comment on the constant and in the PR body. Recording the disposition so the
concern is not left silent rather than because anything is outstanding.

Everything else from this PR's five rounds

  • GPT round 1 (unconditional claim swallowed unlogged-GET refusals): fixed, plus
    the double-log hole narrowing opened, in 1708cee5c.
  • GPT round 2 (refusal filed under the static caller label): fixed in 9f26d191b,
    with the remedy widened to cover the dashboard user.
  • First Principles subtraction (duplicate key spelling): fixed in 25809268b.
  • First Principles Watch (attribution missing from the description): body updated.
  • First Principles Watch (three WS-origin refusals unaudited): closed by the fix -
    two claim their own record, the third is now audited by position.
  • Design Review Watch (private claim key): fixed by moving the marker to
    origin.py.
  • Main-owned census red: proven not mine (comment 5490873097), waited, and cleared
    by rebasing onto fix(ci): route memory.py's pip-stderr logs through the context redactor #7572 (ebc0936f2).
  • Backend Tests (3.12, 4) cancelled at a 30-minute job timeout with zero failing
    tests, which made Coverage Gate fail closed on backend-test=cancelled. Re-ran
    that one job; it passed and the gate went green. Same shard was green on other
    open PR heads, and Backend Tests (3.10, 4) - the same file set - passed on this
    head throughout.

Zero /ai-review override used anywhere in this PR.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.

@bolichen97
bolichen97 enabled auto-merge (squash) September 1, 2026 21:27
@bolichen97
bolichen97 merged commit 22ee98d into main Sep 1, 2026
110 of 112 checks passed
@bolichen97
bolichen97 deleted the fix/deny-before-audit branch September 1, 2026 21:28
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants