fix(claws): harden ox install scripts against symlink escape and foreign checkouts#512
Merged
Conversation
…ign checkouts Co-Authored-By: SageOx <ox@sageox.ai> SageOx-Session: https://sageox.ai/repo/repo_019c5812-01e9-7b7d-b5b1-321c471c9777/sessions/2026-04-13T22-36-galexy-OxdAkf/view
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughPerforms stricter filesystem and repository validations in four installer/updater shell scripts: resolves physical ancestors before mkdir, rejects symlinked clone targets, re-canonicalizes paths under REAL_HOME, and verifies the repo’s origin URL and a clean working tree before aligning and installing/updating from origin. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@claws/openclaw/sageox-distill/scripts/install-ox-git.sh`:
- Around line 164-168: REAL_CLONE_PATH must not be a symlinked final component;
after computing REAL_PARENT and REAL_CLONE_PATH, test the final basename with:
if [ -L "$REAL_PARENT/$(basename "$CLONE_PATH")" ]; then ...; and if true either
canonicalize by resolving the symlink with readlink -f and assign that to
REAL_CLONE_PATH before running ownership/origin checks, or fail fast with an
error asking the user to remove the symlink so the install can create/own the
directory; apply this change around the REAL_CLONE_PATH computation and before
the ownership check (references: REAL_CLONE_PATH, REAL_PARENT, CLONE_PATH,
update-ox.sh).
- Around line 181-189: The origin-checking case that evaluates ORIGIN_URL for
REAL_CLONE_PATH currently only accepts https:// and scp-style git@github.com:
remotes and rejects URI-style SSH remotes like
ssh://git@github.com/sageox/ox(.git); update the case patterns to also accept
the URI SSH form by adding a branch matching ssh://git@github.com/sageox/ox and
ssh://git@github.com/sageox/ox.git (i.e., include a pattern like
ssh://git@github.com/sageox/ox|ssh://git@github.com/sageox/ox.git alongside the
existing patterns) so the origin validation in the case statement allows that
format.
In `@claws/openclaw/sageox-distill/scripts/update-ox.sh`:
- Around line 131-138: The case statement that checks the output of git -C
"$real_clone_path" remote get-url origin currently only matches HTTPS and
scp-style SSH URLs; update the whitelist to also match URI-style SSH remotes by
adding patterns for ssh://git@github.com/sageox/ox and
ssh://git@github.com/sageox/ox.git (i.e., include the ssh://... and optional
.git variants) alongside the existing https://... and git@... patterns so valid
ssh:// URLs do not trigger the skip-update warning.
In `@claws/openclaw/sageox-summary/scripts/install-ox-git.sh`:
- Around line 164-168: REAL_CLONE_PATH may point to a symlinked final path
component so ownership/origin checks can be bypassed; after computing
REAL_CLONE_PATH from REAL_PARENT and basename "$CLONE_PATH" resolve that final
component to its canonical target (e.g., use readlink -f or realpath on
REAL_CLONE_PATH) and replace REAL_CLONE_PATH (and the value later saved as
clone_path) with the resolved path so the script operates on and records the
actual checkout location; ensure subsequent checks that reference
REAL_CLONE_PATH, REAL_PARENT and CLONE_PATH use the resolved value.
- Around line 181-189: The origin validation case for ORIGIN_URL currently only
matches HTTPS and scp-style SSH URIs but omits the ssh:// URI form; update the
case pattern list for ORIGIN_URL (the case statement that checks origin against
https://github.com/sageox/ox(.git) and git@github.com:sageox/ox(.git)) to also
accept ssh://git@github.com/sageox/ox and ssh://git@github.com/sageox/ox.git
(i.e., add patterns matching the ssh://git@github.com/sageox/ox with and without
.git) in the same case block, and apply the identical change in both scripts
that contain this ORIGIN_URL case check so those ssh:// URIs are treated as
valid origins.
In `@claws/openclaw/sageox-summary/scripts/update-ox.sh`:
- Around line 131-138: The case that validates the origin remote (the case on
the command that starts with case "$(git -C "$real_clone_path" remote get-url
origin 2>/dev/null || true)") ignores ssh:// URI-style remotes; update the
pattern list to also accept ssh://git@github.com/sageox/ox and
ssh://git@github.com/sageox/ox.git so the auto-update doesn't skip repositories
using the ssh:// URI form. Locate the case block that compares the remote URL
and add corresponding ssh://... entries alongside the existing https://... and
git@github.com:... patterns.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f5fdb5f-1b84-48b3-9aa4-109f5d4aee64
📒 Files selected for processing (4)
claws/openclaw/sageox-distill/scripts/install-ox-git.shclaws/openclaw/sageox-distill/scripts/update-ox.shclaws/openclaw/sageox-summary/scripts/install-ox-git.shclaws/openclaw/sageox-summary/scripts/update-ox.sh
galexy
commented
Apr 14, 2026
Co-Authored-By: SageOx <ox@sageox.ai> SageOx-Session: https://sageox.ai/repo/repo_019c5812-01e9-7b7d-b5b1-321c471c9777/sessions/2026-04-13T22-36-galexy-OxdAkf/view
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@claws/openclaw/sageox-distill/scripts/install-ox-git.sh`:
- Around line 216-225: The origin URL check (ORIGIN_URL from git -C
"$REAL_CLONE_PATH" remote get-url) is spoofable because it trusts the existing
working tree; change the flow to fetch and reset to a trusted upstream ref
instead of executing code from the existing directory: after validating the
remote URL, perform a fresh fetch from that remote (or clone into a temporary
directory) and then reset the working tree to a known-good ref/commit/branch
(e.g., fetch origin and git reset --hard origin/<trusted-ref> or use a shallow
clone of sageox/ox into a temp path) before running any Makefile or other code;
ensure you reference REAL_CLONE_PATH/ORIGIN_URL in the checks and use the
freshly fetched/checked-out tree for subsequent execution.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6194e8f5-eecf-4126-a4c4-2899ce60b154
📒 Files selected for processing (4)
claws/openclaw/sageox-distill/scripts/install-ox-git.shclaws/openclaw/sageox-distill/scripts/update-ox.shclaws/openclaw/sageox-summary/scripts/install-ox-git.shclaws/openclaw/sageox-summary/scripts/update-ox.sh
✅ Files skipped from review due to trivial changes (1)
- claws/openclaw/sageox-summary/scripts/update-ox.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- claws/openclaw/sageox-distill/scripts/update-ox.sh
- claws/openclaw/sageox-summary/scripts/install-ox-git.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Round-2 follow-up to #511. CodeRabbit re-reviewed after the first round of fixes and flagged 3 criticals + 1 major that I acked before the merge — filing them now as a separate PR.
install-ox-git.shmkdir -pran BEFORE the symlink-escape check — a symlinked ancestor could letmkdircreate dirs in a foreign tree before validation fired.install-ox-git.sh.gitat the target path was accepted without verifyingremote.origin.url, so a hand-edited invocation pointing at any user-owned repo would happily run itsmake install.update-ox.sh"$HOME"/*prefix check broke when$HOMEwas symlinked:install-ox-git.shwrites the PHYSICALREAL_CLONE_PATHinto state, so the stored path starts with$REAL_HOMEand the textual$HOMEcheck rejected every legit auto-update.update-ox.shgit pull && make build && make installran on the recordedclone_pathwithout verifying it's actuallysageox/ox. A hand-edited state file pointing at any user-owned repo under$HOMEwould execute that repo's Makefile on every invocation.Fix shapes
#1 — walk up to the deepest existing ancestor, validate there, THEN mkdir.
Walking up from
PARENT_DIRto the first directory that exists lets us resolvecd … && pwd -Pon a real path, check it against$REAL_HOME, and verify ownership BEFORE any filesystem mutation. Once the ancestor is confirmed inside$HOMEand user-owned,mkdir -pcan only create plain directories underneath it —mkdirnever creates symlinks, so the final physical parent is guaranteed to stay inside the validated ancestor.#2 & #4 — `git -C remote get-url origin` check.
Both scripts now gate
make build && make installon an origin-URL allow-list:Covers HTTPS and SSH, with and without the
.gitsuffix. Forks are intentionally rejected — users with forks can still use the curl install method or adjust their state file manually after understanding what they're opting into.#3 — accept both `$HOME/` and `$REAL_HOME/` prefixes.
The text-level prefix check in
update-ox.shnow accepts either form, so the resolved physical path stored byinstall-ox-git.shpasses through to the physical-resolution + ownership check below, which is where the real security decision happens. The text check stays as a cheap early filter for obviously-bad hand-edits.Test plan
Smoke-tested each fix in isolation:
$HOME/escape-link → /tmp/ox-foreign, invokedinstall-ox-git.sh "$HOME/escape-link/deep/subdir" true. Before the fix,mkdir -p $HOME/escape-link/deepwould create/tmp/ox-foreign/deepbefore validation. After the fix: script emitserror: ancestor directory escapes $HOME via symlink: /private/tmp/ox-foreignand/tmp/ox-foreign/stays empty.$HOME/fake-repowithorigin=https://github.com/someone-else/not-ox.git, invokedinstall-ox-git.sh "$HOME/fake-repo" true. Script emitserror: … is not a sageox/ox checkout (origin: https://github.com/someone-else/not-ox.git)and exits before touching Make.HOME=/tmp/symlink-to-real-homewhere the symlink resolves to/tmp/ox-hardening-test, wrote state withclone_path=/private/tmp/ox-hardening-test/src/sageox-ox(the physical form), ranupdate-ox.sh. Script gets past the text prefix check and into the physical validation, which is what we want. (Before the fix, it would have emittedwarning: clone_path not under $HOMEand skipped every time.)clone_pathto a git repo whoseoriginishttps://github.com/someone-else/not-ox.git, ranupdate-ox.sh. Script emitswarning: clone_path is not a sageox/ox checkout; skipping auto-updateand falls through to the final readiness gate —make installnever runs.clawhub-skill-lint— PASS on both skills, 0 critical, 0 warnings.sageox-distillandsageox-summary.~/.openclaw/memory/— deferred to reviewer.Notes
$HOME/*and$REAL_HOME/*rather than canonicalizing one side. The physical-resolution check below is where the real decision happens; the text check is only an early filter for obvious hand-edits.mkdir -p(which existed in the previous version) is now redundant since the ancestor validation already guarantees the outcome, so the finalREAL_PARENTis computed but not re-validated. That simplification is intentional.Closes the round-2 findings on #511.
Summary by CodeRabbit