security(chat): require chat ownership on /chat/direct (#13982)

[mrveiss]

Closes #13982

Thinking Path

send_direct_chat_response took chat_id from the request body and validated nothing, while its sibling /chats/{chat_id}/message declares Depends(validate_chat_ownership). Authentication was required, so this was never anonymous — the boundary that was missing is the one between authenticated users, which is exactly what validate_chat_ownership exists to enforce.

It matters more than message injection because this endpoint carries approval and denial decisions. Reachable consequences:

• resolving another user's pending command-approval interrupt on their behalf
• remember_choice=True persists that decision for their future turns
• the injected message drives the workflow, so it can trigger tool execution in a session the caller does not own

Decision: an explicit call, not a dependency

Depends(validate_chat_ownership) looks like the obvious fix and is wrong here. That dependency resolves chat_id as a path parameter; this endpoint takes it from the body. FastAPI would therefore validate a different value than the one the request acts on — a check that passes while guarding nothing, which is worse than no check because it reads as covered.

The explicit call is the pattern the nine session endpoints above already use (await validate_chat_ownership(session_id, request) # SECURITY: ...), so this adds no new mechanism and reuses the canonical validator with its feature-flag, metrics and audit integration.

What Changed

• api/chat.py — await validate_chat_ownership(chat_id, request) in send_direct_chat_response, placed before any workflow work.

Verification

Audit (AC 4)

Every endpoint in api/chat.py that accepts a chat/session id — 14 of them. This was the only one unchecked:

OK  set_session_role / work_item / approval_categories (9 endpoints)
OK  send_chat_message_by_id      POST /chats/{chat_id}/message
OK  resume_chat_graph            POST /chats/{chat_id}/resume
OK  save_chat_by_id              POST /chats/{chat_id}/save
OK  delete_chat_by_id            DELETE /chats/{chat_id}
OK  send_direct_chat_response    POST /chat/direct        <- fixed here

Both process_message_stream entrypoints are now covered.

Tests

$ pytest autobot-backend/api/chat_direct_ownership_test.py autobot-backend/api/chat_test.py -q
6 passed

Four new tests, asserting behaviour rather than source text. The existing precedent (api_endpoint_migrations_test.py) uses inspect.getsource + assertIn("validate_session_ownership", ...), which passes on a check that is present but never reached — the exact failure mode worth guarding against here.

The tests that carry the most weight:

• the refusal happens before any workflow work — remember_choice persists a decision, so a check that runs after the side effect is not a check
• the validated chat_id is the one actually used — pins the body-vs-path mismatch described above

Mutation-tested:

mutation	result
ownership check removed (the vulnerability)	KILLED (3)
check moved after the workflow is obtained	KILLED (1)

A note on that second row, because my first run reported it as SURVIVED: my replacement anchor was missing a blank line, so the edit silently did nothing and the harness read "no test failed" as "the mutant lived". With the patch actually applied it is killed. A mutation harness that does not verify its own edit landed will report no-ops as survivors — worth knowing before adding tests to chase a phantom gap.

Lint: black, isort, flake8 clean.

Not covered

The fix relies on validate_chat_ownership behaving correctly; its enforcement is feature-flagged (SessionOwnershipValidator degrades to permissive if the flags service is unavailable). That pre-existing behaviour is unchanged here — if the flag is off in a given deployment, this endpoint is now consistent with the other 13 rather than uniquely open, but the deployment-level posture is a separate question from this issue.

Model Used

Claude Opus 5

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!

[mrveiss]

Review: SAFE — with two adjacent gaps now filed, and one correction to my audit

The diff verifies. @with_error_handling re-raises HTTPException before its generic handler, so the 403 reaches the client unchanged (confirmed in source and by the passing test through the fully decorated endpoint). The tests were confirmed non-tautological: send_direct_chat_response resolves validate_chat_ownership as a module-global at call time, which is exactly what patch.object(chat_api, ...) rebinds — the patch intercepts the real path, not a differently-imported reference.

Correction to my audit — 15 endpoints, not 14

I missed summarize_conversation (POST /chat/summarize, takes body.session_id). It already calls validate_chat_ownership, so there is no functional gap, but the stated count was wrong. The real inventory for api/chat.py is 15 endpoints, all now ownership-checked, with this PR closing the only unchecked one.

Two gaps found while verifying my claims — filed, not folded in

#14011 — export_session has no ownership check. GET /chat/sessions/{session_id}/export in api/chat_sessions.py calls neither validator anywhere in its body. Any authenticated user who knows a session_id can export that user's full transcript. Same class as this issue, different router, and unlike the flag-gated checks this one is simply never called — bulk disclosure is a worse outcome than the injection gap fixed here.

That one is on me: my audit covered api/chat.py and I let "both process_message_stream entrypoints are covered" stand in for "the chat surface is covered". The narrow claim holds — process_message_stream/resume_graph are only reached from this file's three endpoints, all checked — but it was the wrong boundary to audit to.

#14010 — ownership enforcement fails open. _get_enforcement_mode returns "disabled" when the feature-flags service is absent and on any exception, and validate_ownership fast-paths on that. So an infra blip silently disables every ownership check platform-wide, and the unset default is disabled too. I verified this independently rather than taking it on report.

This means the ~15 calls in api/chat.py, including the one added here, are inert in a default deployment. My "Not covered" section said the check is feature-flagged; #14010 states the sharper version — an outage is indistinguishable from a deliberate disabled, which is the part worth fixing first.

I did not fold either into this PR. #14010 changes deployment-wide authorization posture and will start refusing requests that work today; that needs its own rollout (log_only to size the fallout, then enforced), not a side effect of a one-line endpoint fix.

Not changed

request: Request = None yields a 500 rather than a clean 403 on a theoretical no-Request call path. FastAPI always injects it for a Request-typed parameter, so it is unreachable via HTTP, and it fails closed regardless. Pre-existing across this file; noted rather than churned here.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!