From 15d6d6c0c683607fd9079c89e3c7deb3e647bf01 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 00:42:54 -0400 Subject: [PATCH 01/31] Stop job output from injecting phantom jobs The job-start detector matched "Running job: " anywhere in a worker's output, and a worker's output is untrusted: it includes whatever the job prints and whatever the runner echoes of the job's own contextData. The merge commit for #35 contains that literal text in its message, so checking it out made localmost read three job starts that never happened - each one taking a history entry that stays "running" forever, firing a Job Started notification, marking the worker busy, disarming its acquire deadline and rebinding its docker socket. Any repository could do this deliberately with a commit message, branch name or PR title. Anchor the match to the start of a line, behind the runner's own optional timestamp, and refuse any start on a worker that already has a job: the runner is --once, so one spawn is exactly one job and a second start is always output, never a job. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/main/runner-manager.test.ts | 44 +++++++++++++++++++++++++++++++++ src/main/runner-manager.ts | 21 ++++++++++++---- 2 files changed, 60 insertions(+), 5 deletions(-) diff --git a/src/main/runner-manager.test.ts b/src/main/runner-manager.test.ts index 603408f..4f97fe0 100644 --- a/src/main/runner-manager.test.ts +++ b/src/main/runner-manager.test.ts @@ -1530,3 +1530,47 @@ describe('docker access', () => { expect(stamp({ run: { images: ['postgres:16'] } })).toEqual(stamp({ run: { images: ['postgres:16'] } })); }); }); + +describe('job-start detection against injected output', () => { + const startedNames = (events: JobEvent[]) => events.filter((e) => e.type === 'started').map((e) => e.jobName); + + const setup = () => { + const events: JobEvent[] = []; + const manager = new RunnerManager({ + onLog: jest.fn(), + onStatusChange: jest.fn(), + onJobHistoryUpdate: jest.fn(), + onJobEvent: (e: JobEvent) => events.push(e), + }); + const helper = new RunnerManagerTestHelper(manager); + helper.setInstance(1, { name: 'runner-1', status: 'listening' }); + return { helper, events }; + }; + + it('ignores "Running job:" embedded in a line the job merely printed', async () => { + const { helper, events } = setup(); + + // A commit message, PR title or any echoed text can carry this. Here it + // arrives the way it really did: inside the job's contextData JSON. + await helper.parseRunnerOutput(1, '{"k":"message","v":"fix: match the `Running job: ` line properly"}'); + + expect(startedNames(events)).toEqual([]); + }); + + it('ignores a second job start on a worker already running one', async () => { + const { helper, events } = setup(); + + await helper.parseRunnerOutput(1, 'Running job: build'); + // The runner is --once: one spawn runs exactly one job, so anything after + // the first start is not a job, whatever it calls itself. + await helper.parseRunnerOutput(1, 'Running job: evil'); + + expect(startedNames(events)).toEqual(['build']); + }); + + it('still detects a genuine job start', async () => { + const { helper, events } = setup(); + await helper.parseRunnerOutput(1, 'Running job: build'); + expect(startedNames(events)).toEqual(['build']); + }); +}); diff --git a/src/main/runner-manager.ts b/src/main/runner-manager.ts index 0dfb7ae..e7f1f24 100644 --- a/src/main/runner-manager.ts +++ b/src/main/runner-manager.ts @@ -1419,14 +1419,25 @@ export class RunnerManager { return; } - // Detect job start - const jobStartMatch = line.match(/Running job:\s*(.+)/i); + // Detect job start. + // + // Anchored, because this reads the job's own output: any text a job prints + // can contain "Running job: x" - a commit message, a PR title, a checked-out + // file - and an unanchored match turned that into a phantom job, complete + // with history entry, notification, and a worker marked busy. The runner + // emits this at the start of a line, optionally behind its own timestamp. + const jobStartMatch = line.match(/^\s*(?:\d{4}-\d{2}-\d{2}[T ][\d:.]+Z?:?\s*)?Running job:\s*(.+?)\s*$/i); if (jobStartMatch) { const jobName = jobStartMatch[1].trim(); - // Avoid duplicate job start detection - if (instance.status === 'busy' && instance.currentJob?.name === jobName) { - this.log('debug', `[instance ${instanceNum}] Ignoring duplicate job start: ${jobName}`); + // A worker runs with --once: one spawn is exactly one job. So a start on + // a worker that already has a job is never a second job - it is the job's + // output echoing something that looks like one. + if (instance.status === 'busy' || instance.currentJob) { + this.log( + 'debug', + `[instance ${instanceNum}] Ignoring job start while already running ${instance.currentJob?.name ?? 'a job'}: ${jobName}` + ); return; } From be40a7d47f502e52e9e3af25b1e0cff4dc3d6835 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 08:12:51 -0400 Subject: [PATCH 02/31] Close two filter bypasses: upgrade tunnelling and case-folded keys Both let a job reach the daemon unfiltered, and both are exploited in tests that fail before the fix. An Upgrade header on any permitted request opened a raw pipe. handleUpgrade forwarded whatever passed the policy check, and GET /_ping is in the always-on baseline, so a job could ask to upgrade a ping and then pipeline arbitrary Docker API calls down the resulting socket - the test smuggles a denied GET /containers/json over it and gets 200 from the daemon. Only attach is an upgrade now, and the pipe is established only once the daemon has actually answered 101; anything else is relayed and closed. Separately, the evaluator read HostConfig and its fields with case-sensitive property access while the daemon decodes them with Go's encoding/json, which falls back to a case-insensitive field match. A body saying "hostconfig": {"privileged":true,"binds":["/:/host:rw"]} was invisible to every gate and honoured in full by the daemon. Keys are now read the way the daemon reads them: every casing of a gated key must pass, every casing of Image must name a declared image, and the most restrictive NetworkMode wins. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/main/docker/docker-evaluator.test.ts | 43 +++++++++++ src/main/docker/docker-evaluator.ts | 84 +++++++++++++++------ src/main/docker/docker-filter-proxy.test.ts | 55 ++++++++++++++ src/main/docker/docker-filter-proxy.ts | 49 +++++++++++- 4 files changed, 207 insertions(+), 24 deletions(-) diff --git a/src/main/docker/docker-evaluator.test.ts b/src/main/docker/docker-evaluator.test.ts index 03a4091..1e70650 100644 --- a/src/main/docker/docker-evaluator.test.ts +++ b/src/main/docker/docker-evaluator.test.ts @@ -334,3 +334,46 @@ describe('policy hints', () => { } }); }); + +describe('Go case-insensitive JSON decoding', () => { + // The daemon decodes the create body with Go's encoding/json, which matches + // struct fields case-insensitively as a documented fallback. So a key the + // filter reads as absent is honoured by the daemon: every HostConfig gate is + // bypassed by changing one letter. + const p = { run: { images: ['postgres:16'], mounts: [{ path: './', mode: 'ro' as const }], network: 'bridge' } }; + + it('refuses a lowercased HostConfig carrying privileged and a root bind', () => { + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { + Image: 'postgres:16', + hostconfig: { privileged: true, binds: ['/:/host:rw'], pidmode: 'host' }, + }), + ctx(p) + ); + expect(v.allowed).toBe(false); + }); + + it('refuses odd casings of the gated keys inside a correctly-cased HostConfig', () => { + for (const hostConfig of [ + { Privileged: true }, + { PRIVILEGED: true }, + { privileged: true }, + { BINDS: ['/etc:/x'] }, + { binds: ['/etc:/x'] }, + { networkmode: 'host' }, + { NETWORKMODE: 'host' }, + { pidMode: 'host' }, + { devices: [{ PathOnHost: '/dev/kmsg' }] }, + ]) { + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { Image: 'postgres:16', HostConfig: hostConfig }), + ctx(p) + ); + expect([JSON.stringify(hostConfig), v.allowed]).toEqual([JSON.stringify(hostConfig), false]); + } + }); + + it('still permits a correctly-cased create the policy allows', () => { + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: 'postgres:16' }), ctx(p)).allowed).toBe(true); + }); +}); diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index 584144c..c750f55 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -55,6 +55,31 @@ const deny = (reason: string, policyHint?: string): DockerVerdict => // OWN containers, which is enforced per id below. const BASELINE: ReadonlySet = new Set(['ping', 'version', 'info']); +/** + * Every value whose key case-insensitively equals `name`. + * + * The daemon decodes these bodies with Go's encoding/json, which matches a + * struct field by exact name and then, as a documented fallback, case + * -insensitively. Reading `hostConfig.Privileged` in JS therefore sees nothing + * in a body that says "privileged", while the daemon honours it - so the + * filter must consider every casing, not the one it expects. + */ +function valuesFor(obj: Record, name: string): unknown[] { + const wanted = name.toLowerCase(); + const out: unknown[] = []; + for (const [key, value] of Object.entries(obj)) { + if (key.toLowerCase() === wanted) out.push(value); + } + return out; +} + +/** The value the daemon would use: the exact-cased key if present, else any case-insensitive match. */ +function pick(obj: Record, name: string): unknown { + if (Object.prototype.hasOwnProperty.call(obj, name)) return obj[name]; + const matches = valuesFor(obj, name); + return matches.length > 0 ? matches[0] : undefined; +} + const isPlainObject = (value: unknown): value is Record => typeof value === 'object' && value !== null && !Array.isArray(value); @@ -215,8 +240,8 @@ function parseMount(mount: unknown): MountRequest | string | null { function collectMounts(hostConfig: Record): MountRequest[] | string { const requests: MountRequest[] = []; - const binds = hostConfig.Binds; - if (!isUnset(binds)) { + for (const binds of valuesFor(hostConfig, 'Binds')) { + if (isUnset(binds)) continue; if (!Array.isArray(binds)) return 'HostConfig.Binds must be an array'; for (const bind of binds) { if (typeof bind !== 'string') return 'each entry of HostConfig.Binds must be a string'; @@ -225,8 +250,8 @@ function collectMounts(hostConfig: Record): MountRequest[] | st requests.push(parsed); } } - const mounts = hostConfig.Mounts; - if (!isUnset(mounts)) { + for (const mounts of valuesFor(hostConfig, 'Mounts')) { + if (isUnset(mounts)) continue; if (!Array.isArray(mounts)) return 'HostConfig.Mounts must be an array'; for (const mount of mounts) { const parsed = parseMount(mount); @@ -286,12 +311,13 @@ function evaluateCreate(req: DockerRequest, ctx: DockerEvalContext, policy: Dock return deny('the repository docker policy declares no run action', image ? hints.image(image) : hints.run); } - const hostConfig = body.HostConfig ?? {}; + const hostConfig = pick(body, 'HostConfig') ?? {}; if (!isPlainObject(hostConfig)) return deny('HostConfig must be an object'); // Host-reaching settings first: none of these can be permitted by policy, // so the verdict does not depend on anything else in the request. - if (hostConfig.Privileged === true) { + const privilegedValues = valuesFor(hostConfig, 'Privileged'); + if (privilegedValues.some((v) => v === true)) { if (!policy.privileged) { return deny( 'privileged containers are not declared in the repository docker policy; `privileged: true` requires a managed VM backend', @@ -301,33 +327,47 @@ function evaluateCreate(req: DockerRequest, ctx: DockerEvalContext, policy: Dock if (!ctx.supportsPrivileged) { return deny('the repository docker policy declares privileged, which requires a managed VM backend; this daemon is not one'); } - } else if (!isUnset(hostConfig.Privileged) && hostConfig.Privileged !== false) { + } else if (privilegedValues.some((v) => !isUnset(v) && v !== false)) { return deny('HostConfig.Privileged must be a boolean'); } for (const gate of HOST_CONFIG_GATES) { - if (!gate.permitted(hostConfig[gate.key])) { + // Every casing must pass: one that does not is a value the daemon honours. + if (!valuesFor(hostConfig, gate.key).every((v) => gate.permitted(v))) { return deny(`${gate.flag} (HostConfig.${gate.key}) reaches the host and cannot be permitted by policy`); } } // Image. - if (typeof body.Image !== 'string' || body.Image === '') return deny('container create requires an Image'); - const wanted = normalizeImage(body.Image); - if (!(policy.run.images ?? []).some((declared) => normalizeImage(declared) === wanted)) { - return deny( - `image "${body.Image}" is not declared in the repository docker policy (run.images)`, - hints.image(body.Image) - ); + const imageValues = valuesFor(body, 'Image'); + const image = pick(body, 'Image'); + if (typeof image !== 'string' || image === '') return deny('container create requires an Image'); + // Every casing must name a declared image: the daemon uses one of them, and + // which one is not worth depending on. + for (const candidate of imageValues) { + if (typeof candidate !== 'string' || candidate === '') return deny('container create requires an Image'); + const wanted = normalizeImage(candidate); + if (!(policy.run.images ?? []).some((declared) => normalizeImage(declared) === wanted)) { + return deny( + `image "${candidate}" is not declared in the repository docker policy (run.images)`, + hints.image(candidate) + ); + } } // Network. Absent, empty and "default" are the daemon default, bridge. - const rawMode = hostConfig.NetworkMode; - let mode: string; - if (isUnset(rawMode) || rawMode === '' || rawMode === 'default') mode = 'bridge'; - else if (typeof rawMode === 'string') mode = rawMode; - else return deny('HostConfig.NetworkMode must be a string'); - if (mode === 'host' || mode.startsWith('container:')) { - return deny(`--network=${mode} (HostConfig.NetworkMode) reaches the host and cannot be permitted by policy`); + const modeValues = valuesFor(hostConfig, 'NetworkMode'); + const rawModes: unknown[] = modeValues.length > 0 ? modeValues : [undefined]; + let mode = 'bridge'; + for (const rawMode of rawModes) { + let candidate: string; + if (isUnset(rawMode) || rawMode === '' || rawMode === 'default') candidate = 'bridge'; + else if (typeof rawMode === 'string') candidate = rawMode; + else return deny('HostConfig.NetworkMode must be a string'); + if (candidate === 'host' || candidate.startsWith('container:')) { + return deny(`--network=${candidate} (HostConfig.NetworkMode) reaches the host and cannot be permitted by policy`); + } + // The most restrictive reading wins when casings disagree. + if (candidate !== 'bridge') mode = candidate; } if (mode !== 'none' && mode !== policy.run.network) { return deny( diff --git a/src/main/docker/docker-filter-proxy.test.ts b/src/main/docker/docker-filter-proxy.test.ts index 5d8a630..ceb1914 100644 --- a/src/main/docker/docker-filter-proxy.test.ts +++ b/src/main/docker/docker-filter-proxy.test.ts @@ -610,3 +610,58 @@ describe('container ownership tracking', () => { expect((await request(sock, 'GET', '/v1.45/containers/theirs999/json')).status).toBe(403); }); }); + +const rawUpgrade = (sock: string, method: string, url: string): Promise<{ head: string; socket: net.Socket }> => + new Promise((resolve, reject) => { + const socket = net.connect(sock); + let buffered = ''; + const onData = (data: Buffer) => { + buffered += data.toString(); + const end = buffered.indexOf('\r\n\r\n'); + if (end === -1) return; + socket.off('data', onData); + resolve({ head: buffered.slice(0, end), socket }); + }; + socket.on('data', onData); + socket.on('error', reject); + socket.on('connect', () => { + socket.write(`${method} ${url} HTTP/1.1\r\nHost: docker\r\nConnection: Upgrade\r\nUpgrade: tcp\r\n\r\n`); + }); + }); + +describe('upgrade requests', () => { + it('does not turn a permitted baseline read into a raw daemon tunnel', async () => { + const dir = tmp(); + const daemon = await fakeDaemon(dir); + const { proxy, sock } = await startProxy(dir, { backend: backendWith(daemon.sock, dir) }); + proxy.bind('owner/repo', { run: { images: ['postgres:16'], network: 'bridge' } }); + + // GET /_ping is in the always-on baseline, so the policy permits it. If an + // Upgrade header alone opens a raw pipe, the job holds an unfiltered socket + // to the daemon and can pipeline anything over it. + const { head, socket } = await rawUpgrade(sock, 'GET', '/v1.45/_ping'); + expect(head).not.toMatch(/101/); + + // Prove no tunnel: a denied request written on the same socket must not be + // answered by the daemon. + const smuggled = await new Promise((resolve) => { + let got = ''; + socket.on('data', (d: Buffer) => { got += d.toString(); }); + socket.write('GET /v1.45/containers/json HTTP/1.1\r\nHost: docker\r\n\r\n'); + setTimeout(() => resolve(got), 300); + }); + socket.destroy(); + expect(smuggled).not.toMatch(/"ok"\s*:\s*true|Names|\[\s*\{/); + }); + + it('refuses an upgrade on a container the socket does not own', async () => { + const dir = tmp(); + const daemon = await fakeAttachDaemon(dir); + const { proxy, sock } = await startProxy(dir, { backend: backendWith(daemon.sock, dir) }); + proxy.bind('owner/repo', { run: { images: ['postgres:16'], network: 'bridge' } }); + + const { head, socket } = await attach(sock, '/v1.45/containers/theirs999/attach?stream=1'); + socket.destroy(); + expect(head).not.toMatch(/101/); + }); +}); diff --git a/src/main/docker/docker-filter-proxy.ts b/src/main/docker/docker-filter-proxy.ts index 37c1151..1aa4461 100644 --- a/src/main/docker/docker-filter-proxy.ts +++ b/src/main/docker/docker-filter-proxy.ts @@ -49,6 +49,9 @@ const MAX_JSON_BODY_BYTES = 1024 * 1024; /** How much of an upload is drained so an early answer reaches the client, before the connection is cut. */ const MAX_DRAIN_BYTES = 8 * MAX_JSON_BODY_BYTES; +/** A response head larger than this is not an upgrade handshake. */ +const MAX_UPGRADE_HEAD_BYTES = 64 * 1024; + /** Hop-by-hop headers: each leg of the relay decides these for itself. */ const HOP_BY_HOP = ['connection', 'keep-alive', 'proxy-connection']; @@ -582,6 +585,18 @@ export class DockerFilterProxy { const url = req.url ?? '/'; const parsed = parseDockerRequest({ method, url, headers, body: Buffer.alloc(0) }); + // Only attach is an upgrade. Without this, any request the policy permits + // - including a baseline /_ping - could be sent with an Upgrade header to + // open a raw pipe to the daemon, and everything pipelined over that pipe + // would bypass the filter entirely. + const action = classifyDockerRequest(parsed); + if (action !== 'attach') { + const message = `${parsed.method} ${parsed.path} cannot be upgraded through the localmost docker socket`; + this.onLog({ level: 'info', message: `denied upgrade ${parsed.method} ${parsed.path}: ${message}` }); + this.refuseRaw(client, 400, message); + return; + } + const refusal = this.decide(parsed); if (refusal) { this.refuseRaw(client, refusal.status, refusal.message); @@ -605,8 +620,38 @@ export class DockerFilterProxy { } upstream.write(lines.join('\r\n') + '\r\n\r\n'); if (head.length > 0) upstream.write(head); - upstream.pipe(client); - client.pipe(upstream); + + // Pipe only once the daemon has actually agreed to upgrade. Piping on + // connect would hand the job a raw socket even when the daemon answered + // with an ordinary response, which is a tunnel by another name. + let banner = ''; + const onUpstreamHead = (chunk: Buffer): void => { + banner += chunk.toString('latin1'); + const end = banner.indexOf('\r\n\r\n'); + if (end === -1) { + // A daemon that never finishes a response head is not upgrading. + if (banner.length > MAX_UPGRADE_HEAD_BYTES) { + upstream.destroy(); + client.destroy(); + } + return; + } + upstream.off('data', onUpstreamHead); + + const statusLine = banner.slice(0, banner.indexOf('\r\n')); + if (!/^HTTP\/1\.[01] 101\b/.test(statusLine)) { + // Relay what the daemon said, then close. No raw pipe is established. + client.write(Buffer.from(banner, 'latin1')); + client.end(); + upstream.destroy(); + return; + } + + client.write(Buffer.from(banner, 'latin1')); + upstream.pipe(client); + client.pipe(upstream); + }; + upstream.on('data', onUpstreamHead); }); this.connections.add(upstream); upstream.on('close', () => this.connections.delete(upstream)); From 37379f295c8e721cef1e1b82dd786270f86225bf Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 09:12:03 -0400 Subject: [PATCH 03/31] Design the three docker endpoint families a real consumer needs An addendum to the stage 1 design, prompted by wiring a container-heavy repository to the shipped filter. Networks, image existence reads, and stopping an owned container all classify as `other` today and are denied. Networks matter most because the direction is backwards: an --internal network makes a container less reachable, so denying it forces strictly weaker isolation than the workload wants. The grammar declares a name glob and whether the network is internal; the driver stays unnameable, since macvlan on the physical LAN is worse than --network=host, and any create key the filter does not recognise is refused. NetworkMode must then accept an owned network, or the feature cannot be used at all. Image reads are scoped by the policy rather than by a second ownership ledger: inspecting an image run.images already names discloses nothing new, and the container ledger has already produced one defect. kill, stop and logs join the run action with the existing own-container scoping. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- .../2026-09-06-docker-endpoint-families.md | 147 ++++++++++++++++++ 1 file changed, 147 insertions(+) create mode 100644 docs/superpowers/specs/2026-09-06-docker-endpoint-families.md diff --git a/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md new file mode 100644 index 0000000..acfee7e --- /dev/null +++ b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md @@ -0,0 +1,147 @@ +# Docker Filter — Three Endpoint Families a Real Consumer Needs + +An addendum to +[2026-09-05-docker-isolation-design.md](./2026-09-05-docker-isolation-design.md). +Everything here extends the stage 1 filter; nothing here changes its shape. + +> **Status:** design. Prompted by wiring a container-heavy repository (an agent +> eval harness) to the shipped filtering socket — the "end-to-end run on a +> repository that needs the daemon" the original Testing section asks for. + +## Problem + +`pull`, `run` and `build` covered that consumer's pulls, container lifecycle and +image builds unchanged, which is the encouraging part. Three families it needs +classify as `other` and hit `default: deny`: + +1. **Networks.** The harness creates an `--internal` network — no route to + anything — as its *sealing* mechanism: the agent under test runs with no + egress except a broker that accounts for every request. It then reads and + deletes that network. +2. **Image existence.** `docker image inspect` is the natural "do I already have + this?" check, deciding build-vs-pull in a build-once-mount flow. +3. **Killing a container.** Enforcing a wall-clock budget on a container that + overruns it. + +The first is the one that matters most, because the direction is backwards. An +`--internal` network makes a container *less* reachable, not more. With networks +denied, the only containers a job can run are ones on the default bridge — the +filter currently **forces strictly weaker isolation than the workload wants**, +which is the opposite of what a sandbox should do. `run.network` does not help: +it constrains `NetworkMode` at create, and the network has to exist first. + +## Solution + +Three additions, each reusing a mechanism the filter already has. + +### 1. Networks: a declared, owned, bridge-only network + +```yaml +shared: + docker: + run: + networks: + - name: vk-* + internal: true +``` + +`name` is a glob matched against the requested network name. `internal` is the +only other key, and it is a **requirement, not a default**: a policy that wants a +routable network must say `internal: false`, so the approval diff shows it. + +**The driver is unnameable, and that is the point.** The dangerous value in a +network create is not `internal: false` — it is `Driver`. A `macvlan` or `ipvlan` +network puts the container directly on the physical LAN, which is worse than +`--network=host`, and `Options` can carry +`com.docker.network.bridge.host_binding_ipv4`. So the grammar cannot spell a +driver at all: the filter forces `bridge`, and **refuses any create body key it +does not recognise**. That is the same allowlist-of-the-grammar principle the +original spec applies to `HostConfig`, applied to a second body. + +Recognised keys on `POST /networks/create`: `Name`, `Internal`, `CheckDuplicate`, +`Labels`. `Driver` is permitted only when absent or exactly `bridge`. Everything +else — `IPAM`, `Options`, `Attachable`, `Ingress`, `ConfigOnly`, `ConfigFrom`, +`EnableIPv6`, `Scope` — is refused, naming the key. + +`GET /networks/{id}` and `DELETE /networks/{id}` are scoped to networks this +socket created, exactly as per-container endpoints are scoped to containers it +created. `GET /networks` (list) stays denied: it enumerates the daemon. + +**`NetworkMode` must accept an owned network.** This is the part that is easy to +miss and makes the feature useless without it. Today `evaluateCreate` requires +`HostConfig.NetworkMode` to equal `policy.run.network`. A job that creates +`vk-abc` and runs a container with `--network vk-abc` would still be refused. So +the create gate permits a `NetworkMode` that names a network in the socket's +owned set, in addition to the declared `run.network`. + +### 2. Image reads, scoped by the policy rather than by ownership + +`GET /images/{name}/json` is permitted when the reference normalises to an entry +in `run.images`. + +The consumer suggested scoping this the way containers are scoped — to images the +socket pulled or built. Policy-scoping is better here: an inspect of an image the +policy *already names* discloses nothing the policy has not already granted, and +it avoids a second ownership ledger. Ownership bookkeeping is not free — the +container ledger has already produced one defect (a prefix match that outlives +the container it described), and a second one would need to reconcile pulls by +tag with builds by id. + +`GET /images/json` (list) and `DELETE /images/{name}` stay denied: both are +daemon-wide, and the consumer agrees. + +### 3. Stopping a container the job owns + +`POST /containers/{id}/kill` and `POST /containers/{id}/stop` join +`start`/`attach`/`wait`/`remove` under the `run` action, with the same +own-container scoping. + +`stop` is not in the request but belongs in the same change: a timeout path that +can only `kill` is worse than one that can ask politely first, and both are the +same endpoint family with the same scoping. + +`GET /containers/{id}/logs` joins them too. The original spec's baseline is +"reads about the job's own containers", and logs is exactly such a read; refusing +it contradicts the documented behaviour rather than implementing it. + +## What stays denied + +`GET /containers/json`, `GET /networks`, `GET /images/json` and +`DELETE /images/{name}` are daemon-wide by construction — they enumerate or +mutate things outside the job — and no policy key grants them. + +## Not a filter change + +Mounts and build contexts must resolve inside the job workspace. A consumer +building from `tempfile.mkdtemp()` (i.e. `/var/folders/...`) fails that check +**correctly**; pointing `TMPDIR` inside the workspace is the consumer's fix. It +is recorded here only because it reads like a filter bug from the outside, and +the denial message should make the reason obvious enough that it doesn't. + +## Testing + +Per family, and in the same executable-escape style as the original spec: + +- A network create whose name matches no declared pattern is refused; one that + matches is permitted. +- `internal: false` is refused unless declared; `Driver: macvlan`, `Options`, + `IPAM` and any unrecognised key are each refused, naming the key. +- `GET`/`DELETE` of a network the socket did not create is refused. +- A container created with `NetworkMode` naming an owned network is permitted; + one naming an arbitrary network is refused. +- `GET /images/{name}/json` is permitted for a declared image and refused for an + undeclared one; `GET /images/json` is refused. +- `kill`, `stop` and `logs` are permitted on an owned container and refused on + one the socket did not create. +- An end-to-end run that creates an internal network, runs a container on it, + reads its logs, kills it, and deletes the network. + +## Open questions + +- Whether `name` globs should be anchored (`vk-*` matching `vk-abc` but not + `other-vk-abc`). Leaning yes — anchored, with `*` matching within a segment — + since an unanchored glob in a security grammar reads as more permissive than + it looks. +- Whether an owned network should be deleted automatically when the job's worker + exits, as the socket itself is. Leaning yes, for the same reason: nothing + should outlive the job that created it. From 4f82c49ff45f39c6b8f9e7786252118712f356c4 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 09:13:45 -0400 Subject: [PATCH 04/31] Let a job kill, stop and read logs from its own container Three endpoints classified as `other` and hit default-deny, so a job could start a container but never stop one that overran its budget, and never read what it printed. Consumer feedback asked for kill; stop belongs with it, since a timeout path that can only kill is worse than one that can ask politely first, and logs is exactly the "reads about the job's own containers" the design already documents as baseline - refusing it contradicted the docs rather than implementing them. All three reuse the scoping that is already there: kill and stop join the run action, logs joins inspect as a read, and every one of them is permitted only against a container this socket created. Design: docs/superpowers/specs/2026-09-06-docker-endpoint-families.md Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/main/docker/docker-evaluator.test.ts | 22 ++++++++++++++++++++++ src/main/docker/docker-evaluator.ts | 4 ++++ src/main/docker/docker-request.test.ts | 20 +++++++++++++++++++- src/main/docker/docker-request.ts | 10 ++++++++++ 4 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/main/docker/docker-evaluator.test.ts b/src/main/docker/docker-evaluator.test.ts index 1e70650..8f81cca 100644 --- a/src/main/docker/docker-evaluator.test.ts +++ b/src/main/docker/docker-evaluator.test.ts @@ -377,3 +377,25 @@ describe('Go case-insensitive JSON decoding', () => { expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: 'postgres:16' }), ctx(p)).allowed).toBe(true); }); }); + +describe('kill, stop and logs on the job\'s own container', () => { + const p = { run: { images: ['postgres:16'], network: 'bridge' } }; + + it('permits them on an owned container and refuses them on one it did not create', () => { + const own = ctx(p, ['mine123']); + for (const [method, tpl] of [ + ['POST', '/v1.45/containers/%s/kill'], + ['POST', '/v1.45/containers/%s/stop'], + ['GET', '/v1.45/containers/%s/logs?stdout=1&stderr=1'], + ] as const) { + expect([tpl, evaluateDockerRequest(mk(method, tpl.replace('%s', 'mine123')), own).allowed]).toEqual([tpl, true]); + expect([tpl, evaluateDockerRequest(mk(method, tpl.replace('%s', 'theirs999')), own).allowed]).toEqual([tpl, false]); + } + }); + + it('refuses kill and stop when the policy declares no run action', () => { + const noRun = ctx({ pull: { registries: ['docker.io'] } }, ['mine123']); + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/mine123/kill'), noRun).allowed).toBe(false); + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/mine123/stop'), noRun).allowed).toBe(false); + }); +}); diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index c750f55..41a4e0f 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -446,6 +446,8 @@ export function evaluateDockerRequest(req: DockerRequest, ctx: DockerEvalContext case 'create': return evaluateCreate(req, ctx, policy); case 'inspect': + case 'logs': + // Reads about the job's own container: the documented baseline, scoped. return evaluateOwnContainer(req, ctx); case 'list': // No policy key grants it: it would enumerate the whole daemon. @@ -456,6 +458,8 @@ export function evaluateDockerRequest(req: DockerRequest, ctx: DockerEvalContext case 'attach': case 'wait': case 'remove': + case 'kill': + case 'stop': if (!policy.run) return deny('the repository docker policy declares no run action', hints.run); return evaluateOwnContainer(req, ctx); case 'pull': diff --git a/src/main/docker/docker-request.test.ts b/src/main/docker/docker-request.test.ts index d378683..4ad0093 100644 --- a/src/main/docker/docker-request.test.ts +++ b/src/main/docker/docker-request.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from '@jest/globals'; -import { parseDockerRequest, classifyDockerRequest, DockerAction } from './docker-request'; +import { parseDockerRequest, classifyDockerRequest, containerIdFrom, DockerAction } from './docker-request'; const mk = (method: string, url: string, headers: Record = {}, body = Buffer.alloc(0)) => parseDockerRequest({ method, url, headers, body }); @@ -103,3 +103,21 @@ describe('classifyDockerRequest', () => { } }); }); + +describe('container lifecycle endpoints the run action covers', () => { + const mk = (m: string, u: string) => parseDockerRequest({ method: m, url: u, headers: {}, body: Buffer.alloc(0) }); + + it.each([ + ['POST', '/v1.45/containers/abc/kill', 'kill'], + ['POST', '/v1.45/containers/abc/stop', 'stop'], + ['GET', '/v1.45/containers/abc/logs?stdout=1', 'logs'], + ])('maps %s %s to %s', (method, url, action) => { + expect(classifyDockerRequest(mk(method, url))).toBe(action); + }); + + it('extracts the container id from each of them, so they can be scoped', () => { + for (const [m, u] of [['POST', '/v1.45/containers/abc/kill'], ['POST', '/v1.45/containers/abc/stop'], ['GET', '/v1.45/containers/abc/logs']] as const) { + expect(containerIdFrom(mk(m, u))).toBe('abc'); + } + }); +}); diff --git a/src/main/docker/docker-request.ts b/src/main/docker/docker-request.ts index 3cb8f74..4631f6d 100644 --- a/src/main/docker/docker-request.ts +++ b/src/main/docker/docker-request.ts @@ -25,6 +25,9 @@ export type DockerAction = | 'attach' | 'wait' | 'remove' + | 'kill' + | 'stop' + | 'logs' | 'build' | 'other'; @@ -107,6 +110,10 @@ const ENDPOINTS: ReadonlyArray<{ method: string; path: RegExp; action: DockerAct { method: 'POST', path: new RegExp(`^/containers/${ID}/start$`), action: 'start' }, { method: 'POST', path: new RegExp(`^/containers/${ID}/attach$`), action: 'attach' }, { method: 'POST', path: new RegExp(`^/containers/${ID}/wait$`), action: 'wait' }, + { method: 'POST', path: new RegExp(`^/containers/${ID}/kill$`), action: 'kill' }, + { method: 'POST', path: new RegExp(`^/containers/${ID}/stop$`), action: 'stop' }, + // A read about the job's own container, like inspect. + { method: 'GET', path: new RegExp(`^/containers/${ID}/logs$`), action: 'logs' }, { method: 'DELETE', path: new RegExp(`^/containers/${ID}$`), action: 'remove' }, { method: 'POST', path: /^\/build$/, action: 'build' }, ]; @@ -117,6 +124,9 @@ const CONTAINER_ID_PATHS: ReadonlyArray = [ new RegExp(`^/containers/(${ID})/start$`), new RegExp(`^/containers/(${ID})/attach$`), new RegExp(`^/containers/(${ID})/wait$`), + new RegExp(`^/containers/(${ID})/kill$`), + new RegExp(`^/containers/(${ID})/stop$`), + new RegExp(`^/containers/(${ID})/logs$`), new RegExp(`^/containers/(${ID})$`), ]; From e1762df6441a4362c8a3ef56d24ce2a077d55132 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 09:36:17 -0400 Subject: [PATCH 05/31] Close the volume-driver escape and make HostConfig an allowlist Two findings from the review, both letting a create body reach past the filter. An "anonymous" volume - a Mounts entry of Type volume with no Source - was treated as container-lifecycle storage and skipped every mount check. But the built-in local driver with type=none,o=bind,device=, the mechanism compose exposes as driver_opts, makes that entry a bind mount of an arbitrary host path, read-write, whatever the policy declares. A volume carrying DriverConfig is now refused, and the remaining mount keys are read the way the daemon reads them. HostConfig was gated by a blocklist, so every key nobody enumerated was forwarded unexamined - PortBindings among them, publishing a container port on the operator's interfaces and outside the proxy that controls the job's egress. It is an allowlist now: a key the filter does not understand is refused, which is what the grammar already promises about itself. The allowlist is built from the 62 HostConfig keys a real docker run actually sends, captured from the CLI rather than guessed - the first two attempts broke `docker run` outright, on ContainerIDFile and then PortBindings, and the end-to-end test against a real daemon is what caught it. Keys that are only dangerous when non-empty are gated by value rather than refused outright: ContainerIDFile, PortBindings, PublishAllPorts, Cgroup, ExtraHosts, GroupAdd, Links and VolumeDriver. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/main/docker/docker-evaluator.test.ts | 79 ++++++++++++++++++++++ src/main/docker/docker-evaluator.ts | 83 ++++++++++++++++++++++-- 2 files changed, 155 insertions(+), 7 deletions(-) diff --git a/src/main/docker/docker-evaluator.test.ts b/src/main/docker/docker-evaluator.test.ts index 8f81cca..25ca6a8 100644 --- a/src/main/docker/docker-evaluator.test.ts +++ b/src/main/docker/docker-evaluator.test.ts @@ -399,3 +399,82 @@ describe('kill, stop and logs on the job\'s own container', () => { expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/mine123/stop'), noRun).allowed).toBe(false); }); }); + +describe('volume mounts that are really bind mounts', () => { + const p = { run: { images: ['postgres:16'], mounts: [{ path: './', mode: 'ro' as const }], network: 'bridge' } }; + + it('refuses an anonymous volume whose local-driver options bind a host path', () => { + // The local driver with type=none,o=bind,device= IS a bind mount - + // the same thing compose exposes as driver_opts. The entry has no Source, + // so it looked like container-lifecycle storage and skipped every check. + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { + Image: 'postgres:16', + HostConfig: { + Mounts: [{ + Type: 'volume', + Target: '/host', + VolumeOptions: { DriverConfig: { Name: 'local', Options: { type: 'none', o: 'bind', device: '/Users/me/.ssh' } } }, + }], + }, + }), + ctx(p) + ); + expect(v.allowed).toBe(false); + }); + + it('refuses it whatever the casing of the driver keys', () => { + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { + Image: 'postgres:16', + HostConfig: { Mounts: [{ type: 'volume', target: '/host', volumeoptions: { driverconfig: { Name: 'local', Options: { device: '/' } } } }] }, + }), + ctx(p) + ); + expect(v.allowed).toBe(false); + }); + + it('still permits a plain anonymous volume and a tmpfs, which reach no host path', () => { + for (const m of [{ Type: 'volume', Target: '/data' }, { Type: 'tmpfs', Target: '/tmp' }]) { + expect([m.Type, evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: 'postgres:16', HostConfig: { Mounts: [m] } }), ctx(p)).allowed]) + .toEqual([m.Type, true]); + } + }); +}); + +describe('HostConfig is an allowlist, not a blocklist', () => { + const p = { run: { images: ['postgres:16'], mounts: [{ path: './', mode: 'ro' as const }], network: 'bridge' } }; + const create = (hostConfig: Record) => + evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: 'postgres:16', HostConfig: hostConfig }), ctx(p)); + + it('refuses publishing container ports onto the operator host', () => { + // -p 8080:80. Nothing in the grammar can name it, and it exposes a + // service on the operator's interfaces, outside the proxy's egress control. + expect(create({ PortBindings: { '80/tcp': [{ HostPort: '8080' }] } }).allowed).toBe(false); + expect(create({ PublishAllPorts: true }).allowed).toBe(false); + }); + + it('refuses any HostConfig key the grammar cannot name, even one invented later', () => { + for (const key of ['StorageOpt', 'SomeFutureEscape', 'Anything', 'NextApiVersionKey']) { + expect([key, create({ [key]: ['x'] }).allowed]).toEqual([key, false]); + } + }); + + it('refuses the keys that only reach outside the container when non-empty', () => { + expect(create({ Links: ['other:db'] }).allowed).toBe(false); + expect(create({ VolumeDriver: 'local' }).allowed).toBe(false); + expect(create({ ExtraHosts: ['evil:1.2.3.4'] }).allowed).toBe(false); + expect(create({ GroupAdd: ['staff'] }).allowed).toBe(false); + expect(create({ Cgroup: '/other' }).allowed).toBe(false); + }); + + it('refuses a --cidfile that would write to a host path, while allowing the empty default', () => { + expect(create({ ContainerIDFile: '/tmp/pwned.cid' }).allowed).toBe(false); + expect(create({ ContainerIDFile: '' }).allowed).toBe(true); + }); + + it('still permits the keys a plain docker run actually sends', () => { + expect(create({}).allowed).toBe(true); + expect(create({ AutoRemove: true, NetworkMode: 'bridge', Binds: [], RestartPolicy: { Name: '', MaximumRetryCount: 0 }, LogConfig: { Type: '', Config: {} }, ConsoleSize: [0, 0] }).allowed).toBe(true); + }); +}); diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index 41a4e0f..19b4fe3 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -152,8 +152,56 @@ const oneOf = (...allowed: string[]) => (v: unknown): boolean => isUnset(v) || a * only permitted values are the defaults the docker CLI sends for it; the * shapes are bounded by the API version the proxy pins. */ +/** + * HostConfig keys the filter understands and will forward. + * + * An allowlist, not a blocklist: enumerating the dangerous keys means every + * key nobody thought of - and every key a future API version adds - is + * forwarded unexamined. PortBindings was exactly that, publishing a container + * port on the operator's interfaces, outside the proxy that controls the job's + * egress. A key absent from this list is refused, which is the same principle + * the grammar applies to itself: what cannot be named cannot be requested. + * + * These are the keys an ordinary `docker run` sends. Each is either inert + * (resource limits, logging, restart behaviour) or gated below. + */ +const HOST_CONFIG_KNOWN: ReadonlySet = new Set([ + // Gated below by value, or checked by the mount and network logic. + 'privileged', 'binds', 'mounts', 'networkmode', 'containeridfile', 'portbindings', 'publishallports', + 'pidmode', 'ipcmode', 'utsmode', 'usernsmode', 'cgroupnsmode', 'cgroupparent', 'cgroup', + 'devices', 'devicerequests', 'devicecgrouprules', 'securityopt', 'capadd', 'sysctls', 'runtime', + 'isolation', 'maskedpaths', 'readonlypaths', 'volumesfrom', 'extrahosts', 'groupadd', 'links', + 'volumedriver', + // Inert: they bound the container, they do not widen it. Dropping capabilities + // and setting resource limits or DNS search only ever restricts. + 'capdrop', 'autoremove', 'restartpolicy', 'logconfig', 'consolesize', 'readonlyrootfs', 'init', + 'oomscoreadj', 'oomkilldisable', 'shmsize', 'memory', 'memoryswap', 'memoryreservation', + 'memoryswappiness', 'kernelmemory', 'nanocpus', 'cpushares', 'cpuperiod', 'cpuquota', + 'cpurealtimeperiod', 'cpurealtimeruntime', 'cpusetcpus', 'cpusetmems', 'cpucount', 'cpupercent', + 'blkioweight', 'blkioweightdevice', 'blkiodevicereadbps', 'blkiodevicewritebps', + 'blkiodevicereadiops', 'blkiodevicewriteiops', 'pidslimit', 'dns', 'dnsoptions', 'dnssearch', + 'annotations', 'tmpfs', 'ulimits', 'iomaximumbandwidth', 'iomaximumiops', +]); + const HOST_CONFIG_GATES: ReadonlyArray<{ key: string; permitted: (v: unknown) => boolean; flag: string }> = [ + // The daemon writes the new container's id to this HOST path, so a non-empty + // value creates or truncates a file anywhere the daemon can reach. The CLI + // always sends it, empty. + { key: 'ContainerIDFile', permitted: isEmptyString, flag: '--cidfile' }, + // Publishing binds a listening socket on the operator's interfaces, exposing + // a container service to their network and outside the proxy that controls + // this job's egress. The CLI sends both, empty, on every run. + { key: 'PortBindings', permitted: isEmptyObject, flag: '-p/--publish' }, + { key: 'PublishAllPorts', permitted: (v: unknown) => isUnset(v) || v === false, flag: '-P/--publish-all' }, { key: 'PidMode', permitted: isEmptyString, flag: '--pid' }, + // Each is sent empty by every ordinary run, and each reaches outside the + // container when it is not: a cgroup to join, hosts entries, extra groups, + // a link to another job's container, or a volume driver that can bind-mount. + { key: 'Cgroup', permitted: isEmptyString, flag: '--cgroup' }, + { key: 'ExtraHosts', permitted: isEmptyArray, flag: '--add-host' }, + { key: 'GroupAdd', permitted: isEmptyArray, flag: '--group-add' }, + { key: 'Links', permitted: isEmptyArray, flag: '--link' }, + { key: 'VolumeDriver', permitted: isEmptyString, flag: '--volume-driver' }, { key: 'IpcMode', permitted: oneOf('', 'private', 'none', 'shareable'), flag: '--ipc' }, { key: 'UTSMode', permitted: isEmptyString, flag: '--uts' }, { key: 'UsernsMode', permitted: isEmptyString, flag: '--userns' }, @@ -217,25 +265,38 @@ function parseBind(bind: string): MountRequest | string { /** Parse one entry of HostConfig.Mounts; null when it needs no host check. */ function parseMount(mount: unknown): MountRequest | string | null { if (!isPlainObject(mount)) return 'each entry of HostConfig.Mounts must be an object'; - const type = mount.Type; + const type = pick(mount, 'Type'); if (type === 'tmpfs') return null; if (type === 'volume') { - if (isEmptyString(mount.Source)) return null; // anonymous: lives with the container - return `"${mount.Source}" is a named volume, not a workspace path; only declared workspace mounts are permitted`; + // A volume is only container-lifecycle storage while it uses the default + // driver with no options. The built-in local driver with + // type=none,o=bind,device= IS a bind mount - the mechanism compose + // exposes as driver_opts - so an "anonymous" volume carrying driver + // options reaches an arbitrary host path, read-write, having skipped every + // mount check because it declares no Source. + const volumeOptions = pick(mount, 'VolumeOptions'); + if (isPlainObject(volumeOptions) && !isUnset(pick(volumeOptions, 'DriverConfig'))) { + return 'a volume with DriverConfig is not permitted: a volume driver can bind-mount a host path, which only a declared workspace mount may do'; + } + if (isEmptyString(pick(mount, 'Source'))) return null; // anonymous: lives with the container + return `"${String(pick(mount, 'Source'))}" is a named volume, not a workspace path; only declared workspace mounts are permitted`; } if (type !== 'bind') return `mount type "${String(type)}" is not permitted`; - if (typeof mount.Source !== 'string' || !path.isAbsolute(mount.Source)) { + const source = pick(mount, 'Source'); + if (typeof source !== 'string' || !path.isAbsolute(source)) { return 'a bind mount needs an absolute Source'; } - const options = mount.BindOptions; + const options = pick(mount, 'BindOptions'); if (options !== undefined && options !== null) { if (!isPlainObject(options)) return 'BindOptions must be an object'; - const propagation = options.Propagation ?? ''; + const propagation = pick(options, 'Propagation') ?? ''; if (!PROPAGATIONS.has(propagation as string)) { return `mount propagation "${String(propagation)}" is not permitted`; } } - return { source: mount.Source, mode: mount.ReadOnly === true ? 'ro' : 'rw' }; + // Any casing that says read-only counts; a mount is rw only when none does. + const readOnly = valuesFor(mount, 'ReadOnly').some((v) => v === true); + return { source, mode: readOnly ? 'ro' : 'rw' }; } function collectMounts(hostConfig: Record): MountRequest[] | string { @@ -330,6 +391,14 @@ function evaluateCreate(req: DockerRequest, ctx: DockerEvalContext, policy: Dock } else if (privilegedValues.some((v) => !isUnset(v) && v !== false)) { return deny('HostConfig.Privileged must be a boolean'); } + for (const key of Object.keys(hostConfig)) { + if (!HOST_CONFIG_KNOWN.has(key.toLowerCase())) { + return deny( + `HostConfig.${key} is not a setting the localmost docker socket understands, so it cannot be forwarded` + ); + } + } + for (const gate of HOST_CONFIG_GATES) { // Every casing must pass: one that does not is a value the daemon honours. if (!valuesFor(hostConfig, gate.key).every((v) => gate.permitted(v))) { From 20398dbf4af9bfc707f53c1ea61fd763537739b4 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 09:38:56 -0400 Subject: [PATCH 06/31] Keep a drifted worker constrained instead of reopening it at job start When a claim finds the approved policy has moved since the worker was built, it cuts the network back to runner infrastructure, leaves the docker socket closed and retires the worker: the job runs under the boundary that was actually approved for it. The job-start refresh then undid all of that. It calls applyPolicyForTarget without isClaim, so the drift check never runs, and it fell straight through to the widening below - restoring the full host list and binding the docker socket the claim had deliberately left shut. Found independently by two review dimensions, which is what made it worth looking at closely. The constraint is now recorded on the worker and every later refresh leaves it alone. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/main/runner-manager.test.ts | 38 +++++++++++++++++++++++++++++++++ src/main/runner-manager.ts | 16 +++++++++++++- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/main/runner-manager.test.ts b/src/main/runner-manager.test.ts index 4f97fe0..f02fde4 100644 --- a/src/main/runner-manager.test.ts +++ b/src/main/runner-manager.test.ts @@ -1574,3 +1574,41 @@ describe('job-start detection against injected output', () => { expect(startedNames(events)).toEqual(['build']); }); }); + +describe('a worker constrained by policy drift stays constrained', () => { + it('does not reopen the docker socket or restore hosts when the job starts', async () => { + const docker = { pull: { registries: ['docker.io'] }, run: { images: ['alpine:3'] } }; + const manager = new RunnerManager({ + onLog: jest.fn(), + onStatusChange: jest.fn(), + onJobHistoryUpdate: jest.fn(), + getRepoPolicy: async () => ({ + hosts: ['example.com'], level: 'strict' as const, readPaths: [], writePaths: [], docker, + }), + }); + const helper = new RunnerManagerTestHelper(manager); + const proxy = { setPolicyAllowedHosts: jest.fn(), setPolicyLevel: jest.fn(), getStats: jest.fn(), getPolicyLevel: jest.fn() }; + const dockerSocket = { bind: jest.fn(), boundRepository: jest.fn() }; + helper.setProxy(1, proxy); + helper.setDockerProxy(1, dockerSocket); + // A stamp that cannot match the policy above: the approved policy moved + // after this worker was built, so its profile is out of date. + helper.setInstance(1, { + name: 'runner-1', status: 'busy', policyStamp: 'stale-stamp', + currentJob: { name: 'build', repository: 'owner/repo', startedAt: 'now', id: 'job-1', targetDisplayName: 'owner/repo', githubSha: 'abc1234' }, + }); + helper.setPendingTargetContext('1', { targetId: 't1', targetDisplayName: 'owner/repo', githubSha: 'abc1234' }); + + // The claim detects drift: network cut to nothing, docker socket left closed. + await helper.applyPolicyOnClaim(1, 'owner/repo', 'abc1234'); + expect(proxy.setPolicyAllowedHosts).toHaveBeenLastCalledWith([]); + expect(dockerSocket.bind).not.toHaveBeenCalled(); + + // The job-start refresh must not undo that. It runs without isClaim, so it + // never re-checks drift, and it used to fall straight through to widening. + await helper.applyRepoPolicy(1); + + expect(dockerSocket.bind).not.toHaveBeenCalled(); + expect(proxy.setPolicyAllowedHosts).toHaveBeenLastCalledWith([]); + }); +}); diff --git a/src/main/runner-manager.ts b/src/main/runner-manager.ts index e7f1f24..8b13578 100644 --- a/src/main/runner-manager.ts +++ b/src/main/runner-manager.ts @@ -38,6 +38,8 @@ interface RunnerInstance { * the approved policy must not serve a job under it. */ policyStamp?: string; + /** Set when a claim found the approved policy had moved; the worker stays constrained. */ + policyDrifted?: boolean; /** * The repository whose job this worker claimed, as the broker reported it. * The docker socket opens only for this repository, and only when it is @@ -1733,6 +1735,14 @@ export class RunnerManager { // this worker would run the job under the old boundary - so it is refused // rather than run. Approving through the app retires workers eagerly; this // also covers approving through the CLI, which writes the cache directly. + if (instance?.policyDrifted) { + this.log( + 'debug', + `[instance ${instanceNum}] Policy drifted for this worker; leaving it constrained rather than reapplying` + ); + return; + } + const currentStamp = this.stampFor(policy); if (isClaim && instance?.policyStamp && instance.policyStamp !== currentStamp) { // The filesystem half is fixed in this worker's profile and cannot be @@ -1742,7 +1752,11 @@ export class RunnerManager { // worker is retired so nothing further lands on it - this constrains the // job rather than refusing it, which the proxy cannot do on its own. // The docker socket stays as it was born, closed: nothing on this path - // opens it. + // opens it. Sticky, because the job-start refresh runs without isClaim + // and so never re-checks drift - without this it fell straight through + // to the widening below, restoring the hosts and rebinding the socket + // this branch had just closed. + if (instance) instance.policyDrifted = true; proxy.setPolicyAllowedHosts([]); proxy.setPolicyLevel('strict'); this.log( From 5472b521ebaf111aa48c26275fb0c06601949087 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 10:40:58 -0400 Subject: [PATCH 07/31] Filter build parameters, and stop claiming the context path is checked docker build carries its whole configuration in the query string, and the filter read exactly one parameter of it. So `docker build --network host` walked through a door `create` keeps shut, and extrahosts, cachefrom, securityopt and outputs were forwarded unexamined. Build parameters are an allowlist now, on the same reasoning as HostConfig, and networkmode is held to the rule the run path already applies. build.context was reported three times as validated, merged, diffed and serialized but never enforced. It is not enforceable: the Engine API carries a build context as a tar the client already assembled, so no path reaches the filter to check. What confines a local context is the seatbelt profile, since the job can only read what the profile grants, and the filter's part is to refuse a remote context that would have the daemon fetch it instead. The spec and the docs said otherwise; they now say this. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- docs/roadmap/localmostrc.md | 18 ++++++--- .../2026-09-05-docker-isolation-design.md | 10 ++++- src/main/docker/docker-evaluator.test.ts | 27 +++++++++++++ src/main/docker/docker-evaluator.ts | 39 +++++++++++++++++++ 4 files changed, 86 insertions(+), 8 deletions(-) diff --git a/docs/roadmap/localmostrc.md b/docs/roadmap/localmostrc.md index c50bf34..a6a002f 100644 --- a/docs/roadmap/localmostrc.md +++ b/docs/roadmap/localmostrc.md @@ -288,12 +288,18 @@ Actions are CLI-shaped, so a policy reads the way a workflow author thinks: |---|---|---| | `pull` | image pulls | `registries` — the registry each pulled image comes from | | `run` | container create, start, attach, wait and remove | `images` — the image a container is created from; `mounts` — workspace paths a container may bind, each `ro` or `rw`; `network` — the container's network mode | -| `build` | image builds | `context` — where the build context may resolve | - -Conditions are checked against the request itself. Mount and context paths are -resolved through symlinks and must stay inside the job workspace, so `../` -traversal and absolute host paths fail structurally rather than by pattern -match, and a container may write to a mount only where the policy says `rw`. +| `build` | image builds | `context` — which directory the workflow builds from, for the reader and the approval diff | + +Conditions are checked against the request itself. Mount paths are resolved +through symlinks and must stay inside the job workspace, so `../` traversal and +absolute host paths fail structurally rather than by pattern match, and a +container may write to a mount only where the policy says `rw`. + +`build.context` is the exception: it is documentation, not a check. A build +context reaches the daemon as a tar the client already assembled, so there is no +path in the request to test. A local context is confined by the sandbox profile +instead — the job can only read what the profile grants — and the filter refuses +a *remote* context, which would have the daemon fetch it and skip the profile. Anything not listed is denied: an undeclared image, registry, mount or network mode, and every endpoint the proxy does not understand. diff --git a/docs/superpowers/specs/2026-09-05-docker-isolation-design.md b/docs/superpowers/specs/2026-09-05-docker-isolation-design.md index 12d0280..bb1d097 100644 --- a/docs/superpowers/specs/2026-09-05-docker-isolation-design.md +++ b/docs/superpowers/specs/2026-09-05-docker-isolation-design.md @@ -191,8 +191,14 @@ checked against the request body: - **`network`** against `NetworkMode`. - **`images`** against the image reference in the create request. - **`registries`** against the registry of a pull. -- **`context`** against the build context path, with the same resolution rules as - mounts. +- **`context`** documents which directory the workflow builds from. It is not + checked against the request, because there is nothing in the request to check + it against: the Engine API carries a build context as a tar the client already + assembled, so the filter never sees a path. What confines a local context is + the seatbelt profile - the job can only read what the profile grants, so the + tar can only contain workspace content. The filter's job here is to refuse a + *remote* context, which would have the daemon fetch the context itself and so + bypass the profile entirely. Anything not listed is denied. diff --git a/src/main/docker/docker-evaluator.test.ts b/src/main/docker/docker-evaluator.test.ts index 25ca6a8..03e9d17 100644 --- a/src/main/docker/docker-evaluator.test.ts +++ b/src/main/docker/docker-evaluator.test.ts @@ -478,3 +478,30 @@ describe('HostConfig is an allowlist, not a blocklist', () => { expect(create({ AutoRemove: true, NetworkMode: 'bridge', Binds: [], RestartPolicy: { Name: '', MaximumRetryCount: 0 }, LogConfig: { Type: '', Config: {} }, ConsoleSize: [0, 0] }).allowed).toBe(true); }); }); + +describe('build query parameters', () => { + const p = { run: { images: ['postgres:16'], network: 'bridge' }, build: { context: './' } }; + const build = (qs: string, policy: DockerPolicy = p) => + evaluateDockerRequest(mk('POST', `/v1.45/build${qs}`), ctx(policy)); + + it('refuses host and container networking, which the run path already forbids', () => { + expect(build('?networkmode=host').allowed).toBe(false); + expect(build('?networkmode=container%3Aabc').allowed).toBe(false); + }); + + it('refuses an undeclared build network, and permits the declared one', () => { + expect(build('?networkmode=some-other-net').allowed).toBe(false); + expect(build('?networkmode=bridge').allowed).toBe(true); + expect(build('?networkmode=none').allowed).toBe(true); + }); + + it('refuses build parameters that reach the host or the daemon config', () => { + for (const qs of ['?remote=https%3A%2F%2Fevil%2Fctx', '?extrahosts=evil%3A1.2.3.4', '?cachefrom=%5B%22other%3Alatest%22%5D', '?ulimits=x', '?securityopt=seccomp%3Dunconfined', '?outputs=type%3Dlocal%2Cdest%3D%2Ftmp']) { + expect([qs, build(qs).allowed]).toEqual([qs, false]); + } + }); + + it('permits the parameters an ordinary docker build sends', () => { + expect(build('?t=app%3Alatest&dockerfile=Dockerfile&rm=1&buildargs=%7B%7D&labels=%7B%7D&shmsize=0&version=1').allowed).toBe(true); + }); +}); diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index 19b4fe3..3930988 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -467,6 +467,20 @@ function evaluatePull(req: DockerRequest, policy: DockerPolicy): DockerVerdict { return ALLOW; } +/** + * Build query parameters the filter understands. + * + * An allowlist for the same reason HostConfig is one: `docker build` carries + * its whole configuration in the query string, so anything not enumerated is + * forwarded unexamined. `networkmode` is gated separately below, since it is + * the same host reach the run path already refuses. + */ +const BUILD_PARAMS_KNOWN: ReadonlySet = new Set([ + 't', 'dockerfile', 'q', 'nocache', 'rm', 'forcerm', 'pull', 'buildargs', 'labels', 'target', + 'shmsize', 'memory', 'memswap', 'cpushares', 'cpusetcpus', 'cpuperiod', 'cpuquota', 'squash', + 'platform', 'version', 'buildid', 'session', +]); + function evaluateBuild(req: DockerRequest, policy: DockerPolicy): DockerVerdict { if (!policy.build) return deny('the repository docker policy declares no build action', hints.build); // The Engine API carries the context as a tar the client assembled from @@ -476,6 +490,31 @@ function evaluateBuild(req: DockerRequest, policy: DockerPolicy): DockerVerdict if (req.query.remote !== undefined) { return deny('a remote build context is not permitted; send the context with the request'); } + + for (const key of Object.keys(req.query)) { + const name = key.toLowerCase(); + if (name === 'networkmode') continue; + if (!BUILD_PARAMS_KNOWN.has(name)) { + return deny(`build parameter "${key}" is not one the localmost docker socket understands, so it cannot be forwarded`); + } + } + + // A build runs containers, and its network is chosen here rather than in a + // HostConfig - so the same rule the run path applies has to apply here too, + // or `docker build --network host` walks through a door create keeps shut. + const rawMode = req.query.networkmode ?? req.query.NetworkMode; + if (rawMode !== undefined && rawMode !== '' && rawMode !== 'default') { + if (rawMode === 'host' || rawMode.startsWith('container:')) { + return deny(`--network=${rawMode} on a build reaches the host and cannot be permitted by policy`); + } + if (rawMode !== 'none' && rawMode !== policy.run?.network) { + return deny( + `build network "${rawMode}" is not declared in the repository docker policy (run.network)`, + hints.network(rawMode) + ); + } + } + return ALLOW; } From 4ea1b5d9592ee16d14a056b1ecad2c6b2679152b Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 10:44:37 -0400 Subject: [PATCH 08/31] Show docker grants on the approval screen and in the diff Two review findings, both meaning an operator could approve container access without being shown any. The approval screen summarises what a policy grants - network hosts, filesystem reads and writes, sockets - and had no notion of docker at all, so a policy granting a repository the daemon listed nothing for it. It now describes pulls, images, mounts, network and privileged, per workflow as well as shared. Separately the approval diff compared only the conditions inside each action block, so a bare `run: {}` - which permits creating and running containers - produced no diff entry and reached approval invisibly. An action appearing or disappearing is now itself a diff entry, reported only when the block would otherwise leave no trace, so a block that merely changed its conditions is not reported twice. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/main/ipc-handlers/policy.test.ts | 42 ++++++++++++++++++++++++++++ src/main/ipc-handlers/policy.ts | 39 +++++++++++++++++++++++++- src/shared/docker-policy.test.ts | 29 +++++++++++++++++++ src/shared/docker-policy.ts | 17 +++++++++++ 4 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 src/main/ipc-handlers/policy.test.ts diff --git a/src/main/ipc-handlers/policy.test.ts b/src/main/ipc-handlers/policy.test.ts new file mode 100644 index 0000000..386555a --- /dev/null +++ b/src/main/ipc-handlers/policy.test.ts @@ -0,0 +1,42 @@ +import { describe, it, expect } from '@jest/globals'; + +jest.mock('electron', () => ({ ipcMain: { handle: jest.fn() } })); +jest.mock('../policy-cache', () => ({ + listCachedPolicies: jest.fn(), approvePolicy: jest.fn(), denyPolicy: jest.fn(), + removeCachedPolicy: jest.fn(), recordPolicyDecision: jest.fn(), +})); +jest.mock('../app-state', () => ({ getRunnerManager: jest.fn(), getLogger: jest.fn() })); + +import { summarizeGrants } from './policy'; + +describe('summarizeGrants', () => { + it('shows docker grants, which an operator is consenting to when they approve', () => { + const grants = summarizeGrants({ + shared: { + docker: { + pull: { registries: ['docker.io'] }, + run: { images: ['alpine:3'], mounts: [{ path: './', mode: 'ro' }], network: 'bridge' }, + }, + }, + }); + expect(grants.join('\n')).toMatch(/docker pull: docker\.io/); + expect(grants.join('\n')).toMatch(/docker run image: alpine:3/); + expect(grants.join('\n')).toMatch(/docker mount: \.\/ \(ro\)/); + expect(grants.join('\n')).toMatch(/docker network: bridge/); + }); + + it('shows a bare action block, which grants the action itself', () => { + expect(summarizeGrants({ shared: { docker: { build: {} } } }).join('\n')).toMatch(/docker build/); + expect(summarizeGrants({ shared: { docker: { run: {} } } }).join('\n')).toMatch(/docker run/); + }); + + it('shows docker grants from a per-workflow section too', () => { + const grants = summarizeGrants({ workflows: { integration: { docker: { run: { images: ['redis:7'] } } } } }); + expect(grants.join('\n')).toMatch(/integration: docker run image: redis:7/); + }); + + it('still shows the non-docker grants', () => { + const grants = summarizeGrants({ shared: { network: { allow: ['example.com'] }, filesystem: { write: ['~/.npm'] } } }); + expect(grants).toEqual(['network: example.com', 'write: ~/.npm']); + }); +}); diff --git a/src/main/ipc-handlers/policy.ts b/src/main/ipc-handlers/policy.ts index 3e2a53c..44bf39e 100644 --- a/src/main/ipc-handlers/policy.ts +++ b/src/main/ipc-handlers/policy.ts @@ -17,6 +17,7 @@ import { } from '../policy-cache'; import { getRunnerManager, getLogger } from '../app-state'; +import { DockerPolicy } from '../../shared/docker-policy'; /** * Describe what a policy grants, in the terms a reviewer cares about. @@ -25,6 +26,41 @@ interface PolicySection { network?: { allow?: string[] }; filesystem?: { read?: string[]; write?: string[] }; sockets?: { allow?: string[] }; + docker?: DockerPolicy; +} + +/** + * What a docker policy grants, in the reviewer's terms. + * + * Every action block is named even when it carries no conditions: `run: {}` is + * a real grant - it permits creating and running containers - and an approval + * screen that showed nothing for it would be asking consent for an invisible + * capability. + */ +function describeDocker(docker: DockerPolicy | undefined, prefix: string): string[] { + if (!docker) return []; + const grants: string[] = []; + if (docker.pull) { + const registries = docker.pull.registries ?? []; + if (registries.length === 0) grants.push(`${prefix}docker pull`); + for (const registry of registries) grants.push(`${prefix}docker pull: ${registry}`); + } + if (docker.run) { + const { images = [], mounts = [], network } = docker.run; + if (images.length === 0 && mounts.length === 0 && network === undefined) { + grants.push(`${prefix}docker run`); + } + for (const image of images) grants.push(`${prefix}docker run image: ${image}`); + for (const mount of mounts) grants.push(`${prefix}docker mount: ${mount.path} (${mount.mode})`); + if (network !== undefined) grants.push(`${prefix}docker network: ${network}`); + } + if (docker.build) { + grants.push(docker.build.context === undefined + ? `${prefix}docker build` + : `${prefix}docker build: ${docker.build.context}`); + } + if (docker.privileged) grants.push(`${prefix}docker privileged`); + return grants; } function describeSection(section: PolicySection, prefix: string): string[] { @@ -41,6 +77,7 @@ function describeSection(section: PolicySection, prefix: string): string[] { for (const p of section.sockets?.allow || []) { grants.push(`${prefix}socket: ${p}`); } + grants.push(...describeDocker(section.docker, prefix)); return grants; } @@ -51,7 +88,7 @@ function describeSection(section: PolicySection, prefix: string): string[] { * `workflows:` that appears nowhere in `shared`, and approving what the UI * showed would otherwise approve more than was shown. */ -function summarizeGrants(config: { +export function summarizeGrants(config: { shared?: PolicySection; workflows?: Record; }): string[] { diff --git a/src/shared/docker-policy.test.ts b/src/shared/docker-policy.test.ts index aa3521a..17b6f63 100644 --- a/src/shared/docker-policy.test.ts +++ b/src/shared/docker-policy.test.ts @@ -314,3 +314,32 @@ describe('serializeDockerPolicy quoting', () => { expect(reparsed.config?.shared?.docker?.privileged).toBeUndefined(); }); }); + +describe('diffDockerPolicy on bare action blocks', () => { + it('reports the action itself appearing, not just its conditions', () => { + // `run: {}` permits creating and running containers. Diffing only the + // leaves showed nothing, so the grant reached approval invisibly. + const cases: Array<[DockerPolicy, string]> = [ + [{ run: {} }, 'shared.docker.run'], + [{ build: {} }, 'shared.docker.build'], + [{ pull: { registries: [] } }, 'shared.docker.pull'], + ]; + for (const [block, path] of cases) { + const diffs = diffDockerPolicy(undefined, block, 'shared.docker'); + expect([path, diffs.map((d) => d.path)]).toEqual([path, expect.arrayContaining([path])]); + expect([path, diffs.every((d) => d.type === 'added')]).toEqual([path, true]); + } + }); + + it('reports an action being removed as well', () => { + const diffs = diffDockerPolicy({ build: {} }, undefined, 'shared.docker'); + expect(diffs.map((d) => d.path)).toContain('shared.docker.build'); + expect(diffs[0].type).toBe('removed'); + }); + + it('does not double-report an action that merely changed its conditions', () => { + const diffs = diffDockerPolicy({ run: { images: ['a'] } }, { run: { images: ['b'] } }, 'shared.docker'); + expect(diffs.map((d) => d.path)).not.toContain('shared.docker.run'); + expect(diffs.map((d) => d.path)).toContain('shared.docker.run.images'); + }); +}); diff --git a/src/shared/docker-policy.ts b/src/shared/docker-policy.ts index a52d815..bef4967 100644 --- a/src/shared/docker-policy.ts +++ b/src/shared/docker-policy.ts @@ -273,6 +273,23 @@ export function diffDockerPolicy( diffScalar(oldP?.build?.context, newP?.build?.context, `${prefix}.build.context`, diffs); // false grants nothing, the same as absent. diffScalar(oldP?.privileged ? 'true' : undefined, newP?.privileged ? 'true' : undefined, `${prefix}.privileged`, diffs); + + // An action block with no conditions is still a grant - `run: {}` permits + // creating and running containers - and diffing only conditions showed an + // approver nothing for it at all. Named here only when the block is + // otherwise invisible, so a block that changed its conditions is not + // reported twice. + for (const action of ['pull', 'run', 'build'] as const) { + const had = oldP?.[action] !== undefined; + const has = newP?.[action] !== undefined; + if (had === has) continue; + if (diffs.some((d) => d.path.startsWith(`${prefix}.${action}.`))) continue; + diffs.push( + has + ? { path: `${prefix}.${action}`, type: 'added', newValue: action } + : { path: `${prefix}.${action}`, type: 'removed', oldValue: action } + ); + } return diffs; } From 1b0457fa4f7c30da08b0c4b5df91912ee5df5b1e Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 10:48:11 -0400 Subject: [PATCH 09/31] Mount what the filter checked, not what the job spelled The filter resolved each bind source through symlinks to decide, then forwarded the request as written, and the daemon resolved the same string again at mount time. The job can write in its own workspace, so it could point a symlink inside the workspace while the filter looked and somewhere else before the daemon did - a create that passed the check mounting /etc. The verdict now carries the body it approved, with every mount source replaced by the path that was actually resolved and checked, and that is what is forwarded. A fully resolved path resolves to itself, so the daemon's second resolution can no longer disagree with the filter's first. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/main/docker/docker-evaluator.ts | 60 ++++++++++++++++++++- src/main/docker/docker-filter-proxy.test.ts | 30 +++++++++++ src/main/docker/docker-filter-proxy.ts | 18 ++++--- 3 files changed, 99 insertions(+), 9 deletions(-) diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index 3930988..612f525 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -43,6 +43,13 @@ export interface DockerVerdict { reason?: string; /** The policy that would permit it, as YAML under `docker:` (for --updaterc discovery). */ policyHint?: string; + /** + * A create body whose mount sources are rewritten to the paths this verdict + * actually checked. Forwarding the spelling the client sent would let the + * daemon resolve it a second time, and the job can swap a symlink in the gap + * between the two resolutions; forwarding what was checked closes that. + */ + rewrittenBody?: unknown; } const ALLOW: DockerVerdict = { allowed: true }; @@ -330,7 +337,12 @@ function declaredMountPermits(declared: DockerMount, root: string, resolved: str return declared.mode === 'rw' || mode === 'ro'; } -function checkMounts(hostConfig: Record, ctx: DockerEvalContext, declared: DockerMount[]): DockerVerdict { +function checkMounts( + hostConfig: Record, + ctx: DockerEvalContext, + declared: DockerMount[], + resolutions?: Map +): DockerVerdict { const requests = collectMounts(hostConfig); if (typeof requests === 'string') return deny(requests); const realpath = ctx.realpath ?? ((p: string) => fs.realpathSync(p)); @@ -356,6 +368,7 @@ function checkMounts(hostConfig: Record, ctx: DockerEvalContext hints.mount(relative, mode) ); } + resolutions?.set(source, resolved); } return ALLOW; } @@ -364,6 +377,43 @@ function checkMounts(hostConfig: Record, ctx: DockerEvalContext // Actions // ----------------------------------------------------------------------------- +/** A copy of the create body with every bind source replaced by its resolved path. */ +function pinMountSources(body: Record, resolved: Map): unknown { + const pinned: Record = { ...body }; + for (const hostConfigKey of Object.keys(pinned)) { + if (hostConfigKey.toLowerCase() !== 'hostconfig') continue; + const hostConfig = pinned[hostConfigKey]; + if (!isPlainObject(hostConfig)) continue; + const copy: Record = { ...hostConfig }; + for (const key of Object.keys(copy)) { + const name = key.toLowerCase(); + if (name === 'binds' && Array.isArray(copy[key])) { + copy[key] = (copy[key] as unknown[]).map((bind) => { + if (typeof bind !== 'string') return bind; + const parts = bind.split(':'); + const target = resolved.get(parts[0]); + if (target === undefined) return bind; + return [target, ...parts.slice(1)].join(':'); + }); + } + if (name === 'mounts' && Array.isArray(copy[key])) { + copy[key] = (copy[key] as unknown[]).map((mount) => { + if (!isPlainObject(mount)) return mount; + const entry: Record = { ...mount }; + for (const mountKey of Object.keys(entry)) { + if (mountKey.toLowerCase() !== 'source') continue; + const source = entry[mountKey]; + if (typeof source === 'string' && resolved.has(source)) entry[mountKey] = resolved.get(source); + } + return entry; + }); + } + } + pinned[hostConfigKey] = copy; + } + return pinned; +} + function evaluateCreate(req: DockerRequest, ctx: DockerEvalContext, policy: DockerPolicy): DockerVerdict { const body = req.body; if (!isPlainObject(body)) return deny('container create requires a JSON object body'); @@ -445,7 +495,13 @@ function evaluateCreate(req: DockerRequest, ctx: DockerEvalContext, policy: Dock ); } - return checkMounts(hostConfig, ctx, policy.run.mounts ?? []); + // Pin every mount source to the path that was actually checked, so the + // daemon mounts what the filter judged rather than re-resolving a name the + // job can point somewhere else in between. + const resolutions = new Map(); + const verdict = checkMounts(hostConfig, ctx, policy.run.mounts ?? [], resolutions); + if (!verdict.allowed || resolutions.size === 0) return verdict; + return { allowed: true, rewrittenBody: pinMountSources(body, resolutions) }; } function evaluatePull(req: DockerRequest, policy: DockerPolicy): DockerVerdict { diff --git a/src/main/docker/docker-filter-proxy.test.ts b/src/main/docker/docker-filter-proxy.test.ts index ceb1914..8962b62 100644 --- a/src/main/docker/docker-filter-proxy.test.ts +++ b/src/main/docker/docker-filter-proxy.test.ts @@ -665,3 +665,33 @@ describe('upgrade requests', () => { expect(head).not.toMatch(/101/); }); }); + +describe('mount sources are pinned before forwarding', () => { + it('sends the daemon the resolved path, so a swapped symlink cannot change what is mounted', async () => { + const dir = tmp(); + const workspace = fs.realpathSync.native(dir); + const real = path.join(workspace, 'inside'); + const link = path.join(workspace, 'link'); + fs.mkdirSync(real); + fs.symlinkSync(real, link); + + const daemon = await fakeDaemon(dir); + const { proxy, sock } = await startProxy(dir, { + backend: { name: 'test', supportsPrivileged: false, resolveEndpoint: () => ({ socketPath: daemon.sock }), workspaceMountRoot: () => workspace }, + }); + proxy.bind('owner/repo', { run: { images: ['postgres:16'], mounts: [{ path: './', mode: 'rw' }], network: 'bridge' } }); + + const reply = await request(sock, 'POST', '/v1.45/containers/create', { + Image: 'postgres:16', + HostConfig: { Binds: [`${link}:/ws`] }, + }); + expect(reply.status).toBe(201); + + // The filter resolved `link` to decide. If it forwards the spelling it was + // given, the daemon resolves it again at mount time and the job can swap + // the symlink in between. + const create = daemon.seen.find((s) => s.url.includes('/containers/create'))!; + const binds = (JSON.parse(create.body.toString()) as { HostConfig: { Binds: string[] } }).HostConfig.Binds; + expect(binds[0]).toBe(`${real}:/ws`); + }); +}); diff --git a/src/main/docker/docker-filter-proxy.ts b/src/main/docker/docker-filter-proxy.ts index 1aa4461..e53efc0 100644 --- a/src/main/docker/docker-filter-proxy.ts +++ b/src/main/docker/docker-filter-proxy.ts @@ -258,7 +258,7 @@ export class DockerFilterProxy { } /** Null when the request may proceed; otherwise the status and message that refuse it. */ - private decide(req: DockerRequest): { status: number; message: string } | null { + private decide(req: DockerRequest): { refusal: { status: number; message: string } | null; rewrittenBody?: unknown } { if (req.apiVersion) { const version = parseApiVersion(req.apiVersion); if ( @@ -269,7 +269,7 @@ export class DockerFilterProxy { `API version ${req.apiVersion} is not supported by the localmost docker socket ` + `(supported: v${bareVersion(this.minApiVersion)} to v${bareVersion(this.maxApiVersion)})`; this.onLog({ level: 'info', message: `refused ${req.method} ${req.path}: ${message}` }); - return { status: 400, message }; + return { refusal: { status: 400, message } }; } } @@ -287,9 +287,9 @@ export class DockerFilterProxy { message: `denied ${req.method} ${req.path}: ${message}`, ...(verdict.policyHint !== undefined ? { policyHint: verdict.policyHint } : {}), }); - return { status: 403, message }; + return { refusal: { status: 403, message } }; } - return null; + return { refusal: null, rewrittenBody: verdict.rewrittenBody }; } /** Said once per socket: a declaration is a permission, not a requirement. */ @@ -351,13 +351,17 @@ export class DockerFilterProxy { res: http.ServerResponse, bufferedBody: Buffer | null ): void { - const refusal = this.decide(parsed); + const { refusal, rewrittenBody } = this.decide(parsed); if (refusal) { this.writeRefusal(res, refusal.status, refusal.message); if (bufferedBody === null) this.endAfterDrain(req, res); else res.end(); return; } + // The verdict may pin the body it approved - mount sources resolved to the + // paths actually checked - so the daemon mounts what the filter judged + // rather than re-resolving a name the job can repoint in between. + const body = rewrittenBody !== undefined ? Buffer.from(JSON.stringify(rewrittenBody)) : bufferedBody; const endpoint = this.backend.resolveEndpoint(); if (!endpoint) { this.warnNoDaemon(); @@ -366,7 +370,7 @@ export class DockerFilterProxy { else res.end(); return; } - this.forward(parsed, req, res, bufferedBody, endpoint.socketPath); + this.forward(parsed, req, res, body, endpoint.socketPath); } /** The URL as forwarded: an unversioned request is pinned to the version we understand. */ @@ -597,7 +601,7 @@ export class DockerFilterProxy { return; } - const refusal = this.decide(parsed); + const { refusal } = this.decide(parsed); if (refusal) { this.refuseRaw(client, refusal.status, refusal.message); return; From 8796dcfb7584cc02cb25158bb8a3a66b1dbde9c4 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 10:50:02 -0400 Subject: [PATCH 10/31] Attach registry credentials the job is never given The proxy has always been able to attach an X-Registry-Auth header to a pull, and nothing ever passed it the callback that produces one - so private registry pulls could not work, and the design's promise that credentials stop entering the sandbox described a capability that did not exist. Resolve them in the app, where ~/.docker is readable and stays denied to the job, following the docker CLI's own order: a per-registry credential helper, then the configured credential store, then an inline auths entry. Docker Desktop keeps its secrets in a helper rather than in the file, so supporting only the inline form would have covered almost nobody. An identity token is carried as a token rather than as a password, and a registry with no stored credential resolves to nothing rather than failing - an anonymous pull may well succeed. Naming a registry in pull.registries remains the whole grant; the job still never reads a secret. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/main/docker/registry-auth.test.ts | 62 +++++++++++++ src/main/docker/registry-auth.ts | 124 ++++++++++++++++++++++++++ src/main/index.ts | 5 ++ 3 files changed, 191 insertions(+) create mode 100644 src/main/docker/registry-auth.test.ts create mode 100644 src/main/docker/registry-auth.ts diff --git a/src/main/docker/registry-auth.test.ts b/src/main/docker/registry-auth.test.ts new file mode 100644 index 0000000..eb195ea --- /dev/null +++ b/src/main/docker/registry-auth.test.ts @@ -0,0 +1,62 @@ +import { describe, it, expect, jest } from '@jest/globals'; +import { resolveRegistryAuth, RegistryAuthDeps } from './registry-auth'; + +const decode = (header: string | undefined) => + header === undefined ? undefined : JSON.parse(Buffer.from(header, 'base64').toString('utf-8')); + +const deps = (over: Partial): RegistryAuthDeps => ({ + readConfig: () => null, + runHelper: () => null, + ...over, +}); + +describe('resolveRegistryAuth', () => { + it('uses the credential store, which is how Docker Desktop keeps secrets', () => { + const runHelper = jest.fn(() => ({ ServerURL: 'quay.io', Username: 'me', Secret: 's3cret' })); + const header = resolveRegistryAuth('quay.io', deps({ + readConfig: () => ({ credsStore: 'desktop', auths: { 'quay.io': {} } }), + runHelper: runHelper as RegistryAuthDeps['runHelper'], + })); + expect(runHelper).toHaveBeenCalledWith('desktop', 'quay.io'); + expect(decode(header)).toEqual({ username: 'me', password: 's3cret', serveraddress: 'quay.io' }); + }); + + it('prefers a per-registry helper over the general store', () => { + const runHelper = jest.fn(() => ({ Username: 'x', Secret: 'y' })); + resolveRegistryAuth('quay.io', deps({ + readConfig: () => ({ credsStore: 'desktop', credHelpers: { 'quay.io': 'ecr-login' } }), + runHelper: runHelper as RegistryAuthDeps['runHelper'], + })); + expect(runHelper).toHaveBeenCalledWith('ecr-login', 'quay.io'); + }); + + it('reads the default registry under the key docker writes it as', () => { + const runHelper = jest.fn(() => ({ Username: 'me', Secret: 'p' })); + resolveRegistryAuth('docker.io', deps({ + readConfig: () => ({ credsStore: 'desktop' }), + runHelper: runHelper as RegistryAuthDeps['runHelper'], + })); + expect(runHelper).toHaveBeenCalledWith('desktop', 'https://index.docker.io/v1/'); + }); + + it('falls back to an inline auths entry', () => { + const header = resolveRegistryAuth('quay.io', deps({ + readConfig: () => ({ auths: { 'quay.io': { auth: Buffer.from('user:pass').toString('base64') } } }), + })); + expect(decode(header)).toEqual({ username: 'user', password: 'pass', serveraddress: 'quay.io' }); + }); + + it('carries an identity token as a token, not as a password', () => { + const header = resolveRegistryAuth('quay.io', deps({ + readConfig: () => ({ credsStore: 'desktop' }), + runHelper: () => ({ Username: '', Secret: 'tok' }), + })); + expect(decode(header)).toEqual({ identitytoken: 'tok', serveraddress: 'quay.io' }); + }); + + it('returns nothing rather than throwing when there is no credential', () => { + expect(resolveRegistryAuth('quay.io', deps({}))).toBeUndefined(); + expect(resolveRegistryAuth('quay.io', deps({ readConfig: () => ({ credsStore: 'desktop' }) }))).toBeUndefined(); + expect(resolveRegistryAuth('quay.io', deps({ readConfig: () => ({ auths: { 'quay.io': { auth: 'not-base64-pair' } } }) }))).toBeUndefined(); + }); +}); diff --git a/src/main/docker/registry-auth.ts b/src/main/docker/registry-auth.ts new file mode 100644 index 0000000..6c2aefd --- /dev/null +++ b/src/main/docker/registry-auth.ts @@ -0,0 +1,124 @@ +/** + * Registry credentials, resolved in the app and attached by the filtering + * socket. + * + * The point of this module is what the job never sees. Under the old `docker: + * credentials` level a job read `~/.docker/config.json` itself, so using a + * private registry meant handing the repository the operator's secrets. Here + * the app reads them - outside the sandbox, where `~/.docker` stays denied - + * and the proxy attaches an X-Registry-Auth header to a pull the policy already + * permits. Naming a registry in `pull.registries` is the whole grant. + * + * The resolution order follows the docker CLI: a per-registry credential + * helper, then the configured credential store, then an inline `auths` entry. + */ + +import { execFileSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +/** Docker's own name for the default registry, as it appears in config.json. */ +const DEFAULT_REGISTRY = 'docker.io'; +const DEFAULT_REGISTRY_KEY = 'https://index.docker.io/v1/'; + +interface DockerConfig { + auths?: Record; + credsStore?: string; + credHelpers?: Record; +} + +/** What a credential helper prints on stdout. */ +interface HelperCredentials { + ServerURL?: string; + Username?: string; + Secret?: string; +} + +export interface RegistryAuthDeps { + readConfig: () => DockerConfig | null; + /** Run `docker-credential- get` with `serverUrl` on stdin. */ + runHelper: (helper: string, serverUrl: string) => HelperCredentials | null; +} + +const configPath = (): string => path.join(os.homedir(), '.docker', 'config.json'); + +const nodeDeps: RegistryAuthDeps = { + readConfig: () => { + try { + return JSON.parse(fs.readFileSync(configPath(), 'utf-8')) as DockerConfig; + } catch { + // No config, or one we cannot read: the job simply pulls unauthenticated. + return null; + } + }, + runHelper: (helper, serverUrl) => { + try { + const stdout = execFileSync(`docker-credential-${helper}`, ['get'], { + input: serverUrl, + encoding: 'utf-8', + timeout: 10_000, + }); + return JSON.parse(stdout) as HelperCredentials; + } catch { + // A helper that errors means no stored credential for this registry, + // which is the same as having none. + return null; + } + }, +}; + +/** Every key a registry may be stored under, most specific first. */ +function configKeys(registry: string): string[] { + if (registry === DEFAULT_REGISTRY || registry === 'index.docker.io') { + return [DEFAULT_REGISTRY_KEY, 'index.docker.io', DEFAULT_REGISTRY]; + } + return [registry, `https://${registry}`, `${registry}/v1/`, `https://${registry}/v1/`]; +} + +/** The value of an X-Registry-Auth header: base64 of the AuthConfig JSON. */ +function encode(auth: Record): string { + return Buffer.from(JSON.stringify(auth)).toString('base64'); +} + +/** + * The X-Registry-Auth value for a registry, or undefined when the operator has + * no credential for it. Never throws: a pull that cannot be authenticated is + * still a pull, and an anonymous one may well succeed. + */ +export function resolveRegistryAuth(registry: string, deps: RegistryAuthDeps = nodeDeps): string | undefined { + const config = deps.readConfig(); + if (!config) return undefined; + + const keys = configKeys(registry); + const serveraddress = keys[0]; + + const helper = keys.map((key) => config.credHelpers?.[key]).find((h) => h !== undefined) ?? config.credsStore; + if (helper) { + const credentials = deps.runHelper(helper, serveraddress); + if (credentials?.Secret) { + // A helper answers with the literal username when the secret is + // an identity token rather than a password. + return credentials.Username === '' + ? encode({ identitytoken: credentials.Secret, serveraddress }) + : encode({ username: credentials.Username ?? '', password: credentials.Secret, serveraddress }); + } + } + + for (const key of keys) { + const entry = config.auths?.[key]; + if (!entry) continue; + if (entry.identitytoken) return encode({ identitytoken: entry.identitytoken, serveraddress }); + if (!entry.auth) continue; + const decoded = Buffer.from(entry.auth, 'base64').toString('utf-8'); + const separator = decoded.indexOf(':'); + if (separator === -1) continue; + return encode({ + username: decoded.slice(0, separator), + password: decoded.slice(separator + 1), + serveraddress, + }); + } + + return undefined; +} diff --git a/src/main/index.ts b/src/main/index.ts index b609b47..4b0df56 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -102,6 +102,7 @@ import { // Zustand store import { initStore, connectWindow, cleanupStore, store } from './store/init'; import { getEffectivePolicy, effectivePolicyLevel } from '../shared/localmostrc'; +import { resolveRegistryAuth } from './docker/registry-auth'; import { decidePolicyForJob, recordPendingPolicy, @@ -268,6 +269,10 @@ app.whenReady().then(async () => { const runnerManager = new RunnerManager({ onLog: sendLog, + // Resolved here, in the app, where ~/.docker is readable. The job never + // sees a credential: the filtering socket attaches this to a pull the + // policy already permits, so naming a registry is the whole grant. + attachRegistryAuth: (registry: string) => resolveRegistryAuth(registry), onStatusChange: sendStatusUpdate, onJobHistoryUpdate: sendJobHistoryUpdate, onReregistrationNeeded: reRegisterSingleInstance, From 4fd6d92790420155cd426a28d832fb01a524db29 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 15:13:28 -0400 Subject: [PATCH 11/31] Refuse a request target the filter cannot read, instead of throwing parseDockerRequest called new URL() on the raw target with nothing catching it, so `GET //` or `GET http://[` threw out of the http request handler: no refusal was written and the connection sat open until the client gave up. A filter that hangs is a filter that fails open in the only way that matters - the request is neither allowed nor denied, and nobody is told. Worse than the throw is the target that parses. `//evil/v1.45/containers/json` resolves here to host=evil, path=/containers/json, while the daemon reads the target as written - so the filter would judge one request and forward another. Only origin-form targets are accepted now; anything else is reported by the parser and refused with 400 naming the target, which is a bad request rather than a policy denial and reads that way to whoever hit it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/main/docker/docker-filter-proxy.test.ts | 31 +++++++++++++++++++++ src/main/docker/docker-filter-proxy.ts | 7 +++++ src/main/docker/docker-request.test.ts | 26 +++++++++++++++++ src/main/docker/docker-request.ts | 28 ++++++++++++++++++- 4 files changed, 91 insertions(+), 1 deletion(-) diff --git a/src/main/docker/docker-filter-proxy.test.ts b/src/main/docker/docker-filter-proxy.test.ts index 8962b62..30b6220 100644 --- a/src/main/docker/docker-filter-proxy.test.ts +++ b/src/main/docker/docker-filter-proxy.test.ts @@ -695,3 +695,34 @@ describe('mount sources are pinned before forwarding', () => { expect(binds[0]).toBe(`${real}:/ws`); }); }); + +describe('a request target the filter cannot read', () => { + it('is refused, and the connection does not hang', async () => { + const dir = tmp(); + const daemon = await fakeDaemon(dir); + const { proxy, sock } = await startProxy(dir, { backend: backendWith(daemon.sock, dir) }); + proxy.bind('owner/repo', { run: { images: ['postgres:16'], network: 'bridge' } }); + + // Resolve on the response head, not on close: HTTP/1.1 keep-alive means a + // correctly-answered request leaves the socket open. + const answered = await new Promise((resolve, reject) => { + let buffered = ''; + const client = net.connect(sock); + const done = (v: string) => { clearTimeout(timer); client.destroy(); resolve(v); }; + const timer = setTimeout(() => { client.destroy(); reject(new Error('no answer within 3s: the connection hung')); }, 3000); + client.on('connect', () => client.write('GET //evil/v1.45/containers/json HTTP/1.1\r\nHost: docker\r\n\r\n')); + client.on('data', (c: Buffer) => { buffered += c.toString(); if (buffered.includes('\r\n\r\n')) done(buffered); }); + client.on('error', (e) => { clearTimeout(timer); reject(e); }); + client.on('close', () => { clearTimeout(timer); resolve(buffered); }); + }); + + // 400, naming the target: a target the filter cannot read is a bad + // request, not a policy denial, and saying so is the difference between + // "fix your URL" and "ask your operator for a grant". + expect(answered).toMatch(/^HTTP\/1\.[01] 400/); + expect(answered).toMatch(/origin-form|could not be parsed/); + // Nothing reached the daemon. + expect(daemon.seen).toHaveLength(0); + expect(proxy.isRunning()).toBe(true); + }); +}); diff --git a/src/main/docker/docker-filter-proxy.ts b/src/main/docker/docker-filter-proxy.ts index e53efc0..63e75fe 100644 --- a/src/main/docker/docker-filter-proxy.ts +++ b/src/main/docker/docker-filter-proxy.ts @@ -259,6 +259,13 @@ export class DockerFilterProxy { /** Null when the request may proceed; otherwise the status and message that refuse it. */ private decide(req: DockerRequest): { refusal: { status: number; message: string } | null; rewrittenBody?: unknown } { + // A target the parser could not read is a request the filter cannot judge. + // Before this, the parse threw out of the request handler: no refusal was + // written and the connection sat open until the client gave up. + if (req.targetError) { + this.onLog({ level: 'info', message: `refused ${req.method} ${req.raw.url}: ${req.targetError}` }); + return { refusal: { status: 400, message: req.targetError } }; + } if (req.apiVersion) { const version = parseApiVersion(req.apiVersion); if ( diff --git a/src/main/docker/docker-request.test.ts b/src/main/docker/docker-request.test.ts index 4ad0093..c0547cd 100644 --- a/src/main/docker/docker-request.test.ts +++ b/src/main/docker/docker-request.test.ts @@ -121,3 +121,29 @@ describe('container lifecycle endpoints the run action covers', () => { } }); }); + +describe('request targets that are not plain origin-form paths', () => { + const parse = (url: string) => parseDockerRequest({ method: 'GET', url, headers: {}, body: Buffer.alloc(0) }); + + it('does not throw on a target the URL parser rejects', () => { + for (const url of ['//', 'http://[', 'http://user@[::1]:99999/x']) { + expect(() => parse(url)).not.toThrow(); + expect(parse(url).targetError).toBeTruthy(); + } + }); + + it('refuses a target carrying an authority, which the filter and the daemon would read differently', () => { + // `//evil/x` parses to host=evil, path=/x here, while the daemon reads the + // request target as written. Judging one and forwarding the other is how a + // filter gets talked past. + expect(parse('//evil/v1.45/containers/json').targetError).toBeTruthy(); + expect(parse('http://evil/v1.45/_ping').targetError).toBeTruthy(); + }); + + it('leaves an ordinary path alone', () => { + const req = parse('/v1.45/containers/json?all=1'); + expect(req.targetError).toBeUndefined(); + expect(req.path).toBe('/containers/json'); + expect(req.query.all).toBe('1'); + }); +}); diff --git a/src/main/docker/docker-request.ts b/src/main/docker/docker-request.ts index 4631f6d..35565c3 100644 --- a/src/main/docker/docker-request.ts +++ b/src/main/docker/docker-request.ts @@ -44,6 +44,14 @@ export interface DockerRequest { body?: unknown; /** Set when the content type promised JSON and the body did not parse. */ bodyError?: string; + /** + * Set when the request target is not a plain origin-form path. The filter + * refuses these rather than guessing: a target carrying an authority + * (`//evil/x`, `http://evil/x`) is read one way by the URL parser here and + * another by the daemon, and judging one while forwarding the other is how a + * filter gets talked past. + */ + targetError?: string; raw: { method: string; url: string; headers: Record; body: Buffer }; } @@ -61,9 +69,27 @@ const isJsonContentType = (contentType: string | undefined): boolean => contentType !== undefined && contentType.split(';')[0].trim().toLowerCase() === 'application/json'; export function parseDockerRequest(raw: DockerRequest['raw']): DockerRequest { + // Only origin-form is accepted. Anything else either throws here (`//`, + // `http://[`) or parses to a different path than the daemon will read, and + // both are refusals rather than guesses. + if (!raw.url.startsWith('/') || raw.url.startsWith('//')) { + return { + method: raw.method, path: raw.url, query: {}, raw, + targetError: `request target "${raw.url}" is not a plain path; the localmost docker socket accepts origin-form targets only`, + }; + } + // The base is a placeholder so a path-only URL parses; only pathname and // search are read from the result. - const url = new URL(raw.url, 'http://docker'); + let url: URL; + try { + url = new URL(raw.url, 'http://docker'); + } catch { + return { + method: raw.method, path: raw.url, query: {}, raw, + targetError: `request target "${raw.url}" could not be parsed`, + }; + } let path = url.pathname; let apiVersion: string | undefined; From f011fe08950116ee84da3e002a241679fef68436 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 15:16:29 -0400 Subject: [PATCH 12/31] Key per-workflow policy on the workflow filename, as documented The docs say a `workflows:` key matches the workflow filename. An earlier fix in this branch moved the lookup off the scraped job name and onto github.workflow, which was closer but still wrong: github.workflow is the workflow's `name:`, a free-form string that equals the filename only by coincidence. A repository following the documented contract still saw its per-workflow section ignored. github.workflow_ref carries the real path, so the filename is taken from there and the name stays as the fallback for a job that arrives without it. Doing this in the broker means nothing else had to change: the consumer already reads githubWorkflow. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- docs/roadmap/localmostrc.md | 5 ++++- src/main/broker-proxy-service.test.ts | 22 ++++++++++++++++++++++ src/main/broker-proxy-service.ts | 20 ++++++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/docs/roadmap/localmostrc.md b/docs/roadmap/localmostrc.md index a6a002f..7045087 100644 --- a/docs/roadmap/localmostrc.md +++ b/docs/roadmap/localmostrc.md @@ -250,7 +250,10 @@ workflows: - Each workflow gets exactly what it needs, nothing more **Workflow matching:** -- Keys under `workflows:` match the workflow filename (without `.yml`/`.yaml`) +- Keys under `workflows:` match the workflow filename (without `.yml`/`.yaml`), + taken from `github.workflow_ref`. Where a job arrives without that — an older + runner service — the workflow's `name:` is used instead, so name a section + after the file and the two agree - `build` matches `.github/workflows/build.yml` - For matrix workflows, all jobs in the workflow share the workflow's policy diff --git a/src/main/broker-proxy-service.test.ts b/src/main/broker-proxy-service.test.ts index 7b6ea1c..fd48dde 100644 --- a/src/main/broker-proxy-service.test.ts +++ b/src/main/broker-proxy-service.test.ts @@ -450,3 +450,25 @@ describe('extractGitHubJobInfo', () => { expect(info.githubWorkflow).toBe('integration'); }); }); + +describe('the workflow a per-workflow policy section keys on', () => { + it('uses the workflow filename, which is what .localmostrc keys are documented to match', () => { + const info = extractGitHubJobInfo({ github: { d: [ + { k: 'workflow', v: 'CI / build and test' }, + { k: 'workflow_ref', v: 'bfulton/localmost/.github/workflows/ci.yaml@refs/heads/main' }, + ] } }); + expect(info.githubWorkflow).toBe('ci'); + }); + + it('handles a .yml extension and a ref containing slashes', () => { + const info = extractGitHubJobInfo({ github: { d: [ + { k: 'workflow_ref', v: 'o/r/.github/workflows/docker-access.yml@refs/pull/35/merge' }, + ] } }); + expect(info.githubWorkflow).toBe('docker-access'); + }); + + it('falls back to the workflow name when no ref is supplied', () => { + const info = extractGitHubJobInfo({ github: { d: [{ k: 'workflow', v: 'Docker Access' }] } }); + expect(info.githubWorkflow).toBe('Docker Access'); + }); +}); diff --git a/src/main/broker-proxy-service.ts b/src/main/broker-proxy-service.ts index 147570e..f57f10a 100644 --- a/src/main/broker-proxy-service.ts +++ b/src/main/broker-proxy-service.ts @@ -198,11 +198,22 @@ interface ContextDictEntry { * Pull the job's GitHub identity out of the broker job details' contextData. * Pure, so the mapping from context keys to job info can be tested directly. */ +/** The workflow filename, without extension, from a github.workflow_ref value. */ +function workflowFilename(ref: string | undefined): string | undefined { + if (!ref) return undefined; + // owner/repo/.github/workflows/@; the ref itself may contain '/'. + const path = ref.split('@')[0]; + const file = path.slice(path.lastIndexOf('/') + 1); + if (!file) return undefined; + return file.replace(/\.ya?ml$/i, ''); +} + export function extractGitHubJobInfo(contextData: { github?: { d?: ContextDictEntry[] }; job?: { d?: ContextDictEntry[] }; } | undefined): GitHubJobInfo { const info: GitHubJobInfo = {}; + let workflowRef: string | undefined; const github = contextData?.github; if (github?.d && Array.isArray(github.d)) { @@ -213,9 +224,18 @@ export function extractGitHubJobInfo(contextData: { if (item.k === 'sha') info.githubSha = item.v; if (item.k === 'ref') info.githubRef = item.v; if (item.k === 'workflow') info.githubWorkflow = item.v; + if (item.k === 'workflow_ref') workflowRef = item.v; } } + // `.localmostrc` keys under `workflows:` match the workflow FILENAME, but + // `github.workflow` is the workflow's `name:` - a free-form string that only + // equals the filename by coincidence. `github.workflow_ref` carries the real + // path (owner/repo/.github/workflows/@), so the filename comes + // from there when the service sends it, and the name remains the fallback. + const fromRef = workflowFilename(workflowRef); + if (fromRef) info.githubWorkflow = fromRef; + // Job ID (check_run_id) is in the job context const job = contextData?.job; if (job?.d && Array.isArray(job.d)) { From c7c8c7e502837306e6cbaaeab73e850f0552731b Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 15:20:28 -0400 Subject: [PATCH 13/31] Clear a slot's job context when the slot is released releaseInstanceSlot dropped the instance and left the finished job's target context behind under the same slot number. The next worker to take that slot was then judged against the previous repository: its docker socket refused the job it was actually running, and a worker that records no context of its own - one that picked a job up without going through spawnWorkerForJob - would resolve the previous repository's approved filesystem policy at spawn. The context describes one job in one slot, so it goes when the slot does. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/main/runner-manager.test.ts | 29 +++++++++++++++++++++++++++++ src/main/runner-manager.ts | 6 ++++++ 2 files changed, 35 insertions(+) diff --git a/src/main/runner-manager.test.ts b/src/main/runner-manager.test.ts index f02fde4..dda28b7 100644 --- a/src/main/runner-manager.test.ts +++ b/src/main/runner-manager.test.ts @@ -1612,3 +1612,32 @@ describe('a worker constrained by policy drift stays constrained', () => { expect(proxy.setPolicyAllowedHosts).toHaveBeenLastCalledWith([]); }); }); + +describe('a released slot does not carry the finished job\'s context', () => { + it('does not judge the next worker in that slot against the previous repository', async () => { + const docker = { run: { images: ['alpine:3'] } }; + const manager = new RunnerManager({ + onLog: jest.fn(), onStatusChange: jest.fn(), onJobHistoryUpdate: jest.fn(), + getRepoPolicy: async () => ({ hosts: [], level: 'strict' as const, readPaths: [], writePaths: [], docker }), + }); + const helper = new RunnerManagerTestHelper(manager); + + // Slot 1 ran a job for owner/first, then the worker went away. + helper.setInstance(1, { name: 'runner-1', status: 'listening' }); + helper.setPendingTargetContext('1', { targetId: 't1', targetDisplayName: 'owner/first', githubSha: 'aaa1111' }); + helper.releaseInstanceSlot(1); + + // The slot is reused for a different repository, by a worker that did not + // go through spawnWorkerForJob and so records no context of its own. + const dockerSocket = { bind: jest.fn(), boundRepository: jest.fn() }; + helper.setProxy(1, { setPolicyAllowedHosts: jest.fn(), setPolicyLevel: jest.fn() }); + helper.setDockerProxy(1, dockerSocket); + helper.setInstance(1, { name: 'runner-1', status: 'busy', claimedRepository: 'owner/second' }); + + await helper.applyPolicyOnClaim(1, 'owner/second', 'bbb2222'); + + // With the previous job's context still in the slot, this worker is judged + // against owner/first and its socket never opens for the job it is running. + expect(dockerSocket.bind).toHaveBeenCalledWith('owner/second', docker); + }); +}); diff --git a/src/main/runner-manager.ts b/src/main/runner-manager.ts index 8b13578..54f7a9c 100644 --- a/src/main/runner-manager.ts +++ b/src/main/runner-manager.ts @@ -1295,6 +1295,12 @@ export class RunnerManager { instance.status = 'offline'; } this.instances.delete(instanceNum); + // The context describes the job this slot just finished. Left behind, the + // next worker to take the slot is judged against the previous repository - + // its docker socket refuses the job it is actually running, and a spawn + // that records no context of its own would resolve the previous + // repository's filesystem policy. + this.pendingTargetContext.delete(String(instanceNum)); this.updateAggregateStatus(); } From bc48a5cbbf275f7f87f55f39996dc54b12a001ad Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 15:23:17 -0400 Subject: [PATCH 14/31] Address a job's containers by name, and forget them when removed Two findings about container identity, from opposite directions. A job that runs `docker run --name mine` never sees an id: create carries the name as a query parameter and every later call addresses "mine". Only the id from the create response was recorded, so the job was refused access to the container it had just created. The name is recorded too now. The other way, ownership was too generous. A bare prefix of an owned id counted, on the reasoning that the daemon accepts one, and nothing was ever removed from the set - so after a job removed its container, that container's prefix, and its name, still opened the door to whatever the daemon resolved them to next on a shared machine. Identifiers must match exactly now, and a removal the daemon confirms forgets every identifier for that container. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/main/docker/docker-evaluator.ts | 13 ++++--- src/main/docker/docker-filter-proxy.test.ts | 37 +++++++++++++++++++ src/main/docker/docker-filter-proxy.ts | 41 +++++++++++++++++++-- 3 files changed, 81 insertions(+), 10 deletions(-) diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index 612f525..d9b7903 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -575,17 +575,18 @@ function evaluateBuild(req: DockerRequest, policy: DockerPolicy): DockerVerdict } /** - * Permit a per-container request only against a container this socket created. - * The daemon accepts a unique id prefix, so a known id whose prefix was given - * counts as the same container; anything else is another job's, or the - * operator's, and is refused. + * Permit a per-container request only against a container this socket created, + * addressed by the id the daemon assigned or the name the job asked for. + * Anything else is another job's container, or the operator's, and is refused. */ function evaluateOwnContainer(req: DockerRequest, ctx: DockerEvalContext): DockerVerdict { const id = containerIdFrom(req); if (!id) return deny(`${req.method} ${req.path} is not permitted through the localmost docker socket`); - const own = ctx.ownContainerIds; - if (own && (own.has(id) || [...own].some((known) => known.startsWith(id)))) return ALLOW; + // Exact match only. A bare prefix used to count, on the reasoning that the + // daemon accepts one - but a prefix of a container this job has since + // removed can resolve on the shared daemon to somebody else's. + if (ctx.ownContainerIds?.has(id)) return ALLOW; return deny( `container "${id}" was not created through this job's docker socket; only this job's own containers can be addressed` diff --git a/src/main/docker/docker-filter-proxy.test.ts b/src/main/docker/docker-filter-proxy.test.ts index 30b6220..d31c51a 100644 --- a/src/main/docker/docker-filter-proxy.test.ts +++ b/src/main/docker/docker-filter-proxy.test.ts @@ -726,3 +726,40 @@ describe('a request target the filter cannot read', () => { expect(proxy.isRunning()).toBe(true); }); }); + +describe('which containers a job may address', () => { + const setup = async () => { + const dir = tmp(); + const daemon = await fakeDaemon(dir); + const { proxy, sock } = await startProxy(dir, { backend: backendWith(daemon.sock, dir) }); + proxy.bind('owner/repo', { run: { images: ['postgres:16'], network: 'bridge' } }); + return { sock, daemon }; + }; + + it('lets a job address the container it created by --name', async () => { + const { sock } = await setup(); + // docker run --name mine ... -> POST /containers/create?name=mine, and + // every later call addresses it as "mine", never as the id. + expect((await request(sock, 'POST', '/v1.45/containers/create?name=mine', { Image: 'postgres:16' })).status).toBe(201); + expect((await request(sock, 'POST', '/v1.45/containers/mine/start')).status).toBeLessThan(400); + expect((await request(sock, 'GET', '/v1.45/containers/mine/json')).status).toBeLessThan(400); + }); + + it('forgets a container once it is removed, so its name cannot be reused', async () => { + const { sock } = await setup(); + await request(sock, 'POST', '/v1.45/containers/create?name=mine', { Image: 'postgres:16' }); + expect((await request(sock, 'DELETE', '/v1.45/containers/mine')).status).toBeLessThan(400); + // The container is gone; the daemon may hand that name to anyone next. + expect((await request(sock, 'GET', '/v1.45/containers/mine/json')).status).toBe(403); + expect((await request(sock, 'GET', '/v1.45/containers/abc123/json')).status).toBe(403); + }); + + it('does not accept a bare prefix of an owned id', async () => { + const { sock } = await setup(); + // The fake daemon answers create with Id abc123. A prefix could resolve on + // the real daemon to a container this job never created. + expect((await request(sock, 'POST', '/v1.45/containers/create', { Image: 'postgres:16' })).status).toBe(201); + expect((await request(sock, 'GET', '/v1.45/containers/abc123/json')).status).toBeLessThan(400); + expect((await request(sock, 'GET', '/v1.45/containers/ab/json')).status).toBe(403); + }); +}); diff --git a/src/main/docker/docker-filter-proxy.ts b/src/main/docker/docker-filter-proxy.ts index 63e75fe..a6609fa 100644 --- a/src/main/docker/docker-filter-proxy.ts +++ b/src/main/docker/docker-filter-proxy.ts @@ -17,7 +17,7 @@ import * as net from 'net'; import * as path from 'path'; import { DockerPolicy } from '../../shared/docker-policy'; import { DockerBackend } from './docker-backend'; -import { DockerRequest, classifyDockerRequest, parseDockerRequest } from './docker-request'; +import { DockerRequest, classifyDockerRequest, containerIdFrom, parseDockerRequest } from './docker-request'; import { evaluateDockerRequest, registryOf } from './docker-evaluator'; export interface DockerFilterProxyLogEntry { @@ -96,6 +96,8 @@ export class DockerFilterProxy { * job's container by naming its id. */ private readonly ownContainerIds = new Set(); + /** Each identifier this socket may address, mapped to the container it names. */ + private readonly ownContainerAliases = new Map(); private repository: string | undefined; private readonly backend: DockerBackend; private readonly onLog: (entry: DockerFilterProxyLogEntry) => void; @@ -126,6 +128,23 @@ export class DockerFilterProxy { this.realpath = options.realpath ?? ((p) => fs.realpathSync(p)); } + /** Record an identifier the job may use for a container it created. */ + private own(alias: string, containerId: string): void { + this.ownContainerAliases.set(alias, containerId); + this.ownContainerIds.add(alias); + } + + /** Forget every identifier for a container the job has removed. */ + private disown(alias: string): void { + const containerId = this.ownContainerAliases.get(alias); + if (containerId === undefined) return; + for (const [known, owner] of [...this.ownContainerAliases]) { + if (owner !== containerId) continue; + this.ownContainerAliases.delete(known); + this.ownContainerIds.delete(known); + } + } + /** * Bind the socket to a repository and its policy. Until this is called the * socket denies everything but the baseline; the caller binds only once @@ -428,13 +447,20 @@ export class DockerFilterProxy { { socketPath, path: this.forwardedUrl(parsed), method: parsed.method, headers, agent: this.upstreamAgent }, (upstreamRes) => { upstreamRes.on('error', () => res.destroy()); + // A container the daemon actually removed is no longer this job's to + // address; its name in particular may be handed to anyone next. + const removedStatus = upstreamRes.statusCode ?? 502; + if (action === 'remove' && removedStatus >= 200 && removedStatus < 300) { + const addressed = containerIdFrom(parsed); + if (addressed) this.disown(addressed); + } const relayed = action === 'ping' ? this.relayPing(upstreamRes, res) : action === 'version' ? this.relayVersion(upstreamRes, res) : action === 'create' - ? this.relayCreate(upstreamRes, res) + ? this.relayCreate(upstreamRes, res, parsed) : this.relay(upstreamRes, res); relayed.then(() => { answered = true; @@ -513,7 +539,7 @@ export class DockerFilterProxy { * to containers this job actually created. The body is small and the client * needs the id before it can proceed, so buffering it costs nothing. */ - private relayCreate(upstreamRes: http.IncomingMessage, res: http.ServerResponse): Promise { + private relayCreate(upstreamRes: http.IncomingMessage, res: http.ServerResponse, requested: DockerRequest): Promise { return new Promise((resolve) => { const chunks: Buffer[] = []; upstreamRes.on('data', (c: Buffer) => chunks.push(c)); @@ -524,7 +550,14 @@ export class DockerFilterProxy { if (status >= 200 && status < 300) { try { const parsed = JSON.parse(raw.toString('utf8')) as Record; - if (typeof parsed.Id === 'string' && parsed.Id.length > 0) this.ownContainerIds.add(parsed.Id); + if (typeof parsed.Id === 'string' && parsed.Id.length > 0) { + // A job addresses its container by whichever identifier it + // knows: the id the daemon just assigned, or the --name it + // asked for, which is the only one it ever sees when it uses one. + this.own(parsed.Id, parsed.Id); + const name = requested.query.name; + if (name) this.own(name, parsed.Id); + } } catch { // An unreadable create response leaves the container unowned: the // job cannot address it, which fails closed rather than open. From 126bc254fbbcce9826d452410bf4a0534b0306bf Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 15:34:08 -0400 Subject: [PATCH 15/31] Root mounts at the checkout, and reject privileged at approval Declared mount paths resolved against the runner's _work folder, but a job checks out into _work// - GITHUB_WORKSPACE, and what `./` means to whoever wrote the policy. So `path: ./tmp/fixtures` resolved to _work/tmp/fixtures, which never exists: every declared path narrower than `./` was silently unmatchable. The root follows the repository the socket is bound to, and falls back to the work folder while nothing is bound. `privileged: true` also validated clean and was then refused on every request. The design keeps it in the grammar so the capability gap stays visible and rejects it until a backend can contain it, which means rejecting the declaration rather than accepting one that can never be honoured - the second reads as a broken policy instead of a stage that has not shipped. That is the last of the twenty-one findings the review confirmed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/main/docker/docker-backend.test.ts | 15 +++++++++++++++ src/main/docker/docker-backend.ts | 17 ++++++++++++++--- src/main/docker/docker-filter-proxy.ts | 2 +- src/shared/docker-policy.test.ts | 24 ++++++++++++++++++++++-- src/shared/docker-policy.ts | 9 +++++++++ src/shared/localmostrc.test.ts | 1 - 6 files changed, 61 insertions(+), 7 deletions(-) diff --git a/src/main/docker/docker-backend.test.ts b/src/main/docker/docker-backend.test.ts index cd368f0..a73a653 100644 --- a/src/main/docker/docker-backend.test.ts +++ b/src/main/docker/docker-backend.test.ts @@ -54,3 +54,18 @@ describe('DesktopBackend', () => { expect(backend.workspaceMountRoot('/tmp/sandbox/1')).toBe('/tmp/sandbox/1/checkout'); }); }); + +describe('the root that declared mount paths resolve against', () => { + const backend = new DesktopBackend({ resolve: () => null }); + + it('is the repository checkout, which is what "./" means in a workflow', () => { + // The runner checks out into _work// (GITHUB_WORKSPACE). Rooting + // at _work instead made every declared path narrower than "./" unmatchable: + // "./tmp/fixtures" resolved to _work/tmp/fixtures, which never exists. + expect(backend.workspaceMountRoot('/s/1', 'bfulton/localmost')).toBe('/s/1/_work/localmost/localmost'); + }); + + it('falls back to the work folder when no repository is bound yet', () => { + expect(backend.workspaceMountRoot('/s/1')).toBe('/s/1/_work'); + }); +}); diff --git a/src/main/docker/docker-backend.ts b/src/main/docker/docker-backend.ts index 00048e9..d3f73ac 100644 --- a/src/main/docker/docker-backend.ts +++ b/src/main/docker/docker-backend.ts @@ -19,7 +19,13 @@ export interface DockerBackend { /** The daemon endpoint to forward approved requests to, or null when none. */ resolveEndpoint(): DockerEndpoint | null; /** Absolute host path that job mounts must resolve inside (the job workspace). */ - workspaceMountRoot(sandboxDir: string): string; + /** + * The directory declared mount paths resolve against: the repository + * checkout when the socket is bound to one, since that is what `./` means to + * whoever wrote the policy. Without a repository - a socket not yet bound - + * the work folder is the widest honest answer. + */ + workspaceMountRoot(sandboxDir: string, repository?: string): string; } export interface DesktopBackendOptions { @@ -51,7 +57,12 @@ export class DesktopBackend implements DockerBackend { return this.opts.resolve ? this.opts.resolve() : resolveDockerEndpoint(); } - workspaceMountRoot(sandboxDir: string): string { - return path.join(sandboxDir, this.opts.workspaceSubdir ?? RUNNER_WORK_FOLDER); + workspaceMountRoot(sandboxDir: string, repository?: string): string { + const work = path.join(sandboxDir, this.opts.workspaceSubdir ?? RUNNER_WORK_FOLDER); + // The runner checks out into _work//, which is GITHUB_WORKSPACE + // and what a policy's `./` refers to. Rooting at _work made anything + // narrower than `./` unmatchable, since ./tmp resolved to _work/tmp. + const name = repository?.split('/').pop(); + return name ? path.join(work, name, name) : work; } } diff --git a/src/main/docker/docker-filter-proxy.ts b/src/main/docker/docker-filter-proxy.ts index a6609fa..806232a 100644 --- a/src/main/docker/docker-filter-proxy.ts +++ b/src/main/docker/docker-filter-proxy.ts @@ -225,7 +225,7 @@ export class DockerFilterProxy { /** The workspace the backend roots mounts at, resolved so symlinked sandbox dirs compare equal. */ private workspaceRoot(): string { - const root = this.backend.workspaceMountRoot(path.dirname(this.socketPath ?? '')); + const root = this.backend.workspaceMountRoot(path.dirname(this.socketPath ?? ''), this.repository); try { return this.realpath(root); } catch { diff --git a/src/shared/docker-policy.test.ts b/src/shared/docker-policy.test.ts index 17b6f63..a94f66b 100644 --- a/src/shared/docker-policy.test.ts +++ b/src/shared/docker-policy.test.ts @@ -45,12 +45,13 @@ describe('validateDockerPolicy', () => { expect(collect({ run: { images: ['postgres:16'], mounts: [{ path: './', mode: 'ro' }], network: 'bridge' } })).toEqual([]); }); - it('accepts pull, build and privileged alongside run', () => { + it('accepts pull, build and run together', () => { + // privileged is deliberately absent: it is rejected until a managed VM + // backend exists, and has a case of its own below. expect(collect({ pull: { registries: ['docker.io', 'ghcr.io'] }, run: { images: ['postgres:16'] }, build: { context: './' }, - privileged: true, })).toEqual([]); }); @@ -343,3 +344,22 @@ describe('diffDockerPolicy on bare action blocks', () => { expect(diffs.map((d) => d.path)).toContain('shared.docker.run.images'); }); }); + +describe('privileged at validation time', () => { + const collect = (value: unknown, path = 'shared.docker') => { + const errs: string[] = []; + validateDockerPolicy(value, path, (m) => errs.push(m)); + return errs; + }; + + it('rejects privileged: true, naming the backend it would require', () => { + // The design keeps privileged in the grammar so the gap stays honest, and + // rejects it until a managed VM can contain it. Accepting it here and + // refusing every request later reads as a broken policy, not a stage. + expect(collect({ privileged: true }).join('\n')).toMatch(/managed VM/i); + }); + + it('accepts privileged: false, which grants nothing', () => { + expect(collect({ privileged: false })).toEqual([]); + }); +}); diff --git a/src/shared/docker-policy.ts b/src/shared/docker-policy.ts index bef4967..7c53c83 100644 --- a/src/shared/docker-policy.ts +++ b/src/shared/docker-policy.ts @@ -89,6 +89,15 @@ export function validateDockerPolicy(value: unknown, path: string, push: (messag if (value.build !== undefined) validateBuild(value.build, `${path}.build`, push); if (value.privileged !== undefined && typeof value.privileged !== 'boolean') { push(`${path}.privileged must be a boolean`); + } else if (value.privileged === true) { + // Kept in the grammar so the capability gap stays visible, and refused + // until a backend exists that can contain it. Accepting the declaration + // here and then refusing every request it implies would read as a broken + // policy rather than a stage that has not shipped. + push( + `${path}.privileged requires a managed VM backend, which this build does not have; ` + + 'remove it, or run the work without privileged containers' + ); } } diff --git a/src/shared/localmostrc.test.ts b/src/shared/localmostrc.test.ts index 9d8faf1..e77500d 100644 --- a/src/shared/localmostrc.test.ts +++ b/src/shared/localmostrc.test.ts @@ -967,7 +967,6 @@ describe('docker policy through serialization', () => { pull: { registries: ['docker.io', 'ghcr.io'] }, run: { images: ['postgres:16'], mounts: [{ path: './', mode: 'ro' }], network: 'bridge' }, build: { context: './' }, - privileged: true, }, }, workflows: { From d1757a9a27a5ac61b70d698a49950e8860506a11 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 15:35:03 -0400 Subject: [PATCH 16/31] Model the real checkout layout in the docker e2e Rooting mounts at the repository checkout changed where the proxy expects a job's files to be, and the end-to-end test still built its workspace at _work. The two disagreed and two of the four cases failed - correctly: the test was describing a layout the runner does not use. It now asks the backend for the checkout path for the repository it binds, the way the proxy does, so the test and the thing under test agree about where a job's files live. Committed after the fact: the previous commit was pushed with this failing, because the gate and the commit ran in one step and the commit did not wait for the end-to-end result. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- test/e2e/docker.spec.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/test/e2e/docker.spec.ts b/test/e2e/docker.spec.ts index 28bcd4d..7d2bf3a 100644 --- a/test/e2e/docker.spec.ts +++ b/test/e2e/docker.spec.ts @@ -39,6 +39,9 @@ import { DockerPolicy } from '../../src/shared/docker-policy'; const IMAGE = 'alpine:3'; +/** The repository this socket is bound to; the checkout layout follows from it. */ +const REPOSITORY = 'owner/repo'; + /** * What a repository using Docker declares: one image, the workspace read-only, * the default network. Outside a job this file binds it; inside a job the @@ -129,10 +132,12 @@ test.describe('a job using docker through the filtering socket', () => { workspace = fs.realpathSync.native(jobWorkspace); } else { const backend = new DesktopBackend(); - // The checkout dir the backend roots mounts at, resolved as the daemon - // sees it: tmpdir is under /var, a symlink. - const workDir = backend.workspaceMountRoot(scratch); - fs.mkdirSync(workDir); + // The checkout the backend roots mounts at, for the repository this + // socket is bound to below: the runner lays it out as + // _work//, and declared paths resolve against it. Resolved + // as the daemon sees it, since tmpdir is under /var, a symlink. + const workDir = backend.workspaceMountRoot(scratch, REPOSITORY); + fs.mkdirSync(workDir, { recursive: true }); workspace = fs.realpathSync.native(workDir); socketPath = path.join(scratch, 'docker.sock'); @@ -140,7 +145,7 @@ test.describe('a job using docker through the filtering socket', () => { logs = captured; proxy = new DockerFilterProxy({ backend, onLog: (entry) => captured.push(entry) }); await proxy.start(socketPath); - proxy.bind('owner/repo', policy); + proxy.bind(REPOSITORY, policy); } fs.writeFileSync(path.join(workspace, 'hello.txt'), `${nonce}\n`); From fcbb3aa5bfd22e22a49930854c861c0c7230c387 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 15:47:20 -0400 Subject: [PATCH 17/31] Let a job create the internal network it seals itself with Consumer feedback, and the item worth prioritising because the direction was backwards: an --internal network makes a container less reachable, not more, so denying network creation forced strictly weaker isolation than the workload wanted. A harness that seals an agent under test behind a no-egress network could not build the thing doing the sealing. run: networks: - name: vk-* internal: true The name is an anchored glob and `internal` must be stated, so a routable network is a grant somebody asked for and the approval diff shows. The driver is unnameable: macvlan and ipvlan put a container on the physical LAN, which is worse than --network=host, and Options can bind a bridge to a host address. Any create key the filter does not recognise is refused, as with HostConfig. Reading and deleting a network is scoped to ones this socket created, by id and by the name the job asked for, and both are forgotten when the daemon confirms the delete. Listing stays denied - it enumerates the daemon. NetworkMode accepts a network this job created as well as the declared run.network. Without that the feature would be unusable: a job could create vk-1 and then be refused when it tried to run anything on it. Design: docs/superpowers/specs/2026-09-06-docker-endpoint-families.md Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/main/docker/docker-evaluator.test.ts | 49 ++++++++++++ src/main/docker/docker-evaluator.ts | 88 ++++++++++++++++++++- src/main/docker/docker-filter-proxy.test.ts | 50 ++++++++++++ src/main/docker/docker-filter-proxy.ts | 65 ++++++++++++++- src/main/docker/docker-request.test.ts | 5 +- src/main/docker/docker-request.ts | 21 +++++ src/shared/docker-policy.test.ts | 37 +++++++++ src/shared/docker-policy.ts | 75 +++++++++++++++++- 8 files changed, 382 insertions(+), 8 deletions(-) diff --git a/src/main/docker/docker-evaluator.test.ts b/src/main/docker/docker-evaluator.test.ts index 03e9d17..1a0fb1a 100644 --- a/src/main/docker/docker-evaluator.test.ts +++ b/src/main/docker/docker-evaluator.test.ts @@ -505,3 +505,52 @@ describe('build query parameters', () => { expect(build('?t=app%3Alatest&dockerfile=Dockerfile&rm=1&buildargs=%7B%7D&labels=%7B%7D&shmsize=0&version=1').allowed).toBe(true); }); }); + +describe('networks', () => { + const p: DockerPolicy = { run: { images: ['alpine:3'], network: 'bridge', networks: [{ name: 'vk-*', internal: true }] } }; + const create = (body: unknown, c = ctx(p)) => evaluateDockerRequest(mk('POST', '/v1.45/networks/create', body), c); + + it('permits creating a declared internal network', () => { + expect(create({ Name: 'vk-run1', Internal: true, CheckDuplicate: true }).allowed).toBe(true); + }); + + it('refuses a name no declaration matches, anchoring the glob', () => { + expect(create({ Name: 'other', Internal: true }).allowed).toBe(false); + expect(create({ Name: 'not-vk-run1', Internal: true }).allowed).toBe(false); + }); + + it('refuses a routable network where the declaration says internal', () => { + expect(create({ Name: 'vk-run1', Internal: false }).allowed).toBe(false); + expect(create({ Name: 'vk-run1' }).allowed).toBe(false); + }); + + it('refuses any create key the grammar cannot spell, driver above all', () => { + for (const extra of [{ Driver: 'macvlan' }, { Options: { parent: 'en0' } }, { IPAM: { Config: [] } }, { Attachable: true }, { Ingress: true }, { ConfigOnly: true }]) { + const body = { Name: 'vk-run1', Internal: true, ...extra }; + expect([Object.keys(extra)[0], create(body).allowed]).toEqual([Object.keys(extra)[0], false]); + } + // The default driver, stated explicitly, is the one the filter would use anyway. + expect(create({ Name: 'vk-run1', Internal: true, Driver: 'bridge' }).allowed).toBe(true); + }); + + it('never lists the daemon\'s networks', () => { + expect(evaluateDockerRequest(mk('GET', '/v1.45/networks'), ctx(p)).allowed).toBe(false); + }); + + it('scopes reading and deleting a network to ones this socket created', () => { + const own = ctx(p, { ownNetworkIds: new Set(['net123']) }); + expect(evaluateDockerRequest(mk('GET', '/v1.45/networks/net123'), own).allowed).toBe(true); + expect(evaluateDockerRequest(mk('DELETE', '/v1.45/networks/net123'), own).allowed).toBe(true); + expect(evaluateDockerRequest(mk('GET', '/v1.45/networks/theirs'), own).allowed).toBe(false); + expect(evaluateDockerRequest(mk('DELETE', '/v1.45/networks/theirs'), own).allowed).toBe(false); + }); + + it('lets a container join a network this job created, which is the point of declaring one', () => { + const own = ctx(p, { ownNetworkIds: new Set(['vk-run1']) }); + const body = { Image: 'alpine:3', HostConfig: { NetworkMode: 'vk-run1' } }; + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', body), own).allowed).toBe(true); + // An arbitrary network the job did not create is still refused. + const other = { Image: 'alpine:3', HostConfig: { NetworkMode: 'someone-elses' } }; + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', other), own).allowed).toBe(false); + }); +}); diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index d9b7903..8c43c48 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -13,8 +13,8 @@ import * as fs from 'fs'; import * as path from 'path'; -import { DockerPolicy, DockerMount, MountMode } from '../../shared/docker-policy'; -import { DockerRequest, DockerAction, classifyDockerRequest, containerIdFrom } from './docker-request'; +import { DockerPolicy, DockerMount, DockerNetworkPolicy, MountMode } from '../../shared/docker-policy'; +import { DockerRequest, DockerAction, classifyDockerRequest, containerIdFrom, networkIdFrom } from './docker-request'; export interface DockerEvalContext { /** The bound policy; null until the worker claims a job, which denies all. */ @@ -33,6 +33,12 @@ export interface DockerEvalContext { * operator and with other jobs, so an unscoped id reaches outside this job. */ ownContainerIds?: ReadonlySet; + /** + * Networks created through this socket, by id and by name. A container may + * join one of these as well as the declared `run.network`, which is the + * whole point of letting a job create one. + */ + ownNetworkIds?: ReadonlySet; /** Injected for tests; defaults to fs.realpathSync. Must throw when the path does not exist. */ realpath?: (p: string) => string; } @@ -105,6 +111,8 @@ const hints = { registry: (registry: string) => `docker:\n pull:\n registries:\n - ${registry}`, build: 'docker:\n build:\n context: "./"', privileged: 'docker:\n privileged: true', + network_declaration: (name: string, internal: boolean) => + `docker:\n run:\n networks:\n - name: ${yamlString(name)}\n internal: ${internal}`, }; // ----------------------------------------------------------------------------- @@ -488,7 +496,10 @@ function evaluateCreate(req: DockerRequest, ctx: DockerEvalContext, policy: Dock // The most restrictive reading wins when casings disagree. if (candidate !== 'bridge') mode = candidate; } - if (mode !== 'none' && mode !== policy.run.network) { + // A network this job created is as good as the declared one: creating it was + // already checked against run.networks, and refusing to join it would make + // declaring one pointless. + if (mode !== 'none' && mode !== policy.run.network && !ctx.ownNetworkIds?.has(mode)) { return deny( `network mode "${mode}" is not declared in the repository docker policy (run.network)`, hints.network(mode) @@ -537,6 +548,68 @@ const BUILD_PARAMS_KNOWN: ReadonlySet = new Set([ 'platform', 'version', 'buildid', 'session', ]); +/** Keys a network create may carry. Driver is permitted only as the default. */ +const NETWORK_CREATE_KNOWN: ReadonlySet = new Set(['name', 'internal', 'checkduplicate', 'labels', 'driver']); + +/** An anchored glob: `*` matches any run of characters, and nothing else is special. */ +function globMatches(pattern: string, value: string): boolean { + const escaped = pattern.replace(/[.*+?^${}()|[\]\\]/g, (c) => (c === '*' ? '\u0000' : `\\${c}`)); + return new RegExp(`^${escaped.split('\u0000').join('.*')}$`).test(value); +} + +function evaluateNetworkCreate(req: DockerRequest, policy: DockerPolicy): DockerVerdict { + const declared: DockerNetworkPolicy[] = policy.run?.networks ?? []; + if (declared.length === 0) { + return deny( + 'the repository docker policy declares no networks', + hints.network_declaration('name-of-your-network', true) + ); + } + const body = req.body; + if (!isPlainObject(body)) return deny('network create requires a JSON body'); + + for (const key of Object.keys(body)) { + if (!NETWORK_CREATE_KNOWN.has(key.toLowerCase())) { + // A macvlan or ipvlan network puts the container on the physical LAN, + // which is worse than host networking, and Options can bind a bridge to + // a host address. The filter creates a plain bridge or nothing. + return deny(`network create parameter "${key}" is not one the localmost docker socket understands`); + } + } + + const driver = pick(body, 'Driver'); + if (!isUnset(driver) && driver !== '' && driver !== 'bridge') { + return deny(`network driver "${String(driver)}" is not permitted; the localmost docker socket creates bridge networks only`); + } + + const name = pick(body, 'Name'); + if (typeof name !== 'string' || name === '') return deny('network create requires a Name'); + const internal = valuesFor(body, 'Internal').some((v) => v === true); + + const match = declared.find((n) => globMatches(n.name, name)); + if (!match) { + return deny( + `network "${name}" is not declared in the repository docker policy (run.networks)`, + hints.network_declaration(name, internal) + ); + } + if (match.internal && !internal) { + return deny( + `network "${name}" is declared internal, so it cannot be created routable`, + hints.network_declaration(match.name, false) + ); + } + return ALLOW; +} + +/** Permit a per-network request only against a network this socket created. */ +function evaluateOwnNetwork(req: DockerRequest, ctx: DockerEvalContext): DockerVerdict { + const id = networkIdFrom(req); + if (!id) return deny(`${req.method} ${req.path} is not permitted through the localmost docker socket`); + if (ctx.ownNetworkIds?.has(id)) return ALLOW; + return deny(`network "${id}" was not created through this job's docker socket`); +} + function evaluateBuild(req: DockerRequest, policy: DockerPolicy): DockerVerdict { if (!policy.build) return deny('the repository docker policy declares no build action', hints.build); // The Engine API carries the context as a tar the client assembled from @@ -631,6 +704,15 @@ export function evaluateDockerRequest(req: DockerRequest, ctx: DockerEvalContext return evaluatePull(req, policy); case 'build': return evaluateBuild(req, policy); + case 'network-create': + return evaluateNetworkCreate(req, policy); + case 'network-inspect': + case 'network-remove': + return evaluateOwnNetwork(req, ctx); + case 'network-list': + return deny( + 'listing networks is not permitted through the localmost docker socket; it would enumerate networks outside this job' + ); default: return deny(`${req.method} ${req.path} is not permitted through the localmost docker socket`); } diff --git a/src/main/docker/docker-filter-proxy.test.ts b/src/main/docker/docker-filter-proxy.test.ts index d31c51a..da80eaf 100644 --- a/src/main/docker/docker-filter-proxy.test.ts +++ b/src/main/docker/docker-filter-proxy.test.ts @@ -763,3 +763,53 @@ describe('which containers a job may address', () => { expect((await request(sock, 'GET', '/v1.45/containers/ab/json')).status).toBe(403); }); }); + +describe('networks a job creates', () => { + it('may be read, joined and deleted, and are forgotten once removed', async () => { + const dir = tmp(); + const daemon = await networkDaemon(dir); + const { proxy, sock } = await startProxy(dir, { backend: backendWith(daemon.sock, dir) }); + proxy.bind('owner/repo', { + run: { images: ['alpine:3'], network: 'bridge', networks: [{ name: 'vk-*', internal: true }] }, + }); + + expect((await request(sock, 'POST', '/v1.45/networks/create', { Name: 'vk-1', Internal: true })).status).toBe(201); + + // Both the id the daemon assigned and the name the job asked for. + expect((await request(sock, 'GET', '/v1.45/networks/net123')).status).toBeLessThan(400); + expect((await request(sock, 'GET', '/v1.45/networks/vk-1')).status).toBeLessThan(400); + // A container may join it. + expect((await request(sock, 'POST', '/v1.45/containers/create', { Image: 'alpine:3', HostConfig: { NetworkMode: 'vk-1' } })).status).toBe(201); + // Someone else's network is still refused. + expect((await request(sock, 'GET', '/v1.45/networks/theirs')).status).toBe(403); + + expect((await request(sock, 'DELETE', '/v1.45/networks/vk-1')).status).toBeLessThan(400); + expect((await request(sock, 'GET', '/v1.45/networks/vk-1')).status).toBe(403); + expect((await request(sock, 'GET', '/v1.45/networks/net123')).status).toBe(403); + }); +}); + +/** A fake daemon that also answers network create. */ +const networkDaemon = (dir: string): Promise<{ sock: string }> => + new Promise((resolve) => { + const sock = path.join(dir, 'netd.sock'); + const server = http.createServer((req, res) => { + const chunks: Buffer[] = []; + req.on('data', (c: Buffer) => chunks.push(c)); + req.on('end', () => { + const p = req.url!.replace(/^\/v\d+\.\d+/, '').split('?')[0]; + if (p === '/networks/create') { + res.writeHead(201, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ Id: 'net123', Warning: '' })); + } else if (p === '/containers/create') { + res.writeHead(201, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ Id: 'abc123', Warnings: [] })); + } else { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ ok: true })); + } + }); + }); + servers.push(server); + server.listen(sock, () => resolve({ sock })); + }); diff --git a/src/main/docker/docker-filter-proxy.ts b/src/main/docker/docker-filter-proxy.ts index 806232a..5ac9443 100644 --- a/src/main/docker/docker-filter-proxy.ts +++ b/src/main/docker/docker-filter-proxy.ts @@ -17,7 +17,7 @@ import * as net from 'net'; import * as path from 'path'; import { DockerPolicy } from '../../shared/docker-policy'; import { DockerBackend } from './docker-backend'; -import { DockerRequest, classifyDockerRequest, containerIdFrom, parseDockerRequest } from './docker-request'; +import { DockerRequest, classifyDockerRequest, containerIdFrom, networkIdFrom, parseDockerRequest } from './docker-request'; import { evaluateDockerRequest, registryOf } from './docker-evaluator'; export interface DockerFilterProxyLogEntry { @@ -82,6 +82,9 @@ function flattenHeaders(headers: http.IncomingHttpHeaders): Record => + typeof v === 'object' && v !== null && !Array.isArray(v); + const isJsonContentType = (contentType: string | undefined): boolean => contentType !== undefined && contentType.split(';')[0].trim().toLowerCase() === 'application/json'; @@ -98,6 +101,9 @@ export class DockerFilterProxy { private readonly ownContainerIds = new Set(); /** Each identifier this socket may address, mapped to the container it names. */ private readonly ownContainerAliases = new Map(); + /** Networks created through this socket, by id and by the name the job asked for. */ + private readonly ownNetworkIds = new Set(); + private readonly ownNetworkAliases = new Map(); private repository: string | undefined; private readonly backend: DockerBackend; private readonly onLog: (entry: DockerFilterProxyLogEntry) => void; @@ -134,6 +140,23 @@ export class DockerFilterProxy { this.ownContainerIds.add(alias); } + /** Record an identifier for a network the job created. */ + private ownNetwork(alias: string, networkId: string): void { + this.ownNetworkAliases.set(alias, networkId); + this.ownNetworkIds.add(alias); + } + + /** Forget every identifier for a network the job has removed. */ + private disownNetwork(alias: string): void { + const networkId = this.ownNetworkAliases.get(alias); + if (networkId === undefined) return; + for (const [known, owner] of [...this.ownNetworkAliases]) { + if (owner !== networkId) continue; + this.ownNetworkAliases.delete(known); + this.ownNetworkIds.delete(known); + } + } + /** Forget every identifier for a container the job has removed. */ private disown(alias: string): void { const containerId = this.ownContainerAliases.get(alias); @@ -304,6 +327,7 @@ export class DockerFilterProxy { workspaceRoot: this.workspaceRoot(), supportsPrivileged: this.backend.supportsPrivileged, ownContainerIds: this.ownContainerIds, + ownNetworkIds: this.ownNetworkIds, realpath: this.realpath, }); if (!verdict.allowed) { @@ -454,6 +478,10 @@ export class DockerFilterProxy { const addressed = containerIdFrom(parsed); if (addressed) this.disown(addressed); } + if (action === 'network-remove' && removedStatus >= 200 && removedStatus < 300) { + const addressed = networkIdFrom(parsed); + if (addressed) this.disownNetwork(addressed); + } const relayed = action === 'ping' ? this.relayPing(upstreamRes, res) @@ -461,7 +489,9 @@ export class DockerFilterProxy { ? this.relayVersion(upstreamRes, res) : action === 'create' ? this.relayCreate(upstreamRes, res, parsed) - : this.relay(upstreamRes, res); + : action === 'network-create' + ? this.relayNetworkCreate(upstreamRes, res, parsed) + : this.relay(upstreamRes, res); relayed.then(() => { answered = true; if (bufferedBody !== null) { @@ -539,6 +569,37 @@ export class DockerFilterProxy { * to containers this job actually created. The body is small and the client * needs the id before it can proceed, so buffering it costs nothing. */ + /** Relay a network create and record the network, by id and by requested name. */ + private relayNetworkCreate(upstreamRes: http.IncomingMessage, res: http.ServerResponse, requested: DockerRequest): Promise { + return new Promise((resolve) => { + const chunks: Buffer[] = []; + upstreamRes.on('data', (c: Buffer) => chunks.push(c)); + upstreamRes.on('end', () => { + const raw = Buffer.concat(chunks); + const status = upstreamRes.statusCode ?? 502; + if (status >= 200 && status < 300) { + try { + const parsed = JSON.parse(raw.toString('utf8')) as Record; + if (typeof parsed.Id === 'string' && parsed.Id.length > 0) { + this.ownNetwork(parsed.Id, parsed.Id); + const body = requested.body; + const name = isPlainRecord(body) ? body.Name : undefined; + if (typeof name === 'string' && name.length > 0) this.ownNetwork(name, parsed.Id); + } + } catch { + // An unreadable create response leaves the network unowned, which + // fails closed: the job cannot address what it cannot name. + } + } + const headers = { ...this.relayedHeaders(upstreamRes), 'content-length': String(raw.length) }; + delete headers['transfer-encoding']; + res.writeHead(status, headers); + if (raw.length > 0) res.write(raw); + resolve(); + }); + }); + } + private relayCreate(upstreamRes: http.IncomingMessage, res: http.ServerResponse, requested: DockerRequest): Promise { return new Promise((resolve) => { const chunks: Buffer[] = []; diff --git a/src/main/docker/docker-request.test.ts b/src/main/docker/docker-request.test.ts index c0547cd..b595473 100644 --- a/src/main/docker/docker-request.test.ts +++ b/src/main/docker/docker-request.test.ts @@ -87,7 +87,10 @@ describe('classifyDockerRequest', () => { it('classifies everything outside the map as other', () => { const others: Array<[string, string]> = [ - ['POST', '/v1.45/networks/create'], + // /networks/create is a mapped action now; these are not. + ['PUT', '/v1.45/networks/create'], + ['POST', '/v1.45/networks/net123'], + ['POST', '/v1.45/volumes/create'], ['POST', '/v1.45/containers/abc123/exec'], ['GET', '/v1.45/build'], ['GET', '/v1.45/containers/create'], diff --git a/src/main/docker/docker-request.ts b/src/main/docker/docker-request.ts index 35565c3..9858a9a 100644 --- a/src/main/docker/docker-request.ts +++ b/src/main/docker/docker-request.ts @@ -28,6 +28,10 @@ export type DockerAction = | 'kill' | 'stop' | 'logs' + | 'network-create' + | 'network-inspect' + | 'network-remove' + | 'network-list' | 'build' | 'other'; @@ -141,9 +145,26 @@ const ENDPOINTS: ReadonlyArray<{ method: string; path: RegExp; action: DockerAct // A read about the job's own container, like inspect. { method: 'GET', path: new RegExp(`^/containers/${ID}/logs$`), action: 'logs' }, { method: 'DELETE', path: new RegExp(`^/containers/${ID}$`), action: 'remove' }, + { method: 'POST', path: /^\/networks\/create$/, action: 'network-create' }, + { method: 'GET', path: new RegExp(`^/networks/${ID}$`), action: 'network-inspect' }, + { method: 'DELETE', path: new RegExp(`^/networks/${ID}$`), action: 'network-remove' }, + // Listing enumerates the daemon, like the container list; no key grants it. + { method: 'GET', path: /^\/networks$/, action: 'network-list' }, { method: 'POST', path: /^\/build$/, action: 'build' }, ]; +/** Per-network endpoints, for scoping to networks this socket created. */ +const NETWORK_ID_PATHS: ReadonlyArray = [new RegExp(`^/networks/(${ID})$`)]; + +/** The network a request addresses, or undefined when it addresses none. */ +export function networkIdFrom(req: DockerRequest): string | undefined { + for (const pattern of NETWORK_ID_PATHS) { + const match = pattern.exec(req.path); + if (match) return match[1]; + } + return undefined; +} + /** Per-container endpoints, for scoping an action to the containers this socket created. */ const CONTAINER_ID_PATHS: ReadonlyArray = [ new RegExp(`^/containers/(${ID})/json$`), diff --git a/src/shared/docker-policy.test.ts b/src/shared/docker-policy.test.ts index a94f66b..1186335 100644 --- a/src/shared/docker-policy.test.ts +++ b/src/shared/docker-policy.test.ts @@ -363,3 +363,40 @@ describe('privileged at validation time', () => { expect(collect({ privileged: false })).toEqual([]); }); }); + +describe('run.networks grammar', () => { + const collect = (value: unknown, path = 'shared.docker') => { + const errs: string[] = []; + validateDockerPolicy(value, path, (m) => errs.push(m)); + return errs; + }; + + it('accepts a declared network by name glob and internal flag', () => { + expect(collect({ run: { networks: [{ name: 'vk-*', internal: true }] } })).toEqual([]); + expect(collect({ run: { networks: [{ name: 'build', internal: false }] } })).toEqual([]); + }); + + it('requires internal to be stated, so a routable network is never the default', () => { + expect(collect({ run: { networks: [{ name: 'vk-*' }] } }).join('\n')).toMatch(/internal/i); + }); + + it('refuses a network key the grammar cannot spell, driver above all', () => { + for (const entry of [{ name: 'x', internal: true, driver: 'macvlan' }, { name: 'x', internal: true, options: {} }, { name: 'x', internal: true, ipam: {} }]) { + expect([Object.keys(entry).join(','), collect({ run: { networks: [entry] } }).length > 0]).toEqual([Object.keys(entry).join(','), true]); + } + }); + + it('composes shared and workflow networks additively', () => { + const merged = mergeDockerPolicy( + { run: { networks: [{ name: 'vk-*', internal: true }] } }, + { run: { networks: [{ name: 'build', internal: false }] } } + ); + expect(merged?.run?.networks).toEqual([{ name: 'vk-*', internal: true }, { name: 'build', internal: false }]); + }); + + it('shows a declared network in the approval diff', () => { + const diffs = diffDockerPolicy(undefined, { run: { networks: [{ name: 'vk-*', internal: true }] } }, 'shared.docker'); + expect(diffs.map((d) => d.path)).toContain('shared.docker.run.networks'); + expect(diffs[0].newValue).toMatch(/vk-\*/); + }); +}); diff --git a/src/shared/docker-policy.ts b/src/shared/docker-policy.ts index 7c53c83..09fd220 100644 --- a/src/shared/docker-policy.ts +++ b/src/shared/docker-policy.ts @@ -21,8 +21,21 @@ export interface DockerMount { } /** Container create, start, attach, wait and remove. */ +/** A network the job may create: a name glob, and whether it is internal. */ +export interface DockerNetworkPolicy { + /** Anchored glob; `*` matches any run of characters. */ + name: string; + /** + * Whether the network is cut off from anything outside it. Required rather + * than defaulted: a routable network is a real grant and has to be asked for + * in a way the approval diff shows. + */ + internal: boolean; +} + export interface DockerRunPolicy { images?: string[]; + networks?: DockerNetworkPolicy[]; mounts?: DockerMount[]; network?: string; } @@ -108,6 +121,7 @@ function validateRun(value: unknown, path: string, push: (message: string) => vo } if (value.images !== undefined) validateStringArray(value.images, `${path}.images`, push); if (value.mounts !== undefined) validateMounts(value.mounts, `${path}.mounts`, push); + if (value.networks !== undefined) validateNetworks(value.networks, `${path}.networks`, push); if (value.network !== undefined && typeof value.network !== 'string') { push(`${path}.network must be a string`); } else if (value.network === 'host' || (typeof value.network === 'string' && value.network.startsWith('container:'))) { @@ -117,6 +131,36 @@ function validateRun(value: unknown, path: string, push: (message: string) => vo } } +/** Keys a declared network may carry. Driver above all is absent by design. */ +const NETWORK_KEYS: readonly string[] = ['name', 'internal']; + +function validateNetworks(value: unknown, path: string, push: (message: string) => void): void { + if (!Array.isArray(value)) { + push(`${path} must be an array`); + return; + } + value.forEach((entry, i) => { + const at = `${path}[${i}]`; + if (!isPlainObject(entry)) { + push(`${at} must be an object with name and internal`); + return; + } + for (const key of Object.keys(entry)) { + if (!NETWORK_KEYS.includes(key)) { + // A macvlan or ipvlan network puts the container on the physical LAN, + // which is worse than host networking, and driver options can bind a + // bridge to a host address. None of it can be named, so none of it can + // be asked for; the filter always creates a plain internal bridge. + push(`${at}.${key} cannot be declared: a network may only name itself and say whether it is internal`); + } + } + if (typeof entry.name !== 'string' || entry.name === '') push(`${at}.name must be a non-empty string`); + if (typeof entry.internal !== 'boolean') { + push(`${at}.internal must be stated as true or false: a routable network is a grant of its own`); + } + }); +} + function validateMounts(value: unknown, path: string, push: (message: string) => void): void { if (!Array.isArray(value)) { push(`${path} must be an array`); @@ -200,11 +244,29 @@ function mergeRun(base?: DockerRunPolicy, override?: DockerRunPolicy): DockerRun if (images) run.images = images; const mounts = mergeMounts(base?.mounts, override?.mounts); if (mounts) run.mounts = mounts; + const networks = mergeNetworks(base?.networks, override?.networks); + if (networks) run.networks = networks; const network = override?.network ?? base?.network; if (network !== undefined) run.network = network; return run; } +function mergeNetworks( + base?: DockerNetworkPolicy[], + override?: DockerNetworkPolicy[] +): DockerNetworkPolicy[] | undefined { + if (!base && !override) return undefined; + const merged: DockerNetworkPolicy[] = []; + const seen = new Set(); + for (const entry of [...(base ?? []), ...(override ?? [])]) { + const key = `${entry.name}:${entry.internal}`; + if (seen.has(key)) continue; + seen.add(key); + merged.push(entry); + } + return merged.length > 0 ? merged : undefined; +} + function mergeBuild(base?: DockerBuildPolicy, override?: DockerBuildPolicy): DockerBuildPolicy | undefined { if (!base && !override) return undefined; const context = override?.context ?? base?.context; @@ -244,6 +306,7 @@ export interface DockerPolicyDiff { /** A mount as one string, in the shape a -v flag takes, so it diffs per grant. */ const mountKey = (m: DockerMount): string => `${m.path}:${m.mode}`; +const networkKey = (n: DockerNetworkPolicy): string => `${n.name}${n.internal ? ' (internal)' : ''}`; function diffLists(oldList: string[] | undefined, newList: string[] | undefined, path: string, diffs: DockerPolicyDiff[]): void { const oldSet = new Set(oldList ?? []); @@ -278,6 +341,7 @@ export function diffDockerPolicy( diffLists(oldP?.pull?.registries, newP?.pull?.registries, `${prefix}.pull.registries`, diffs); diffLists(oldP?.run?.images, newP?.run?.images, `${prefix}.run.images`, diffs); diffLists(oldP?.run?.mounts?.map(mountKey), newP?.run?.mounts?.map(mountKey), `${prefix}.run.mounts`, diffs); + diffLists(oldP?.run?.networks?.map(networkKey), newP?.run?.networks?.map(networkKey), `${prefix}.run.networks`, diffs); diffScalar(oldP?.run?.network, newP?.run?.network, `${prefix}.run.network`, diffs); diffScalar(oldP?.build?.context, newP?.build?.context, `${prefix}.build.context`, diffs); // false grants nothing, the same as absent. @@ -330,8 +394,8 @@ export function serializeDockerPolicy(policy: DockerPolicy, indent: string): str } if (policy.run) { - const { images, mounts, network } = policy.run; - if (!images?.length && !mounts?.length && network === undefined) { + const { images, mounts, network, networks } = policy.run; + if (!images?.length && !mounts?.length && !networks?.length && network === undefined) { lines.push(`${i1}run: {}`); } else { lines.push(`${i1}run:`); @@ -339,6 +403,13 @@ export function serializeDockerPolicy(policy: DockerPolicy, indent: string): str lines.push(`${i2}images:`); for (const image of images) lines.push(`${i3}- ${quote(image)}`); } + if (networks?.length) { + lines.push(`${i2}networks:`); + for (const n of networks) { + lines.push(`${i3}- name: ${quote(n.name)}`); + lines.push(`${i3} internal: ${n.internal}`); + } + } if (mounts?.length) { lines.push(`${i2}mounts:`); for (const mount of mounts) { From 659c769826f3bec5bcdba7a2ecce0042f9d9a030 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 15:48:41 -0400 Subject: [PATCH 18/31] Let a job inspect an image its policy already names `docker image inspect` is the natural "do I already have this?" check, and a build-once-mount flow uses it to decide between building and pulling. It classified as `other` and was denied. Scoped by the policy rather than by a second ownership ledger, which is where the consumer's suggestion and this differ: an inspect of an image run.images already grants discloses nothing the policy has not granted, and it avoids bookkeeping that would have to reconcile pulls by tag with builds by id. The container ledger has already produced one defect of exactly that kind. Listing and deleting images stay denied: both are daemon-wide, and the consumer agrees. That completes the three endpoint families the feedback asked for. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- docs/roadmap/localmostrc.md | 2 +- src/main/docker/docker-evaluator.test.ts | 28 ++++++++++++++++++++++++ src/main/docker/docker-evaluator.ts | 18 ++++++++++++++- src/main/docker/docker-request.ts | 15 +++++++++++++ 4 files changed, 61 insertions(+), 2 deletions(-) diff --git a/docs/roadmap/localmostrc.md b/docs/roadmap/localmostrc.md index 7045087..06862a9 100644 --- a/docs/roadmap/localmostrc.md +++ b/docs/roadmap/localmostrc.md @@ -290,7 +290,7 @@ Actions are CLI-shaped, so a policy reads the way a workflow author thinks: | Action | Covers | Conditions | |---|---|---| | `pull` | image pulls | `registries` — the registry each pulled image comes from | -| `run` | container create, start, attach, wait and remove | `images` — the image a container is created from; `mounts` — workspace paths a container may bind, each `ro` or `rw`; `network` — the container's network mode | +| `run` | container create, start, attach, wait, kill, stop, remove and logs; creating a declared network; inspecting a declared image | `images` — the image a container is created from, and the only images it may inspect; `networks` — networks the job may create, each an anchored name glob plus whether it is `internal`; `mounts` — workspace paths a container may bind, each `ro` or `rw`; `network` — the container's network mode | | `build` | image builds | `context` — which directory the workflow builds from, for the reader and the approval diff | Conditions are checked against the request itself. Mount paths are resolved diff --git a/src/main/docker/docker-evaluator.test.ts b/src/main/docker/docker-evaluator.test.ts index 1a0fb1a..13feccc 100644 --- a/src/main/docker/docker-evaluator.test.ts +++ b/src/main/docker/docker-evaluator.test.ts @@ -554,3 +554,31 @@ describe('networks', () => { expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', other), own).allowed).toBe(false); }); }); + +describe('image inspect', () => { + const p: DockerPolicy = { run: { images: ['alpine:3', 'ghcr.io/o/app:1'], network: 'bridge' }, pull: { registries: ['docker.io'] } }; + const inspect = (ref: string, c = ctx(p)) => + evaluateDockerRequest(mk('GET', `/v1.45/images/${encodeURIComponent(ref)}/json`), c); + + it('permits inspecting an image the policy already names', () => { + // Scoped by the policy rather than by a second ownership ledger: an + // inspect of an image run.images already grants discloses nothing new. + expect(inspect('alpine:3').allowed).toBe(true); + expect(inspect('docker.io/library/alpine:3').allowed).toBe(true); + expect(inspect('ghcr.io/o/app:1').allowed).toBe(true); + }); + + it('refuses an image the policy does not name', () => { + expect(inspect('postgres:16').allowed).toBe(false); + expect(inspect('ghcr.io/o/other:1').allowed).toBe(false); + }); + + it('refuses it when the policy declares no run action at all', () => { + expect(inspect('alpine:3', ctx({ pull: { registries: ['docker.io'] } })).allowed).toBe(false); + }); + + it('never lists or deletes images, which are daemon-wide', () => { + expect(evaluateDockerRequest(mk('GET', '/v1.45/images/json'), ctx(p)).allowed).toBe(false); + expect(evaluateDockerRequest(mk('DELETE', '/v1.45/images/alpine:3'), ctx(p)).allowed).toBe(false); + }); +}); diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index 8c43c48..69728b7 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -14,7 +14,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { DockerPolicy, DockerMount, DockerNetworkPolicy, MountMode } from '../../shared/docker-policy'; -import { DockerRequest, DockerAction, classifyDockerRequest, containerIdFrom, networkIdFrom } from './docker-request'; +import { DockerRequest, DockerAction, classifyDockerRequest, containerIdFrom, imageRefFrom, networkIdFrom } from './docker-request'; export interface DockerEvalContext { /** The bound policy; null until the worker claims a job, which denies all. */ @@ -704,6 +704,22 @@ export function evaluateDockerRequest(req: DockerRequest, ctx: DockerEvalContext return evaluatePull(req, policy); case 'build': return evaluateBuild(req, policy); + case 'image-inspect': { + // Scoped by the policy, not by a second ownership ledger: an inspect of + // an image run.images already names discloses nothing the policy has not + // granted, and the container ledger has already produced one defect. + if (!policy.run) return deny('the repository docker policy declares no run action', hints.run); + const ref = imageRefFrom(req); + if (!ref) return deny(`${req.method} ${req.path} is not permitted through the localmost docker socket`); + const wanted = normalizeImage(ref); + if (!(policy.run.images ?? []).some((declared) => normalizeImage(declared) === wanted)) { + return deny( + `image "${ref}" is not declared in the repository docker policy (run.images)`, + hints.image(ref) + ); + } + return ALLOW; + } case 'network-create': return evaluateNetworkCreate(req, policy); case 'network-inspect': diff --git a/src/main/docker/docker-request.ts b/src/main/docker/docker-request.ts index 9858a9a..579aa48 100644 --- a/src/main/docker/docker-request.ts +++ b/src/main/docker/docker-request.ts @@ -32,6 +32,7 @@ export type DockerAction = | 'network-inspect' | 'network-remove' | 'network-list' + | 'image-inspect' | 'build' | 'other'; @@ -136,6 +137,9 @@ const ENDPOINTS: ReadonlyArray<{ method: string; path: RegExp; action: DockerAct // container on the daemon, including other jobs'. { method: 'GET', path: /^\/containers\/json$/, action: 'list' }, { method: 'POST', path: /^\/images\/create$/, action: 'pull' }, + // The reference may carry a registry, a path and a tag, so it is anything up + // to the trailing /json. Listing is deliberately absent: it is daemon-wide. + { method: 'GET', path: /^\/images\/(?!json$).+\/json$/, action: 'image-inspect' }, { method: 'POST', path: /^\/containers\/create$/, action: 'create' }, { method: 'POST', path: new RegExp(`^/containers/${ID}/start$`), action: 'start' }, { method: 'POST', path: new RegExp(`^/containers/${ID}/attach$`), action: 'attach' }, @@ -153,6 +157,17 @@ const ENDPOINTS: ReadonlyArray<{ method: string; path: RegExp; action: DockerAct { method: 'POST', path: /^\/build$/, action: 'build' }, ]; +/** The image reference an inspect addresses, decoded, or undefined. */ +export function imageRefFrom(req: DockerRequest): string | undefined { + const match = /^\/images\/(.+)\/json$/.exec(req.path); + if (!match) return undefined; + try { + return decodeURIComponent(match[1]); + } catch { + return match[1]; + } +} + /** Per-network endpoints, for scoping to networks this socket created. */ const NETWORK_ID_PATHS: ReadonlyArray = [new RegExp(`^/networks/(${ID})$`)]; From 7ef9e41596e5147fd500a8798caae2bcd8da9fc4 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 16:02:25 -0400 Subject: [PATCH 19/31] Replace extract-zip with the fork that fixes its path traversal Dependabot #167 (GHSA-jmr9-qjv8-65gv, high): extract-zip does not validate symlink paths when extracting. It reaches us only through @electron-forge/cli -> @electron/packager, so it is build tooling and never ships - `npm audit --omit=dev` reports nothing - but a zip that extracts outside its destination is worth removing from a build that signs an app. There is no patched extract-zip and there never will be: 2.0.1 is the last release and the advisory lists no fix. npm's own suggestion is to downgrade to @electron-forge/cli@6.4.2, which is older and breaking - worse than the problem. Upstream's answer is @electron-internal/extract-zip, a maintained fork that calls itself a drop-in replacement, which @electron/packager 20 uses in place of the original. Aliasing the dependency to that fork applies the same fix while staying inside the range forge declares. Overriding @electron/packager to ^20 instead was tried and rejected: forge 7.11.2 is the newest forge and still asks for ^18.3.5, and packaging fails with "TypeError: done is not a function" from forge's own api/package.js. The constraint is forge, not packager's stability. Verified by packaging the app, which is the code path that unzips. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- package-lock.json | 97 ++++++----------------------------------------- package.json | 3 +- 2 files changed, 13 insertions(+), 87 deletions(-) diff --git a/package-lock.json b/package-lock.json index 8503943..2560662 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1261,6 +1261,17 @@ "url": "https://github.com/electron/packager?sponsor=1" } }, + "node_modules/@electron/packager/node_modules/extract-zip": { + "name": "@electron-internal/extract-zip", + "version": "1.0.5", + "resolved": "https://registry.npmjs.org/@electron-internal/extract-zip/-/extract-zip-1.0.5.tgz", + "integrity": "sha512-+bqFCP98pLI0Tt0XQo1TmlXtwjWchISndDOxCkEcIuUgXWpBnLyRI+2DU+mesvnMMX6L1XDqYNA0lXNDHd/yiA==", + "dev": true, + "license": "BSD-2-Clause", + "engines": { + "node": ">=22.12.0" + } + }, "node_modules/@electron/packager/node_modules/fs-extra": { "version": "11.4.0", "resolved": "https://registry.npmjs.org/fs-extra/-/fs-extra-11.4.0.tgz", @@ -3725,17 +3736,6 @@ "dev": true, "license": "MIT" }, - "node_modules/@types/yauzl": { - "version": "2.10.3", - "resolved": "https://registry.npmjs.org/@types/yauzl/-/yauzl-2.10.3.tgz", - "integrity": "sha512-oJoftv0LSuaDZE3Le4DbKX+KS9G36NzOeSap90UIK0yMA/NhKJhqlSGtNDORNRaIbQfzjXDrQa0ytJ6mNRGz/Q==", - "dev": true, - "license": "MIT", - "optional": true, - "dependencies": { - "@types/node": "*" - } - }, "node_modules/@typescript-eslint/eslint-plugin": { "version": "8.50.0", "resolved": "https://registry.npmjs.org/@typescript-eslint/eslint-plugin/-/eslint-plugin-8.50.0.tgz", @@ -5136,16 +5136,6 @@ "ieee754": "^1.1.13" } }, - "node_modules/buffer-crc32": { - "version": "0.2.13", - "resolved": "https://registry.npmjs.org/buffer-crc32/-/buffer-crc32-0.2.13.tgz", - "integrity": "sha512-VO9Ht/+p3SN7SKWqcrgEzjGbRSJYTx+Q1pTQC0wrWqHx0vpJraQ6GtHx8tvcg1rlK1byhU5gccxgOgj7B0TDkQ==", - "dev": true, - "license": "MIT", - "engines": { - "node": "*" - } - }, "node_modules/buffer-from": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/buffer-from/-/buffer-from-1.1.2.tgz", @@ -7582,43 +7572,6 @@ "node": ">=4" } }, - "node_modules/extract-zip": { - "version": "2.0.1", - "resolved": "https://registry.npmjs.org/extract-zip/-/extract-zip-2.0.1.tgz", - "integrity": "sha512-GDhU9ntwuKyGXdZBUgTIe+vXnWj0fppUEtMDL0+idd5Sta8TGpHssn/eusA9mrPr9qNDym6SxAYZjNvCn/9RBg==", - "dev": true, - "license": "BSD-2-Clause", - "dependencies": { - "debug": "^4.1.1", - "get-stream": "^5.1.0", - "yauzl": "^2.10.0" - }, - "bin": { - "extract-zip": "cli.js" - }, - "engines": { - "node": ">= 10.17.0" - }, - "optionalDependencies": { - "@types/yauzl": "^2.9.1" - } - }, - "node_modules/extract-zip/node_modules/get-stream": { - "version": "5.2.0", - "resolved": "https://registry.npmjs.org/get-stream/-/get-stream-5.2.0.tgz", - "integrity": "sha512-nBF+F1rAZVCu/p7rjzgA+Yb4lfYXrpl7a6VmJrU8wF9I1CKvP/QwPNZHnOlwbTkY6dvtFIzFMSyQXbLoTQPRpA==", - "dev": true, - "license": "MIT", - "dependencies": { - "pump": "^3.0.0" - }, - "engines": { - "node": ">=8" - }, - "funding": { - "url": "https://github.com/sponsors/sindresorhus" - } - }, "node_modules/fast-deep-equal": { "version": "3.1.3", "resolved": "https://registry.npmjs.org/fast-deep-equal/-/fast-deep-equal-3.1.3.tgz", @@ -7705,16 +7658,6 @@ "bser": "2.1.1" } }, - "node_modules/fd-slicer": { - "version": "1.1.0", - "resolved": "https://registry.npmjs.org/fd-slicer/-/fd-slicer-1.1.0.tgz", - "integrity": "sha512-cE1qsB/VwyQozZ+q1dGxR8LBYNZeofhEdUNGSMbQD3Gw2lAzX9Zb3uIU6Ebc/Fmyjo9AWWfnn0AUCHqtevs/8g==", - "dev": true, - "license": "MIT", - "dependencies": { - "pend": "~1.2.0" - } - }, "node_modules/figures": { "version": "6.1.0", "resolved": "https://registry.npmjs.org/figures/-/figures-6.1.0.tgz", @@ -12844,13 +12787,6 @@ "url": "https://github.com/sponsors/jet2jet" } }, - "node_modules/pend": { - "version": "1.2.0", - "resolved": "https://registry.npmjs.org/pend/-/pend-1.2.0.tgz", - "integrity": "sha512-F3asv42UuXchdzt+xXqfW1OGlVBe+mxa2mqI0pg5yAHZPvFmY3Y6drSf/GQ1A86WgWEN9Kzh/WrgKa6iGcHXLg==", - "dev": true, - "license": "MIT" - }, "node_modules/picocolors": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/picocolors/-/picocolors-1.1.1.tgz", @@ -16617,17 +16553,6 @@ "node": ">=8" } }, - "node_modules/yauzl": { - "version": "2.10.0", - "resolved": "https://registry.npmjs.org/yauzl/-/yauzl-2.10.0.tgz", - "integrity": "sha512-p4a9I6X6nu6IhoGmBqAcbJy1mlC4j27vEPZX9F4L4/vZT3Lyq1VkFHw/V/PUcB9Buo+DG3iHkT0x3Qya58zc3g==", - "dev": true, - "license": "MIT", - "dependencies": { - "buffer-crc32": "~0.2.3", - "fd-slicer": "~1.1.0" - } - }, "node_modules/yocto-queue": { "version": "0.1.0", "resolved": "https://registry.npmjs.org/yocto-queue/-/yocto-queue-0.1.0.tgz", diff --git a/package.json b/package.json index 325b013..cfd7400 100644 --- a/package.json +++ b/package.json @@ -101,6 +101,7 @@ "tar": "$tar", "brace-expansion@1": "^1.1.18", "minimatch@3": "^3.1.5", - "tmp": "^0.2.6" + "tmp": "^0.2.6", + "extract-zip": "npm:@electron-internal/extract-zip@^1.0.5" } } From 9a067c31b6ec2b9c0a3fae43b53948ddf7e810b2 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 18:14:30 -0400 Subject: [PATCH 20/31] Let the real CLI create a network, and declare images by glob Two blockers from wiring a container-heavy consumer to the filter, and four review findings. `docker network create` was refused for every stock CLI invocation. The allowlist was built from the spec rather than from real traffic, and the CLI sends Scope, IPAM, Attachable, Ingress, ConfigOnly, ConfigFrom and Options unconditionally with inert defaults - so the feature was reachable only from a hand-written API client, and the spec's own end-to-end scenario would have failed the moment it went through the CLI. They are gated by value now, the way HostConfig already treats the keys a plain `docker run` always sends: the default passes, anything meaningful is refused. This is the third time an allowlist has been written from the documentation instead of from the wire; the captured body is now in the test. `run.images` matched exactly, which excludes a content-addressed tag - the image cannot be named when the policy is written, and re-approving on every rebuild is not a workflow. Entries are anchored globs now, reusing the matcher that already backs network names, so `vk/grader:*` covers create and inspect together while `evil/vk/grader:x` still does not match. Review findings: network create now reads every casing of Name, Driver and Internal, since the daemon decodes them case-insensitively and a second casing may be the one it honours; declared networks appear in the approval-grants summary; the approval diff says internal or routable explicitly rather than leaving routable as a bare name; and DockerRunPolicy's doc comment, which I had detached by inserting DockerNetworkPolicy above it, is reattached. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- docs/roadmap/localmostrc.md | 2 +- .../2026-09-06-docker-endpoint-families.md | 13 ++- src/main/docker/docker-evaluator.test.ts | 65 ++++++++++++- src/main/docker/docker-evaluator.ts | 95 +++++++++++++------ src/main/ipc-handlers/policy.test.ts | 10 ++ src/main/ipc-handlers/policy.ts | 9 +- src/shared/docker-policy.test.ts | 10 ++ src/shared/docker-policy.ts | 6 +- 8 files changed, 174 insertions(+), 36 deletions(-) diff --git a/docs/roadmap/localmostrc.md b/docs/roadmap/localmostrc.md index 06862a9..fe78d2d 100644 --- a/docs/roadmap/localmostrc.md +++ b/docs/roadmap/localmostrc.md @@ -290,7 +290,7 @@ Actions are CLI-shaped, so a policy reads the way a workflow author thinks: | Action | Covers | Conditions | |---|---|---| | `pull` | image pulls | `registries` — the registry each pulled image comes from | -| `run` | container create, start, attach, wait, kill, stop, remove and logs; creating a declared network; inspecting a declared image | `images` — the image a container is created from, and the only images it may inspect; `networks` — networks the job may create, each an anchored name glob plus whether it is `internal`; `mounts` — workspace paths a container may bind, each `ro` or `rw`; `network` — the container's network mode | +| `run` | container create, start, attach, wait, kill, stop, remove and logs; creating a declared network; inspecting a declared image | `images` — the images a container may be created from, and the only images it may inspect; each entry is an anchored glob, so a content-addressed tag can be declared as `vk/grader:*`; `networks` — networks the job may create, each an anchored name glob plus whether it is `internal`; `mounts` — workspace paths a container may bind, each `ro` or `rw`; `network` — the container's network mode | | `build` | image builds | `context` — which directory the workflow builds from, for the reader and the approval diff | Conditions are checked against the request itself. Mount paths are resolved diff --git a/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md index acfee7e..d44779a 100644 --- a/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md +++ b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md @@ -59,9 +59,16 @@ does not recognise**. That is the same allowlist-of-the-grammar principle the original spec applies to `HostConfig`, applied to a second body. Recognised keys on `POST /networks/create`: `Name`, `Internal`, `CheckDuplicate`, -`Labels`. `Driver` is permitted only when absent or exactly `bridge`. Everything -else — `IPAM`, `Options`, `Attachable`, `Ingress`, `ConfigOnly`, `ConfigFrom`, -`EnableIPv6`, `Scope` — is refused, naming the key. +`Labels`. `Driver` is permitted only when absent or exactly `bridge`. + +The rest — `Scope`, `IPAM`, `Options`, `Attachable`, `Ingress`, `ConfigOnly`, +`ConfigFrom`, `EnableIPv6` — are **gated by value rather than refused outright**, +the same way `HostConfig` treats the keys a plain `docker run` always sends. The +CLI sends all eight unconditionally with inert defaults, so refusing them made +the feature reachable only from a hand-written API client. The default passes; +anything meaningful (a subnet, a non-default IPAM driver, driver options, an +attachable or ingress or config-only network, a config source, a scope) is +refused, naming the key. `GET /networks/{id}` and `DELETE /networks/{id}` are scoped to networks this socket created, exactly as per-container endpoints are scoped to containers it diff --git a/src/main/docker/docker-evaluator.test.ts b/src/main/docker/docker-evaluator.test.ts index 13feccc..481b732 100644 --- a/src/main/docker/docker-evaluator.test.ts +++ b/src/main/docker/docker-evaluator.test.ts @@ -525,7 +525,7 @@ describe('networks', () => { }); it('refuses any create key the grammar cannot spell, driver above all', () => { - for (const extra of [{ Driver: 'macvlan' }, { Options: { parent: 'en0' } }, { IPAM: { Config: [] } }, { Attachable: true }, { Ingress: true }, { ConfigOnly: true }]) { + for (const extra of [{ Driver: 'macvlan' }, { Options: { parent: 'en0' } }, { IPAM: { Config: [{ Subnet: '10.0.0.0/8' }] } }, { Attachable: true }, { Ingress: true }, { ConfigOnly: true }]) { const body = { Name: 'vk-run1', Internal: true, ...extra }; expect([Object.keys(extra)[0], create(body).allowed]).toEqual([Object.keys(extra)[0], false]); } @@ -582,3 +582,66 @@ describe('image inspect', () => { expect(evaluateDockerRequest(mk('DELETE', '/v1.45/images/alpine:3'), ctx(p)).allowed).toBe(false); }); }); + +describe('network create as the real CLI sends it', () => { + const p: DockerPolicy = { run: { images: ['alpine:3'], network: 'bridge', networks: [{ name: 'vk-*', internal: true }] } }; + // Captured off the wire from docker CLI 29.3.1. Every one of these keys is + // sent unconditionally, with an inert default. + const cliBody = (over: Record = {}) => ({ + Name: 'vk-probe-net', Driver: 'bridge', Scope: '', + IPAM: { Driver: 'default', Options: {}, Config: [] }, + Internal: true, Attachable: false, Ingress: false, ConfigOnly: false, + ConfigFrom: null, Options: {}, Labels: {}, ...over, + }); + const create = (body: unknown, c = ctx(p)) => evaluateDockerRequest(mk('POST', '/v1.45/networks/create', body), c); + + it('permits what `docker network create --internal` actually sends', () => { + expect(create(cliBody()).allowed).toBe(true); + }); + + it('still refuses those same keys when they carry a meaningful value', () => { + for (const over of [ + { Scope: 'swarm' }, + { IPAM: { Driver: 'default', Options: {}, Config: [{ Subnet: '10.0.0.0/8' }] } }, + { IPAM: { Driver: 'macvlan', Options: {}, Config: [] } }, + { IPAM: { Driver: 'default', Options: { parent: 'en0' }, Config: [] } }, + { Attachable: true }, { Ingress: true }, { ConfigOnly: true }, + { ConfigFrom: { Network: 'other' } }, + { Options: { 'com.docker.network.bridge.host_binding_ipv4': '0.0.0.0' } }, + { EnableIPv6: true }, + ]) { + expect([Object.keys(over)[0], create(cliBody(over)).allowed]).toEqual([Object.keys(over)[0], false]); + } + }); + + it('is fail-closed when casings disagree, as Go would decode them', () => { + // Go matches struct fields case-insensitively, so a second casing with a + // different value may be the one the daemon honours. + expect(create({ ...cliBody(), internal: false }).allowed).toBe(false); + expect(create({ ...cliBody(), name: 'not-declared' }).allowed).toBe(false); + expect(create({ ...cliBody(), driver: 'macvlan' }).allowed).toBe(false); + }); +}); + +describe('image globs', () => { + // A content-addressed tag cannot be known when the policy is written. + const p: DockerPolicy = { run: { images: ['vk/grader:*', 'alpine:3'], network: 'bridge' } }; + + it('permits creating and inspecting an image matching a declared glob', () => { + const body = { Image: 'vk/grader:7f2-0123456789ab' }; + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', body), ctx(p)).allowed).toBe(true); + expect(evaluateDockerRequest(mk('GET', '/v1.45/images/vk%2Fgrader%3A7f2-0123456789ab/json'), ctx(p)).allowed).toBe(true); + }); + + it('anchors the glob, so a lookalike repository does not match', () => { + for (const image of ['evil/vk/grader:x', 'notvk/grader:x', 'vk/grader-evil:x']) { + expect([image, evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: image }), ctx(p)).allowed]) + .toEqual([image, false]); + } + }); + + it('leaves an exact declaration exact', () => { + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: 'alpine:3' }), ctx(p)).allowed).toBe(true); + expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: 'alpine:4' }), ctx(p)).allowed).toBe(false); + }); +}); diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index 69728b7..6523885 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -473,7 +473,7 @@ function evaluateCreate(req: DockerRequest, ctx: DockerEvalContext, policy: Dock for (const candidate of imageValues) { if (typeof candidate !== 'string' || candidate === '') return deny('container create requires an Image'); const wanted = normalizeImage(candidate); - if (!(policy.run.images ?? []).some((declared) => normalizeImage(declared) === wanted)) { + if (!(policy.run.images ?? []).some((declared) => globMatches(normalizeImage(declared), wanted))) { return deny( `image "${candidate}" is not declared in the repository docker policy (run.images)`, hints.image(candidate) @@ -548,9 +548,37 @@ const BUILD_PARAMS_KNOWN: ReadonlySet = new Set([ 'platform', 'version', 'buildid', 'session', ]); -/** Keys a network create may carry. Driver is permitted only as the default. */ +/** Keys a network create may carry freely: they name the network or are inert. */ const NETWORK_CREATE_KNOWN: ReadonlySet = new Set(['name', 'internal', 'checkduplicate', 'labels', 'driver']); +/** Is an IPAM block the default one the CLI always sends, granting nothing? */ +const isDefaultIpam = (v: unknown): boolean => { + if (isUnset(v)) return true; + if (!isPlainObject(v)) return false; + const driver = pick(v, 'Driver'); + if (!isUnset(driver) && driver !== '' && driver !== 'default') return false; + return isEmptyObject(pick(v, 'Options')) && isEmptyArray(pick(v, 'Config')); +}; + +/** + * Keys the docker CLI sends on every `network create` with an inert value. + * + * Refusing them outright made the feature reachable only from a hand-written + * API client - the CLI sends all of these unconditionally. So they are gated by + * value, exactly as HostConfig gates the keys a plain `docker run` always + * sends: the default passes, anything meaningful is refused. + */ +const NETWORK_CREATE_GATES: ReadonlyArray<{ key: string; permitted: (v: unknown) => boolean; why: string }> = [ + { key: 'Scope', permitted: isEmptyString, why: 'a scope reaches beyond this daemon' }, + { key: 'IPAM', permitted: isDefaultIpam, why: 'an IPAM driver or subnet places the network on a chosen address range' }, + { key: 'Options', permitted: isEmptyObject, why: 'driver options can bind a bridge to a host address' }, + { key: 'Attachable', permitted: (v) => isUnset(v) || v === false, why: 'an attachable network can be joined from outside this job' }, + { key: 'Ingress', permitted: (v) => isUnset(v) || v === false, why: 'an ingress network is swarm routing mesh' }, + { key: 'ConfigOnly', permitted: (v) => isUnset(v) || v === false, why: 'a config-only network is a template for others' }, + { key: 'ConfigFrom', permitted: (v) => isUnset(v) || isEmptyObject(v), why: 'it copies configuration from another network' }, + { key: 'EnableIPv6', permitted: (v) => isUnset(v) || v === false, why: 'IPv6 is not part of what the grammar can describe' }, +]; + /** An anchored glob: `*` matches any run of characters, and nothing else is special. */ function globMatches(pattern: string, value: string): boolean { const escaped = pattern.replace(/[.*+?^${}()|[\]\\]/g, (c) => (c === '*' ? '\u0000' : `\\${c}`)); @@ -568,36 +596,49 @@ function evaluateNetworkCreate(req: DockerRequest, policy: DockerPolicy): Docker const body = req.body; if (!isPlainObject(body)) return deny('network create requires a JSON body'); + const gatedKeys = new Set(NETWORK_CREATE_GATES.map((g) => g.key.toLowerCase())); for (const key of Object.keys(body)) { - if (!NETWORK_CREATE_KNOWN.has(key.toLowerCase())) { - // A macvlan or ipvlan network puts the container on the physical LAN, - // which is worse than host networking, and Options can bind a bridge to - // a host address. The filter creates a plain bridge or nothing. - return deny(`network create parameter "${key}" is not one the localmost docker socket understands`); + const name = key.toLowerCase(); + if (NETWORK_CREATE_KNOWN.has(name) || gatedKeys.has(name)) continue; + return deny(`network create parameter "${key}" is not one the localmost docker socket understands`); + } + for (const gate of NETWORK_CREATE_GATES) { + // Every casing must pass: the daemon decodes these case-insensitively. + if (!valuesFor(body, gate.key).every((v) => gate.permitted(v))) { + return deny(`network ${gate.key} is not permitted: ${gate.why}`); } } - const driver = pick(body, 'Driver'); - if (!isUnset(driver) && driver !== '' && driver !== 'bridge') { - return deny(`network driver "${String(driver)}" is not permitted; the localmost docker socket creates bridge networks only`); + // The filter creates a plain bridge or nothing. macvlan and ipvlan put a + // container on the physical LAN, which is worse than host networking. + for (const driver of valuesFor(body, 'Driver')) { + if (!isUnset(driver) && driver !== '' && driver !== 'bridge') { + return deny(`network driver "${String(driver)}" is not permitted; the localmost docker socket creates bridge networks only`); + } } - const name = pick(body, 'Name'); - if (typeof name !== 'string' || name === '') return deny('network create requires a Name'); - const internal = valuesFor(body, 'Internal').some((v) => v === true); - - const match = declared.find((n) => globMatches(n.name, name)); - if (!match) { - return deny( - `network "${name}" is not declared in the repository docker policy (run.networks)`, - hints.network_declaration(name, internal) - ); - } - if (match.internal && !internal) { - return deny( - `network "${name}" is declared internal, so it cannot be created routable`, - hints.network_declaration(match.name, false) - ); + const names = valuesFor(body, 'Name'); + if (names.length === 0) return deny('network create requires a Name'); + const internalValues = valuesFor(body, 'Internal'); + const internal = internalValues.length > 0 && internalValues.every((v) => v === true); + + // Every casing must name a declared network, since which one the daemon uses + // is not worth depending on. + for (const name of names) { + if (typeof name !== 'string' || name === '') return deny('network create requires a Name'); + const match = declared.find((n) => globMatches(n.name, name)); + if (!match) { + return deny( + `network "${name}" is not declared in the repository docker policy (run.networks)`, + hints.network_declaration(name, internal) + ); + } + if (match.internal && !internal) { + return deny( + `network "${name}" is declared internal, so it cannot be created routable`, + hints.network_declaration(match.name, false) + ); + } } return ALLOW; } @@ -712,7 +753,7 @@ export function evaluateDockerRequest(req: DockerRequest, ctx: DockerEvalContext const ref = imageRefFrom(req); if (!ref) return deny(`${req.method} ${req.path} is not permitted through the localmost docker socket`); const wanted = normalizeImage(ref); - if (!(policy.run.images ?? []).some((declared) => normalizeImage(declared) === wanted)) { + if (!(policy.run.images ?? []).some((declared) => globMatches(normalizeImage(declared), wanted))) { return deny( `image "${ref}" is not declared in the repository docker policy (run.images)`, hints.image(ref) diff --git a/src/main/ipc-handlers/policy.test.ts b/src/main/ipc-handlers/policy.test.ts index 386555a..a19c61a 100644 --- a/src/main/ipc-handlers/policy.test.ts +++ b/src/main/ipc-handlers/policy.test.ts @@ -40,3 +40,13 @@ describe('summarizeGrants', () => { expect(grants).toEqual(['network: example.com', 'write: ~/.npm']); }); }); + +describe('network grants on the approval screen', () => { + it('shows a declared network and whether it is routable', () => { + const grants = summarizeGrants({ + shared: { docker: { run: { networks: [{ name: 'vk-*', internal: true }, { name: 'build', internal: false }] } } }, + }); + expect(grants.join('\n')).toMatch(/docker network create: vk-\* \(internal\)/); + expect(grants.join('\n')).toMatch(/docker network create: build \(routable\)/); + }); +}); diff --git a/src/main/ipc-handlers/policy.ts b/src/main/ipc-handlers/policy.ts index 44bf39e..a860330 100644 --- a/src/main/ipc-handlers/policy.ts +++ b/src/main/ipc-handlers/policy.ts @@ -46,12 +46,17 @@ function describeDocker(docker: DockerPolicy | undefined, prefix: string): strin for (const registry of registries) grants.push(`${prefix}docker pull: ${registry}`); } if (docker.run) { - const { images = [], mounts = [], network } = docker.run; - if (images.length === 0 && mounts.length === 0 && network === undefined) { + const { images = [], mounts = [], network, networks = [] } = docker.run; + if (images.length === 0 && mounts.length === 0 && networks.length === 0 && network === undefined) { grants.push(`${prefix}docker run`); } for (const image of images) grants.push(`${prefix}docker run image: ${image}`); for (const mount of mounts) grants.push(`${prefix}docker mount: ${mount.path} (${mount.mode})`); + // Creating a network is a grant, and whether it is routable is the part an + // operator most needs to see. + for (const n of networks) { + grants.push(`${prefix}docker network create: ${n.name} (${n.internal ? 'internal' : 'routable'})`); + } if (network !== undefined) grants.push(`${prefix}docker network: ${network}`); } if (docker.build) { diff --git a/src/shared/docker-policy.test.ts b/src/shared/docker-policy.test.ts index 1186335..d37d731 100644 --- a/src/shared/docker-policy.test.ts +++ b/src/shared/docker-policy.test.ts @@ -400,3 +400,13 @@ describe('run.networks grammar', () => { expect(diffs[0].newValue).toMatch(/vk-\*/); }); }); + +describe('network entries in the approval diff', () => { + it('says internal or routable explicitly, since both are consent-relevant', () => { + const diffs = diffDockerPolicy(undefined, { + run: { networks: [{ name: 'vk-*', internal: true }, { name: 'open', internal: false }] }, + }, 'shared.docker'); + const values = diffs.filter((d) => d.path.endsWith('run.networks')).map((d) => d.newValue); + expect(values).toEqual(expect.arrayContaining(['vk-* (internal)', 'open (routable)'])); + }); +}); diff --git a/src/shared/docker-policy.ts b/src/shared/docker-policy.ts index 09fd220..2c4dd37 100644 --- a/src/shared/docker-policy.ts +++ b/src/shared/docker-policy.ts @@ -20,7 +20,6 @@ export interface DockerMount { mode: MountMode; } -/** Container create, start, attach, wait and remove. */ /** A network the job may create: a name glob, and whether it is internal. */ export interface DockerNetworkPolicy { /** Anchored glob; `*` matches any run of characters. */ @@ -33,6 +32,7 @@ export interface DockerNetworkPolicy { internal: boolean; } +/** Container create, start, attach, wait, kill, stop, remove and logs. */ export interface DockerRunPolicy { images?: string[]; networks?: DockerNetworkPolicy[]; @@ -306,7 +306,9 @@ export interface DockerPolicyDiff { /** A mount as one string, in the shape a -v flag takes, so it diffs per grant. */ const mountKey = (m: DockerMount): string => `${m.path}:${m.mode}`; -const networkKey = (n: DockerNetworkPolicy): string => `${n.name}${n.internal ? ' (internal)' : ''}`; +// Both states are named: a routable network is a real grant, and showing it as +// a bare name left the approval diff silent about the part that matters. +const networkKey = (n: DockerNetworkPolicy): string => `${n.name} (${n.internal ? 'internal' : 'routable'})`; function diffLists(oldList: string[] | undefined, newList: string[] | undefined, path: string, diffs: DockerPolicyDiff[]): void { const oldSet = new Set(oldList ?? []); From 2c180e68b5855fc0ad1a9044bb6667c7b0b86561 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 18:19:30 -0400 Subject: [PATCH 21/31] Drop the docker workflow's dead runner-selection job Removing docker-localmost's `if: needs.check.outputs.runner == 'self-hosted'` left nothing in this workflow referencing the check: docker-linux names ubuntu-latest outright and docker-localmost names self-hosted. The job still ran on every pull request, deciding nothing and adding a third identical "check" to the status list. Two remain, and they are not duplicates of each other: CI's picks the runner its build job actually uses, and Test Inline Check exists to exercise the inlined form of that logic rather than the reusable one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- .github/workflows/docker.yaml | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/.github/workflows/docker.yaml b/.github/workflows/docker.yaml index 675bfa5..efea49c 100644 --- a/.github/workflows/docker.yaml +++ b/.github/workflows/docker.yaml @@ -24,11 +24,9 @@ permissions: contents: read jobs: - check: - uses: ./.github/workflows/check.yaml - with: - fallback: ubuntu-latest - + # No runner-selection job here: both legs below name their runner outright, + # so calling the reusable check would add a third copy of it to every PR's + # status list and decide nothing. docker-linux: runs-on: ubuntu-latest steps: From 11c99534917250fbb9898bf1a45d8915d92962c8 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 18:21:52 -0400 Subject: [PATCH 22/31] Stop a declared glob at the path separator The spec left open whether `*` should match within a segment. A consumer measured the first implementation and answered it empirically, in the direction nobody was leaning: the glob anchored correctly, but `*` crossed `/`, so `vk/grader:*` matched `vk/grader:a/b`. Harmless for that consumer, and the security-relevant half - anchoring - was already right. Tightened anyway, because both halves fail the same way: a glob that quietly spans more than it appears to reads as narrower than it is. `*` stops at `/` now, so `vk/*` reaches one level under `vk` and each further segment has to be asked for. Tag globs are unaffected, a tag having no slash in it, so the content-addressed case that motivated globs still works. The open question in the addendum is closed with what was measured. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- docs/roadmap/localmostrc.md | 2 +- .../2026-09-06-docker-endpoint-families.md | 13 +++++++--- src/main/docker/docker-evaluator.test.ts | 25 +++++++++++++++++++ src/main/docker/docker-evaluator.ts | 14 +++++++++-- 4 files changed, 47 insertions(+), 7 deletions(-) diff --git a/docs/roadmap/localmostrc.md b/docs/roadmap/localmostrc.md index fe78d2d..3b4a2b1 100644 --- a/docs/roadmap/localmostrc.md +++ b/docs/roadmap/localmostrc.md @@ -290,7 +290,7 @@ Actions are CLI-shaped, so a policy reads the way a workflow author thinks: | Action | Covers | Conditions | |---|---|---| | `pull` | image pulls | `registries` — the registry each pulled image comes from | -| `run` | container create, start, attach, wait, kill, stop, remove and logs; creating a declared network; inspecting a declared image | `images` — the images a container may be created from, and the only images it may inspect; each entry is an anchored glob, so a content-addressed tag can be declared as `vk/grader:*`; `networks` — networks the job may create, each an anchored name glob plus whether it is `internal`; `mounts` — workspace paths a container may bind, each `ro` or `rw`; `network` — the container's network mode | +| `run` | container create, start, attach, wait, kill, stop, remove and logs; creating a declared network; inspecting a declared image | `images` — the images a container may be created from, and the only images it may inspect; each entry is an anchored glob where `*` stops at `/`, so a content-addressed tag can be declared as `vk/grader:*` while `vk/*` reaches one level under `vk` and no further; `networks` — networks the job may create, each an anchored name glob plus whether it is `internal`; `mounts` — workspace paths a container may bind, each `ro` or `rw`; `network` — the container's network mode | | `build` | image builds | `context` — which directory the workflow builds from, for the reader and the approval diff | Conditions are checked against the request itself. Mount paths are resolved diff --git a/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md index d44779a..6d58366 100644 --- a/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md +++ b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md @@ -145,10 +145,15 @@ Per family, and in the same executable-escape style as the original spec: ## Open questions -- Whether `name` globs should be anchored (`vk-*` matching `vk-abc` but not - `other-vk-abc`). Leaning yes — anchored, with `*` matching within a segment — - since an unanchored glob in a security grammar reads as more permissive than - it looks. +- ~~Whether `name` globs should be anchored.~~ **Decided: yes, anchored, and + `*` stops at `/`.** A consumer measured the first implementation and found + that while it anchored correctly, `*` crossed path separators — `vk/grader:*` + matched `vk/grader:a/b` — which answered the question empirically in the + direction nobody wanted. Both halves now hold, for the same reason: a glob + that quietly spans more than it appears to reads as narrower than it is. + `vk/*` reaches one level under `vk` and no further; each extra segment has to + be asked for. Tag globs are unaffected, since a tag cannot contain a slash, + so `vk/grader:*` still covers a content-addressed tag. - Whether an owned network should be deleted automatically when the job's worker exits, as the socket itself is. Leaning yes, for the same reason: nothing should outlive the job that created it. diff --git a/src/main/docker/docker-evaluator.test.ts b/src/main/docker/docker-evaluator.test.ts index 481b732..1995da2 100644 --- a/src/main/docker/docker-evaluator.test.ts +++ b/src/main/docker/docker-evaluator.test.ts @@ -645,3 +645,28 @@ describe('image globs', () => { expect(evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: 'alpine:4' }), ctx(p)).allowed).toBe(false); }); }); + +describe('what * spans in a declared glob', () => { + const withImages = (images: string[]) => ctx({ run: { images, network: 'bridge' } }); + const create = (image: string, images: string[]) => + evaluateDockerRequest(mk('POST', '/v1.45/containers/create', { Image: image }), withImages(images)).allowed; + + it('spans a tag but not a path separator', () => { + // The spec left this open. Decided here: `*` stops at `/`, so a declared + // repository cannot be widened into deeper paths by a reference that adds + // segments. A tag glob - the content-addressed case - is unaffected, + // because a tag cannot contain a slash. + expect(create('vk/grader:7f2-0123456789ab', ['vk/grader:*'])).toBe(true); + expect(create('vk/grader:a/b', ['vk/grader:*'])).toBe(false); + }); + + it('still anchors, so a lookalike repository never matches', () => { + expect(create('evil/vk/grader:x', ['vk/grader:*'])).toBe(false); + }); + + it('needs a segment of its own to span one', () => { + // `vk/*:*` reaches one level under vk, and no further. + expect(create('vk/app:1', ['vk/*:*'])).toBe(true); + expect(create('vk/team/app:1', ['vk/*:*'])).toBe(false); + }); +}); diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index 6523885..035fa7d 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -579,10 +579,20 @@ const NETWORK_CREATE_GATES: ReadonlyArray<{ key: string; permitted: (v: unknown) { key: 'EnableIPv6', permitted: (v) => isUnset(v) || v === false, why: 'IPv6 is not part of what the grammar can describe' }, ]; -/** An anchored glob: `*` matches any run of characters, and nothing else is special. */ +/** + * An anchored glob. `*` matches any run of characters except `/`, and nothing + * else is special. + * + * Anchored so a declared name cannot be widened by a prefix: `vk-*` does not + * match `other-vk-abc`. Stopping at `/` for the same reason one level down - a + * glob that silently spans path separators reads as narrower than it is, so + * `vk/*` reaches one level under `vk` and no further, and each extra segment + * has to be asked for. A tag glob is unaffected, since a tag cannot contain a + * slash: `vk/grader:*` still covers a content-addressed tag. + */ function globMatches(pattern: string, value: string): boolean { const escaped = pattern.replace(/[.*+?^${}()|[\]\\]/g, (c) => (c === '*' ? '\u0000' : `\\${c}`)); - return new RegExp(`^${escaped.split('\u0000').join('.*')}$`).test(value); + return new RegExp(`^${escaped.split('\u0000').join('[^/]*')}$`).test(value); } function evaluateNetworkCreate(req: DockerRequest, policy: DockerPolicy): DockerVerdict { From fd2050430b72133ea39cd5609ed679bceffb7cc0 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 18:23:05 -0400 Subject: [PATCH 23/31] Revert "Drop the docker workflow's dead runner-selection job" This reverts commit 2c180e68b5855fc0ad1a9044bb6667c7b0b86561. --- .github/workflows/docker.yaml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/workflows/docker.yaml b/.github/workflows/docker.yaml index efea49c..675bfa5 100644 --- a/.github/workflows/docker.yaml +++ b/.github/workflows/docker.yaml @@ -24,9 +24,11 @@ permissions: contents: read jobs: - # No runner-selection job here: both legs below name their runner outright, - # so calling the reusable check would add a third copy of it to every PR's - # status list and decide nothing. + check: + uses: ./.github/workflows/check.yaml + with: + fallback: ubuntu-latest + docker-linux: runs-on: ubuntu-latest steps: From 6dbbbbbeaccfc74ed074c04ee2ea1b5103cc48fc Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 18:41:22 -0400 Subject: [PATCH 24/31] Pin jobs to the builder the filter can actually see `build:` policy describes POST /build, and a real `docker build` never calls it. BuildKit has been the default since Docker 23: it negotiates a session and streams over POST /grpc. A consumer replayed 1,429 captured requests from a suite that built about twenty images - zero POST /build, 63 POST /grpc, with DOCKER_BUILDKIT unset. So `build:` was grammar that validated, diffed and tested green against synthetic requests while being unreachable from the CLI. Jobs are pinned to the classic builder with DOCKER_BUILDKIT=0, set beside DOCKER_HOST when the worker spawns. Filtering the BuildKit session instead is not filterable in the sense this design means: it is a bidirectional gRPC stream over which the client exports host filesystem access to the daemon, so "which paths may this build read" stops being a property of any request the proxy can inspect. Choosing the builder the filter can see keeps the boundary where it can be enforced, at the cost of BuildKit's cache and speed - and with a shelf life, since the classic builder is deprecated and stage 2's VM would contain a build by construction. /grpc and /session are refused by name, saying jobs are pinned to the classic builder, so hitting that denial reads as "something turned BuildKit back on" rather than "unknown endpoint". Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- docs/roadmap/localmostrc.md | 2 +- .../2026-09-06-docker-endpoint-families.md | 26 +++++++++++++++++++ src/main/docker/docker-evaluator.test.ts | 21 +++++++++++++++ src/main/docker/docker-evaluator.ts | 7 +++++ src/main/docker/docker-request.ts | 5 ++++ src/main/runner-manager.test.ts | 4 +++ src/main/runner-manager.ts | 7 +++++ 7 files changed, 71 insertions(+), 1 deletion(-) diff --git a/docs/roadmap/localmostrc.md b/docs/roadmap/localmostrc.md index 3b4a2b1..b8a99c9 100644 --- a/docs/roadmap/localmostrc.md +++ b/docs/roadmap/localmostrc.md @@ -291,7 +291,7 @@ Actions are CLI-shaped, so a policy reads the way a workflow author thinks: |---|---|---| | `pull` | image pulls | `registries` — the registry each pulled image comes from | | `run` | container create, start, attach, wait, kill, stop, remove and logs; creating a declared network; inspecting a declared image | `images` — the images a container may be created from, and the only images it may inspect; each entry is an anchored glob where `*` stops at `/`, so a content-addressed tag can be declared as `vk/grader:*` while `vk/*` reaches one level under `vk` and no further; `networks` — networks the job may create, each an anchored name glob plus whether it is `internal`; `mounts` — workspace paths a container may bind, each `ro` or `rw`; `network` — the container's network mode | -| `build` | image builds | `context` — which directory the workflow builds from, for the reader and the approval diff | +| `build` | image builds, with the classic builder (jobs run with `DOCKER_BUILDKIT=0`, since a BuildKit build streams over a gRPC session the filter cannot inspect) | `context` — which directory the workflow builds from, for the reader and the approval diff | Conditions are checked against the request itself. Mount paths are resolved through symlinks and must stay inside the job workspace, so `../` traversal and diff --git a/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md index 6d58366..8478516 100644 --- a/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md +++ b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md @@ -111,6 +111,32 @@ same endpoint family with the same scoping. "reads about the job's own containers", and logs is exactly such a read; refusing it contradicts the documented behaviour rather than implementing it. +## Builds use the classic builder + +`build:` policy describes `POST /build`, and a real `docker build` on a default +install never calls it. BuildKit has been the default builder since Docker 23: +it negotiates a session and streams the build over `POST /grpc`. A consumer +replayed 1,429 captured API requests from a suite that built about twenty +images and found **zero** `POST /build` and 63 `POST /grpc`, with +`DOCKER_BUILDKIT` unset — stock behaviour, not an opt-in. + +So the filter pins each job to the classic builder with `DOCKER_BUILDKIT=0`, +set alongside `DOCKER_HOST` when the worker is spawned. + +The alternative was to filter the BuildKit session, and it is not filterable in +the sense this design means. The session is a bidirectional gRPC stream over +which the client exports host filesystem access to the daemon; "which paths may +this build read" stops being a property of a request body, which is the only +thing the proxy can inspect. Choosing the builder the filter can actually see +keeps the boundary honest, at the cost of BuildKit's cache and speed. The +classic builder is deprecated, so this is a stage-1 answer with a shelf life: +stage 2's managed VM contains a build by construction and would not need it. + +`POST /grpc` and `POST /session` are refused by name, saying that jobs are +pinned to the classic builder — seeing that denial means something set +`DOCKER_BUILDKIT` back on, which is worth reading as an error rather than as an +unknown endpoint. + ## What stays denied `GET /containers/json`, `GET /networks`, `GET /images/json` and diff --git a/src/main/docker/docker-evaluator.test.ts b/src/main/docker/docker-evaluator.test.ts index 1995da2..24b4296 100644 --- a/src/main/docker/docker-evaluator.test.ts +++ b/src/main/docker/docker-evaluator.test.ts @@ -670,3 +670,24 @@ describe('what * spans in a declared glob', () => { expect(create('vk/team/app:1', ['vk/*:*'])).toBe(false); }); }); + +describe('BuildKit endpoints', () => { + const p: DockerPolicy = { run: { images: ['alpine:3'], network: 'bridge' }, build: { context: './' } }; + + it('refuses a BuildKit session, and says why rather than shrugging', () => { + // A real `docker build` on a default install issues zero POST /build: it + // negotiates a session and streams over /grpc. Denying it generically read + // as "unknown endpoint" when the real answer is "that builder cannot be + // filtered, and we pinned you off it". + for (const url of ['/v1.45/grpc', '/v1.45/session']) { + const v = evaluateDockerRequest(mk('POST', url), ctx(p)); + expect([url, v.allowed]).toEqual([url, false]); + expect(v.reason).toMatch(/BuildKit/i); + expect(v.reason).toMatch(/DOCKER_BUILDKIT/); + } + }); + + it('still permits the classic build the policy describes', () => { + expect(evaluateDockerRequest(mk('POST', '/v1.45/build?t=app%3A1'), ctx(p)).allowed).toBe(true); + }); +}); diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index 035fa7d..8bd1bbd 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -776,6 +776,13 @@ export function evaluateDockerRequest(req: DockerRequest, ctx: DockerEvalContext case 'network-inspect': case 'network-remove': return evaluateOwnNetwork(req, ctx); + case 'buildkit': + return deny( + 'BuildKit builds cannot be filtered: the build streams over a gRPC session that exports host ' + + 'filesystem access to the daemon, so no request carries the paths it reads. Jobs are pinned to ' + + 'the classic builder with DOCKER_BUILDKIT=0, which `build:` policy does describe - seeing this ' + + 'means something set DOCKER_BUILDKIT back on.' + ); case 'network-list': return deny( 'listing networks is not permitted through the localmost docker socket; it would enumerate networks outside this job' diff --git a/src/main/docker/docker-request.ts b/src/main/docker/docker-request.ts index 579aa48..d9837d3 100644 --- a/src/main/docker/docker-request.ts +++ b/src/main/docker/docker-request.ts @@ -33,6 +33,7 @@ export type DockerAction = | 'network-remove' | 'network-list' | 'image-inspect' + | 'buildkit' | 'build' | 'other'; @@ -155,6 +156,10 @@ const ENDPOINTS: ReadonlyArray<{ method: string; path: RegExp; action: DockerAct // Listing enumerates the daemon, like the container list; no key grants it. { method: 'GET', path: /^\/networks$/, action: 'network-list' }, { method: 'POST', path: /^\/build$/, action: 'build' }, + // BuildKit's session and stream. Named so the refusal can say why, rather + // than falling through to "unknown endpoint". + { method: 'POST', path: /^\/grpc$/, action: 'buildkit' }, + { method: 'POST', path: /^\/session$/, action: 'buildkit' }, ]; /** The image reference an inspect addresses, decoded, or undefined. */ diff --git a/src/main/runner-manager.test.ts b/src/main/runner-manager.test.ts index dda28b7..4c20db6 100644 --- a/src/main/runner-manager.test.ts +++ b/src/main/runner-manager.test.ts @@ -1328,6 +1328,10 @@ describe('RunnerManager', () => { expect(socket.start.mock.invocationCallOrder[0]).toBeLessThan(mockSpawnSandboxed.mock.invocationCallOrder[0]); const options = mockSpawnSandboxed.mock.calls[0][2]!; expect(options.env?.DOCKER_HOST).toBe(`unix://${socketPath}`); + // BuildKit, the default builder since Docker 23, streams a build over a + // gRPC session the filter cannot inspect. The classic builder is the one + // `build:` policy actually describes, so the job is pinned to it. + expect(options.env?.DOCKER_BUILDKIT).toBe('0'); // The profile grants this socket by name; the daemon's is no longer handed over. expect(options).toHaveProperty('dockerSocket', socketPath); expect(options).not.toHaveProperty('dockerGrants'); diff --git a/src/main/runner-manager.ts b/src/main/runner-manager.ts index 54f7a9c..80fe8ce 100644 --- a/src/main/runner-manager.ts +++ b/src/main/runner-manager.ts @@ -993,6 +993,13 @@ export class RunnerManager { const dockerSocketPath = path.join(sandboxDir, DOCKER_SOCKET_NAME); const dockerSocket = await this.startDockerProxy(instanceNum, dockerSocketPath); env.DOCKER_HOST = `unix://${dockerSocketPath}`; + // Pin the job to the classic builder. BuildKit - the default since + // Docker 23 - does not use POST /build at all: it negotiates a session + // and streams the build over gRPC, exporting host filesystem access to + // the daemon as it goes. "Which paths may this build read" then stops + // being a property of any request the filter can see, so `build:` policy + // would describe an endpoint a real `docker build` never calls. + env.DOCKER_BUILDKIT = '0'; instance.process = spawnSandboxed(runnerBinary, ['--once'], { cwd: sandboxDir, From b6143d4c02ea097e5eb96fce08ffa3701bf2bb38 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 19:09:34 -0400 Subject: [PATCH 25/31] Refuse an image glob that does not say which tags it covers A consumer measured the matcher again and found a boundary nobody had written down: `vk/*` allowed `vk/setup-tools` but denied `vk/setup-tools:1`. The glob itself is innocent - `[^/]*` spans a colon happily. It is normalisation: a reference with no tag gets `:latest` on both sides, so `vk/*` is matched as `vk/*:latest` and covers the latest tag of each repository and nothing else. Someone writing it to mean "any image in our namespace" gets a policy covering almost none of them, and an approval diff shows the wide-looking form, not the narrow meaning. Same silently-doesn't-apply class as the `build:` key that used to be ignored. Two ways out: read a tagless glob as `:*`, or refuse it. Refusing wins for the reason `docker: true` is refused rather than interpreted - the grammar does not guess at intent it can ask for. Validation now rejects a tagless glob with the form that means what it looks like in the message. Exact references are untouched: `alpine` still means `alpine:latest`, which is what it looks like. The evaluator keeps a test pinning what a tagless glob would match, so the reason the rejection exists stays visible, and the three places that claimed "`vk/*` reaches one level under `vk`" now say `vk/*:*` and name the tag boundary. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- docs/roadmap/localmostrc.md | 2 +- .../2026-09-06-docker-endpoint-families.md | 16 +++++++++-- src/main/docker/docker-evaluator.test.ts | 10 +++++++ src/main/docker/docker-evaluator.ts | 8 +++++- src/shared/docker-policy.test.ts | 28 +++++++++++++++++++ src/shared/docker-policy.ts | 21 +++++++++++++- 6 files changed, 79 insertions(+), 6 deletions(-) diff --git a/docs/roadmap/localmostrc.md b/docs/roadmap/localmostrc.md index b8a99c9..bd4c2de 100644 --- a/docs/roadmap/localmostrc.md +++ b/docs/roadmap/localmostrc.md @@ -290,7 +290,7 @@ Actions are CLI-shaped, so a policy reads the way a workflow author thinks: | Action | Covers | Conditions | |---|---|---| | `pull` | image pulls | `registries` — the registry each pulled image comes from | -| `run` | container create, start, attach, wait, kill, stop, remove and logs; creating a declared network; inspecting a declared image | `images` — the images a container may be created from, and the only images it may inspect; each entry is an anchored glob where `*` stops at `/`, so a content-addressed tag can be declared as `vk/grader:*` while `vk/*` reaches one level under `vk` and no further; `networks` — networks the job may create, each an anchored name glob plus whether it is `internal`; `mounts` — workspace paths a container may bind, each `ro` or `rw`; `network` — the container's network mode | +| `run` | container create, start, attach, wait, kill, stop, remove and logs; creating a declared network; inspecting a declared image | `images` — the images a container may be created from, and the only images it may inspect; each entry is an anchored glob where `*` stops at `/`, so a content-addressed tag can be declared as `vk/grader:*` while `vk/*:*` reaches one level under `vk` and no further; a glob must say which tags it covers, since a tagless reference means `:latest` — `vk/*` is refused, `vk/*:*` accepted; `networks` — networks the job may create, each an anchored name glob plus whether it is `internal`; `mounts` — workspace paths a container may bind, each `ro` or `rw`; `network` — the container's network mode | | `build` | image builds, with the classic builder (jobs run with `DOCKER_BUILDKIT=0`, since a BuildKit build streams over a gRPC session the filter cannot inspect) | `context` — which directory the workflow builds from, for the reader and the approval diff | Conditions are checked against the request itself. Mount paths are resolved diff --git a/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md index 8478516..c912ee5 100644 --- a/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md +++ b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md @@ -177,9 +177,19 @@ Per family, and in the same executable-escape style as the original spec: matched `vk/grader:a/b` — which answered the question empirically in the direction nobody wanted. Both halves now hold, for the same reason: a glob that quietly spans more than it appears to reads as narrower than it is. - `vk/*` reaches one level under `vk` and no further; each extra segment has to - be asked for. Tag globs are unaffected, since a tag cannot contain a slash, - so `vk/grader:*` still covers a content-addressed tag. + `vk/*:*` reaches one level under `vk` and no further; each extra segment has + to be asked for. Tag globs are unaffected, since a tag cannot contain a + slash, so `vk/grader:*` still covers a content-addressed tag. +- ~~What a glob with no tag covers.~~ **Decided: nothing - it is refused at + validation.** The same consumer measured again and found a second boundary + nobody had written down: a reference with no tag normalises to `:latest`, so + `vk/*` is matched as `vk/*:latest` and covers only the latest tag of each + repository - almost none of what it reads as, and invisible in an approval + diff. Two ways out: treat a tagless glob as `:*`, or refuse it. Refusing it + wins for the reason `docker: true` is refused rather than interpreted - the + grammar does not guess at intent it can ask for - so validation rejects a + tagless glob with a message naming `vk/*:*`. Exact references are untouched: + `alpine` still means `alpine:latest`, which is what it looks like. - Whether an owned network should be deleted automatically when the job's worker exits, as the socket itself is. Leaning yes, for the same reason: nothing should outlive the job that created it. diff --git a/src/main/docker/docker-evaluator.test.ts b/src/main/docker/docker-evaluator.test.ts index 24b4296..9f0d271 100644 --- a/src/main/docker/docker-evaluator.test.ts +++ b/src/main/docker/docker-evaluator.test.ts @@ -669,6 +669,16 @@ describe('what * spans in a declared glob', () => { expect(create('vk/app:1', ['vk/*:*'])).toBe(true); expect(create('vk/team/app:1', ['vk/*:*'])).toBe(false); }); + + it('is bounded by the tag too, which is why a tagless glob is refused upstream', () => { + // Normalisation appends :latest to a tagless reference on both sides, so a + // tagless glob is matched as `vk/*:latest` - it covers latest and nothing + // else, however wide it reads. validateDockerPolicy rejects the form for + // that reason; this pins the behaviour the rejection exists to prevent. + expect(create('vk/app', ['vk/*'])).toBe(true); + expect(create('vk/app:1', ['vk/*'])).toBe(false); + expect(create('vk/app:1', ['vk/*:*'])).toBe(true); + }); }); describe('BuildKit endpoints', () => { diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index 8bd1bbd..822365d 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -586,9 +586,15 @@ const NETWORK_CREATE_GATES: ReadonlyArray<{ key: string; permitted: (v: unknown) * Anchored so a declared name cannot be widened by a prefix: `vk-*` does not * match `other-vk-abc`. Stopping at `/` for the same reason one level down - a * glob that silently spans path separators reads as narrower than it is, so - * `vk/*` reaches one level under `vk` and no further, and each extra segment + * `vk/*:*` reaches one level under `vk` and no further, and each extra segment * has to be asked for. A tag glob is unaffected, since a tag cannot contain a * slash: `vk/grader:*` still covers a content-addressed tag. + * + * The tag is a second boundary, and it comes from normalisation rather than + * from here: a declaration with no tag normalises to `:latest`, so `vk/*` is + * matched as `vk/*:latest` and covers only latest. That reads as far wider + * than it is, so validation refuses a tagless glob and names `vk/*:*`; the + * behaviour is pinned by test rather than relied upon. */ function globMatches(pattern: string, value: string): boolean { const escaped = pattern.replace(/[.*+?^${}()|[\]\\]/g, (c) => (c === '*' ? '\u0000' : `\\${c}`)); diff --git a/src/shared/docker-policy.test.ts b/src/shared/docker-policy.test.ts index d37d731..587291e 100644 --- a/src/shared/docker-policy.test.ts +++ b/src/shared/docker-policy.test.ts @@ -410,3 +410,31 @@ describe('network entries in the approval diff', () => { expect(values).toEqual(expect.arrayContaining(['vk-* (internal)', 'open (routable)'])); }); }); + +describe('a glob in run.images must say what tag it covers', () => { + const collect = (value: unknown, path = 'shared.docker') => { + const errs: string[] = []; + validateDockerPolicy(value, path, (m) => errs.push(m)); + return errs; + }; + + it('rejects a tagless glob, naming the form that means what it looks like', () => { + // Normalisation appends :latest to a tagless reference, so `vk/*` silently + // means "any repo under vk, but only its latest tag" - almost none of them. + // Guessing :* instead would be the same class of guess as accepting + // `docker: true`, so it is refused with the fix in the message. + const errs = collect({ run: { images: ['vk/*'] } }).join('\n'); + expect(errs).toMatch(/vk\/\*:\*/); + expect(errs).toMatch(/tag/i); + }); + + it('accepts a glob that carries a tag, globbed or exact', () => { + expect(collect({ run: { images: ['vk/*:*'] } })).toEqual([]); + expect(collect({ run: { images: ['vk/grader:*'] } })).toEqual([]); + expect(collect({ run: { images: ['vk/*:1'] } })).toEqual([]); + }); + + it('leaves exact references alone, tagless or not', () => { + expect(collect({ run: { images: ['alpine', 'alpine:3', 'ghcr.io/o/app:1'] } })).toEqual([]); + }); +}); diff --git a/src/shared/docker-policy.ts b/src/shared/docker-policy.ts index 2c4dd37..00445f2 100644 --- a/src/shared/docker-policy.ts +++ b/src/shared/docker-policy.ts @@ -119,7 +119,26 @@ function validateRun(value: unknown, path: string, push: (message: string) => vo push(`${path} must be an object`); return; } - if (value.images !== undefined) validateStringArray(value.images, `${path}.images`, push); + if (value.images !== undefined) { + validateStringArray(value.images, `${path}.images`, push); + if (Array.isArray(value.images)) { + for (const image of value.images) { + if (typeof image !== 'string' || !image.includes('*')) continue; + // A reference with no tag normalises to :latest, so a tagless glob + // means "any repository here, but only its latest tag" - which is not + // what it looks like, and an approval diff cannot show the difference. + // Guessing :* instead would be the same guess this grammar refuses when + // it rejects `docker: true`, so say what to write instead. + const lastSegment = image.slice(image.lastIndexOf('/') + 1); + if (!lastSegment.includes(':') && !lastSegment.includes('@')) { + push( + `${path}.images entry "${image}" globs a repository but names no tag, which matches only ` + + `its "latest" tag. Write "${image}:*" for any tag, or name the tag you mean.` + ); + } + } + } + } if (value.mounts !== undefined) validateMounts(value.mounts, `${path}.mounts`, push); if (value.networks !== undefined) validateNetworks(value.networks, `${path}.networks`, push); if (value.network !== undefined && typeof value.network !== 'string') { From d9445f4dbb67cc497429bf3e21f0167bb0714e69 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 19:15:35 -0400 Subject: [PATCH 26/31] Refuse a body that spells one key two ways, rather than guess Review flagged `pick()` as a case-folding bypass: a body can carry both `HostConfig` and `hostconfig`, and the filter judges one while the daemon honours another. Real, and worse than the report - the suggested fix was to read the last duplicate, on the theory that Go's decoder is last-wins. Measured against a real daemon rather than reasoned about. A create body with `HostConfig`, `hostconfig` and `HOSTCONFIG` came back carrying fields from all three: AutoRemove from the first, Memory from the second, OomScoreAdj from the third. Go decodes each key into the same struct field in document order, so nested objects MERGE; scalars and arrays within one object are last-wins. Reading the last duplicate is therefore exactly as wrong as reading the first, and getting it right means reimplementing encoding/json inside the filter. So the ambiguity is refused instead of modelled. Go's encoder emits unique, exactly-cased keys, so no real client sends a case-variant duplicate - the docker CLI's own bodies are clean, which the e2e exercises end to end. A recursive check at the evaluator's entry denies any body containing two casings of one key, naming both spellings. One check, every action with a body, fail-closed. Second finding, also real: relayNetworkCreate recorded ownership from `body.Name` case-sensitively, so a client sending `name` created a network the evaluator had approved and the proxy then refused to let it join, inspect or delete. It now reads the key the way the daemon does, which the check above makes unambiguous by construction. Container aliases are unaffected - they come from the query string, and Go's url.Values is case-sensitive. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- .../2026-09-06-docker-endpoint-families.md | 13 ++++ src/main/docker/docker-evaluator.test.ts | 59 +++++++++++++++++++ src/main/docker/docker-evaluator.ts | 46 +++++++++++++++ src/main/docker/docker-filter-proxy.test.ts | 18 ++++++ src/main/docker/docker-filter-proxy.ts | 17 +++++- test-results/.last-run.json | 4 ++ 6 files changed, 156 insertions(+), 1 deletion(-) create mode 100644 test-results/.last-run.json diff --git a/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md index c912ee5..e7ba18e 100644 --- a/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md +++ b/docs/superpowers/specs/2026-09-06-docker-endpoint-families.md @@ -190,6 +190,19 @@ Per family, and in the same executable-escape style as the original spec: grammar does not guess at intent it can ask for - so validation rejects a tagless glob with a message naming `vk/*:*`. Exact references are untouched: `alpine` still means `alpine:latest`, which is what it looks like. +- ~~What the filter should do when a body spells one key two ways.~~ + **Decided: refuse the body.** Review raised this as a case-folding bypass and + proposed reading the last duplicate, on the theory that Go's decoder is + last-wins. Measured against a real daemon instead: a create body carrying + `HostConfig`, `hostconfig` and `HOSTCONFIG` came back with fields from **all + three** - Go decodes each key into the same struct field in document order, + so nested objects merge, while scalars and arrays inside one object are + last-wins. Reading the last is therefore as wrong as reading the first, and + emulating the merge means reimplementing `encoding/json`. Since Go's encoder + emits unique exactly-cased keys, no real client sends a case-variant + duplicate - the real CLI's bodies are clean, which the e2e exercises - so the + ambiguity is refused recursively at the evaluator's entry, once, for every + action with a body. - Whether an owned network should be deleted automatically when the job's worker exits, as the socket itself is. Leaning yes, for the same reason: nothing should outlive the job that created it. diff --git a/src/main/docker/docker-evaluator.test.ts b/src/main/docker/docker-evaluator.test.ts index 9f0d271..dad5da4 100644 --- a/src/main/docker/docker-evaluator.test.ts +++ b/src/main/docker/docker-evaluator.test.ts @@ -701,3 +701,62 @@ describe('BuildKit endpoints', () => { expect(evaluateDockerRequest(mk('POST', '/v1.45/build?t=app%3A1'), ctx(p)).allowed).toBe(true); }); }); + +describe('duplicate keys that differ only in case', () => { + const runPolicy: DockerPolicy = { run: { images: ['alpine:3'], network: 'bridge' } }; + + it('refuses a body carrying two casings of the same key, rather than guessing which one counts', () => { + // Measured against a real daemon: with `HostConfig`, `hostconfig` and + // `HOSTCONFIG` all present, Go's decoder MERGED all three into one struct + // (AutoRemove from the first, Memory from the second, OomScoreAdj from the + // third). Scalars and arrays inside one object are last-wins instead. + // No filter can read one of those objects and know what the daemon will + // do, and picking the last is as wrong as picking the first - the merge + // keeps fields from both. Go's encoder never emits case-variant duplicates, + // so a body containing them is not a client we model. + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { + Image: 'alpine:3', + HostConfig: { NetworkMode: 'bridge' }, + hostconfig: { Binds: ['/etc:/host-etc'] }, + }), + ctx(runPolicy) + ); + expect(v.allowed).toBe(false); + expect(v.reason).toMatch(/case/i); + expect(v.reason).toMatch(/HostConfig|hostconfig/); + }); + + it('finds them however deep they are nested', () => { + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { + Image: 'alpine:3', + HostConfig: { NetworkMode: 'bridge', Mounts: [{ Type: 'bind', Source: '/ws', type: 'tmpfs' }] }, + }), + ctx(runPolicy) + ); + expect(v.allowed).toBe(false); + expect(v.reason).toMatch(/case/i); + }); + + it('leaves an ordinary body alone, including keys that merely resemble each other', () => { + const v = evaluateDockerRequest( + mk('POST', '/v1.45/containers/create', { + Image: 'alpine:3', + HostConfig: { NetworkMode: 'bridge', Memory: 0, MemorySwap: 0, Binds: [] }, + }), + ctx(runPolicy) + ); + expect(v.allowed).toBe(true); + }); + + it('applies to every action with a body, not just create', () => { + const p: DockerPolicy = { run: { images: ['alpine:3'], network: 'bridge', networks: [{ name: 'vk-1', internal: true }] } }; + const v = evaluateDockerRequest( + mk('POST', '/v1.45/networks/create', { Name: 'vk-1', Internal: true, internal: false }), + ctx(p) + ); + expect(v.allowed).toBe(false); + expect(v.reason).toMatch(/case/i); + }); +}); diff --git a/src/main/docker/docker-evaluator.ts b/src/main/docker/docker-evaluator.ts index 822365d..87da22b 100644 --- a/src/main/docker/docker-evaluator.ts +++ b/src/main/docker/docker-evaluator.ts @@ -727,6 +727,47 @@ function evaluateOwnContainer(req: DockerRequest, ctx: DockerEvalContext): Docke // Entry point // ----------------------------------------------------------------------------- +/** + * The first key in `value` that has a case-variant twin, named with its path. + * + * Measured against a real daemon rather than reasoned about: a create body + * carrying `HostConfig`, `hostconfig` and `HOSTCONFIG` came back with fields + * from ALL THREE - Go decodes each key into the same struct field in document + * order, so nested objects merge, while scalars and arrays inside one object + * are last-wins. There is no single value a filter can read and be right + * about: reading the first misses what the later ones added, and reading the + * last misses what the first one set. + * + * So the ambiguity is refused instead of modelled. Go's encoder emits unique, + * exactly-cased keys, so no real client sends a case-variant duplicate; a body + * that does is either a client the filter does not model or an attempt to be + * judged on one value and served another. + */ +function caseAmbiguity(value: unknown, at = 'the request body'): string | undefined { + if (Array.isArray(value)) { + for (const [i, item] of value.entries()) { + const found = caseAmbiguity(item, `${at}[${i}]`); + if (found) return found; + } + return undefined; + } + if (!isPlainObject(value)) return undefined; + const seen = new Map(); + for (const key of Object.keys(value)) { + const folded = key.toLowerCase(); + const first = seen.get(folded); + if (first !== undefined) { + return `${at} names both "${first}" and "${key}", which the daemon reads as the same key: it decodes them case-insensitively and merges or overwrites, so the value it would use is not the value this filter can read. Send each key once.`; + } + seen.set(folded, key); + } + for (const [key, child] of Object.entries(value)) { + const found = caseAmbiguity(child, `${at}.${key}`); + if (found) return found; + } + return undefined; +} + export function evaluateDockerRequest(req: DockerRequest, ctx: DockerEvalContext): DockerVerdict { const action = classifyDockerRequest(req); if (BASELINE.has(action)) return ALLOW; @@ -737,6 +778,11 @@ export function evaluateDockerRequest(req: DockerRequest, ctx: DockerEvalContext // A body the parser could not read is a request the filter cannot judge. if (req.bodyError) return deny(req.bodyError); + // Nor can it judge a body whose keys the daemon would read differently than + // it does. Checked once, here, so every action with a body is covered. + const ambiguous = caseAmbiguity(req.body); + if (ambiguous) return deny(ambiguous); + switch (action) { case 'create': return evaluateCreate(req, ctx, policy); diff --git a/src/main/docker/docker-filter-proxy.test.ts b/src/main/docker/docker-filter-proxy.test.ts index da80eaf..8475556 100644 --- a/src/main/docker/docker-filter-proxy.test.ts +++ b/src/main/docker/docker-filter-proxy.test.ts @@ -787,6 +787,24 @@ describe('networks a job creates', () => { expect((await request(sock, 'GET', '/v1.45/networks/vk-1')).status).toBe(403); expect((await request(sock, 'GET', '/v1.45/networks/net123')).status).toBe(403); }); + + it('are recorded under the name whatever casing the client spelled the key with', async () => { + // The daemon decodes `name` into the same field as `Name`, so it creates + // the network either way, and the evaluator already judges either way. + // Reading only `Name` here left the network created but unaddressable: the + // job could not join, inspect or delete what it had just made. + const dir = tmp(); + const daemon = await networkDaemon(dir); + const { proxy, sock } = await startProxy(dir, { backend: backendWith(daemon.sock, dir) }); + proxy.bind('owner/repo', { + run: { images: ['alpine:3'], network: 'bridge', networks: [{ name: 'vk-*', internal: true }] }, + }); + + expect((await request(sock, 'POST', '/v1.45/networks/create', { name: 'vk-1', internal: true })).status).toBe(201); + + expect((await request(sock, 'GET', '/v1.45/networks/vk-1')).status).toBeLessThan(400); + expect((await request(sock, 'POST', '/v1.45/containers/create', { Image: 'alpine:3', HostConfig: { NetworkMode: 'vk-1' } })).status).toBe(201); + }); }); /** A fake daemon that also answers network create. */ diff --git a/src/main/docker/docker-filter-proxy.ts b/src/main/docker/docker-filter-proxy.ts index 5ac9443..6e81455 100644 --- a/src/main/docker/docker-filter-proxy.ts +++ b/src/main/docker/docker-filter-proxy.ts @@ -82,6 +82,21 @@ function flattenHeaders(headers: http.IncomingHttpHeaders): Record, name: string): unknown => { + const wanted = name.toLowerCase(); + for (const [key, value] of Object.entries(obj)) if (key.toLowerCase() === wanted) return value; + return undefined; +}; + const isPlainRecord = (v: unknown): v is Record => typeof v === 'object' && v !== null && !Array.isArray(v); @@ -583,7 +598,7 @@ export class DockerFilterProxy { if (typeof parsed.Id === 'string' && parsed.Id.length > 0) { this.ownNetwork(parsed.Id, parsed.Id); const body = requested.body; - const name = isPlainRecord(body) ? body.Name : undefined; + const name = isPlainRecord(body) ? readFolded(body, 'Name') : undefined; if (typeof name === 'string' && name.length > 0) this.ownNetwork(name, parsed.Id); } } catch { diff --git a/test-results/.last-run.json b/test-results/.last-run.json new file mode 100644 index 0000000..cbcc1fb --- /dev/null +++ b/test-results/.last-run.json @@ -0,0 +1,4 @@ +{ + "status": "passed", + "failedTests": [] +} \ No newline at end of file From 25f7eb87af60eb5e31c0e4f9628df3eb96fde705 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 19:21:29 -0400 Subject: [PATCH 27/31] Stop committing Playwright's run state `test-results/.last-run.json` went in with the previous commit. Mine, and avoidable twice over: I ran `npx playwright test test/e2e/docker.spec.ts` without `--config test/playwright.config.ts`, which is what CI passes, so Playwright used its default output directory instead of the build/ path the config sets - and then `git add -A` swept the result in. Removed, with the two default paths ignored so a run that forgets the config flag cannot commit its state either. Verified by running it that way again: the directory comes back, git does not see it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- .gitignore | 5 +++++ test-results/.last-run.json | 4 ---- 2 files changed, 5 insertions(+), 4 deletions(-) delete mode 100644 test-results/.last-run.json diff --git a/.gitignore b/.gitignore index 1453312..d93ae3c 100644 --- a/.gitignore +++ b/.gitignore @@ -22,6 +22,11 @@ bundled-runner/ *.swp *.swo +# Playwright's defaults, for a run that forgets --config test/playwright.config.ts. +# The config routes output into build/; these are where it lands without it. +/test-results/ +/playwright-report/ + # OS .DS_Store Thumbs.db diff --git a/test-results/.last-run.json b/test-results/.last-run.json deleted file mode 100644 index cbcc1fb..0000000 --- a/test-results/.last-run.json +++ /dev/null @@ -1,4 +0,0 @@ -{ - "status": "passed", - "failedTests": [] -} \ No newline at end of file From b121a7c68812dc7e9394f6bee2664258da829130 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 19:40:33 -0400 Subject: [PATCH 28/31] Cover the network path with the real CLI, and make the check output earn its job Three things, all of them gaps rather than bugs. The docker policy's network path had no real-CLI coverage anywhere. That is the one path where fixture-driven tests have now been wrong twice: the allowlist was built from the spec instead of the wire, so a create body the unit tests accepted was refused on the socket, and again for the CLI keys the second time. The e2e now creates a network, inspects it by the name the job chose, joins a container to it and removes it, plus the two refusals - an undeclared name, and a name declared internal asked for routable. It runs on both legs, so it drives the production proxy inside a localmost job and this file's own proxy outside one. That needs the repository to declare the network, so .localmostrc gains `localmost-e2e-*`, internal. The localmost runner will treat this as policy drift until the new .localmostrc is approved in the app; the localmost-legged jobs fail until then, by design. docker.yaml's check job was consumed by nothing: docker-linux and docker-localmost both hardcode their runner, and neither declared `needs: check`. Rather than delete a job whose output is worth having, docker-linux now reads it and says out loud when there is no heartbeat - that a queued docker-localmost is waiting for a runner, not broken. It is the job that can say it, since it always runs and docker-localmost by design may not. Neither job's runner changes: docker-localmost stays self-hosted and still queues rather than skipping. Three check jobs appear in the PR checks list and two of them displayed identically as "check / check". They now carry distinct names - "ci runner", "docker runner", "inline runner". Job ids are untouched, so `needs: check` still resolves, and no ruleset pins a status check name. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- .github/workflows/ci.yaml | 5 ++- .github/workflows/docker.yaml | 15 ++++++++ .github/workflows/test-inline.yaml | 4 +++ .localmostrc | 7 ++++ test/e2e/docker.spec.ts | 55 +++++++++++++++++++++++++++++- 5 files changed, 84 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 602b654..73d88cd 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -12,7 +12,10 @@ permissions: jobs: check: - # Use the check workflow from the same ref (branch/SHA) as this workflow + # Use the check workflow from the same ref (branch/SHA) as this workflow. + # Named to distinguish it from the other workflows' check jobs; the job id + # stays `check`, which is what `needs:` below refers to. + name: ci runner uses: ./.github/workflows/check.yaml with: fallback: ubuntu-latest # This repo builds on Linux; typical users would use macos-latest diff --git a/.github/workflows/docker.yaml b/.github/workflows/docker.yaml index 675bfa5..babce97 100644 --- a/.github/workflows/docker.yaml +++ b/.github/workflows/docker.yaml @@ -25,13 +25,28 @@ permissions: jobs: check: + # Named so it is tellable apart from the other workflows' check jobs in the + # PR checks list, where three of them appear. + name: docker runner uses: ./.github/workflows/check.yaml with: fallback: ubuntu-latest docker-linux: + # Reads the check output so a stale heartbeat is stated rather than left to + # be inferred from a job that sits queued. This job is the one that can say + # it: it always runs, on a GitHub-hosted runner, while docker-localmost is + # by design waiting for a runner that may not be online yet. + needs: check runs-on: ubuntu-latest steps: + - name: Say whether a localmost runner is online + run: | + if [ "${{ needs.check.outputs.runner }}" = "self-hosted" ]; then + echo "localmost runner is online; docker-localmost will run on it" + else + echo "::warning::No localmost heartbeat - docker-localmost stays queued until a runner comes online. That is the intended behaviour, not a failure: the job exists to exercise the filtering socket, which only localmost serves." + fi - uses: actions/checkout@v4 - uses: ./.github/actions/docker-access diff --git a/.github/workflows/test-inline.yaml b/.github/workflows/test-inline.yaml index 75a6bec..899d4ac 100644 --- a/.github/workflows/test-inline.yaml +++ b/.github/workflows/test-inline.yaml @@ -12,6 +12,10 @@ permissions: jobs: check: + # This workflow deliberately inlines the heartbeat check that check.yaml + # offers as a reusable workflow: it is what proves the copy-paste-inline + # integration style still works, so the duplication is the point. + name: inline runner runs-on: ubuntu-latest outputs: runner: ${{ steps.check.outputs.runner }} diff --git a/.localmostrc b/.localmostrc index 54077e4..b6c76a7 100644 --- a/.localmostrc +++ b/.localmostrc @@ -15,6 +15,13 @@ shared: - path: ./ mode: ro network: bridge + # The docker e2e creates one network per run, joins a container to it and + # removes it, so the network path has real-CLI coverage rather than unit + # tests alone - twice now a body the tests accepted was refused on the + # wire. Internal: these carry no traffic off the machine. + networks: + - name: "localmost-e2e-*" + internal: true network: allow: diff --git a/test/e2e/docker.spec.ts b/test/e2e/docker.spec.ts index 7d2bf3a..b839972 100644 --- a/test/e2e/docker.spec.ts +++ b/test/e2e/docker.spec.ts @@ -49,7 +49,12 @@ const REPOSITORY = 'owner/repo'; */ const policy: DockerPolicy = { pull: { registries: ['docker.io'] }, - run: { images: [IMAGE], mounts: [{ path: './', mode: 'ro' }], network: 'bridge' }, + run: { + images: [IMAGE], + mounts: [{ path: './', mode: 'ro' }], + network: 'bridge', + networks: [{ name: 'localmost-e2e-*', internal: true }], + }, }; /** The docker CLI a job would run, found on PATH the way the job's shell finds it. */ @@ -110,6 +115,7 @@ test.describe('a job using docker through the filtering socket', () => { let workspace: string; let env: NodeJS.ProcessEnv; const nonce = `hello-${process.pid}-${Date.now()}`; + const network = `localmost-e2e-${process.pid}`; // A real directory outside any workspace, so the refusal is "outside the job // workspace" rather than "cannot be resolved". Stands in for ~/.ssh. @@ -163,6 +169,8 @@ test.describe('a job using docker through the filtering socket', () => { }); test.afterAll(async () => { + // Before the proxy stops, and tolerant of a test that already removed it. + if (env) await docker('network', 'rm', network).catch(() => undefined); await proxy?.stop(); if (scratch) fs.rmSync(scratch, { recursive: true, force: true }); if (jobWorkspace) fs.rmSync(jobWorkspace, { recursive: true, force: true }); @@ -232,4 +240,49 @@ test.describe('a job using docker through the filtering socket', () => { expect(denial?.policyHint).toMatch(/path: "\.\/"\n\s*mode: rw/); } }); + test('creates a declared network, joins a container to it, and removes it', async () => { + // The unit tests judge a body this file cannot see. Twice a create body + // they accepted was refused on the wire, because the real CLI sends keys + // the allowlist was never shown - so the network path is driven by the + // real CLI here, not only by fixtures. + const at = mark(); + + const created = await docker('network', 'create', '--internal', network); + expect(created.code, created.stderr).toBe(0); + + // Addressable by the name the job chose, not only by the id the daemon + // assigned: the proxy records both when it relays the create. + const inspect = await docker('network', 'inspect', network); + expect(inspect.code, inspect.stderr).toBe(0); + + const joined = await docker('run', '--rm', '--network', network, IMAGE, 'true'); + expect(joined.code, joined.stderr).toBe(0); + + const removed = await docker('network', 'rm', network); + expect(removed.code, removed.stderr).toBe(0); + + if (logs) { + const since = logsSince(at); + expect(since.some((l) => /forwarded POST \/networks\/create/.test(l.message))).toBe(true); + expect(since.filter((l) => /^(denied|refused) /.test(l.message))).toEqual([]); + } + }); + + test('refuses a network the policy does not declare, and one declared internal made routable', async () => { + const at = mark(); + + const undeclared = await docker('network', 'create', 'not-declared-by-policy'); + expect(undeclared.code).not.toBe(0); + expect(undeclared.stderr).toMatch(/not declared in the repository docker policy \(run\.networks\)/); + + // The name matches, but dropping --internal asks for a routable network, + // which is strictly more reachable than what the policy granted. + const routable = await docker('network', 'create', `${network}-routable`); + expect(routable.code).not.toBe(0); + expect(routable.stderr).toMatch(/declared internal, so it cannot be created routable/); + + if (logs) { + expect(logsSince(at).some((l) => /denied POST \/networks\/create/.test(l.message))).toBe(true); + } + }); }); From 0c44fa90b5ea5f9f4fd6cd79ad0701f7b49d4ccf Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 19:47:50 -0400 Subject: [PATCH 29/31] Show the docker grants the CLI was approving unseen `localmost policy approve` writes the whole .localmostrc to the cache, docker section included, and the runner enforces every bit of it. But `localmost policy show` rendered network, filesystem and env only: its PrintablePolicy type had no docker field at all, so images, mounts, networks and privileged could never print. An operator approving from the CLI never saw the container grants. The diff path does not save it either. The runner caches a policy when it first reads it, so by the time anyone runs `approve` the cached config already matches the working tree and diffConfigs has nothing to show. That is how it went here: the new networks entry was cached, unapproved, the instant CI picked the job up. Same class as the two the consumer found - a rule that is enforced but invisible where it is meant to be reviewed. The app already had a complete describer, so this moves it to src/shared/docker-policy.ts as describeDockerGrants and points both at it: the CLI and the app now describe one policy the same way rather than keeping two renderers that can drift. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/cli/policy.test.ts | 42 ++++++++++++++++++++++++++++++++- src/cli/policy.ts | 15 +++++++++++- src/main/ipc-handlers/policy.ts | 34 ++------------------------ src/shared/docker-policy.ts | 37 +++++++++++++++++++++++++++++ 4 files changed, 94 insertions(+), 34 deletions(-) diff --git a/src/cli/policy.test.ts b/src/cli/policy.test.ts index e45bc7d..743f972 100644 --- a/src/cli/policy.test.ts +++ b/src/cli/policy.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from '@jest/globals'; -import { parsePolicyArgs } from './policy'; +import { parsePolicyArgs, printPolicy } from './policy'; describe('CLI policy command', () => { describe('parsePolicyArgs', () => { @@ -82,3 +82,43 @@ describe('CLI policy command', () => { }); }); }); + +describe('policy show renders the docker grants', () => { + const capture = (policy: unknown): string => { + const lines: string[] = []; + const original = console.log; + console.log = (...args: unknown[]) => void lines.push(args.join(' ')); + try { + printPolicy(policy as never); + } finally { + console.log = original; + } + return lines.join('\n'); + }; + + it('names every container grant, since approving is what these are shown for', () => { + // `localmost policy approve` writes the whole .localmostrc to the cache, + // docker section included, but `show` rendered network, filesystem and env + // only - so the container, mount and network grants were approved unseen. + const out = capture({ + docker: { + pull: { registries: ['docker.io'] }, + run: { + images: ['alpine:3'], + mounts: [{ path: './', mode: 'ro' }], + network: 'bridge', + networks: [{ name: 'localmost-e2e-*', internal: true }], + }, + }, + }); + expect(out).toMatch(/docker pull: docker\.io/); + expect(out).toMatch(/docker run image: alpine:3/); + expect(out).toMatch(/docker mount: \.\/ \(ro\)/); + // Routable vs internal is the part an operator most needs to see. + expect(out).toMatch(/docker network create: localmost-e2e-\* \(internal\)/); + }); + + it('says nothing about docker when none is declared', () => { + expect(capture({ network: { allow: ['github.com'] } })).not.toMatch(/docker/i); + }); +}); diff --git a/src/cli/policy.ts b/src/cli/policy.ts index 15c199e..c2bb970 100644 --- a/src/cli/policy.ts +++ b/src/cli/policy.ts @@ -11,6 +11,7 @@ import * as fs from 'fs'; import * as path from 'path'; +import { DockerPolicy, describeDockerGrants } from '../shared/docker-policy'; import { findLocalmostrc, parseLocalmostrc, @@ -105,12 +106,17 @@ interface PrintablePolicy { network?: { allow?: string[]; deny?: string[] }; filesystem?: { read?: string[]; write?: string[]; deny?: string[] }; env?: { allow?: string[]; deny?: string[] }; + docker?: DockerPolicy; } /** * Print a policy section. + * + * Exported so a test can drive it directly: this is what an operator reads + * before running `localmost policy approve`, so what it leaves out is approved + * unseen. */ -function printPolicy(policy: PrintablePolicy): void { +export function printPolicy(policy: PrintablePolicy): void { if (!policy || Object.keys(policy).length === 0) { console.log(' (empty - uses defaults only)'); return; @@ -152,6 +158,13 @@ function printPolicy(policy: PrintablePolicy): void { } } + // The container grants: images, mounts, networks and whether a created + // network is routable. Described by the same code the app uses, so the two + // cannot drift into showing different things for one policy. + for (const grant of describeDockerGrants(policy.docker, '')) { + console.log(` ${colors.green}+${colors.reset} ${grant}`); + } + if (policy.env) { if (policy.env.allow?.length) { console.log(' Environment allow:'); diff --git a/src/main/ipc-handlers/policy.ts b/src/main/ipc-handlers/policy.ts index a860330..266251e 100644 --- a/src/main/ipc-handlers/policy.ts +++ b/src/main/ipc-handlers/policy.ts @@ -17,7 +17,7 @@ import { } from '../policy-cache'; import { getRunnerManager, getLogger } from '../app-state'; -import { DockerPolicy } from '../../shared/docker-policy'; +import { DockerPolicy, describeDockerGrants } from '../../shared/docker-policy'; /** * Describe what a policy grants, in the terms a reviewer cares about. @@ -37,36 +37,6 @@ interface PolicySection { * screen that showed nothing for it would be asking consent for an invisible * capability. */ -function describeDocker(docker: DockerPolicy | undefined, prefix: string): string[] { - if (!docker) return []; - const grants: string[] = []; - if (docker.pull) { - const registries = docker.pull.registries ?? []; - if (registries.length === 0) grants.push(`${prefix}docker pull`); - for (const registry of registries) grants.push(`${prefix}docker pull: ${registry}`); - } - if (docker.run) { - const { images = [], mounts = [], network, networks = [] } = docker.run; - if (images.length === 0 && mounts.length === 0 && networks.length === 0 && network === undefined) { - grants.push(`${prefix}docker run`); - } - for (const image of images) grants.push(`${prefix}docker run image: ${image}`); - for (const mount of mounts) grants.push(`${prefix}docker mount: ${mount.path} (${mount.mode})`); - // Creating a network is a grant, and whether it is routable is the part an - // operator most needs to see. - for (const n of networks) { - grants.push(`${prefix}docker network create: ${n.name} (${n.internal ? 'internal' : 'routable'})`); - } - if (network !== undefined) grants.push(`${prefix}docker network: ${network}`); - } - if (docker.build) { - grants.push(docker.build.context === undefined - ? `${prefix}docker build` - : `${prefix}docker build: ${docker.build.context}`); - } - if (docker.privileged) grants.push(`${prefix}docker privileged`); - return grants; -} function describeSection(section: PolicySection, prefix: string): string[] { const grants: string[] = []; @@ -82,7 +52,7 @@ function describeSection(section: PolicySection, prefix: string): string[] { for (const p of section.sockets?.allow || []) { grants.push(`${prefix}socket: ${p}`); } - grants.push(...describeDocker(section.docker, prefix)); + grants.push(...describeDockerGrants(section.docker, prefix)); return grants; } diff --git a/src/shared/docker-policy.ts b/src/shared/docker-policy.ts index 00445f2..86b40fa 100644 --- a/src/shared/docker-policy.ts +++ b/src/shared/docker-policy.ts @@ -480,3 +480,40 @@ export function parseDockerPolicyHint(hint: string): DockerPolicy | undefined { if (errors.length > 0) return undefined; return loaded.docker as DockerPolicy; } + +/** + * The container grants a docker policy makes, one line each, for anything that + * asks an operator to approve them. Shared so the CLI and the app describe the + * same policy the same way: `localmost policy show` once rendered network, + * filesystem and env only, and approved the docker section unseen. + */ +export function describeDockerGrants(docker: DockerPolicy | undefined, prefix: string): string[] { + if (!docker) return []; + const grants: string[] = []; + if (docker.pull) { + const registries = docker.pull.registries ?? []; + if (registries.length === 0) grants.push(`${prefix}docker pull`); + for (const registry of registries) grants.push(`${prefix}docker pull: ${registry}`); + } + if (docker.run) { + const { images = [], mounts = [], network, networks = [] } = docker.run; + if (images.length === 0 && mounts.length === 0 && networks.length === 0 && network === undefined) { + grants.push(`${prefix}docker run`); + } + for (const image of images) grants.push(`${prefix}docker run image: ${image}`); + for (const mount of mounts) grants.push(`${prefix}docker mount: ${mount.path} (${mount.mode})`); + // Creating a network is a grant, and whether it is routable is the part an + // operator most needs to see. + for (const n of networks) { + grants.push(`${prefix}docker network create: ${n.name} (${n.internal ? 'internal' : 'routable'})`); + } + if (network !== undefined) grants.push(`${prefix}docker network: ${network}`); + } + if (docker.build) { + grants.push(docker.build.context === undefined + ? `${prefix}docker build` + : `${prefix}docker build: ${docker.build.context}`); + } + if (docker.privileged) grants.push(`${prefix}docker privileged`); + return grants; +} From db34fcb7ee53efd34309fa865e1e921857690f66 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 19:55:07 -0400 Subject: [PATCH 30/31] Describe a policy in one place, and refuse a key nothing describes Moving the docker describer to shared fixed one instance. The class was still there: three renderers enumerated the policy keys by hand and each dropped a different one. `docker:` never printed in the CLI. `env:` never printed in the app's approval summary, so environment grants were approved unseen. The app still described `sockets:`, which the grammar stopped accepting. And `secrets:` - which workflow scope does accept and which decides what a job can read - printed nowhere at all. src/shared/policy-describe.ts now enumerates the keys once and returns structured grants. Presentation stays with the caller: `group` and `marker` for the CLI's grouped listing, `summary` for the app's flat one, so both show the same policy without a second enumeration to drift. PolicyApprovals renders the IPC grants, so the UI follows. The other half is validation. validatePolicy ignored any key it did not recognise, so `dokcer:` parsed clean, granted nothing, and appeared in no approval diff because no parser produced it - the exact shape of the `build:` defect a consumer reported. Unknown keys are now refused, naming the accepted ones, from the same list the describer walks. Scoped: a workflow may declare `secrets`, the shared section may not. That list is what stops this recurring. The guard test asserts every key in it produces a grant, so a key cannot be added to the grammar, be enforced, and stay invisible where it is meant to be read. It earned that immediately - `secrets` was missing from my first list, and the test found it rather than a person doing so later. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- src/cli/policy.ts | 76 +++++------------------------ src/main/ipc-handlers/policy.ts | 20 ++------ src/shared/localmostrc.test.ts | 44 +++++++++++++++++ src/shared/localmostrc.ts | 23 ++++++++- src/shared/policy-describe.test.ts | 54 +++++++++++++++++++++ src/shared/policy-describe.ts | 77 ++++++++++++++++++++++++++++++ 6 files changed, 211 insertions(+), 83 deletions(-) create mode 100644 src/shared/policy-describe.test.ts create mode 100644 src/shared/policy-describe.ts diff --git a/src/cli/policy.ts b/src/cli/policy.ts index c2bb970..90347f4 100644 --- a/src/cli/policy.ts +++ b/src/cli/policy.ts @@ -11,7 +11,7 @@ import * as fs from 'fs'; import * as path from 'path'; -import { DockerPolicy, describeDockerGrants } from '../shared/docker-policy'; +import { DescribablePolicy, describePolicy } from '../shared/policy-describe'; import { findLocalmostrc, parseLocalmostrc, @@ -102,12 +102,6 @@ shared: } } -interface PrintablePolicy { - network?: { allow?: string[]; deny?: string[] }; - filesystem?: { read?: string[]; write?: string[]; deny?: string[] }; - env?: { allow?: string[]; deny?: string[] }; - docker?: DockerPolicy; -} /** * Print a policy section. @@ -116,68 +110,22 @@ interface PrintablePolicy { * before running `localmost policy approve`, so what it leaves out is approved * unseen. */ -export function printPolicy(policy: PrintablePolicy): void { - if (!policy || Object.keys(policy).length === 0) { +export function printPolicy(policy: DescribablePolicy): void { + const grants = describePolicy(policy); + if (grants.length === 0) { console.log(' (empty - uses defaults only)'); return; } - if (policy.network) { - if (policy.network.allow?.length) { - console.log(' Network allow:'); - for (const domain of policy.network.allow) { - console.log(` ${colors.green}+${colors.reset} ${domain}`); - } - } - if (policy.network.deny?.length) { - console.log(' Network deny:'); - for (const domain of policy.network.deny) { - console.log(` ${colors.red}-${colors.reset} ${domain}`); - } - } - } - - if (policy.filesystem) { - if (policy.filesystem.read?.length) { - console.log(' Filesystem read:'); - for (const filePath of policy.filesystem.read) { - console.log(` ${colors.cyan}r${colors.reset} ${filePath}`); - } - } - if (policy.filesystem.write?.length) { - console.log(' Filesystem write:'); - for (const filePath of policy.filesystem.write) { - console.log(` ${colors.green}w${colors.reset} ${filePath}`); - } - } - if (policy.filesystem.deny?.length) { - console.log(' Filesystem deny:'); - for (const filePath of policy.filesystem.deny) { - console.log(` ${colors.red}-${colors.reset} ${filePath}`); - } - } - } - - // The container grants: images, mounts, networks and whether a created - // network is routable. Described by the same code the app uses, so the two - // cannot drift into showing different things for one policy. - for (const grant of describeDockerGrants(policy.docker, '')) { - console.log(` ${colors.green}+${colors.reset} ${grant}`); - } - - if (policy.env) { - if (policy.env.allow?.length) { - console.log(' Environment allow:'); - for (const name of policy.env.allow) { - console.log(` ${colors.green}+${colors.reset} ${name}`); - } - } - if (policy.env.deny?.length) { - console.log(' Environment deny:'); - for (const name of policy.env.deny) { - console.log(` ${colors.red}-${colors.reset} ${name}`); - } + const colorFor: Record = { '+': colors.green, '-': colors.red, r: colors.cyan, w: colors.green }; + let group = ''; + for (const grant of grants) { + if (grant.group !== group) { + group = grant.group; + console.log(` ${group}:`); } + const color = colorFor[grant.marker] ?? colors.green; + console.log(` ${color}${grant.marker}${colors.reset} ${grant.value}`); } } diff --git a/src/main/ipc-handlers/policy.ts b/src/main/ipc-handlers/policy.ts index 266251e..3ae4ff2 100644 --- a/src/main/ipc-handlers/policy.ts +++ b/src/main/ipc-handlers/policy.ts @@ -17,7 +17,8 @@ import { } from '../policy-cache'; import { getRunnerManager, getLogger } from '../app-state'; -import { DockerPolicy, describeDockerGrants } from '../../shared/docker-policy'; +import { DockerPolicy } from '../../shared/docker-policy'; +import { describePolicy } from '../../shared/policy-describe'; /** * Describe what a policy grants, in the terms a reviewer cares about. @@ -25,7 +26,6 @@ import { DockerPolicy, describeDockerGrants } from '../../shared/docker-policy'; interface PolicySection { network?: { allow?: string[] }; filesystem?: { read?: string[]; write?: string[] }; - sockets?: { allow?: string[] }; docker?: DockerPolicy; } @@ -39,21 +39,7 @@ interface PolicySection { */ function describeSection(section: PolicySection, prefix: string): string[] { - const grants: string[] = []; - for (const host of section.network?.allow || []) { - grants.push(`${prefix}network: ${host}`); - } - for (const p of section.filesystem?.read || []) { - grants.push(`${prefix}read: ${p}`); - } - for (const p of section.filesystem?.write || []) { - grants.push(`${prefix}write: ${p}`); - } - for (const p of section.sockets?.allow || []) { - grants.push(`${prefix}socket: ${p}`); - } - grants.push(...describeDockerGrants(section.docker, prefix)); - return grants; + return describePolicy(section, prefix).map((grant) => grant.summary); } /** diff --git a/src/shared/localmostrc.test.ts b/src/shared/localmostrc.test.ts index e77500d..3f34a18 100644 --- a/src/shared/localmostrc.test.ts +++ b/src/shared/localmostrc.test.ts @@ -989,3 +989,47 @@ describe('docker policy through serialization', () => { expect(serializeLocalmostrc(config)).not.toContain('docker'); }); }); + +describe('a policy key the grammar does not know', () => { + const parse = (body: string) => parseLocalmostrcContent(`version: 1\nshared:\n${body}`); + + it('is refused rather than ignored, since an ignored key grants nothing while looking like it grants', () => { + // The failure this prevents: a misspelled key validates clean, shows up in + // no approval diff because nothing parses it, and silently applies none of + // what it appears to declare. Already seen once with `build:`. + const result = parse(' dokcer:\n run:\n images: ["alpine:3"]\n'); + expect(result.success).toBe(false); + expect(result.errors.map((e) => e.message).join('\n')).toMatch(/dokcer/); + }); + + it('names the keys that are accepted, so the fix is in the message', () => { + const errors = parse(' filesystm:\n read: ["/etc"]\n').errors.map((e) => e.message).join('\n'); + for (const key of ['network', 'filesystem', 'env', 'docker']) expect(errors).toContain(key); + }); + + it('still accepts every key the grammar does know', () => { + const ok = parse( + ' network:\n allow: ["github.com"]\n' + + ' filesystem:\n read: ["/etc"]\n' + + ' env:\n allow: ["CI"]\n' + + ' docker:\n run:\n images: ["alpine:3"]\n' + ); + expect(ok.errors).toEqual([]); + expect(ok.success).toBe(true); + }); +}); + +describe('secrets is a workflow-scoped key', () => { + it('is accepted under a workflow', () => { + const r = parseLocalmostrcContent( + 'version: 1\nworkflows:\n deploy:\n secrets:\n require: ["DEPLOY_KEY"]\n' + ); + expect(r.errors).toEqual([]); + }); + + it('is refused at shared scope, where nothing reads it', () => { + const r = parseLocalmostrcContent('version: 1\nshared:\n secrets:\n require: ["DEPLOY_KEY"]\n'); + expect(r.success).toBe(false); + expect(r.errors.map((e) => e.message).join('\n')).toMatch(/shared\.secrets is not a policy key/); + }); +}); diff --git a/src/shared/localmostrc.ts b/src/shared/localmostrc.ts index 67c1e7a..2208985 100644 --- a/src/shared/localmostrc.ts +++ b/src/shared/localmostrc.ts @@ -4,6 +4,7 @@ * Handles parsing, validation, and merging of declarative sandbox policies. */ import * as yaml from 'js-yaml'; +import { POLICY_SECTION_KEYS, WORKFLOW_POLICY_KEYS } from './policy-describe'; import * as fs from 'fs'; import * as path from 'path'; import { SandboxPolicy, NetworkPolicy, FilesystemPolicy, EnvPolicy } from './sandbox-profile'; @@ -178,7 +179,8 @@ export function parseLocalmostrcContent(content: string): ParseResult { errors.push({ message: '"workflows" must be an object' }); } else { for (const [workflowName, policy] of Object.entries(config.workflows as Record)) { - validatePolicy(policy, `workflows.${workflowName}`, errors); + // A workflow may also require secrets; the shared scope may not. + validatePolicy(policy, `workflows.${workflowName}`, errors, WORKFLOW_POLICY_KEYS); validateSecretsPolicy(policy, `workflows.${workflowName}`, errors); } } @@ -207,7 +209,12 @@ export function parseLocalmostrcContent(content: string): ParseResult { /** * Validate a sandbox policy object. */ -function validatePolicy(policy: unknown, path: string, errors: ParseError[]): void { +function validatePolicy( + policy: unknown, + path: string, + errors: ParseError[], + accepted: readonly string[] = POLICY_SECTION_KEYS +): void { if (policy === null || policy === undefined) { return; // Empty policy is valid } @@ -219,6 +226,18 @@ function validatePolicy(policy: unknown, path: string, errors: ParseError[]): vo const p = policy as Record; + // A key nobody parses grants nothing while reading as though it grants + // something, and shows up in no approval diff because no parser produced it. + // The keys are listed in one place, shared with what describes a policy, so + // a new one cannot be accepted without also being shown. + for (const key of Object.keys(p)) { + if (accepted.includes(key)) continue; + if (key === 'sockets') continue; // Has its own message, below. + errors.push({ + message: `${path}.${key} is not a policy key. Accepted keys: ${accepted.join(', ')}.`, + }); + } + // Validate network policy if (p.network !== undefined) { validateNetworkPolicy(p.network, `${path}.network`, errors); diff --git a/src/shared/policy-describe.test.ts b/src/shared/policy-describe.test.ts new file mode 100644 index 0000000..7a13758 --- /dev/null +++ b/src/shared/policy-describe.test.ts @@ -0,0 +1,54 @@ +import { describe, it, expect } from '@jest/globals'; +import { describePolicy, POLICY_SECTION_KEYS, WORKFLOW_POLICY_KEYS } from './policy-describe'; + +/** + * A section declaring something under every key a policy may carry. The guard + * test below leans on it: whatever the grammar grows, it has to appear here + * and it has to come back out of describePolicy. + */ +const everything = { + network: { allow: ['github.com'], deny: ['evil.example'] }, + filesystem: { read: ['/etc'], write: ['~/.npm'], deny: ['~/.ssh'] }, + env: { allow: ['CI'], deny: ['AWS_SECRET_ACCESS_KEY'] }, + docker: { + pull: { registries: ['docker.io'] }, + run: { images: ['alpine:3'], mounts: [{ path: './', mode: 'ro' as const }], networks: [{ name: 'vk-*', internal: true }] }, + }, + secrets: { require: ['DEPLOY_KEY'] }, +}; + +describe('describePolicy', () => { + it('names every value the policy declares, whatever key it sits under', () => { + const text = describePolicy(everything).map((g) => `${g.group} ${g.marker} ${g.value} ${g.summary}`).join('\n'); + for (const value of [ + 'github.com', 'evil.example', '/etc', '~/.npm', '~/.ssh', + 'CI', 'AWS_SECRET_ACCESS_KEY', 'docker.io', 'alpine:3', 'vk-*', 'DEPLOY_KEY', + ]) { + expect(text).toContain(value); + } + }); + + it('covers every key the grammar accepts, so a new one cannot be enforced unseen', () => { + // The defect this guards: a key that validates and is enforced but that no + // renderer prints is approved without being read. It has happened twice - + // `docker:` was missing from the CLI, `env:` from the app. + for (const key of WORKFLOW_POLICY_KEYS) { + const only = { [key]: (everything as Record)[key] }; + expect(describePolicy(only).length).toBeGreaterThan(0); + } + }); + + it('keeps the workflow-only key out of the shared list, which is what validation scopes on', () => { + expect(POLICY_SECTION_KEYS).not.toContain('secrets'); + expect(WORKFLOW_POLICY_KEYS).toContain('secrets'); + }); + + it('describes nothing for an empty section', () => { + expect(describePolicy({})).toEqual([]); + }); + + it('prefixes the flat summary, which is how a workflow scope is shown', () => { + const [grant] = describePolicy({ network: { allow: ['github.com'] } }, 'ci: '); + expect(grant.summary).toBe('ci: network: github.com'); + }); +}); diff --git a/src/shared/policy-describe.ts b/src/shared/policy-describe.ts new file mode 100644 index 0000000..2d94266 --- /dev/null +++ b/src/shared/policy-describe.ts @@ -0,0 +1,77 @@ +/** + * One description of a policy, for everything that shows one to a person. + * + * There were three renderers before this: the CLI's `policy show`, the app's + * approval summary, and the diff. Each enumerated the policy keys by hand, and + * each left out a different one - `docker:` never printed in the CLI, `env:` + * never printed in the app, and the app still described `sockets:`, a key the + * grammar had stopped accepting. A grant that is enforced but never rendered + * is approved without being read, which is the whole failure this file exists + * to prevent. + * + * So the keys are enumerated once, here. Presentation stays with the caller: + * `group` and `marker` are for a grouped, coloured listing, `summary` is the + * flat one-line form. Adding a key to the grammar means adding it here, and + * the guard test in policy-describe.test.ts fails until it is. + */ + +import { DockerPolicy, describeDockerGrants } from './docker-policy'; + +/** Every key a policy section may declare at any scope. */ +export const POLICY_SECTION_KEYS = ['network', 'filesystem', 'env', 'docker'] as const; + +/** + * What a workflow-scoped policy may declare on top of those: which secrets the + * workflow requires, which is a grant like any other and is shown like one. + */ +export const WORKFLOW_POLICY_KEYS = [...POLICY_SECTION_KEYS, 'secrets'] as const; + +export type PolicySectionKey = (typeof POLICY_SECTION_KEYS)[number]; + +/** A section of a policy, as the grammar accepts it. */ +export interface DescribablePolicy { + network?: { allow?: string[]; deny?: string[] }; + filesystem?: { read?: string[]; write?: string[]; deny?: string[] }; + env?: { allow?: string[]; deny?: string[] }; + docker?: DockerPolicy; + /** Workflow scope only. */ + secrets?: { require?: string[] }; +} + +export interface PolicyGrant { + /** Heading for a grouped listing, printed once per run of grants. */ + group: string; + /** Single character marking what the entry does: + grant, - deny, r/w access. */ + marker: string; + /** The declared value, as written. */ + value: string; + /** The flat one-line form, already prefixed. */ + summary: string; +} + +export function describePolicy(policy: DescribablePolicy, prefix = ''): PolicyGrant[] { + const grants: PolicyGrant[] = []; + const add = (group: string, marker: string, label: string, values: string[] | undefined) => { + for (const value of values ?? []) { + grants.push({ group, marker, value, summary: `${prefix}${label}: ${value}` }); + } + }; + + add('Network allow', '+', 'network', policy.network?.allow); + add('Network deny', '-', 'network denied', policy.network?.deny); + add('Filesystem read', 'r', 'read', policy.filesystem?.read); + add('Filesystem write', 'w', 'write', policy.filesystem?.write); + add('Filesystem deny', '-', 'denied', policy.filesystem?.deny); + add('Environment allow', '+', 'env', policy.env?.allow); + add('Environment deny', '-', 'env denied', policy.env?.deny); + + add('Secrets required', '+', 'secret', policy.secrets?.require); + + // Docker describes itself: what a container grant means is the docker + // grammar's business, and the line it produces is already the flat form. + for (const grant of describeDockerGrants(policy.docker, '')) { + grants.push({ group: 'Docker', marker: '+', value: grant, summary: `${prefix}${grant}` }); + } + + return grants; +} From aeb1b8bd87d0fb30652a5fc5427732d521547522 Mon Sep 17 00:00:00 2001 From: Bright Fulton Date: Sun, 6 Sep 2026 20:02:50 -0400 Subject: [PATCH 31/31] Let the check job announce its own finding docker-linux was given `needs: check` so it could say when there is no localmost heartbeat. That was make-work to justify a job whose output docker.yaml does not otherwise consume, and it cost what coupling costs: when the check job was cancelled, docker-linux was cancelled with it, though it runs GitHub-hosted whatever the heartbeat says. The fact belongs to the job that determines it. check.yaml already has a single point where it gives up on a local runner, so the warning goes there, and no caller needs a dependency to repeat it. Every caller gets it, which is right: a run silently falling back to a GitHub-hosted runner is worth seeing on a machine bought to run them locally. Not solved with `timeout-minutes` on docker-localmost, the other obvious candidate: that bounds execution, not time spent queued for a self-hosted runner, so a runner that never comes online still hangs until GitHub's own 24h limit. docker-linux is independent again. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh --- .github/workflows/check.yaml | 9 ++++++++- .github/workflows/docker.yaml | 15 +++------------ 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/.github/workflows/check.yaml b/.github/workflows/check.yaml index a9d6f6c..ea0c6ff 100644 --- a/.github/workflows/check.yaml +++ b/.github/workflows/check.yaml @@ -53,6 +53,13 @@ jobs: echo "No LOCALMOST_HEARTBEAT variable found" fi - # Default: use fallback runner + # Default: use fallback runner. Announced here rather than by a + # caller, because this is the job that knows: a caller would need + # `needs: check` purely to repeat it, which couples a job to one it + # does not otherwise depend on and takes it down whenever this one + # fails. A job pinned to self-hosted cannot fall back and will queue, + # and `timeout-minutes` does not bound time spent waiting for a + # runner, so this line is the only warning it gets. + echo "::warning title=localmost runner offline::No localmost heartbeat, so this run falls back to $FALLBACK. Any job pinned to the self-hosted runner stays queued until one comes online." echo "No localmost runner available, using $FALLBACK" echo "runner=$FALLBACK" >> $GITHUB_OUTPUT diff --git a/.github/workflows/docker.yaml b/.github/workflows/docker.yaml index babce97..5edf02c 100644 --- a/.github/workflows/docker.yaml +++ b/.github/workflows/docker.yaml @@ -33,20 +33,11 @@ jobs: fallback: ubuntu-latest docker-linux: - # Reads the check output so a stale heartbeat is stated rather than left to - # be inferred from a job that sits queued. This job is the one that can say - # it: it always runs, on a GitHub-hosted runner, while docker-localmost is - # by design waiting for a runner that may not be online yet. - needs: check + # Deliberately independent of the check job: it runs GitHub-hosted whatever + # the heartbeat says, so depending on check would only mean a cancelled + # check cancels this too. The heartbeat is announced by check itself. runs-on: ubuntu-latest steps: - - name: Say whether a localmost runner is online - run: | - if [ "${{ needs.check.outputs.runner }}" = "self-hosted" ]; then - echo "localmost runner is online; docker-localmost will run on it" - else - echo "::warning::No localmost heartbeat - docker-localmost stays queued until a runner comes online. That is the intended behaviour, not a failure: the job exists to exercise the filtering socket, which only localmost serves." - fi - uses: actions/checkout@v4 - uses: ./.github/actions/docker-access