fix(file_send): add the two missing auth rungs to the Slack leg

[iamwhatever]

Closes #7290.

file_send's Slack leg ran neither of the two authorization rungs the channel leg runs on every send. So an incognito/temporary session that refuses to write a transcript still uploaded local file bytes into a Slack channel or DM, and a profile denying the channels scope refused a Telegram upload while allowing a Slack one — on the leg with the broadest audience and the only leg whose destination a request can name.

This is an authorization change, not a refactor: a send that succeeds today can now be denied.

Rebased onto main after #7293 landed, so both rungs sit in the destination oracle (dashboard/upload_destination.resolve_slack) — the landing site #7290 names — instead of inline in the handler.

Rung shape: the direct-vet precedent

Rung 1 is a direct vet_and_audit("channels", SLACK_NAMESPACE, fail_closed=True) call (upload_destination._slack_egress_permitted), not a channel_transports registry entry. This follows the shipped precedent dashboard/chat_compaction_notice._channel_egress_permitted, whose docstring names this exact case ("Slack is deliberately absent from channel_transports and so never reaches that ladder"). The registry keeps meaning "transports the shared ladder can drive"; no registry change.

Rung 2 is the existing shared predicate upload_gate.uploads_restricted(channel_type="slack") — the same one resolve_channel and the Telegram/Discord renderers' extraction path use. Its persisted_probe is passed in from the handler, matching the module's existing parameter-not-import contract for ambient lookups.

Both run ahead of any destination work, so a denied caller never opens an owner DM and never reads the session map — a refusal leaks nothing about where the file would have gone. Both are Refusals, not Skips: 403 with a machine-readable code (channels_governance_denied / restricted_session) plus the leg's usual SEL denial record, so the caller is told the file did not leave rather than reading success. That is deliberately unlike the channel leg, where "cannot deliver here" is the common case and a skip is correct.

The oracle's rung table is updated: the two NOT RUN (#6060 step 2) rows now read as run on both legs, and the off-the-event-loop row records that these two ceilings are offloaded even though open_dm keeps the rest of the ladder on the loop.

Blast radius honored

• Sessionless owner-DM callers are not muted. An empty X-Session-Key (cron, heartbeat, out-of-band host action reaching the owner-DM fallback) is vetted under HOST_SESSION_KEY (_host), the same sentinel handlers.messaging uses on its channel legs: an empty key classifies as unknown and matches no profile at all, so vetting under it would make host-side governance inert here, while _host is the stable bind target operators attach it to. On an ungoverned host the outcome is unchanged, and the restricted rung reads no slot and no channel privacy mode for such a key.
• The lenient/strict identity asymmetry is inherited, not widened. This leg resolves its caller leniently (_resolve_session_key(), including the /proc ancestor walk) while the channel leg gets the strict key. Both rungs read that same key; neither adds an identity source.
• Fail-closed. A degraded governance evaluation denies; PlatformCompositionError propagates rather than being reported as a routine authorization answer.

Tests

New TestSlackUploadAuthorizationRungs (7 tests). Revert-verified by removing only the two rung blocks while keeping the signature: six fail, and the seventh is the regression guard that must pass on both sides.

• a restricted session is denied the Slack upload
• a governance-denied channels scope denies the leg, and writes exactly one denied SEL record with downstream_service=slack
• the vet names ("channels", "slack") with fail_closed=True under the caller's key
• a degraded governance evaluation denies
• a sessionless owner-DM caller is not muted
• a sessionless caller is vetted under HOST_SESSION_KEY
• both ceilings precede any destination work (no open_dm, no get_slack_link)

Pinned outcomes changed

No shipped Slack outcome changed. Three test setups did, because the new restricted rung reads a dashboard slot that an auto-attribute MagicMock answers as truthy (i.e. restricted) for sessions none of those cases is about:

• TestFileUploadSlotThreading._make_state_with_link — sets state.get_slot.return_value.is_restricted = False, with a comment saying why. Covers its six dashboard:-keyed tests.
• TestDestinationOracleEquivalence._state — same, for the destination-resolution cases.
• TestDestinationOracleEquivalence.test_an_unreadable_credential_store_is_a_skip_not_a_500 — additionally pins vet_and_audit permitted. Its KiroCrewConfig.load patch raises for every reader, including the lazy platform-context build behind the governance vet, so the fail-closed rung would answer 403 before the owner-DM fallback this case is about ever ran.

Gates

pytest (touched surface 116 passed; full suite's 117 reds reproduce identically on a clean origin/main in this environment — 118 in the same file set — and none are in file_send / upload_destination / Slack-upload tests), isort, flake8, mypy, tsc -b, vitest, plus the four diff-scoped gates (brand, changelog-history, focus-cue, harness-parity) run with their base refs exported.

Pattern harvest

Rule candidate: parity test (the repo's existing guard shape), not semgrep — the defect is an ABSENCE on one branch, which a pattern matcher cannot see.

Pattern: a feature with two sibling egress legs where an authorization rung is applied on the leg that reaches a shared ladder and silently skipped on the leg that is deliberately absent from that ladder's registry. Here channel_transports is the registry, _resolve_mirror_target is the ladder, and Slack's non-membership is what made both ceilings vanish from its leg while reading as intentional.

Generalizable guard: for each egress leg in upload_destination, assert the resolver reaches both the channels-scope vet and uploads_restricted. That test would have failed on main before this change and now passes on both legs, so it is a real ratchet rather than a restatement. The broader lesson for reviewers: "not in the registry" is a routing fact, never an authorization exemption — every registry-absent transport needs its rungs restated by direct call, which is exactly the precedent chat_compaction_notice._channel_egress_permitted set and this change follows.

Round 2 — GPT finding fixed at the shared predicate

GPT flagged (correctly) that the restricted-session ceiling can be bypassed after a gateway restart: for a channel-native key it reads privacy_mode's process-local trackers, which only an INBOUND channel message populates, so a turn no inbound message drove — a cron, a webhook resume, a monitor/auto-nudge re-injection, an explicit file_send — reads empty trackers and ships bytes the user's !incognito forbade.

Real, and pre-existing in the shared predicate rather than introduced here: dashboard/handlers/_shared._is_restricted_session already documents this exact restart gap and calls privacy_mode.hydrate to close it.

Fixed in messaging/upload_gate.uploads_restricted — the shared predicate — not in resolve_slack as the finding suggested. A leg-local hydrate would have fixed the Slack leg and left the channel leg and the renderers' extraction path exposed, re-creating the exact asymmetry this PR exists to remove. hydrate is idempotent, allocation-free for an unflagged key, and an in-memory SessionMap read rather than disk, so it is safe on the loop at every decision point.

Tests, in test/test_telegram_parity.py::TestUploadGate:

• a durable incognito flag with empty trackers is DENIED (the restart case) — revert-verified load-bearing: removing only the hydrate call fails this and nothing else
• an unflagged channel key stays ALLOWED — the guard that the restore does not turn the common case into a refusal, passing on both sides

One existing assertion changed: test_hydration_uses_the_post_rotation_key compared the hydrate-call list by equality, which pinned an incidental CALL COUNT rather than the key it names. It is now a set comparison, so it still proves the post-rotation key is restored and the pre-rotation one never is, without failing because a second gate on the same turn also runs the idempotent restore.

Round 3 — rebased onto main, GPT's test-side-effect finding fixed

Rebased onto origin/main 7eb5d8f94 (33 commits). Clean — no conflicts. Two of those commits touch files this PR also edits (#7705 rewrote a chunk of docs/system-specs/modules/security.md, #7678 edited dashboard/handlers/files.py), and neither collided with these hunks. test_security_posture.py (47 tests) passes on the rebased head, so #7705's spec-census changes and this PR's security.md addition coexist.

GPT flagged test/test_telegram_parity.py:1429 under no-test-side-effects: sm.set_flag(...) leaves a SessionMap flush task pending across loop teardown. Real, and confirmed empirically rather than from the finding text — set_flag → _save(), and _save on a thread with a running loop takes the loop.create_task(self._flush_async()) branch, so the async test schedules a debounced flush on its own loop. Running the test alone printed:

Task was destroyed but it is pending!
task: <Task pending name='Task-2' coro=<SessionMap._flush_async() running at src/kiro_crew/session_map.py:568> ...>

Fixed with await sm.aclose() in a finally — aclose is SessionMap's documented retirement path, cancelling the registered task and landing any owed snapshot. The leak count on that test is now 0.

Scoped to the one test that mutates: the sibling test_an_unflagged_channel_key_stays_allowed_after_the_restore constructs a SessionMap but never writes, so _save is never reached and there is no task to retire — verified at 0 leaks without a change. Adding aclose there too would have been cargo cult.

Worth noting for anyone reading the earlier CI: the leak did not reproduce when the whole TestUploadGate class ran, only when that test ran alone — the warning fires at GC time, so scheduling decides whether it surfaces. That is an argument for fixing it rather than waiting for it to fail, since the destroyed task lands on whichever test happens to run next.

Round 4 — the three CI reds were main-side, fixed by rebase

Rebased onto origin/main 9580a8b98 (27 commits). Clean, payload unchanged at 506 insertions across the same 6 files.

Three FAILUREs on the prior head, none in a file this PR touches:

Check	Test	Verdict
Backend Tests (Windows) (3)	test_session_control.py::test_the_created_agent_name_is_sanitized_before_storage	main-side, fixed by #7840
Backend Tests (Windows) (3)	test_session_control.py::test_the_audit_write_does_not_run_on_the_event_loop	main-side, fixed by #7840
Backend Tests (3.10, 2)	test_external_logout_detection.py::TestStoreRelocation::test_a_leftover_default_store_is_not_read_when_relocated	flake, SEL chain-lock contention

Coverage Gate was a cascade of the two backend reds, not an independent failure.

The two test_session_control reds were bisected rather than assumed. test/test_session_control.py run ALONE, with no xdist and no sharding, reproduced both failures on this PR's head — which ruled out shard-membership reshuffling as the cause. The same file then passed on current origin/main and failed on 7eb5d8f94, this PR's own base, with this PR's commit absent. That isolates the cause to the base commit, not to this change.

The mechanism: dashboard/create_rate_limit.py holds a process-global _buckets dict with a budget of MAX_SESSION_CREATES_PER_WINDOW = 20, and session_control.py:821 refuses the 21st create with create_rate_limited. The module ships a reset_for_tests() for exactly this, and test_session_control.py already resets its OTHER piece of process-wide state (stop_retry.reset_for_tests() in an autouse fixture) — but nothing reset the rate limiter, so session-creating tests accumulated across the file until the budget ran out. 77db85951 (#7840, "test: isolate the creation rate limiter's module-level buckets between tests") wires it. Verified: the two files go from 2 failed / 225 passed to 227 passed on the rebased head.

The test_external_logout_detection red is a genuine flake, not a main-side fix: no commit in the range touches that test, kiro_prerequisite, hooks, or sel, and the test passes 3/3 in isolation here. Its CI failure carried OSError: SEL chain lock is held by another writer; refusing to wait for it on the event-loop thread, so identity_fingerprint returned "" and the assertion read assert '' != '' — lock contention between parallel xdist workers, which a rerun clears. Flagging rather than patching, since the contention is in the SEL audit chain and not in this PR's surface.

Local gates on the rebased head: 530 passed on the touched surface, check_black_formatting / isort / flake8 clean, mypy clean on 1264 files, and all four diff-scoped gates PASS with BASE_REF exported.

Round 5 — rebased onto main to pick up the fast-uri dependency fix

Rebased onto origin/main ca55e411a (22 commits). Clean, still one commit, payload unchanged at 506 insertions across the same 6 files.

Dependency Audit / Audit Production Dependencies was the only real FAILURE on the prior head — the other reds were CANCELLED runs from the superseded head. It failed on four high-severity fast-uri advisories in website/electron/package-lock.json:

ERROR: unexcepted high/critical production vulnerabilities:
  website/electron/package-lock.json: fast-uri GHSA-5jgf-p345-68v8 (high) - host confusion via skipped IDN canonicalization on scheme-relative references
  website/electron/package-lock.json: fast-uri GHSA-f65p-4m7j-42xc (high) - SSRF via malformed IPv6 normalization
  website/electron/package-lock.json: fast-uri GHSA-fph4-wmhf-6fwf (high) - SSRF via repeated hostname percent-decoding
  website/electron/package-lock.json: fast-uri GHSA-jqff-g426-hqxp (high) - host confusion via percent-encoded scheme normalization

Main-side, not introduced here: this PR touches no dependency manifest at all — its six files are Python, tests, and one spec doc. 0545b668e (#7936, "fix(deps): unpin fast-uri so the patched 3.1.7 can resolve") landed on main after this branch's base, and is now an ancestor of this head with fast-uri resolving to 3.1.7.

The rebase also carries 51198168b (#7780, Python floor to 3.12 / 3.10 CI lane dropped), so the Backend Tests (3.10, ...) lane that reported the test_external_logout_detection SEL chain-lock flake in Round 4 no longer runs.

Local gates on the rebased head: 763 passed across the touched surface plus the two files that were red in Round 4 (test_session_control.py, test_external_logout_detection.py), isort / flake8 clean, mypy clean on 1264 files, and all five scripted gates PASS (check_black_formatting, check_brand_name, check_changelog_history, check_focus_cue, check_harness_parity) with BASE_REF exported.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 98a6c26

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

All claims verify against the code: the direct-vet precedent exists in chat_compaction_notice.py:89, privacy_mode.hydrate is documented as idempotent and safe per decision point, the spec is updated in the same commit, and the tests pin reach, ordering, fail-closed behavior, and the sessionless-caller carve-out. The rungs land in the destination oracle (the layer both legs share), the hydrate fix lands at the shared predicate rather than per-leg, and the change is fully reversible with no schema or persisted-data impact. The one candidate concern — that registry-absent egress legs must each remember to restate the rungs by direct call — is already named by the author with a proposed ratchet, and the shipped tests pin both rungs on this leg. No design-level findings survive.

Design-Verdict: PASS

Closes a real governance/privacy asymmetry at the shared oracle layer, fail-closed, before destination work — the right shape, following shipped precedent.

[DESIGN-REVIEWED] 98a6c26

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

Both candidates share one root cause: the two new Slack-leg ceilings (_slack_egress_permitted and uploads_restricted) read the lenient session_key (request.headers.get("X-Session-Key"), populated by the _post("/api/slack/upload-file") call at mcp_tools/messaging.py:808 which passes no session_key=, defaulting to _resolve_session_key()), while the channel leg is handed the strict key at messaging.py:768. I confirmed the header wiring, the _post default, and that only files.py calls resolve_slack. The candidates do not survive falsification:

• The diff only ADDS ceilings that can exclusively DENY. Before this PR the Slack leg ran no channels vet and no restricted-session check at all, so for the one exploitable direction (sub-agent's own profile denies channels, lenient walk resolves to a permitting parent) the file uploaded before the PR and still uploads after it — identical outcome, no regression. (c) "an observable wrong outcome the changed lines produce" cannot be grounded: the changed lines cannot cause any leak that would not already have occurred.
• The lenient identity is pre-existing and used consistently across the whole leg (destination resolution and the prior tracked-channel authorization already read it); the new rungs introduce no new identity source. It is a deliberate, documented design choice, not a guard the diff removed.
• An upstream strict three-state classifier (_classify_slack_identity, mcp_core.py:1573) already refuses an unresolved caller for audience safety before the leg runs.
• Even granting the residual gap, the minimal fix (pass the already-computed strict_key on the Slack _post) lives in mcp_tools/messaging.py, which this PR does not touch — the demotion override would bar BLOCKING regardless, and reverting the hunk removes a net-positive control rather than fixing anything.

Candidate 2 is weaker still by its own terms (fails in the safe direction) and merges into the same root cause.

Step 2: the privacy_mode.hydrate insertion in upload_gate.py handles a None session manager as a no-op, is confined to the non-dashboard-key branch, and the persisted_probe parameter is supplied by the sole caller; no new grounded defect.

[OPUS-REVIEWED] 98a6c26

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

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 98a6c26b6e7e26320397b6e4830a12bfc9dfb420 — 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 claims verified. Producing the review.

First-Principles-Verdict: CONCERNS

Every rung earns its place against a reported defect (#7290), but _slack_egress_permitted is a second spelling of a wrapper that already ships.

What this change ships

Intent: make the Slack file_send leg obey the same two authorization ceilings the channel leg already runs — a FIX.

1. A profile denying the channels scope now denies Slack uploads (403) — justified
2. An incognito/temporary session is now denied Slack uploads (403) — justified
3. Both denials run before any destination work, leaking nothing — justified
4. Sessionless cron/heartbeat callers vetted under _host, not muted — justified (precedent: handlers/messaging.py:1411)
5. A durable !incognito now survives a gateway restart for every upload-gate caller — justified, landed in the shared predicate
6. New _slack_egress_permitted helper — duplicate of chat_compaction_notice._channel_egress_permitted
7. resolve_slack gains a required persisted_probe parameter — justified by upload_gate's documented parameter-not-import contract
8. Spec section + rung-table rewrite — justified (same-commit spec rule)
9. Existing hydrate-call assertion loosened from exact list to set — rides along, necessitated by item 5
10. Three test setups pin unrestricted slots — declared

Watch

• Grep _channel_egress_permitted|_slack_egress_permitted: 2 copies of the same ~20-line fail-closed vet_and_audit wrapper (identical re-raise/deny/getattr(decision, "permitted", False) body); only tool_name, the hardcoded namespace, and the call-site-movable or HOST_SESSION_KEY differ. The description cites the precedent's shape but never says why it copies rather than reuses; the copies have already diverged once.
• The root cause item 5 patches — process-local trackers populated only by inbound messages — has 4 other readers of privacy_mode.is_restricted (slack/handler.py:642, telegram/transport_dispatch.py:894,2064,2732). Most are inbound-driven, but _persist_turn also covers drained-queue and steered continuations; if any such turn follows a restart, the transcript writer keeps the gap the upload gate just closed. Accepted-and-deferred, not a demand.

Subtractions

• Delete _slack_egress_permitted's body: parameterize chat_compaction_notice._channel_egress_permitted on tool_name and call it, keeping session_key or HOST_SESSION_KEY at the call site — removes ~40 duplicated lines and the future divergence.

[FIRST-PRINCIPLES-REVIEWED] 98a6c26

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #6831 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6831: KEEP. Complementary, not duplicative -- different features, and the merged work does not implement any part of the note mirror. Before merge the PR should add upload_destination to the shared-gate inventory and update the two references to the symbol it deletes; the repo's own rule is that a spec is updated in the same commit as the API it documents. Files: src/kiro_crew/dashboard/slack_egress.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.