mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-03 08:06:01 +08:00
* fix(security): block cross-channel approval thread hijacking (#1485) Add source_channel to Thread and verify channel authorization before allowing approval messages to target threads by UUID. The web gateway channel is allowed as a trusted approval UI. Threads without source_channel (deserialized from older DB records) are permitted for backward compatibility. Closes #1485 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: run cargo fmt https://claude.ai/code/session_01Mdiz3XwyZcjqMkqicaynGs * fix(security): address review feedback on source_channel - hydrate_thread_from_db now passes message.channel as source_channel instead of None, ensuring DB-hydrated threads get proper channel auth - Replace is_none_or (unstable) with map_or(true, ...) for MSRV compat - Add "gateway" to trusted approval channels alongside "web" - Document why bootstrap thread uses None for source_channel Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(clippy): use is_none_or instead of map_or for Option check is_none_or is stable since Rust 1.82 and preferred by clippy over map_or(true, ...) pattern. https://claude.ai/code/session_012nbbEyFXjDwdZZrHg7gFNK * fix(security): persist source_channel to DB, harden cross-channel authorization Address PR #1590 review feedback: 1. Persist source_channel to DB: Add source_channel column to conversations table in both PostgreSQL (V14 migration) and libSQL (incremental migration + base schema). Add get_conversation_source_channel trait method to ConversationStore with both backend implementations. 2. Fix hydrate_thread_from_db: Read source_channel from DB instead of stamping the requesting message's channel, preventing channel confusion after server restart. 3. Reject reserved WASM channel names: Validate that WASM channels cannot register as "web", "gateway", "cli", or "repl" to prevent authorization bypass via name spoofing. 4. Require pending_approval exists: Authorization check now verifies thread.pending_approval.is_some() before allowing approval-shaped messages to target a thread. 5. Fail-closed for None source_channel: Use "__bootstrap__" sentinel for bootstrap threads (authorized from any channel). None now means "deny by default" instead of "allow by default". 6. Extract and test authorization predicate: is_approval_authorized() helper with 6 unit tests covering same-channel, cross-channel blocked, web/gateway always allowed, None denied, and bootstrap sentinel. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: resolve merge conflicts from staging rebase - Fix Thread::with_id calls to include source_channel parameter - Fix ensure_conversation calls to include source_channel parameter - Bump libsql source_channel migration to V15 (V14 taken by users) - Remove stale conflict markers - Fix clippy warning in users.rs https://claude.ai/code/session_01Esh8QQzHACYyfsVwCb479F * style: fix cargo fmt formatting https://claude.ai/code/session_01Ci7CAdGaHhssYdio7wxVvd * fix(security): address review feedback on cross-channel approval checks 1. thread_ops.rs: Remove .or(Some(&*message.channel)) fallback in maybe_hydrate_thread() so that when source_channel is NULL in the DB, it stays None rather than being stamped with the requesting channel. This preserves the fail-closed behavior of is_approval_authorized(). 2. libsql_migrations.rs: Remove source_channel from base SCHEMA to eliminate duplicate column definition. The column is now added solely by V14 migration, preventing fresh databases from failing on startup. 3. wasm/setup.rs: Expand RESERVED_CHANNEL_NAMES to cover all built-in channels (http, signal, slack-relay, secret_save) and add a dynamic collision check against already-registered channel names passed from the startup sequence. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(security): harden cross-channel approval authorization - Fix migration number collisions (V14 already taken by users migration; rename to V15 for PostgreSQL, bump to 16 for libSQL) - Extract TRUSTED_APPROVAL_CHANNELS constant to replace hardcoded "web"/"gateway" in is_approval_authorized(); WASM setup imports it - Add __bootstrap__ sentinel to WASM reserved channel names to prevent impersonation granting universal approval rights - Fix TenantScope::ensure_conversation passing None for source_channel, which silently blocked approvals for tenant-created threads - Add 11 regression tests: authorization logic, WASM reserved name validation, libSQL source_channel DB round-trip and upsert invariant Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address code review findings for cross-channel approval security 1. Add "telegram" to WASM channel name blocklist -- bundled channels like telegram were claimable by malicious WASM modules that load before the bundled one, bypassing cross-channel approval auth. 2. Make V16 libSQL migration (ADD COLUMN source_channel) idempotent -- the runner now checks pragma_table_info before executing ALTER TABLE, preventing startup failures if the base schema already includes the column. 3. Replace silent .unwrap_or(None) in thread hydration with explicit match on DB result -- legacy threads without stored source_channel now log a warning, and DB errors log an error. Both cases remain fail-closed (approvals denied) but are no longer silent. 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: ilblackdragon@gmail.com <ilblackdragon@gmail.com>
190 lines
6.5 KiB
Rust
190 lines
6.5 KiB
Rust
//! E2E regression test: forged thread IDs must not cross user boundaries.
|
|
//!
|
|
//! Demonstrates that a client cannot provide another user's conversation UUID
|
|
//! and get that history hydrated into prompt context or written into.
|
|
|
|
#[cfg(feature = "libsql")]
|
|
mod support;
|
|
|
|
#[cfg(feature = "libsql")]
|
|
mod tests {
|
|
use std::time::Duration;
|
|
|
|
use ironclaw::channels::{IncomingMessage, OutgoingResponse};
|
|
use uuid::Uuid;
|
|
|
|
use crate::support::test_rig::TestRigBuilder;
|
|
use crate::support::trace_llm::{LlmTrace, TraceResponse, TraceStep};
|
|
|
|
fn assert_safe_thread_rejection(response: &OutgoingResponse) {
|
|
let msg = response.content.to_lowercase();
|
|
assert!(
|
|
msg.contains("thread") && (msg.contains("invalid") || msg.contains("unauthorized")),
|
|
"expected safe thread-id rejection response, got: {}",
|
|
response.content
|
|
);
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn forged_existing_foreign_thread_id_is_rejected_without_hydration_or_persistence() {
|
|
let trace = LlmTrace::single_turn(
|
|
"thread-id-isolation",
|
|
"attacker turn",
|
|
vec![TraceStep {
|
|
request_hint: None,
|
|
response: TraceResponse::Text {
|
|
content: "safe response".to_string(),
|
|
input_tokens: 12,
|
|
output_tokens: 4,
|
|
},
|
|
expected_tool_results: Vec::new(),
|
|
}],
|
|
);
|
|
|
|
let rig = TestRigBuilder::new().with_trace(trace).build().await;
|
|
|
|
let foreign_thread_id = Uuid::new_v4();
|
|
let marker = format!("FOREIGN-MARKER-{}", Uuid::new_v4());
|
|
let store = rig.database();
|
|
assert!(
|
|
store
|
|
.ensure_conversation(
|
|
foreign_thread_id,
|
|
"gateway",
|
|
"victim-user",
|
|
None,
|
|
Some("gateway")
|
|
)
|
|
.await
|
|
.expect("failed to create victim conversation"),
|
|
"test setup failed: victim conversation was not created"
|
|
);
|
|
store
|
|
.add_conversation_message(
|
|
foreign_thread_id,
|
|
"user",
|
|
&format!("victim-only secret marker: {marker}"),
|
|
)
|
|
.await
|
|
.expect("failed to seed victim conversation message");
|
|
|
|
let before_messages = store
|
|
.list_conversation_messages(foreign_thread_id)
|
|
.await
|
|
.expect("failed to read victim conversation before forged send");
|
|
assert!(
|
|
before_messages.iter().any(|m| m.content.contains(&marker)),
|
|
"test setup failed: victim marker message missing"
|
|
);
|
|
let before_len = before_messages.len();
|
|
|
|
let forged = IncomingMessage::new("test", "test-user", "attacker turn")
|
|
.with_thread(foreign_thread_id.to_string());
|
|
rig.send_incoming(forged).await;
|
|
let responses = rig.wait_for_responses(1, Duration::from_secs(20)).await;
|
|
assert_eq!(
|
|
responses.len(),
|
|
1,
|
|
"expected one assistant response for forged-thread request"
|
|
);
|
|
assert_safe_thread_rejection(&responses[0]);
|
|
|
|
let captured = rig.captured_llm_requests();
|
|
assert!(
|
|
captured.is_empty(),
|
|
"forged thread-id request should be rejected before any LLM call"
|
|
);
|
|
let prompt_dump = captured
|
|
.iter()
|
|
.flat_map(|req| req.iter().map(|m| m.content.as_str()))
|
|
.collect::<Vec<_>>()
|
|
.join("\n");
|
|
assert!(
|
|
!prompt_dump.contains(&marker),
|
|
"forged thread_id leaked foreign marker into LLM prompt context: {prompt_dump}"
|
|
);
|
|
|
|
let after_messages = store
|
|
.list_conversation_messages(foreign_thread_id)
|
|
.await
|
|
.expect("failed to read victim conversation after forged send");
|
|
assert_eq!(
|
|
after_messages.len(),
|
|
before_len,
|
|
"forged thread_id wrote new messages into victim conversation"
|
|
);
|
|
assert!(
|
|
after_messages
|
|
.iter()
|
|
.all(|m| m.content != "attacker turn" && m.content != "safe response"),
|
|
"forged request content was persisted to victim conversation"
|
|
);
|
|
|
|
rig.shutdown();
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn forged_nonexistent_thread_id_is_rejected_and_followup_request_still_works() {
|
|
let trace = LlmTrace::single_turn(
|
|
"thread-id-isolation-nonexistent",
|
|
"real follow-up turn",
|
|
vec![TraceStep {
|
|
request_hint: None,
|
|
response: TraceResponse::Text {
|
|
content: "safe response".to_string(),
|
|
input_tokens: 12,
|
|
output_tokens: 4,
|
|
},
|
|
expected_tool_results: Vec::new(),
|
|
}],
|
|
);
|
|
|
|
let rig = TestRigBuilder::new().with_trace(trace).build().await;
|
|
|
|
let forged_thread_id = Uuid::new_v4();
|
|
let store = rig.database();
|
|
|
|
let forged = IncomingMessage::new("test", "test-user", "attacker turn")
|
|
.with_thread(forged_thread_id.to_string());
|
|
rig.send_incoming(forged).await;
|
|
let responses = rig.wait_for_responses(1, Duration::from_secs(20)).await;
|
|
assert_eq!(
|
|
responses.len(),
|
|
1,
|
|
"expected one response for forged nonexistent-thread request"
|
|
);
|
|
assert_safe_thread_rejection(&responses[0]);
|
|
assert!(
|
|
rig.captured_llm_requests().is_empty(),
|
|
"forged nonexistent thread-id request should be rejected before any LLM call"
|
|
);
|
|
assert!(
|
|
store
|
|
.get_conversation_metadata(forged_thread_id)
|
|
.await
|
|
.expect("get metadata for forged thread id")
|
|
.is_none(),
|
|
"forged nonexistent thread id must not create a conversation row"
|
|
);
|
|
|
|
rig.send_message("real follow-up turn").await;
|
|
let responses = rig.wait_for_responses(2, Duration::from_secs(20)).await;
|
|
assert_eq!(
|
|
responses.len(),
|
|
2,
|
|
"expected follow-up response after rejection"
|
|
);
|
|
assert_eq!(
|
|
responses[1].content, "safe response",
|
|
"follow-up valid request should still be handled normally"
|
|
);
|
|
assert_eq!(
|
|
rig.captured_llm_requests().len(),
|
|
1,
|
|
"only follow-up request should reach LLM"
|
|
);
|
|
|
|
rig.shutdown();
|
|
}
|
|
}
|