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. //