Docker filter hardening: 21 review findings, three endpoint families, and a log-injection fix

[bfulton]

Hardening for the docker isolation filter merged in #35, plus the three endpoint families a real consumer needs, plus a job-start bug that turned out to be a log-injection hole.

Everything here is TDD: each fix has a test that fails first, for the right reason, and the end-to-end suite runs against a real Docker daemon after every change.

Where the work came from

A six-dimension adversarial review of the merged filter (escape/bypass, fail-closed, policy grammar, socket lifecycle, real-CLI correctness, test quality), each finding then attacked by three skeptics on distinct lenses; a majority refute killed it. 21 findings survived and all 21 are fixed here. Two more I found while reproducing them, and one arrived as consumer feedback from wiring a container-heavy repo to the shipped socket.

Sandbox escapes and filter bypasses

• Anonymous volume with local-driver bind options (critical) — a Mounts entry of Type: volume with no Source skipped every mount check, but the built-in local driver with type=none,o=bind,device=<path> is a bind mount. Arbitrary host path, read-write, whatever the policy declared.
• Case-folded keys (critical) — the evaluator read HostConfig.Privileged with case-sensitive property access while the daemon decodes with Go's encoding/json, which falls back to a case-insensitive field match. {"hostconfig":{"privileged":true,"binds":["/:/host:rw"]}} was invisible to every gate and honoured in full.
• Upgrade tunnelling (critical) — GET /_ping is baseline, so an Upgrade header on it opened a raw pipe to the daemon; a denied GET /containers/json smuggled over that socket returned 200. Only attach upgrades now, and only after the daemon actually answers 101.
• HostConfig was a blocklist (high) — so every key nobody enumerated was forwarded unexamined, PortBindings among them, publishing a container port on the operator's interfaces outside the proxy that controls the job's egress. It is an allowlist now, built from the 62 keys a real docker run sends — captured from the CLI, after two attempts guessed wrong and broke docker run outright.
• Build parameters unfiltered (high) — docker build --network host walked through a door create keeps shut.
• Bind-mount TOCTOU (high) — the filter resolved a symlink to decide and forwarded the job's spelling for the daemon to resolve again. The verdict now carries the body it approved, with sources pinned to the paths actually checked.

Consent and correctness

• The approval screen showed no docker grants at all (high), and a bare run: {} — which permits creating and running containers — produced zero approval diff (high). An operator could approve container access without being shown any.
• Registry auth was never wired (high) — the proxy could always attach X-Registry-Auth and nothing passed it the callback, so private-registry pulls could not work and "credentials stop entering the sandbox" described a capability that did not exist. Resolved in the app now, following the CLI's own order: per-registry helper, credential store, then inline auths. Docker Desktop keeps secrets in a helper, so inline-only would have covered nobody.
• A drifted worker was reopened at job start (high) — the claim path cuts the network and leaves the docker socket shut; the job-start refresh runs without isClaim, never re-checks drift, and fell straight through to re-widening both. Found independently by two dimensions.
• Phantom jobs from job output (high, found while reproducing) — the job-start detector matched Running job: anywhere in worker output. The merge commit for CLI target management, Docker isolation, config hardening, and App permission docs #35 contains that literal text, so checking it out made localmost read three job starts that never happened. Any repo could do it deliberately with a commit message.
• Malformed request targets threw out of the handler and hung the connection; a released slot kept the finished job's context, so the next worker was judged against the previous repository; --name containers were refused though the job created them, while a removed container's prefix still opened the door; mounts rooted at _work instead of the checkout, making any path narrower than ./ unmatchable; per-workflow keys resolved against github.workflow (the workflow's name:) rather than the filename the docs promise.

The three endpoint families (consumer feedback)

• Networks — the one that mattered most, because the direction was backwards: an --internal network makes a container less reachable, so denying network creation forced strictly weaker isolation than the workload wanted. Two corrections to the original proposal: the driver is unnameable (macvlan/ipvlan puts a container on the physical LAN, worse than --network=host), and NetworkMode must accept an owned network or a job could create vk-1 and then be refused when it tried to use it.
• Image inspect — scoped by the policy rather than a second ownership ledger: inspecting an image run.images already names discloses nothing new, and the container ledger had already produced one defect of exactly that kind.
• kill / stop / logs — stop and logs added beyond the ask; refusing logs contradicted the documented "reads about the job's own containers" baseline.

What wiring a real consumer up to it found

Four more rounds came back from a container-heavy eval harness pointed at the filtering socket. Every one was the same failure mode — a rule that reads wider than it matches — and each was measured on the wire, not read off the spec.

• The allowlist was built from the spec, not the wire. docker network create was refused outright, because the CLI always sends keys the allowlist had never seen. Same class as the earlier ContainerIDFile and PortBindings breaks: the fix is a captured body, not a careful reading. Keys the CLI always sends are now value-gated rather than absent — present-and-inert passes, present-and-set is refused by name.
• * crossed /. vk/grader:* matched vk/grader:a/b. Anchored and segment-scoped now; a tag glob is unaffected, since a tag cannot contain a slash.
• BuildKit is unfilterable, so jobs are pinned off it. A default docker build issues zero POST /build — it negotiates a gRPC session and streams the context over /grpc, so no request carries the paths it reads. Jobs now run with DOCKER_BUILDKIT=0, and /grpc gets a denial that says which builder and which variable rather than shrugging "unknown endpoint". This has a shelf life: the classic builder is deprecated.
• A tagless glob covered almost nothing. vk/* allowed vk/setup-tools and denied vk/setup-tools:1 — not the glob's doing, but normalisation appending :latest to both sides. Rather than guess that :* was meant, validation refuses a tagless glob and names vk/*:* in the message, for the reason docker: true is refused rather than interpreted.

Dependabot #167

extract-zip symlink path traversal (high, dev-only). There is no patched release and never will be; npm's suggested fix is a downgrade to a breaking older forge. Aliased to @electron-internal/extract-zip, the maintained fork @electron/packager 20 uses. Overriding packager to ^20 was tried and rejected — forge 7.11.2 is the newest forge, still asks for ^18.3.5, and packaging fails with TypeError: done is not a function.

Things a reviewer should look at rather than take on trust

1. 126bc25 is red in history. I chained the gate and the commit into one step and it committed despite a failing e2e. Fixed forward in d1757a9; it will bite a bisect.
2. Two deliberate behaviour changes: the daemon now receives the resolved mount path rather than the job's spelling, and mounts root at the checkout (_work/<repo>/<repo>) rather than _work — narrower than before.
3. privileged: true is now a validation error, not just a request-time denial. An existing .localmostrc declaring it stops parsing.
4. build.context was fixed in the docs, not the code. It is not enforceable — the Engine API carries the context as a tar the client already assembled, so no path reaches the filter; the seatbelt profile is what confines it. Writing a fake check would have been worse than saying so.

1076 tests, tsc and eslint clean, e2e 4/4 against a live daemon.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XVXbqWE6zBi1HV7b4uobKh

[Copilot]

🟡 Changes recommended

The consent/approval surfaces still omit or under-describe newly added docker grants (notably run.networks) which can make operator approval incomplete or misleading.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Hardens the Docker filtering socket introduced in #35 by expanding supported Engine API coverage (networks, image inspect, lifecycle endpoints), tightening request parsing and evaluation to close bypasses, and fixing policy/consent correctness issues in the runner runtime.

Changes:

• Extends docker policy grammar and evaluator to support controlled network creation, image inspect, and additional container lifecycle endpoints (kill/stop/logs), with strict allowlisting and ownership scoping.
• Hardens the proxy/evaluator against bypasses (malformed request targets, upgrade tunneling, HostConfig key allowlist, case-insensitive JSON decoding, mount TOCTOU via pinned resolved sources).
• Fixes runner/runtime correctness issues (job-start output injection, policy drift reopening, stale slot context) and wires registry auth resolution in the app so credentials never enter the sandbox.

File summaries

File	Description
test/e2e/docker.spec.ts	Updates e2e setup for repo-aware workspace mount root and proxy bind.
src/shared/localmostrc.test.ts	Adjusts config serialization tests for privileged validation change.
src/shared/docker-policy.ts	Adds run.networks, validates/merges it, and improves approval diffing/serialization.
src/shared/docker-policy.test.ts	Adds tests for bare action-block diffing, privileged validation, and run.networks.
src/main/runner-manager.ts	Fixes slot context cleanup, job-start detection hardening, and policy drift stickiness.
src/main/runner-manager.test.ts	Adds regression tests for injected job-start output, drift stickiness, and slot context carryover.
src/main/ipc-handlers/policy.ts	Adds docker grants into policy approval summarization and exports summarizeGrants.
src/main/ipc-handlers/policy.test.ts	New tests ensuring docker grants and bare action blocks appear in approval summary.
src/main/index.ts	Wires registry auth resolution into RunnerManager via attachRegistryAuth.
src/main/docker/registry-auth.ts	Implements Docker credential resolution (helper → store → inline auths) and header encoding.
src/main/docker/registry-auth.test.ts	Tests credential resolution order, default registry keying, and failure behavior.
src/main/docker/docker-request.ts	Adds endpoint classification for new actions and refuses non-origin-form/malformed targets.
src/main/docker/docker-request.test.ts	Tests new endpoint mappings and target parsing refusal behavior.
src/main/docker/docker-filter-proxy.ts	Tracks network/container ownership aliases, pins approved mount paths, hardens upgrade handling, and refuses unreadable targets cleanly.
src/main/docker/docker-filter-proxy.test.ts	Adds coverage for upgrade tunneling prevention, mount pinning, unreadable targets, name-based ownership, and networks.
src/main/docker/docker-evaluator.ts	Hardens request evaluation (case-insensitive decoding behavior, HostConfig/build allowlists, build network gating, networks and image inspect).
src/main/docker/docker-evaluator.test.ts	Adds extensive regression tests for case-folding, volumes-as-binds, HostConfig allowlist, build params, networks, and image inspect.
src/main/docker/docker-backend.ts	Makes workspace mount root repo-aware (checkout-rooted) to match workflow path semantics.
src/main/docker/docker-backend.test.ts	Tests repo-aware mount root vs fallback behavior.
src/main/broker-proxy-service.ts	Derives workflow keying from github.workflow_ref filename (fallback to github.workflow).
src/main/broker-proxy-service.test.ts	Tests workflow filename extraction across extensions and ref formats.
package.json	Aliases extract-zip to @electron-internal/extract-zip fork to address advisory constraints.
package-lock.json	Locks the aliased extract-zip fork and removes now-unused transitive deps.
docs/superpowers/specs/2026-09-06-docker-endpoint-families.md	Adds design/spec addendum for the three endpoint families and rationale.
docs/superpowers/specs/2026-09-05-docker-isolation-design.md	Corrects build context enforcement claims (documentation-only + remote refusal).
docs/roadmap/localmostrc.md	Updates docs for workflow matching semantics and expanded docker action coverage/constraints.

Review details

• Files reviewed: 25/26 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The approval UI still omits run.networks grants and network-create evaluation is not fail-closed under Go’s case-insensitive JSON decoding, which can undermine consent and enforcement.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/main/ipc-handlers/policy.ts:56

• describeDocker() doesn’t include the new run.networks grants (ability to create networks), so the approval UI can still under-report what an operator is consenting to when a policy adds networks.

  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}`);
  }

• Files reviewed: 25/26 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

src/main/docker/docker-evaluator.ts still has mount-parsing paths that are not fail-closed under Go’s case-insensitive JSON decoding, leaving a potential host mount bypass.

Review details

Suppressed comments (7)

Previously missed (1) — in code that hasn't changed since the last review.

src/main/docker/registry-auth.ts:96

• resolveRegistryAuth always calls credential helpers with serveraddress=keys[0] (usually the bare registry), even if the helper was discovered under a different config key (e.g. credHelpers["https://ghcr.io"]). That can make stored credentials undiscoverable. Track which key selected the helper and use that as serveraddress when invoking the helper and encoding the auth.

This issue also appears on line 108 of the same file.

src/main/docker/docker-evaluator.ts:284

• parseMount reads Mount.Type via pick(), which is not fail-closed under Go's case-insensitive JSON decoding: a request can supply multiple casings (e.g. Type vs type) with different values and the daemon may honor a different Type than the filter judged, potentially bypassing mount checks (e.g. tmpfs vs bind). Refuse when Type values disagree across casings before continuing.

  const type = pick(mount, 'Type');
  if (type === 'tmpfs') return null;

src/main/docker/docker-evaluator.ts:295

• The volume-driver bypass fix is still case-sensitive: it checks VolumeOptions.DriverConfig via pick(), so a request can include both DriverConfig (null) and driverconfig (object) and potentially have the daemon honor the latter while the filter judges the former. Check every casing with valuesFor(...) and deny if any is present.

    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';
    }

src/main/docker/docker-evaluator.ts:297

• For volume mounts, Source is checked via pick(), so a request can supply Source:"" (anonymous) alongside source:"named" and potentially have the daemon honor the named volume while the filter allows it. Refuse when any casing supplies a non-empty Source value (and validate types), rather than only the picked one.

    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`;

src/main/docker/docker-evaluator.ts:303

• For bind mounts, Source is selected via pick(), so multiple casings with different values (e.g. Source:"/safe" and source:"/") can cause the filter to validate one path while the daemon mounts another. Refuse when non-unset Source values disagree across casings before proceeding.

  const source = pick(mount, 'Source');
  if (typeof source !== 'string' || !path.isAbsolute(source)) {
    return 'a bind mount needs an absolute Source';
  }

src/main/docker/docker-evaluator.ts:310

• BindOptions.Propagation is read via pick(), so conflicting casings can bypass the propagation allowlist (the daemon may honor a different casing/value than the filter checked). Validate every casing with valuesFor(...) so any disallowed value is refused.

    if (!isPlainObject(options)) return 'BindOptions must be an object';
    const propagation = pick(options, 'Propagation') ?? '';
    if (!PROPAGATIONS.has(propagation as string)) {
      return `mount propagation "${String(propagation)}" is not permitted`;
    }

src/main/docker/registry-auth.ts:112

• When falling back to inline auths entries, the code encodes serveraddress=keys[0] even if the matching auth entry came from a different key (e.g. https:///v1/). That can break auth on registries whose config key isn’t the bare hostname. Encode the serveraddress using the specific auth-entry key that matched.

  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;

• Files reviewed: 26/27 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

It introduces at least one committed generated artifact (test-results/.last-run.json) and a Jest test file that uses jest.* without importing jest from @jest/globals, which can break test execution in ESM mode.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/main/ipc-handlers/policy.test.ts:1

• This test uses jest.mock/jest.fn but doesn’t import jest from @jest/globals. In Jest’s ESM mode, jest is not injected as a global, so this can fail at runtime; other tests in this repo import jest explicitly when needed.

• Files reviewed: 26/27 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Registry credential helper lookup likely fails when credentials are configured under a non-primary config key due to always using keys[0] as the helper/serveraddress, which can break private-registry pulls.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/main/docker/registry-auth.ts:121

• resolveRegistryAuth() selects a per-registry helper by scanning multiple possible config keys, but always passes keys[0] as the serverUrl to the helper (and as serveraddress in the encoded auth). If the helper/credential is configured under a different key (e.g. https://<registry>/v1/), this can cause helper lookups (and/or the daemon’s auth matching) to fail even though a credential exists.
  src/main/docker/docker-filter-proxy.ts:318
• The doc comment above decide() says it returns null when the request may proceed, but decide() now returns an object containing a refusal (or null) plus an optional rewritten body. This mismatch can cause incorrect usage when the code is modified later.

• Files reviewed: 25/27 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

There are at least two correctness issues in the new/changed code (a Jest test file that may not have jest in scope, and registry-helper auth lookup using a potentially wrong server URL key) that should be fixed before merging.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/main/docker/registry-auth.ts:106

• Registry helper lookup can select a helper configured under a non-primary key (e.g. https://<registry>), but still calls the helper with serveraddress = keys[0]. That makes the helper query the wrong server URL and can silently drop auth for registries that Docker stores under the alternate keys.
  src/main/ipc-handlers/policy.test.ts:1
• This test uses jest.mock/jest.fn() but doesn’t import jest from @jest/globals, unlike other Jest tests in this repo that use module mocking. If jest isn’t provided globally in the test runtime, this will throw before the tests run.

• Files reviewed: 29/31 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

evaluateNetworkCreate uses only the first matching run.networks declaration, which can incorrectly deny routable network creates when multiple matching declarations exist with different internal values.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/main/docker/docker-evaluator.ts:658

• Network declarations can be additive (mergeNetworks dedupes by name+internal), so it’s possible for multiple run.networks entries to match the same requested network name with different internal values. Using declared.find(...) means only the first match is considered; this can incorrectly deny a routable create even if another matching declaration allows internal: false.

Consider matching against all declarations: deny only when no declarations match, and (for a routable request) deny only when all matching declarations are internal: true.

• Files reviewed: 31/33 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It makes wide, security-sensitive changes across the Docker filter/proxy/evaluator surface and needs final human review despite strong test additions.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/main/docker/registry-auth.ts:106

• resolveRegistryAuth always passes keys[0] as serveraddress to the credential helper. If credHelpers is configured under a different key variant (e.g. https://<registry>/v1/), the helper lookup will succeed but the helper will be queried with a different serverUrl than the key it was configured for, which can miss the stored credential.

This issue also appears on line 108 of the same file.

src/main/docker/registry-auth.ts:112

• When falling back to an inline auths entry, the encoded AuthConfig uses serveraddress = keys[0] even if the matching entry was found under a different key variant. This can produce an X-Registry-Auth payload whose serveraddress doesn’t match the config entry that provided the credential.

  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;

• Files reviewed: 34/36 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Registry auth resolution currently always uses keys[0] as serveraddress, which can mis-handle credentials stored under other common config keys (e.g. https://<registry>), potentially breaking authenticated pulls.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/main/docker/registry-auth.ts:99

• serveraddress is always set to keys[0] (e.g. quay.io), but the helper can be configured under a different key (e.g. https://quay.io in credHelpers). In that case helper is found, but runHelper(helper, serveraddress) queries the wrong address and will return no credential even though one exists.

This issue also appears on line 108 of the same file.

src/main/docker/registry-auth.ts:112

• When falling back to inline auths, the code always encodes serveraddress as keys[0], even if the matching config entry was found under a different key (e.g. https://quay.io or quay.io/v1/). This can produce an X-Registry-Auth payload whose serveraddress doesn’t correspond to the credential entry that was actually found.

  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;

• Files reviewed: 35/37 changed files
• Comments generated: 0 new
• Review effort level: Lite