From 8760323aaca9483d56763c70715534b93b99f084 Mon Sep 17 00:00:00 2001 From: Steven Miller Date: Tue, 27 Jan 2026 13:32:41 -0500 Subject: [PATCH] Fix image parsing in middleware --- cmd/api/main_test.go | 89 ++++++++++++++++++++++++++++++++ lib/middleware/resolve.go | 11 ++++ lib/middleware/resolve_test.go | 94 ++++++++++++++++++++++++++++++++++ 3 files changed, 194 insertions(+) create mode 100644 lib/middleware/resolve_test.go diff --git a/cmd/api/main_test.go b/cmd/api/main_test.go index c461bd3..0975852 100644 --- a/cmd/api/main_test.go +++ b/cmd/api/main_test.go @@ -136,6 +136,95 @@ func TestOapiRuntimeBindStyledParameter_URLDecoding(t *testing.T) { } } +func TestGetImage_URLEncodedSlashes(t *testing.T) { + // This test reproduces the bug where URL-encoded image names are not properly + // decoded before being passed to the image reference parser. + // + // Bug: curl "https://server/images/docker.io%2Flibrary%2Fnginx:alpine" + // Returns: {"code":"invalid_name","message":"invalid image reference"} + // Expected: 200 with image details (or 404 if not found) + // + // The server passes "docker.io%2Flibrary%2Fnginx:alpine" (still encoded) + // to the parser instead of "docker.io/library/nginx:alpine" (decoded). + + r := chi.NewRouter() + + // Track what name the handler receives through the oapi wrapper + var receivedName string + + // Create a handler that implements oapi.ServerInterface + handler := &testImageHandler{ + getImage: func(w http.ResponseWriter, r *http.Request, name string) { + receivedName = name + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + w.Write([]byte(`{"code":"not_found","message":"image not found"}`)) + }, + } + + // Mount using oapi's HandlerFromMux which uses the generated wrappers + // This tests the full path: chi.URLParam -> runtime.BindStyledParameterWithOptions -> handler + oapi.HandlerFromMux(handler, r) + + token, err := generateValidJWT("user-123") + require.NoError(t, err) + + tests := []struct { + name string + path string + expectedName string + }{ + { + name: "simple image name", + path: "/images/alpine:latest", + expectedName: "alpine:latest", + }, + { + name: "URL-encoded slashes should be decoded", + path: "/images/docker.io%2Flibrary%2Fnginx%3Aalpine", + expectedName: "docker.io/library/nginx:alpine", // Must be decoded! + }, + { + name: "URL-encoded with colon", + path: "/images/docker.io%2Flibrary%2Falpine%3Alatest", + expectedName: "docker.io/library/alpine:latest", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + receivedName = "" // Reset + + req := httptest.NewRequest(http.MethodGet, tt.path, nil) + req.Header.Set("Authorization", "Bearer "+token) + + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + // We expect 404 (image not found) - that's fine, we're testing the name decoding + // NOT 400 (invalid_name) which would indicate the encoded string wasn't decoded + assert.NotEqual(t, http.StatusBadRequest, w.Code, + "Got 400 Bad Request - URL-encoded name was likely not decoded. Path: %s, Body: %s", + tt.path, w.Body.String()) + + assert.Equal(t, tt.expectedName, receivedName, + "Handler received wrong name - URL decoding may have failed") + }) + } +} + +// testImageHandler implements oapi.ServerInterface with just GetImage for testing +type testImageHandler struct { + oapi.Unimplemented + getImage func(w http.ResponseWriter, r *http.Request, name string) +} + +func (h *testImageHandler) GetImage(w http.ResponseWriter, r *http.Request, name string) { + if h.getImage != nil { + h.getImage(w, r, name) + } +} + func TestImageNameWithSlashes_URLEncoding(t *testing.T) { // This test verifies how chi router handles image names with slashes. // Image names like "docker.io/onkernel/chromium-headful:latest" contain slashes diff --git a/lib/middleware/resolve.go b/lib/middleware/resolve.go index 028dcad..a722da6 100644 --- a/lib/middleware/resolve.go +++ b/lib/middleware/resolve.go @@ -4,6 +4,7 @@ package middleware import ( "context" "net/http" + "net/url" "strings" "github.com/go-chi/chi/v5" @@ -96,6 +97,8 @@ func ResolveResource(resolvers Resolvers, errResponder ErrorResponder) func(http } // Get the ID parameter from the URL + // chi.URLParam returns the raw URL-encoded value, so we must decode it. + // For example, "docker.io%2Flibrary%2Falpine" -> "docker.io/library/alpine" idOrName := chi.URLParam(r, paramName) if idOrName == "" { // No ID in path (e.g., list or create endpoint) @@ -103,6 +106,14 @@ func ResolveResource(resolvers Resolvers, errResponder ErrorResponder) func(http return } + // URL-decode the parameter (handles %2F -> /, %3A -> :, etc.) + decoded, err := url.PathUnescape(idOrName) + if err != nil { + // If decoding fails, use the original value (should be rare) + decoded = idOrName + } + idOrName = decoded + // Resolve the resource resolvedID, resource, err := resolver.Resolve(ctx, idOrName) if err != nil { diff --git a/lib/middleware/resolve_test.go b/lib/middleware/resolve_test.go new file mode 100644 index 0000000..8b2435f --- /dev/null +++ b/lib/middleware/resolve_test.go @@ -0,0 +1,94 @@ +package middleware + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// mockResolver records what name was passed to Resolve +type mockResolver struct { + receivedName string + returnErr error +} + +func (m *mockResolver) Resolve(ctx context.Context, idOrName string) (string, any, error) { + m.receivedName = idOrName + if m.returnErr != nil { + return "", nil, m.returnErr + } + return idOrName, struct{}{}, nil +} + +func TestResolveResource_URLDecodesImageName(t *testing.T) { + // This test reproduces the bug where URL-encoded image names are not + // properly decoded before being passed to the resolver. + // + // Bug: curl "https://server/images/docker.io%2Flibrary%2Fnginx:alpine" + // The resolver receives "docker.io%2Flibrary%2Fnginx:alpine" (still encoded) + // instead of "docker.io/library/nginx:alpine" (decoded). + + tests := []struct { + name string + path string + expectedName string + }{ + { + name: "simple image name", + path: "/images/alpine:latest", + expectedName: "alpine:latest", + }, + { + name: "URL-encoded slashes must be decoded", + path: "/images/docker.io%2Flibrary%2Fnginx%3Aalpine", + expectedName: "docker.io/library/nginx:alpine", // Must be decoded! + }, + { + name: "URL-encoded with colon", + path: "/images/docker.io%2Flibrary%2Falpine%3Alatest", + expectedName: "docker.io/library/alpine:latest", + }, + { + name: "nested repo URL-encoded", + path: "/images/docker.io%2Fonkernel%2Fchromium-headful%3Alatest", + expectedName: "docker.io/onkernel/chromium-headful:latest", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resolver := &mockResolver{} + + errResponder := func(w http.ResponseWriter, err error, lookup string) { + w.WriteHeader(http.StatusBadRequest) + w.Write([]byte(`{"error":"` + err.Error() + `"}`)) + } + + middleware := ResolveResource(Resolvers{ + Image: resolver, + }, errResponder) + + // Create a chi router to properly parse URL params + r := chi.NewRouter() + r.With(middleware).Get("/images/{name}", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + req := httptest.NewRequest(http.MethodGet, tt.path, nil) + w := httptest.NewRecorder() + + r.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code, + "Expected 200 OK, got %d. Response: %s", w.Code, w.Body.String()) + + assert.Equal(t, tt.expectedName, resolver.receivedName, + "Resolver received wrong name - URL decoding may have failed") + }) + } +}