fix(gateway): resolve assistant thread for threadless broadcasts - #2444
Conversation
Mission notifications, self-repair alerts, and extension activation messages broadcast via channels without a thread_id. The gateway's broadcast() rejected these with MissingRoutingTarget, silently dropping the messages. Two fixes: 1. Mission notification now chains .in_thread() — the thread_id was already available on MissionNotification but not being passed through. 2. Gateway broadcast() falls back to the user's assistant conversation when thread_id is None and a DB store is available. This routes threadless messages (self-repair, extension activation) to a known thread instead of rejecting them. When no store is available, the original MissingRoutingTarget error is preserved. Fixes nearai#2405 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
serrrfirat
left a comment
There was a problem hiding this comment.
Paranoid Security Review — NEEDS CHANGES
PR: #2444 — Resolve assistant thread for threadless broadcasts
Reviewer context: Automated deep security review
Medium
Cross-user thread ID leakage in mission notification path. In src/bridge/router.rs:2930-2935, the PR adds .in_thread(notif.thread_id.to_string()) to the broadcast. However, broadcast_user can be notif.notify_user — a different user than the mission owner. The notif.thread_id belongs to the mission owner's thread. When notify_user is set, the broadcast sends the owner's thread_id to a different user's context.
Fix: When notify_user.is_some() and differs from user_id, do NOT attach notif.thread_id. Let the gateway's broadcast() fallback resolve the recipient's own assistant thread instead.
Low
- TOCTOU race in
get_or_create_assistant_conversation(pre-existing): Non-transactional SELECT-then-INSERT. Two concurrent broadcasts could create duplicate assistant threads on PostgreSQL. The INSERT lacksON CONFLICT. Fine as a follow-up.
Positive
- No cross-user thread access vulnerability in the fallback path —
user_idflows from server-side callers, not client input. - No
.unwrap()or.expect()in production code. - Test coverage is solid with a real libSQL backend.
Summary
The fallback design is sound and well-tested. The mission notification thread_id mismatch is the only blocking issue — straightforward to fix by conditionally attaching the thread_id only when the broadcast target matches the thread owner.
When notify_user differs from the mission owner, omit .in_thread() so the gateway's broadcast() fallback resolves the recipient's own assistant thread instead of attaching the owner's thread_id. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
||
| #[tokio::test] | ||
| async fn gateway_broadcast_with_thread_id_succeeds() { | ||
| let gw = test_gateway(); |
There was a problem hiding this comment.
Medium Severity — Test doesn't verify the resolved thread_id
The new test asserts result.is_ok() but doesn't verify that the message was actually routed to the correct assistant thread. A regression where the fallback "succeeds" but uses a wrong or stale thread_id would pass this test.
Suggestion: Subscribe to gw.state.sse before calling broadcast(), receive the emitted AppEvent::Response, and assert its thread_id matches the UUID returned by get_or_create_assistant_conversation("test-user", "gateway"). Also query the DB to verify the conversation row exists.
| if broadcast_user == notif.user_id { | ||
| response = response.in_thread(notif.thread_id.to_string()); | ||
| } | ||
| if let Err(e) = channels |
There was a problem hiding this comment.
Medium Severity — No test for the cross-user thread_id guard
The if broadcast_user == notif.user_id condition is a security-sensitive guard that prevents leaking the owner's mission thread_id to a different recipient. Per .claude/rules/testing.md ("Test Through the Caller"), when a predicate gates a side effect with computed inputs, a caller-level test is required.
Suggestion: Add an integration test that exercises handle_mission_notification (or the broadcast path) with a MissionNotification where notify_user = Some("other-user"), and verify the response sent to the channel does NOT carry the owner's thread_id. Separately test the notify_user == None case to verify thread_id IS attached.
serrrfirat
left a comment
There was a problem hiding this comment.
Approving — the fix is correct and well-scoped. The cross-user thread_id guard and the gateway fallback are both sound.
Follow-up request: Please address the two inline comments about test coverage in a subsequent PR:
- Verify the resolved thread_id in the fallback test — subscribe to SSE, assert the emitted
thread_idmatches the assistant conversation UUID, and confirm the DB row exists. - Add a caller-level test for the cross-user guard — exercise
handle_mission_notificationwithnotify_user = Some("other-user")and verify the response does NOT carry the owner's thread_id.
These are Medium-severity gaps (security-sensitive condition + fallback correctness) but the code itself is correct, so they shouldn't block merge.
Address review feedback on nearai#2444: 1. Fallback test now subscribes to SSE, verifies the emitted thread_id matches the DB assistant conversation UUID, and confirms the row exists. 2. Three new caller-level tests for the cross-user guard in handle_mission_notification: - cross-user: notify_user != user_id -> owner's thread_id is NOT attached, recipient gets their own assistant thread - same-user: notify_user is None -> owner's mission thread_id IS attached to the broadcast - explicit same-user: notify_user = Some(user_id) -> guard still matches, thread_id is attached (catches is_none() refactors) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
I prefer this solution to the problem because it addresses the root cause. |
serrrfirat
left a comment
There was a problem hiding this comment.
LGTM — the cross-user thread_id guard and DB fallback are both correct and well-tested.
Follow-up items from security review
A few pre-existing issues that this PR doesn't introduce but does increase exposure to:
-
TOCTOU race in
get_or_create_assistant_conversation(CRITICAL, pre-existing): Both backends use SELECT-then-INSERT without a transaction or unique constraint. Compare with the heartbeat path (store.rs:1997) which correctly usesON CONFLICT DO NOTHING. With the DB fallback now being the hot path for all threadless broadcasts, concurrent calls can create duplicate assistant conversations. Tracked in a follow-up issue. -
Direct SSE broadcast bypasses cross-user guard (MEDIUM, pre-existing):
router.rs:2947always sends tonotif.user_idwith the owner's thread_id, even whennotify_userroutes to a different recipient. The channel broadcast path (this PR) respects the guard, but the direct SSE path doesn't. -
notify_useris unvalidated LLM-derived input (MEDIUM, pre-existing):effect_adapter.rs:1206accepts arbitrary user strings from tool params without checking the user exists or the caller has permission to notify them.
None of these block merge — they're all pre-existing and the PR is well-scoped.
Collapse handle_mission_notification call sites to single-line form to satisfy cargo fmt. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
RUSTSEC-2026-0098 (URI name constraint bypass in rustls-webpki) affects 0.102.8, which is pinned by the libsql 0.6.0 transitive dependency chain (libsql -> rustls 0.22 -> rustls-webpki 0.102.x). The fix (>=0.103.12) is only available on the 0.103.x line, so the 0.102.8 instance cannot be upgraded without a libsql major bump. Add the advisory to deny.toml ignore list (same rationale as the existing RUSTSEC-2026-0049 exception for the same crate/version). Also bump rustls-webpki 0.103.10 -> 0.103.12 for the non-pinned instance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
serrrfirat
left a comment
There was a problem hiding this comment.
CI is green now. Re-approving after fixing:
- rustfmt: Collapsed
handle_mission_notificationcall sites to single-line form in the cross-user guard tests - cargo-deny: Added
RUSTSEC-2026-0098exception forrustls-webpki 0.102.8(pinned by libsql transitive dep, same chain as existing RUSTSEC-2026-0049 exception) and bumped 0.103.10 → 0.103.12
Follow-up items tracked in #2488.
…ndary External channel thread ids (Telegram chat id, web UUID, Slack thread_ts) flow as raw Option<String> through IncomingMessage, StatusUpdate, and pending-gate store. Wraps them in a validated ExternalThreadId so the compiler distinguishes boundary-layer ids from the internal ThreadId(Uuid). Maps to bug pattern from #2349, #2444, #2517 where thread-id confusion crossed a layer silently.
…ndary (#2685) * refactor(channels): introduce ExternalThreadId newtype at channel boundary External channel thread ids (Telegram chat id, web UUID, Slack thread_ts) flow as raw Option<String> through IncomingMessage, StatusUpdate, and pending-gate store. Wraps them in a validated ExternalThreadId so the compiler distinguishes boundary-layer ids from the internal ThreadId(Uuid). Maps to bug pattern from #2349, #2444, #2517 where thread-id confusion crossed a layer silently. * fix(bridge): adapt test thread_id to ExternalThreadId newtype Post-merge fix: a test added in staging (insert_and_notify_pending_gate_uses_extension_manager_for_auth_display_name) assigned a raw String to message.thread_id, but the field type became ExternalThreadId on this branch. Wrap with ExternalThreadId::from_trusted to match the other tests in the same module. * refactor(types): address review feedback — byte units, shared validate, try_-variants, dedup pending-gate * refactor(types): validate scope_thread_id + relay respond prefers typed msg.thread_id - router.rs: scope_thread_id written to PendingGate was wrapped via ExternalThreadId::from_trusted from message.conversation_scope(), which can carry untrusted WASM/metadata-sourced strings. Now validates via ExternalThreadId::new; invalid values log at debug and store None. Applied at both call sites (authentication-fallback path and generic gate-insertion path). - relay/channel.rs: respond() derived thread_id only from response or metadata — now also consults the validated msg.thread_id as the second fallback (before raw metadata) and filters empty strings so we never emit thread_ts: "" to Slack.
…ack/slash
Wires the verifier from the previous commit into a real HTTP surface so
Slack's `/ironclaw <prompt>` invocations land somewhere meaningful. The
agent reply will be delivered out-of-band via the body's `response_url`
in a subsequent commit; this commit ships the verified ack path so a
Slack admin can install the IronClaw app, paste the manifest, and watch
slash invocations succeed without any 4xx noise from the user.
Receiver flow (in order):
1. Pull `slack_signing_secret` from the secrets store, keyed on the
deployment owner. The secret name matches the
`channels-src/slack/slack.capabilities.json` value
(`config.signing_secret_name`) so the WASM channel and the core
surface read from the same row.
2. Verify `X-Slack-Signature` against the raw bytes via
`crate::channels::slack::sig::verify` (constant-time-compared,
5-minute replay window).
3. Parse the form-encoded body via `parse_slash_command`.
Uses `url::form_urlencoded` rather than serde_urlencoded so we
don't pull in a new direct dep just for a flat string-to-string
decode.
4. Resolve `(channel='slack', external_id=workspace_id)` in
`channel_identities`. Lack of a row → friendly self-service hint
pointing at `ironclaw channels install slack <workspace_id>`,
not a vague 4xx.
5. Ack within 3 seconds with an ephemeral placeholder.
Per Enterprise Grid handling: `effective_workspace_id` prefers
`enterprise_id` when present, mirroring the OAuth response handling
from the workspace-install commit.
What this commit deliberately does NOT ship (subsequent commits on
this branch):
* Agent invocation + the out-of-band POST to `response_url` —
threading the engine through this path needs care.
* Per-user pairing on first invocation from an unknown Slack user
(will reuse `crate::pairing` — already channel-generic).
* `channel_audit_log` row write per in/out (compliance trail).
* Interactivity surface at `/api/channels/slack/interactivity` —
same verifier, different body shape.
* Cross-channel approval hijack guard (nearai#2444) — applies once we
plumb approvals through this surface.
Tests:
* `parse_slash_command` (5 unit tests, real-shape Slack fixture
with all 14 documented fields):
- parses_slash_command_fixture
- enterprise_grid_uses_enterprise_id_for_workspace_lookup
- rejects_body_missing_team_id
- rejects_body_missing_response_url
- ack_payload_emits_ephemeral_default
* `slash_command_handler` helpers (4 unit tests):
- header_str_returns_empty_for_missing_header
- header_str_returns_value_for_present_header
- truncate_for_display_appends_ellipsis_above_limit
- truncate_handles_multibyte_chars_without_panic
Also rolls in a small fmt drift on `sig.rs` from the previous commit
that CI flagged after the no-panics fix.
Constraint adherence: does not fork the OAuth handler in `auth.rs`;
slash is a sibling surface with its own verifier. Does not touch the
Slack WASM channel under `channels-src/slack/` — its `/webhook/slack`
events route is unaffected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rai#2444) * fix(gateway): resolve assistant thread for threadless broadcasts Mission notifications, self-repair alerts, and extension activation messages broadcast via channels without a thread_id. The gateway's broadcast() rejected these with MissingRoutingTarget, silently dropping the messages. Two fixes: 1. Mission notification now chains .in_thread() — the thread_id was already available on MissionNotification but not being passed through. 2. Gateway broadcast() falls back to the user's assistant conversation when thread_id is None and a DB store is available. This routes threadless messages (self-repair, extension activation) to a known thread instead of rejecting them. When no store is available, the original MissingRoutingTarget error is preserved. Fixes nearai#2405 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: don't leak owner thread_id to notify_user in mission broadcasts When notify_user differs from the mission owner, omit .in_thread() so the gateway's broadcast() fallback resolves the recipient's own assistant thread instead of attaching the owner's thread_id. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: verify broadcast thread_id resolution and cross-user guard Address review feedback on nearai#2444: 1. Fallback test now subscribes to SSE, verifies the emitted thread_id matches the DB assistant conversation UUID, and confirms the row exists. 2. Three new caller-level tests for the cross-user guard in handle_mission_notification: - cross-user: notify_user != user_id -> owner's thread_id is NOT attached, recipient gets their own assistant thread - same-user: notify_user is None -> owner's mission thread_id IS attached to the broadcast - explicit same-user: notify_user = Some(user_id) -> guard still matches, thread_id is attached (catches is_none() refactors) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: fix rustfmt in cross-user guard tests Collapse handle_mission_notification call sites to single-line form to satisfy cargo fmt. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: ignore RUSTSEC-2026-0098 and bump rustls-webpki 0.103.12 RUSTSEC-2026-0098 (URI name constraint bypass in rustls-webpki) affects 0.102.8, which is pinned by the libsql 0.6.0 transitive dependency chain (libsql -> rustls 0.22 -> rustls-webpki 0.102.x). The fix (>=0.103.12) is only available on the 0.103.x line, so the 0.102.8 instance cannot be upgraded without a libsql major bump. Add the advisory to deny.toml ignore list (same rationale as the existing RUSTSEC-2026-0049 exception for the same crate/version). Also bump rustls-webpki 0.103.10 -> 0.103.12 for the non-pinned instance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: serrrfirat <f@nuff.tech>
…ndary (nearai#2685) * refactor(channels): introduce ExternalThreadId newtype at channel boundary External channel thread ids (Telegram chat id, web UUID, Slack thread_ts) flow as raw Option<String> through IncomingMessage, StatusUpdate, and pending-gate store. Wraps them in a validated ExternalThreadId so the compiler distinguishes boundary-layer ids from the internal ThreadId(Uuid). Maps to bug pattern from nearai#2349, nearai#2444, nearai#2517 where thread-id confusion crossed a layer silently. * fix(bridge): adapt test thread_id to ExternalThreadId newtype Post-merge fix: a test added in staging (insert_and_notify_pending_gate_uses_extension_manager_for_auth_display_name) assigned a raw String to message.thread_id, but the field type became ExternalThreadId on this branch. Wrap with ExternalThreadId::from_trusted to match the other tests in the same module. * refactor(types): address review feedback — byte units, shared validate, try_-variants, dedup pending-gate * refactor(types): validate scope_thread_id + relay respond prefers typed msg.thread_id - router.rs: scope_thread_id written to PendingGate was wrapped via ExternalThreadId::from_trusted from message.conversation_scope(), which can carry untrusted WASM/metadata-sourced strings. Now validates via ExternalThreadId::new; invalid values log at debug and store None. Applied at both call sites (authentication-fallback path and generic gate-insertion path). - relay/channel.rs: respond() derived thread_id only from response or metadata — now also consults the validated msg.thread_id as the second fallback (before raw metadata) and filters empty strings so we never emit thread_ts: "" to Slack.
Problem
Mission notifications, self-repair alerts, and extension activation messages broadcast via channels without a
thread_id. The gateway'sbroadcast()rejects these withMissingRoutingTarget, silently dropping the messages.Root cause
The gateway requires
thread_idfor SSE routing (added in the no-silent-drop hardening). Three callers send without one:src/bridge/router.rs:2933— mission notifications (thread_id available but not passed)src/extensions/manager.rs:6849— Telegram owner verification (no thread context)src/agent/agent_loop.rs:752,765— self-repair notifications (no thread context)Note: the heartbeat path (
src/agent/heartbeat.rs:416) already resolves a thread_id viaget_or_create_heartbeat_conversationbefore sending, so it is NOT affected.Fix
Mission notification now chains
.in_thread(notif.thread_id.to_string()). The thread_id was already onMissionNotification— the SSE broadcast 3 lines below was using it.Gateway
broadcast()falls back to the user's assistant conversation (viaget_or_create_assistant_conversation) whenthread_idis None. This routes threadless messages to the assistant thread instead of rejecting them. When no DB store is available, the originalMissingRoutingTargeterror is preserved.Test plan
cargo clippy --all— zero warningscargo test -- no_silent_drop— 5 tests pass:gateway_broadcast_without_thread_id_and_no_store_returns_error— no store = error (renamed)gateway_broadcast_with_thread_id_succeeds— unchangedgateway_broadcast_without_thread_id_falls_back_to_assistant_thread— new (libsql): creates a gateway with a real DB store, broadcasts without thread_id, verifies success via assistant thread fallbackgateway_respond_without_thread_id_returns_error— unchangedgateway_respond_with_thread_id_succeeds— unchangedLocal verification attempted: Started IronClaw locally with Qwen on MLX, but the three affected callers (missions, self-repair, extension activation) are difficult to trigger in a minimal setup. The heartbeat fires successfully but uses its own thread_id resolution and doesn't exercise the broken path. The unit test exercises the exact
broadcast()method with a real libsql-backed gateway, which is the code path all three callers share.notify_channels: ["gateway"]and verify the notification appears in the assistant threadFixes #2405