fix: persist the installer's branch so a device remembers what it was built with - #385
fix: persist the installer's branch so a device remembers what it was built with#385KrasimirKralev wants to merge 8 commits into
Conversation
β¦ built with install.sh has always READ $PROJECT_DIR/.update-branch and never written it, so the pin existed only where a human had created one. Without it a device falls through to resolve_update_branch rule 2 β the current branch, and only if that branch tracks a remote. That upstream link does not survive a re-clone, so a box flashed with an explicit CLAWBOX_BRANCH can later resolve to main and update itself onto a branch it was never built for. Two freshly provisioned devices turned up with no pin at all. persist_update_branch_pin records an explicit CLAWBOX_BRANCH into the pin from step_git_pull, in both directions: - explicit CLAWBOX_BRANCH writes the pin and overwrites a different existing one, matching the precedence resolve_update_branch already documents (CLAWBOX_BRANCH > .update-branch > current branch > main). The repo has just been hard-reset onto that branch; a pin naming the old one would pull the device back off it on the next unattended update. The change is announced. - no CLAWBOX_BRANCH never writes and never deletes, so a bare re-run of the installer β and every updater-triggered `--step` β leaves the device where it is. The pin is chowned to the app user and left 0644 on every run: the web app rewrites this file itself through /setup-api/system/update-branch, so a root-owned pin would turn that into an EACCES. Re-asserting it each run also repairs a pin left root-owned by a hand-written redirect. The file is already gitignored, so it does not dirty the working tree. Also documents in updater.ts who writes the pin, and pins the two survival claims its docstring makes (factory reset, git reset) with tests.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
π WalkthroughWalkthroughThe installer validates and persists update branches in ChangesUpdate branch handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant Repository
participant PinFile
participant Updater
Installer->>Repository: resolve and synchronize update branch
Installer->>PinFile: validate and persist .update-branch
Updater->>PinFile: read persisted branch
PinFile-->>Updater: return valid pin or fallback input
Updater->>Repository: resolve update target
Suggested reviewers: π₯ Pre-merge checks | β 4 | β 1β Failed checks (1 warning)
β Passed checks (4 passed)
β¨ Finishing Touches π‘ 1π Generate docstrings π‘
π§ͺ 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 |
π¦ ClawReviewScuttled over to say hello and get you oriented π¦ This fix closes a real gap: devices flashed with a specific branch (e.g. At a glance
Good to know
β ClawReview π¦. I set the scene; CodeRabbit reviews the code; you decide. Conventions: docs. |
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 `@install.sh`:
- Around line 1034-1061: The update-branch persistence flow around pin_file must
not follow an attacker-controlled symlink when running as root. Validate that
pin_file is not a symlink before reading or writing it, and abort safely if it
is; ensure the subsequent printf, chown, and chmod operate only on the intended
regular file. Add a regression test covering a symlinked .update-branch path and
confirming the target remains unchanged.
πͺ Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 8b29f05e-fb51-45c5-95d0-88f2686b9b61
π Files selected for processing (3)
install.shsrc/lib/updater.tssrc/tests/unit/install-update-branch-pin.test.ts
A pin the runtime refuses is not an error β resolveUpdateBranch falls through to rule 2 and then to `main`. So every disagreement between the thing that WRITES .update-branch and the things that READ it is silent branch drift, and persisting the installer's branch was not by itself enough. Measured against the PR head, per scenario, as install.sh resolve / updater.ts resolve after one installer run: no pin, on beta, upstream LOST main/main -> beta/beta pin='feat/a+b', upstream OK feat/a+b/beta -> beta/beta pin='feat/a+b', upstream LOST feat/a+b/main -> main/main pin='HEAD' beta/HEAD -> beta/beta no pin, detached HEAD main/main -> main/main (see below) on main: pinned, unpinned, upstream lost, detached β main through every path, unchanged. One validator, three consumers. install.sh's is_safe_git_ref used only `git check-ref-format --branch`, which accepts `feat/a+b` and `ΓΌnΓ―code` β refs updater.ts refused β so the installer wrote pins that read correct and resolved to main. The character class now lives in src/lib/update-branch.ts and is shared by the updater and the Settings route, while install.sh applies the same class before git's grammar check. That grammar check is the half updater.ts was missing: `HEAD` passed its regex, and `git reset --hard origin/HEAD` follows origin's default branch, so a device pinned to HEAD tracked main. The Settings POST returned 200 for it. A test runs both validators over one corpus and asserts install.sh can never write what the runtime would refuse. An unpinned device now adopts the branch it is checked out on. This is where the re-clone case actually bites: the upstream LINK does not survive a re-clone even though the branch does, and rule 2 requires the link. Adoption does not move the device β it records where it already is β and it is narrow: never over an existing pin, never `main` (rule 3's fallback is main anyway), never a ref the updater would refuse, and never a branch origin does not carry, which would turn today's fallback into a failing `reset --hard`. The call site moves ahead of resolve/sync in step_git_pull because sync_repo_to_update_target destroys the evidence, and because pinning first stops the bootstrap block and step_git_pull disagreeing inside one run. Ownership repair now runs whether or not CLAWBOX_BRANCH was given. The previous version returned before it on a bare run, so the claim that it healed a root-owned pin was not true for the only run that had to. A pin the app user cannot read is invisible to the updater β which resolves main β while install.sh as root still reads it and disagrees. The pin is written through a temp file and renamed, and a symlink in its place is refused rather than followed: the project dir belongs to the app user and this code runs as root, so `[ -f ]` alone let the write, chown and chmod land on the link's target. Not closed: a detached HEAD with no pin still resolves to main. There is no branch name to record, and inferring one from the commit is a guess β on this repo main and beta sit on the same commit, so `--points-at HEAD` is ambiguous exactly when it would matter. The bootstrap block has also already reset to origin/main by then. A device only reaches that state by hand; the flasher passing CLAWBOX_BRANCH is the fix. 201 files / 2563 tests pass.
Matches how resolve_update_branch spells the same call, and avoids the word-splitting the unquoted $git_cmd variable relied on.
There was a problem hiding this comment.
Actionable comments posted: 3
π€ 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 `@install.sh`:
- Around line 1113-1119: The temporary pin update in install.sh (the
printf/chown/chmod/mv sequence) must not use the user-writable $pin_file.tmp
path; create the temporary file securely in a root-controlled location or keep
all writes on a safely opened descriptor, then preserve ownership, mode, and
atomic replacement behavior. Add ignore rules for the generated temporary names.
In src/tests/unit/install-update-branch-pin.test.ts, add a regression case
placing a symlink at ${pinFile}.tmp and verify the update does not follow or
modify its target.
In `@src/tests/unit/install-update-branch-pin.test.ts`:
- Around line 157-172: Add a regression test alongside the existing symlink test
that creates a decoy target and symlink at `${pinFile}.tmp`, then runs
`runPersist` with the branch set. Assert the decoy content and mode are
unchanged, no unintended ownership changes occur, and the temporary symlink
remains protected while verifying the persist operationβs expected result.
In `@src/tests/unit/update-branch-validator.test.ts`:
- Around line 36-44: Update installShAccepts to pass ref through bash argv
rather than interpolating JSON.stringify(ref) into the script. Invoke the shell
with a positional argument and call is_safe_git_ref using that argument,
preserving literal newlines and preventing command substitution so the shell
validator receives the exact same value as isSafeBranch.
πͺ Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 8a134aa8-a409-45b8-8bff-93c4374cabe2
π Files selected for processing (7)
.gitignoreinstall.shsrc/app/setup-api/system/update-branch/route.tssrc/lib/update-branch.tssrc/lib/updater.tssrc/tests/unit/install-update-branch-pin.test.tssrc/tests/unit/update-branch-validator.test.ts
β¦text JSON.stringify quotes for JavaScript, not for bash, so two corpus entries were never actually compared: a real newline arrived at is_safe_git_ref as a literal backslash-n, and "$(id)" was command-substituted, so the shell was validating the output of `id` while isSafeBranch validated the literal. Both sides still returned "reject", so the agreement assertion passed on two different strings β exactly the failure mode the test exists to catch. The ref now arrives as $1 and is never interpolated. Two canaries are added that make the mistake impossible to reintroduce quietly: "$(echo main)" and "`echo main`" would be substituted to `main` and ACCEPTED by a shell that interpolated them, while isSafeBranch keeps rejecting the literal. Verified both ways β under the old quoting the two agreement tests fail on exactly those entries; under the fix all 8 pass. Caught by CodeRabbit.
β¦se picks Two ways the writer could still be steered from outside the ref it was given. The temp file. Guarding "$pin_file" against a symlink moved the problem one path across: "$pin_file.tmp" is a fixed, guessable name in the same app-user-writable directory, and `printf >` follows an existing symlink there. Measured against the previous revision with a decoy planted at that path β the decoy went from ORIGINAL-DECOY-CONTENT / 600 to `beta` / 644, and the pin itself ended up a symlink. mktemp now picks the name and creates the file with O_EXCL, so the write, the chown and the chmod land on a file this function just made; the same decoy is untouched afterwards. A failed step removes the temp file rather than stranding it, and .gitignore covers .update-branch.* instead of the single old name. The working directory. `git check-ref-format` needs no repository, but git runs repository discovery from the working directory first, and a broken .git there β a moved worktree, a half-restored backup β makes it exit 128 for EVERY ref. is_safe_git_ref then rejects everything, which does not read as an error: no pin is written and the device falls back to main, this PR's whole failure mode, decided by nothing but the directory the operator happened to be standing in. `git -C /` pins the answer to the ref. Measured: `is_safe_git_ref beta` from a directory holding a dangling .git returned reject before, ACCEPT after, while `HEAD` and `a..b` are still rejected. Tests for both, plus the temp-path decoy. 201 files / 2565 tests pass. Temp-path finding from CodeRabbit.
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 `@install.sh`:
- Around line 1128-1135: Eliminate the pathname race in the update-branch pin
write around the install.sh pin-update logic by using file-descriptor-safe
operations or a root-only temporary directory, ensuring an unprivileged clawbox
user cannot replace the created temporary path before writing or moving it. Add
a regression in src/tests/unit/install-update-branch-pin.test.ts covering
replacement of the actual mktemp result after creation, and update .gitignore
only as needed for the test artifact.
πͺ Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: c6133179-64f9-4320-b278-8119b16c0931
π Files selected for processing (4)
.gitignoreinstall.shsrc/tests/unit/install-update-branch-pin.test.tssrc/tests/unit/update-branch-validator.test.ts
mktemp closed the predictable-path problem but not the race behind it. The temp
file still lived in $PROJECT_DIR, which $CLAWBOX_USER can write, so that account
could watch the directory, see the name appear, unlink it and leave a symlink
before the printf/chown/chmod landed. What that buys is not a wrong pin β it is
root's chown aimed at a path of someone else's choosing, which is an escalation
primitive rather than a pin problem. config/clawbox-sudoers already treats
clawbox-level access as a boundary worth defending (see its note on why the
apt-get grants are exact-match), so "they could just sudo" is not an answer here.
The write is now staged inside a 0700 directory this function creates, so
nothing but root can create or unlink the file being chowned. It stays inside
$PROJECT_DIR so the final step is still a rename, and rename replaces the pin's
directory entry rather than following a symlink left at it.
Measured, planting a decoy symlink at the staging/temp path:
before plant=tmp decoy=beta / 644 (clobbered) chown-> .update-branch.tmp
parent-dir=755
after plant=tmp decoy untouched / 600 chown-> .stage/pin
parent-dir=700
after plant=stage decoy untouched / 600 chown-> .stage/pin
parent-dir=700
Losing the race for the staging path itself only costs the write: mkdir fails on
an existing path and the pin is left as found. Nothing is redirected. An account
that can do that can already write the pin file directly β the Settings route
does exactly that β so pin contents were never protected from it. Root's
authority is what had to be.
A planted symlink does not even cost that much: `rm -rf` removes the link and
never its target, so the staging dir is created fresh and the pin lands
normally, as the test now asserts.
201 files / 2567 tests pass.
Race reported by CodeRabbit.
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 `@install.sh`:
- Around line 1139-1149: Update the update-branch pin staging and
ownership-repair logic in install.sh to use trusted directory descriptors with
no-follow openat/renameat-style operations, preventing replacement of
.update-branch.stage or $pin_file between validation and mutation. Preserve the
existing permissions and ownership behavior, and add tests in
src/tests/unit/install-update-branch-pin.test.ts covering replacement after
mkdir (lines 156-168) and replacement before ownership repair (lines 195-215).
πͺ Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 65489ff0-f126-4189-ac02-4a81004f30f9
π Files selected for processing (2)
install.shsrc/tests/unit/install-update-branch-pin.test.ts
My previous message claimed losing the race for the staging path "only costs the write". That is wrong and I am correcting it rather than leaving it standing: unlinking a directory entry is governed by the PARENT's write bit, which $CLAWBOX_USER has on $PROJECT_DIR, so $stage can be rmdir'd and replaced between the mkdir and the write. 0700 on the directory does not prevent that. What staging actually buys is the case that needed no timing at all β a symlink pre-planted under a name that never changes. That one is now shut. The remaining exposure is a genuine race, and closing it needs descriptor-bound openat/renameat with no-follow semantics, which POSIX shell cannot express. Accepted deliberately, and the reason is three lines further down the same function: sync_repo_to_update_target runs `git reset --hard` as root inside this same app-writable tree and then `chown -R` over all of it. Whoever can win the race on the pin already has a far larger version of the same primitive in the same step. Hardening one file past this point while that stands would be motion, not progress β if the class is worth closing it has to be closed for the tree, not for the pin. No behaviour change; comments and one test name/assertion only.
The gap
A device flashed with an explicit branch gets it as
CLAWBOX_BRANCH. What it later updates to is decided separately, byresolve_update_branch(install.sh) andresolveUpdateBranch(src/lib/updater.ts):.update-branchpin in the project rootmaininstall.shhas always read that pin and never written it, so it only existed where a human had created one. Two freshly provisioned devices turned up with no pin at all. Rule 2 covered them β but a branch's upstream link does not survive a re-clone even though the branch does, so an unpinned unit can fall through tomainand update itself onto a branch it was never built for.That matters because units flashed from a given branch are shipped relying on the in-app updater to bring them current in transit.
The thing that makes all of this sharp
A pin the runtime refuses is not an error.
resolveUpdateBranchdoes not fail on a value it rejects β it falls through to rule 2, then tomain. So every disagreement between the thing that writes.update-branchand the things that read it is silent branch drift, with a pin that still reads as a deliberate choice.That is why this PR is larger than "write the file".
Scope
Deliberately bounded. A device on
mainresolves to main through every path β pinned, unpinned, upstream lost, detached HEAD β because rule 2 returns main when the current branch is main and rule 3's fallback is main. So none of this is load-bearing for main-flashed units. It matters for beta and feature-branch flashes, and for a support engineer who later runs the installer with an explicit branch.Measured, per scenario
install.shresolve /updater.tsresolve, after one installer run:beta/betabeta/betamain/mainbeta/betafeat/a+b, upstream OKfeat/a+b/betabeta/betafeat/a+b, upstream LOSTfeat/a+b/mainmain/mainHEADbeta/HEADbeta/betamain/mainmain/mainβ not closed, see belowmain/mainmain/mainCLAWBOX_BRANCH=beta, on main, no pinbeta/betabeta/betaThe
feat/a+brows are the interesting ones: the installer and the updater were resolving to different branches on the same device.And on the writer side β what
install.shactually persists:CLAWBOX_BRANCH=betabetabetafeat/a+bfeat/a+bΓΌnΓ―codeΓΌnΓ―codeHEAD,a..b,x/,-DOne validator, three consumers
is_safe_git_refused onlygit check-ref-format --branch, which acceptsfeat/a+bandΓΌnΓ―codeβ refsupdater.tsrefused. The installer wrote pins that read correct and resolved tomain.The character class now lives in
src/lib/update-branch.ts, shared by the updater and the Settings route (it was duplicated between them).install.shapplies the same class before git's grammar check.Git's grammar check is the half
updater.tswas missing.HEADpasses the regex, andgit reset --hard origin/HEADfollows origin's default branch β so a device pinned toHEADtrackedmain, andPOST /setup-api/system/update-branch {"branch":"HEAD"}returned 200. Same fora..b,x/,a.lock.src/tests/unit/update-branch-validator.test.tsruns both validators over one shared corpus and asserts the safety-critical direction: install.sh can never write a value the runtime would refuse.An unpinned device adopts the branch it is on
Only when there is no pin at all, and only via
adoptable_checkout_branch, which refuses to guess:mainβ rule 3's fallback is already main; a pin would only freeze a box an operator later moves by handreset --hard origin/<branch>on every future updateThis does not move the device; it records where it already is. The call site moves ahead of resolve/sync in
step_git_pull, becausesync_repo_to_update_targetdestroys the evidence, and because pinning first stops the bootstrap block (which follows the checked-out branch with no upstream requirement) andstep_git_pull(rule 2, which requires one) disagreeing inside a single run.Ownership
Re-asserting owner and mode now runs whether or not
CLAWBOX_BRANCHwas given. It previously returned before that on a bare run, so the claim that it healed a root-owned pin was not true for the only run that had to do it. This matters beyond the Settings POST: a pin the app user cannot read is invisible to the updater, which resolvesmain, whileinstall.shas root still reads it and disagrees.0644, not0600: a build record, not a secret, and root reads it during the bootstrap re-exec.Writing the pin
The pin path is refused outright if it is a symlink β
[ -f ]alone follows one, and the project dir belongs to the app user while this code runs as root. Measured against the pre-fix revision: a root-owned decoy went from600/ORIGINAL-ROOT-CONTENTto644/beta.Guarding only the pin path moved the problem one path across, though: the file the write staged through lived in the same app-user-writable directory, so it could be unlinked and replaced with a symlink β as a fixed
.update-branch.tmpwith no timing needed at all, or, once the name was randomised, by watching the directory and winning a race. What that buys is not a wrong pin but root'schownaimed at a path of someone else's choosing.The write is now staged inside a
0700directory the function creates, so nothing but root can create or unlink the file being chowned. It stays inside$PROJECT_DIR, so the final step is still a rename β andrename(2)replaces the pin's directory entry rather than following a symlink left at it..gitignorecovers.update-branch.*.Measured, planting a decoy symlink at the temp/staging path (
parent-dir= mode of the directory holding the chown target):.tmpbeta, mode644β clobbered.update-branch.tmp755.tmp600.update-branch.stage/pin700.stage600.update-branch.stage/pin700What staging does not do is close the race behind it, and the
0700is not what would: unlinking a directory entry is governed by the parent's write bit, which the app user has on$PROJECT_DIR, so the staging directory can still be replaced between themkdirand the write. Closing that needs descriptor-boundopenat/renameatwith no-follow semantics, which POSIX shell cannot express.That residual is accepted deliberately, and the reason is three lines further down the same function:
sync_repo_to_update_targetrunsgit reset --hardas root inside this same app-writable tree and thenchown -Rover all of it β recursive, hundreds of paths, in the same step. Anyone who can win the race on the pin already has a far larger version of the same primitive on the same run. Hardening one file past this point while that stands would be motion rather than progress; if the class is worth closing it has to be closed for the tree, and that is a different change with a different blast radius than "persist the installer's branch".The validator must not depend on where you ran the installer
git check-ref-formatneeds no repository, but git runs repository discovery from the working directory first. A broken.gitthere β a moved worktree, a half-restored backup β makes it exit128for every ref.is_safe_git_refthen rejects everything, and that does not read as an error: no pin is written and the device falls back tomain. This PR's exact failure mode, decided by nothing but the directory the operator happened to be standing in.git -C /pins the answer to the ref. Measured from a directory holding a dangling.git:is_safe_git_ref betareturnedrejectbefore andACCEPTafter, whileHEADanda..bstay rejected.Not closed
A detached HEAD with no pin still resolves to
main. There is no branch name to record. Inferring one from the commit is a guess: on this repomainandbetacurrently sit on the same commit, sogit for-each-ref --points-at HEADreturns several remote branches β ambiguous exactly when it would matter (measured: 5 refs in the lab repo). The bootstrap block at the top ofinstall.shhas also alreadyreset --hard origin/mainby the timestep_git_pullruns, so even the commit identity is gone.A device only reaches that state by hand. The fix is for the flasher to pass
CLAWBOX_BRANCH, which this PR then records.Tests
src/tests/unit/install-update-branch-pin.test.tsβ 33 tests (was 17). Shell cases source the functions verbatim out ofinstall.shand run them against a throwaway project dir; the adoption cases build a real clone of a real throwaway origin, because whatadoptable_checkout_branchinspects is remote-tracking refs and HEAD.chownis stubbed and logged;chmodis left real so the mode is read off the filesystem.src/tests/unit/update-branch-validator.test.tsβ the validator-agreement corpus (38 refs), including two canaries ($(echo main)and a backticked equivalent) that fail the agreement assertion if the ref is ever interpolated into the shell script instead of passed as an argument, rather than letting the two sides silently compare different strings.Covers, beyond the original set: adoption and each of its four refusals; ownership repaired on a bare run; the symlink refusal on the pin path and on the temp path; no temp file left behind; the broken-
.gitworking directory;step_git_pullpinning before it resolves and syncs.Verification
Full unit suite: 201 files / 2567 tests passing, ESLint clean on the changed files. (Baseline before this PR: 200 / 2544 β the delta is the 23 tests added here, no regressions.)
The before/after matrices above are produced by running the real
resolve_update_branchextracted from each revision ofinstall.shand the realresolveUpdateBranchextracted from each revision ofupdater.tsagainst purpose-built git repos, so both columns are measurements rather than readings of the diff.Earlier hardware verification of the original commits (OpenClaw box, from an existing
betapin) still stands: explicit branch over a different pin re-pinned and announced;owner=clawbox group=clawbox mode=644; a bare--step git_pullafterwards left the pin byte-identical;GET/POST /setup-api/system/update-branchround-tripped as the app user over a root-written pin;git statusclean with the pin correctly ignored.