diff --git a/AGENTS.md b/AGENTS.md index 0e19440..8ce3dd2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -37,17 +37,17 @@ deploy/ — Docker Compose + integration tests - Zero external deps where possible (Go: yaml.v3, Rust: tokio/hyper/serde/clap, TS: yaml) ## Test Coverage -- Go: 102 unit tests (main/listener: 23, policy: 10, middleware: 29, proxy: 36, audit: 4) +- Go: 103 unit tests (main/listener: 24, policy: 10, middleware: 29, proxy: 36, audit: 4) - Rust: 140 unit tests (main/listener: 23, policy: 15, middleware: 50, proxy: 42, handler: 4, audit: 4, transport: 2) - TypeScript: 157 unit tests, 1 skipped (flags: 44, listen: 13 incl. 1 skipped concurrency test (#46), middleware: 41, proxy: 29, policy: 10, handler: 6, shutdown: 5, transport: 5, audit: 4) -- Integration, per implementation: 35 tests via deploy/test.sh and 15 socket tests via deploy/test-sock.sh (docker-compose) +- Integration, per implementation: 36 tests via deploy/test.sh and 15 socket tests via deploy/test-sock.sh (docker-compose) - Quint: `make test-spec` runs the `spec/listener.qnt` `run` tests (instances `listener_locked`, `listener_unlocked`) and the `spec/router.qnt` `run` tests (instances `router`, `router_pre48`) ## Test Conventions - Go: stdlib `testing` package, `go test ./...` - Rust: `#[cfg(test)]` inline modules, `cargo test` - TypeScript: `node:test` framework, `npm run build && node --test dist/*.test.js` -- Integration: `make test-integration` (35 test cases) and `make test-integration-sock` (15 socket cases) via Docker Compose +- Integration: `make test-integration` (36 test cases) and `make test-integration-sock` (15 socket cases) via Docker Compose ## Contribution Workflow diff --git a/README.md b/README.md index f6a2dfa..0c9de58 100644 --- a/README.md +++ b/README.md @@ -100,7 +100,7 @@ All three implementations expose the same API surface, share the same [Quint spe | Language | Directory | Tests | Stack | |----------|-----------|-------|-------| -| Go | [go/](go/) | 102 unit + 35 integration | stdlib net/http + yaml.v3 | +| Go | [go/](go/) | 103 unit + 36 integration | stdlib net/http + yaml.v3 | | Rust | [rs/](rs/) | 140 unit | tokio, hyper, serde, clap | | TypeScript | [ts/](ts/) | 157 unit (1 skipped) | Node 22 ESM, built-in http | diff --git a/deploy/test.sh b/deploy/test.sh index 8ce1dd1..2cf0dea 100755 --- a/deploy/test.sh +++ b/deploy/test.sh @@ -336,6 +336,11 @@ check "POST /v1.45/containers/*/start -> 404 (daemon answered, not proxy 403)" " S=$(delete_status "$PROXY/v1.45/containers/json") check "DELETE /v1.45/containers/json -> 403 (reserved survives version strip)" "403" "$S" +# #57: /volumes/ is not an API-version prefix. The daemon would run a volume +# removal; the proxy must not classify it as a container delete. +S=$(delete_status "$PROXY/volumes/containers/no-such-container") +check "DELETE /volumes/containers/no-such-container -> 403 (not a version prefix)" "403" "$S" + # #52: versioned create, the `docker run` path. Mirrors the unversioned # allowed-image create above: the daemon answers 201, or 404 when the image is # not present locally — anything but a proxy 403. diff --git a/go/internal/proxy/router.go b/go/internal/proxy/router.go index cb6767d..2c7471b 100644 --- a/go/internal/proxy/router.go +++ b/go/internal/proxy/router.go @@ -158,16 +158,35 @@ func (r *Router) routeImagePull(body map[string]interface{}) *RouteResult { return &RouteResult{Action: ActionAllow, Image: fromImage} } +// stripAPIVersion strips one leading /v/ or /v./ (ASCII digits only), else returns path unchanged (#57). func stripAPIVersion(path string) string { - if strings.HasPrefix(path, "/v") { - parts := strings.SplitN(path, "/", 3) - if len(parts) >= 3 { - return "/" + parts[2] + if !strings.HasPrefix(path, "/v") { + return path + } + i := scanDigits(path, 2) + if i == 2 { + return path + } + if i < len(path) && path[i] == '.' { + j := scanDigits(path, i+1) + if j == i+1 { + return path } + i = j + } + if i < len(path) && path[i] == '/' { + return path[i:] } return path } +func scanDigits(s string, i int) int { + for i < len(s) && s[i] >= '0' && s[i] <= '9' { + i++ + } + return i +} + func matchEndpoint(path, resource, endpoint string) bool { path = strings.TrimPrefix(path, "/") parts := strings.SplitN(path, "/", 3) diff --git a/go/internal/proxy/router_test.go b/go/internal/proxy/router_test.go index fbff51a..ee02066 100644 --- a/go/internal/proxy/router_test.go +++ b/go/internal/proxy/router_test.go @@ -401,14 +401,12 @@ allowed_image_prefixes: } } -// TestRouteVersionedPaths is the cross-language parity guard for #52. +// TestRouteVersionedPaths is the cross-language parity guard for #52 and #57. // // The Docker CLI prefixes every request with a dotted API version // (/v1.43/containers/create). The router must strip the prefix and route the -// rest exactly like the unversioned path. TypeScript only stripped undotted -// prefixes (/v1/...), so dotted paths fell through to the default deny. -// The TS table also pins TS-only over-strip rows, because Go and Rust strip -// any /v…/ first segment (#55). +// rest exactly like the unversioned path. The table pins one canonical prefix +// rule, ^/v\d+(\.\d+)?/ with ASCII digits, in all three languages (#52, #57). func TestRouteVersionedPaths(t *testing.T) { m := newTestManager(t, map[string]string{ "beacon.yaml": ` @@ -435,6 +433,28 @@ allowed_image_prefixes: {"POST", "/v1/containers/beacon/start", nil, ActionAllow}, // #52: the reserved segment survives the strip (#24 parity). {"DELETE", "/v1.43/containers/json", nil, ActionDeny}, + // #57: undotted version, container lifecycle delete. + {"DELETE", "/v1/containers/foo", nil, ActionAllow}, + // #57: multi-digit major version. + {"DELETE", "/v10.0/containers/foo", nil, ActionAllow}, + // #57: /volumes/ is not a version; the daemon routes this as a volume removal. + {"DELETE", "/volumes/containers/foo", nil, ActionDeny}, + // #57: /version/ is not a version prefix. + {"DELETE", "/version/containers/foo", nil, ActionDeny}, + // #57: two dots is not an API version. + {"DELETE", "/v1.2.3/containers/foo", nil, ActionDeny}, + // #57: no digits after v. + {"DELETE", "/vabc/containers/foo", nil, ActionDeny}, + // #57: bare v. + {"DELETE", "/v/containers/foo", nil, ActionDeny}, + // #57: dot without a minor version. + {"DELETE", "/v1./containers/foo", nil, ActionDeny}, + // #57: strip once; the remaining /v1.43/containers/foo matches no route. + {"DELETE", "/v1/v1.43/containers/foo", nil, ActionDeny}, + // #57: a non-ASCII digit (U+0661 ARABIC-INDIC DIGIT ONE) is not a version digit. + {"DELETE", "/v\u0661/containers/foo", nil, ActionDeny}, + // #57 sanity row, cannot fail: GET /version is allowed as a read-only request. + {"GET", "/version", nil, ActionAllow}, } for _, tt := range tests { t.Run(tt.method+" "+tt.path, func(t *testing.T) { diff --git a/rs/src/proxy.rs b/rs/src/proxy.rs index a8d37f4..44763bc 100644 --- a/rs/src/proxy.rs +++ b/rs/src/proxy.rs @@ -197,11 +197,32 @@ fn deny(msg: &str) -> RouteResult { } } +// Strips one leading /v/ or /v./ (ASCII digits only); anything else is left as is (#57). fn strip_api_version(path: &str) -> &str { - if let Some(rest) = path.strip_prefix("/v") { - if let Some(idx) = rest.find('/') { - return &rest[idx..]; + let Some(rest) = path.strip_prefix("/v") else { + return path; + }; + let bytes = rest.as_bytes(); + let digits = |from: usize| { + bytes[from..] + .iter() + .take_while(|b| b.is_ascii_digit()) + .count() + }; + let major = digits(0); + if major == 0 { + return path; + } + let mut i = major; + if bytes.get(i) == Some(&b'.') { + let minor = digits(i + 1); + if minor == 0 { + return path; } + i += 1 + minor; + } + if bytes.get(i) == Some(&b'/') { + return &rest[i..]; } path } @@ -507,13 +528,11 @@ mod tests { } } - /// Cross-language parity guard for #52. The Docker CLI prefixes every - /// request with a dotted API version (/v1.43/containers/create). The - /// router must strip the prefix and route the rest exactly like the - /// unversioned path. TypeScript only stripped undotted prefixes - /// (/v1/...), so dotted paths fell through to the default deny. - /// The TS table also pins TS-only over-strip rows, because Go and Rust - /// strip any /v…/ first segment (#55). + /// Cross-language parity guard for #52 and #57. The Docker CLI prefixes + /// every request with a dotted API version (/v1.43/containers/create). + /// The router must strip the prefix and route the rest exactly like the + /// unversioned path. The table pins one canonical prefix rule, + /// ^/v\d+(\.\d+)?/ with ASCII digits, in all three languages (#52, #57). #[test] fn test_route_versioned_paths() { let router = Router::new(make_manager(vec!["alpine"])); @@ -530,6 +549,28 @@ mod tests { ("POST", "/v1/containers/beacon/start", None, Action::Allow), // #52: the reserved segment survives the strip (#24 parity). ("DELETE", "/v1.43/containers/json", None, Action::Deny), + // #57: undotted version, container lifecycle delete. + ("DELETE", "/v1/containers/foo", None, Action::Allow), + // #57: multi-digit major version. + ("DELETE", "/v10.0/containers/foo", None, Action::Allow), + // #57: /volumes/ is not a version; the daemon routes this as a volume removal. + ("DELETE", "/volumes/containers/foo", None, Action::Deny), + // #57: /version/ is not a version prefix. + ("DELETE", "/version/containers/foo", None, Action::Deny), + // #57: two dots is not an API version. + ("DELETE", "/v1.2.3/containers/foo", None, Action::Deny), + // #57: no digits after v. + ("DELETE", "/vabc/containers/foo", None, Action::Deny), + // #57: bare v. + ("DELETE", "/v/containers/foo", None, Action::Deny), + // #57: dot without a minor version. + ("DELETE", "/v1./containers/foo", None, Action::Deny), + // #57: strip once; the remaining /v1.43/containers/foo matches no route. + ("DELETE", "/v1/v1.43/containers/foo", None, Action::Deny), + // #57: a non-ASCII digit (U+0661 ARABIC-INDIC DIGIT ONE) is not a version digit. + ("DELETE", "/v\u{0661}/containers/foo", None, Action::Deny), + // #57 sanity row, cannot fail: GET /version is allowed as a read-only request. + ("GET", "/version", None, Action::Allow), ]; for (method, path, body, want) in cases { let got = router.route(method, path, body); diff --git a/ts/src/proxy.test.ts b/ts/src/proxy.test.ts index 5b3b680..f176bc4 100644 --- a/ts/src/proxy.test.ts +++ b/ts/src/proxy.test.ts @@ -225,15 +225,11 @@ describe("Router", () => { } }); - // Guard for #52. The first five rows are the cross-language parity table: - // the Docker CLI prefixes every request with a dotted API version - // (/v1.43/containers/create), and the router must strip the prefix and route - // the rest exactly like the unversioned path. stripAPIVersion only stripped - // undotted prefixes (/v1/...), so dotted paths fell through to the default - // deny. The last three rows are TS-only: they pin TS's intended strip - // (/v or /v. only). Go and Rust currently strip any - // /v…/ first segment and so ALLOW both over-strip paths — tracked in - // #55; do not copy those rows there as parity. + // Cross-language parity guard for #52 and #57. The Docker CLI prefixes every + // request with a dotted API version (/v1.43/containers/create), and the + // router must strip the prefix and route the rest exactly like the + // unversioned path. The table pins one canonical prefix rule, + // ^/v\d+(\.\d+)?/ with ASCII digits, in all three languages (#52, #57). it("routes dotted API-version paths like the unversioned ones", () => { const cases: [string, string, Record | undefined, Action][] = [ // #52: dotted version, container lifecycle delete. @@ -246,15 +242,28 @@ describe("Router", () => { ["POST", "/v1/containers/beacon/start", undefined, Action.Allow], // #52: the reserved segment survives the strip (#24 parity). ["DELETE", "/v1.43/containers/json", undefined, Action.Deny], - // #52 sanity row, cannot fail: GET /version lands on the GET passthrough - // whatever is stripped. Kept as documentation of the expected outcome. - ["GET", "/version", undefined, Action.Allow], - // #52 over-strip guard (TS-only, see header): stripping /volumes/ as a - // version prefix would make this DELETE /containers/foo -> Allow. + // #57: undotted version, container lifecycle delete. + ["DELETE", "/v1/containers/foo", undefined, Action.Allow], + // #57: multi-digit major version. + ["DELETE", "/v10.0/containers/foo", undefined, Action.Allow], + // #57: /volumes/ is not a version; the daemon routes this as a volume removal. ["DELETE", "/volumes/containers/foo", undefined, Action.Deny], - // #52 over-strip guard (TS-only, see header): two dots is not an API - // version; stripping /v1.2.3/ would make this DELETE /containers/foo -> Allow. + // #57: /version/ is not a version prefix. + ["DELETE", "/version/containers/foo", undefined, Action.Deny], + // #57: two dots is not an API version. ["DELETE", "/v1.2.3/containers/foo", undefined, Action.Deny], + // #57: no digits after v. + ["DELETE", "/vabc/containers/foo", undefined, Action.Deny], + // #57: bare v. + ["DELETE", "/v/containers/foo", undefined, Action.Deny], + // #57: dot without a minor version. + ["DELETE", "/v1./containers/foo", undefined, Action.Deny], + // #57: strip once; the remaining /v1.43/containers/foo matches no route. + ["DELETE", "/v1/v1.43/containers/foo", undefined, Action.Deny], + // #57: a non-ASCII digit (U+0661 ARABIC-INDIC DIGIT ONE) is not a version digit. + ["DELETE", "/v\u0661/containers/foo", undefined, Action.Deny], + // #57 sanity row, cannot fail: GET /version is allowed as a read-only request. + ["GET", "/version", undefined, Action.Allow], ]; for (const [method, path, body, want] of cases) { const r = router.route(method, path, body);