fix(core): fix SpeculationEngine sweeper abort and requires_confirmation default#3647
Merged
fix(core): fix SpeculationEngine sweeper abort and requires_confirmation default#3647
Conversation
…ion default Fixes two bugs found in the security audit of PR #3640: 1. ErasedToolExecutor::requires_confirmation_erased defaulted to false, allowing speculative dispatch without confirmation for any future executor that opts into is_tool_speculatable_erased but forgets to override the gate. Inverted the default to true (safe-by-default). Added requires_confirmation to ToolExecutor (default false) with a blanket impl delegation. TrustGateExecutor overrides requires_confirmation to mirror check_trust. All existing direct ErasedToolExecutor impls updated to explicitly return false. 2. SpeculationEngine::new (supervisor=None branch) used mem::forget on the sweeper JoinHandle and immediately dropped the AbortHandle, leaking the tokio task until runtime shutdown. Replaced the dummy TaskHandle approach with a SweepHandle enum (Supervised(TaskHandle) | Raw(JoinHandle<()>)) that calls abort(self) in Drop. Closes #3644, Closes #3645
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ErasedToolExecutor::requires_confirmation_eraseddefault inverted fromfalsetotrue(safe-by-default). AddedToolExecutor::requires_confirmation(defaultfalse) with blanket impl delegation.TrustGateExecutoroverrides to mirrorcheck_trustpolicy. All 6 directErasedToolExecutorimpls updated to explicitly returnfalse. Closes fix(core): invert requires_confirmation_erased default to prevent unsafe speculative dispatch #3644.SpeculationEnginesweeper task was leaked in thesupervisor=Nonebranch due tomem::forget+ droppedAbortHandle. Replaced with aSweepHandleenum (Supervised(TaskHandle)|Raw(JoinHandle<()>)) that callsabort(self)inDrop. Closes fix(core): fix broken sweeper abort path in SpeculationEngine::new (no-supervisor branch) #3645.DynExecutor::requires_confirmationmissing delegation).Test plan
cargo +nightly fmt --check— cleancargo clippy --workspace --all-targets --features "desktop,ide,server,chat,pdf,scheduler" -- -D warnings— cleancargo nextest run --workspace --features "desktop,ide,server,chat,pdf,scheduler" --lib --bins— 9138 passed, 0 failedrust-security-maintenance— both fixes approved