security(update): stage binary replacement at an unpredictable, exclusive path - #751
security(update): stage binary replacement at an unpredictable, exclusive path#751PierrunoYT wants to merge 13 commits into
Conversation
…sive path Fixes Gitlawb#742. installBinary staged downloaded release bytes at the fixed <target>.new path and opened it with O_CREATE|O_WRONLY|O_TRUNC. In an installation directory writable by a lower-privileged process, that process could pre-create <target>.new as a hard link or reparse point to another file the (possibly elevated) updater can write; the truncating open then overwrote that unintended file with verified Zero executable bytes. stagingFilePath now generates a cryptographically random suffix so the path can't be targeted in advance, and platform-specific createStagingFile opens it exclusively without following a pre-existing link: O_CREATE|O_EXCL on POSIX (which POSIX guarantees fails on a pre-existing path, symlink or not, without resolving it), and CreateFile with CREATE_NEW|FILE_FLAG_OPEN_REPARSE_POINT plus a post-open GetFileInformationByHandle check on Windows (not a reparse point, not a directory, single hard link). replace_windows.go's rename-swap sequence is unchanged: the dangerous operation was always the truncating open of the staged path, not the .old rename/removal that follows. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the standalone updater’s staging step to prevent an arbitrary-file-overwrite primitive (Issue #742) by removing the predictable <target>.new staging path and ensuring staging files are created exclusively without link/reparse-point traversal.
Changes:
- Generate an unpredictable staging filename (crypto-random suffix) instead of using a fixed
.newname. - Introduce platform-specific
createStagingFileimplementations to guarantee exclusive creation and avoid link/reparse-point following (with defense-in-depth verification on Windows). - Add regression tests covering pre-created hard links/symlinks and a concurrent creation race, and update existing tests to use a deterministic staging suffix hook.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/update/apply.go | Randomizes staging filename and switches staging writes to createStagingFile. |
| internal/update/apply_test.go | Pins staging suffix in tests to deterministically occupy the computed staging path. |
| internal/update/stage_other.go | Adds POSIX `O_CREAT |
| internal/update/stage_other_test.go | Adds non-Windows regression tests for hard link/symlink pre-creation and a concurrency race. |
| internal/update/stage_windows.go | Adds Windows `CreateFile(CREATE_NEW |
| internal/update/stage_windows_test.go | Adds Windows regression tests for hard link/symlink pre-creation and a concurrency race. |
| internal/update/stage_test_helpers_test.go | Adds test helper to stub the random staging suffix deterministically. |
Comments suppressed due to low confidence (1)
internal/update/apply.go:233
- In installBinary, the staged file is only scheduled for removal after copyFile succeeds. If copyFile creates the staging file and then fails (e.g., disk full / short write), the partially-written random staging file will be left behind in the install directory. Consider deferring the cleanup immediately after stagingFilePath succeeds so failures during copyFile are also cleaned up.
if err := copyFile(sourcePath, stagedPath); err != nil {
return fmt.Errorf("stage %s: %w", filepath.Base(targetPath), err)
}
defer func() {
_ = os.Remove(stagedPath)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WalkthroughThe updater replaces predictable pathname-based staging with exclusive, handle-bound staging objects. POSIX and Windows promotion paths verify object identity, support cleanup and recovery, classify possible tampering, and add coverage for link, race, substitution, and failure scenarios. ChangesSecure updater staging
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant installBinary
participant stageBinary
participant PlatformStaging
participant TargetBinary
participant Recovery
installBinary->>stageBinary: Create and populate staged binary
stageBinary->>PlatformStaging: Create verified staging object
stageBinary->>PlatformStaging: Copy and sync verified bytes
PlatformStaging->>TargetBinary: Promote using handle-bound operation
PlatformStaging->>Recovery: Restore original after failed promotion
Recovery->>TargetBinary: Preserve or restore recovery copy
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/update/apply.go (1)
225-234: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the
deferblock beforecopyFileto prevent leaking temporary files.If
copyFilefails (e.g., due to a full disk or an interrupted read),installBinaryreturns early and the partially written staging file is never removed, causing a permanent resource leak on every failed update.Moving the
deferblock immediately afterstagedPathgeneration guarantees cleanup across all error paths. This is entirely safe: ifreplaceBinarysuccessfully renames the file later,os.Removewill return a silentos.ErrNotExistthat safely gets ignored by the blank identifier.🛠 Proposed fix
stagedPath, err := stagingFilePath(targetPath) if err != nil { return fmt.Errorf("stage %s: %w", filepath.Base(targetPath), err) } + defer func() { + _ = os.Remove(stagedPath) + }() if err := copyFile(sourcePath, stagedPath); err != nil { return fmt.Errorf("stage %s: %w", filepath.Base(targetPath), err) } - defer func() { - _ = os.Remove(stagedPath) - }() if err := replaceBinary(targetPath, stagedPath); err != nil { return fmt.Errorf("install %s: %w", filepath.Base(targetPath), err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/apply.go` around lines 225 - 234, Move the cleanup defer in installBinary to immediately after successful stagingFilePath generation and before copyFile, so partially created staging files are removed when copying fails. Keep the existing os.Remove callback and ignored error behavior unchanged.
🧹 Nitpick comments (1)
internal/update/stage_other_test.go (1)
17-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated test files into a single cross-platform
stage_test.go. Both test files contain virtually identical test suites. As per coding guidelines, prefer one cross-platform function with small conditional checks over duplicated helpers when behavior can remain unified.
internal/update/stage_other_test.go#L17-L129: merge this test suite into a new unifiedstage_test.go.internal/update/stage_windows_test.go#L17-L130: merge this identical logic into the unified file, retaining the single conditionalif runtime.GOOS == "windows"for thet.Skipf("symlink unavailable...")skip logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/stage_other_test.go` around lines 17 - 129, Merge the duplicated test suites from internal/update/stage_other_test.go lines 17-129 and internal/update/stage_windows_test.go lines 17-130 into a single cross-platform internal/update/stage_test.go. Preserve the existing tests for createStagingFile, including hard-link, symlink, fresh-path, and concurrent-race coverage; retain one runtime.GOOS == "windows" conditional for the symlink-unavailable t.Skipf behavior, and remove the duplicated platform-specific test files.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/update/apply.go`:
- Around line 225-234: Move the cleanup defer in installBinary to immediately
after successful stagingFilePath generation and before copyFile, so partially
created staging files are removed when copying fails. Keep the existing
os.Remove callback and ignored error behavior unchanged.
---
Nitpick comments:
In `@internal/update/stage_other_test.go`:
- Around line 17-129: Merge the duplicated test suites from
internal/update/stage_other_test.go lines 17-129 and
internal/update/stage_windows_test.go lines 17-130 into a single cross-platform
internal/update/stage_test.go. Preserve the existing tests for
createStagingFile, including hard-link, symlink, fresh-path, and concurrent-race
coverage; retain one runtime.GOOS == "windows" conditional for the
symlink-unavailable t.Skipf behavior, and remove the duplicated
platform-specific test files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 754ad950-6559-4c37-bbdf-f873fe80c35e
📒 Files selected for processing (7)
internal/update/apply.gointernal/update/apply_test.gointernal/update/stage_other.gointernal/update/stage_other_test.gointernal/update/stage_test_helpers_test.gointernal/update/stage_windows.gointernal/update/stage_windows_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approve. I went through this on the security question and could not find a bypass: the verified binary reaches disk only through an exclusively-created, no-follow handle at an unpredictable path, then gets renamed into place.
Checked on head f62c4fc:
- POSIX (stage_other.go): createStagingFile is O_CREATE|O_EXCL|O_WRONLY, so it fails on any pre-existing name including a dangling symlink, without following it. The suffix is 16 bytes from crypto/rand, not math/rand or time-derived.
- Windows (stage_windows.go): CreateFile with CREATE_NEW|FILE_FLAG_OPEN_REPARSE_POINT operates on the reparse point itself and fails on a planted symlink or junction, and verifyFreshRegularFile then rejects reparse point / directory / NumberOfLinks>1 via GetFileInformationByHandle before any bytes are written. Belt and suspenders.
- Ordering is right: the checksum is verified before the binary is moved into the live target, and copyFile writes through the exact handle from createStagingFile with no reopen-by-path in between. No residual predictable
.newname remains. - Built and ran internal/update locally: gofmt, vet and build clean, tests pass including the hardlink/symlink refusal and checksum-mismatch cases. CI green.
One non-blocking nit worth a quick follow-up: installBinary registers defer os.Remove(stagedPath) after the copyFile error check (apply.go ~229-234), and copyFile only closes dest on an io.Copy failure, never removes it. So a mid-write copy failure (short write, full disk) leaks the partial staging file in the install directory. Not a security issue (128-bit random O_EXCL name, contents are a partial copy of the already-public binary), but easy to close by moving the removal defer into copyFile right after createStagingFile succeeds, plus a test for the mid-write path. Both bots flagged the same thing.
LGTM.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
APPROVE — the core hardening is correct and well-tested; the only residue is orphaned staging files, which is a pre-existing cleanup gap the randomization mildly amplifies (minor), plus a test-coverage nit.
The substantive fix for #742 is solid: createStagingFile opens with O_CREATE|O_EXCL|O_WRONLY (stage_other.go), so a pre-created hard link or symlink at the staging path fails EEXIST instead of being truncated through, and the Windows path adds CREATE_NEW|FILE_FLAG_OPEN_REPARSE_POINT plus the verifyFreshRegularFile link-count/reparse checks. I reproduced the base arbitrary-file-truncate primitive (fixed <target>.new + O_TRUNC opened through a pre-created hard link) and confirmed the new code refuses it. The regression tests in stage_other_test.go genuinely exercise both the hard-link and symlink variants and assert the victim is untouched. Good, well-commented work.
[Minor] Orphaned <binary>.<hex>.new staging files are never reclaimed and now accumulate — PR-introduced amplification of a pre-existing gap
internal/update/apply.go:229-234
Two paths leave a staged file behind, and no code ever removes .new files (CleanupStaleBinary only handles <binary>.old, and on non-Windows it is a no-op — replace_other.go:19 / replace_windows.go:56):
- Copy failure (pre-existing ordering, PR-introduced amplification): the
defer os.Remove(stagedPath)is registered at line 232, aftercopyFileat line 229. IfcopyFilefails (ENOSPC on the install volume, unreadable source),installBinaryreturns at line 230 before the defer is ever registered, so the partial file survives. On base this ordering existed too, but the fixed<target>.newname meant the next attempt truncated and reused the same orphan (self-healing). With the PR's random suffix, each retry on a persistently full disk writes a fresh<binary>.<hex>.new, so partial-binary-sized files pile up in the install directory. - Hard crash mid-window (PR-introduced): if the process is SIGKILLed / loses power after
copyFilesucceeds (line 229) but beforereplaceBinaryrenames (line 235), the deferred remove never runs. On base the single fixed orphan was reused by the next upgrade; now each crashed upgrade leaves a uniquely-named orphan that nothing collects.
Neither is a security or correctness bug — the swap still works and disk pressure is the only cost — hence Minor. Two independent fixes close it: (a) register the staged-file cleanup immediately after stagingFilePath succeeds (or have copyFile/createStagingFile remove its own dest on a non-nil return) so the copy-failure exit path is covered; and (b) on startup, best-effort glob and remove stale <binary>.*.new files in the target directory, analogous to CleanupStaleBinary's .old handling, to sweep crash leftovers.
[Nit] The "unpredictable path" half of the fix has no mutation-sensitive test — PR-introduced
internal/update/apply.go:260
I reverted stagingFilePath to a fixed filepath.Base(targetPath) + ".new" (dropping the random suffix) while keeping O_EXCL, and go test -count=1 ./internal/update/ still passed. The new tests call createStagingFile directly with hand-built paths, and TestApplyStandaloneUpdateWarnsWhenHelperRefreshFails computes the occupied path via stagingFilePath itself, so both pass regardless of whether the suffix is random. The randomness (the title's stated defense-in-depth against pre-creation/guessing) can therefore be removed with zero test signal. Not a security hole — removing O_EXCL instead fails 3 tests, confirming that check is the real guard — but a small unit test asserting stagingFilePath(target) places the file in filepath.Dir(target), contains the base name, and returns two different paths on successive calls with the real randomStagingSuffix would lock in the property.
Tests: gofmt, go vet (incl. GOOS=windows), and go test -race -count=1 ./internal/update/... all pass on the PR head; no PR-attributable failures, no environmental interference in this package.
Merge is kevin's call per the program gate.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep the staged file bound to its verified handle through replacement
internal/update/apply.go:229-235
copyFilecloses the exclusively-created staging handle beforereplaceBinaryconsumesstagedPathby name. Under #742's documented model—where the lower-privileged principal can create or replace sibling entries—it can observe the new randomized filename, replace that entry after the close, and have the updater promote its replacement instead of the verified bytes. On POSIX,replaceBinaryfirst follows the substituted path inos.Chmodand then renames it into the executable path; Windows likewise performs the final rename by pathname after the handle has been closed. Randomizing the name and usingO_EXCLonly prevent pre-creation, not this live handoff race, so the updater can still install attacker-controlled code. Retain and validate object identity through the final swap (or use a handle-bound replacement primitive), and add an end-to-end substitution-race regression test. -
[P2] Clean up a staged file when copying it fails
internal/update/apply.go:229-234
The removal defer is registered only aftercopyFilereturns successfully. Ifio.CopyorClosefails aftercreateStagingFilehas created the destination (for example, on ENOSPC), that partial file is left behind. This change makes each retry use a fresh randomized*.newname, whereas the prior fixed name was reused, and startup cleanup only removes.oldfiles. Repeated failed upgrades can therefore accumulate release-sized artifacts until the installation volume is exhausted. Register cleanup before copying (and clean crash leftovers) so an unsuccessful update does not consume space permanently.
…pathname Addresses the two open findings on Gitlawb#751. [P1] The staged file is no longer promoted by pathname. Randomizing the name and creating it exclusively stops PRE-creation, but under Gitlawb#742's model — a lower-privileged principal that can write in the installation directory — the name can be observed after the fact and the entry replaced between the write and the swap, so the updater would install the substituted file. installBinary now keeps the creating handle open and promotes through it: - Windows renames through the handle itself (SetFileInformationByHandle/FileRenameInfo, hence the added DELETE access), so there is no second pathname lookup to win. replaceBinary is gone; promote owns the aside-rename of the running binary and its restore-on-failure retry. - POSIX has no rename-by-descriptor, so staging moves into a private directory created next to the target by os.MkdirTemp (mode 0700, random name, created exclusively). An attacker who can write in the installation directory cannot create, replace, or list entries inside it. promote additionally sets the executable bit through the handle (os.Chmod would re-resolve the path) and fails closed if the entry ever stops naming the object it wrote. [P2] Staging cleanup now covers every failure path: the removal defer is registered before the copy, and stageBinary discards the object if the copy fails, so a mid-write error (ENOSPC, unreadable source) no longer leaks a release-sized file that the next attempt will not reuse. CleanupStaleBinary also sweeps leftovers from a hard crash — staging files on Windows, staging directories on POSIX, where it used to be a no-op — skipping anything younger than an hour so a concurrent update is never disturbed. Tests: substitution-race regression per platform (Windows asserts the verified bytes are installed despite a replaced staging entry; POSIX asserts the private 0700 directory and that promote refuses a substituted entry), success controls, cleanup-on-failure, and the crash-leftover sweep. The forced-staging-failure test for helper refresh now uses a stageBinary seam, because the staging location can no longer be occupied from outside — which is the point of the fix.
fa02593
|
Pushed [P1] Keep the staged file bound to its verified handle through replacement — this was a real hole and the fix is structural:
[P2] Clean up a staged file when copying it fails (also raised by @Vasanthdev2004, @gnanam1990, CodeRabbit and Copilot) — the removal defer is registered before the copy, and Tests
Validation — honest about the platform limits of this host (Windows, Go 1.26.5):
@jatmn @Vasanthdev2004 @gnanam1990 — this is a bigger change than the previous round (the promotion primitive itself changed per platform), so a fresh look at @coderabbitai review |
|
✅ Action performedReview finished.
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Fix the Windows handle-promotion path before merging
internal/update/stage_windows.go:106-118
The required Windows job is red on the current head:TestPromoteInstallsTheStagedObjectNotTheStagedPathgets a nil result fromstaged.promote, but after closing the handlezero.exedoes not exist. At that point the old executable has already been moved to.oldandpromotedsuppresses deferred removal, so an update can report success while stranding the user without the executable. Correct theFILE_RENAME_INFOconstruction/handle rename behavior and keep an end-to-end Windows assertion that the verified binary is reachable attargetPath. -
[P1] Keep the POSIX staging directory bound through the final rename
internal/update/stage_other.go:65-69
A0700directory protects its contents, but not its directory entry: under the issue's writable-install-directory threat model, an attacker can rename the.zero-stage-*directory from its writable parent afterverifyStagedIdentityreturns, recreate that directory with an attacker-controlled file at the same basename, and win the gap beforeos.Rename(staged.path, targetPath). The final path-based rename then installs attacker bytes. The current test only substitutes the child entry before the check, so it misses the ancestor replacement race; use a promotion primitive that remains safe when the parent namespace can change and cover this race end to end. -
[P1] Do not leave a raced target executable in place on Windows
internal/update/stage_windows.go:101-114
After the updater renames the running binary to.old, a writer of the installation directory can create a malicioustargetPath.renameFileByHandledeliberately usesReplaceIfExists=false, so promotion fails; the restore also fails because that attacker file occupiestargetPath. The function returns an error but leaves the attacker-controlled file at the executable path for the next launch, with the original only at.old. Restore/replace must safely handle this namespace race rather than treating it as a harmless failed update. -
[P2] Restrict stale cleanup to artifacts the updater can prove it owns
internal/update/stage_other.go:114-122
internal/update/replace_windows.go:55-64
Cleanup runs on everyApply, including the no-update path, but recognizes arbitrary old sibling entries by loose names: every.zero-stage-*directory is recursively removed on POSIX and every<binary>.*.newfile is removed on Windows. Those are broader than the generated artifact formats and can match legitimate user data such as.zero-stage-backuporzero.exe.release-notes.new. The pathname is also re-resolved after the age check, allowing a writable-parent actor to swap a checked directory beforeRemoveAll. Validate exact owned artifacts and avoid recursive deletion of untrusted pathnames before deleting anything. -
[P3] Do not age out an update that is still copying
internal/update/stage_other.go:118-122
The POSIX cleanup uses the staging directory's mtime as its liveness signal, butcopyFromonly writes the child file and never refreshes that timestamp. If a copy is slow or stalled for over an hour, a secondzero upgraderemoves the live staging directory; the first updater then fails identity verification or promotion even though it is still active. Track live staging independently, refresh a liveness marker, or avoid age-based removal of active directories.
Address jatmn's review findings on PR Gitlawb#751: - Windows promote() now verifies targetPath is actually reachable after a reported-successful handle rename before trusting it. Some Windows versions have been observed accepting SetFileInformationByHandle against a handle whose directory entry was substituted out from under it without the object actually moving, which let promote report success while targetPath was left missing entirely. renameFileByHandle is now a package var so this is covered by a deterministic regression test rather than relying on reproducing the exact trigger condition. - When a promotion failure's restore-to-.old also fails (a writable-parent attacker occupying targetPath with a lock MOVEFILE_REPLACE_EXISTING can't get past), that combination is now wrapped in ErrTargetPossiblyTampered instead of reading like an ordinary failed update, with a best-effort MOVEFILE_DELAY_UNTIL_REBOOT fallback so an admin-context updater can still recover the original at next boot. - POSIX promote() now binds its final rename to a directory descriptor opened when the staging directory is created (unix.Renameat), not a pathname. The 0700 staging directory protects its contents from a writable-parent principal, but not its own directory entry — that principal could rename the staging directory aside and recreate a look-alike with an attacker file at the same basename in the gap between the identity check and the rename, which a plain os.Rename would silently follow. - Stale-cleanup on both platforms now matches the exact generated artifact shape (POSIX: prefix + os.MkdirTemp's all-digit suffix; Windows: prefix + 32 lowercase hex chars + ".new") instead of a loose prefix/suffix, so a user's own similarly-named file or directory is never swept up. Both also re-check identity immediately before deleting to shrink the window a writable-parent principal could swap the checked path in. - copyFrom now copies in chunks and refreshes the POSIX staging directory's mtime between them, since writing into an already-created file never touches the parent directory's own mtime — a large or slow copy could previously look abandoned to a concurrent update's cleanup sweep while still in progress. Verified on real Windows and Linux (via WSL), plus cross-compiled build/vet checks for darwin/linux-arm64. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (5)
internal/update/replace_windows.go (1)
98-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCleanup-sweep logic duplicates the POSIX implementation's structure.
Same scan→age-filter→identity-recheck→delete skeleton as
removeStaleStagingLeftoversininternal/update/stage_other.go, only the name predicate (isGeneratedStagingFileNamevs.isGeneratedStagingDirName) and the deletion call (os.Removevs.os.RemoveAll) differ. See the consolidated comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/replace_windows.go` around lines 98 - 129, Consolidate the duplicated cleanup-sweep logic shared by removeStaleStagingLeftovers and the POSIX implementation in stage_other.go into a reusable helper. Parameterize the name predicate and deletion operation so file cleanup uses isGeneratedStagingFileName with os.Remove, while directory cleanup uses isGeneratedStagingDirName with os.RemoveAll; preserve the existing age and identity-recheck behavior.Source: Coding guidelines
internal/update/stage_other.go (1)
152-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCleanup-sweep logic is nearly identical to the Windows counterpart.
This scan→age-filter→identity-recheck→delete loop mirrors
removeStaleStagingLeftoversininternal/update/replace_windows.goalmost line for line, differing only in the name-matching predicate and whether the target is a directory (RemoveAll) or a file (Remove). See the consolidated comment for a suggested shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/stage_other.go` around lines 152 - 184, Consolidate the duplicated scan, age-filter, identity-recheck, and deletion logic shared by removeStaleStagingLeftovers and its Windows counterpart into a reusable helper. Parameterize the helper for the existing name predicate and directory/file deletion behavior, then update both callers while preserving their current cleanup semantics.Source: Coding guidelines
internal/update/stage_promote_windows_test.go (1)
172-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
assertNoStagingLeftovershelper vs. the POSIX test file.This Windows-only helper (matching
.newsuffix) duplicates the intent ofassertNoStagingLeftoversinstage_promote_other_test.go(which also matches the generated staging-directory prefix). See consolidated comment for the proposed unification. As per coding guidelines, "Prefer one cross-platform function with small conditional checks over duplicated platform-specific helpers when behavior can remain unified."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/stage_promote_windows_test.go` around lines 172 - 184, Unify the Windows-only assertNoStagingLeftovers helper with the cross-platform helper in stage_promote_other_test.go, removing the duplicate definition from the Windows test file. Preserve checks for both the generated staging-directory prefix and the Windows “.new” suffix using small platform-appropriate conditions within the shared helper.Source: Coding guidelines
internal/update/stage_promote_other_test.go (2)
134-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only checks the end-state, not "during copy" behavior it claims to regress-test.
The test forces small chunks and asserts the staging directory's mtime is fresh after
copyFromreturns. An implementation that only refreshes the mtime once at the very end of the copy (rather than periodically "as it goes," per the doc comment's stated intent) would pass this assertion too — yet that implementation would not actually fix the concurrent-sweep-sees-stale-mtime-mid-copy race this test is meant to guard against. Consider asserting freshness partway through the copy (e.g., wrap the source reader to pause/check after the first chunk) to actually exercise the "during copy" guarantee.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/stage_promote_other_test.go` around lines 134 - 176, Strengthen TestCopyFromRefreshesStagingLivenessDuringCopy to verify the staging directory mtime during copy, not only after staged.copyFrom returns. Wrap or control the source read so the test pauses after the first chunk, checks that staged.dir has a mtime newer than stale, then allows copying to finish; retain the final completion assertion and cleanup behavior.
282-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
assertNoStagingLeftovershelper vs. the Windows test file.This helper (matching generated staging prefix or
.newsuffix) duplicatesassertNoStagingLeftoversinstage_promote_windows_test.go(which matches.newsuffix only). Since checking both patterns is harmless on either platform (the POSIX-only prefix pattern will simply never match on Windows and vice versa), this looks like a good candidate to unify into a single helper in the sharedstage_test_helpers_test.gorather than maintaining two near-identical, platform-forked copies that can drift. See consolidated comment for details. As per coding guidelines, "Prefer one cross-platform function with small conditional checks over duplicated platform-specific helpers when behavior can remain unified."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/stage_promote_other_test.go` around lines 282 - 294, Move the shared assertNoStagingLeftovers helper into stage_test_helpers_test.go and remove the duplicate definitions from the POSIX and Windows test files. Preserve both checks in the unified helper: stagingDirPrefix matches and the .new suffix matches, so leftover detection remains consistent across platforms.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/update/replace_windows.go`:
- Around line 98-129: Consolidate the duplicated cleanup-sweep logic shared by
removeStaleStagingLeftovers and the POSIX implementation in stage_other.go into
a reusable helper. Parameterize the name predicate and deletion operation so
file cleanup uses isGeneratedStagingFileName with os.Remove, while directory
cleanup uses isGeneratedStagingDirName with os.RemoveAll; preserve the existing
age and identity-recheck behavior.
In `@internal/update/stage_other.go`:
- Around line 152-184: Consolidate the duplicated scan, age-filter,
identity-recheck, and deletion logic shared by removeStaleStagingLeftovers and
its Windows counterpart into a reusable helper. Parameterize the helper for the
existing name predicate and directory/file deletion behavior, then update both
callers while preserving their current cleanup semantics.
In `@internal/update/stage_promote_other_test.go`:
- Around line 134-176: Strengthen TestCopyFromRefreshesStagingLivenessDuringCopy
to verify the staging directory mtime during copy, not only after
staged.copyFrom returns. Wrap or control the source read so the test pauses
after the first chunk, checks that staged.dir has a mtime newer than stale, then
allows copying to finish; retain the final completion assertion and cleanup
behavior.
- Around line 282-294: Move the shared assertNoStagingLeftovers helper into
stage_test_helpers_test.go and remove the duplicate definitions from the POSIX
and Windows test files. Preserve both checks in the unified helper:
stagingDirPrefix matches and the .new suffix matches, so leftover detection
remains consistent across platforms.
In `@internal/update/stage_promote_windows_test.go`:
- Around line 172-184: Unify the Windows-only assertNoStagingLeftovers helper
with the cross-platform helper in stage_promote_other_test.go, removing the
duplicate definition from the Windows test file. Preserve checks for both the
generated staging-directory prefix and the Windows “.new” suffix using small
platform-appropriate conditions within the shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c330dc9-d61a-49c6-9d09-7c211cf38cc5
📒 Files selected for processing (7)
internal/update/apply.gointernal/update/replace_windows.gointernal/update/replace_windows_test.gointernal/update/stage_other.gointernal/update/stage_promote_other_test.gointernal/update/stage_promote_windows_test.gointernal/update/stage_windows.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/update/apply.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore gofmt cleanliness before merging
internal/update/replace_windows_test.go:90
The required Ubuntu smoke job currently fails its formatting step on this file, so vet, tests, build, and smoke are all skipped. Run gofmt and commit the resulting alignment change. -
[P1] Bind the POSIX staging directory before an attacker can replace it
internal/update/stage_other.go:41
MkdirTempcreates the private directory, but the code subsequently opens it again by pathname. A principal that can write the installation directory can rename that entry and recreate it beforeos.Open, makingdirHandlerefer to an attacker-writable directory. The attacker can then replace the child afterverifyStagedIdentityand beforeRenameat, causing the verified updater to install attacker bytes. Acquire/validate the created directory without this pathname handoff and cover that interleaving. -
[P1] Do not recursively remove an attacker-substituted staging directory
internal/update/apply.go:337
The POSIX promotion deliberately tolerates the staging directory being renamed aside, but deferreddiscardlater callsRemoveAll(staged.dir)through the old pathname. An attacker can replace that pathname with a chosen directory; the elevated updater then recursively deletes the replacement. The stale-cleanup path instage_other.go:177-182has the same Lstat-to-RemoveAll race. Keep deletion bound to the original object or fail closed rather than recursively deleting a mutable pathname; the ancestor-replacement test should assert the impostor survives deferred cleanup. -
[P1] Verify that the promoted Windows target is the staged object
internal/update/stage_windows.go:149
This check accepts any non-directory attargetPath. In the anomalous success case it is meant to handle—handle rename reports success but does not link the staged file—an attacker with write access to the installation directory can create a regular malicious target beforeos.Stat. The check then passes and the update is reported successful with attacker bytes. Verify object identity, not only target existence. -
[P1] Do not schedule reboot recovery from a mutable source pathname
internal/update/replace_windows.go:66
After an immediate restore failure, this queues<target>.oldto replace the executable at reboot. Under the stated writable-directory threat model, an attacker can replace that source entry before reboot, turning the recovery action into a delayed installation of attacker-selected bytes. Surface the recovery failure or use a recovery source outside the attacker-controlled directory. -
[P2] Do not infer cleanup ownership from a public filename shape
internal/update/stage_other.go:163
Any old directory named.zero-stage-<1..10 digits>is treated as an updater artifact and recursively removed; no provenance distinguishes it from user data. Windows makes the same assumption for any exact<binary>.<32 hex>.newfile inreplace_windows.go:109. Exact pattern matching reduces accidental collisions but does not establish ownership, so preserve non-verifiable entries or introduce durable, unforgeable ownership metadata with an appropriate migration story.
|
Addressed the requested changes in
Validation completed:
Environment limitations: the repository-wide test run reaches an unrelated CLI completion test that requires an installed WSL distribution, and |
|
@Vasanthdev2004 @jatmn — CI is now fully green on PierrunoYT addressed the cross-platform breakage and the outstanding staging findings in
All previously open inline threads have finding-specific replies and are resolved. The branch also contains a normal merge of current |
|
Final security follow-up on current head
The PR is mergeable. PierrunoYT requests a fresh human review to supersede the pre-fix |
…-staging-path-overwrite
|
PierrunoYT merged the latest Fresh review confirms the still-valid findings are covered:
Local validation on Go 1.26.5:
Native Windows execution is not available in the Linux orb (Wine is unavailable); the new CI follow-up: All pushed-head checks are now green, including native |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Preserve
ErrTargetPossiblyTamperedin the error chain returned frompromote
internal/update/stage_windows.go:119-121
When Windows promotion fails andrestoreOriginalBinaryalso fails,restoreOriginalBinarywrapsErrTargetPossiblyTampered, butpromoteformats that value with%vinstead of%w. A caller that useserrors.Is(err, update.ErrTargetPossiblyTampered)on the error frominstallBinaryorApplytherefore cannot tell this writable-directory race apart from an ordinary failed update, even thoughreplace_windows.godocuments the sentinel for exactly that case andTestRestoreOriginalBinaryFlagsPossibleTamperingWhenRestoreFailsonly exercisesrestoreOriginalBinarydirectly. Change the failure return to wraprestoreErrwith%w(or return it when it is the authoritative failure), and add a promotion-path regression test that assertserrors.Isworks throughinstallBinary. -
[P3] Align
.oldcleanup with the preservation message after a failed promotion
internal/update/apply.go:62
internal/update/replace_windows.go:58-59
internal/update/stage_windows.go:119-121
This behavior predates the staging hardening:CleanupStaleBinaryalready removed<target>.oldon everyApply, and the aside-rename failure path already reported(original preserved at %s). The newErrTargetPossiblyTamperedframing makes that contradiction harder to ignore. After a failed promotion/restore, the nextApplycan still delete the only on-disk copy of the known-good binary whiletargetPathmay be missing or hold unverified bytes. Worth fixing in the same area, but it is follow-up hardening rather than a regression from the #742 staging changes. Restrict stale cleanup to genuinely obsolete.oldfiles, restore from.oldwhentargetPathis absent, or stop promising preservation in the error when cleanup will remove it on the next run.
Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
PierrunoYT addressed the updater review in
Validation: |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I do not see merge-blocking issues in the #742 staging hardening on head e517cf8.
The unpredictable exclusive staging path, handle-bound Windows promotion with post-rename identity verification, descriptor-bound POSIX promotion/cleanup, and the adversarial regression coverage address the reported arbitrary-file-overwrite primitive. Prior P1 review items on live handoff races, recursive cleanup of substituted paths, ErrTargetPossiblyTampered discoverability through installBinary, and preservation of unverifiable crash leftovers are resolved on the current head.
Non-blocking follow-up
-
[P3] Tighten
.oldcleanup semantics after a tampering failure
internal/update/replace_windows.go:60-64
internal/update/apply.go:62
AfterErrTargetPossiblyTampered,targetPathmay hold unverified bytes while.oldstill has the known-good copy, yet the nextApplydeletes.oldwheneverLstat(target)succeeds. This behavior predates the staging work — basemainalways removed.oldunconditionally — ande517cf8already improved the absent-target case. Worth a follow-up so cleanup does not erase the recovery copy while the error text still says the original was preserved at.old, but it is not a regression from this PR. -
[P3] Clean up the staging directory when post-create
Fstatatfails
internal/update/stage_other.go:84-87
createStagingDirectorycan leave an empty.zero-stage-*directory behind ifFstatatfails immediately afterMkdirat. Small error-path leak introduced by the new POSIX staging flow; best-effortUnlinkatwith the existing identity checks would close it.
Out of scope for this PR
- Immediate Windows restore still renames through the predictable
<target>.oldpathname, same as pre-refactorreplaceBinary. Reboot recovery from.oldwas correctly removed here for that reason; pathname-bound immediate restore is adjacent hardening, not a new defect in the staging changes. - Optional helper refresh failures remain warnings while the main binary failure aborts. That predates this PR; only the possible appearance of
ErrTargetPossiblyTamperedin a helper warning is new.
Two error-path follow-ups from the Gitlawb#742 staging review. When a promotion fails and restoring the original also fails, the error tells the operator their original is preserved at <binary>.old — and then the next Apply deleted it, because a present target read as proof the copy was stale. What occupies the target in exactly that case is the bytes the updater could not verify, so cleanup was erasing the one it could. A failed restore now marks the copy, cleanup honors the mark, and a successful promotion clears it. The POSIX staging flow could also leave an empty .zero-stage-* directory behind when the stat immediately after mkdirat failed. Nothing knows that name yet, so it would have stayed for good; it is now removed best-effort with rmdir semantics, which cannot touch anything but the empty directory just created. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for the review. Both non-blocking P3s are addressed in [P3] One adjacent thing I changed for the same reason: The marker is explicitly a note to the next run, not a security control — it sits in the same directory as the binary, so anyone who can write there can delete it, which just returns cleanup to today's behavior. That's documented on the function. [P3] Staging directory leak on post-create Tests. Two new Windows regressions, both verified to fail when the cleanup guard is removed:
The POSIX directory-leak fix has no test: forcing I left both "out of scope" items alone as you scoped them. Validation
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/update/replace_windows.go`:
- Around line 64-76: Update markOldBinaryPreserved to create the .old.keep
marker exclusively with the updater’s existing no-link protections, without
truncating or following pre-existing hard links, symlinks, or reparse points. If
marker creation or writing fails, conservatively preserve the .old binary rather
than allowing routine cleanup to remove it. Add regression coverage for
link/reparse attacks and marker-creation failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c5b2935c-e20e-46a1-b21b-d1ef748419bd
📒 Files selected for processing (5)
internal/update/replace_windows.gointernal/update/replace_windows_test.gointernal/update/stage_other.gointernal/update/stage_promote_windows_test.gointernal/update/stage_windows.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/update/stage_windows.go
- internal/update/stage_other.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
The #742 staging hardening on the prior head looks sound: unpredictable exclusive staging, handle-bound Windows promotion with post-rename identity checks, and descriptor-bound POSIX promotion address the reported primitive. The blockers below are in head d9523bab's recovery-marker follow-up, not in that core staging work.
Findings
-
[P1] A marked recovery
.oldis destroyed the first time promotion retries
internal/update/stage_windows.go:102-111
AfterErrTargetPossiblyTampered, disk state istargetPathholding unverified bytes,<target>.oldholding the last known-good copy, and<target>.old.keeppresent. On the nextinstallBinary→promote,oldBinaryPreservedskips the pre-renameos.Remove, butos.Rename(targetPath, oldPath)still runs. Go's Windowsos.Renamereplaces an existing destination file (MOVEFILE_REPLACE_EXISTING), so the known-good bytes at.oldare overwritten by whatever occupiestargetPathbefore the new staged binary is installed. That contradicts thed9523babgoal of keeping the recovery copy the failure message promised: cleanup no longer deletes.old, but retry promotion still destroys it. If promotion fails after that rename,restoreOriginalBinarycan only move the unverified copy back — the known-good recovery is already gone. Please keep the marked recovery copy intact across retries (for example, move it aside under a fresh name, or refuse promotion until the operator clears the tamper state) and add a Windows regression test that drives a secondpromotewith.keeppresent. -
[P1]
markOldBinaryPreservedreintroduces a predictable link-following truncate write
internal/update/replace_windows.go:68-76
The marker path<target>.old.keepis fixed and opened withos.OpenFile(..., O_CREATE|O_TRUNC)and noO_EXCL/FILE_FLAG_OPEN_REPARSE_POINT/ post-open identity check. Under the same writable-install-directory model as #742, a lower-privileged writer can pre-create that path as a hard link or reparse point; after a failed restore, the elevated updater truncates and writes through it. CodeRabbit's critical comment on headd9523babis still open. Please create the marker with the same exclusive, no-follow semantics used for staging files, and add regression coverage for pre-created hard links and symlinks on the.keeppath. -
[P2] Helper tampering is downgraded to a success-path warning
internal/update/apply.go:186-201
Main-binaryinstallBinaryfailures abortapplyStandaloneUpdate, but optional helper installs always becomewarningsand the function returns(warnings, nil). That helper-as-warning pattern predates this PR (TestApplyStandaloneUpdateWarnsWhenHelperRefreshFailsexpects it). What is new on head isErrTargetPossiblyTampered: when a helper hits the Windows tamper path,Applystill reportsApplied: trueandzero upgradeexits successfully while a sibling executable such aszero-windows-command-runner.exemay hold unverified bytes. Those helpers are resolved from the install directory and executed by the sandbox runner. Please propagate helperErrTargetPossiblyTamperedthe same way as the main binary (fail the apply, preserveerrors.IsthroughApply) and add coverage parallel toTestInstallBinaryPreservesPossibleTamperingError. -
[P3] A failed marker write can still cause the next run to delete the only recovery copy
internal/update/replace_windows.go:47-57,68-71,103-111
restoreOriginalBinaryalways tells the operator the original is preserved at<target>.old, butmarkOldBinaryPreservedreturns silently onOpenFilefailure. The nextApplycallsCleanupStaleBinaryfirst; with a presenttargetPathand no.keepmarker, it deletes.old. The code documents the marker as best-effort rather than a security control, so this is a narrow reliability gap (disk full, permissions), not the same class as #742. Still worth closing: treat marker creation as part of the recovery contract (surface failure if the marker cannot be written, or otherwise keep.olduntil a marker is observable) and add a regression for marker-write failure followed byCleanupStaleBinary/Apply.
…attempt The marker added in d9523ba kept cleanup from deleting <target>.old, but four paths still undermined the promise it was meant to make. Promotion destroyed the copy it was preserving. Skipping the pre-rename cleanup was not enough: os.Rename uses MOVEFILE_REPLACE_EXISTING on Windows, so renaming the running binary aside overwrote the last verified copy with the unverified bytes the earlier failure left at the target — and a promotion that then failed could only move those unverified bytes back. Promotion now refuses while the tamper state is unresolved. Only the operator can say whether the file at the target is theirs, so the error names both moves that end it: restore the copy, or delete the marker to accept what is installed. The marker itself was a predictable link-following truncate write. Its path is fixed, so a writer in the install directory could pre-create it as a hard link or reparse point and have the elevated updater write through it. It is now created with the same CREATE_NEW + FILE_FLAG_OPEN_REPARSE_POINT + fresh-regular- file check as a staging file, and never truncates; an existing name is treated as already-marked rather than an object to open. A marker that could not be written left the promise unqualified while the next run's cleanup deleted exactly the file it named. That failure now rides in the error, telling the operator to copy it somewhere safe now. Helper refreshes downgraded tampering to a warning. An ordinary helper failure still warns — a stale helper is better than a failed update — but a helper whose path may hold unverified content fails the apply, because helpers are resolved from the install directory and executed by the sandbox runner, and reporting Applied: true would hand the operator a success while a sibling executable is suspect. ErrTargetPossiblyTampered moved to apply.go so callers on every platform can test for it without build tags. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All four findings on [P1] A marked recovery copy is no longer destroyed by the next promotion. I had reasoned that the rename replacing Of your two options I took refusal rather than moving the copy aside. Moving it aside keeps automation running but leaves the operator with two opaque copies and no statement about which is which, and the state it is recovering from is one where only they can say whether the file at the target is theirs. So Worth flagging the tradeoff: [P1] Marker creation no longer follows a pre-created link. Now [P2] Helper tampering fails the apply. Ordinary helper-refresh failures stay warnings — [P3] A failed marker write is disclosed. Each new test was verified to fail against the previous implementation — the two marker tests against The two P3s from your earlier review ( Validation
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/update/apply_test.go (1)
253-317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate archive/server/result scaffolding across tests.
Lines 264-317 essentially repeat the archive-build, checksum,
httptestserver, andResultconstruction boilerplate already inTestApplyStandaloneUpdateReplacesBinary(lines 63-106), and reportedly again in the rewrittenTestApplyStandaloneUpdateWarnsWhenHelperRefreshFails. Worth extracting a small helper (e.g.newTestUpdateServer(t) (Result, func())) shared across these tests to cut the copy-paste and reduce drift risk when the archive layout changes.♻️ Sketch of a shared helper
func newTestUpdateServer(t *testing.T) (Result, func()) { t.Helper() archiveName := "zero-v0.2.0-linux-x64.tar.gz" archiveDir := t.TempDir() archivePath := filepath.Join(archiveDir, archiveName) writeTestTarGz(t, archivePath, map[string]string{ "zero": "new-binary", "zero.exe": "new-binary-exe", "zero-seccomp": "new-helper", "zero-windows-command-runner.exe": "new-helper-exe", }) checksum, err := release.SHA256File(archivePath) if err != nil { t.Fatalf("SHA256File: %v", err) } checksumText, err := release.FormatSHA256Checksum(checksum, archiveName) if err != nil { t.Fatalf("FormatSHA256Checksum: %v", err) } server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { case "/" + archiveName: http.ServeFile(w, r, archivePath) case "/" + archiveName + ".sha256": _, _ = w.Write([]byte(checksumText)) default: http.NotFound(w, r) } })) return Result{ LatestVersion: "0.2.0", ReleaseAsset: AssetCheck{ Platform: "linux", Arch: "x64", ArchiveName: archiveName, ArchiveURL: server.URL + "/" + archiveName, ChecksumName: archiveName + ".sha256", ChecksumURL: server.URL + "/" + archiveName + ".sha256", ArchiveFound: true, ChecksumFound: true, Verified: true, }, }, server.Close }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/apply_test.go` around lines 253 - 317, Extract the repeated archive creation, checksum generation, test HTTP server, and Result setup from TestApplyStandaloneUpdateReplacesBinary, TestApplyStandaloneUpdateWarnsWhenHelperRefreshFails, and TestApplyStandaloneUpdateFailsWhenHelperRefreshReportsTampering into a shared newTestUpdateServer helper. Update each test to use the helper and register its returned cleanup function, preserving the existing archive contents and Result values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/update/apply_test.go`:
- Around line 253-317: Extract the repeated archive creation, checksum
generation, test HTTP server, and Result setup from
TestApplyStandaloneUpdateReplacesBinary,
TestApplyStandaloneUpdateWarnsWhenHelperRefreshFails, and
TestApplyStandaloneUpdateFailsWhenHelperRefreshReportsTampering into a shared
newTestUpdateServer helper. Update each test to use the helper and register its
returned cleanup function, preserving the existing archive contents and Result
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8348e67c-7021-4605-a36d-cef76cbacf4e
📒 Files selected for processing (6)
internal/update/apply.gointernal/update/apply_test.gointernal/update/replace_windows.gointernal/update/replace_windows_test.gointernal/update/stage_promote_windows_test.gointernal/update/stage_windows.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/update/apply.go
- internal/update/stage_promote_windows_test.go
- internal/update/stage_windows.go
…shed Surfacing a failed marker write told the operator to hurry; it did not stop the next run from deleting the file they were being told to save. When no marker can be established, the copy is now moved to an unpredictable sibling name that routine cleanup never touches — CleanupStaleBinary only ever removes the exact "<target>.old" — and the error names where it went. Telling the operator to act now remains the fallback for when even that move fails. The marker check is conservative in the same direction: only a definite "not there" allows the copy to be deleted. An Lstat that fails for any other reason leaves the question open, and deleting is irreversible while keeping costs one stale file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Following up on the CodeRabbit marker comment, which was auto-marked as addressed by What
What it did not: "conservatively retain
The marker check now errs the same way:
New tests: Validation: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/update/replace_windows_test.go`:
- Around line 290-298: Update the recovery-copy assertions in the test around
CleanupStaleBinary to verify oldPath no longer exists before cleanup, using
os.IsNotExist (or equivalent errors.Is with os.ErrNotExist). Keep the existing
kept-file content assertion and perform this check immediately before calling
CleanupStaleBinary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a560402-2fdd-48d2-85bc-314c780fb135
📒 Files selected for processing (2)
internal/update/replace_windows.gointernal/update/replace_windows_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/update/replace_windows.go
| got, readErr := os.ReadFile(kept) | ||
| if readErr != nil { | ||
| t.Fatalf("recovery copy was not kept: %v", readErr) | ||
| } | ||
| if string(got) != "known-good" { | ||
| t.Fatalf("kept copy = %q, want the last verified binary", got) | ||
| } | ||
| // And routine cleanup cannot reach it: it only ever removes "<target>.old". | ||
| CleanupStaleBinary(targetPath) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the recovery copy was moved, not merely copied.
The test verifies the .recovery file exists, but not that oldPath no longer exists before cleanup. A regression that copies the binary and leaves <target>.old behind would still pass. Assert os.ErrNotExist for oldPath immediately before CleanupStaleBinary.
Proposed test addition
if string(got) != "known-good" {
t.Fatalf("kept copy = %q, want the last verified binary", got)
}
+ if _, err := os.Lstat(oldPath); !errors.Is(err, os.ErrNotExist) {
+ t.Fatalf("old recovery path still exists after move: %v", err)
+ }
// And routine cleanup cannot reach it: it only ever removes "<target>.old".
CleanupStaleBinary(targetPath)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| got, readErr := os.ReadFile(kept) | |
| if readErr != nil { | |
| t.Fatalf("recovery copy was not kept: %v", readErr) | |
| } | |
| if string(got) != "known-good" { | |
| t.Fatalf("kept copy = %q, want the last verified binary", got) | |
| } | |
| // And routine cleanup cannot reach it: it only ever removes "<target>.old". | |
| CleanupStaleBinary(targetPath) | |
| got, readErr := os.ReadFile(kept) | |
| if readErr != nil { | |
| t.Fatalf("recovery copy was not kept: %v", readErr) | |
| } | |
| if string(got) != "known-good" { | |
| t.Fatalf("kept copy = %q, want the last verified binary", got) | |
| } | |
| if _, err := os.Lstat(oldPath); !errors.Is(err, os.ErrNotExist) { | |
| t.Fatalf("old recovery path still exists after move: %v", err) | |
| } | |
| // And routine cleanup cannot reach it: it only ever removes "<target>.old". | |
| CleanupStaleBinary(targetPath) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/update/replace_windows_test.go` around lines 290 - 298, Update the
recovery-copy assertions in the test around CleanupStaleBinary to verify oldPath
no longer exists before cleanup, using os.IsNotExist (or equivalent errors.Is
with os.ErrNotExist). Keep the existing kept-file content assertion and perform
this check immediately before calling CleanupStaleBinary.
Source: Coding guidelines
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
The #742 staging hardening on head fd383fd3 looks sound: unpredictable exclusive staging, handle-bound Windows promotion with post-rename identity verification, descriptor-bound POSIX promotion/cleanup, and the adversarial regression coverage address the reported arbitrary-file-overwrite primitive. Your follow-up commits on d9523bab also resolve my earlier blockers around marked-recovery overwrite on retry, exclusive .keep creation, helper ErrTargetPossiblyTampered propagation, and relocating the recovery copy when the marker cannot be written.
The remaining blockers below are in the new Windows recovery/cleanup contract this PR added, not in the core staging swap.
Findings
-
[P1] Do not destroy the verified
.oldcopy when the operator follows the documented marker-clearing recovery path
internal/update/apply.go:72
internal/update/replace_windows.go:204-212
internal/update/stage_windows.go:114-124
After a tampering failure,promotetells the operator to delete<target>.old.keepand update again. On that nextApply,CleanupStaleBinary(executablePath)runs before any promotion attempt. With the marker gone,oldBinaryPreservedis false,targetPathis still present, and cleanup removes<target>.old— the last verified copy the refusal message said to use for recovery. Even if startup cleanup were reordered,promotestill unconditionallyos.Remove(oldPath)at line 122 and thenos.Rename(targetPath, oldPath)at line 123, which destroys the known-good copy and moves whatever unverified bytes occupytargetPathonto.oldbefore installing the new release. That contradicts the recovery options the refusal text documents.TestPromoteRefusesWhileRecoveryCopyIsMarkedonly checkstargetPathafter the operator clears the marker; it never asserts.oldstill holds the verified bytes. Please keep the verified recovery copy intact across both paths (defer or narrow cleanup, and do not remove or overwrite.olduntil promotion has a verified target to rename aside) and add a Windows regression that deletes.keep, callsApply, and asserts.oldstill holds the last verified binary through cleanup and the subsequentpromoteattempt. -
[P1] Do not remove an unmarked recovery
.oldwhen retrying promotion after an interrupted aside-rename
internal/update/stage_windows.go:122-124
If promotion renames the running binary aside and then crashes before the staged object lands attargetPath, disk state is:targetPathmissing,<target>.oldholding the last verified binary, no.keepmarker.CleanupStaleBinarycorrectly preserves.oldwhiletargetPathis absent, but the nextpromoteunconditionallyos.Remove(oldPath)beforeos.Rename(targetPath, oldPath). That deletes the only recovery copy and then fails becausetargetPathdoes not exist, leaving the install directory with neither executable. The pre-renameRemovepattern predates this PR, but this change made startup cleanup safer for the absent-target case without closing the same hole inpromoteitself. Please only remove.oldwhentargetPathis present and the aside-rename can succeed, or refuse promotion whiletargetPathis missing and.oldis the sole known-good copy. Add a Windows test that simulates crash-after-aside and a retry. -
[P2] Stop appending
(original preserved at <target>.old)after the recovery copy was relocated
internal/update/stage_windows.go:140-141
internal/update/replace_windows.go:68-72
WhenrestoreOriginalBinarycannot write the marker andkeepUnmarkedRecoveryCopysucceeds, it renames the verified copy away from<target>.oldand the inner error names the new*.recoverypath.promotestill suffixes(original preserved at %s)with the now-emptyoldPath, so the outerinstallBinary/Applyerror can point at a path that no longer holds the recovery bytes. Please omit that suffix whenrestoreErralready records relocation, or derive the suffix from the authoritative path inrestoreErr. Add a promotion-path test throughinstallBinarythat asserts the final error does not claim preservation atoldPathafter relocation. -
[P2] Protect the recovery marker from deletion under the writable-directory threat model
internal/update/replace_windows.go:188-212
internal/update/apply.go:72
Recovery now depends on a predictable<target>.old.keepmarker that did not exist on basemain. Under the same writable-install-directory model as #742, a principal who can write the install directory can delete that marker between runs. The nextApplythen treats the recovery copy as an ordinary leftover and removes<target>.oldwhiletargetPathmay still hold unverified bytes — the same class of loss the marker was added to prevent. Please make marker presence durable against deletion (for example, bind protection to inode identity rather than a deletable sibling name, or relocate/mark the recovery copy in a way cleanup cannot reach without proving ownership) and add regression coverage for marker deletion betweenApplycalls. -
[P2] Retry after helper tampering can skip helper refresh once the main binary is already updated
internal/update/apply.go:74-76
internal/update/apply.go:192-218
MaininstallBinaryruns before the helper loop. Failing closed on helperErrTargetPossiblyTamperedis the right security call, but it leaves a new operational gap: if a helper returns that error after the main binary was already promoted,Applyreports failure withApplied: falseeven though the main executable on disk is already the new release. A laterzero upgradeafter restarting into that new main binary exits at “already up to date” with a nil error becauseCheckuses the running process version and never re-entersapplyStandaloneUpdate, so the suspect helper is not refreshed and the prior tampering failure is not mentioned. Please either roll back the main install on helper tampering, refresh helpers even when no main update is pending, or surface a distinct recovery path that retries helper installation without requiring a newer release. Add coverage for main-updated + helper-tampered disk state followed by a secondApplyafter the version gate would be false.
Non-blocking follow-up
-
When both marker creation and recovery relocation fail, the next
Applystill deletes<target>.oldeven though the error already warns the operator to copy it immediately (replace_windows.go:74-77,apply.go:72). That behavior is explicit in the error text; tightening it would improve reliability but is not the same class as the recovery-contract blockers above. -
A planted
<target>.old.keepcan deny updates until manual cleanup (replace_windows.go:188-190,stage_windows.go:114-120). That is an intentional fail-safe in the current code; worth revisiting only if you want to distinguish operator-planted markers from updater-created recovery state.
Amp-Thread-ID: https://ampcode.com/threads/T-019faf74-99d4-75cf-ac7a-661c308240ef Co-authored-by: Amp <amp@ampcode.com>
|
Addressed the latest What changed
Validation
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/update/replace_windows.go (2)
135-143: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not relocate recovery state after leaving a
.keepmarker behind.A write or close failure returns
markErrbut leaves the freshly created marker in place.restoreOriginalBinarythen relocates.old, stranding<target>.old.keepwithout its recovery copy and causing future recovery-state checks to see inconsistent state. Treat successful marker creation/verification as preservation success, or remove the marker before returning an error that triggers relocation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/replace_windows.go` around lines 135 - 143, The recovery-marker failure path in the marker-writing logic must not leave a partially created `.keep` marker before `restoreOriginalBinary` relocates recovery state. Update the handling around `marker.WriteString` and `marker.Close` so successful marker creation remains a preservation success, while any write or close error removes the marker before returning the error that triggers relocation.
156-168: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftBind recovery relocation to the verified recovery object.
keepUnmarkedRecoveryCopychecksoldPathbyos.Lstat, opensoldPath, and renamesoldPathby pathname withos.Rename(oldPath, kept), so a writable-install-directory thread can replace either endpoint after the marker check and havepromotereturn an unverified recovery path. Open and verifyoldPathwithout following reparse points/CREATE_ALWAYS/truncation, retain its identity, and use a handle-bound atomic move to the unreplaced destination name; fail closed withErrTargetPossiblyTamperedon identity/race failures. Add a substitution-race regression for this unmarked recovery path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/update/replace_windows.go` around lines 156 - 168, Harden keepUnmarkedRecoveryCopy by opening and verifying oldPath through a non-reparse, non-truncating handle, retaining the verified object identity, and moving that handle atomically to the generated kept destination without pathname-based replacement. Treat identity mismatches, endpoint substitutions, and other race failures as ErrTargetPossiblyTampered, while preserving existing collision handling. Add a regression test covering substitution of the unmarked recovery path during relocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/update/replace_windows.go`:
- Around line 135-143: The recovery-marker failure path in the marker-writing
logic must not leave a partially created `.keep` marker before
`restoreOriginalBinary` relocates recovery state. Update the handling around
`marker.WriteString` and `marker.Close` so successful marker creation remains a
preservation success, while any write or close error removes the marker before
returning the error that triggers relocation.
- Around line 156-168: Harden keepUnmarkedRecoveryCopy by opening and verifying
oldPath through a non-reparse, non-truncating handle, retaining the verified
object identity, and moving that handle atomically to the generated kept
destination without pathname-based replacement. Treat identity mismatches,
endpoint substitutions, and other race failures as ErrTargetPossiblyTampered,
while preserving existing collision handling. Add a regression test covering
substitution of the unmarked recovery path during relocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fbc8e9b5-8eb3-486c-a8b4-64b65a611b5a
📒 Files selected for processing (6)
internal/update/apply.gointernal/update/apply_test.gointernal/update/replace_windows.gointernal/update/replace_windows_test.gointernal/update/stage_promote_windows_test.gointernal/update/stage_windows.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/update/stage_windows.go
- internal/update/apply_test.go
- internal/update/apply.go
- internal/update/replace_windows_test.go
Summary
Fixes #742 — the Windows updater staged verified executable bytes at the predictable path
<target>.newand opened it with truncating, link-following semantics, letting a lower-privileged process pre-create that path as a hard link or reparse point to another file the (possibly elevated) updater can write.Root cause
installBinary(internal/update/apply.go) always staged at a fixed<target>.newname viacopyFile, which opened the destination withO_CREATE|O_WRONLY|O_TRUNC. In an installation directory writable by a lower-privileged principal, that principal could pre-create<target>.newas a hard link or supported reparse-point link to another file writable by the elevated updater. The subsequent staging write then truncated and overwrote that unintended file with the verified Zero executable bytes.The
.oldrename/removal sequence inreplace_windows.gowas not itself the vulnerable primitive (per the issue) and is unchanged.Changes
stagingFilePathnow derives the staging name fromcrypto/rand(128 bits), so it can no longer be predicted or pre-created before the update runs.createStagingFile(platform-specific) opens the staging path exclusively without following any pre-existing link:internal/update/stage_other.go(POSIX):O_CREATE|O_EXCL, which POSIX guarantees fails on a pre-existing path — including a dangling symlink — without resolving it.internal/update/stage_windows.go:CreateFilewithCREATE_NEW|FILE_FLAG_OPEN_REPARSE_POINTso a pre-existing reparse point fails creation instead of being resolved through, plus a post-openGetFileInformationByHandlecheck (not a reparse point, not a directory, single hard link) as defense in depth.randomStagingSuffixis a swappable package var so the existing helper-refresh-failure test can pin a deterministic suffix instead of relying on the old guessable name.Tests
internal/update/stage_other_test.go/stage_windows_test.goreproduce the reported primitive directly: pre-create the staging path as a hard link (and, where privileges allow, a symlink) to a "victim" file, then assertcreateStagingFilerefuses it and the victim is untouched. Also covers the fresh-path success control and a concurrent-race case (exactly one winner, no silent truncation).TestApplyStandaloneUpdateWarnsWhenHelperRefreshFailsto pin the staging suffix via the new test hook instead of relying on the removed fixed.newname.Verification
go build ./...— pass (windows, plus cross-compiled linux/darwin forinternal/update)go vet ./...— pass (same platforms)go test ./internal/update/... -count=1— pass, including all new regression tests (hard link, symlink, concurrent race, fresh path) on Windowsgofmt -l— cleanSummary by CodeRabbit
Security Improvements
Tests