From 855fc3c23f6d8cb0460b4611dea7ba125e437edf Mon Sep 17 00:00:00 2001 From: JounQin Date: Sun, 4 Oct 2026 00:35:50 +0000 Subject: [PATCH 01/10] fix: the two containment checks that still compared spellings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `setup` joined `~/.dotfiles` lexically and refused a git config only when it was spelled the host's way. On this host the same directory is `/var/services/homes/u` and `/volume1/homes/u`, so the container's spelling was accepted and the host's own git config was written where the container can rewrite it. It uses `insideMountedPath` now — the general form of the rule, with one definition of the mounts instead of a second literal. `discoverCheckouts` compared a `realpathSync`'d git directory against the root as written, so a worktree (whose `.git` file records the real path) was skipped whenever the root was given by an alias, while the plain checkout beside it was served. Both sides are canonical now. `canonicalPath` moves to `src/paths.ts`: the opener is a core module, and importing it from `commands/support.ts` would pull the CLI's dependencies into it. --- src/commands/setup.ts | 17 +++++++++--- src/commands/stage.ts | 3 ++- src/commands/support.ts | 39 ++-------------------------- src/host-request.ts | 21 ++++++++++----- src/paths.ts | 47 ++++++++++++++++++++++++++++++++++ test/unit/commands.test.ts | 21 +++++++++++++++ test/unit/host-request.test.ts | 25 ++++++++++++++++++ 7 files changed, 124 insertions(+), 49 deletions(-) create mode 100644 src/paths.ts diff --git a/src/commands/setup.ts b/src/commands/setup.ts index c433aab..23c16ff 100644 --- a/src/commands/setup.ts +++ b/src/commands/setup.ts @@ -25,6 +25,7 @@ import { findSystemCaBundle, gitConfigList, gitConfigSet, + insideMountedPath, listFlag, parseArgs, say, @@ -51,6 +52,8 @@ export interface SetupInput { readonly rewriteSshHost: string | null; /** Report what would happen without writing anything. */ readonly dryRun: boolean; + /** Home whose mounts the config must not be written into; defaults to the real home. */ + readonly home?: string; /** Platform to answer for; injectable so the refusal is testable. */ readonly platform?: NodeJS.Platform; } @@ -107,10 +110,15 @@ export async function performSetup(input: SetupInput): Promise { const target = fs.realpathSync(input.gitconfig); throw new Error(`${input.gitconfig} is a symlink to ${target}; refusing to write through it`); } - const dotfiles = path.join(os.homedir(), '.dotfiles'); - const resolved = path.resolve(input.gitconfig); - if (resolved === dotfiles || resolved.startsWith(`${dotfiles}${path.sep}`)) { - throw new Error(`${input.gitconfig} resolves into ~/.dotfiles, which is the host's own configuration`); + // The general form of "~/.gitconfig is a symlink into ~/.dotfiles": the config must not be written into + // any directory the container can rewrite. Canonical, so the host's spelling of a mount and the + // container's spelling of the same directory are one place. + const mounted = insideMountedPath(input.gitconfig, input.home); + if (mounted) { + throw new Error( + `${input.gitconfig} is inside ${mounted}, which is mounted into the container; refusing to write ` + + "this environment's git config where the container can rewrite it", + ); } const warnings: string[] = []; @@ -228,6 +236,7 @@ export async function runSetup(argv: readonly string[]): Promise { caBundlePath: args.value('ca-bundle') ?? path.join(home, '.config', 'git-credential-broker', 'ca-bundle.pem'), rewriteSshHost: args.has('no-rewrite-ssh') ? null : (args.value('rewrite-ssh') ?? 'github.com'), dryRun: args.has('dry-run'), + home, }); say(`git config : ${result.gitconfig}${args.has('dry-run') ? ' (dry run)' : ''}`); diff --git a/src/commands/stage.ts b/src/commands/stage.ts index 9d0acb7..53177e6 100644 --- a/src/commands/stage.ts +++ b/src/commands/stage.ts @@ -21,7 +21,8 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; -import { canonicalPath, fail, insideMountedPath, parseArgs, say, warn, writeSecretFile } from './support.ts'; +import { canonicalPath } from '../paths.ts'; +import { 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'; diff --git a/src/commands/support.ts b/src/commands/support.ts index bfe17ae..f066b17 100644 --- a/src/commands/support.ts +++ b/src/commands/support.ts @@ -9,6 +9,8 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; +import { canonicalPath } from '../paths.ts'; + /** One parsed command line. */ export interface Args { /** Every value given for a flag, in order. */ @@ -111,43 +113,6 @@ 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. * diff --git a/src/host-request.ts b/src/host-request.ts index bfc0203..eb6d9a2 100644 --- a/src/host-request.ts +++ b/src/host-request.ts @@ -24,7 +24,9 @@ import { } from 'node:fs'; import { realpathSync } from 'node:fs'; import { lstatSync } from 'node:fs'; -import { dirname, join, resolve } from 'node:path'; +import { dirname, join, resolve, sep } from 'node:path'; + +import { canonicalPath } from './paths.ts'; /** The directory under the git directory that carries requests and answers. */ export const CHANNEL_PATH = 'git-credential-broker/pr-requests'; @@ -157,15 +159,20 @@ const SKIP = new Set(['.git', 'node_modules', '@eaDir', '#recycle', '$RECYCLE.BI */ export function discoverCheckouts(root: string, maxDepth = 4): { repo: string; dir: string }[] { const found: { repo: string; dir: string }[] = []; + // The root can be one name for a directory the filesystem knows by another, and a worktree records its + // git directory under the real one. Both sides are canonical, or the plain checkout is served and the + // worktree beside it is silently not. + const realRoot = canonicalPath(root).replace(/[/\\]+$/, ''); const walk = (current: string, depth: number): void => { const gitDir = gitDirAt(current); if (gitDir !== undefined) { - // A `gitdir:` pointer is written by whoever has the checkout, which is the container. Serving a channel - // at a path it names would let it choose where the opener creates files, and running `gh` there would let - // it choose which repository that checkout means. A worktree's pointer stays under the same root. - if (gitDir.startsWith(`${root.replace(/[/\\]+$/, '')}/`)) { - found.push({ repo: current, dir: channelDir(gitDir) }); - } + // A `gitdir:` pointer is written by whoever has the checkout, which is the container. Serving a + // channel at a path it names would let it choose where the opener creates files, and running `gh` + // there would let it choose which repository that checkout means. A worktree's pointer stays under + // the same root. + if (canonicalPath(gitDir).startsWith(`${realRoot}${sep}`)) { + found.push({ repo: current, dir: channelDir(gitDir) }); + } return; } if (depth >= maxDepth) return; diff --git a/src/paths.ts b/src/paths.ts new file mode 100644 index 0000000..f7cf422 --- /dev/null +++ b/src/paths.ts @@ -0,0 +1,47 @@ +/** + * Canonical paths, for the checks that compare one place against another. + * + * A directory can have more than one name — a symlinked `$HOME`, a mount the host spells one way and + * the container another — and a lexical prefix comparison calls those different places. The checks + * that keep the broker's code out of a container mount, and the opener inside the roots it serves, + * have to compare real paths rather than spellings. + */ +import fs from 'node:fs'; +import path from 'node:path'; + +/** + * 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 these 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; + } + } +} diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index cb592bb..96fcc96 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -225,6 +225,27 @@ describe('setup', () => { await performSetup(input); assert.equal(gitEntries(input.gitconfig).some((entry) => entry.includes('insteadof')), false); }); + + it('refuses a config inside a container mount, whatever the mount is spelled', async () => { + // The host spells $HOME /var/services/homes/u and the container spells the same directory + // /volume1/homes/u. The check this replaced joined ~/.dotfiles lexically, so the second spelling was + // accepted and the host's own git config would be written where the container can rewrite it. + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'setup-mount-alias-')); + const real = path.join(base, 'real'); + fs.mkdirSync(path.join(real, '.dotfiles'), { recursive: true }); + const alias = path.join(base, 'alias'); + fs.symlinkSync(real, alias, 'junction'); + try { + const gitconfig = path.join(real, '.dotfiles', 'gitconfig'); + await assert.rejects( + () => performSetup(setupInput({ gitconfig, home: alias, dryRun: true })), + /mounted into the container/, + ); + assert.equal(fs.existsSync(gitconfig), false, 'and nothing was written'); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); }); describe('init', () => { diff --git a/test/unit/host-request.test.ts b/test/unit/host-request.test.ts index ab3cd8b..a5b3904 100644 --- a/test/unit/host-request.test.ts +++ b/test/unit/host-request.test.ts @@ -208,6 +208,31 @@ describe('finding the checkouts to serve', () => { rmSync(root, { recursive: true, force: true }); }); + it('serves a worktree whose git directory is spelled through an alias of the root', () => { + // The root can be one name for a directory the filesystem knows by another ($HOME on the host this was + // found on), and a worktree records its git directory under the real one. Comparing the raw spellings + // served the plain checkout and silently skipped the worktree beside it. + const base = mkdtempSync(join(tmpdir(), 'discover-alias-')); + const real = join(base, 'real'); + mkdirSync(join(real, 'checkout', 'worktree'), { recursive: true }); + mkdirSync(join(real, 'gitdir'), { recursive: true }); + writeFileSync(join(real, 'gitdir', 'HEAD'), 'ref: refs/heads/main\n', 'utf8'); + writeFileSync(join(real, 'checkout', 'worktree', '.git'), `gitdir: ${join(real, 'gitdir')}\n`, 'utf8'); + mkdirSync(join(real, 'plain', '.git'), { recursive: true }); + writeFileSync(join(real, 'plain', '.git', 'HEAD'), 'ref: refs/heads/main\n', 'utf8'); + const alias = join(base, 'alias'); + symlinkSync(real, alias, 'junction'); + + try { + const repos = discoverCheckouts(alias, 4) + .map((found) => found.repo.slice(alias.length + 1)) + .sort(); + assert.deepEqual(repos, ['checkout/worktree', 'plain']); + } finally { + rmSync(base, { recursive: true, force: true }); + } + }); + it('returns a channel directory per checkout', () => { const root = mkdtempSync(join(tmpdir(), 'discover-test-')); mkdirSync(join(root, 'a', '.git'), { recursive: true }); From efba317b51a0c6077009269ebdfe89bb46bdfd3a Mon Sep 17 00:00:00 2001 From: JounQin Date: Sun, 4 Oct 2026 00:43:35 +0000 Subject: [PATCH 02/10] fix: the shared directories are the user's, not a list in this file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mountedPaths` returned `[Workspaces, .dsh, .dotfiles]` — three names that happen to be this deployment's, and `.dotfiles` in particular is only ever what the user called it, so the check was a guess dressed as a constant. The list now comes from `GIT_BROKER_MOUNTS` when it is set, which is how a host process is told: it cannot see the container. A process *inside* the container reads its own mount table (`/proc/self/mountinfo`) instead. When neither is available the check refuses rather than guessing, because the guess is what would put the broker's code, or the key, in a directory the agent can rewrite. `stage`, `init`, `setup` and `authorize` all go through it, and each takes an injectable list so its tests state their own rather than read the machine's. --- AGENTS.md | 7 +++ README.md | 11 +++++ src/commands/index.ts | 3 ++ src/commands/init.ts | 4 +- src/commands/setup.ts | 4 +- src/commands/stage.ts | 4 +- src/commands/support.ts | 99 ++++++++++++++++++++++++++++++++++---- test/unit/commands.test.ts | 72 ++++++++++++++++++++------- test/unit/stage.test.ts | 39 ++++++++++----- 9 files changed, 202 insertions(+), 41 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c5326d1..5768629 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,6 +81,13 @@ sources must stay erasable (`erasableSyntaxOnly` is on: no enums, no parameter p container can create one. Compare real paths (`realpathSync`), and ask what else can make a path point elsewhere: the first fix for this was a prefix check, and the bypass was found by a review bot asking the question the fix had not. + + **The other half is what it compares against.** The directories shared with the container are the + user's to name — `~/.dotfiles` is only ever what that user called it — so there is no list to hard-code. + `GIT_BROKER_MOUNTS` declares them for a host process, which cannot see the container; a process inside + the container reads its own mount table (`/proc/self/mountinfo`) instead. When neither is available the + check refuses rather than guesses, because the guess is what would put the code, or the key, somewhere + the agent can rewrite. - **The process that owns a boundary decides what crosses it, not the error class.** A response body put into the message of a `ProviderConfigError` reached the container, because a caller that wraps that error to name a missing permission wraps it in the same class and so passes the whole message through. Detail diff --git a/README.md b/README.md index 95ab39e..3be5dca 100644 --- a/README.md +++ b/README.md @@ -105,6 +105,11 @@ read-only mount, so nothing is fetched or built at boot. DEPLOY=/srv/git-cred-broker # broker.config.json, app.pem and log/ live here SOCKET_DIR=~/.dsh/git-broker # a host directory the pushing container already mounts +# The directories the agent container can see. The host commands that write — init, stage, authorize — +# refuse without this rather than guess: what the container mounts is the user's to name. Inside the +# container, `setup` reads the container's own mount table and needs no variable. +export GIT_BROKER_MOUNTS="$HOME/Workspaces:$HOME/.dsh:$HOME/.dotfiles" + git-credential-broker init --mode sidecar --dir "$DEPLOY" \ --cert ~/Downloads/app.private-key.pem \ --allow owner/repo --client-id Iv23li… --app-id 123456 @@ -115,6 +120,12 @@ git-credential-broker compose --code "$DEPLOY/app" --socket-dir "$SOCKET_DIR" \ docker compose -f "$DEPLOY/docker-compose.broker.yml" up -d ``` +`GIT_BROKER_MOUNTS` is the one piece of deployment knowledge the host needs: which directories the +agent container shares with it. Name them as your deployment mounts them — `~/.dotfiles` above is +whatever you called it, and a deployment that mounts none should say so with an empty value rather +than leave the variable out. The container side discovers its own mounts, so a `setup` inside it +never needs this. + `stage` prints the commit it exported; check that sha against your own clone or the remote before trusting it, since the repository you export from is one an agent can write to. It leaves a `STAGED.json` beside the code recording where it came from. diff --git a/src/commands/index.ts b/src/commands/index.ts index e8a46e6..a4a36ce 100644 --- a/src/commands/index.ts +++ b/src/commands/index.ts @@ -41,6 +41,9 @@ Environment GIT_BROKER_SOCKET Broker socket path GIT_BROKER_REQUIRE=1 Fail closed instead of behaving as if not installed when unconfigured GIT_BROKER_SOCKET_FILE Where to read the socket path from when no variable is set + GIT_BROKER_MOUNTS Directories the agent container shares with this host, ":"-separated; the + commands that write refuse without it on the host (in the container the + mount table is read instead) Run \`git-credential-broker --help\` for the options of one command.`; diff --git a/src/commands/init.ts b/src/commands/init.ts index ba3c45a..14770b8 100644 --- a/src/commands/init.ts +++ b/src/commands/init.ts @@ -42,6 +42,8 @@ export interface InitEnvironment { readonly isInsideContainer?: () => boolean; /** The home directory whose mounts must not contain the key. */ readonly home?: string; + /** The directories the container shares, when the caller already has them. */ + readonly mounts?: readonly string[]; } /** Everything `init` needs, resolved. */ @@ -235,7 +237,7 @@ export function performInit(input: InitInput): InitResult { } const home = input.env?.home ?? (process.env['HOME'] ?? ''); - const mounted = insideMountedPath(input.dir, home); + const mounted = insideMountedPath(input.dir, home, input.env?.mounts); if (mounted) { throw new Error( `${input.dir} is inside ${mounted}, which is mounted into the container; the private key must live outside every mount`, diff --git a/src/commands/setup.ts b/src/commands/setup.ts index 23c16ff..0d5c37d 100644 --- a/src/commands/setup.ts +++ b/src/commands/setup.ts @@ -54,6 +54,8 @@ export interface SetupInput { readonly dryRun: boolean; /** Home whose mounts the config must not be written into; defaults to the real home. */ readonly home?: string; + /** The directories the container shares, when the caller already has them. */ + readonly mounts?: readonly string[]; /** Platform to answer for; injectable so the refusal is testable. */ readonly platform?: NodeJS.Platform; } @@ -113,7 +115,7 @@ export async function performSetup(input: SetupInput): Promise { // The general form of "~/.gitconfig is a symlink into ~/.dotfiles": the config must not be written into // any directory the container can rewrite. Canonical, so the host's spelling of a mount and the // container's spelling of the same directory are one place. - const mounted = insideMountedPath(input.gitconfig, input.home); + const mounted = insideMountedPath(input.gitconfig, input.home, input.mounts); if (mounted) { throw new Error( `${input.gitconfig} is inside ${mounted}, which is mounted into the container; refusing to write ` + diff --git a/src/commands/stage.ts b/src/commands/stage.ts index 53177e6..1715370 100644 --- a/src/commands/stage.ts +++ b/src/commands/stage.ts @@ -37,6 +37,8 @@ export interface StageInput { readonly ref: string; /** The home whose mounts the target must not be inside. */ readonly home?: string; + /** The directories the container shares, when the caller already has them. */ + readonly mounts?: readonly string[]; /** Replace a target that this command did not stage itself. */ readonly force?: boolean; } @@ -63,7 +65,7 @@ export function performStage(input: StageInput): StageResult { const to = path.resolve(input.to); const home = input.home ?? process.env['HOME'] ?? ''; - const mounted = insideMountedPath(to, home); + const mounted = insideMountedPath(to, home, input.mounts); if (mounted) { throw new Error( `${to} is inside ${mounted}, which is mounted into the container. The broker runs with the ` + diff --git a/src/commands/support.ts b/src/commands/support.ts index f066b17..1ce620e 100644 --- a/src/commands/support.ts +++ b/src/commands/support.ts @@ -113,20 +113,96 @@ export function isInsideContainer(): boolean { return fs.existsSync('/home/app/Workspaces') && !fs.existsSync('/volume1'); } +/** The environment variable naming the directories the container shares with the host. */ +export const MOUNTS_VAR = 'GIT_BROKER_MOUNTS'; + +/** Where the shared directories came from, when a caller supplies them instead of the environment. */ +export interface MountSource { + /** The raw value of {@link MOUNTS_VAR}. */ + readonly declared?: string; + /** The contents of `/proc/self/mountinfo`. */ + readonly mountinfo?: string; + /** Whether this process is in the container, whose own mount table is the authority. */ + readonly container?: boolean; +} + /** - * The container's mounts under the user's home, as seen from the host. + * Decode one field of `/proc/self/mountinfo`. * - * A private key must never live under one of these. + * The kernel escapes space, tab, newline and backslash as octal, so a mount point containing a space + * arrives as `\040` instead of splitting the line. * - * @param home - the home directory to resolve against. - * @returns the absolute paths. + * @param value - the raw field. + * @returns the decoded path. */ -export function mountedPaths(home: string = os.homedir()): string[] { - return [path.join(home, 'Workspaces'), path.join(home, '.dsh'), path.join(home, '.dotfiles')]; +function decodeMountField(value: string): string { + return value.replace(/\\([0-7]{3})/g, (_whole, octal: string) => String.fromCharCode(Number.parseInt(octal, 8))); +} + +/** + * The mount points at or under a home, read from a mount table. + * + * The container is the thing that knows what it mounts. A list of directory names in this file would + * be a guess about someone else's home — `.dotfiles` is only ever what the user called it. + * + * @param mountinfo - the contents of `/proc/self/mountinfo`. + * @param home - the home to measure against. + * @returns the mount points, spelled as the table spells them. + */ +export function mountsUnderHome(mountinfo: string, home: string): string[] { + const root = canonicalPath(home); + const found: string[] = []; + for (const line of mountinfo.split('\n')) { + const field = line.split(' ')[4]; + if (field === undefined) continue; + const point = path.resolve(decodeMountField(field)); + const real = canonicalPath(point); + if (real === root || real.startsWith(`${root}${path.sep}`)) found.push(point); + } + return found; +} + +/** + * The directories the container shares with the host. + * + * `GIT_BROKER_MOUNTS` wins when it is set, which is how a host process is told: it cannot see the + * container's mount table. Inside the container the table is read instead. When neither is available + * this refuses rather than guessing, because the guess is what would put the broker's code, or the + * private key, in a directory the agent can rewrite. + * + * @param home - the home whose mounts are wanted. + * @param source - a declared value or mount table, for a test. + * @returns the shared directories. + * @throws {Error} when neither source is available. + */ +export function mountedPaths(home: string = os.homedir(), source: MountSource = {}): string[] { + const declared = source.declared ?? process.env[MOUNTS_VAR]; + if (declared !== undefined && declared.trim() !== '') { + return declared + .split(path.delimiter) + .map((entry) => entry.trim()) + .filter((entry) => entry !== ''); + } + const container = source.container ?? fs.existsSync('/.dockerenv'); + let mountinfo = source.mountinfo; + if (mountinfo === undefined && container) { + try { + mountinfo = fs.readFileSync('/proc/self/mountinfo', 'utf8'); + } catch { + mountinfo = undefined; + } + } + if (mountinfo === undefined) { + throw new Error( + `cannot tell which directories the container shares: set ${MOUNTS_VAR} to them, separated by ` + + `"${path.delimiter}" (a process in the container reads its own mount table; this one cannot)`, + ); + } + return mountsUnderHome(mountinfo, home); } /** - * Whether a path is inside one of the container's mounts. + * Whether a path is inside one of the directories the container shares with the host. * * 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 @@ -134,11 +210,16 @@ export function mountedPaths(home: string = os.homedir()): string[] { * * @param target - the path to check. * @param home - the home directory to resolve against. + * @param mounts - the shared directories, when the caller already has them. * @returns the mount it is inside, or null. */ -export function insideMountedPath(target: string, home: string = os.homedir()): string | null { +export function insideMountedPath( + target: string, + home: string = os.homedir(), + mounts?: readonly string[], +): string | null { const resolved = canonicalPath(target); - for (const mounted of mountedPaths(home)) { + for (const mounted of mounts ?? mountedPaths(home)) { const real = canonicalPath(mounted); if (resolved === real || resolved.startsWith(`${real}${path.sep}`)) return mounted; } diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index 96fcc96..33ee0b9 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -97,13 +97,37 @@ describe('parseArgs', () => { }); describe('mounted path detection', () => { - it('knows what the container can see', () => { - const home = '/home/app'; - assert.deepEqual(mountedPaths(home), ['/home/app/Workspaces', '/home/app/.dsh', '/home/app/.dotfiles']); - assert.equal(insideMountedPath('/home/app/Workspaces/x', home), '/home/app/Workspaces'); - assert.equal(insideMountedPath('/home/app/.dsh/git-broker', home), '/home/app/.dsh'); - 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'); + /** A home and a shared-directory list stated here, so no case reads the machine's own mount table. */ + const home = '/home/app'; + const mounts = ['/home/app/Workspaces', '/home/app/.dsh', '/home/app/.dotfiles']; + + it('is inside a shared directory, and not a sibling that merely starts the same', () => { + assert.equal(insideMountedPath('/home/app/Workspaces/x', home, mounts), '/home/app/Workspaces'); + assert.equal(insideMountedPath('/home/app/.dsh/git-broker', home, mounts), '/home/app/.dsh'); + assert.equal(insideMountedPath('/volume1/docker/git-cred-broker', home, mounts), null); + assert.equal(insideMountedPath('/home/app/Workspaces-evil', home, mounts), null, 'must not prefix-match a sibling'); + }); + + it('takes the shared directories from GIT_BROKER_MOUNTS when it is set', () => { + const declared = ['/srv/a', '/srv/b'].join(path.delimiter); + assert.deepEqual(mountedPaths(home, { declared }), ['/srv/a', '/srv/b']); + assert.deepEqual(mountedPaths(home, { declared: ` ${['/srv/a'].join(path.delimiter)} ` }), ['/srv/a']); + }); + + it('reads them from the container mount table when nothing is declared', () => { + const table = [ + '675 573 0:36 /x /home/app/Workspaces rw - btrfs /dev/x rw', + '676 573 0:36 /y /home/app/with\\040space rw - btrfs /dev/x rw', + '677 573 0:36 /z /somewhere/else rw - btrfs /dev/x rw', + ].join('\n'); + assert.deepEqual(mountedPaths(home, { declared: '', mountinfo: table, container: true }), [ + '/home/app/Workspaces', + '/home/app/with space', + ]); + }); + + it('refuses rather than guess when neither the declaration nor a mount table is available', () => { + assert.throws(() => mountedPaths(home, { declared: '', container: false }), /GIT_BROKER_MOUNTS/); }); it('resolves the aliases of one directory instead of trusting the spelling', () => { @@ -119,11 +143,16 @@ describe('mounted path detection', () => { 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'); + const shared = [mounted]; + assert.equal( + insideMountedPath(path.join(real, 'Workspaces', 'x'), alias, shared), + mounted, + 'real target, aliased mount', + ); + assert.equal(insideMountedPath(path.join(real, 'Workspaces'), alias, shared), mounted, 'and the mount itself'); + assert.equal(insideMountedPath(path.join(real, 'elsewhere'), alias, shared), null, 'a sibling is still outside'); assert.equal( - insideMountedPath(path.join(alias, 'Workspaces', 'y'), real), + insideMountedPath(path.join(alias, 'Workspaces', 'y'), real, [path.join(real, 'Workspaces')]), path.join(real, 'Workspaces'), 'and the reverse spelling', ); @@ -142,10 +171,11 @@ describe('mounted path detection', () => { 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); + const shared = [mounted]; + assert.equal(insideMountedPath(path.join(real, 'Workspaces', 'new', 'deep'), alias, shared), mounted); + assert.equal(insideMountedPath(path.join(real, 'Workspaces'), alias, shared), mounted); assert.equal( - insideMountedPath(path.join(real, 'Workspaces-evil'), alias), + insideMountedPath(path.join(real, 'Workspaces-evil'), alias, shared), null, 'a sibling is not inside, with the mount missing too', ); @@ -173,6 +203,8 @@ describe('setup', () => { caBundlePath: path.join(base, '.config', 'git-credential-broker', 'ca-bundle.pem'), rewriteSshHost: 'github.com', dryRun: false, + // Stated, so the shared-directory resolution does not read this machine's mount table. + mounts: [], ...overrides, }; } @@ -238,7 +270,7 @@ describe('setup', () => { try { const gitconfig = path.join(real, '.dotfiles', 'gitconfig'); await assert.rejects( - () => performSetup(setupInput({ gitconfig, home: alias, dryRun: true })), + () => performSetup(setupInput({ gitconfig, home: alias, mounts: [path.join(alias, '.dotfiles')], dryRun: true })), /mounted into the container/, ); assert.equal(fs.existsSync(gitconfig), false, 'and nothing was written'); @@ -260,7 +292,7 @@ describe('init', () => { force: false, // This suite runs inside the very container init refuses to run in, so the detected // environment is injected. The guard itself is exercised by the tests below. - env: { isInsideContainer: () => false, home: fakeHome }, + env: { isInsideContainer: () => false, home: fakeHome, mounts: [] }, }; it('installs the key 0600 and writes a config the daemon accepts', () => { @@ -339,7 +371,13 @@ describe('init', () => { const cert = writeKey(path.join(root, 'downloaded.pem')); const mounted = path.join(fakeHome, 'Workspaces', 'somewhere'); assert.throws( - () => performInit({ ...baseInput, cert, dir: mounted }), + () => + performInit({ + ...baseInput, + cert, + dir: mounted, + env: { ...baseInput.env, mounts: [path.join(fakeHome, 'Workspaces')] }, + }), /mounted into the container/, 'the private key must never live where the container can read it', ); diff --git a/test/unit/stage.test.ts b/test/unit/stage.test.ts index 3edc457..7585cef 100644 --- a/test/unit/stage.test.ts +++ b/test/unit/stage.test.ts @@ -37,6 +37,14 @@ function makeRepo(name: string): string { } describe('stage', () => { + /** + * `performStage` with the shared directories supplied. + * + * Resolving them for real refuses on a host with no `GIT_BROKER_MOUNTS`, which is the point in + * production and noise here: each case states its own list, and the mount cases pass theirs. + */ + const stage = (input: Parameters[0]) => performStage({ mounts: [], ...input }); + it('exports the commit, records where it came from, and ignores the working tree', () => { const repo = makeRepo('clean'); const to = path.join(scratch, 'staged/clean'); @@ -44,7 +52,7 @@ describe('stage', () => { // An uncommitted edit that must NOT reach the staged tree. fs.writeFileSync(path.join(repo, 'src', 'daemon.ts'), 'export const version = 999;\n'); - const result = performStage({ repo, to, ref: 'HEAD', home: scratch }); + const result = stage({ repo, to, ref: 'HEAD', home: scratch }); assert.equal(result.dirtyWorkingTree, true, 'the dirty tree is reported'); assert.equal(result.files > 0, true); @@ -58,13 +66,13 @@ describe('stage', () => { const repo = makeRepo('restage'); const to = path.join(scratch, 'staged/restage'); - performStage({ repo, to, ref: 'HEAD', home: scratch }); + stage({ repo, to, ref: 'HEAD', home: scratch }); fs.writeFileSync(path.join(repo, 'src', 'old.ts'), 'export const gone = true;\n'); execFileSync('git', ['add', '-A'], { cwd: repo }); execFileSync('git', ['-c', 'user.name=t', '-c', 'user.email=t@example.com', 'commit', '-q', '-m', 'second'], { cwd: repo, }); - performStage({ repo, to, ref: 'HEAD', home: scratch }); + stage({ repo, to, ref: 'HEAD', home: scratch }); assert.equal(fs.existsSync(path.join(to, 'src', 'old.ts')), true, 'the new commit added it'); @@ -74,7 +82,7 @@ describe('stage', () => { execFileSync('git', ['-c', 'user.name=t', '-c', 'user.email=t@example.com', 'commit', '-q', '-m', 'third'], { cwd: repo, }); - performStage({ repo, to, ref: 'HEAD', home: scratch }); + stage({ repo, to, ref: 'HEAD', home: scratch }); assert.equal(fs.existsSync(path.join(to, 'src', 'old.ts')), false, 'no stale file survives a restage'); }); @@ -85,7 +93,7 @@ describe('stage', () => { fs.mkdirSync(inside, { recursive: true }); assert.throws( - () => performStage({ repo, to: inside, ref: 'HEAD', home }), + () => stage({ repo, to: inside, ref: 'HEAD', home, mounts: [path.join(home, 'Workspaces')] }), /mounted into the container/, 'code the agent can rewrite must never be staged where it will run with the key', ); @@ -102,7 +110,14 @@ describe('stage', () => { fs.symlinkSync(real, alias, 'junction'); assert.throws( - () => performStage({ repo, to: path.join(real, 'Workspaces', 'app'), ref: 'HEAD', home: alias }), + () => + stage({ + repo, + to: path.join(real, 'Workspaces', 'app'), + ref: 'HEAD', + home: alias, + mounts: [path.join(alias, 'Workspaces')], + }), /mounted into the container/, ); assert.equal(fs.existsSync(path.join(real, 'Workspaces', 'app')), false, 'and nothing was written'); @@ -115,7 +130,7 @@ describe('stage', () => { fs.writeFileSync(path.join(to, 'app.pem'), 'the key\n'); fs.writeFileSync(path.join(to, 'broker.config.json'), '{}\n'); - assert.throws(() => performStage({ repo, to, ref: 'HEAD', home: scratch, force: true }), /deployment directory/); + assert.throws(() => stage({ repo, to, ref: 'HEAD', home: scratch, force: true }), /deployment directory/); assert.equal(fs.readFileSync(path.join(to, 'app.pem'), 'utf8'), 'the key\n', 'the key is untouched'); }); @@ -125,15 +140,15 @@ describe('stage', () => { fs.mkdirSync(to, { recursive: true }); fs.writeFileSync(path.join(to, 'something.txt'), 'mine\n'); - assert.throws(() => performStage({ repo, to, ref: 'HEAD', home: scratch }), /not ours to clear/); + assert.throws(() => stage({ repo, to, ref: 'HEAD', home: scratch }), /not ours to clear/); assert.equal(fs.existsSync(path.join(to, 'something.txt')), true, 'nothing was removed'); }); it('refuses a target that overlaps the repository', () => { const repo = makeRepo('overlap'); - assert.throws(() => performStage({ repo, to: repo, ref: 'HEAD', home: scratch }), /overlap/); + assert.throws(() => stage({ repo, to: repo, ref: 'HEAD', home: scratch }), /overlap/); assert.throws( - () => performStage({ repo, to: path.join(repo, 'dist'), ref: 'HEAD', home: scratch }), + () => stage({ repo, to: path.join(repo, 'dist'), ref: 'HEAD', home: scratch }), /overlap/, ); }); @@ -146,14 +161,14 @@ describe('stage', () => { const link = path.join(scratch, 'link-to-repo'); fs.symlinkSync(repo, link, 'junction'); assert.throws( - () => performStage({ repo, to: link, ref: 'HEAD', home: scratch }), + () => stage({ 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 }), + () => stage({ repo, to: sub, ref: 'HEAD', home: scratch }), /repository and the target overlap/, ); }); From b401b025951689f77163d60116733f03385ec9ce Mon Sep 17 00:00:00 2001 From: JounQin Date: Sun, 4 Oct 2026 00:48:07 +0000 Subject: [PATCH 03/10] fix: ask the container runtime on the host, not the operator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit made a host process take the shared directories from `GIT_BROKER_MOUNTS` and refuse without it. It does not have to be told: the host's own mount table cannot show a bind mount, but the container runtime can, so `docker ps` + `docker inspect` are asked for the host paths its containers bind-mount. `GIT_BROKER_MOUNTS` stays as the override, and only when nothing can answer — no runtime, no declaration — does the check refuse. It is deliberately a superset: every running container counts, not only the agent, because any container that can rewrite the broker's code is as good a reason not to stage there. One thing measurement caught: a candidate whose real path cannot be read (a Synology document-viewer mount answers EACCES) must not take the whole list down, so candidate paths fall back to their lexical form while the path being guarded stays strict. --- AGENTS.md | 9 +-- README.md | 20 +++--- src/commands/index.ts | 6 +- src/commands/support.ts | 125 ++++++++++++++++++++++++++++++++----- test/unit/commands.test.ts | 35 ++++++++++- 5 files changed, 161 insertions(+), 34 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5768629..263bb4b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -84,10 +84,11 @@ sources must stay erasable (`erasableSyntaxOnly` is on: no enums, no parameter p **The other half is what it compares against.** The directories shared with the container are the user's to name — `~/.dotfiles` is only ever what that user called it — so there is no list to hard-code. - `GIT_BROKER_MOUNTS` declares them for a host process, which cannot see the container; a process inside - the container reads its own mount table (`/proc/self/mountinfo`) instead. When neither is available the - check refuses rather than guesses, because the guess is what would put the code, or the key, somewhere - the agent can rewrite. + A process inside the container reads its own mount table (`/proc/self/mountinfo`); a host process asks + the container runtime (`docker inspect`) for the paths its containers bind-mount, which is a superset on + purpose: any container that can rewrite the broker's code is as good a reason not to stage there as the + agent is. `GIT_BROKER_MOUNTS` overrides both. When none can answer, the check refuses rather than + guesses — the guess is what would put the code, or the key, somewhere a container can rewrite. - **The process that owns a boundary decides what crosses it, not the error class.** A response body put into the message of a `ProviderConfigError` reached the container, because a caller that wraps that error to name a missing permission wraps it in the same class and so passes the whole message through. Detail diff --git a/README.md b/README.md index 3be5dca..5fc02f4 100644 --- a/README.md +++ b/README.md @@ -105,10 +105,10 @@ read-only mount, so nothing is fetched or built at boot. DEPLOY=/srv/git-cred-broker # broker.config.json, app.pem and log/ live here SOCKET_DIR=~/.dsh/git-broker # a host directory the pushing container already mounts -# The directories the agent container can see. The host commands that write — init, stage, authorize — -# refuse without this rather than guess: what the container mounts is the user's to name. Inside the -# container, `setup` reads the container's own mount table and needs no variable. -export GIT_BROKER_MOUNTS="$HOME/Workspaces:$HOME/.dsh:$HOME/.dotfiles" +# Optional: the directories the agent container shares with this host. Left unset, a host process asks +# the container runtime instead, and a process inside the container reads its own mount table. Set it +# only to override that — a host with no runtime to ask, or to narrow what counts as shared. +# export GIT_BROKER_MOUNTS="$HOME/Workspaces:$HOME/.dsh:$HOME/.dotfiles" git-credential-broker init --mode sidecar --dir "$DEPLOY" \ --cert ~/Downloads/app.private-key.pem \ @@ -120,11 +120,13 @@ git-credential-broker compose --code "$DEPLOY/app" --socket-dir "$SOCKET_DIR" \ docker compose -f "$DEPLOY/docker-compose.broker.yml" up -d ``` -`GIT_BROKER_MOUNTS` is the one piece of deployment knowledge the host needs: which directories the -agent container shares with it. Name them as your deployment mounts them — `~/.dotfiles` above is -whatever you called it, and a deployment that mounts none should say so with an empty value rather -than leave the variable out. The container side discovers its own mounts, so a `setup` inside it -never needs this. +Nothing above has to name the shared directories. A host process asks the container runtime for the +host paths its containers bind-mount; a process inside the container reads `/proc/self/mountinfo`. +`GIT_BROKER_MOUNTS` overrides both, for a host with no runtime to ask or to narrow what counts as +shared. When none of the three can answer, `init`, `stage` and `authorize` refuse rather than guess. +The runtime route is deliberately a superset: a directory bind-mounted into *any* running container +counts as shared, because any container that can rewrite the broker's code is as good a reason not to +stage there as the agent is. `stage` prints the commit it exported; check that sha against your own clone or the remote before trusting it, since the repository you export from is one an agent can write to. It leaves a diff --git a/src/commands/index.ts b/src/commands/index.ts index a4a36ce..e058c3b 100644 --- a/src/commands/index.ts +++ b/src/commands/index.ts @@ -41,9 +41,9 @@ Environment GIT_BROKER_SOCKET Broker socket path GIT_BROKER_REQUIRE=1 Fail closed instead of behaving as if not installed when unconfigured GIT_BROKER_SOCKET_FILE Where to read the socket path from when no variable is set - GIT_BROKER_MOUNTS Directories the agent container shares with this host, ":"-separated; the - commands that write refuse without it on the host (in the container the - mount table is read instead) + GIT_BROKER_MOUNTS Override the directories the agent container shares with this host, + ":"-separated; otherwise the container runtime (on the host) or the mount + table (in the container) is asked Run \`git-credential-broker --help\` for the options of one command.`; diff --git a/src/commands/support.ts b/src/commands/support.ts index 1ce620e..ef7c1f3 100644 --- a/src/commands/support.ts +++ b/src/commands/support.ts @@ -122,10 +122,15 @@ export interface MountSource { readonly declared?: string; /** The contents of `/proc/self/mountinfo`. */ readonly mountinfo?: string; + /** `Mounts` arrays as the container runtime prints them; null when it could not be asked. */ + readonly inspect?: string | null; /** Whether this process is in the container, whose own mount table is the authority. */ readonly container?: boolean; } +/** The command that reports what running containers mount. */ +const CONTAINER_CLI = 'docker'; + /** * Decode one field of `/proc/self/mountinfo`. * @@ -139,6 +144,26 @@ function decodeMountField(value: string): string { return value.replace(/\\([0-7]{3})/g, (_whole, octal: string) => String.fromCharCode(Number.parseInt(octal, 8))); } +/** + * `canonicalPath`, but a path that cannot be resolved is answered lexically. + * + * For a *candidate* in a list this is the safe direction. A host runs containers whose mounts this + * user cannot read — measured: a Synology document-viewer mount answers EACCES — and one of those + * must not stop the whole list being read. A candidate that is lexically under the home is still + * treated as shared, which over-refuses rather than under-refuses. The strict form stays for the path + * being guarded, where an answer that cannot be known is a refusal. + * + * @param target - the path to resolve. + * @returns the real path, or the resolved path when it cannot be read. + */ +function canonicalOrResolved(target: string): string { + try { + return canonicalPath(target); + } catch { + return path.resolve(target); + } +} + /** * The mount points at or under a home, read from a mount table. * @@ -150,30 +175,90 @@ function decodeMountField(value: string): string { * @returns the mount points, spelled as the table spells them. */ export function mountsUnderHome(mountinfo: string, home: string): string[] { - const root = canonicalPath(home); + const root = canonicalOrResolved(home); const found: string[] = []; for (const line of mountinfo.split('\n')) { const field = line.split(' ')[4]; if (field === undefined) continue; const point = path.resolve(decodeMountField(field)); - const real = canonicalPath(point); + const real = canonicalOrResolved(point); if (real === root || real.startsWith(`${root}${path.sep}`)) found.push(point); } return found; } +/** + * The host paths a container runtime bound from the user's home. + * + * `docker inspect --format '{{json .Mounts}}'` prints one array per container. Only bind mounts count: + * a volume or a tmpfs is not a directory the user named. Every container is considered, not only the + * agent's — any container that can rewrite a directory is a reason not to put the broker's code there + * — so this is deliberately a superset. + * + * @param output - the runtime's output, one array per line. + * @param home - the home to keep mounts under. + * @returns the host paths, unique and canonical. + */ +export function boundPathsUnderHome(output: string, home: string): string[] { + const root = canonicalOrResolved(home); + const found = new Set(); + for (const line of output.split('\n')) { + if (line.trim() === '') continue; + let mounts: unknown; + try { + mounts = JSON.parse(line); + } catch { + continue; + } + if (!Array.isArray(mounts)) continue; + for (const mount of mounts as Array<{ Type?: unknown; Source?: unknown }>) { + if (mount.Type !== 'bind' || typeof mount.Source !== 'string') continue; + const real = canonicalOrResolved(mount.Source); + if (real === root || real.startsWith(`${root}${path.sep}`)) found.add(real); + } + } + return [...found]; +} + +/** + * Ask the container runtime which directories it has bound from the user's home. + * + * The host's own mount table cannot answer this: a bind mount exists only in the container's + * namespace, so the runtime is asked instead. + * + * @param home - the home to keep mounts under. + * @returns the paths, or undefined when the runtime is not there to ask. + */ +function detectedMounts(home: string): string[] | undefined { + const run = (args: readonly string[]): string => + execFileSync(CONTAINER_CLI, args, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }); + let ids: string; + try { + ids = run(['ps', '--quiet']); + } catch { + return undefined; + } + const running = ids.split('\n').map((id) => id.trim()).filter((id) => id !== ''); + if (running.length === 0) return []; + try { + return boundPathsUnderHome(run(['inspect', '--format', '{{json .Mounts}}', ...running]), home); + } catch { + return undefined; + } +} + /** * The directories the container shares with the host. * - * `GIT_BROKER_MOUNTS` wins when it is set, which is how a host process is told: it cannot see the - * container's mount table. Inside the container the table is read instead. When neither is available - * this refuses rather than guessing, because the guess is what would put the broker's code, or the - * private key, in a directory the agent can rewrite. + * In order: `GIT_BROKER_MOUNTS`, which is how an operator overrides everything; the container's own + * mount table, when this process is in the container; the container runtime, when it is not. Only + * when none of the three can answer does this refuse — the guess it will not make is what would put + * the broker's code, or the private key, in a directory a container can rewrite. * * @param home - the home whose mounts are wanted. - * @param source - a declared value or mount table, for a test. + * @param source - a declared value, mount table or runtime output, for a test. * @returns the shared directories. - * @throws {Error} when neither source is available. + * @throws {Error} when nothing can answer. */ export function mountedPaths(home: string = os.homedir(), source: MountSource = {}): string[] { const declared = source.declared ?? process.env[MOUNTS_VAR]; @@ -183,22 +268,30 @@ export function mountedPaths(home: string = os.homedir(), source: MountSource = .map((entry) => entry.trim()) .filter((entry) => entry !== ''); } - const container = source.container ?? fs.existsSync('/.dockerenv'); - let mountinfo = source.mountinfo; - if (mountinfo === undefined && container) { + + let mounts: string[] | undefined; + if (source.mountinfo !== undefined) { + mounts = mountsUnderHome(source.mountinfo, home); + } else if (source.inspect !== undefined) { + mounts = source.inspect === null ? undefined : boundPathsUnderHome(source.inspect, home); + } else if (source.container ?? fs.existsSync('/.dockerenv')) { try { - mountinfo = fs.readFileSync('/proc/self/mountinfo', 'utf8'); + mounts = mountsUnderHome(fs.readFileSync('/proc/self/mountinfo', 'utf8'), home); } catch { - mountinfo = undefined; + mounts = undefined; } + } else { + mounts = detectedMounts(home); } - if (mountinfo === undefined) { + + if (mounts === undefined) { throw new Error( `cannot tell which directories the container shares: set ${MOUNTS_VAR} to them, separated by ` + - `"${path.delimiter}" (a process in the container reads its own mount table; this one cannot)`, + `"${path.delimiter}" (inside the container its mount table is read; on the host the container ` + + 'runtime is asked, and it did not answer)', ); } - return mountsUnderHome(mountinfo, home); + return mounts; } /** diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index 33ee0b9..a8d2bda 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -126,8 +126,39 @@ describe('mounted path detection', () => { ]); }); - it('refuses rather than guess when neither the declaration nor a mount table is available', () => { - assert.throws(() => mountedPaths(home, { declared: '', container: false }), /GIT_BROKER_MOUNTS/); + it('asks the container runtime on the host, and keeps only bind mounts under the home', () => { + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-inspect-')); + const inside = path.join(base, 'Workspaces'); + const deeper = path.join(base, 'sub', 'deep'); + fs.mkdirSync(inside, { recursive: true }); + fs.mkdirSync(deeper, { recursive: true }); + const inspect = [ + JSON.stringify([ + { Type: 'bind', Source: inside, Destination: '/home/app/Workspaces' }, + { Type: 'bind', Source: deeper, Destination: '/home/app/.dsh' }, + { Type: 'volume', Source: path.join(base, 'volume'), Destination: '/vol' }, + { Type: 'bind', Source: '/srv/elsewhere', Destination: '/srv/elsewhere' }, + ]), + ].join('\n'); + try { + const got = mountedPaths(base, { declared: '', container: false, inspect }) + .map((entry) => fs.realpathSync(entry)) + .sort(); + assert.deepEqual( + got, + [inside, deeper].map((entry) => fs.realpathSync(entry)).sort(), + 'a volume and anything outside the home are not shared directories', + ); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); + + it('refuses rather than guess when nothing can answer', () => { + assert.throws( + () => mountedPaths(home, { declared: '', container: false, inspect: null }), + /GIT_BROKER_MOUNTS/, + ); }); it('resolves the aliases of one directory instead of trusting the spelling', () => { From d2392f8f8584b60326b396bad7a3a124e998544d Mon Sep 17 00:00:00 2001 From: JounQin Date: Sun, 4 Oct 2026 00:59:43 +0000 Subject: [PATCH 04/10] fix: the tool has no opinion about anyone's home MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mechanism was generic, but the code, comments, docs and fixtures still talked about `.dotfiles` and about `~/.gitconfig` resolving into it — one deployment's arrangement, generalized into the tool. `isInsideContainer` was worse: it keyed on a `Workspaces` directory existing, so any machine that happens to have one looks like the agent container. Nothing names a directory now. `isInsideContainer` uses the markers Docker and Podman leave. `setup`'s comments state the rule its check enforces — a config must not be written where a container can rewrite it — instead of one example of it, and the fixtures use neutral names. --- AGENTS.md | 14 +++--- README.md | 15 +++--- examples/docker-compose.snippet.yml | 6 +-- src/commands/compose.ts | 2 +- src/commands/setup.ts | 11 ++--- src/commands/support.ts | 15 +++--- src/host-request.ts | 2 +- src/paths.ts | 6 +-- test/unit/commands.test.ts | 74 ++++++++++++++--------------- test/unit/host-request.test.ts | 18 +++---- test/unit/stage.test.ts | 18 +++---- 11 files changed, 92 insertions(+), 89 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 263bb4b..5b3d799 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -83,7 +83,7 @@ sources must stay erasable (`erasableSyntaxOnly` is on: no enums, no parameter p question the fix had not. **The other half is what it compares against.** The directories shared with the container are the - user's to name — `~/.dotfiles` is only ever what that user called it — so there is no list to hard-code. + user's to name, so there is no list to hard-code. A process inside the container reads its own mount table (`/proc/self/mountinfo`); a host process asks the container runtime (`docker inspect`) for the paths its containers bind-mount, which is a superset on purpose: any container that can rewrite the broker's code is as good a reason not to stage there as the @@ -172,7 +172,7 @@ sources must stay erasable (`erasableSyntaxOnly` is on: no enums, no parameter p is still the broker — `--via-host` is asked for, never assumed. The opener is started once per machine, not once per repository: it discovers the checkouts under - its `--root`s (default `$HOME`, so `~/Workspaces`, `~/.dotfiles` and anything added later are + its `--root`s (default `$HOME`, so every checkout under it, including anything added later, is served). The paths on the two sides never have to match — each side reads and writes its own view of the same `.git` directory — so only the roots it scans are host-side paths. A request is served in the checkout it was written in. That is a mechanism, not a boundary: the container can edit any checkout it can @@ -237,14 +237,16 @@ Full detail and the measurements behind them: - **Egress requires the proxy.** Direct connections to github.com hang rather than fail. The credential channel is a unix socket, so it is unaffected — that is why it is a socket. - **The container is recreated often**, so its writable layer is ephemeral. Anything that must - survive belongs in compose `environment:` or on a mounted path. `~/Workspaces`, `~/.dsh` and - `~/.dotfiles` are mounts; `/home/app` itself is not; `/volume1` does not exist inside. + survive belongs in compose `environment:` or on a mounted path. Which directories this deployment + mounts is the deployment's business — the tool discovers them rather than knowing them — and + `/home/app` itself is not one of them, nor does `/volume1` exist inside. - **Do not rely on the executable bit in commits**: this share's ACLs defeat git's exec-bit detection, so every sibling repository has `core.fileMode=false`. The exec bit for the CLI entry points is applied by `scripts/postbuild.mjs` at build time, and npm sets it for the installed bins, so nothing depends on it being tracked. -- **`~/.dsh` lives *inside* the `~/Workspaces` mount**, so "put it in `~/.dsh`" means "put it on - the shared NAS share". Never treat it as a private location for secrets. +- **A directory the container shares is not a private location**, even one that looks like the + tool's own home: this deployment's socket directory lives inside another shared directory, so + treating it as private would put a secret on the share. ## Where things are diff --git a/README.md b/README.md index 5fc02f4..2af0d7e 100644 --- a/README.md +++ b/README.md @@ -103,12 +103,12 @@ read-only mount, so nothing is fetched or built at boot. ```sh DEPLOY=/srv/git-cred-broker # broker.config.json, app.pem and log/ live here -SOCKET_DIR=~/.dsh/git-broker # a host directory the pushing container already mounts +SOCKET_DIR=/srv/git-broker-socket # a host directory the pushing container already mounts # Optional: the directories the agent container shares with this host. Left unset, a host process asks # the container runtime instead, and a process inside the container reads its own mount table. Set it # only to override that — a host with no runtime to ask, or to narrow what counts as shared. -# export GIT_BROKER_MOUNTS="$HOME/Workspaces:$HOME/.dsh:$HOME/.dotfiles" +# export GIT_BROKER_MOUNTS="/srv/shared-a:/srv/shared-b" git-credential-broker init --mode sidecar --dir "$DEPLOY" \ --cert ~/Downloads/app.private-key.pem \ @@ -170,7 +170,7 @@ when the image has none. In compose: - GIT_BROKER_SOCKET=/run/git-broker/broker.sock - GIT_BROKER_REQUIRE=1 volumes: - - ~/.dsh/git-broker:/run/git-broker + - /srv/git-broker-socket:/run/git-broker ``` ## Everyday changes @@ -297,8 +297,8 @@ git-credential-broker pr --via-host --repo owner/repo --head feature --base main The opener creates pull requests with the credentials of whoever started it, which is the point; it never runs a shell, and the program it calls is configuration rather than a hard-coded `gh`: `--command /path/to/gh`, or `$GIT_BROKER_PR_COMMAND`. It cannot push, merge, close or read -anything. `--root` may be repeated and defaults to `$HOME`, so `~/Workspaces`, `~/.dotfiles` and -checkouts created later are all served with no further setup. To start it once and keep it, +anything. `--root` may be repeated and defaults to `$HOME`, so every checkout under it — including +ones created later — is served with no further setup. To start it once and keep it, `examples/host-opener.service` is a systemd user unit; on a system without user services, `examples/host-opener-boot.sh` is the same thing for a DSM boot-up task — absolute paths, one instance at a time, and its output in a log file. Without `--via-host`, or with no opener running, `pr` behaves @@ -466,8 +466,9 @@ matter: can rewrite that code, the agent owns the allowlist. That is what `stage` is for. With a shared `.git/config` between host and container: never set credentials with `git config ---local` (it is the same file on both sides); the container's own `~/.gitconfig` is private, but -`~/.dotfiles` is mounted, which is why `setup` refuses to write through a symlink into it. +--local` (it is the same file on both sides); the container's own `~/.gitconfig` is private, but any +directory the container shares is not, which is why `setup` refuses to write the config through a +symlink or into one. Credential helpers are only consulted for HTTP(S) remotes, so none of this affects the host's SSH pushes. diff --git a/examples/docker-compose.snippet.yml b/examples/docker-compose.snippet.yml index 81f7658..05719d9 100644 --- a/examples/docker-compose.snippet.yml +++ b/examples/docker-compose.snippet.yml @@ -122,6 +122,6 @@ Then, inside the container: git-credential-broker setup ``` -Do **not** put the private key or any credential in `~/Workspaces`, `~/.dsh` or `~/.dotfiles`: -all three are mounted into the container. `~/.dsh` in particular is *inside* the -`~/Workspaces` mount, so it is not a safe place for anything secret. +Do **not** put the private key or any credential anywhere the agent container mounts: every such +directory is shared with it. The broker's configuration, key and token belong outside all of them, +and `init`, `stage` and `authorize` refuse a target they detect as shared. diff --git a/src/commands/compose.ts b/src/commands/compose.ts index dd89c04..9c9916f 100644 --- a/src/commands/compose.ts +++ b/src/commands/compose.ts @@ -196,7 +196,7 @@ export function runCompose(argv: readonly string[]): number { const configPath = resolveConfigPath(args.value('config')); const socketDir = args.value('socket-dir'); if (!socketDir) { - fail('--socket-dir is required (a host directory the pushing container already mounts, e.g. its ~/.dsh)'); + fail('--socket-dir is required (a host directory the pushing container already mounts)'); } const dir = args.value('dir') ?? deploymentDir(configPath); diff --git a/src/commands/setup.ts b/src/commands/setup.ts index 0d5c37d..be9b444 100644 --- a/src/commands/setup.ts +++ b/src/commands/setup.ts @@ -97,7 +97,7 @@ export function defaultHelperPath(): string { * * @param input - the resolved options. * @returns what was done. - * @throws {Error} when the target config is a symlink or resolves into the host's dotfiles. + * @throws {Error} when the target config is a symlink, or inside a directory the container shares. */ export async function performSetup(input: SetupInput): Promise { // Refuse on native Windows: configuring git there would look like success and never work, because @@ -106,15 +106,14 @@ export async function performSetup(input: SetupInput): Promise { if (refusal !== null) { throw new Error(refusal); } - // Writing through a symlink would edit whatever it points at. On the host, ~/.gitconfig is a - // symlink into ~/.dotfiles, which is mounted into the container — so this is not theoretical. + // Writing through a symlink would edit whatever it points at, and a config that lives in a directory + // the container shares is one the container can rewrite. if (fs.existsSync(input.gitconfig) && fs.lstatSync(input.gitconfig).isSymbolicLink()) { const target = fs.realpathSync(input.gitconfig); throw new Error(`${input.gitconfig} is a symlink to ${target}; refusing to write through it`); } - // The general form of "~/.gitconfig is a symlink into ~/.dotfiles": the config must not be written into - // any directory the container can rewrite. Canonical, so the host's spelling of a mount and the - // container's spelling of the same directory are one place. + // The config must not be written into any directory the container can rewrite. Canonical, so the + // host's spelling of a shared directory and the container's spelling of the same one are one place. const mounted = insideMountedPath(input.gitconfig, input.home, input.mounts); if (mounted) { throw new Error( diff --git a/src/commands/support.ts b/src/commands/support.ts index ef7c1f3..55867aa 100644 --- a/src/commands/support.ts +++ b/src/commands/support.ts @@ -102,15 +102,16 @@ export function listFlag(args: Args, name: string): string[] { } /** - * Whether this process is running inside the agent container rather than on the host. + * Whether this process is running inside a container rather than on the host. * - * The container mounts the workspace but has no `/volume1`, which is where the host keeps - * everything this project needs to protect. + * A credential must not be written anywhere a container can read it, and the broker must not run + * inside the agent container. Docker and Podman each leave a marker; nothing here names a directory, + * because no directory is this tool's to assume. * - * @returns true when running inside the container. + * @returns true when running inside a container. */ export function isInsideContainer(): boolean { - return fs.existsSync('/home/app/Workspaces') && !fs.existsSync('/volume1'); + return fs.existsSync('/.dockerenv') || fs.existsSync('/run/.containerenv'); } /** The environment variable naming the directories the container shares with the host. */ @@ -168,7 +169,7 @@ function canonicalOrResolved(target: string): string { * The mount points at or under a home, read from a mount table. * * The container is the thing that knows what it mounts. A list of directory names in this file would - * be a guess about someone else's home — `.dotfiles` is only ever what the user called it. + * be a guess about a home this code has never seen. * * @param mountinfo - the contents of `/proc/self/mountinfo`. * @param home - the home to measure against. @@ -274,7 +275,7 @@ export function mountedPaths(home: string = os.homedir(), source: MountSource = mounts = mountsUnderHome(source.mountinfo, home); } else if (source.inspect !== undefined) { mounts = source.inspect === null ? undefined : boundPathsUnderHome(source.inspect, home); - } else if (source.container ?? fs.existsSync('/.dockerenv')) { + } else if (source.container ?? isInsideContainer()) { try { mounts = mountsUnderHome(fs.readFileSync('/proc/self/mountinfo', 'utf8'), home); } catch { diff --git a/src/host-request.ts b/src/host-request.ts index eb6d9a2..3067ce9 100644 --- a/src/host-request.ts +++ b/src/host-request.ts @@ -150,7 +150,7 @@ const SKIP = new Set(['.git', 'node_modules', '@eaDir', '#recycle', '$RECYCLE.BI * * The opener is meant to be started once for a machine, so it is given roots rather than one * repository: every checkout below them is served, including ones created later. Dotted - * directories are *not* skipped — a dotfiles repository is one, and it is a checkout like any + * directories are *not* skipped — a repository whose name begins with a dot is a checkout like any * other. A directory holding a checkout is not descended into, which keeps the walk cheap. * * @param root - where to start. diff --git a/src/paths.ts b/src/paths.ts index f7cf422..98bdca2 100644 --- a/src/paths.ts +++ b/src/paths.ts @@ -13,9 +13,9 @@ import path from 'node:path'; * 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 these checks exist to close. + * nearest existing ancestor is resolved and the rest is appended. Comparing raw spellings is what lets + * one directory with two names — a symlinked home, a bind mount reached two ways — look like two + * different places, which is exactly the bypass these checks exist to close. * * @param target - the path to canonicalize. * @returns the real path, or the resolved path when nothing on it exists. diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index a8d2bda..a2b6161 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -98,14 +98,14 @@ describe('parseArgs', () => { describe('mounted path detection', () => { /** A home and a shared-directory list stated here, so no case reads the machine's own mount table. */ - const home = '/home/app'; - const mounts = ['/home/app/Workspaces', '/home/app/.dsh', '/home/app/.dotfiles']; + const home = '/home/u'; + const mounts = ['/home/u/shared', '/home/u/cache', '/home/u/notes']; it('is inside a shared directory, and not a sibling that merely starts the same', () => { - assert.equal(insideMountedPath('/home/app/Workspaces/x', home, mounts), '/home/app/Workspaces'); - assert.equal(insideMountedPath('/home/app/.dsh/git-broker', home, mounts), '/home/app/.dsh'); - assert.equal(insideMountedPath('/volume1/docker/git-cred-broker', home, mounts), null); - assert.equal(insideMountedPath('/home/app/Workspaces-evil', home, mounts), null, 'must not prefix-match a sibling'); + assert.equal(insideMountedPath('/home/u/shared/x', home, mounts), '/home/u/shared'); + assert.equal(insideMountedPath('/home/u/cache/git-broker', home, mounts), '/home/u/cache'); + assert.equal(insideMountedPath('/srv/other', home, mounts), null); + assert.equal(insideMountedPath('/home/u/shared-evil', home, mounts), null, 'must not prefix-match a sibling'); }); it('takes the shared directories from GIT_BROKER_MOUNTS when it is set', () => { @@ -116,26 +116,26 @@ describe('mounted path detection', () => { it('reads them from the container mount table when nothing is declared', () => { const table = [ - '675 573 0:36 /x /home/app/Workspaces rw - btrfs /dev/x rw', - '676 573 0:36 /y /home/app/with\\040space rw - btrfs /dev/x rw', + `675 573 0:36 /x ${home}/shared rw - btrfs /dev/x rw`, + `676 573 0:36 /y ${home}/with\\040space rw - btrfs /dev/x rw`, '677 573 0:36 /z /somewhere/else rw - btrfs /dev/x rw', ].join('\n'); assert.deepEqual(mountedPaths(home, { declared: '', mountinfo: table, container: true }), [ - '/home/app/Workspaces', - '/home/app/with space', + `${home}/shared`, + `${home}/with space`, ]); }); it('asks the container runtime on the host, and keeps only bind mounts under the home', () => { const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-inspect-')); - const inside = path.join(base, 'Workspaces'); + const inside = path.join(base, 'shared'); const deeper = path.join(base, 'sub', 'deep'); fs.mkdirSync(inside, { recursive: true }); fs.mkdirSync(deeper, { recursive: true }); const inspect = [ JSON.stringify([ - { Type: 'bind', Source: inside, Destination: '/home/app/Workspaces' }, - { Type: 'bind', Source: deeper, Destination: '/home/app/.dsh' }, + { Type: 'bind', Source: inside, Destination: '/home/u/shared' }, + { Type: 'bind', Source: deeper, Destination: '/home/u/cache' }, { Type: 'volume', Source: path.join(base, 'volume'), Destination: '/vol' }, { Type: 'bind', Source: '/srv/elsewhere', Destination: '/srv/elsewhere' }, ]), @@ -162,29 +162,29 @@ describe('mounted path detection', () => { }); 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. + // One shared directory can have two spellings — a symlinked home, a bind mount reached two ways — + // so a lexical prefix check calls a path inside it outside, which is how `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 }); + fs.mkdirSync(path.join(real, 'data'), { 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'); + const mounted = path.join(alias, 'data'); const shared = [mounted]; assert.equal( - insideMountedPath(path.join(real, 'Workspaces', 'x'), alias, shared), + insideMountedPath(path.join(real, 'data', 'x'), alias, shared), mounted, 'real target, aliased mount', ); - assert.equal(insideMountedPath(path.join(real, 'Workspaces'), alias, shared), mounted, 'and the mount itself'); + assert.equal(insideMountedPath(path.join(real, 'data'), alias, shared), mounted, 'and the mount itself'); assert.equal(insideMountedPath(path.join(real, 'elsewhere'), alias, shared), null, 'a sibling is still outside'); assert.equal( - insideMountedPath(path.join(alias, 'Workspaces', 'y'), real, [path.join(real, 'Workspaces')]), - path.join(real, 'Workspaces'), + insideMountedPath(path.join(alias, 'data', 'y'), real, [path.join(real, 'data')]), + path.join(real, 'data'), 'and the reverse spelling', ); } finally { @@ -201,12 +201,12 @@ describe('mounted path detection', () => { const alias = path.join(root, 'alias'); fs.symlinkSync(real, alias, 'junction'); try { - const mounted = path.join(alias, 'Workspaces'); + const mounted = path.join(alias, 'data'); const shared = [mounted]; - assert.equal(insideMountedPath(path.join(real, 'Workspaces', 'new', 'deep'), alias, shared), mounted); - assert.equal(insideMountedPath(path.join(real, 'Workspaces'), alias, shared), mounted); + assert.equal(insideMountedPath(path.join(real, 'data', 'new', 'deep'), alias, shared), mounted); + assert.equal(insideMountedPath(path.join(real, 'data'), alias, shared), mounted); assert.equal( - insideMountedPath(path.join(real, 'Workspaces-evil'), alias, shared), + insideMountedPath(path.join(real, 'data-evil'), alias, shared), null, 'a sibling is not inside, with the mount missing too', ); @@ -290,18 +290,18 @@ describe('setup', () => { }); it('refuses a config inside a container mount, whatever the mount is spelled', async () => { - // The host spells $HOME /var/services/homes/u and the container spells the same directory - // /volume1/homes/u. The check this replaced joined ~/.dotfiles lexically, so the second spelling was - // accepted and the host's own git config would be written where the container can rewrite it. + // One shared directory can have two spellings (a symlinked home, a bind mount). The check this + // replaced compared spellings, so the container's was accepted and the host's own git config would + // be written where the container can rewrite it. const base = fs.mkdtempSync(path.join(os.tmpdir(), 'setup-mount-alias-')); const real = path.join(base, 'real'); - fs.mkdirSync(path.join(real, '.dotfiles'), { recursive: true }); + fs.mkdirSync(path.join(real, 'shared'), { recursive: true }); const alias = path.join(base, 'alias'); fs.symlinkSync(real, alias, 'junction'); try { - const gitconfig = path.join(real, '.dotfiles', 'gitconfig'); + const gitconfig = path.join(real, 'shared', 'gitconfig'); await assert.rejects( - () => performSetup(setupInput({ gitconfig, home: alias, mounts: [path.join(alias, '.dotfiles')], dryRun: true })), + () => performSetup(setupInput({ gitconfig, home: alias, mounts: [path.join(alias, 'shared')], dryRun: true })), /mounted into the container/, ); assert.equal(fs.existsSync(gitconfig), false, 'and nothing was written'); @@ -400,14 +400,14 @@ describe('init', () => { it('refuses a key inside a container mount', () => { const root = dir(`init-mount-${Math.random().toString(36).slice(2)}`); const cert = writeKey(path.join(root, 'downloaded.pem')); - const mounted = path.join(fakeHome, 'Workspaces', 'somewhere'); + const mounted = path.join(fakeHome, 'shared', 'somewhere'); assert.throws( () => performInit({ ...baseInput, cert, dir: mounted, - env: { ...baseInput.env, mounts: [path.join(fakeHome, 'Workspaces')] }, + env: { ...baseInput.env, mounts: [path.join(fakeHome, 'shared')] }, }), /mounted into the container/, 'the private key must never live where the container can read it', @@ -755,7 +755,7 @@ describe('config discovery', () => { describe('compose', () => { const rendered = renderCompose({ dir: '/volume1/docker/git-cred-broker', - socketDir: '/volume1/homes/u/Workspaces/h/.dsh/git-broker', + socketDir: '/srv/shared/git-broker', user: '1026:100', image: 'node:24-slim', packageSpec: 'git-credential-broker@0.1.0', @@ -783,7 +783,7 @@ describe('compose', () => { it('can run staged code from a mount instead, fetching and building nothing at boot', () => { const fromCode = renderCompose({ dir: '/volume1/docker/git-cred-broker', - socketDir: '/volume1/homes/u/Workspaces/h/.dsh/git-broker', + socketDir: '/srv/shared/git-broker', user: '1026:100', image: 'node:24-slim', packageSpec: 'git-credential-broker@0.1.0', @@ -831,7 +831,7 @@ describe('compose', () => { assert.equal(rendered.includes(`${deployDir}/app.pem:${SIDECAR.keyPath}:ro`), true); assert.equal(rendered.includes(`${deployDir}/log:${SIDECAR.auditDir}`), true); assert.equal( - rendered.includes('/volume1/homes/u/Workspaces/h/.dsh/git-broker:' + SIDECAR.socketDir), + rendered.includes('/srv/shared/git-broker:' + SIDECAR.socketDir), true, ); }); diff --git a/test/unit/host-request.test.ts b/test/unit/host-request.test.ts index a5b3904..c7cfa4a 100644 --- a/test/unit/host-request.test.ts +++ b/test/unit/host-request.test.ts @@ -185,25 +185,25 @@ describe('reading a pull request URL out of a program', () => { }); describe('finding the checkouts to serve', () => { - it('finds every checkout under a root, at any depth, dotfiles included', () => { + it('finds every checkout under a root, at any depth, hidden directories included', () => { const root = mkdtempSync(join(tmpdir(), 'discover-test-')); - for (const repo of ['.dotfiles', 'Workspaces/one', 'Workspaces/two/deep', 'plain']) { + for (const repo of ['.hidden', 'trees/one', 'trees/two/deep', 'plain']) { mkdirSync(join(root, repo, '.git'), { recursive: true }); writeFileSync(join(root, repo, '.git', 'HEAD'), 'ref: refs/heads/main\n', 'utf8'); writeFileSync(join(root, repo, '.git', 'HEAD'), 'ref: refs/heads/main\n', 'utf8'); } // never descended into, and never enough on its own - mkdirSync(join(root, 'Workspaces/one/node_modules/decoy/.git'), { recursive: true }); - writeFileSync(join(root, 'Workspaces/one/node_modules/decoy/.git', 'HEAD'), 'ref: refs/heads/main\n', 'utf8'); - writeFileSync(join(root, 'Workspaces/one/node_modules/decoy/.git', 'HEAD'), 'ref: refs/heads/main\n', 'utf8'); - mkdirSync(join(root, 'Workspaces/three/four/five/six/seven/.git'), { recursive: true }); - writeFileSync(join(root, 'Workspaces/three/four/five/six/seven/.git', 'HEAD'), 'ref: refs/heads/main\n', 'utf8'); - writeFileSync(join(root, 'Workspaces/three/four/five/six/seven/.git', 'HEAD'), 'ref: refs/heads/main\n', 'utf8'); + mkdirSync(join(root, 'trees/one/node_modules/decoy/.git'), { recursive: true }); + writeFileSync(join(root, 'trees/one/node_modules/decoy/.git', 'HEAD'), 'ref: refs/heads/main\n', 'utf8'); + writeFileSync(join(root, 'trees/one/node_modules/decoy/.git', 'HEAD'), 'ref: refs/heads/main\n', 'utf8'); + mkdirSync(join(root, 'trees/three/four/five/six/seven/.git'), { recursive: true }); + writeFileSync(join(root, 'trees/three/four/five/six/seven/.git', 'HEAD'), 'ref: refs/heads/main\n', 'utf8'); + writeFileSync(join(root, 'trees/three/four/five/six/seven/.git', 'HEAD'), 'ref: refs/heads/main\n', 'utf8'); const repos = discoverCheckouts(root, 4) .map((found) => found.repo.slice(root.length + 1)) .sort(); - assert.deepEqual(repos, ['.dotfiles', 'Workspaces/one', 'Workspaces/two/deep', 'plain']); + assert.deepEqual(repos, ['.hidden', 'plain', 'trees/one', 'trees/two/deep']); rmSync(root, { recursive: true, force: true }); }); diff --git a/test/unit/stage.test.ts b/test/unit/stage.test.ts index 7585cef..7eea5d7 100644 --- a/test/unit/stage.test.ts +++ b/test/unit/stage.test.ts @@ -89,23 +89,23 @@ describe('stage', () => { it('refuses a target inside a container mount', () => { const repo = makeRepo('mounted'); const home = path.join(scratch, 'home'); - const inside = path.join(home, 'Workspaces', 'somewhere'); + const inside = path.join(home, 'shared', 'somewhere'); fs.mkdirSync(inside, { recursive: true }); assert.throws( - () => stage({ repo, to: inside, ref: 'HEAD', home, mounts: [path.join(home, 'Workspaces')] }), + () => stage({ repo, to: inside, ref: 'HEAD', home, mounts: [path.join(home, 'shared')] }), /mounted into the container/, 'code the agent can rewrite must never be staged where it will run with the key', ); }); 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. + // One shared directory can have two spellings (a symlinked home, a bind mount). 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 }); + fs.mkdirSync(path.join(real, 'shared'), { recursive: true }); const alias = path.join(scratch, 'alias-link'); fs.symlinkSync(real, alias, 'junction'); @@ -113,14 +113,14 @@ describe('stage', () => { () => stage({ repo, - to: path.join(real, 'Workspaces', 'app'), + to: path.join(real, 'shared', 'app'), ref: 'HEAD', home: alias, - mounts: [path.join(alias, 'Workspaces')], + mounts: [path.join(alias, 'shared')], }), /mounted into the container/, ); - assert.equal(fs.existsSync(path.join(real, 'Workspaces', 'app')), false, 'and nothing was written'); + assert.equal(fs.existsSync(path.join(real, 'shared', 'app')), false, 'and nothing was written'); }); it('refuses to clear the deployment directory that holds the key', () => { From d09d787600ee125f4c16ddd07d465d72001e83b4 Mon Sep 17 00:00:00 2001 From: JounQin Date: Sun, 4 Oct 2026 01:01:01 +0000 Subject: [PATCH 05/10] fix: drop GIT_BROKER_MOUNTS; detection is the only source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The runtime is asked, so the variable was a second way to say the same thing — and one an operator has to keep true by hand. It is gone, with the branch that read it, the help line, the docs and its test. When nothing can answer — not in a container, and no runtime to ask — the check still refuses, and now says which of the two failed. --- AGENTS.md | 4 ++-- README.md | 14 ++++--------- src/commands/index.ts | 3 --- src/commands/support.ts | 41 ++++++++++++++------------------------ test/unit/commands.test.ts | 16 +++++---------- test/unit/stage.test.ts | 4 ++-- 6 files changed, 28 insertions(+), 54 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5b3d799..7595620 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -87,8 +87,8 @@ sources must stay erasable (`erasableSyntaxOnly` is on: no enums, no parameter p A process inside the container reads its own mount table (`/proc/self/mountinfo`); a host process asks the container runtime (`docker inspect`) for the paths its containers bind-mount, which is a superset on purpose: any container that can rewrite the broker's code is as good a reason not to stage there as the - agent is. `GIT_BROKER_MOUNTS` overrides both. When none can answer, the check refuses rather than - guesses — the guess is what would put the code, or the key, somewhere a container can rewrite. + agent is. When neither can answer, the check refuses rather than guesses — the guess is what would put + the code, or the key, somewhere a container can rewrite. - **The process that owns a boundary decides what crosses it, not the error class.** A response body put into the message of a `ProviderConfigError` reached the container, because a caller that wraps that error to name a missing permission wraps it in the same class and so passes the whole message through. Detail diff --git a/README.md b/README.md index 2af0d7e..4ae257e 100644 --- a/README.md +++ b/README.md @@ -105,11 +105,6 @@ read-only mount, so nothing is fetched or built at boot. DEPLOY=/srv/git-cred-broker # broker.config.json, app.pem and log/ live here SOCKET_DIR=/srv/git-broker-socket # a host directory the pushing container already mounts -# Optional: the directories the agent container shares with this host. Left unset, a host process asks -# the container runtime instead, and a process inside the container reads its own mount table. Set it -# only to override that — a host with no runtime to ask, or to narrow what counts as shared. -# export GIT_BROKER_MOUNTS="/srv/shared-a:/srv/shared-b" - git-credential-broker init --mode sidecar --dir "$DEPLOY" \ --cert ~/Downloads/app.private-key.pem \ --allow owner/repo --client-id Iv23li… --app-id 123456 @@ -122,11 +117,10 @@ docker compose -f "$DEPLOY/docker-compose.broker.yml" up -d Nothing above has to name the shared directories. A host process asks the container runtime for the host paths its containers bind-mount; a process inside the container reads `/proc/self/mountinfo`. -`GIT_BROKER_MOUNTS` overrides both, for a host with no runtime to ask or to narrow what counts as -shared. When none of the three can answer, `init`, `stage` and `authorize` refuse rather than guess. -The runtime route is deliberately a superset: a directory bind-mounted into *any* running container -counts as shared, because any container that can rewrite the broker's code is as good a reason not to -stage there as the agent is. +When neither can answer, `init`, `stage` and `authorize` refuse rather than guess. The runtime route +is deliberately a superset: a directory bind-mounted into *any* running container counts as shared, +because any container that can rewrite the broker's code is as good a reason not to stage there as +the agent is. `stage` prints the commit it exported; check that sha against your own clone or the remote before trusting it, since the repository you export from is one an agent can write to. It leaves a diff --git a/src/commands/index.ts b/src/commands/index.ts index e058c3b..e8a46e6 100644 --- a/src/commands/index.ts +++ b/src/commands/index.ts @@ -41,9 +41,6 @@ Environment GIT_BROKER_SOCKET Broker socket path GIT_BROKER_REQUIRE=1 Fail closed instead of behaving as if not installed when unconfigured GIT_BROKER_SOCKET_FILE Where to read the socket path from when no variable is set - GIT_BROKER_MOUNTS Override the directories the agent container shares with this host, - ":"-separated; otherwise the container runtime (on the host) or the mount - table (in the container) is asked Run \`git-credential-broker --help\` for the options of one command.`; diff --git a/src/commands/support.ts b/src/commands/support.ts index 55867aa..6f0eb92 100644 --- a/src/commands/support.ts +++ b/src/commands/support.ts @@ -114,13 +114,8 @@ export function isInsideContainer(): boolean { return fs.existsSync('/.dockerenv') || fs.existsSync('/run/.containerenv'); } -/** The environment variable naming the directories the container shares with the host. */ -export const MOUNTS_VAR = 'GIT_BROKER_MOUNTS'; - -/** Where the shared directories came from, when a caller supplies them instead of the environment. */ +/** Where the shared directories came from, when a caller supplies them instead of detecting them. */ export interface MountSource { - /** The raw value of {@link MOUNTS_VAR}. */ - readonly declared?: string; /** The contents of `/proc/self/mountinfo`. */ readonly mountinfo?: string; /** `Mounts` arrays as the container runtime prints them; null when it could not be asked. */ @@ -251,31 +246,23 @@ function detectedMounts(home: string): string[] | undefined { /** * The directories the container shares with the host. * - * In order: `GIT_BROKER_MOUNTS`, which is how an operator overrides everything; the container's own - * mount table, when this process is in the container; the container runtime, when it is not. Only - * when none of the three can answer does this refuse — the guess it will not make is what would put - * the broker's code, or the private key, in a directory a container can rewrite. + * The container's own mount table answers when this process is in the container; the container runtime + * answers when it is not. Only when neither can does this refuse — the guess it will not make is what + * would put the broker's code, or the private key, in a directory a container can rewrite. * * @param home - the home whose mounts are wanted. - * @param source - a declared value, mount table or runtime output, for a test. + * @param source - a mount table or runtime output, for a test. * @returns the shared directories. * @throws {Error} when nothing can answer. */ export function mountedPaths(home: string = os.homedir(), source: MountSource = {}): string[] { - const declared = source.declared ?? process.env[MOUNTS_VAR]; - if (declared !== undefined && declared.trim() !== '') { - return declared - .split(path.delimiter) - .map((entry) => entry.trim()) - .filter((entry) => entry !== ''); - } - + const container = source.container ?? isInsideContainer(); let mounts: string[] | undefined; if (source.mountinfo !== undefined) { mounts = mountsUnderHome(source.mountinfo, home); } else if (source.inspect !== undefined) { mounts = source.inspect === null ? undefined : boundPathsUnderHome(source.inspect, home); - } else if (source.container ?? isInsideContainer()) { + } else if (container) { try { mounts = mountsUnderHome(fs.readFileSync('/proc/self/mountinfo', 'utf8'), home); } catch { @@ -287,9 +274,11 @@ export function mountedPaths(home: string = os.homedir(), source: MountSource = if (mounts === undefined) { throw new Error( - `cannot tell which directories the container shares: set ${MOUNTS_VAR} to them, separated by ` + - `"${path.delimiter}" (inside the container its mount table is read; on the host the container ` + - 'runtime is asked, and it did not answer)', + 'cannot tell which directories the container shares: ' + + (container + ? 'its own mount table could not be read' + : 'the container runtime could not be asked (is docker installed and reachable?)') + + '; refusing rather than guessing', ); } return mounts; @@ -298,9 +287,9 @@ export function mountedPaths(home: string = os.homedir(), source: MountSource = /** * Whether a path is inside one of the directories the container shares with the host. * - * 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. + * Both sides are canonicalized before they are compared: the host may spell a directory one way and + * the container another (a symlinked home, a bind mount reached two ways), 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. diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index a2b6161..4569e3a 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -108,19 +108,13 @@ describe('mounted path detection', () => { assert.equal(insideMountedPath('/home/u/shared-evil', home, mounts), null, 'must not prefix-match a sibling'); }); - it('takes the shared directories from GIT_BROKER_MOUNTS when it is set', () => { - const declared = ['/srv/a', '/srv/b'].join(path.delimiter); - assert.deepEqual(mountedPaths(home, { declared }), ['/srv/a', '/srv/b']); - assert.deepEqual(mountedPaths(home, { declared: ` ${['/srv/a'].join(path.delimiter)} ` }), ['/srv/a']); - }); - - it('reads them from the container mount table when nothing is declared', () => { + it('reads them from the container mount table', () => { const table = [ `675 573 0:36 /x ${home}/shared rw - btrfs /dev/x rw`, `676 573 0:36 /y ${home}/with\\040space rw - btrfs /dev/x rw`, '677 573 0:36 /z /somewhere/else rw - btrfs /dev/x rw', ].join('\n'); - assert.deepEqual(mountedPaths(home, { declared: '', mountinfo: table, container: true }), [ + assert.deepEqual(mountedPaths(home, { mountinfo: table, container: true }), [ `${home}/shared`, `${home}/with space`, ]); @@ -141,7 +135,7 @@ describe('mounted path detection', () => { ]), ].join('\n'); try { - const got = mountedPaths(base, { declared: '', container: false, inspect }) + const got = mountedPaths(base, { container: false, inspect }) .map((entry) => fs.realpathSync(entry)) .sort(); assert.deepEqual( @@ -156,8 +150,8 @@ describe('mounted path detection', () => { it('refuses rather than guess when nothing can answer', () => { assert.throws( - () => mountedPaths(home, { declared: '', container: false, inspect: null }), - /GIT_BROKER_MOUNTS/, + () => mountedPaths(home, { container: false, inspect: null }), + /refusing rather than guessing/, ); }); diff --git a/test/unit/stage.test.ts b/test/unit/stage.test.ts index 7eea5d7..c830ab3 100644 --- a/test/unit/stage.test.ts +++ b/test/unit/stage.test.ts @@ -40,8 +40,8 @@ describe('stage', () => { /** * `performStage` with the shared directories supplied. * - * Resolving them for real refuses on a host with no `GIT_BROKER_MOUNTS`, which is the point in - * production and noise here: each case states its own list, and the mount cases pass theirs. + * Resolving them for real asks the container runtime, which is right in production and noise here: + * each case states its own list, and the mount cases pass theirs. */ const stage = (input: Parameters[0]) => performStage({ mounts: [], ...input }); From 8097b3301a1d452c3c191ec6b9e401952da9d80a Mon Sep 17 00:00:00 2001 From: JounQin Date: Sun, 4 Oct 2026 01:10:57 +0000 Subject: [PATCH 06/10] fix: ask podman and nerdctl too, and say what the refusal means MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `docker` was the only runtime asked, so a host running Podman, or containerd through nerdctl, was refused for no reason but the CLI's name. All three share the `ps`/`inspect` interface, so one path asks each and unions the answers: a container under any of them can still rewrite the staged code. Plain containerd (`ctr`, `crictl`) has no equivalent interface — a different language, in a namespace it chooses — so a host that offers only that still refuses, and the message now names what was tried. --- AGENTS.md | 8 +++---- README.md | 13 ++++++------ src/commands/support.ts | 46 ++++++++++++++++++++++++++++++++--------- 3 files changed, 47 insertions(+), 20 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7595620..5ce2eea 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -85,10 +85,10 @@ sources must stay erasable (`erasableSyntaxOnly` is on: no enums, no parameter p **The other half is what it compares against.** The directories shared with the container are the user's to name, so there is no list to hard-code. A process inside the container reads its own mount table (`/proc/self/mountinfo`); a host process asks - the container runtime (`docker inspect`) for the paths its containers bind-mount, which is a superset on - purpose: any container that can rewrite the broker's code is as good a reason not to stage there as the - agent is. When neither can answer, the check refuses rather than guesses — the guess is what would put - the code, or the key, somewhere a container can rewrite. + a container runtime — docker, podman or nerdctl, whichever is there — for the paths its containers + bind-mount, which is a superset on purpose: any container that can rewrite the broker's code is as good + a reason not to stage there as the agent is. When neither can answer, the check refuses rather than + guesses — the guess is what would put the code, or the key, somewhere a container can rewrite. - **The process that owns a boundary decides what crosses it, not the error class.** A response body put into the message of a `ProviderConfigError` reached the container, because a caller that wraps that error to name a missing permission wraps it in the same class and so passes the whole message through. Detail diff --git a/README.md b/README.md index 4ae257e..792f258 100644 --- a/README.md +++ b/README.md @@ -115,12 +115,13 @@ git-credential-broker compose --code "$DEPLOY/app" --socket-dir "$SOCKET_DIR" \ docker compose -f "$DEPLOY/docker-compose.broker.yml" up -d ``` -Nothing above has to name the shared directories. A host process asks the container runtime for the -host paths its containers bind-mount; a process inside the container reads `/proc/self/mountinfo`. -When neither can answer, `init`, `stage` and `authorize` refuse rather than guess. The runtime route -is deliberately a superset: a directory bind-mounted into *any* running container counts as shared, -because any container that can rewrite the broker's code is as good a reason not to stage there as -the agent is. +Nothing above has to name the shared directories. A host process asks the container runtime — docker, +podman or nerdctl, whichever is there — for the host paths its containers bind-mount; a process inside +the container reads `/proc/self/mountinfo`. When neither can answer — including a host whose only +runtime is plain `containerd`, which has no interface this knows — `init`, `stage` and `authorize` +refuse rather than guess. The runtime route is deliberately a superset: a directory bind-mounted into +*any* running container counts as shared, because any container that can rewrite the broker's code is +as good a reason not to stage there as the agent is. `stage` prints the commit it exported; check that sha against your own clone or the remote before trusting it, since the repository you export from is one an agent can write to. It leaves a diff --git a/src/commands/support.ts b/src/commands/support.ts index 6f0eb92..ff085d2 100644 --- a/src/commands/support.ts +++ b/src/commands/support.ts @@ -124,8 +124,14 @@ export interface MountSource { readonly container?: boolean; } -/** The command that reports what running containers mount. */ -const CONTAINER_CLI = 'docker'; +/** + * The commands that can report what running containers mount. + * + * Docker, Podman and nerdctl share the `ps`/`inspect` interface used here, so one code path covers all + * three. Plain containerd has no equivalent — `ctr` speaks a different language, in a namespace it + * chooses — so a host that offers only that is refused rather than guessed at. + */ +const CONTAINER_CLIS = ['docker', 'podman', 'nerdctl'] as const; /** * Decode one field of `/proc/self/mountinfo`. @@ -217,17 +223,15 @@ export function boundPathsUnderHome(output: string, home: string): string[] { } /** - * Ask the container runtime which directories it has bound from the user's home. - * - * The host's own mount table cannot answer this: a bind mount exists only in the container's - * namespace, so the runtime is asked instead. + * One runtime's answer, or undefined when that command is not there to ask. * + * @param cli - the command to run. * @param home - the home to keep mounts under. - * @returns the paths, or undefined when the runtime is not there to ask. + * @returns the paths, or undefined when this command could not answer. */ -function detectedMounts(home: string): string[] | undefined { +function mountsFromRuntime(cli: string, home: string): string[] | undefined { const run = (args: readonly string[]): string => - execFileSync(CONTAINER_CLI, args, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }); + execFileSync(cli, args, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }); let ids: string; try { ids = run(['ps', '--quiet']); @@ -243,6 +247,28 @@ function detectedMounts(home: string): string[] | undefined { } } +/** + * Ask every container runtime on this host which directories it has bound from the user's home. + * + * The host's own mount table cannot answer this: a bind mount exists only in the container's + * namespace, so the runtimes are asked instead. Their answers are unioned, because one host can run + * more than one of them and a container under either is still a container that can rewrite the code. + * + * @param home - the home to keep mounts under. + * @returns the paths, or undefined when no runtime was there to ask. + */ +function detectedMounts(home: string): string[] | undefined { + const found = new Set(); + let answered = false; + for (const cli of CONTAINER_CLIS) { + const mounts = mountsFromRuntime(cli, home); + if (mounts === undefined) continue; + answered = true; + for (const mount of mounts) found.add(mount); + } + return answered ? [...found] : undefined; +} + /** * The directories the container shares with the host. * @@ -277,7 +303,7 @@ export function mountedPaths(home: string = os.homedir(), source: MountSource = 'cannot tell which directories the container shares: ' + (container ? 'its own mount table could not be read' - : 'the container runtime could not be asked (is docker installed and reachable?)') + + : 'none of docker, podman or nerdctl could be asked') + '; refusing rather than guessing', ); } From 358a3f2fc55999a56e82ca4c93298f6be4b29d8c Mon Sep 17 00:00:00 2001 From: JounQin Date: Sun, 4 Oct 2026 01:41:04 +0000 Subject: [PATCH 07/10] test: the config-path case states its mounts, like every other `writes to an explicit --config path, creating its directory` built its own init input and left `mounts` out, so `performInit` resolved the shared directories for real. Linux has docker, so an answer came back; the macOS and WSL runners have none of docker, podman or nerdctl, and the refusal failed the test. This is the only guard test that did not state its own list. --- test/unit/commands.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index 4569e3a..4945444 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -737,7 +737,7 @@ describe('config discovery', () => { appId: 123456, permissions: { contents: 'write' }, force: false, - env: { isInsideContainer: () => false, home: dir('discovery-home') }, + env: { isInsideContainer: () => false, home: dir('discovery-home'), mounts: [] }, }); assert.equal(result.configPath, custom); From 4f5c0b0880cd4e653dcd2eb8d1d8f53c32b43661 Mon Sep 17 00:00:00 2001 From: JounQin Date: Sun, 4 Oct 2026 01:54:56 +0000 Subject: [PATCH 08/10] fix: a stopped container is still a container that holds the mount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mountsFromRuntime` asked `docker ps --quiet`, which lists only *running* containers, and returned `[]` — not "cannot tell" — when a runtime knew of none. `mountedPaths` accepted that as an answer, so the guard was blind exactly when `init`/`stage`/`authorize` is most likely to run with the agent container down: the key would be written into a directory that container binds when it starts. That is a regression against the hard-coded list this replaced, and against "refuses rather than guesses". It now asks `ps --all --quiet` (docker, podman, nerdctl), and a runtime that knows of no container at all is not an answer: the check refuses. Both cases are covered by tests that drive the real `mountsFromRuntime` through an injected executor — nothing exercised it before, only `inspect` directly. While there: a mount that *contains* the home (a container binding /srv while the home is /srv/u) was dropped by the same under-home filter, so the guard was blind in the other direction too. Both sides compare for overlap now. `setup` reads the mount table it is standing on — it is in the container by definition — so a container leaving no vendor marker (plain containerd, a k8s pod) no longer hunts for a runtime it does not have; `isInsideContainer`'s gap is documented where it does double duty. The compose snippet's two sections also named different socket directories, one a host path and one a named volume, so the example shared no socket at all. --- AGENTS.md | 8 +- README.md | 13 ++-- examples/docker-compose.snippet.yml | 8 +- src/commands/setup.ts | 8 +- src/commands/support.ts | 111 ++++++++++++++++++++-------- test/unit/commands.test.ts | 52 ++++++++++++- 6 files changed, 151 insertions(+), 49 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5ce2eea..0b6a2f6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -86,9 +86,11 @@ sources must stay erasable (`erasableSyntaxOnly` is on: no enums, no parameter p user's to name, so there is no list to hard-code. A process inside the container reads its own mount table (`/proc/self/mountinfo`); a host process asks a container runtime — docker, podman or nerdctl, whichever is there — for the paths its containers - bind-mount, which is a superset on purpose: any container that can rewrite the broker's code is as good - a reason not to stage there as the agent is. When neither can answer, the check refuses rather than - guesses — the guess is what would put the code, or the key, somewhere a container can rewrite. + bind-mount, running or stopped, which is a superset on purpose: any container that can rewrite the + broker's code is as good a reason not to stage there as the agent is. A runtime that knows of no + container is not an answer — that is exactly when the check is blind, since the agent container may + simply be stopped with its mounts still configured — so it refuses rather than guesses, as it does + when nothing can answer at all. - **The process that owns a boundary decides what crosses it, not the error class.** A response body put into the message of a `ProviderConfigError` reached the container, because a caller that wraps that error to name a missing permission wraps it in the same class and so passes the whole message through. Detail diff --git a/README.md b/README.md index 792f258..aca45e8 100644 --- a/README.md +++ b/README.md @@ -116,12 +116,13 @@ docker compose -f "$DEPLOY/docker-compose.broker.yml" up -d ``` Nothing above has to name the shared directories. A host process asks the container runtime — docker, -podman or nerdctl, whichever is there — for the host paths its containers bind-mount; a process inside -the container reads `/proc/self/mountinfo`. When neither can answer — including a host whose only -runtime is plain `containerd`, which has no interface this knows — `init`, `stage` and `authorize` -refuse rather than guess. The runtime route is deliberately a superset: a directory bind-mounted into -*any* running container counts as shared, because any container that can rewrite the broker's code is -as good a reason not to stage there as the agent is. +podman or nerdctl, whichever is there — for the host paths its containers bind-mount, running or +stopped; a process inside the container reads `/proc/self/mountinfo`. When neither can answer — no +runtime, a runtime that knows of no container at all, or a host whose only runtime is plain +`containerd`, which has no interface this knows — `init`, `stage` and `authorize` refuse rather than +guess. The runtime route is deliberately a superset: a directory bind-mounted into *any* container +counts as shared, because any container that can rewrite the broker's code is as good a reason not to +stage there as the agent is. `stage` prints the commit it exported; check that sha against your own clone or the remote before trusting it, since the repository you export from is one an agent can write to. It leaves a diff --git a/examples/docker-compose.snippet.yml b/examples/docker-compose.snippet.yml index 05719d9..318b0ca 100644 --- a/examples/docker-compose.snippet.yml +++ b/examples/docker-compose.snippet.yml @@ -88,7 +88,7 @@ services: - ./log:/var/log/git-cred-broker # The socket directory, shared with the DSH container below. This is the only interface # between the two: no ports are published, and none should be added. - - /volume1/homes//Workspaces//.dsh/git-broker:/run/git-broker + - /srv/git-broker-socket:/run/git-broker ``` Pin a version (`npx --yes --package 'git-credential-broker@0.1.0'`) if you would rather not @@ -110,10 +110,8 @@ services: - GIT_BROKER_REQUIRE=1 volumes: # ... existing mounts ... - - git-cred-broker-run:/run/git-broker - -volumes: - git-cred-broker-run: + # The *same* host directory the broker binds, or the two are not sharing a socket at all. + - /srv/git-broker-socket:/run/git-broker ``` Then, inside the container: diff --git a/src/commands/setup.ts b/src/commands/setup.ts index be9b444..2f25152 100644 --- a/src/commands/setup.ts +++ b/src/commands/setup.ts @@ -27,6 +27,7 @@ import { gitConfigSet, insideMountedPath, listFlag, + mountedPaths, parseArgs, say, warn, @@ -114,7 +115,12 @@ export async function performSetup(input: SetupInput): Promise { } // The config must not be written into any directory the container can rewrite. Canonical, so the // host's spelling of a shared directory and the container's spelling of the same one are one place. - const mounted = insideMountedPath(input.gitconfig, input.home, input.mounts); + // `setup` runs in the container by definition, so the mount table it is standing on is the + // authority. Reading it directly means a container that leaves no marker for `isInsideContainer` + // (plain containerd, a Kubernetes pod) still gets the right answer instead of hunting for a runtime + // it does not have. + const mounts = input.mounts ?? mountedPaths(input.home, { container: true }); + const mounted = insideMountedPath(input.gitconfig, input.home, mounts); if (mounted) { throw new Error( `${input.gitconfig} is inside ${mounted}, which is mounted into the container; refusing to write ` + diff --git a/src/commands/support.ts b/src/commands/support.ts index ff085d2..e8d1e05 100644 --- a/src/commands/support.ts +++ b/src/commands/support.ts @@ -108,6 +108,13 @@ export function listFlag(args: Args, name: string): string[] { * inside the agent container. Docker and Podman each leave a marker; nothing here names a directory, * because no directory is this tool's to assume. * + * Those markers are also this function's known gap, and it does double duty. A container that leaves + * neither — plain containerd, a Kubernetes pod — reads as a host: the "run this on the host" refusal + * in `init` and `authorize` does not fire, and `mountedPaths` looks for a runtime instead of reading + * the mount table it is standing on. `setup`, which is in the container by definition, says so itself + * and reads the table. The refusal cases still fail closed afterwards, when no runtime can be asked, + * but they fail with a message about the runtime rather than about the container. + * * @returns true when running inside a container. */ export function isInsideContainer(): boolean { @@ -122,6 +129,8 @@ export interface MountSource { readonly inspect?: string | null; /** Whether this process is in the container, whose own mount table is the authority. */ readonly container?: boolean; + /** How to run a container runtime command, so a test need not have one installed. */ + readonly exec?: (cli: string, args: readonly string[]) => string; } /** @@ -167,7 +176,22 @@ function canonicalOrResolved(target: string): string { } /** - * The mount points at or under a home, read from a mount table. + * Whether one canonical directory contains the other. + * + * Both directions matter. A mount *under* the home is shared, and so is a mount that *contains* the + * home: a container that binds `/srv` while the home is `/srv/u` can rewrite everything in the home, + * so dropping that mount for sitting above the home is the same blind spot in the other direction. + * + * @param a - one canonical path. + * @param b - the other canonical path. + * @returns true when either contains the other. + */ +function overlaps(a: string, b: string): boolean { + return a === b || a.startsWith(`${b}${path.sep}`) || b.startsWith(`${a}${path.sep}`); +} + +/** + * The mount points that overlap a home, read from a mount table. * * The container is the thing that knows what it mounts. A list of directory names in this file would * be a guess about a home this code has never seen. @@ -176,21 +200,20 @@ function canonicalOrResolved(target: string): string { * @param home - the home to measure against. * @returns the mount points, spelled as the table spells them. */ -export function mountsUnderHome(mountinfo: string, home: string): string[] { +export function mountsTouchingHome(mountinfo: string, home: string): string[] { const root = canonicalOrResolved(home); const found: string[] = []; for (const line of mountinfo.split('\n')) { const field = line.split(' ')[4]; if (field === undefined) continue; const point = path.resolve(decodeMountField(field)); - const real = canonicalOrResolved(point); - if (real === root || real.startsWith(`${root}${path.sep}`)) found.push(point); + if (overlaps(canonicalOrResolved(point), root)) found.push(point); } return found; } /** - * The host paths a container runtime bound from the user's home. + * The host paths a container runtime bound that overlap the user's home. * * `docker inspect --format '{{json .Mounts}}'` prints one array per container. Only bind mounts count: * a volume or a tmpfs is not a directory the user named. Every container is considered, not only the @@ -198,10 +221,10 @@ export function mountsUnderHome(mountinfo: string, home: string): string[] { * — so this is deliberately a superset. * * @param output - the runtime's output, one array per line. - * @param home - the home to keep mounts under. + * @param home - the home to compare against. * @returns the host paths, unique and canonical. */ -export function boundPathsUnderHome(output: string, home: string): string[] { +export function boundPathsTouchingHome(output: string, home: string): string[] { const root = canonicalOrResolved(home); const found = new Set(); for (const line of output.split('\n')) { @@ -216,57 +239,81 @@ export function boundPathsUnderHome(output: string, home: string): string[] { for (const mount of mounts as Array<{ Type?: unknown; Source?: unknown }>) { if (mount.Type !== 'bind' || typeof mount.Source !== 'string') continue; const real = canonicalOrResolved(mount.Source); - if (real === root || real.startsWith(`${root}${path.sep}`)) found.add(real); + if (overlaps(real, root)) found.add(real); } } return [...found]; } +/** One runtime's answer: how many containers it knows of, and the host paths they bind near the home. */ +interface RuntimeAnswer { + readonly containers: number; + readonly mounts: string[]; +} + +/** Run a container runtime command; a failure means that runtime could not be asked. */ +function runtimeExec(cli: string, args: readonly string[]): string { + return execFileSync(cli, args, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }); +} + /** * One runtime's answer, or undefined when that command is not there to ask. * + * `--all`, not only the running containers: a stopped container's bind mounts are still configured and + * it can be started again at any time. This is most likely to run while the agent container is down — + * `init` on a fresh host, or a boot script — which is exactly when a running-only list is blind. + * * @param cli - the command to run. - * @param home - the home to keep mounts under. - * @returns the paths, or undefined when this command could not answer. + * @param home - the home to compare against. + * @param exec - how to run it. + * @returns the answer, or undefined when this command could not answer. */ -function mountsFromRuntime(cli: string, home: string): string[] | undefined { - const run = (args: readonly string[]): string => - execFileSync(cli, args, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }); +function mountsFromRuntime( + cli: string, + home: string, + exec: (cli: string, args: readonly string[]) => string, +): RuntimeAnswer | undefined { let ids: string; try { - ids = run(['ps', '--quiet']); + ids = exec(cli, ['ps', '--all', '--quiet']); } catch { return undefined; } - const running = ids.split('\n').map((id) => id.trim()).filter((id) => id !== ''); - if (running.length === 0) return []; + const known = ids.split('\n').map((id) => id.trim()).filter((id) => id !== ''); + if (known.length === 0) return { containers: 0, mounts: [] }; try { - return boundPathsUnderHome(run(['inspect', '--format', '{{json .Mounts}}', ...running]), home); + const listed = exec(cli, ['inspect', '--format', '{{json .Mounts}}', ...known]); + return { containers: known.length, mounts: boundPathsTouchingHome(listed, home) }; } catch { return undefined; } } /** - * Ask every container runtime on this host which directories it has bound from the user's home. + * Ask every container runtime on this host which directories it has bound near the user's home. * * The host's own mount table cannot answer this: a bind mount exists only in the container's * namespace, so the runtimes are asked instead. Their answers are unioned, because one host can run * more than one of them and a container under either is still a container that can rewrite the code. * - * @param home - the home to keep mounts under. - * @returns the paths, or undefined when no runtime was there to ask. + * A runtime that knows of no container at all is not an answer. That is precisely the state in which + * this check is blind — the agent container may simply be stopped, its mounts still configured — so it + * is treated as "cannot tell" and the caller refuses. + * + * @param home - the home to compare against. + * @param exec - how to run a runtime. + * @returns the paths, or undefined when no runtime could name a container. */ -function detectedMounts(home: string): string[] | undefined { +function detectedMounts(home: string, exec: (cli: string, args: readonly string[]) => string): string[] | undefined { const found = new Set(); - let answered = false; + let containers = 0; for (const cli of CONTAINER_CLIS) { - const mounts = mountsFromRuntime(cli, home); - if (mounts === undefined) continue; - answered = true; - for (const mount of mounts) found.add(mount); + const answer = mountsFromRuntime(cli, home, exec); + if (answer === undefined) continue; + containers += answer.containers; + for (const mount of answer.mounts) found.add(mount); } - return answered ? [...found] : undefined; + return containers === 0 ? undefined : [...found]; } /** @@ -285,17 +332,17 @@ export function mountedPaths(home: string = os.homedir(), source: MountSource = const container = source.container ?? isInsideContainer(); let mounts: string[] | undefined; if (source.mountinfo !== undefined) { - mounts = mountsUnderHome(source.mountinfo, home); + mounts = mountsTouchingHome(source.mountinfo, home); } else if (source.inspect !== undefined) { - mounts = source.inspect === null ? undefined : boundPathsUnderHome(source.inspect, home); + mounts = source.inspect === null ? undefined : boundPathsTouchingHome(source.inspect, home); } else if (container) { try { - mounts = mountsUnderHome(fs.readFileSync('/proc/self/mountinfo', 'utf8'), home); + mounts = mountsTouchingHome(fs.readFileSync('/proc/self/mountinfo', 'utf8'), home); } catch { mounts = undefined; } } else { - mounts = detectedMounts(home); + mounts = detectedMounts(home, source.exec ?? runtimeExec); } if (mounts === undefined) { @@ -303,7 +350,7 @@ export function mountedPaths(home: string = os.homedir(), source: MountSource = 'cannot tell which directories the container shares: ' + (container ? 'its own mount table could not be read' - : 'none of docker, podman or nerdctl could be asked') + + : 'none of docker, podman or nerdctl could name a container') + '; refusing rather than guessing', ); } diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index 4945444..1cf7779 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -120,7 +120,7 @@ describe('mounted path detection', () => { ]); }); - it('asks the container runtime on the host, and keeps only bind mounts under the home', () => { + it('asks the container runtime on the host, and keeps the bind mounts that touch the home', () => { const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-inspect-')); const inside = path.join(base, 'shared'); const deeper = path.join(base, 'sub', 'deep'); @@ -141,13 +141,61 @@ describe('mounted path detection', () => { assert.deepEqual( got, [inside, deeper].map((entry) => fs.realpathSync(entry)).sort(), - 'a volume and anything outside the home are not shared directories', + 'a volume and an unrelated path are not shared directories', ); } finally { fs.rmSync(base, { recursive: true, force: true }); } }); + it('asks for every container, not only the running ones', () => { + // A stopped container still has its bind mounts configured, and can be started again at any time. + // `init` runs on a fresh host and from boot scripts, which is exactly when a running-only list is + // blind. Regression: an empty runtime answer used to be trusted as "nothing is shared". + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-runtime-')); + const inside = path.join(base, 'shared'); + fs.mkdirSync(inside, { recursive: true }); + const calls: string[][] = []; + const exec = (cli: string, args: readonly string[]): string => { + calls.push([cli, ...args]); + if (args[0] === 'ps') return 'b3f1c2d4e5f6\n'; + return JSON.stringify([{ Type: 'bind', Source: inside, Destination: '/home/u/shared' }]); + }; + try { + assert.deepEqual( + mountedPaths(base, { container: false, exec }).map((entry) => fs.realpathSync(entry)), + [fs.realpathSync(inside)], + 'a stopped container is still a container that can rewrite the staged code', + ); + assert.equal(calls[0]?.includes('--all'), true, 'ps must include the containers that are not running'); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); + + it('counts a mount that contains the home, not only ones under it', () => { + // A container that binds the home's parent can rewrite everything in the home. + const base = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-ancestor-')); + const homeDir = path.join(base, 'u'); + fs.mkdirSync(homeDir, { recursive: true }); + const exec = (_cli: string, args: readonly string[]): string => + args[0] === 'ps' ? 'b3f1c2d4e5f6\n' : JSON.stringify([{ Type: 'bind', Source: base, Destination: '/mnt' }]); + try { + assert.deepEqual( + mountedPaths(homeDir, { container: false, exec }).map((entry) => fs.realpathSync(entry)), + [fs.realpathSync(base)], + ); + } finally { + fs.rmSync(base, { recursive: true, force: true }); + } + }); + + it('refuses when the runtime knows of no container at all', () => { + // Installed but empty is not "nothing is shared": it is the state in which the check is blind. + const exec = (_cli: string, args: readonly string[]): string => (args[0] === 'ps' ? '' : '[]'); + assert.throws(() => mountedPaths(home, { container: false, exec }), /could name a container/); + }); + it('refuses rather than guess when nothing can answer', () => { assert.throws( () => mountedPaths(home, { container: false, inspect: null }), From 45ffa7f5a2c3b669b517c296c69f2a0b5aba767e Mon Sep 17 00:00:00 2001 From: JounQin Date: Sun, 4 Oct 2026 02:07:27 +0000 Subject: [PATCH 09/10] fix: an unaskable runtime, and an unreadable mount, are both refusals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both real. `mountsFromRuntime` returned "cannot tell" for every kind of failure, and `detectedMounts` skipped it whenever another runtime answered — so a docker daemon that was down was invisible while podman was up, and a bind mount only docker knew about passed the guard. An absent command (ENOENT) is now distinguished from one that is installed and failing: the first is skipped, the second refuses the whole answer, because its containers are precisely the ones nothing else can see. `insideMountedPath` resolved each discovered mount with the strict `canonicalPath`, so a mount that answers EACCES — the Synology document-viewer case discovery already tolerates — aborted the check for a target anywhere on the host, with a raw filesystem error. The mount side takes the tolerant form now; the target stays strict, which is the half that must not be guessed. The host opener's documented scope also overreached: checkouts within `--depth` levels of a root (4 by default) are served, not "every checkout". The bound stays — the walk repeats every `--interval` seconds — so the words are fixed instead. --- AGENTS.md | 6 ++-- README.md | 6 ++-- src/commands/support.ts | 62 +++++++++++++++++++++++----------- test/unit/commands.test.ts | 27 ++++++++++++++- test/unit/host-request.test.ts | 2 +- 5 files changed, 77 insertions(+), 26 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0b6a2f6..08ffcb4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -174,8 +174,10 @@ sources must stay erasable (`erasableSyntaxOnly` is on: no enums, no parameter p is still the broker — `--via-host` is asked for, never assumed. The opener is started once per machine, not once per repository: it discovers the checkouts under - its `--root`s (default `$HOME`, so every checkout under it, including anything added later, is - served). The paths on the two sides never have to match — each side reads and writes its own view + its `--root`s (default `$HOME`) within `--depth` levels of each (4 by default), so anything added + later is picked up by the next sweep. The bound is deliberate — the walk repeats every `--interval` + seconds — and `--depth` raises it for a deeper layout. The paths on the two sides never have to + match — each side reads and writes its own view of the same `.git` directory — so only the roots it scans are host-side paths. A request is served in the checkout it was written in. That is a mechanism, not a boundary: the container can edit any checkout it can write, its remote included, so what a deployment controls is which roots are served and which program runs. diff --git a/README.md b/README.md index aca45e8..d84bf54 100644 --- a/README.md +++ b/README.md @@ -293,8 +293,10 @@ git-credential-broker pr --via-host --repo owner/repo --head feature --base main The opener creates pull requests with the credentials of whoever started it, which is the point; it never runs a shell, and the program it calls is configuration rather than a hard-coded `gh`: `--command /path/to/gh`, or `$GIT_BROKER_PR_COMMAND`. It cannot push, merge, close or read -anything. `--root` may be repeated and defaults to `$HOME`, so every checkout under it — including -ones created later — is served with no further setup. To start it once and keep it, +anything. `--root` may be repeated and defaults to `$HOME`; checkouts within `--depth` levels of a root +(4 by default) are served with no further setup, including ones created later, which the next sweep +picks up. The bound exists because the walk repeats every `--interval` seconds; raise `--depth` for +deeper layouts. To start it once and keep it, `examples/host-opener.service` is a systemd user unit; on a system without user services, `examples/host-opener-boot.sh` is the same thing for a DSM boot-up task — absolute paths, one instance at a time, and its output in a log file. Without `--via-host`, or with no opener running, `pr` behaves diff --git a/src/commands/support.ts b/src/commands/support.ts index e8d1e05..a486db9 100644 --- a/src/commands/support.ts +++ b/src/commands/support.ts @@ -245,11 +245,14 @@ export function boundPathsTouchingHome(output: string, home: string): string[] { return [...found]; } -/** One runtime's answer: how many containers it knows of, and the host paths they bind near the home. */ -interface RuntimeAnswer { - readonly containers: number; - readonly mounts: string[]; -} +/** + * What a runtime had to say: it is not installed (skip it), it is there and could not be asked (a + * blind spot, so the whole check refuses), or it answered. + */ +type RuntimeQuery = + | { readonly kind: 'absent' } + | { readonly kind: 'failed' } + | { readonly kind: 'ok'; readonly containers: number; readonly mounts: string[] }; /** Run a container runtime command; a failure means that runtime could not be asked. */ function runtimeExec(cli: string, args: readonly string[]): string { @@ -257,7 +260,21 @@ function runtimeExec(cli: string, args: readonly string[]): string { } /** - * One runtime's answer, or undefined when that command is not there to ask. + * Whether a failed spawn means the command is not installed, rather than installed and failing. + * + * The difference decides whether the guard can proceed at all. A host that has only docker must not be + * refused because podman is missing; a host whose docker daemon is unreachable must be, because its + * containers are precisely the ones nothing else can see. + * + * @param error - the error a spawn threw. + * @returns true when the command was not found at all. + */ +function notInstalled(error: unknown): boolean { + return (error as { code?: unknown } | null | undefined)?.code === 'ENOENT'; +} + +/** + * One runtime's answer. * * `--all`, not only the running containers: a stopped container's bind mounts are still configured and * it can be started again at any time. This is most likely to run while the agent container is down — @@ -266,26 +283,27 @@ function runtimeExec(cli: string, args: readonly string[]): string { * @param cli - the command to run. * @param home - the home to compare against. * @param exec - how to run it. - * @returns the answer, or undefined when this command could not answer. + * @returns what this runtime had to say. */ function mountsFromRuntime( cli: string, home: string, exec: (cli: string, args: readonly string[]) => string, -): RuntimeAnswer | undefined { +): RuntimeQuery { let ids: string; try { ids = exec(cli, ['ps', '--all', '--quiet']); - } catch { - return undefined; + } catch (error) { + return notInstalled(error) ? { kind: 'absent' } : { kind: 'failed' }; } const known = ids.split('\n').map((id) => id.trim()).filter((id) => id !== ''); - if (known.length === 0) return { containers: 0, mounts: [] }; + if (known.length === 0) return { kind: 'ok', containers: 0, mounts: [] }; try { const listed = exec(cli, ['inspect', '--format', '{{json .Mounts}}', ...known]); - return { containers: known.length, mounts: boundPathsTouchingHome(listed, home) }; + return { kind: 'ok', containers: known.length, mounts: boundPathsTouchingHome(listed, home) }; } catch { - return undefined; + // It listed containers a moment ago, so it is installed. Failing now is a blind spot, not absence. + return { kind: 'failed' }; } } @@ -296,20 +314,22 @@ function mountsFromRuntime( * namespace, so the runtimes are asked instead. Their answers are unioned, because one host can run * more than one of them and a container under either is still a container that can rewrite the code. * - * A runtime that knows of no container at all is not an answer. That is precisely the state in which - * this check is blind — the agent container may simply be stopped, its mounts still configured — so it - * is treated as "cannot tell" and the caller refuses. + * Two states are not answers, and both make the caller refuse. A runtime that knows of no container at + * all is one — the agent container may simply be stopped, its mounts still configured. A runtime that + * is installed but cannot be queried is the other: its containers are invisible, and another runtime's + * list cannot cover for them. * * @param home - the home to compare against. * @param exec - how to run a runtime. - * @returns the paths, or undefined when no runtime could name a container. + * @returns the paths, or undefined when they cannot be known. */ function detectedMounts(home: string, exec: (cli: string, args: readonly string[]) => string): string[] | undefined { const found = new Set(); let containers = 0; for (const cli of CONTAINER_CLIS) { const answer = mountsFromRuntime(cli, home, exec); - if (answer === undefined) continue; + if (answer.kind === 'absent') continue; + if (answer.kind === 'failed') return undefined; containers += answer.containers; for (const mount of answer.mounts) found.add(mount); } @@ -350,7 +370,7 @@ export function mountedPaths(home: string = os.homedir(), source: MountSource = 'cannot tell which directories the container shares: ' + (container ? 'its own mount table could not be read' - : 'none of docker, podman or nerdctl could name a container') + + : 'none of docker, podman or nerdctl could be asked, or none knows of a container') + '; refusing rather than guessing', ); } @@ -376,7 +396,9 @@ export function insideMountedPath( ): string | null { const resolved = canonicalPath(target); for (const mounted of mounts ?? mountedPaths(home)) { - const real = canonicalPath(mounted); + // The mount side takes the tolerant form: discovery may have kept a mount that answers EACCES in + // its lexical spelling, and one unreadable mount elsewhere must not fail a check about this target. + const real = canonicalOrResolved(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 1cf7779..e9bb7d2 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -193,7 +193,32 @@ describe('mounted path detection', () => { it('refuses when the runtime knows of no container at all', () => { // Installed but empty is not "nothing is shared": it is the state in which the check is blind. const exec = (_cli: string, args: readonly string[]): string => (args[0] === 'ps' ? '' : '[]'); - assert.throws(() => mountedPaths(home, { container: false, exec }), /could name a container/); + assert.throws( + () => mountedPaths(home, { container: false, exec }), + /could be asked, or none knows of a container/, + ); + }); + + it('treats a missing runtime as absent, but an installed failing one as a blind spot', () => { + const failure = (code: string): NodeJS.ErrnoException => Object.assign(new Error(code), { code }); + const one = (cli: string, args: readonly string[]): string => { + if (cli !== 'podman') throw failure('ENOENT'); + return args[0] === 'ps' ? 'id-1\n' : JSON.stringify([{ Type: 'bind', Source: '/srv', Destination: '/srv' }]); + }; + assert.deepEqual( + mountedPaths('/srv/u', { container: false, exec: one }), + ['/srv'], + 'a host with one runtime must not be refused because the others are not installed', + ); + const unreachable = (cli: string, args: readonly string[]): string => { + if (cli === 'docker') throw failure('ECONNREFUSED'); + return one(cli, args); + }; + assert.throws( + () => mountedPaths('/srv/u', { container: false, exec: unreachable }), + /could be asked, or none knows of a container/, + 'a runtime that is installed but unreachable hides its containers, so another list cannot cover for it', + ); }); it('refuses rather than guess when nothing can answer', () => { diff --git a/test/unit/host-request.test.ts b/test/unit/host-request.test.ts index c7cfa4a..33fa9f1 100644 --- a/test/unit/host-request.test.ts +++ b/test/unit/host-request.test.ts @@ -185,7 +185,7 @@ describe('reading a pull request URL out of a program', () => { }); describe('finding the checkouts to serve', () => { - it('finds every checkout under a root, at any depth, hidden directories included', () => { + it('finds the checkouts within the given depth, hidden directories included', () => { const root = mkdtempSync(join(tmpdir(), 'discover-test-')); for (const repo of ['.hidden', 'trees/one', 'trees/two/deep', 'plain']) { mkdirSync(join(root, repo, '.git'), { recursive: true }); From fdd2cd35f32153ceae7f034d638583b712fbac6e Mon Sep 17 00:00:00 2001 From: JounQin Date: Sun, 4 Oct 2026 03:59:36 +0000 Subject: [PATCH 10/10] fix: a container that binds the host root is inside everything MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `overlaps` and `insideMountedPath` both compared with `startsWith(parent + sep)`, and for a root that is `//`, so `/` was treated as containing nothing. That is the nit @i1stG raised, and it was in two places — the second worse than the first: discovery kept the root bind and the guard then never matched it, so the mount was in the list and every target still passed. The ordinary prefix stays for everything else. The container's own root filesystem — which every mount table lists and the image made — is skipped in the mount-table path, or `setup` would refuse its own `~/.gitconfig`; a bind of the host root arrives through the runtime, where it counts. --- src/commands/support.ts | 40 +++++++++++++++++++++++++++++++++++--- test/unit/commands.test.ts | 24 +++++++++++++++++++++++ 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/src/commands/support.ts b/src/commands/support.ts index a486db9..926e5ce 100644 --- a/src/commands/support.ts +++ b/src/commands/support.ts @@ -175,6 +175,37 @@ function canonicalOrResolved(target: string): string { } } +/** + * Whether a path is the root of its filesystem. + * + * Every mount table lists the container's own root filesystem, and that root is the image's, not a + * directory the host handed over. Counting it as shared would put every path "inside a mount" and + * refuse `setup` its own config; a *bind* of the host root arrives through the runtime instead, where + * it does count. + * + * @param target - the path to test. + * @returns true when it is a filesystem root. + */ +function filesystemRoot(target: string): boolean { + const resolved = path.resolve(target); + return resolved === path.parse(resolved).root; +} + +/** + * Whether one canonical directory contains another. + * + * The root is special in its spelling and nothing else: `'/'.startsWith('/' + '/')` is false, so the + * ordinary prefix test says the host root contains nothing at all. + * + * @param parent - the directory that would contain. + * @param child - the path it would contain. + * @returns true when parent holds child. + */ +function contains(parent: string, child: string): boolean { + if (child === parent) return true; + return filesystemRoot(parent) ? child.startsWith(parent) : child.startsWith(`${parent}${path.sep}`); +} + /** * Whether one canonical directory contains the other. * @@ -187,7 +218,7 @@ function canonicalOrResolved(target: string): string { * @returns true when either contains the other. */ function overlaps(a: string, b: string): boolean { - return a === b || a.startsWith(`${b}${path.sep}`) || b.startsWith(`${a}${path.sep}`); + return contains(a, b) || contains(b, a); } /** @@ -207,6 +238,8 @@ export function mountsTouchingHome(mountinfo: string, home: string): string[] { const field = line.split(' ')[4]; if (field === undefined) continue; const point = path.resolve(decodeMountField(field)); + // The container's own root filesystem is in every mount table, and it is not a shared directory. + if (filesystemRoot(point)) continue; if (overlaps(canonicalOrResolved(point), root)) found.push(point); } return found; @@ -398,8 +431,9 @@ export function insideMountedPath( for (const mounted of mounts ?? mountedPaths(home)) { // The mount side takes the tolerant form: discovery may have kept a mount that answers EACCES in // its lexical spelling, and one unreadable mount elsewhere must not fail a check about this target. - const real = canonicalOrResolved(mounted); - if (resolved === real || resolved.startsWith(`${real}${path.sep}`)) return mounted; + // `contains`, not a prefix test, or a container binding the host root would be discovered and then + // never matched — `'/x'.startsWith('//')` is false. + if (contains(canonicalOrResolved(mounted), resolved)) return mounted; } return null; } diff --git a/test/unit/commands.test.ts b/test/unit/commands.test.ts index e9bb7d2..0dedad0 100644 --- a/test/unit/commands.test.ts +++ b/test/unit/commands.test.ts @@ -190,6 +190,30 @@ describe('mounted path detection', () => { } }); + it('counts a container that binds the host root, which contains everything', () => { + // The root is where the prefix test used to fail: `'/'.startsWith('//')` is false, so a bind of + // the host root was dropped even though such a container can rewrite every path on the host. + const root = path.parse(path.resolve(path.sep)).root; + const exec = (_cli: string, args: readonly string[]): string => + args[0] === 'ps' ? 'b3f1c2d4e5f6\n' : JSON.stringify([{ Type: 'bind', Source: root, Destination: '/host' }]); + assert.deepEqual(mountedPaths(home, { container: false, exec }), [root]); + assert.equal( + insideMountedPath(`${home}/Workspaces/x`, home, [root]), + root, + 'and the guard refuses a target under it, rather than matching nothing', + ); + }); + + it("ignores the container's own root filesystem, which the image made", () => { + // Every mount table lists it. Counting it would put every path inside a mount and refuse `setup` + // its own `~/.gitconfig`. A *bind* of the host root arrives through the runtime, above. + const table = [ + '675 573 0:36 / / rw,relatime - overlay overlay rw', + `676 573 0:36 /x ${home}/shared rw - btrfs /dev/x rw`, + ].join('\n'); + assert.deepEqual(mountedPaths(home, { mountinfo: table, container: true }), [`${home}/shared`]); + }); + it('refuses when the runtime knows of no container at all', () => { // Installed but empty is not "nothing is shared": it is the state in which the check is blind. const exec = (_cli: string, args: readonly string[]): string => (args[0] === 'ps' ? '' : '[]');