Skip to content

Commit 92bb178

Browse files
author
easonLiangWorldedtech
committed
fix(integrations): never adopt an autosaved match for an unauthorized edit
Series alignment with fws/u6-apply-patch-wiring. The autosave adoption branch accepted ANY GuardRejectedError when the buffer was clean. An "edit" save with no pre-open observation is rejected by the unobserved-edit guard - an authorization verdict, not a moved-token verdict - so if VS Code autosave had already written the buffer, adoption reported success AND recorded a partial observation for a file the model never read, which would then authorize a later targeted publish. Adoption is now limited to saves that were authorized before open(). Also removes the dead committed flag in safeWriteText (declared and set, never read) and corrects the comment describing a backup-restore guard that does not exist: the backup is a copy and is never restored.
1 parent e7138ab commit 92bb178

3 files changed

Lines changed: 40 additions & 8 deletions

File tree

‎src/integrations/editor/DiffViewProvider.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -561,6 +561,11 @@ export class DiffViewProvider {
561561
error instanceof GuardRejectedError &&
562562
saveTask &&
563563
encodedContent &&
564+
// An edit with no pre-open observation was rejected for AUTHORIZATION, not for a
565+
// moved token. Adopting the match would report success and record a partial
566+
// observation for a file the model never read, which would then authorize a
567+
// later edit publish - exactly what the unobserved-edit guard exists to prevent.
568+
!(writeKind === "edit" && this.preOpenObservation === null) &&
564569
// Only the autosave shape: a clean buffer means its content is what autosave
565570
// already put on disk. A dirty buffer means the disk content came from
566571
// someone else, so the discard cleanup below is still the right outcome.

‎src/integrations/editor/__tests__/DiffViewProvider.spec.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2328,6 +2328,39 @@ describe("DiffViewProvider", () => {
23282328
expect(diffViewProvider["closeAllDiffViews"]).toHaveBeenCalled()
23292329
})
23302330

2331+
it("does not adopt an autosaved match for an edit that was never authorized", async () => {
2332+
// Same autosave shape, different verdict: with no observation from before open()
2333+
// the guard rejects for AUTHORIZATION. Adopting the byte match would report a
2334+
// modified result and record a partial observation for a file the model never
2335+
// read, which would then authorize a later targeted publish.
2336+
const cleanEditor = {
2337+
document: {
2338+
uri: { fsPath: `${mockCwd}/test.ts`, scheme: "file" },
2339+
getText: vi.fn().mockReturnValue("new content"),
2340+
lineCount: 0,
2341+
encoding: "utf8",
2342+
isDirty: false,
2343+
save: vi.fn().mockResolvedValue(undefined),
2344+
},
2345+
selection: { active: { line: 0, character: 0 }, anchor: { line: 0, character: 0 } },
2346+
edit: vi.fn().mockResolvedValue(true),
2347+
revealRange: vi.fn(),
2348+
} as unknown as vscode.TextEditor
2349+
diffViewProvider["activeDiffEditor"] = cleanEditor
2350+
diffViewProvider.editType = "modify"
2351+
diffViewProvider["preOpenObservation"] = null
2352+
mockTask.observationRegistry.clear()
2353+
vi.mocked(computeVersionToken).mockResolvedValue("moved")
2354+
vi.mocked(fs.stat).mockResolvedValue(previewStats)
2355+
vi.mocked(fs.readFile).mockResolvedValue("new content")
2356+
2357+
await expect(diffViewProvider.saveChanges(false, 0, "edit")).rejects.toThrow(/File not read yet/)
2358+
2359+
// No adoption read, and no observation granted.
2360+
expect(fs.readFile).not.toHaveBeenCalled()
2361+
expect(mockTask.observationRegistry.get(`${mockCwd}/test.ts`)).toBeUndefined()
2362+
})
2363+
23312364
it("still rejects when the disk content does not match what the save intended", async () => {
23322365
// Same autosave shape, different bytes: the guard verdict stands.
23332366
const cleanEditor = {

‎src/services/file-safety/safeWriteText.ts‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -300,10 +300,6 @@ export async function safeWriteText(
300300

301301
let backupPath: string | null = null
302302
let releaseBackupOnSuccess = false
303-
// Set once the commit rename has published the new content. After that point the
304-
// backup is no longer a safe restore source: rolling it back would overwrite
305-
// content the caller can already observe at the target path.
306-
let committed = false
307303
// Non-null only when the win32 step-2 block saved a successful DACL dump:
308304
// it gates the step-5 restore and is tracked for the cleanup unlinks.
309305
let daclDumpPath: string | null = null
@@ -482,7 +478,6 @@ export async function safeWriteText(
482478

483479
// -- Step 4: atomic rename temp -> target ---------------------
484480
await fs.rename(tempPath, targetPath)
485-
committed = true
486481

487482
// -- Step 4b (POSIX): fsync the parent directory so the directory entry
488483
// changed by the commit rename is durable, not just the file content.
@@ -548,9 +543,8 @@ export async function safeWriteText(
548543
await fs.rmdir(stagingDir).catch(() => {})
549544
}
550545
} catch (originalError: unknown) {
551-
// Only a pre-commit failure can restore the backup. Once the commit rename
552-
// published, a later failure (for example the post-commit directory fsync)
553-
// must not overwrite the published content with the old file.
546+
// The backup is never restored: it is a copy, and the target already holds
547+
// either the pre-write content (before the commit) or the published content.
554548
if (backupPath && releaseBackupOnSuccess) {
555549
// Nothing to restore: the backup is a copy, so the target still holds whatever
556550
// the commit left there - before the commit that is the pre-write content, and

0 commit comments

Comments
 (0)