4 Commits

Include renames

Author	SHA1	Message	Date
cairn-internandcairn-intern	a3852bac59	fix(update): keep Windows failed-restore state in trusted recovery records (#979) ... * fix(update): keep Windows failed-restore state in trusted recovery records A failed restore currently records the last verified binary only as a .deletable .keep marker in the install directory. Write an unresolved identity-bound record to the existing per-user update-recovery store on that failure path too, and consult it during preflight so deleting the marker cannot silence the tamper refusal. Fixes #868 * fix(update): keep a fail-closed signal if unresolved recovery records cannot be written Ignoring a failed appendUnresolvedRecoveryRecord left only the sibling .keep marker. An install-directory writer can delete that marker, after which preflight finds no unresolved state and a retry can overwrite the last verified recovery copy. Relocate that copy to a .recovery name when the per-user record cannot be persisted so promotion still refuses. Fixes #868 * fix(update): do not relocate the live binary when failed-restore compensation cannot record persistFailedRestoreSignal relocated through the handle when the per-user unresolved record could not be written, without checking that the object was still the aside copy. A restore that had already put the verified bytes back at targetPath then had that binary moved out of the executable path. Verify identity with verifyPromotedTarget before relocating. Unreadable recovery-state JSON now fails closed instead of looking like an empty queue. The per-user vs per-machine split is documented: a second account still relies on the install-directory .keep marker. Fixes #868 * fix(update): durably commit recovery state on Windows --------- Co-authored-by: cairn-intern <[email protected]>	2026-09-12 08:29:11 +08:00
3 people	cabfeefc4e	security(update): stage binary replacement at an unpredictable, exclusive path (#751) ... * security(update): stage binary replacement at an unpredictable, exclusive path Fixes #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 <[email protected]> * security(update): bind the binary swap to the staged object, not its pathname Addresses the two open findings on #751. [P1] The staged file is no longer promoted by pathname. Randomizing the name and creating it exclusively stops PRE-creation, but under #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. * security(update): close remaining updater staging races Address jatmn's review findings on PR #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 <[email protected]> * fix(update): address staging review findings * fix(update): verify promoted object identity safely Bind POSIX staging creation and both rename endpoints to directory descriptors, clean only identity-matched empty directories, and verify Windows promotion by file identity rather than path spelling. Preserve unverifiable crash leftovers and add adversarial cleanup, hard-link, and reparse-ancestor coverage. Amp-Thread-ID: https://ampcode.com/threads/T-019fa019-27da-72ec-8e6d-5d43f127b6a1 Co-authored-by: Pierre Bruno <[email protected]> * fix(update): preserve promotion recovery signals Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <[email protected]> * fix(update): keep the recovery copy a failed restore promised Two error-path follow-ups from the #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) <[email protected]> * fix(update): stop the recovery copy from being destroyed by the next 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) <[email protected]> * fix(update): keep the recovery copy when its marker cannot be established 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) <[email protected]> * fix(update): preserve Windows recovery state across retries Amp-Thread-ID: https://ampcode.com/threads/T-019faf74-99d4-75cf-ac7a-661c308240ef Co-authored-by: Amp <[email protected]> * fix(update): watch every recovery path and never orphan a partial marker A failed second-or-later update marks the randomized aside it used, but promotion only consulted the canonical <target>.old marker, so a retry proceeded over an unverified target while the verified copy sat marked on a path the refusal logic did not watch. Promotion now enumerates every recovery candidate — canonical and randomized asides — and refuses while any of them is marked, naming each recovery path and its marker. The missing-target refusal had the same single-path blind spot: with a stale canonical .old from an earlier successful update plus the aside from an interrupted attempt, it named only the canonical path, which can be the wrong binary. It now names the single actual candidate or refuses the ambiguous layout outright until the operator resolves it. A failed marker write could also leave a partial .keep behind while the recovery copy was relocated elsewhere; later refusals then named a location that no longer held the verified bytes. markOldBinaryPreserved now removes the entry it created before reporting a write/close failure, and conservatively reports success when that removal (or the state check) fails so oldPath stays authoritative. keepUnmarkedRecoveryCopy is handle-bound end to end: it pins the recovery copy with a no-delete-sharing, no-reparse open, verifies it is a single-link regular file, renames through the handle with ReplaceIfExists false, and confirms identity at the destination, failing closed with ErrTargetPossiblyTampered on any race. Finally, the error for an unestablishable marker still promised that a later update would otherwise remove the copy, but CleanupStaleBinary is now a no-op; the operator is told that a manual copy is required. * fix(update): tolerate the fail-closed outcome of a fully-unlinked substitution TestPromoteInstallsTheStagedObjectNotTheStagedPath deletes the staging file's only directory entry and recreates it as an "attacker" file, then required promote to succeed with the verified bytes installed. CI's windows-latest runner permits that delete (this workstation's exclusive share mode blocks it, so the path went unexercised here); on that runner, renaming the now fully-unlinked staging handle back into existence is not something every Windows build honors, so verifyPromotedTarget's post- rename identity check finds nothing at targetPath, promote fails, and restoreOriginalBinary moves the pre-update binary back — a safe, fail- closed outcome, not a security regression. The test now accepts either outcome after a real substitution: promote succeeding with the verified bytes, or promote failing as long as the attacker's substituted bytes were never installed. Any promote failure that occurs without substitution having actually happened still fails the test outright. * fix(update): surface unverified recovery path, match .old case-insensitively keepUnmarkedRecoveryCopy discarded the destination path when the rename to it succeeded but the post-rename identity verification failed, leaving restoreOriginalBinary's error telling the operator to save oldPath after it had already been vacated. Return the attempted kept path alongside the verification error so the caller can still name it. existingRecoveryPaths compared "<target>.old" filenames case-sensitively, but NTFS is case-insensitive/case-preserving, so a recovery or marker file spelled e.g. "zero.exe.OLD" could be silently skipped by every caller's fail-closed check. Fold both sides before matching. Co-Authored-By: Claude Sonnet 5 <[email protected]> * fix(update): bound Windows recovery cleanup Amp-Thread-ID: https://ampcode.com/threads/T-019fbdec-f8dc-71f9-abdd-ea044a902b9a Co-authored-by: Pierre Bruno <[email protected]> * fix(update): secure Windows recovery cleanup Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-a912-7462-8938-1b4a8d95f1f3 Co-authored-by: Pierre Bruno <[email protected]> * fix(update): terminate Windows rename paths Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-a912-7462-8938-1b4a8d95f1f3 Co-authored-by: Pierre Bruno <[email protected]> * test(update): allow namespaced recovery copies Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-a912-7462-8938-1b4a8d95f1f3 Co-authored-by: Pierre Bruno <[email protected]> * fix(update): bind Windows cleanup to handles, drop dead cleanup API Failure-path staging cleanup on Windows re-resolved the staging pathname after the exclusive handle was closed, so a principal who can write in the installation directory could substitute that entry in the gap and have the updater delete a file of their choosing. Removal is now requested through the handle (FileDispositionInfo) before it is released, and nothing is removed by name afterwards; the same change covers the staging-file verification failure path and the partial recovery marker. CleanupStaleBinary has had no production caller since bounded, identity-bound cleanup moved into promote, so the exported no-op and the tests that could only pass vacuously against it are removed. The POSIX link-regression tests now drive createStagingFileAt, the primitive createStagedBinary actually uses, instead of a path-taking helper kept alive only for them. Also documents the fail-closed Windows recovery states in docs/UPDATE.md, including the refusal a planted .old/.keep pair can cause. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> * fix(update): bind Windows recovery to handles and bound its state Addresses the 2026-08-02 review round on #751. Keep the recovery restore bound to the object promote moved aside instead of its pathname: promote now opens the target with delete access and no delete sharing before renaming it aside, and restoreOriginalBinary renames that same handle back (retrying through the handle, never re-resolving the source name), so a directory writer cannot swap the aside entry during the promotion gap and have the substitute installed. Run the recovery-state checks as a standalone-install precondition, under the same lock promotion uses, so "already up to date" can no longer report success while a .keep/.recovery state is unresolved. The in-promotion check stays to close the race. Make the cleanup record a queue: entries are only retired once their object is actually gone, so a copy held open by a scanner is retried by a later update rather than leaking, while records for paths that vanished or that this updater could never have created are retired instead of accumulating. Also from review of the above: - delete the pathname restore chain that lost its production callers (restoreOriginalBinary's pathname form, openIdentityFile, recordRecoveryCleanup) and point their tests at the live handle path - name the relocation path in the error when the post-move verification fails, since oldPath is already vacated by then - scope the recovery docs to Windows and describe POSIX replace semantics Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --------- Co-authored-by: Claude Sonnet 5 <[email protected]> Co-authored-by: Amp <[email protected]>	2026-08-11 20:03:18 +08:00
PierrunoYTandCopilot Autofix powered by AI	008bc9b3f3	fix: address bugs found in a multi-agent codebase audit (#481) ... * fix: address bugs found in a multi-agent codebase audit Five parallel review agents each swept a different area (self-update, TUI keybindings, agent/session handling, sandbox/shell safety, and auth/credential storage) and surfaced nine concrete defects, all fixed here with regression tests: - sandbox: grant_scope path comparisons were case-sensitive, letting a persisted deny grant be bypassed by spelling a Windows path with different case - securefile/oauth: createSecretFile's Windows ERROR_ACCESS_DENIED handling was only applied to the oauth token store, not the shared credstore backend; both also masked the real lock-contention error behind a misleading "file does not exist" timeout - cli: firstUsableProvider ignored OAuth logins when picking a fallback provider, forcing unnecessary re-onboarding for an already-authenticated user - sessions: exec prompt summarization truncated on a raw byte offset, which could split a multi-byte UTF-8 rune and embed invalid UTF-8 into a resumed session's prompt - tui: configurable keybindings had no collision detection, so a remapped chord could silently make a hardcoded shortcut (or another configured binding) permanently unreachable - tools: the Windows shell-syntax pre-flight check wasn't quote-aware and could block harmless commands whose quoted arguments merely contained operator-like text - update: checksum verification didn't cross-check the archive filename it verified against the one being extracted; extractZip didn't reject symlink-mode entries like extractTarGz already did; replaceBinary's restore-on-failure rename had no retry, risking a permanently missing binary on a transient Windows file lock * fix: close remaining PR 481 review findings * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <[email protected]> * fix(tools): drop stale ; test case after rebasing onto main's MSYS check Rebasing fix/bug-hunt-findings onto main pulled in #476, which replaced this file's regex-based windows_shell_syntax detection with a segment/word-anchored check and deliberately stopped treating ; as a command separator on Windows (cmd.exe doesn't split on ; the way bash does). detectShellCommandIssue itself came out of the rebase unchanged from main (our quote-masking version was fully superseded by #476's more thorough segment-based approach), but the quoted-content test this PR added still asserted the old bash-style semantics for `echo "foo" ; ls -la`, which now correctly returns no issue since it's a single echo invocation with literal arguments under cmd.exe, not a real ls invocation. --------- Co-authored-by: Copilot Autofix powered by AI <[email protected]>	2026-07-06 18:48:28 +08:00
PierrunoYTandVasanthdev2004	5f36349c18	feat(update): add zero upgrade command to apply self-updates (#461) ... * feat(update): add zero upgrade command to apply self-updates Extends the existing `zero update --check` (which only reported an available release) with an actual install step: - `zero update --apply` / `zero upgrade` download, sha256-verify, and install the latest release. - npm installs delegate to `npm install -g @gitlawb/zero@latest`. - Standalone installs download the release archive, verify its checksum, extract it (zip-slip guarded), and atomically replace the running binary plus any pre-existing optional sandbox helpers. On Windows, the running exe is renamed aside (NTFS allows this even while it's executing) since it can't be overwritten directly; the leftover ".old" file is cleaned up on a later invocation. Closes #459. * fix(update): correct macOS expectations in apply test The optional-helper assertion in TestApplyStandaloneUpdateReplacesBinary assumed every non-Windows platform has a linux-style "zero-seccomp" helper to refresh, but applyStandaloneUpdate correctly ships no optional helpers on macOS (matching scripts/postinstall.mjs). This made the test fail on macOS CI. Only assert the helper refresh on linux/windows. * fix(update): address CodeRabbit review findings on zero upgrade - Reject --target when combined with --apply/upgrade: applying installs onto the current machine, so a mismatched --target could replace the running binary with an incompatible-arch build. - Bound archive/checksum downloads with a dedicated timeout separate from the release-metadata check timeout, so a stalled connection can't hang zero upgrade forever. - Surface a failed rollback in the Windows replaceBinary path instead of swallowing it, so a botched install is reported rather than silently leaving the binary missing. - Run CleanupStaleBinary regardless of whether an update is available, so a leftover Windows ".old" file isn't stuck waiting for a future upgrade that may never come. - Report optional sandbox-helper refresh failures as warnings on ApplyResult (surfaced in text and JSON output) instead of discarding them silently. * fix(oauth): retry secret-lock on Windows delete-pending race to unblock CI TestLoadOrCreateSecretConcurrentConverges was failing this PR's Windows smoke job with "create token secret lock: ...Access is denied." — a pre-existing flake in internal/oauth, not in the upgrade code. On Windows a concurrent holder's os.Remove leaves the .lock in a delete-pending state, so an O_EXCL create races it with ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist; createSecretFile only treated ErrExist as contention and hard-failed otherwise. Treat os.ErrPermission as retryable too, mirroring acquireFileLock in lock.go. This is the same root-cause fix as #445 (diagnosis by @gnanam1990); folded in here to make this PR's CI green. Verified on Windows: the concurrent-converge test passes 30/30. --------- Co-authored-by: Vasanthdev2004 <[email protected]>	2026-07-04 08:52:07 +08:00