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

[abienkowski]

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?

• Go
• Rust
• TypeScript
• Quint specification
• CI / infrastructure

[abienkowski]

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.

[abienkowski]

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.

[abienkowski]

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.

[abienkowski]

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.

[abienkowski]

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.