fix: prevent file save before user approval when PREVENT_FOCUS_DISRUPTION is enabled #7684
Conversation
…TION is enabled - Move saveDirectly call to occur AFTER user approval in writeToFileTool - Reset diffViewProvider state when user rejects approval - Add tests to verify files are not saved before approval - Fixes issue where files were being written to disk before user clicked Save Fixes #7683
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| // Set up diffViewProvider properties needed for saveDirectly BEFORE asking for approval |
There was a problem hiding this comment.
The comment explains WHAT we're doing but not WHY it's critical for security. Could we make this more explicit? Something like:
| // Set up diffViewProvider properties needed for saveDirectly BEFORE asking for approval | |
| // Set up diffViewProvider properties needed for saveDirectly BEFORE asking for approval | |
| // This ensures we have the original content for comparison but critically prevents any | |
| // file writes from occurring until the user explicitly approves the operation |
|
|
||
| // Set up diffViewProvider properties needed for saveDirectly BEFORE asking for approval | ||
| // This ensures we have the original content for comparison but don't write anything yet | ||
| cline.diffViewProvider.editType = fileExists ? "modify" : "create" |
There was a problem hiding this comment.
I notice this block (lines 207-213) duplicates similar logic from the non-experiment path. Could we extract this into a helper function to reduce duplication? Something like prepareDiffViewState(fileExists, relPath)?
| cline.diffViewProvider.editType = fileExists ? "modify" : "create" | ||
| if (fileExists) { | ||
| const absolutePath = path.resolve(cline.cwd, relPath) | ||
| cline.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8") |
There was a problem hiding this comment.
Should we add error handling here in case fs.readFile fails? Currently if reading the original file fails, the error would bubble up and might not be handled gracefully.
|
|
||
| if (!didApprove) { | ||
| // Reset the diffViewProvider state since we're not proceeding | ||
| cline.diffViewProvider.editType = undefined |
There was a problem hiding this comment.
Good defensive programming! Properly resetting the state when the user rejects ensures no lingering state that could cause issues.
| expect(mockCline.diffViewProvider.originalContent).toBe(undefined) | ||
| }) | ||
|
|
||
| it("should save file AFTER user approval when experiment is enabled", async () => { |
There was a problem hiding this comment.
Great test coverage for the happy path! Consider adding a test case for when fs.readFile fails to ensure we handle that error scenario gracefully.
hannesrudolph
added
the
Issue/PR - Triage
|
I'm unfamiliar with this code but this PR doesn't seem to be materially changing the save order. It just moves the diff viewer to show earlier. I doubt this will fix my bug. |
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
|
Closing, see #7683 (comment) |
github-project-automation
bot
moved this from PR [Needs Prelim Review]
to Done
in Roo Code Roadmap
Summary
This PR fixes a critical trust protection issue where files were being saved to disk before the user clicked the "Save" button when the PREVENT_FOCUS_DISRUPTION experiment is enabled.
Problem
As reported in #7683, when Auto-Approve Write is disabled and a user asks Roo to write something to a file, the file was being created on disk immediately, even before the user clicked "Save". This bypassed the trust protection mechanism designed to give users control over file writes.
Solution
The fix reorders the operations in the
writeToFileToolto ensure that:saveDirectly) only happens AFTER user approvalChanges
saveDirectlycall to occur after user approvalTesting
Security Impact
This fix strengthens the security posture by properly enforcing trust boundaries and ensuring no file writes occur without explicit user consent.
Fixes #7683
Important
Fixes file save order in
writeToFileTool.tsto require user approval before saving whenPREVENT_FOCUS_DISRUPTIONis enabled.writeToFileTool.ts, movedsaveDirectlyto occur after user approval whenPREVENT_FOCUS_DISRUPTIONis enabled.diffViewProviderproperties are set up before approval request and reset if approval is denied.writeToFileTool.spec.tsto verify files are not saved before approval and are saved after approval.This description was created by
for cf6b384. You can customize this summary. It will automatically update as commits are pushed.