Skip to content

fix: a mount with two names was still outside the containment check - #6

Merged
JounQin merged 2 commits into
mainfrom
fix/mount-alias-and-node-floor
Oct 4, 2026
Merged

JounQin merged 2 commits into
mainfrom
fix/mount-alias-and-node-floor

Conversation

@JounQin

@JounQin JounQin commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

Found while deploying #5 to the host.

The guard

insideMountedPath compared resolve()d strings. On this host $HOME is
/var/services/homes/JounQin, which readlink -f resolves to
/volume1/homes/JounQin — the same directory the container mounts. So:

stage --to /volume1/homes/JounQin/Workspaces/<x>

wrote the broker's code into the agent-writable share, exactly what the guard
exists to prevent. It happened for real during verification; the stray tree was
identified by its STAGED.json and removed.

canonicalPath resolves the symlinks in a path's existing part — the nearest
existing ancestor plus the part that does not exist yet, because stage is
asked to create a directory that is not there. insideMountedPath compares
those, and so does stage's repository-overlap guard: a --to that is a
symlink into the repository is the repository. Only ENOENT/ENOTDIR are
walked up; any other realpathSync failure propagates, so a path whose real
form cannot be known is refused instead of being answered lexically.

Tests cover the host alias, a mount that does not exist yet behind that alias,
and a --to symlinked into the repository. The alias and overlap cases fail
against the old code.

Also

ci: name the platform jobs after their platform renames the test and
native-windows job ids to linux and windows, which is how macos and
wsl already read. Display names are untouched, so no required context moves.

Tests

corepack yarn check: 224 tracked tests (215 unit, 9 e2e), green.

Copilot AI balanced review requested due to automatic review settings October 3, 2026 19:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3b735cab-4726-43d4-8cd0-6961e24de436
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@git-credential-broker
git-credential-broker Bot force-pushed the fix/mount-alias-and-node-floor branch 5 times, most recently from a0ee868 to f84ae0e Compare October 3, 2026 19:13
@git-credential-broker git-credential-broker Bot changed the title fix: a mount with two names, and a Node floor that refused working interpreters fix: a mount with two names was still outside the containment check Oct 3, 2026
`insideMountedPath` compared `resolve()`d strings, so `/var/services/homes/u`
— the host's `$HOME` — and `/volume1/homes/u`, the same directory as the
container sees it, were two different places. `stage` therefore accepted
`--to /volume1/homes/u/Workspaces/...` and wrote the broker's code into a
directory the agent can rewrite, which is the one thing the check exists to
refuse. Reproduced on this host; the stray tree was removed.

Both sides are canonicalized now: the nearest existing ancestor is
`realpathSync`d and the part that does not exist yet is appended, because
`stage` is asked to write to a path that does not exist. The unit test
reproduces the alias, and the `stage` test asserts the refusal before
anything is written.
`test` and `native-windows` were the only job ids that named the action
rather than the platform; `linux`, `macos` and `wsl` already did. The
display names keep the `test on …` stem the checks are found by, so no
required context moves.
@git-credential-broker
git-credential-broker Bot force-pushed the fix/mount-alias-and-node-floor branch from f84ae0e to 850ca1d Compare October 3, 2026 19:38

@i1stG i1stG left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. I reproduced the bypass against the pre-fix code and checked the new guard, the overlap
change, the CI rename, and the suite.

Verified

  • The bug is real. insideMountedPath(realTarget, aliasHome) returns null on the parent of
    this branch and the mount here — the alias is the whole difference. The fix is not
    theoretical.
  • canonicalPath handles the half-written path stage is asked for. It resolves the nearest
    existing ancestor and appends the missing tail; only ENOENT/ENOTDIR are walked up, everything
    else propagates, so it fails closed. I also checked the dangling-symlink edge (a --to symlink
    whose target does not exist yet): mkdirSync fails ENOENT before any write, so it is not a
    write-through into a mount.
  • The overlap guard canonicalizes both sides, so a --to that is a symlink to the repository
    (or into it) is refused rather than compared by spelling.
  • corepack yarn check: 224 pass (215 unit, 9 e2e), matching the description. CI is green on
    Linux, macOS, Windows and WSL.
  • The CI rename is safe. The required contexts in the main ruleset are the name: values
    (test on Linux, test on Windows (refused), …), which are unchanged, and no job carries a
    needs: on the renamed ids.

Follow-ups — same class, pre-existing, not blockers for this diff

  • src/commands/setup.ts:110-113 compares path.resolve(input.gitconfig) against a lexical
    path.join(os.homedir(), '.dotfiles'). On the alias host this is defeated the same way — I
    measured false for the real spelling where a canonical comparison is true. canonicalPath is
    now exported; reusing it (or insideMountedPath) would close it.
  • src/host-request.ts:166 compares a realpathSync'd gitDir against a lexically resolved
    root. I reproduced it: discoverCheckouts(aliasHome) serves the plain checkout but skips a
    worktree whose .git file points under the real name. Fail-closed, but the host opener would
    silently not serve worktrees/submodules on exactly this host. canonicalPath(root) at line 183
    fixes it.

Neither is a regression introduced here, so I am approving the stated scope. Happy to take both as
a small follow-up, or folded in if you prefer.

@JounQin
JounQin added this pull request to the merge queue Oct 4, 2026
Merged via the queue into main with commit b83ce2f Oct 4, 2026
7 checks passed
@JounQin
JounQin deleted the fix/mount-alias-and-node-floor branch October 4, 2026 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants