fix(agent): block thread_id-based context pollution across users - #760
Conversation
github-actions
Bot
added
scope: agent
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This PR addresses a critical security vulnerability where a malicious actor could manipulate Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a high-severity context pollution risk by implementing robust ownership validation during thread hydration and persistence. The introduction of the ensure_writable_conversation helper centralizes critical security logic, making the codebase more secure and maintainable. The new end-to-end test e2e_thread_id_isolation.rs provides excellent coverage, verifying that forged thread_ids do not lead to cross-user data leakage or persistence. The refactoring to use the new helper function across various persistence paths is well-executed, enhancing the overall integrity of conversation management.
There was a problem hiding this comment.
Pull request overview
Addresses a high-severity cross-user context pollution risk by preventing hydration/persistence against a forged thread_id that targets another user’s conversation UUID.
Changes:
- Added ownership validation before hydrating conversation history from the DB during thread restoration.
- Introduced a guarded persistence helper to ensure DB writes only occur for user-owned or newly-created conversations.
- Added an E2E regression test covering forged
thread_idisolation across users.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/agent/thread_ops.rs |
Adds ownership checks for hydration and a shared guard for safe persistence to prevent cross-user thread ID abuse. |
tests/e2e_thread_id_isolation.rs |
Adds an end-to-end test ensuring forged thread IDs do not hydrate or persist foreign conversation data. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !owned { | ||
| tracing::warn!( | ||
| user = %message.user_id, | ||
| thread_id = %thread_uuid, | ||
| "Rejected hydration for unowned thread id" |
There was a problem hiding this comment.
maybe_hydrate_thread now rejects hydration whenever conversation_belongs_to_user returns false. That includes the common case where the conversation row does not exist yet (brand-new thread UUID from the frontend), which defeats the function’s stated purpose of creating an in-memory thread with the exact external UUID and will cause resolve_thread to mint a new internal UUID (breaking persistence into the intended conversation). Consider distinguishing "missing conversation" from "foreign conversation" (e.g., if belongs=false then check get_conversation_metadata(thread_uuid): allow/initialize empty thread on None, but reject on Some(_)).
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Security Review: thread_id-based context pollution fix
This PR addresses a real and high-severity vulnerability -- a client submitting a forged thread UUID could hydrate a foreign user's conversation history into the LLM prompt and persist attacker-controlled messages into the victim's conversation. The overall approach (ownership checks at hydration + persistence) is correct and necessary. However, there are several issues that should be addressed before merging.
Critical: TOCTOU race in ensure_writable_conversation
The new ensure_writable_conversation method performs three sequential non-atomic database calls:
conversation_belongs_to_user(thread_id, user_id)-- returnsfalseget_conversation_metadata(thread_id)-- returnsNone(not yet created)ensure_conversation(thread_id, "gateway", user_id, None)-- INSERT ... ON CONFLICT DO UPDATE
Between steps 2 and 3, a concurrent request from a different user could create the same conversation UUID (e.g., a race between two users both trying to claim the same uncreated UUID). The ON CONFLICT (id) DO UPDATE SET last_activity = ... upsert in both postgres and libsql backends does NOT check user_id on the conflict path -- it just bumps last_activity. This means:
- User A's request checks: no conversation exists -> proceeds to step 3
- User B's request creates the conversation with their
user_idbetween steps 2 and 3 - User A's
ensure_conversationhits theON CONFLICTpath, bumpslast_activitybut does NOT changeuser_id - The post-check at step 4 (
conversation_belongs_to_useragain) correctly catches this
So the post-insert re-check (the third conversation_belongs_to_user call) does mitigate the race for the attacker, but the ensure_conversation call still unnecessarily bumps last_activity on the victim's conversation. This is a minor side effect but worth noting.
More importantly, this three-query dance should be a single transaction or a conditional INSERT. Consider:
INSERT INTO conversations (id, channel, user_id, ...)
VALUES ($1, $2, $3, ...)
ON CONFLICT (id) DO NOTHINGThen check conversation_belongs_to_user once after. This avoids the metadata lookup entirely and is both simpler and safer. The DO NOTHING variant means a concurrent insert from another user simply fails silently, and the post-check catches it.
Issue: ensure_conversation upsert semantics are unsafe for multi-user
The underlying ensure_conversation uses ON CONFLICT (id) DO UPDATE SET last_activity = NOW(). In a multi-user context, this is dangerous -- if an attacker guesses a valid conversation UUID, calling ensure_conversation will silently bump the victim's last_activity timestamp even though the ownership check later rejects the write. This leaks timing information (the victim sees their conversation's last_activity change) and is a minor integrity violation. The ensure_conversation contract should probably be tightened: either the upsert should verify user_id matches on conflict, or ensure_writable_conversation should use a conditional INSERT instead.
Issue: Hydration guard uses early return silently
In maybe_hydrate_thread, when ownership validation fails, the method does return; silently (after logging). The caller (process_user_input or wherever this is invoked) has no way to know that hydration was rejected vs. simply no history existed. For a security boundary, failing loudly is preferable. Consider:
- Returning a
Resultorboolso the caller can decide whether to proceed or reject the message entirely - Currently, a forged thread_id that fails hydration will still be processed as a new conversation (the agent will respond). The attacker doesn't get the victim's history, but the response still goes through. Is that the intended behavior? If so, document it.
Issue: "gateway" channel hardcoded in ensure_writable_conversation
The method hardcodes "gateway" as the channel when creating a new conversation:
store.ensure_conversation(thread_id, "gateway", user_id, None).awaitThis means the guard only works correctly for the gateway channel. If other channels (Telegram, webhook, REPL) ever flow through this code path, the channel metadata will be wrong. Consider passing the channel from the incoming message context.
Nit: Redundant ownership check pattern
ensure_writable_conversation calls conversation_belongs_to_user up to 3 times in the worst case (initial check, implicit in ensure, post-ensure re-check). The logic would be cleaner as:
// Try conditional insert first (no-op if already exists)
store.ensure_conversation_if_new(thread_id, user_id, ...).await?; // INSERT ... ON CONFLICT DO NOTHING
// Single ownership check
store.conversation_belongs_to_user(thread_id, user_id).awaitTest coverage
The regression test is well-structured and covers the key scenarios (no hydration of foreign history, no persistence into foreign conversation). It runs under libsql, which is good. A few suggestions:
- Consider also testing the case where the attacker uses a UUID that does not exist at all (not just one owned by another user). The current
ensure_writable_conversationshould handle this correctly by creating a new conversation owned by the attacker, but it's worth having an explicit assertion. - The test only covers the libsql backend (
#[cfg(feature = "libsql")]). The postgres backend has identical SQL semantics here, but the ownership check is a security boundary -- consider adding a note or tracking issue for postgres integration test coverage.
Summary
The fix correctly identifies and addresses the core vulnerability. The hydration guard and persistence guards are placed at the right points. However, the ensure_writable_conversation implementation has TOCTOU concerns and the underlying ensure_conversation upsert semantics are arguably too permissive for a multi-user environment. I'd recommend:
- Switch
ensure_conversationtoON CONFLICT DO NOTHING(or add auser_id-aware variant) to avoid touching foreign conversations - Simplify
ensure_writable_conversationto: conditional-insert + single ownership check - Consider whether the hydration rejection should propagate to the caller (reject the message vs. silently start a new conversation)
- Avoid hardcoding
"gateway"channel
github-actions
Bot
added
scope: channel/web
daead7c to
12a7a8f
Compare
github-actions
Bot
added
size: XL
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/channels/web/server.rs:1227
- When
ensure_conversationreturnsOk(false)(UUID conflict with a different(channel,user_id)), the handler still callsupdate_conversation_metadata_field(thread_id, ...). Sinceupdate_conversation_metadata_fieldupdates byidonly (no ownership filter), this can mutate metadata on a foreign conversation row despite detecting the conflict. Gate the metadata update onOk(true)(and ideally return an error / retry with a new UUID onOk(false)), so no writes occur against an unowned conversation ID.
Ok(true) => {}
Ok(false) => tracing::warn!(
user = %state.user_id,
thread_id = %thread_id,
"Skipped persisting new thread due to ownership/channel conflict"
),
Err(e) => tracing::warn!("Failed to persist new thread: {}", e),
}
let metadata_val = serde_json::json!("thread");
if let Err(e) = store
.update_conversation_metadata_field(thread_id, "thread_type", &metadata_val)
.await
src/channels/web/handlers/chat.rs:544
ensure_conversationreturningOk(false)indicates thethread_idalready exists for a different(channel,user_id), but the code still unconditionally callsupdate_conversation_metadata_field(thread_id, ...). Because that update is keyed only byid, this can write metadata into a foreign conversation row even after detecting the conflict. Only update metadata whenensure_conversationreturnsOk(true)(and consider surfacing an error/retry path onOk(false)), to keep all persistence behind the ownership/channel guard.
Ok(true) => {}
Ok(false) => tracing::warn!(
user = %state.user_id,
thread_id = %thread_id,
"Skipped persisting new thread due to ownership/channel conflict"
),
Err(e) => tracing::warn!("Failed to persist new thread: {}", e),
}
let metadata_val = serde_json::json!("thread");
if let Err(e) = store
.update_conversation_metadata_field(thread_id, "thread_type", &metadata_val)
.await
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3055c62 to
18f4eeb
Compare
github-actions
Bot
added
scope: channel
github-actions
Bot
added
scope: setup
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 69 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
.github/workflows/staging-ci.yml:122
- The
actions/create-github-app-token@v2step is no longer guarded. On forks / environments whereGH_RELEASES_MANAGER_APP_IDandGH_RELEASES_MANAGER_APP_PRIVATE_KEYare unset, this step will fail before you can fall back togithub.token. Restore the conditional guard (or addcontinue-on-error: true) so the workflow remains functional without those secrets.
- name: Generate GitHub App token
id: app-token
uses: actions/create-github-app-token@v2
with:
app-id: ${{ secrets.GH_RELEASES_MANAGER_APP_ID }}
private-key: ${{ secrets.GH_RELEASES_MANAGER_APP_PRIVATE_KEY }}
.github/workflows/staging-ci.yml:236
- Same issue as earlier in this workflow: this
create-github-app-tokenstep is unconditionally executed, so missingGH_RELEASES_MANAGER_APP_*secrets will fail the job and prevent the intended fallback togithub.token. Add a guard orcontinue-on-errorto keep staging CI usable in environments without those secrets.
- name: Generate GitHub App token
id: app-token
uses: actions/create-github-app-token@v2
with:
app-id: ${{ secrets.GH_RELEASES_MANAGER_APP_ID }}
private-key: ${{ secrets.GH_RELEASES_MANAGER_APP_PRIVATE_KEY }}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let tool_defs = ctx.tools.tool_definitions().await; | ||
|
|
||
| let request = ToolCompletionRequest::new(messages.clone(), tool_defs) | ||
| .with_max_tokens(effective_max_tokens) | ||
| .with_temperature(0.3); | ||
|
|
There was a problem hiding this comment.
execute_lightweight_with_tools exposes all tool definitions to the LLM (ctx.tools.tool_definitions()), but execute_routine_tool later blocks UnlessAutoApproved/Always tools. This makes the model likely to select tools that will be rejected, wasting iterations and producing noisy error tool-results. Consider filtering the advertised tool definitions up-front to only the subset allowed in lightweight routines (e.g., tools whose requires_approval is Never).
|
|
||
| // ── Tunnel setup ─────────────────────────────────────────────────── | ||
|
|
||
| let (config, active_tunnel) = start_tunnel(config).await; | ||
| let (config, active_tunnel) = ironclaw::tunnel::start_managed_tunnel(config).await; | ||
|
|
||
| // ── Orchestrator / container job manager ──────────────────────────── | ||
|
|
||
| // Proactive Docker detection | ||
| let docker_status = if config.sandbox.enabled { | ||
| let detection = ironclaw::sandbox::check_docker().await; | ||
| match detection.status { | ||
| ironclaw::sandbox::DockerStatus::Available => { | ||
| tracing::info!("Docker is available"); | ||
| } | ||
| ironclaw::sandbox::DockerStatus::NotInstalled => { | ||
| tracing::warn!( | ||
| "Docker is not installed -- sandbox disabled for this session. {}", | ||
| detection.platform.install_hint() | ||
| ); | ||
| } | ||
| ironclaw::sandbox::DockerStatus::NotRunning => { | ||
| tracing::warn!( | ||
| "Docker is installed but not running -- sandbox disabled for this session. {}", | ||
| detection.platform.start_hint() | ||
| ); | ||
| } | ||
| ironclaw::sandbox::DockerStatus::Disabled => {} | ||
| } | ||
| detection.status | ||
| } else { | ||
| ironclaw::sandbox::DockerStatus::Disabled | ||
| }; | ||
|
|
||
| let job_event_tx: Option< | ||
| tokio::sync::broadcast::Sender<(uuid::Uuid, ironclaw::channels::web::types::SseEvent)>, | ||
| > = if config.sandbox.enabled && docker_status.is_ok() { | ||
| let (tx, _) = tokio::sync::broadcast::channel(256); | ||
| Some(tx) | ||
| } else { | ||
| None | ||
| }; | ||
| let prompt_queue = Arc::new(tokio::sync::Mutex::new(std::collections::HashMap::< | ||
| uuid::Uuid, | ||
| std::collections::VecDeque<ironclaw::orchestrator::api::PendingPrompt>, | ||
| >::new())); | ||
|
|
||
| let container_job_manager: Option<Arc<ContainerJobManager>> = | ||
| if config.sandbox.enabled && docker_status.is_ok() { | ||
| let token_store = TokenStore::new(); | ||
| let job_config = ContainerJobConfig { | ||
| image: config.sandbox.image.clone(), | ||
| memory_limit_mb: config.sandbox.memory_limit_mb, | ||
| cpu_shares: config.sandbox.cpu_shares, | ||
| orchestrator_port: 50051, | ||
| claude_code_api_key: std::env::var("ANTHROPIC_API_KEY").ok(), | ||
| claude_code_oauth_token: ironclaw::config::ClaudeCodeConfig::extract_oauth_token(), | ||
| claude_code_model: config.claude_code.model.clone(), | ||
| claude_code_max_turns: config.claude_code.max_turns, | ||
| claude_code_memory_limit_mb: config.claude_code.memory_limit_mb, | ||
| claude_code_allowed_tools: config.claude_code.allowed_tools.clone(), | ||
| }; | ||
| let jm = Arc::new(ContainerJobManager::new(job_config, token_store.clone())); | ||
|
|
||
| // Start the orchestrator internal API in the background | ||
| let orchestrator_state = OrchestratorState { | ||
| llm: components.llm.clone(), | ||
| job_manager: Arc::clone(&jm), | ||
| token_store, | ||
| job_event_tx: job_event_tx.clone(), | ||
| prompt_queue: Arc::clone(&prompt_queue), | ||
| store: components.db.clone(), | ||
| secrets_store: components.secrets_store.clone(), | ||
| user_id: "default".to_string(), | ||
| }; | ||
|
|
||
| tokio::spawn(async move { | ||
| if let Err(e) = OrchestratorApi::start(orchestrator_state, 50051).await { | ||
| tracing::error!("Orchestrator API failed: {}", e); | ||
| } | ||
| }); | ||
|
|
||
| if config.claude_code.enabled { | ||
| tracing::info!( | ||
| "Claude Code sandbox mode available (model: {}, max_turns: {})", | ||
| config.claude_code.model, | ||
| config.claude_code.max_turns | ||
| ); | ||
| } | ||
| Some(jm) | ||
| } else { | ||
| None | ||
| }; | ||
| let orch = ironclaw::orchestrator::setup_orchestrator( | ||
| &config, | ||
| &components.llm, | ||
| components.db.as_ref(), | ||
| components.secrets_store.as_ref(), | ||
| ) | ||
| .await; | ||
| let container_job_manager = orch.container_job_manager; | ||
| let job_event_tx = orch.job_event_tx; | ||
| let prompt_queue = orch.prompt_queue; | ||
| let docker_status = orch.docker_status; |
There was a problem hiding this comment.
The PR description focuses on thread_id ownership validation, but this diff also includes broad refactors and behavior changes (orchestrator/tunnel setup extraction, onboarding quick mode, MCP factory, tracing level changes, tool approval changes, etc.). This scope mismatch makes it hard to reason about risk for a security fix; consider splitting the unrelated refactors into separate PRs so the thread_id isolation change can be reviewed and landed independently.
| Ok(McpClient::new_with_transport( | ||
| &server_name, | ||
| transport as Arc<dyn McpTransport>, | ||
| None, | ||
| secrets, | ||
| user_id, | ||
| Some(server), | ||
| )) |
There was a problem hiding this comment.
spawn_stdio() returns Arc<StdioMcpTransport>, but this code attempts to cast it with as Arc<dyn McpTransport>. Arc<T> → Arc<dyn Trait> should use trait-object coercion (e.g., bind to Arc<dyn McpTransport> / Arc::from(transport)), not an as cast; as written this is likely a compile error and blocks MCP stdio support.
| Ok(McpClient::new_with_transport( | ||
| &server_name, | ||
| Arc::new(transport) as Arc<dyn McpTransport>, | ||
| None, | ||
| secrets, | ||
| user_id, | ||
| Some(server), | ||
| )) |
There was a problem hiding this comment.
The Unix transport path uses Arc::new(transport) as Arc<dyn McpTransport>. Similar to the stdio case, prefer trait-object coercion to Arc<dyn McpTransport> rather than an as cast; otherwise this is likely to fail to compile or behave unexpectedly across Rust versions.
18f4eeb to
fc46fff
Compare
github-actions
Bot
added
risk: medium
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review: thread_id-based context pollution fix
All four issues from my previous review have been addressed in the new commits:
-
TOCTOU race / ensure_conversation upsert semantics -- Fixed correctly. The
ON CONFLICT DO UPDATEnow has aWHEREclause checkinguser_idandchannelmatch on both postgres and libsql backends. This is the right approach -- a single atomic SQL statement that returns affected-rows > 0 only when the row is newly inserted or belongs to the same user. No more multi-query dance. The return type change fromResult<()>toResult<bool>cleanly communicates ownership rejection. -
Hydration guard returns silently -- Fixed.
maybe_hydrate_threadnow returnsOption<String>, and the caller inagent_loop.rspropagates the rejection as an error response. Gateway/test channels get hard rejection before any LLM call. Good. -
Hardcoded "gateway" channel -- Fixed. All persist methods now accept
channelfrommessage.channel. -
Redundant ownership checks -- Fixed.
ensure_writable_conversationis now a thin wrapper around the singleensure_conversationcall + boolean check.
New design observations
The requires_preexisting_uuid_thread helper distinguishing "gateway"/"test" channels from others is a reasonable policy choice. Gateway clients should never be creating conversations by providing unknown UUIDs (the server issues them), so rejecting unknown UUIDs is correct. Non-gateway channels that happen to pass UUID-shaped thread IDs get the softer behavior (skip hydration, proceed as new conversation).
The error path in maybe_hydrate_thread where conversation_belongs_to_user returns an Err (DB failure) defaults to rejection for gateway channels and permissive for others. This is the right fail-closed posture for the security-critical path.
Test coverage
Good regression tests:
test_ensure_conversation_foreign_conflict_does_not_touch_last_activityverifies the SQL-level guard (attacker cannot bumplast_activity)- E2E test covers both foreign-existing and nonexistent thread UUIDs
- Verifies no LLM calls leak foreign context and no messages are persisted to victim conversation
- Verifies agent still works for legitimate requests after rejection
Minor notes for future
- The E2E test and the
test_ensure_conversation_foreign_conflict_does_not_touch_last_activitytest are both libsql-only. Worth tracking a postgres integration test for the same SQL guard, since the two SQL dialects are slightly different (EXCLUDEDvsexcluded). Not a blocker. - The Copilot comments about unrelated scope (orchestrator refactor, MCP factory, etc.) appear to be noise from a stale diff on a different base -- the current diff only touches thread_id isolation files. Ignore those.
Security fix looks correct and complete. Approving.
fc46fff to
16979dc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/channels/web/server.rs:1436
ensure_conversationcan now returnOk(false)when the UUID already exists under a different (channel, user_id). In that case this handler still callsupdate_conversation_metadata_field(thread_id, ...), which updates byidonly and would mutate the existing foreign row. Consider only updating metadata whenensure_conversationreturnsOk(true)(and skipping/returning an error onOk(false)).
match store
.ensure_conversation(thread_id, "gateway", &state.user_id, None)
.await
{
Ok(true) => {}
Ok(false) => tracing::warn!(
user = %state.user_id,
thread_id = %thread_id,
"Skipped persisting new thread due to ownership/channel conflict"
),
Err(e) => tracing::warn!("Failed to persist new thread: {}", e),
}
let metadata_val = serde_json::json!("thread");
if let Err(e) = store
.update_conversation_metadata_field(thread_id, "thread_type", &metadata_val)
.await
src/channels/web/handlers/chat.rs:568
ensure_conversationcan now returnOk(false)when the UUID already exists under a different (channel, user_id). In that case this handler still callsupdate_conversation_metadata_field(thread_id, ...), which updates byidonly and would mutate the existing foreign row. Consider only updating metadata whenensure_conversationreturnsOk(true)(and skipping/returning an error onOk(false)).
match store
.ensure_conversation(thread_id, "gateway", &state.user_id, None)
.await
{
Ok(true) => {}
Ok(false) => tracing::warn!(
user = %state.user_id,
thread_id = %thread_id,
"Skipped persisting new thread due to ownership/channel conflict"
),
Err(e) => tracing::warn!("Failed to persist new thread: {}", e),
}
let metadata_val = serde_json::json!("thread");
if let Err(e) = store
.update_conversation_metadata_field(thread_id, "thread_type", &metadata_val)
.await
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let thread_uuid = match Uuid::parse_str(external_thread_id) { | ||
| Ok(id) => id, | ||
| Err(_) => return, | ||
| Err(_) => return None, |
There was a problem hiding this comment.
For gateway/test channels you appear to require server-issued UUID thread IDs, but invalid/non-UUID external_thread_id currently returns None (no rejection). That allows clients to bypass the preexisting-UUID requirement by sending a non-UUID thread_id, causing resolve_thread to create a new thread keyed by that string. Consider returning the same safe rejection message when Uuid::parse_str fails and requires_preexisting_uuid_thread(&message.channel) is true.
| Err(_) => return None, | |
| Err(_) => { | |
| // For gateway-style channels that require server-issued UUIDs, | |
| // reject non-UUID / malformed thread IDs instead of silently | |
| // letting a new thread be created for them. | |
| if requires_preexisting_uuid_thread(&message.channel) { | |
| return Some(FORGED_THREAD_ID_ERROR.to_string()); | |
| } | |
| return None; | |
| } |
| if let Some(store) = self.store() { | ||
| // Never hydrate history from a conversation UUID that isn't owned | ||
| // by the current authenticated user. | ||
| let owned = match store | ||
| .conversation_belongs_to_user(thread_uuid, &message.user_id) | ||
| .await |
There was a problem hiding this comment.
Hydration authorization checks only conversation_belongs_to_user(id, user_id) (no channel), but writes are later guarded by ensure_conversation(id, channel, user_id, ...) which enforces both user and channel. This mismatch can lead to a thread being hydrated from a different channel for the same user and then all persistence being rejected due to channel conflict. Consider validating (id, channel, user_id) consistently during hydration (or explicitly documenting/handling the cross-channel case).
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review: thread_id-based context pollution fix (post-rebase)
Reviewing the 3-commit series after the previous review was dismissed due to new commits.
Previous issues -- all addressed
-
TOCTOU race / ensure_conversation upsert -- Fixed. Both postgres (
src/history/store.rs) and libsql (src/db/libsql/conversations.rs) now useON CONFLICT (id) DO UPDATE ... WHERE conversations.user_id = EXCLUDED.user_id AND conversations.channel = EXCLUDED.channel. This is a single atomic statement that returns affected-rows > 0 only when the row is new or belongs to the same (user, channel). Clean fix. -
Silent hydration rejection -- Fixed.
maybe_hydrate_threadnow returnsOption<String>, caller inagent_loop.rspropagates it as an error response. Gateway/test channels get hard rejection before any LLM call reaches the provider. -
Hardcoded "gateway" channel -- Fixed. All
persist_*methods now acceptchannelfrommessage.channeland pass it through toensure_writable_conversation. -
Redundant ownership checks -- Fixed.
ensure_writable_conversationis now a singleensure_conversationcall + boolean check. No more multi-query dance.
New code analysis
requires_preexisting_uuid_thread -- Reasonable policy. Gateway/test channels require server-issued UUIDs; unknown UUIDs are rejected. Other channels get softer behavior (skip hydration, proceed as new conversation). The matches!(channel, "gateway" | "test") is tight and appropriate.
SQL correctness -- The WHERE clause on the ON CONFLICT DO UPDATE is correct for both dialects. PostgreSQL uses EXCLUDED (uppercase by convention but case-insensitive), libsql uses excluded (lowercase) -- both are valid for their respective engines. The affected > 0 check works because: INSERT succeeds (affected=1, new row), or UPDATE matches WHERE (affected=1, owned row refreshed), or UPDATE WHERE fails (affected=0, foreign row untouched).
Test coverage -- test_ensure_conversation_foreign_conflict_does_not_touch_last_activity directly verifies the SQL guard. E2E test covers both foreign-existing and nonexistent thread UUIDs. Both verify no LLM context leakage and no foreign message persistence. The follow-up-after-rejection test is a nice liveness check.
Remaining minor items (non-blocking)
-
update_conversation_metadata_fieldnot guarded onOk(false)-- In bothsrc/channels/web/handlers/chat.rsandsrc/channels/web/server.rs, whenensure_conversationreturnsOk(false)(UUID conflict), the code logs a warning but still falls through to callupdate_conversation_metadata_field(thread_id, "thread_type", ...). That method updates byidalone with no ownership filter, so it could mutate metadata on a foreign conversation row. In practice this is low-risk inchat_new_thread_handler(the UUID is freshly generated byUuid::new_v4()so collision is astronomically unlikely), but for defense-in-depth the metadata update should be gated onOk(true). Worth a follow-up. -
Non-UUID thread_id bypass for gateway channels -- Copilot flagged this: if a gateway client sends a non-UUID
thread_id,Uuid::parse_strfails andmaybe_hydrate_threadreturnsNone(no rejection). The message proceeds toresolve_threadwhich creates a new thread keyed by that string. This bypasses therequires_preexisting_uuid_threadguard. Since the gateway should only ever issue UUID thread IDs, consider rejecting non-UUID thread IDs for gateway channels too. Low-risk since the attacker just gets their own new thread, but it violates the stated invariant. Worth a follow-up. -
libsql-only test coverage -- Both the unit test and E2E test are
#[cfg(feature = "libsql")]. The postgres SQL usesEXCLUDED(standard) while libsql usesexcluded-- worth tracking a postgres integration test for the same guard. Not blocking since the SQL semantics are equivalent.
Verdict
The core security fix is correct, complete, and well-tested. The atomic upsert approach is the right design. The remaining items are defense-in-depth improvements that don't affect the security boundary this PR establishes. Approving.
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Re-review: thread_id-based context pollution fix (post-rebase)
Reviewing the 3-commit series after the previous review was dismissed due to new commits.
Previous issues -- all addressed
-
TOCTOU race / ensure_conversation upsert -- Fixed. Both postgres and libsql now use ON CONFLICT (id) DO UPDATE ... WHERE conversations.user_id = EXCLUDED.user_id AND conversations.channel = EXCLUDED.channel. Single atomic statement, returns affected-rows > 0 only when the row is new or belongs to the same (user, channel).
-
Silent hydration rejection -- Fixed. maybe_hydrate_thread now returns Option, caller in agent_loop.rs propagates it as an error response. Gateway/test channels get hard rejection before any LLM call.
-
Hardcoded gateway channel -- Fixed. All persist_* methods now accept channel from message.channel.
-
Redundant ownership checks -- Fixed. ensure_writable_conversation is now a single ensure_conversation call + boolean check.
New code analysis
requires_preexisting_uuid_thread -- Reasonable policy. Gateway/test channels require server-issued UUIDs; unknown UUIDs are rejected. Other channels get softer behavior.
SQL correctness -- The WHERE clause on ON CONFLICT DO UPDATE is correct for both dialects. affected > 0 works because: INSERT succeeds (1, new row), or UPDATE matches WHERE (1, owned row refreshed), or UPDATE WHERE fails (0, foreign row untouched).
Test coverage -- test_ensure_conversation_foreign_conflict_does_not_touch_last_activity verifies the SQL guard. E2E tests cover foreign-existing and nonexistent thread UUIDs. Both verify no LLM context leakage and no foreign persistence.
Remaining minor items (non-blocking)
-
update_conversation_metadata_field not guarded on Ok(false) -- In both chat.rs handlers, when ensure_conversation returns Ok(false), code still calls update_conversation_metadata_field which updates by id with no ownership filter. Low-risk since UUID is freshly minted by new_v4(), but for defense-in-depth the metadata update should be gated on Ok(true). Worth a follow-up.
-
Non-UUID thread_id bypass for gateway channels -- If a gateway client sends a non-UUID thread_id, Uuid::parse_str fails and maybe_hydrate_thread returns None (no rejection). This bypasses the requires_preexisting_uuid_thread guard. Low-risk since attacker just gets their own thread, but violates the stated invariant. Worth a follow-up.
-
libsql-only test coverage -- Both tests are cfg(feature = libsql). Worth tracking a postgres integration test. Not blocking since SQL semantics are equivalent.
Verdict
The core security fix is correct, complete, and well-tested. The atomic upsert approach is the right design. Remaining items are defense-in-depth improvements that don't affect the security boundary. Approving.
…rai#760) * fix(agent): prevent forged thread UUID context/write contamination * fix(agent): close thread_id race and reject forged UUID hydration * fix(ci): satisfy clippy and fmt checks after rebase
…rai#760) * fix(agent): prevent forged thread UUID context/write contamination * fix(agent): close thread_id race and reject forged UUID hydration * fix(ci): satisfy clippy and fmt checks after rebase
Summary
This PR fixes a high-severity context pollution risk where a client could submit a forged
thread_idUUID and cause cross-user conversation hydration/persistence against a foreign conversation ID.Why this is severe
What changed
Regression test
tests/e2e_thread_id_isolation.rsto verify a forgedthread_id:Validation
Ran:
OPENSSL_DIR=/usr OPENSSL_LIB_DIR=/usr/lib/x86_64-linux-gnu OPENSSL_INCLUDE_DIR=/usr/include cargo test --test e2e_thread_id_isolation --no-default-features --features libsql -- --nocaptureResult:
1 passed; 0 failed