From f4ef75667e3f9fe92fb0d102c11e1dca20a4b3af Mon Sep 17 00:00:00 2001 From: wenshao Date: Sun, 3 May 2026 12:23:24 +0800 Subject: [PATCH] fix(core): close fileExists TOCTOU on WriteFile prior-read enforcement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WriteFile gated prior-read enforcement on `fileExists` from `isFilefileExists()`, but a file that sprang into existence between that check and the write would still be overwritten without enforcement — `fileExists === false` skipped the check entirely. Made the gate unconditional on `fileExists`. checkPriorRead's own `fs.stat` decides what to do: - ENOENT → ok:true, fall through to the new-file path as before - file exists right now (whether or not isFilefileExists saw it) → unknown / stale check runs, the race-created file is rejected. Applied to both getConfirmationDetails and execute. The path that actually creates new files is unchanged because checkPriorRead's ENOENT branch is the disappearance-race exit, which is the correct exit for "the file truly does not exist yet". Reported by glm-5.1 via /review on PR #3774. --- packages/core/src/tools/write-file.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index c77e13b83..f62333d1b 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -126,7 +126,15 @@ class WriteFileToolInvocation extends BaseToolInvocation< // computed from current bytes that the model has never received, // and the subsequent execute() would still reject the call — // confusing UX for any approve flow. - if (fileExists && !this.config.getFileReadCacheDisabled()) { + // + // Run unconditionally (not gated on `fileExists`): checkPriorRead's + // own stat decides whether the file actually exists right now. + // ENOENT means the path is genuinely absent → ok:true → fall + // through to the new-file diff; any other "stat says yes" outcome + // (including the file appearing between isFilefileExists() and + // here, a race window the pre-fix gating left wide open) means + // the model is about to clobber bytes it never read → reject. + if (!this.config.getFileReadCacheDisabled()) { const decision = await checkPriorRead( this.config.getFileReadCache(), this.params.file_path, @@ -222,7 +230,13 @@ class WriteFileToolInvocation extends BaseToolInvocation< // - we should not be holding bytes of a file the model never // legitimately saw, even transiently. // Mirrors the order in getConfirmationDetails() above. - if (fileExists && !this.config.getFileReadCacheDisabled()) { + // + // Run unconditionally (not gated on `fileExists`): checkPriorRead + // re-stats so a file that sprang into existence between + // isFilefileExists() and here — exactly the TOCTOU window pointed + // out in review — is now caught and rejected instead of being + // silently overwritten. + if (!this.config.getFileReadCacheDisabled()) { const decision = await checkPriorRead( this.config.getFileReadCache(), file_path,