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.goscreens are unreachable on every route that exists today, because every route reaching them resolves the parent throughfindRepositoryForRequestfirst. They stay as second-line defense, and the white-boxTestChildResolves_UnstorableRepositoryNameIs404WithoutReadis the only thing pinning them: a route-driven test passes with both deleted. storableTagNamedelegates to the shared predicate instead of being renamed. The rename failsscripts/ci/check-comment-caps.shon two blocks,container_tag_delete.goat 21 lines against an exported-doc cap of 3 andcontainer_tag_delete_test.goat 15 against a_test.gocap 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/urlrejects 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 pina%01banda%7Fbas accepted and forwarded verbatim. detail.go:80is a secondFindByNamebind and is exempt, because itsrow.Namecame from the rowfindRepositoryForRequesthad just read.- The Error Cases table has no row for this class, so the status extends
:501and:503by analogy alongside the rows at:1403and:1412. One asymmetry is worth naming::1408and:1420answer400for 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.FindByNameas 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
storableTagNameto 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.goscreens 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.Containsplusutf8.ValidStringmeasures 0.60 to 0.73 ms per MiB, allocation-free, against pgx'sQuoteStringat 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
descriptionrequest-body field on create (create.go:243) and update (update.go:105). A JSON\u0000escape decodes to a real NUL that reachesnullableStringExpr, so the unstorable-byte class is not closed forinternal/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.