fix(chat): authorize History resume against the conversation it loads

[leonlaiyc]

Problem / Motivation

POST /api/chat/slots/{slot}/resume has two branches, and only one of them authorizes.

When a live slot already exists, the App Kit §5.2 check runs against that slot before anything is returned. When it does not — which is the ordinary History case, a conversation with no open tab — the handler went straight to:

slot = state.get_or_create_slot(name, app=request.get("app", ""), ...)
meta = state.conversation_log.get_metadata(history_key)
all_messages = state.conversation_log.read_messages_chained(history_key)

history_key is body["key"] — caller-supplied, and not required to match the slot name in the URL. So it names an arbitrary conversation, and nothing on that path compares the caller against that conversation's owner. The only slot in scope is the one the request just created carrying the caller's own identity, so authorizing against it would let the claim stand as its own evidence.

On 24a6f8ee5, an app token can read any persisted conversation this way:

requested history	result before
dashboard:chat-7-… — an ordinary dashboard user's conversation	200, full transcript
dashboard:spec-builder-99 — meta["app"] == "spec-builder"	200, full transcript
slack:1785370133.085469 — a Slack thread	200, full transcript

In every case the URL slot was app-a-scratch, unrelated to the key — so this is not about channel-shaped names. It is also not read-only: the transcript is hydrated into a new app-owned slot, which then owns that conversation's messages, its title, agent, model and folder, and clears its closed flag on the way in.

Why it matters

This is an authorization bypass, not a cosmetic gap. An app token that owns a
scratch slot can name any conversation key in the request body and receive the
full transcript back: a dashboard user's private chat, another app's conversation,
or a Slack thread. The slot in the URL never has to be related to the key that is
read, so no naming convention limits the blast radius.

/resume is the only reader of a persisted conversation that ignored
meta["app"]; every other restore path already treats it as authoritative.

What changed

Authorize against the persisted conversation, before a slot exists:

meta = state.conversation_log.get_metadata(history_key)
request_app = request.get("app", "")
if request_app and meta.get("app") != request_app:
    ...  # SEL app_isolation denial, then the shared 404

meta["app"] is the durable owner. It is written by the slot save (chat_persistence.py:1669), it is one of history.SLOT_OWNED_META_KEYS, and both restore paths already rebuild slot._app from it (chat_persistence.py:485/:559 and :869/:917). Resume was the only reader of a persisted conversation that ignored it.

Absence is an answer, not a gap. Because app is a slot-owned key, an unscoped conversation's save deliberately omits it — that states "no app owns this", not "unknown". Deny-by-default for app callers therefore follows the writer's own contract rather than guessing about legacy rows.

One metadata read. It moved above get_or_create_slot and the same dict already fed the title/agent/model/folder restoration below, so authorization and hydration decide from one snapshot rather than two reads.

A refusal is not an existence oracle. A transcript that does not exist also has no app, so a foreign conversation and a missing one take the same branch and are answered identically — no probing the history namespace for keys that exist. The two pre-existing §5.2 denials on this route gained the same code so all three bodies stay byte-identical; per AGENTS.md a new non-2xx body must carry a machine-readable code, and giving it only to the new one would itself have been the oracle. error-code-baseline.json regenerated for those three (dashboard/chat_handlers.py 76 → 74).

Dashboard callers have no app scope and are untouched.

Relationship to #2783 — adjacent, not overlapping

• fix(chat): refuse a linked session binding on an app-scoped slot #2783 governs which live session a slot may acquire authority over (linked_session_key).
• This governs which persisted transcript a caller may read and adopt.

They are independent: even with no session binding at all, an app that can hydrate another conversation's messages has already crossed the isolation boundary. Neither fix makes the other unnecessary.

Tests — test/test_resume_history_ownership.py

Behaviour-level, through the real handler with the auth middleware's app stamp. 9 failed / 5 passed against 24a6f8ee5; 14 passed after.

Refused — an ordinary dashboard conversation, another app's conversation, a channel conversation, and a conversation with no ownership metadata (deny-by-default).

Not an oracle — a foreign conversation answers with the same status and the same JSON body as a nonexistent one; and the create-branch refusal is identical to the existing-slot branch's.

No side effects on refusal — no slot is created; no existing slot is mutated (title, agent, model, pin, linked_session_key, channel_origin, message count, dirty flag, the slot table and the restricted-key set are all snapshot-compared); and the foreign transcript is not rewritten, which the closed-flag clear would otherwise do.

Preserved, all passing before the change — an app resumes its own conversation; a dashboard caller resumes a channel conversation, an app-owned one, and an ordinary one; and the existing-slot branch still serves its owner.

Verification

Windows 11 26200 / py3.10.6.

• test_resume_history_ownership.py — 9 failed / 5 passed before, 14 passed after.
• Run against the exact base and against this branch, comparing failing node
  sets rather than counts: test_error_code_contract.py, test_dashboard_chat.py,
  test_open_slots_persistence.py, test_session_restore.py,
  test_rehydrate_async.py, test_channel_slots.py,
  test_one_conversation_one_session.py, test_trusted_apps_api.py,
  test_slot_detail_full_history.py plus the new file — base: 9 failed, 860
  passed, 2 skipped; branch: 869 passed, 2 skipped, 0 failed. Every one of the
  9 base failures is in the new file, so the delta across 869 tests is exactly
  the security coverage this PR adds and nothing else moved.
• flake8, isort and git diff --check clean; mypy reports nothing in chat_handlers.py.
• No spec change: docs/app-kit/ documents no resume contract, and the route's own §5.2 comments are the existing statement of the rule this extends.

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Advisory design-level review of b0172227a2d39a5955228e55a65444ce1f29d3cf via the fork AI-review pipeline — updated in place on each push; does not block merge.

Verified the base handler (chat_handlers.py:3414-3544), the sibling §5.2 checks across the route family, SLOT_OWNED_META_KEYS semantics in history.py:156, and the restore paths that rebuild slot._app from meta["app"] (chat_persistence.py:487/567/877/931). The fix is at the same layer as every other §5.2 check, authorizes against the durable owner rather than the just-created slot, fails closed on unreadable metadata, and the identical-404 anti-oracle design forces the one behavior change it introduces (an app resuming a nonexistent key now gets 404 instead of a 200-empty slot). No one-way doors; the code field is additive per the AGENTS.md contract.

Design-Verdict: PASS

Real authorization bypass, fixed at the durable owner (meta["app"]) in the same layer and idiom as every sibling §5.2 check — fail-closed, oracle-free, reversible.

Suggestions

• The byte-identical refusal body is now a security invariant held by three hand-kept copies in one route (tests pin it); a tiny shared _app_isolation_denied(operation, resources, error) helper would enforce it by construction.

[DESIGN-REVIEWED] b017222

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed b0172227a2d39a5955228e55a65444ce1f29d3cf via the fork AI-review pipeline; updated in place on each push.

BLOCKING -- src/kiro_crew/dashboard/chat_handlers.py:3540 -- persisted owner is app-rewritable
if request_app and meta.get("app") != request_app:
App creates the victim-named slot, sends once, then deletes it -> save replaces meta["app"]"] while preserving victim messages -> resume returns the transcript with 200.
Anchor: backend-security-controls
Fix: Reject all app callers on this branch until ownership uses data app chat saves cannot rewrite.
[BLOCK-MERGE] b017222
[GPT-REVIEWED] b017222

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Advisory premise-level review of b0172227a2d39a5955228e55a65444ce1f29d3cf via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push; does not block merge.

I've read the contract, the intent file, the authoritative patch, and the relevant base sources (chat_handlers.py resume handler, token_auth.py app-scope gate, history.py SLOT_OWNED_META_KEYS, chat_persistence.py restore paths, handlers/sessions.py). Verified the bypass exists in the base, that meta["app"] is the durable owner the restore paths already trust, and counted siblings and consumers. Final review follows.

First-Principles-Verdict: CONCERNS

The fix is real and cause-level, but "resume was the only reader that ignored meta['app']" is contradicted by four ungated handlers in handlers/sessions.py.

What this change ships

Intent: stop an app token from reading and adopting any persisted conversation by naming its key in a resume body — a FIX.

1. An app token can no longer read or adopt a foreign persisted conversation via History resume — justified (App Kit §5.2 boundary; bypass confirmed at base chat_handlers.py:3516).
2. An app is refused a conversation with no owner metadata — justified; mirrors the live-slot branch's "unscoped" rule at chat_handlers.py:3454.
3. An app resuming a key with no transcript now gets 404 instead of a fresh empty slot — rides along, declared only as the anti-oracle property.
4. The two pre-existing refusals gain code: "slot_not_found", byte-identical to the new one — justified (AGENTS.md code mandate + CWE-204 pattern already documented at chat_fork.py:82).
5. Refusals are SEL-audited naming the history key — justified, matches every sibling deny site.
6. One metadata read instead of two on the create branch — justified deletion.
7. error-code-baseline.json totals move 76→74 — mechanical.
8. New behavior-level test file pinning refusal, oracle-equivalence, and no-side-effects — justified.

Watch

• The description claims "/resume is the only reader of a persisted conversation that ignored meta['app']". Grep request.get("app" in handlers/sessions.py: 0 hits, yet api_session_detail (sessions.py:1167) returns any transcript's messages, and api_sessions_search (:1130), api_sessions preview (:946), api_session_delete (:1176) read or delete with no ownership check. Today they're reachable only via a manifest declaring /api/sessions (0 builtin manifests do), so this is deferred-sibling, not a blocker — but the root cause ("a path grant is coarser than per-conversation ownership") has 4 counted unfixed siblings this PR's framing denies exist.
• Item 3 removes a capability apps may have used as create-or-attach (resume with a not-yet-persisted key previously returned a live empty slot). POST /api/chat/slots still covers that job; worth one sentence of release awareness.

[FIRST-PRINCIPLES-REVIEWED] b017222

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed b0172227a2d39a5955228e55a65444ce1f29d3cf via the fork AI-review pipeline; updated in place on each push.

Review details

The candidate concerns whether read_messages_chained (unchanged line 3629) could concatenate a sibling transcript owned by a different app, bypassing the new guard that checks only the primary key's meta["app"].

To ground this I'd need a concrete reachable input where two session files share a tab_id but carry different app owners. tab_id is server-minted (append/update_metadata set it from the slot's own value, never caller-supplied through this endpoint), and both tab_id and app are SLOT_OWNED_META_KEYS (history.py:172,180) stamped at slot creation and carried forward on rotation/fork — so a tab lineage shares one owner. There is no code path in which an app can force its file to adopt a foreign chain's tab_id. The candidate itself concedes it "could not construct a concrete reachable input." I cannot re-derive (a) a concrete input, so it fails the bar. It is also pre-existing code the diff does not touch.

The added guard itself is sound: it fails closed (if request_app and meta.get("app") != request_app), exempts dashboard callers (request_app == ""), runs before slot creation so no foothold is left, and the refusal body is uniform (not an existence oracle). Nothing else in the diff introduces a reachable defect.

No findings.

[OPUS-REVIEWED] b017222

[bolichen97]

🤖 Kiro Crew [operator: bolichen97]: This PR has been inactive for 7+ days. I reviewed the blockers but they require your input:

• Security-model decision (GPT 5.6 BLOCKING vs Opus 4.8 PASS). GPT 5.6 rejects the whole approach of authorizing /resume against meta["app"], arguing an app can create a victim-named slot, send once so the save rewrites meta["app"] while preserving the victim's messages, then delete it — after which meta["app"] == request_app and resume returns the transcript. Its prescribed fix is to reject all app callers on this branch until ownership uses data an app's chat-save cannot rewrite. That directly guts the PR's stated goal (letting an app resume its own conversation) and contradicts the PR body's core claim that meta["app"] is the durable, authoritative owner. Opus 4.8, Design, and UX all passed with no blocking findings. Choosing between "reject all app callers" and "keep the metadata-ownership allowlist" (and, if kept, deciding what tamper-proof signal establishes ownership) is a security-policy decision only you can make.
• Merge conflict. The branch is CONFLICTING against main and needs a rebase/conflict resolution from you (the pipeline will not change the design direction while resolving it).

When you've addressed these, the pipeline will re-assess on its next cycle.

[bolichen97]

Audit note — part of this has already landed; the rest has not

This PR is not a duplicate and is not finished by anything on main. The audit checked it part by part against main, and some of what it does is already there. Flagging it so a reviewer does not have to rediscover the overlap, and so the PR is not mistaken for fully-covered work.

Which parts main already has

1 of 5. Only the 'One metadata read' dedup: origin/main chat_handlers.py:6292 # Reuse the SNAPSHOT the guard above validated. replaced the second get_metadata(history_key), landed by a231501 / #6210.

What is still genuinely yours

4 of 5, including the entire security fix. (a) No conversation-owner check on the resume create branch — meta.get("app") appears nowhere in origin/main chat_handlers.py, so an app token can still put an arbitrary dashboard:* / slack:* / other-app key in the body and have that transcript hydrated into a slot it owns. (b) The two live-slot 5.2 denials in _live_slot_resume_response (main:5825, :5835) still return {"error": "not found"} with no code. (c) error-code-baseline.json is at 64 / 1209 / 1531, not the PR's 74 / 1397 / 764. (d) test/test_resume_history_ownership.py has no equivalent on main.

Suggested action: REBASE — the remainder is real work; rebase onto the landed part rather than closing.

---

_{From a repository-wide duplicate/overlap audit of every pull request open against main (2026-09-02, 330 PRs, one reviewer per PR). Each PR was read as its full merge-base diff plus its description and every comment and review, then compared against each candidate PR's own diff and against origin/main at 1a765b88ceb7. This PR is not being closed — the note is informational. If the reading is wrong, please correct the reasoning rather than just the conclusion.}

[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 #7308 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 #7308: CONTINUE_DEVELOPMENT. Both change the same handler's body handling for different reasons; sequence them rather than closing either. Files: src/kiro_crew/dashboard/chat_handlers.py.
• This PR is PARTIALLY_COVERED with PR #6210. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #2842: REBASE. One of five parts landed incidentally; rebase onto it rather than closing, since the entire authorization fix is still missing. Files: src/kiro_crew/dashboard/chat_handlers.py.

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

[bolichen97]

@leonlaiyc, one overlap to flag from an audit of the open PR set. #6813 (@rnoack1, "fix(chat): persist undrained pending context across a close") edits the same window of api_chat_slot_resume in src/kiro_crew/dashboard/chat_handlers.py that this PR does.

Where they collide: your ownership gate (if request_app and meta.get("app") != request_app) is inserted directly after meta = state.conversation_log.get_metadata(history_key). #6813 rewrites that exact line into an off-loop get_metadata_with_overflow read, and adds an unconditional _live_slot_resume_response call plus _resume_binding immediately before state.get_or_create_slot(...), which is the same few lines your check occupies.

What differs: this PR decides which persisted conversation a caller may adopt, while #6813 decides whether queued pending context survives a close. There is no behavioural contradiction, so both can land.

Which side is further along: #6813 spans 46 files and ships census tests that pin the new spelling. test_the_resume_handler_folds_the_spill_on_every_metadata_read asserts zero occurrences of state.conversation_log.get_metadata,, and a companion test asserts every metadata read in that handler is offloaded. Your anchor line stops existing once #6813 lands.

Suggestion: land this PR first. It is 3 files, it closes an authorization bypass, and re-anchoring one gate afterwards is cheaper than re-deriving #6813's tests. If #6813 goes first instead, please re-place the gate inside the folded, offloaded read rather than rebasing the hunk mechanically, and keep it after the member-binding await.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.