Repository navigation
Deferred minor findings from the #43/#47/#50/#51 review cycles #55
Description
Activity
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(Gorouter.go:161-169, Ruststrip_api_version), not just/v<digits>[.<digits>]/. Observed (Go router, main8865134):TypeScript, after the TypeScript proxy denies dotted API-version paths (/v1.43/...) that the Docker CLI always sends #52 fix, strips onlyDELETE /volumes/containers/foo -> Allow (routed as container "foo") DELETE /v1.2.3/containers/foo -> Allow DELETE /version/containers/foo -> Allow/v<major>and/v<major>.<minor>and denies all three;ts/src/proxy.test.tspins 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 refusecontainers/fooas 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.
- API-version over-strip in Go and Rust. Both strip any first segment starting with
The API-version over-strip item is now tracked as its own bug: #57 (P2). Docker confirmed that
DELETE /volumes/containers/foois 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-goruns onlygo vet,lint-rsonlycargo check, andlint-tsonlytypecheck. Onmain(a654ff8),gofmt -l go/internal/proxy/listsrouter.goandtransport_test.go,ts/src/proxy.test.tsfailsprettier --check, andcargo fmt --checkreports diffs. Proposal: one formatting commit per language, then addgofmt -l(fail if it prints anything),cargo fmt --checkandprettier --checkto 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
urifield differs across implementations (pre-existing). Go logsr.RequestURI(raw, with the query), Rust logspath(no query), and TS logsreq.url(raw, with the query). After #53, Go'sextra.pathis the escaped routing path, so Go's audit line carries both forms. Proposal: log the raw request target (path + query) asuriin all three, and pin it with a shared audit test.From the #54 final review (non-blocking): in
release.yml's version step, the|| trueon the latest-tag search also hides a realgit tagfailure. That would fall through to the no-tag path and could cut a strayv0.0.1. It is unlikely withfetch-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 onPOST /containers/create, so the gate never sees an exec path; the router denies exec first (segment-exact since #49). The README middleware row ("DeniesPOST /containers/*/execandPOST /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 theexec-runnerfalse positive. If the gate stays, (c) matters.From the #49 final review:
spec/docker_socket_policy.qntendpointsTable(~:181, and the literal list at ~:453) still lists onlyPOST /containers/:name/exec. After #49, exec is denied for every method on/containers/<name>/execand 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
execskips the exec check in all three routers.GET //exec/<id>/json(and/v1.45//exec/...) has an empty first segment, soisExecPathdoes 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) answers301with an empty body andLocation: /exec/<id>/jsonfor 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.- added a commit that references this issue
on Oct 9, 2026 From the PR #68 review, deferred (not blocking #68):
- TS runtime image ships devDependencies.
ts/Dockerfile:26copies the build stage'snode_modulesinto the runtime image, so typescript,@types/nodeand now prettier (~8 MB) ship in a security-sensitive image. Proposal:RUN npm prune --omit=devafternpx tscin the build stage, then verify the reproducible build and the integration suites. rs/rustfmt.tomlhas nostyle_edition. A future edition bump to 2024 would change import ordering and reformat the crate. Consider pinningstyle_edition = "2021"(check that Rust 1.85's rustfmt accepts it) when the edition changes.
- TS runtime image ships devDependencies.
- added a commit that references this issue
on Oct 10, 2026 - added 4 commits that reference this issue
on Oct 10, 2026
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 /containers/x— Go and TS allow via the GET/HEAD passthrough; Rust denies in its lifecycle branch (rs/src/proxy.rscatch-all). Related to Routing parity: TS path-wide exec deny; Go matchEndpoint accepts endpoint subpaths #49.--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./etc/groupparser don't range-check a gid obtained by name lookup (the digits-only path does). Exploiting it requires control of/etc/group.connect: permission deniedvs RustPermission denied (os error 13)vs Nodeconnect EACCES); tests assert therefusing to removeprefix only.Listener (from #45's reviews)
Lstat→Removewindow: a lockless process's swapped-in file could be removed (tiny race, design accepts it).t.Fatalin Go (addt.Cleanup); GC test passes partly by construction.getegid()==0.Tests & tooling
t.Runper row.extractContainerNameempty-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).VALUE_FLAGS/BOOL_FLAGSexported mutable.ROUTER_SPEC :=vs?=used by the other Makefile spec vars.test-sock.shhas a duplicate "GET /_ping -> 200" label (granted vs default-group).Docs
spec/listener-design.mdstill 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?