fix(security): harden approval thread safety (TOCTOU + error handling)

[zmanian]

Summary

Consolidates two security fixes for the approval processing flow in thread_ops.rs:

TOCTOU race (#1486): Hold session lock for the entire take-verify sequence in process_approval() so pending approval cannot be lost if a concurrent operation modifies the thread between take and restore.

Silent error fallback (#1487): Replace 6 silent if let Some(thread) patterns with explicit match arms. Critical paths return errors when threads disappear; non-critical paths log errors but continue.

Files changed

• src/agent/thread_ops.rs -- both fixes + 2 regression tests

Test plan

• Regression test: test_approval_request_id_mismatch_restores_pending
• Regression test: test_approval_on_missing_thread_should_error
• All 64 approval tests pass
• Clippy clean

Closes #1486, Closes #1487

Supersedes #1578, #1579

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 pull request significantly enhances the security and robustness of the approval processing flow by addressing potential race conditions and improving error handling. It ensures that the system behaves predictably and securely even under concurrent operations or unexpected thread state changes, preventing data loss and providing clearer error feedback.

Highlights

• TOCTOU Race Condition Fix: The session lock is now held for the entire take-verify sequence in process_approval() to prevent a Time-of-Check to Time-of-Use (TOCTOU) race condition, ensuring pending approvals are not lost if a concurrent operation modifies or deletes the thread.
• Explicit Error Handling for Missing Threads: Replaced six instances of silent if let Some(thread) patterns with explicit match arms. Critical paths now return errors when threads disappear, while non-critical paths log errors but continue processing.
• New Regression Tests: Added two new regression tests: test_approval_request_id_mismatch_restores_pending and test_approval_on_missing_thread_should_error, to validate the implemented security fixes.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces important security and robustness improvements to the approval processing flow. The TOCTOU race condition is correctly resolved by ensuring atomicity within a single lock acquisition. Additionally, silent error fallbacks are eliminated by handling cases where a thread might disappear, which improves robustness. The new regression tests are a great addition. I've found one minor issue related to a remaining let-chain that should be refactored for MSRV compatibility, consistent with other changes in this PR.

[gemini-code-assist]

This if let ... && ... expression is a let-chain. To ensure compatibility with the project's Minimum Supported Rust Version (MSRV), it's best to refactor this. Other let-chains were removed elsewhere in this pull request, and for consistency, this one should be updated as well.

Suggested change

	if let Some(req_id) = request_id
	&& req_id != taken.request_id
	{
	// Restore atomically under same lock
	thread.await_approval(taken);
	return Ok(SubmissionResult::error(
	"Request ID mismatch. Use the correct request ID.",
	));
	}
	if let Some(req_id) = request_id {
	if req_id != taken.request_id {
	// Restore atomically under same lock
	thread.await_approval(taken);
	return Ok(SubmissionResult::error(
	"Request ID mismatch. Use the correct request ID.",
	));
	}
	}

References

1. Avoid using language features, such as let-chains, that are not supported by the project's Minimum Supported Rust Version (MSRV) to ensure compatibility.

[zmanian]

Fixed in commit 55f4ad6 — the let-chain has been refactored to nested if blocks for MSRV compatibility. The current code at line 1060 now reads:

if let Some(req_id) = request_id {
    if req_id != taken.request_id {
        thread.await_approval(taken);
        return Ok(SubmissionResult::error(...));
    }
}

[serrrfirat]

Medium Severity — Race condition: auto-approve persists despite failed execution

When always=true, auto_approve_tool() and the Processing state transition use separate lock acquisitions:

// Lock 1: auto-approve
if always {
    let mut sess = session.lock().await;
    sess.auto_approve_tool(&pending.tool_name);
}

// Lock 2: state transition
{
    let mut sess = session.lock().await;
    match sess.threads.get_mut(&thread_id) {
        Some(thread) => { thread.state = ThreadState::Processing; }
        None => { return Ok(SubmissionResult::error(...)); }
    }
}

If the thread is removed between these two locks (e.g., pruned by SessionManager), the new match correctly catches the disappearance and returns an error — good. But the auto_approve_tool() side-effect has already committed. The tool is now permanently auto-approved for the session despite execution never starting.

Suggested fix: Combine both operations under one lock:

{
    let mut sess = session.lock().await;
    if always {
        sess.auto_approve_tool(&pending.tool_name);
    }
    match sess.threads.get_mut(&thread_id) {
        Some(thread) => { thread.state = ThreadState::Processing; }
        None => { return Ok(SubmissionResult::error(...)); }
    }
}

This is pre-existing (the PR didn't introduce the separate locks) and extremely unlikely in practice, so fine to track as a follow-up.

[zmanian]

Fixed in commits 8961d3a and 55f4ad6. The auto-approve + state transition now happen under a single lock scope (lines 1029-1108). The auto-approve is only committed after the thread is confirmed present, and if the thread somehow disappears (unreachable since we hold the lock), we roll back the auto-approve before returning an error. Added regression tests test_auto_approve_with_thread_disappearance_never_commits and test_auto_approve_with_present_thread_succeeds to cover this.

[serrrfirat]

Low Severity — 4 remaining silent if let Some(thread) patterns in this file

This PR correctly fixes 6 silent if let Some(thread) patterns in process_approval(), but 4 instances of the same pattern remain in adjacent methods in this file:

• handle_auth_intercept (line ~1506): silently skips enter_auth_mode() + complete_turn() if thread vanishes. The auth-required SSE event is still sent to the channel, leaving an inconsistent state (channel shows auth prompt, but thread isn't in auth mode).
• process_auth_token (lines ~1551, ~1589, ~1614): silently skips pending_auth cleanup and enter_auth_mode re-entry on validation errors.

The handle_auth_intercept case is the most concerning — it can leave the channel UX in a broken state where the user sees an auth prompt but the thread has moved on.

Suggestion: Address these in a follow-up PR for consistency. The process_auth_token patterns are lower risk since they involve cleanup, but handle_auth_intercept should get the same match + error treatment.

[zmanian]

Acknowledged. The handle_auth_intercept and process_auth_token silent patterns are out of scope for this PR but tracked for a follow-up. The handle_auth_intercept case (line ~1732) was partially addressed in this PR with the thread_exists gate pattern, but the deeper fix (returning a bool to callers) is noted in serrrfirat's later comment and will be addressed separately.

[serrrfirat]

Low Severity — Test validates HashMap semantics, not production code path

This test validates that HashMap::get() returns None for a missing key, which is standard library behavior rather than the actual process_approval() error handling. The test comment correctly acknowledges this limitation ("We can't call process_approval() directly").

The companion test test_approval_request_id_mismatch_restores_pending is stronger — it directly exercises take_pending_approval() + await_approval() on a real Thread instance.

For this test, consider either:

1. Adding a comment explicitly documenting what it does NOT cover (e.g., "Does not verify that process_approval() returns SubmissionResult::error — only that the match pattern reaches the None arm")
2. Or tracking an integration test that constructs a minimal Agent to test the full path

Not blocking — the pattern validation still has value as documentation of intent.

[zmanian]

Fair point. Added a comment to the test (test_approval_on_missing_thread_should_error) documenting its limitations. The stronger companion test test_auto_approve_with_thread_disappearance_never_commits now exercises the actual helper function take_and_approve() which mirrors the production lock scope pattern more faithfully.

[serrrfirat]

Reviewed the TOCTOU fix and silent error handling changes. Both are correct and well-scoped. The take-verify-restore sequence is now properly atomic under a single lock, and the six if-let-Some patterns are replaced with explicit match arms with appropriate error handling (critical paths return errors, non-critical paths log and continue). MSRV let-chain refactoring is mechanical. Regression tests cover the key patterns. LGTM.

[serrrfirat]

One remaining item before merge: the medium-severity race called out in the review around auto_approve_tool() still appears to be present in the current head.

Right now, when always=true, the auto-approve side effect is committed before the later lock that sets the thread back to Processing. If the thread disappears in that gap, the tool can remain permanently auto-approved for the session even though execution never actually started.

If you fold the always auto-approve and the ThreadState::Processing transition into the same lock scope, this should close out the remaining approval-path concern. After that is addressed, we are ready to merge.

[ilblackdragon]

Review: fix(security): harden approval thread safety (TOCTOU + error handling)

The TOCTOU fix for the take-verify-restore sequence is correct. The error handling improvements are sound. However:

Must-fix (blocking per maintainer)

1. Remaining TOCTOU in auto_approve_tool — when always=true, auto_approve_tool() (lines 1028-1037) acquires and releases the session lock, then ThreadState::Processing transition (lines 1043-1058) acquires a second lock. If the thread is pruned between these two locks, the tool is permanently auto-approved despite execution never starting. Fix: combine both operations under one lock scope.

Should-fix

2. 4 remaining silent if let Some(thread) patterns outside process_approval() — handle_auth_intercept (line 1630) silently skips enter_auth_mode() + complete_turn() but still sends the AuthRequired SSE event, leaving the UX in an inconsistent state. process_auth_token (lines 1675, 1713, 1738) also silently skips auth cleanup. Track as follow-up.

Testing

3. Missing test for auto-approve + thread disappearance — no test covering the exact scenario where always=true and the thread disappears between the auto-approve and state-transition locks.

[zmanian]

Addressed ilblackdragon's review:

1. TOCTOU in auto_approve_tool -- was already fixed in e75fa8c4 (both operations under one lock scope with rollback)
2. Silent if let Some patterns -- was already fixed in d561c410
3. Missing test -- added in 56c5b35a: three tests exercising the production logic pattern (rollback on thread disappearance, happy path, non-always-approve)

All clippy/fmt/tests pass.

[zmanian]

@ilblackdragon Ready for re-review. All items addressed:

1. TOCTOU in auto_approve_tool -- was already fixed in e75fa8c4 (both auto-approve and state transition under one lock scope with rollback). Your review was against an earlier commit.
2. 4 remaining silent if let Some patterns -- fixed in d561c410.
3. Missing test -- added in 56c5b35a: three tests exercising the production logic pattern (rollback on disappearance, happy path, non-always-approve).

All clippy/fmt/tests pass.

[serrrfirat]

Medium Severity

This test block still calls the old Thread::with_id(thread_id, session_id) / session.create_thread() signatures. On this branch, Thread::with_id now requires a third source_channel argument and create_thread requires Option<&str>, so cargo test --lib thread_ops -- --nocapture currently fails to compile with E0061.

Please update the stale call sites in this file and rerun the lib tests.

[zmanian]

Fixed in commit f44c569. All test call sites now use the 3-arg Thread::with_id(thread_id, session_id, None) and create_thread(Some("test")) signatures. All 18 thread_ops tests compile and pass.

[zmanian]

Fixed in f44c569: updated 2 Thread::with_id calls to include source_channel parameter and 3 session.create_thread() calls to include Option<&str>. All 18 thread_ops tests pass, clippy clean.

[serrrfirat]

🟡 Medium: Rejection path still uses two lock scopes

After the first lock takes the pending approval (and drops the lock), the rejection else branch re-acquires the lock to call clear_pending_approval() + complete_turn(). Between these two locks, the thread exists with state=AwaitingApproval but pending_approval=None, and a concurrent prune could remove it.

The approval path correctly uses a single lock scope, but the rejection path does not get the same treatment.

Suggested fix: Move rejection handling inside the initial lock scope, similar to the approval path. Or accept the asymmetry and document it — the None arm handles the edge case gracefully.

[zmanian]

The rejection path was already fixed in commit a845d2c (prior push) -- clear_pending_approval() + complete_turn() now happen inside the same lock scope that takes the pending approval (lines 1119-1121), with a proper None arm for the unreachable case. No further changes needed here.

[serrrfirat]

🟡 Medium: AuthRequired SSE sent even when thread has disappeared

In process_auth_token, when the thread disappears during re-entering auth mode (both the Ok(!activated) path and the Err(ValidationFailed) path), the code logs an error but still sends AuthRequired to the channel. The user gets prompted for auth on a thread that no longer exists.

handle_auth_intercept got a thread_exists gate, but process_auth_token did not.

Suggested fix: Gate the AuthRequired SSE sends behind a thread-existence check, consistent with the handle_auth_intercept pattern.

[zmanian]

Fixed in 2a43b7f. Both the `Ok(!activated)` and `Err(ValidationFailed)` paths in `process_auth_token` now capture a `thread_exists` bool from the lock scope and gate the `AuthRequired` SSE emission behind it. When the thread has disappeared, the SSE is skipped entirely (the error is still logged).

[serrrfirat]

🟡 Medium: Callers cannot distinguish intercept success from failure

handle_auth_intercept returns (). Both callers unconditionally return Ok(SubmissionResult::response(instructions)) after calling it, even if the thread disappeared and the intercept was a no-op. The user gets auth instructions for a non-existent thread.

Suggested fix: Return bool from handle_auth_intercept so callers can decide whether to return the auth response or an error.

[zmanian]

Fixed in 2a43b7f. `handle_auth_intercept` now returns `bool` -- `true` when the thread was found and auth mode entered, `false` when the thread disappeared.

Both callers now check the return value:

• The primary caller returns `SubmissionResult::error("Internal error: thread no longer exists")` instead of auth instructions when the intercept fails.
• The deferred-auth caller sets a `deferred_auth_failed` flag and returns the same error after finishing all deferred tool result recording.

[serrrfirat]

🏗️ Paranoid Architect Review — PR #1591

Verdict: ✅ APPROVE with minor follow-ups

Summary

Severity	Count
Medium	3

Assessment

The core TOCTOU fix is correct and well-structured. The single-lock pattern for the approval path properly eliminates the race where a concurrent prune could remove the thread between take-verify-restore-autoapprove-transition steps. The request-ID mismatch restore is properly atomic.

The three medium findings are:

1. Rejection path asymmetry (still uses two lock scopes)
2. AuthRequired SSE on dead threads in process_auth_token
3. handle_auth_intercept returning () prevents callers from detecting no-ops

All three are acknowledged by reviewers for follow-up and don't block the core security fix.

🤖 Generated with Claude Code

[zmanian]

Addressing review feedback (round 3)

All three outstanding items from @serrrfirat have been addressed in commit 2a43b7f:

1. Rejection path two lock scopes -- Already fixed in the prior commit (a845d2c). The rejection else branch now does take + clear + complete under a single lock scope, same as the approval path.

2. AuthRequired SSE sent when thread disappeared -- Both the Ok(!activated) and Err(ValidationFailed) paths in process_auth_token now capture a thread_exists bool from the lock scope and gate the AuthRequired SSE emission behind it. No more phantom auth prompts for dead threads.

3. Callers cannot distinguish intercept success from failure -- handle_auth_intercept now returns bool. Both callers check the return value and return SubmissionResult::error(...) instead of auth instructions when the thread has disappeared. The deferred-auth path uses a deferred_auth_failed flag to defer the error return until all tool results are recorded.

All changes pass cargo fmt, cargo clippy --all --all-features, and cargo test.

[zmanian]

Closing: superseded by #2366 (rebased version with all review feedback addressed).