From fbee6e8b1456bcab49d03dff8a2f5d7eebd8c09a Mon Sep 17 00:00:00 2001 From: JounQin Date: Sat, 3 Oct 2026 19:03:09 +0000 Subject: [PATCH 1/2] fix: a mount with two names was still outside the containment check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- src/commands/stage.ts | 12 ++++++++-- src/commands/support.ts | 46 ++++++++++++++++++++++++++++++++++-- test/unit/commands.test.ts | 48 ++++++++++++++++++++++++++++++++++++++ test/unit/stage.test.ts | 37 +++++++++++++++++++++++++++++ 4 files changed, 139 insertions(+), 4 deletions(-) diff --git a/src/commands/stage.ts b/src/commands/stage.ts index dda4aa7..9d0acb7 100644 --- a/src/commands/stage.ts +++ b/src/commands/stage.ts @@ -21,7 +21,7 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; -import { fail, insideMountedPath, parseArgs, say, warn, writeSecretFile } from './support.ts'; +import { canonicalPath, fail, insideMountedPath, parseArgs, say, warn, writeSecretFile } from './support.ts'; /** The record `stage` leaves behind, so the running code is identifiable afterwards. */ export const STAGED_RECORD = 'STAGED.json'; @@ -69,7 +69,15 @@ export function performStage(input: StageInput): StageResult { 'private key in reach, so code there is code the agent can rewrite — stage outside every mount.', ); } - if (to === repo || to.startsWith(repo + path.sep) || repo.startsWith(to + path.sep)) { + // Canonical, for the same reason the mount check is: a `--to` that is a symlink into the repository + // *is* the repository, and comparing the spellings would miss it. + const realRepo = canonicalPath(repo); + const realTo = canonicalPath(to); + if ( + realTo === realRepo || + realTo.startsWith(realRepo + path.sep) || + realRepo.startsWith(realTo + path.sep) + ) { throw new Error(`refusing to stage ${repo} into ${to}: the repository and the target overlap`); } diff --git a/src/commands/support.ts b/src/commands/support.ts index c5e0632..bfe17ae 100644 --- a/src/commands/support.ts +++ b/src/commands/support.ts @@ -111,6 +111,43 @@ export function isInsideContainer(): boolean { return fs.existsSync('/home/app/Workspaces') && !fs.existsSync('/volume1'); } +/** + * A path with the symlinks in its existing part resolved. + * + * `realpathSync` throws for a path that does not exist yet, and `stage` is asked to write to one, so the + * nearest existing ancestor is resolved and the rest is appended. Comparing the raw spellings is what let + * `/var/services/homes/u/Workspaces` and `/volume1/homes/u/Workspaces` — one directory with two names — + * look like different places, which is exactly the bypass the mount and overlap checks exist to close. + * + * @param target - the path to canonicalize. + * @returns the real path, or the resolved path when nothing on it exists. + * @throws {Error} when the path cannot be resolved for a reason other than being absent, because a + * lexical answer there could call a path inside a mount outside it. + */ +export function canonicalPath(target: string): string { + const absolute = path.resolve(target); + let existing = absolute; + const missing: string[] = []; + for (;;) { + try { + const real = fs.realpathSync(existing); + return missing.length === 0 ? real : path.join(real, ...missing.toReversed()); + } catch (error) { + // Only a path that is not there is walked up. Anything else — EACCES, ELOOP — means the real path + // cannot be known, and a lexical fallback could miss a symlinked component and pass a path the + // check exists to refuse, so it propagates and the caller fails closed. + const code = (error as NodeJS.ErrnoException).code; + if (code !== 'ENOENT' && code !== 'ENOTDIR') throw error; + const parent = path.dirname(existing); + // Unreachable while the root exists and realpaths; kept so a filesystem that refuses even `/` + // cannot spin here forever. + if (parent === existing) return absolute; + missing.push(path.basename(existing)); + existing = parent; + } + } +} + /** * The container's mounts under the user's home, as seen from the host. * @@ -126,14 +163,19 @@ export function mountedPaths(home: string = os.homedir()): string[] { /** * Whether a path is inside one of the container's mounts. * + * Both sides are canonicalized before they are compared: `$HOME` on the host may be one name for a + * directory the container sees under another (`/var/services/homes/u` and `/volume1/homes/u`), and a + * lexical prefix check calls the same directory two different places. + * * @param target - the path to check. * @param home - the home directory to resolve against. * @returns the mount it is inside, or null. */ export function insideMountedPath(target: string, home: string = os.homedir()): string | null { - const resolved = path.resolve(target); + const resolved = canonicalPath(target); for (const mounted of mountedPaths(home)) { - if (resolved === mounted || resolved.startsWith(`${mounted}${path.sep}`)) return mounted; + const real = canonicalPath(mounted); + if (resolved === real || resolved.startsWith(`${real}${path.sep}`)) return mounted; } return null; } diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index aaad021..cb592bb 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -105,6 +105,54 @@ describe('mounted path detection', () => { assert.equal(insideMountedPath('/volume1/docker/git-cred-broker', home), null); assert.equal(insideMountedPath('/home/app/Workspaces-evil', home), null, 'must not prefix-match a sibling'); }); + + it('resolves the aliases of one directory instead of trusting the spelling', () => { + // Measured on the host this was found on: `$HOME` is /var/services/homes/u while the container sees the + // same directory as /volume1/homes/u, so a lexical prefix check called a path inside the mount outside + // it — and `stage` wrote the broker's code into a directory the agent can rewrite. + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-mount-alias-')); + const real = path.join(root, 'real'); + fs.mkdirSync(path.join(real, 'Workspaces'), { recursive: true }); + const alias = path.join(root, 'alias'); + // 'junction' so the case also runs on Windows, where a plain symlink needs a privilege; the type is + // ignored on the platforms that have no junctions. + fs.symlinkSync(real, alias, 'junction'); + try { + const mounted = path.join(alias, 'Workspaces'); + assert.equal(insideMountedPath(path.join(real, 'Workspaces', 'x'), alias), mounted, 'real target, aliased home'); + assert.equal(insideMountedPath(path.join(real, 'Workspaces'), alias), mounted, 'and the mount itself'); + assert.equal(insideMountedPath(path.join(real, 'elsewhere'), alias), null, 'a sibling is still outside'); + assert.equal( + insideMountedPath(path.join(alias, 'Workspaces', 'y'), real), + path.join(real, 'Workspaces'), + 'and the reverse spelling', + ); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); + + it('canonicalizes a mount that does not exist yet, which is the path stage is asked to create', () => { + // `stage --to` is normally a directory that is not there yet, so the mount's own path may not exist + // either. The nearest existing ancestor has to be resolved on both sides or the spellings differ. + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-mount-missing-')); + const real = path.join(root, 'real'); + fs.mkdirSync(real, { recursive: true }); + const alias = path.join(root, 'alias'); + fs.symlinkSync(real, alias, 'junction'); + try { + const mounted = path.join(alias, 'Workspaces'); + assert.equal(insideMountedPath(path.join(real, 'Workspaces', 'new', 'deep'), alias), mounted); + assert.equal(insideMountedPath(path.join(real, 'Workspaces'), alias), mounted); + assert.equal( + insideMountedPath(path.join(real, 'Workspaces-evil'), alias), + null, + 'a sibling is not inside, with the mount missing too', + ); + } finally { + fs.rmSync(root, { recursive: true, force: true }); + } + }); }); describe('setup', () => { diff --git a/test/unit/stage.test.ts b/test/unit/stage.test.ts index ae0d3c2..3edc457 100644 --- a/test/unit/stage.test.ts +++ b/test/unit/stage.test.ts @@ -91,6 +91,23 @@ describe('stage', () => { ); }); + it('refuses a mount whose path is spelled through an alias', () => { + // The host spells $HOME /var/services/homes/u; the container sees the same directory as + // /volume1/homes/u. A lexical check let a target under the second name through, which staged the + // broker's code into a directory the agent can rewrite — the one thing this command exists to stop. + const repo = makeRepo('alias'); + const real = path.join(scratch, 'alias-real'); + fs.mkdirSync(path.join(real, 'Workspaces'), { recursive: true }); + const alias = path.join(scratch, 'alias-link'); + fs.symlinkSync(real, alias, 'junction'); + + assert.throws( + () => performStage({ repo, to: path.join(real, 'Workspaces', 'app'), ref: 'HEAD', home: alias }), + /mounted into the container/, + ); + assert.equal(fs.existsSync(path.join(real, 'Workspaces', 'app')), false, 'and nothing was written'); + }); + it('refuses to clear the deployment directory that holds the key', () => { const repo = makeRepo('deployment'); const to = path.join(scratch, 'deployment-dir'); @@ -120,4 +137,24 @@ describe('stage', () => { /overlap/, ); }); + + it('refuses a target that is a symlink into the repository', () => { + // The overlap check compares canonical paths for the same reason the mount check does: a `--to` that + // is a symlink to the repository *is* the repository, and the spellings differ. + const repo = makeRepo('overlap-link'); + + const link = path.join(scratch, 'link-to-repo'); + fs.symlinkSync(repo, link, 'junction'); + assert.throws( + () => performStage({ repo, to: link, ref: 'HEAD', home: scratch }), + /repository and the target overlap/, + ); + + const sub = path.join(scratch, 'link-into-repo'); + fs.symlinkSync(path.join(repo, 'src'), sub, 'junction'); + assert.throws( + () => performStage({ repo, to: sub, ref: 'HEAD', home: scratch }), + /repository and the target overlap/, + ); + }); }); From 850ca1d5a4aef39a4425cbd40d75be7b41dd3ae6 Mon Sep 17 00:00:00 2001 From: JounQin Date: Sat, 3 Oct 2026 19:13:20 +0000 Subject: [PATCH 2/2] ci: name the platform jobs after their platform MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .github/workflows/ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4ea910b..6799ba0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,7 +29,7 @@ jobs: # The job names share the `test on …` stem so that one search finds all three: the required status # checks in a ruleset are picked by name from a flat list, and names without a common stem mean one # search per check. Keep the stem when adding a job. - test: + linux: timeout-minutes: 15 name: test on Linux runs-on: ubuntu-latest @@ -60,7 +60,7 @@ jobs: # typecheck + build + unit + the end-to-end push against a local git-http-backend. - run: corepack yarn check - native-windows: + windows: timeout-minutes: 15 name: test on Windows (refused) runs-on: windows-latest