Deferred minor findings from the #43/#47/#50/#51 review cycles

Problem

The review processes for #45 (PR #47), #24 (PR #43), #39 (PR #50) and #48 (PR #51) produced a set of deliberately deferred Minor findings — real but not merge-blocking. Recording them here so they stop living only in session logs.

Cross-language parity (smallest first)

HEAD on a named container: HEAD /containers/x — Go and TS allow via the GET/HEAD passthrough; Rust denies in its lifecycle branch (rs/src/proxy.rs catch-all). Related to Routing parity: TS path-wide exec deny; Go matchEndpoint accepts endpoint subpaths #49.
Numeric gid edge parity (--listen-socket-group): negative values — Go rejects with "negative gid", Rust/TS fail via name lookup; leading + — all three now reject (post-feat!: dockerd-parity listening socket, Unix path only #47 fix) but via different paths; oversized values fail differently per language. Behaviourally all deny; messages differ.
Name-lookup gid range: Go's pure-Go resolver and TS's /etc/group parser don't range-check a gid obtained by name lookup (the digits-only path does). Exploiting it requires control of /etc/group.
Probe "other connect error" messages differ by OS error wording (Go connect: permission denied vs Rust Permission denied (os error 13) vs Node connect EACCES); tests assert the refusing to remove prefix only.
Linux full-backlog probe: a live listener with a full backlog → Rust times out → "in use by another process"; Go gets EAGAIN → "refusing to remove … resource temporarily unavailable". Both safe (nothing unlinked); message diverges.

Listener (from #45's reviews)

macOS full-backlog live listener can return ECONNREFUSED → probe treats it as stale (only lockless peers exposed; Go/Rust hold the flock).
Lstat→Remove window: a lockless process's swapped-in file could be removed (tiny race, design accepts it).
Rust leaves the 0600 socket file on disk when chown/chmod fails (Go's Close unlinks); pre-existing parity gap.
SIGKILL test child can leak on t.Fatal in Go (add t.Cleanup); GC test passes partly by construction.
Concurrency-test losers in Go assert a substring ("is in use by another"), Rust asserts the exact message.
EPERM tests could also skip when getegid()==0.

Tests & tooling

Integration checks assert HTTP status only; asserting the deny body ("not allowed") would stop a daemon-side 403 masking a proxy regression (no authz plugin in the test stack today, so theoretical).
Rust/TS route-table tests stop at the first failing row; Go uses t.Run per row.
Go's extractContainerName empty-string test documents the contract but can't fail for the Rust extract_container_name treats empty segment as a container name (DELETE /containers/ is forwarded) #48 cause ("" is both "no name" and the raw value).
Go flag-removal test builds the binary in-test; TS VALUE_FLAGS/BOOL_FLAGS exported mutable.
ROUTER_SPEC := vs ?= used by the other Makefile spec vars.
test-sock.sh has a duplicate "GET /_ping -> 200" label (granted vs default-group).

Docs

README test table shows "+ 32 integration" on the Go row only, though the suite runs per implementation; the 15 socket tests aren't in the table.
README "all 9 invariants" sentence predates the listener/router modules.
spec/README's "tracked with the other routing divergences" (exec note) could cite Routing parity: TS path-wide exec deny; Go matchEndpoint accepts endpoint subpaths #49 directly.
spec/listener-design.md still has pre-feat!: dockerd-parity listening socket, Unix path only #47 group-table wording in one historical section (fixed in the normative table; the history records the old plan).

Proposed solution

Work through these in one or two chore:/docs:/test: PRs, or pick items off when touching the files anyway. None changes behaviour except the parity items, which should each get the #24/#48 treatment (decide canonical row, converge, pin with same-named tests).

Which implementation(s) would this affect?

Adding a cross-language item found while pinning #52 (verified by running the Go router; Rust by reading strip_api_version, rs/src/proxy.rs:200-207):

API-version over-strip in Go and Rust. Both strip any first segment starting with v (Go router.go:161-169, Rust strip_api_version), not just /v<digits>[.<digits>]/. Observed (Go router, main 8865134):
```
DELETE /volumes/containers/foo  -> Allow (routed as container "foo")
DELETE /v1.2.3/containers/foo   -> Allow
DELETE /version/containers/foo  -> Allow
```
TypeScript, after the TypeScript proxy denies dotted API-version paths (/v1.43/...) that the Docker CLI always sends #52 fix, strips only /v<major> and /v<major>.<minor> and denies all three; ts/src/proxy.test.ts pins that as TS-only rows.
Why it is more than cosmetic: a request outside the containers namespace (/volumes/…, whose DELETEs are otherwise denied) is classified as a container lifecycle call and forwarded with its original path. The daemon then routes it as a volume request. The impact looks low because volume names cannot contain /, so the daemon should refuse containers/foo as a volume name. That has not been checked against a live daemon, so treat this as worth a Go router doesn't exclude reserved path segments in extractContainerName (cross-language parity) #24/Rust extract_container_name treats empty segment as a container name (DELETE /containers/ is forwarded) #48-style convergence (canonical rule ^/v\d+(\.\d+)?/, same-named rows in all three languages), not just a doc note.

The API-version over-strip item is now tracked as its own bug: #57 (P2). Docker confirmed that DELETE /volumes/containers/foo is handled as a volume removal, while Go and Rust classify it as a container delete.

One more item: no formatter check in make lint-*. lint-go runs only go vet, lint-rs only cargo check, and lint-ts only typecheck. On main (a654ff8), gofmt -l go/internal/proxy/ lists router.go and transport_test.go, ts/src/proxy.test.ts fails prettier --check, and cargo fmt --check reports diffs. Proposal: one formatting commit per language, then add gofmt -l (fail if it prints anything), cargo fmt --check and prettier --check to the lint targets and CI. Keep it separate from behaviour changes so the diffs stay reviewable.

One more item, found in the #53 review: the audit uri field differs across implementations (pre-existing). Go logs r.RequestURI (raw, with the query), Rust logs path (no query), and TS logs req.url (raw, with the query). After #53, Go's extra.path is the escaped routing path, so Go's audit line carries both forms. Proposal: log the raw request target (path + query) as uri in all three, and pin it with a shared audit test.

From the #54 final review (non-blocking): in release.yml's version step, the || true on the latest-tag search also hides a real git tag failure. That would fall through to the no-tag path and could cut a stray v0.0.1. It is unlikely with fetch-depth: 0. The tag-selection pipeline (strict filter, version sort, --merged HEAD) also has no automated test; only a one-off simulation covered it.

From the #49 review: ExecGate can never fire, and the README overstates it. All three ExecGates check the substring /exec (go/internal/middleware/exec.go:14, rs/src/middleware.rs:84, ts/src/middleware.ts:54). The middleware chain runs only on POST /containers/create, so the gate never sees an exec path; the router denies exec first (segment-exact since #49). The README middleware row ("Denies POST /containers/*/exec and POST /exec/*/start") describes something the gate never does. Options: (a) drop the gate; (b) keep it as defence in depth and reword the README row; (c) make it segment-exact like the router, so a future chain caller cannot reintroduce the exec-runner false positive. If the gate stays, (c) matters.

From the #49 final review: spec/docker_socket_policy.qnt endpointsTable (~:181, and the literal list at ~:453) still lists only POST /containers/:name/exec. After #49, exec is denied for every method on /containers/<name>/exec and on the whole /exec/* namespace. The table is documentation only (a self-tautology, see spec/README Modeling Notes), so the gap is cosmetic. Update it together with the ExecGate item above.

From the #49 review: double slash before exec skips the exec check in all three routers. GET //exec/<id>/json (and /v1.45//exec/...) has an empty first segment, so isExecPath does not match, and the GET passthrough forwards it. Pre-#49 TS (includes("/exec")) denied it, so TS regressed slightly. No data leaks. A live daemon (29.4.0, with a real running exec instance) answers 301 with an empty body and Location: /exec/<id>/json for every double-slash form, and a client that follows the redirect re-enters the proxy on the clean path and is denied. Non-GET methods hit the default deny. Proposed shared fix for all three routers plus the Quint model: deny or normalise paths with empty interior segments. This is the same family as the #48 empty-segment rule.

chore: call the repo prettier directly; document fmt targets and hooks (

From the PR #68 review, deferred (not blocking #68):

TS runtime image ships devDependencies. ts/Dockerfile:26 copies the build stage's node_modules into the runtime image, so typescript, @types/node and now prettier (~8 MB) ship in a security-sensitive image. Proposal: RUN npm prune --omit=dev after npx tsc in the build stage, then verify the reproducible build and the integration suites.
rs/rustfmt.toml has no style_edition. A future edition bump to 2024 would change import ordering and reformat the crate. Consider pinning style_edition = "2021" (check that Rust 1.85's rustfmt accepts it) when the edition changes.

chore: enforce formatting with lint checks and git hooks (#55) (#68)

test(release): cover a latest-tag lookup with an unresolvable ref (#55)

fix(release): fail on git errors in the latest-tag lookup and test it (…

test: pin the TypeScript absolute-form denial; split the empty-segmen…

fix: deny empty interior path segments; allow HEAD on named container…

fix: log the full request target in Rust audit and unlink the socket …

test: tighten listener, route-table and integration assertions (#55) (#…

docs: state the router model scope and the --readonly gap precisely (#55

refactor: remove the unreachable ExecGate; refresh docs and counts (#55…

Resolved in separate PRs

Item	PR	Release
Formatters, lint gates, git hooks	#68	v0.3.2
Release latest-tag lookup fails on git errors; `ROUTER_SPEC ?=`; socket test labels	#69	v0.3.3
`//` in the path denied; HEAD on named containers in Rust; spec `router_pre55`	#71	v0.3.4
Rust audit `uri` = request target; Rust unlinks the socket when chown/chmod fails	#72	v0.3.5
Test-quality fixes; integration checks assert the proxy deny message	#73	v0.3.6
ExecGate removed (unreachable); docs and counts refreshed	#74	v0.3.7

Found along the way:

fix: deny empty interior path segments; allow HEAD on named containers in Rust (#55) #71 also fixed a TypeScript exec-inspect leak in v0.3.1–v0.3.3 through absolute-form request targets (GET http://docker/exec/<id>/json → 200 with the command line).
Follow-ups: Router allows exec paths hidden behind dot segments (/./exec, /x/../exec) #70 (dot segments /./exec, /x/../exec), --readonly only blocks container create; lifecycle actions and DELETE are still forwarded #75 (--readonly only blocks container create).
Still deferred from the chore: enforce formatting with lint checks and git hooks (#55) #68 review (comment above): TS runtime image ships devDependencies; rs/rustfmt.toml has no style_edition.

Navigation Menu

Repository navigation

Deferred minor findings from the #43/#47/#50/#51 review cycles #55

Problem

Cross-language parity (smallest first)

Listener (from #45's reviews)

Tests & tooling

Docs

Proposed solution

Which implementation(s) would this affect?

Resolved in separate PRs

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

Deferred minor findings from the #43/#47/#50/#51 review cycles #55

Description

Problem

Cross-language parity (smallest first)

Listener (from #45's reviews)

Tests & tooling

Docs

Proposed solution

Which implementation(s) would this affect?

Activity

abienkowski commented on Oct 7, 2026

abienkowski commented on Oct 7, 2026

abienkowski commented on Oct 7, 2026

abienkowski commented on Oct 7, 2026

abienkowski commented on Oct 7, 2026

abienkowski commented on Oct 8, 2026

abienkowski commented on Oct 8, 2026

abienkowski commented on Oct 8, 2026

abienkowski commented on Oct 9, 2026

abienkowski commented on Oct 11, 2026

Resolved in separate PRs

Metadata

Metadata

Assignees

Labels

Type

Fields

Projects

Milestone

Relationships

Development

Issue actions