From 85f312f953561902283feb530ed2080361a5e5f7 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Wed, 2 Sep 2026 21:03:20 -0500 Subject: [PATCH] appview: refuse /v2/_catalog with UNSUPPORTED, and send Allow on every 405 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /v2/_catalog answered a bare request with 200 {"repositories":[]} but 400ed any n, because buildDistributionConfig leaves Catalog.MaxEntries at 0 and the library rejects n > max. crane catalog sends n=1000, so it always failed. The same endpoint both worked and rejected a legal parameter. Setting MaxEntries was the obvious fix and is the wrong one. This registry has no global catalog and will not grow one: repositories live in per-user ATProto PDS namespaces, and buildStorageConfig hands the library a placeholder inmemory driver, so its enumeration is empty by construction. An empty 200 asserts that this registry contains no repositories, which is false and silently so. UNSUPPORTED says the true thing, and matches the vocabulary DomainRoutingMiddleware already uses to refuse /v2/* on the UI domain. There is no conformance cost: _catalog is not in the OCI distribution spec at all. It is a Docker Registry HTTP API V2 extension, and the spec places repository discovery out of scope. Docker Hub and GHCR both refuse it outright and Quay returns an unconditional empty list; none of them 400s a legal n. The path had three distinct behaviours, not two, and all three now collapse to one: GET varied by parameter, HEAD was answered by gorilla's MethodHandler with a bare 405, and the trailing-slash form 301-redirected because distribution sets StrictSlash(true). Both path forms are registered, and chi prefers a static pattern over the /v2/* wildcard regardless of declaration order (verified against the pinned chi version, and pinned by a test that fails if a request reaches the distribution stand-in). Also adds the Allow header that RFC 9110 requires on any 405 — a MUST in both 15.5.6 and 10.2.1, not a SHOULD. The value is empty, which 10.2.1 defines as "the resource allows no methods": true for the catalog, and true for /v2/* on the UI domain, so the pre-existing gap in DomainRoutingMiddleware is closed too. Naming a method there would advertise something that does not work. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9 --- pkg/appview/catalog_test.go | 174 ++++++++++++++++++++++++++++++++++++ pkg/appview/server.go | 67 ++++++++++++++ 2 files changed, 241 insertions(+) create mode 100644 pkg/appview/catalog_test.go diff --git a/pkg/appview/catalog_test.go b/pkg/appview/catalog_test.go new file mode 100644 index 0000000..7d151ce --- /dev/null +++ b/pkg/appview/catalog_test.go @@ -0,0 +1,174 @@ +package appview + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-chi/chi/v5" +) + +// catalogRouter wires the production registration, mountCatalogRefusal, over a +// stand-in for the distribution app, so the test can tell an intercepted request +// from one that reached the library. Calling the real helper rather than +// restating its routes keeps the test from drifting away from NewServer. +func catalogRouter() (chi.Router, *bool) { + reachedDistribution := false + r := chi.NewRouter() + // Registered first on purpose: chi must prefer the static patterns below + // even when the wildcard is declared ahead of them. + r.Handle("/v2/*", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + reachedDistribution = true + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"repositories":[]}`)) + })) + mountCatalogRefusal(r) + return r, &reachedDistribution +} + +// TestCatalogUnsupported pins the contract: /v2/_catalog answers UNSUPPORTED +// with 405 for every parameter shape, so no query string can change the answer. +func TestCatalogUnsupported(t *testing.T) { + paths := []string{ + "/v2/_catalog", + "/v2/_catalog?n=1", + "/v2/_catalog?n=1000", + "/v2/_catalog?n=0", + "/v2/_catalog?n=-1", + "/v2/_catalog?n=abc", + "/v2/_catalog?last=foo", + "/v2/_catalog?n=1000&last=foo", + "/v2/_catalog/", + "/v2/_catalog/?n=1000", + } + methods := []string{http.MethodGet, http.MethodHead, http.MethodPost, http.MethodDelete} + + for _, path := range paths { + for _, method := range methods { + t.Run(method+" "+path, func(t *testing.T) { + router, reached := catalogRouter() + rec := httptest.NewRecorder() + router.ServeHTTP(rec, httptest.NewRequest(method, path, nil)) + + if *reached { + t.Fatalf("request reached the distribution handler, chi routed %q to the wildcard", path) + } + if rec.Code != http.StatusMethodNotAllowed { + t.Errorf("status = %d, want %d", rec.Code, http.StatusMethodNotAllowed) + } + if ct := rec.Header().Get("Content-Type"); ct != "application/json" { + t.Errorf("Content-Type = %q, want application/json", ct) + } + + var body struct { + Errors []struct { + Code string `json:"code"` + Message string `json:"message"` + } `json:"errors"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode body %q: %v", rec.Body.String(), err) + } + if len(body.Errors) != 1 { + t.Fatalf("got %d errors, want 1: %s", len(body.Errors), rec.Body.String()) + } + if body.Errors[0].Code != "UNSUPPORTED" { + t.Errorf("code = %q, want UNSUPPORTED", body.Errors[0].Code) + } + if body.Errors[0].Message == "" { + t.Error("message is empty, it should explain why there is no catalog") + } + }) + } + } +} + +// TestCatalogResponseIsIdentical asserts the stronger property: not merely that +// every form is UNSUPPORTED, but that every form produces byte-identical output, +// so no parameter is left able to influence the answer. +func TestCatalogResponseIsIdentical(t *testing.T) { + paths := []string{ + "/v2/_catalog", + "/v2/_catalog?n=1", + "/v2/_catalog?n=1000", + "/v2/_catalog?n=0", + "/v2/_catalog?n=-1", + "/v2/_catalog?n=abc", + "/v2/_catalog?last=foo", + "/v2/_catalog/", + } + + var wantCode int + var wantBody string + for i, path := range paths { + router, _ := catalogRouter() + rec := httptest.NewRecorder() + router.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil)) + if i == 0 { + wantCode, wantBody = rec.Code, rec.Body.String() + continue + } + if rec.Code != wantCode || rec.Body.String() != wantBody { + t.Errorf("%s returned %d %q, want %d %q", path, rec.Code, rec.Body.String(), wantCode, wantBody) + } + } +} + +// TestCatalogDoesNotShadowRepositories guards the interception's blast radius: +// real repository paths must still reach the distribution handler. +func TestCatalogDoesNotShadowRepositories(t *testing.T) { + paths := []string{ + "/v2/", + "/v2/alice.example.com/nginx/tags/list", + "/v2/alice.example.com/_catalog/manifests/latest", + "/v2/_catalogue/nginx/tags/list", + } + for _, path := range paths { + t.Run(path, func(t *testing.T) { + router, reached := catalogRouter() + rec := httptest.NewRecorder() + router.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil)) + if !*reached { + t.Errorf("%s was intercepted, it should reach the distribution handler (got %d %q)", path, rec.Code, rec.Body.String()) + } + }) + } +} + +// TestCatalogSends405Allow pins the Allow header RFC 9110 requires on a 405. +// +// 15.5.6 and 10.2.1 both state it as a MUST: "An origin server MUST generate an +// Allow header field in a 405 (Method Not Allowed) response." UNSUPPORTED maps +// to 405, so the header has to be there. The value is empty on purpose — +// 10.2.1: "An empty Allow field value indicates that the resource allows no +// methods" — which is the truth for a catalog that does not exist. +// +// Asserted via the header map rather than the raw wire because Go writes an +// empty field value as "Allow: " and a client reads it back as present with an +// empty string, which is the distinction that matters: present-and-empty is +// compliant, absent is not. +func TestCatalogSends405Allow(t *testing.T) { + r, _ := catalogRouter() + + for _, method := range []string{http.MethodGet, http.MethodHead, http.MethodPost} { + for _, path := range []string{"/v2/_catalog", "/v2/_catalog/", "/v2/_catalog?n=1000"} { + req := httptest.NewRequest(method, path, nil) + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusMethodNotAllowed { + t.Errorf("%s %s: status = %d, want %d", method, path, rec.Code, http.StatusMethodNotAllowed) + } + values, present := rec.Result().Header["Allow"] + if !present { + t.Errorf("%s %s: no Allow header on a 405; RFC 9110 15.5.6 makes it a MUST", method, path) + continue + } + if len(values) != 1 || values[0] != "" { + t.Errorf("%s %s: Allow = %q, want exactly one empty value (RFC 9110 10.2.1: the resource allows no methods)", + method, path, values) + } + } + } +} diff --git a/pkg/appview/server.go b/pkg/appview/server.go index dd9eb21..f14a24d 100644 --- a/pkg/appview/server.go +++ b/pkg/appview/server.go @@ -559,6 +559,9 @@ func NewAppViewServer(cfg *Config, branding *BrandingOverrides) (*AppViewServer, wrappedApp := middleware.BearerChallenge(cfg.Server.BaseURL+"/auth/token", cfg.Auth.Services)( middleware.RetryAfterMiddleware(middleware.ExtractAuthMethod(app))) + // Refuse the global catalog before the distribution handler can see it. + mountCatalogRefusal(mainRouter) + // Mount registry at /v2/ mainRouter.Handle("/v2/*", wrappedApp) @@ -846,6 +849,7 @@ func DomainRoutingMiddleware(registryDomains []string, uiBaseURL string) func(ht case host == uiHost: // UI domain: block /v2/*, serve everything else if isV2 { + writeEmptyAllow(w) if err := errcode.ServeJSON(w, errcode.ErrorCodeUnsupported.WithMessage( fmt.Sprintf("registry API is not available on this domain, use %s", primaryReg), )); err != nil { @@ -1150,6 +1154,69 @@ var gzipUI = func() func(http.Handler) http.HandlerFunc { return wrapper }() +// mountCatalogRefusal registers the /v2/_catalog refusal on r. It must be +// called on the same router that mounts "/v2/*": chi prefers a static pattern +// over a wildcard, so these two registrations win regardless of declaration +// order. The trailing-slash form is spelled out because the distribution router +// runs with StrictSlash(true) and would otherwise answer it with a 301 to the +// bare path, costing a round trip to reach the same refusal. +// +// Handle registers every method, so HEAD and anything else land here too. +func mountCatalogRefusal(r chi.Router) { + r.Handle("/v2/_catalog", http.HandlerFunc(handleCatalogUnsupported)) + r.Handle("/v2/_catalog/", http.HandlerFunc(handleCatalogUnsupported)) +} + +// handleCatalogUnsupported answers GET /v2/_catalog (and every other method and +// query string on that path) with an OCI UNSUPPORTED error, HTTP 405. +// +// This registry has no global catalog and will not grow one. Repositories live +// in per-user ATProto PDS namespaces, not in a single enumerable store, so +// there is nothing for the distribution library to walk: buildStorageConfig() +// hands it a placeholder inmemory driver, which makes its enumeration empty by +// construction. Left to itself the library answers a bare request with +// 200 {"repositories":[]} and 400s any n (Catalog.MaxEntries is 0), so the same +// endpoint both "works" and rejects a legal parameter. Worse, the empty 200 +// asserts that this registry contains no repositories, which is false and +// silently so. UNSUPPORTED says the true thing. +// +// There is no conformance cost. _catalog is not in the OCI distribution spec at +// all: it is a Docker Registry HTTP API V2 extension, and the OCI spec places +// repository discovery out of scope. Docker Hub and GHCR refuse _catalog +// outright; Quay returns an unconditional empty list. None of them 400s a legal +// n. Fixing this by setting Catalog.MaxEntries was considered and rejected: it +// would make the lie coherent rather than remove it. +// +// HEAD gets the same status and headers, with the body suppressed by net/http. +func handleCatalogUnsupported(w http.ResponseWriter, _ *http.Request) { + writeEmptyAllow(w) + if err := errcode.ServeJSON(w, errcode.ErrorCodeUnsupported.WithMessage( + "this registry does not offer a global catalog, repositories are namespaced per user: list tags for a known repository at /v2/{handle-or-did}/{image}/tags/list", + )); err != nil { + slog.Error("failed to write OCI error response", "error", err) + } +} + +// writeEmptyAllow sets the Allow header required on a 405 response. +// +// RFC 9110 makes this a MUST, in both 15.5.6 and 10.2.1: "An origin server MUST +// generate an Allow header field in a 405 (Method Not Allowed) response." The +// OCI UNSUPPORTED error code maps to 405 (its descriptor in the distribution +// library sets HTTPStatusCode: http.StatusMethodNotAllowed), so every place we +// serve UNSUPPORTED owes the header. +// +// The value is deliberately empty. 10.2.1: "An empty Allow field value +// indicates that the resource allows no methods." That is precisely the case at +// both call sites — the global catalog does not exist and never will, and the +// registry API is not served on the UI domain under any method — so naming a +// method here would advertise something that does not work. Go writes the field +// as "Allow: " and clients read it back as present-and-empty. +// +// Must be called before ServeJSON, which commits the status line. +func writeEmptyAllow(w http.ResponseWriter) { + w.Header().Set("Allow", "") +} + // compressUIResponses gzips UI, static and JSON responses and leaves the OCI // registry API at /v2/* alone. //