fix(managementapi): screen unstorable bytes out of path values

Why

{slug} and {repository_name} reached the datastore unscreened. LabKit's pool runs pgx in simple protocol, which interpolates the path value into the query text, so PostgreSQL rejects the statement itself: 08P01 for a NUL, 22021 for invalid UTF-8. resolveSlug mapped both to logAndWriteInternalError, so a%00b in a path segment was a client-typable, ERROR-logged 500 on every management route, needing nothing but a valid Bearer token (internal/auth/dispatch.go:202-215). This screens both byte classes where the value enters the handler and answers the route's existing existence-hiding 404. A PostgreSQL text value can hold neither class, so "resolves to no namespace" holds by the type rather than by a constraint.

404 rather than 400 is S17's identifier-lookup rule, not a codebase-wide one. docs/specs/S17-rest-management-api.md:501 and :503 give one status for missing-or-invalid to avoid a syntax oracle, and ## Resolutions records it settled at :1618. The contract enforces it: 24 of the 44 OpenAPI operations on {slug} paths declare no 400, none lacks 404, and internal/managementapi/contract_test.go:133 validates responses with IncludeResponseStatus: true, so a 400 would fail the existing suite. Other surfaces decide on their own rules, and two answer 400: S10 Maven for these exact bytes (internal/format/maven/parse.go:98) and npm (docs/testing/e2e/npm.md:165).

What the fix removes is the ERROR level and the 5xx classification, not the signal. internal/logging/handler.go:306 still logs every status at or above 400 unsampled, and labkit's access logger records the raw request target, so the %00 stays in the line. The residual gap is metrics-only: no label separates a screened 404 from a genuine miss.

What

  • The two packages.go screens are unreachable on every route that exists today, because every route reaching them resolves the parent through findRepositoryForRequest first. They stay as second-line defense, and the white-box TestChildResolves_UnstorableRepositoryNameIs404WithoutRead is the only thing pinning them: a route-driven test passes with both deleted.
  • storableTagName delegates to the shared predicate instead of being renamed. The rename fails scripts/ci/check-comment-caps.sh on two blocks, container_tag_delete.go at 21 lines against an exported-doc cap of 3 and container_tag_delete_test.go at 15 against a _test.go cap of 2, costing roughly 31 lines of reviewed contract prose. Substituting the predicate at its three call sites fails the same two blocks. The one-line delegation is the accepted trade-off.
  • Control bytes stay out of scope. Percent-encoded they match no row and already answer 404, and literal ones net/url rejects with a 400 before routing. The narrowing is against the title of Reject NUL and control bytes in path values bef... (#484) • Hayley Swimelar, whose body names only the NUL mechanism, and the tests pin a%01b and a%7Fb as accepted and forwarded verbatim.
  • detail.go:80 is a second FindByName bind and is exempt, because its row.Name came from the row findRepositoryForRequest had just read.
  • The Error Cases table has no row for this class, so the status extends :501 and :503 by analogy alongside the rows at :1403 and :1412. One asymmetry is worth naming: :1408 and :1420 answer 400 for a cursor "carrying a boundary no stored value can hold", the same semantic class on a different carrier. A spec row belongs in a spec MR.
  • The screen this reuses landed on feat(managementapi): untag a container image tag (!1545 - merged) • Hayley Swimelar • 19.3, whose reviewer named RepositoryStore.FindByName as unscreened at the time.

No e2e scenario is affected. docs/testing/e2e/ holds four per-format client-API user-journey catalogs, and none covers a management-API malformed path segment. Conformance tests do not apply, since this is the management API rather than Maven, npm, or OCI protocol behavior.

Test plan

CI. The test(...) commit carries two panic stubs in resolve.go, the sanctioned skeleton that makes it red and what git log --stat shows on the first commit.

Ten screen mutations ran against the suite. Each was caught by the test written to catch it, both widening directions fail (control bytes plus DEL, and an ASCII-range check in place of UTF-8 validity), and both halves of the predicate are pinned independently.

This covers the management API's path values. Three surfaces keep the identical failure and are untouched: npm (internal/format/npm/middleware.go:102-106 into resolver.go:225), Maven (handler.go:647-648 into resolver.go:220), and container (internal/namespace/namespace.go:268, which answers 503). The issue stays open as the umbrella for them.

Related to #484

Context for LLM agents

Design rationale and rejected alternatives

  • Renaming storableTagName to a general name was rejected on a measurement. The caps gate puts any touched comment block under a cap, and the rename touches a 21-line exported doc at cap 3 and a 15-line test doc at cap 2. Delegation leaves both blocks untouched and costs one wrapper line.
  • Dropping the two unreachable packages.go screens was rejected. The reader helper is what stops a future call site reading the raw value at all, and a screen dead today goes live the moment a route resolves a format child without its parent.
  • A store-tier predicate on the four finders was rejected here and is the follow-up's shape. It would close all four surfaces at once, but those stores map their existing argument guards to 500 for caller wiring bugs, and a byte-shape sentinel needs the opposite mapping. The status decision belongs at the handler.
  • A length bound alongside the predicate was rejected, as it was on the precedent MR. strings.Contains plus utf8.ValidString measures 0.60 to 0.73 ms per MiB, allocation-free, against pgx's QuoteString at 1.02 to 1.12 ms per MiB plus a 2.1 MB allocation for the same value, so this branch creates no exposure a bound would remove.

Non-goals

  • The three client surfaces carrying the identical defect. The follow-up's shape is one exported predicate their resolvers share, and the container path answers 503 rather than 500, so a search for 500 misses it.
  • The description request-body field on create (create.go:243) and update (update.go:105). A JSON \u0000 escape decodes to a real NUL that reaches nullableStringExpr, so the unstorable-byte class is not closed for internal/managementapi. It sits outside this issue's path-value scope and puts a 400-versus-404 call on a different carrier.
  • A row in S17's Error Cases table. The status is settled by :501, :503, and :1618, and a table row is a spec MR's change.
Edited by Hayley Swimelar

Merge request reports

Loading
Loading