From 5a2aa946914ec50170da727307318b3ed68d3246 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 26 Oct 2026 09:34:37 +0000 Subject: [PATCH] Remove the code nothing in production calls (closes #312) Service.Warm, the ImageCache interface, both GenerateSignedURL methods, signature.ParseParams, imagerequest.ParseImagePath, four symbols of internal/magic, the image processor's lists of supported formats, IsEmpty and Count of the allowlist, ssrfSafeDialer, CacheStale, ErrCacheMiss, ErrNegativeCache, ImageResponse.LastModified and the Load methods of ContentStorage and VariantStorage had no caller outside tests. They are gone, with the tests that tested only them. The handlers tests sign their image URLs with generateSignedURL, a test helper that builds the URL as the removed generator did. The SSRF dialer tests call dialSSRFSafe with no operator blocklist, which is all ssrfSafeDialer did. The tests of ParseImagePath and ContentStorage.Load call ParseImageURL and LoadWithSize instead. MIMEToImageFormat, ContentStorage.Store and CleanExpired stay, as pixa calls them. Model: opus-5-5 --- TODO.md | 16 ++ internal/allowlist/allowlist.go | 10 - internal/allowlist/allowlist_test.go | 84 ------- .../handlers/format_auto_internal_test.go | 16 +- internal/handlers/handlers_internal_test.go | 2 - .../image_cache_control_internal_test.go | 7 +- .../handlers/image_signature_internal_test.go | 146 +++++++----- internal/handlers/request_id_internal_test.go | 13 +- internal/httpfetcher/fetch_internal_test.go | 8 +- internal/httpfetcher/httpfetcher.go | 7 - internal/imageprocessor/imageprocessor.go | 24 -- .../imageprocessor_internal_test.go | 16 -- internal/imagerequest/urlparser.go | 17 -- .../imagerequest/urlparser_internal_test.go | 120 ++-------- internal/imgcache/cache.go | 6 - internal/imgcache/imgcache.go | 28 +-- .../partial_cache_files_internal_test.go | 4 +- internal/imgcache/service.go | 34 +-- internal/imgcache/storage.go | 32 --- internal/imgcache/storage_internal_test.go | 14 +- internal/magic/jpegxl_internal_test.go | 11 +- internal/magic/magic.go | 77 +------ internal/magic/magic_internal_test.go | 176 --------------- internal/server/source_url_internal_test.go | 33 +-- internal/signature/golden_test.go | 36 +-- internal/signature/signature.go | 54 ----- internal/signature/signature_test.go | 213 +----------------- 27 files changed, 187 insertions(+), 1017 deletions(-) diff --git a/TODO.md b/TODO.md index a2785f7..8c2ff37 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,22 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-26 the code nothing in production called is gone (closes #312). The + handlers tests sign their image URLs with `generateSignedURL`, a test helper + that writes them as the removed generator did. Removed: + - `Service.Warm` and the `ImageCache` interface + - `Service.GenerateSignedURL` and `Signer.GenerateSignedURL` + - `signature.ParseParams` + - `imagerequest.ParseImagePath` + - `PeekAndValidate`, `IsSupportedMIMEType`, `ImageFormatToMIME` and + `FormatOriginal` of `internal/magic` + - `SupportedInputFormats` and `SupportedOutputFormats` of the image + processor + - `IsEmpty` and `Count` of the allowlist + - `ssrfSafeDialer` + - `CacheStale`, `ErrCacheMiss`, `ErrNegativeCache` and + `ImageResponse.LastModified` + - `ContentStorage.Load` and `VariantStorage.Load` - 2026-10-26 cache hits, misses, upstream fetches, the bytes those fetches read and transforms are counted as Prometheus counters in memory (closes #311). They are `pixa_cache_hits_total`, `pixa_cache_misses_total`, diff --git a/internal/allowlist/allowlist.go b/internal/allowlist/allowlist.go index 6dc2c4b..256c764 100644 --- a/internal/allowlist/allowlist.go +++ b/internal/allowlist/allowlist.go @@ -72,13 +72,3 @@ func (w *HostAllowList) IsAllowed(u *url.URL) bool { return false } - -// IsEmpty returns true if the allow list has no entries. -func (w *HostAllowList) IsEmpty() bool { - return len(w.exactHosts) == 0 && len(w.suffixHosts) == 0 -} - -// Count returns the total number of allow list entries. -func (w *HostAllowList) Count() int { - return len(w.exactHosts) + len(w.suffixHosts) -} diff --git a/internal/allowlist/allowlist_test.go b/internal/allowlist/allowlist_test.go index 1d68a14..a1730b9 100644 --- a/internal/allowlist/allowlist_test.go +++ b/internal/allowlist/allowlist_test.go @@ -139,87 +139,3 @@ func TestHostAllowList_IsAllowed_SuffixMatch(t *testing.T) { }, }) } - -func TestHostAllowList_IsEmpty(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - patterns []string - want bool - }{ - { - name: "empty", - patterns: []string{}, - want: true, - }, - { - name: "nil", - patterns: nil, - want: true, - }, - { - name: "whitespace only", - patterns: []string{" ", ""}, - want: true, - }, - { - name: "has entries", - patterns: []string{"example.com"}, - want: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - w := allowlist.New(tt.patterns) - if got := w.IsEmpty(); got != tt.want { - t.Errorf("IsEmpty() = %v, want %v", got, tt.want) - } - }) - } -} - -func TestHostAllowList_Count(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - patterns []string - want int - }{ - { - name: "empty", - patterns: []string{}, - want: 0, - }, - { - name: "exact hosts only", - patterns: []string{"a.com", "b.com", "c.com"}, - want: 3, - }, - { - name: "suffix hosts only", - patterns: []string{".a.com", ".b.com"}, - want: 2, - }, - { - name: "mixed", - patterns: []string{"exact.com", ".suffix.com"}, - want: 2, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - w := allowlist.New(tt.patterns) - if got := w.Count(); got != tt.want { - t.Errorf("Count() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/internal/handlers/format_auto_internal_test.go b/internal/handlers/format_auto_internal_test.go index d899b36..c543902 100644 --- a/internal/handlers/format_auto_internal_test.go +++ b/internal/handlers/format_auto_internal_test.go @@ -75,11 +75,13 @@ func TestFormatForAccept(t *testing.T) { // autoPhotoURLs returns a signed /v1/image/ URL and an encrypted /v1/e/ URL, // both valid for a minute, for the JPEG at photoPath on signedHost at 50x50 in -// the format auto, made with h's image service and generator. +// the format auto. The first is signed with testSigningKey, the key of the +// image service newSignedHostServer builds, and the second is made with h's +// generator. func autoPhotoURLs(t *testing.T, h *Handlers) (string, string) { t.Helper() - signedURL, err := h.imgSvc.GenerateSignedURL("", &imagerequest.Request{ + signedURL := generateSignedURL(testSigningKey, &imagerequest.Request{ Source: imagerequest.Source{ Host: signedHost, Path: photoPath, @@ -89,9 +91,6 @@ func autoPhotoURLs(t *testing.T, h *Handlers) (string, string) { Format: imagerequest.FormatAuto, }, }, time.Minute) - if err != nil { - t.Fatalf("GenerateSignedURL() error = %v", err) - } token, err := h.encGen.Generate(&imagerequest.Request{ Source: imagerequest.Source{ @@ -188,9 +187,9 @@ func TestFormatAuto_ChosenFromAccept(t *testing.T) { func TestFormatAuto_SignatureCoversAuto(t *testing.T) { t.Parallel() - h, srv := newSignedHostServer(t, slog.New(slog.DiscardHandler)) + _, srv := newSignedHostServer(t, slog.New(slog.DiscardHandler)) - signedForAVIF, err := h.imgSvc.GenerateSignedURL("", &imagerequest.Request{ + signedForAVIF := generateSignedURL(testSigningKey, &imagerequest.Request{ Source: imagerequest.Source{ Host: signedHost, Path: photoPath, @@ -200,9 +199,6 @@ func TestFormatAuto_SignatureCoversAuto(t *testing.T) { Format: imagerequest.FormatAVIF, }, }, time.Minute) - if err != nil { - t.Fatalf("GenerateSignedURL() error = %v", err) - } target := strings.Replace(signedForAVIF, "/50x50.avif?", "/50x50.auto?", 1) if target == signedForAVIF { diff --git a/internal/handlers/handlers_internal_test.go b/internal/handlers/handlers_internal_test.go index 82957fb..d175569 100644 --- a/internal/handlers/handlers_internal_test.go +++ b/internal/handlers/handlers_internal_test.go @@ -27,7 +27,6 @@ import ( // testFixtures contains test data. type testFixtures struct { handler *Handlers - service *imgcache.Service goodHost string } @@ -71,7 +70,6 @@ func setupTestHandler(t *testing.T) *testFixtures { return &testFixtures{ handler: h, - service: svc, goodHost: goodHost, } } diff --git a/internal/handlers/image_cache_control_internal_test.go b/internal/handlers/image_cache_control_internal_test.go index 0fc236f..79c4a38 100644 --- a/internal/handlers/image_cache_control_internal_test.go +++ b/internal/handlers/image_cache_control_internal_test.go @@ -107,9 +107,9 @@ func getMaxAge(t *testing.T, srv http.Handler, target string) int { func TestHandleImage_SignedURL_MaxAgeEndsAtExp(t *testing.T) { t.Parallel() - h, srv := newSignedHostServer(t, slog.New(slog.DiscardHandler)) + _, srv := newSignedHostServer(t, slog.New(slog.DiscardHandler)) - signedURL, err := h.imgSvc.GenerateSignedURL("", &imagerequest.Request{ + signedURL := generateSignedURL(testSigningKey, &imagerequest.Request{ Source: imagerequest.Source{ Host: signedHost, Path: photoPath, @@ -119,9 +119,6 @@ func TestHandleImage_SignedURL_MaxAgeEndsAtExp(t *testing.T) { Format: imagerequest.FormatJPEG, }, }, time.Minute) - if err != nil { - t.Fatalf("GenerateSignedURL() error = %v", err) - } maxAge := getMaxAge(t, srv, signedURL) if maxAge < 50 || maxAge > 60 { diff --git a/internal/handlers/image_signature_internal_test.go b/internal/handlers/image_signature_internal_test.go index 2849565..3d2b766 100644 --- a/internal/handlers/image_signature_internal_test.go +++ b/internal/handlers/image_signature_internal_test.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "net/url" "testing" "time" @@ -36,6 +37,43 @@ func getImage(t *testing.T, fix *testFixtures, target string) int { return rec.Code } +// generateSignedURL returns the /v1/image/ URL of req, signed with key and +// expiring after ttl, and sets req's expiry. An unset quality or fit of req is +// first set to 85 or cover, the values the route uses when a URL has no q or +// fit. The URL carries q and fit next to sig and exp, so the route verifies it +// for the quality and fit it was signed with. +func generateSignedURL( + key string, req *imagerequest.Request, ttl time.Duration, +) string { + if req.Transform.Quality == 0 { + req.Transform.Quality = 85 + } + + if req.Transform.FitMode == "" { + req.Transform.FitMode = imagerequest.FitCover + } + + req.Expires = time.Now().Add(ttl) + sig := signature.New(key).Sign(req) + + // A source query follows the source path after a "?". The image route + // reads the path decoded once, so the whole path is written in Go's own + // percent-encoding of a URL path, which writes "?" as %3F and "%" as %25, + // and leaves "/", ";", ",", "=" and "&" as they are. + sourcePathAndQuery := req.Source.Path + if req.Source.Query != "" { + sourcePathAndQuery += "?" + req.Source.Query + } + + unescapedPath := fmt.Sprintf("/v1/image/%s%s/%dx%d.%s", + req.Source.Host, sourcePathAndQuery, + req.Transform.Size.Width, req.Transform.Size.Height, req.Transform.Format) + + return fmt.Sprintf("%s?sig=%s&exp=%d&q=%d&fit=%s", + (&url.URL{Path: unescapedPath}).EscapedPath(), sig, req.Expires.Unix(), + req.Transform.Quality, req.Transform.FitMode) +} + // TestHandleImage_SignatureCoversQualityAndFit signs a URL for quality 85 // and fit cover, the values the route uses when a URL has no q or fit, and // sends that signature with each q and fit below. @@ -83,8 +121,8 @@ func TestHandleImage_SignatureCoversQualityAndFit(t *testing.T) { } } -// TestHandleImage_GeneratedSignedURLVerifies sends URLs built by the -// service's signed-URL generator to the route. +// TestHandleImage_GeneratedSignedURLVerifies sends URLs built by +// generateSignedURL to the route. func TestHandleImage_GeneratedSignedURLVerifies(t *testing.T) { t.Parallel() @@ -101,25 +139,21 @@ func TestHandleImage_GeneratedSignedURLVerifies(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - fix := setupTestHandler(t) + signedURL := generateSignedURL("test-signing-key-must-be-32-chars", + &imagerequest.Request{ + Source: imagerequest.Source{ + Host: signedHost, + Path: "/images/photo.jpg", + }, + Transform: imagerequest.Transform{ + Size: imagerequest.Size{Width: 50, Height: 50}, + Format: imagerequest.FormatJPEG, + Quality: tt.quality, + FitMode: tt.fitMode, + }, + }, time.Hour) - signedURL, err := fix.service.GenerateSignedURL("", &imagerequest.Request{ - Source: imagerequest.Source{ - Host: signedHost, - Path: "/images/photo.jpg", - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 50, Height: 50}, - Format: imagerequest.FormatJPEG, - Quality: tt.quality, - FitMode: tt.fitMode, - }, - }, time.Hour) - if err != nil { - t.Fatalf("GenerateSignedURL() error = %v", err) - } - - status := getImage(t, fix, signedURL) + status := getImage(t, setupTestHandler(t), signedURL) if status != http.StatusBadGateway { t.Errorf("status = %d, want %d", status, http.StatusBadGateway) } @@ -178,10 +212,10 @@ func TestHandleImage_InvalidExp_Returns400(t *testing.T) { } // TestHandleImage_GeneratedSignedURLWithSourceQueryVerifies sends URLs built -// by the service's signed-URL generator for source URLs with a query to the -// route, through a chi router: chi routes on the path as the client wrote it -// whenever that is not Go's own encoding of it. A 502 means the signature -// passed, so the route read the source path and query that were signed. +// by generateSignedURL for source URLs with a query to the route, through a +// chi router: chi routes on the path as the client wrote it whenever that is +// not Go's own encoding of it. A 502 means the signature passed, so the route +// read the source path and query that were signed. func TestHandleImage_GeneratedSignedURLWithSourceQueryVerifies(t *testing.T) { t.Parallel() @@ -201,24 +235,20 @@ func TestHandleImage_GeneratedSignedURLWithSourceQueryVerifies(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - fix := setupTestHandler(t) + signedURL := generateSignedURL("test-signing-key-must-be-32-chars", + &imagerequest.Request{ + Source: imagerequest.Source{ + Host: signedHost, + Path: photoPath, + Query: tt.query, + }, + Transform: imagerequest.Transform{ + Size: imagerequest.Size{Width: 50, Height: 50}, + Format: imagerequest.FormatJPEG, + }, + }, time.Hour) - signedURL, err := fix.service.GenerateSignedURL("", &imagerequest.Request{ - Source: imagerequest.Source{ - Host: signedHost, - Path: photoPath, - Query: tt.query, - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 50, Height: 50}, - Format: imagerequest.FormatJPEG, - }, - }, time.Hour) - if err != nil { - t.Fatalf("GenerateSignedURL() error = %v", err) - } - - status := getImage(t, fix, signedURL) + status := getImage(t, setupTestHandler(t), signedURL) if status != http.StatusBadGateway { t.Errorf("status = %d, want %d", status, http.StatusBadGateway) } @@ -280,7 +310,7 @@ func TestHandleImage_SourceQueryEncodedAnyWay(t *testing.T) { // TestHandleImage_SourceURLHoldingPercent25Verifies signs a source path and a // source query that each hold %25, the percent-escape of a literal "%", and // sends each to the route written as the README says, with that "%" written as -// %25, and as GenerateSignedURL writes it. A 502 means the signature passed. +// %25, and as generateSignedURL writes it. A 502 means the signature passed. func TestHandleImage_SourceURLHoldingPercent25Verifies(t *testing.T) { t.Parallel() @@ -326,27 +356,23 @@ func TestHandleImage_SourceURLHoldingPercent25Verifies(t *testing.T) { } }) - t.Run(tt.name+" written by GenerateSignedURL", func(t *testing.T) { + t.Run(tt.name+" written by generateSignedURL", func(t *testing.T) { t.Parallel() - fix := setupTestHandler(t) + signedURL := generateSignedURL("test-signing-key-must-be-32-chars", + &imagerequest.Request{ + Source: imagerequest.Source{ + Host: signedHost, + Path: tt.sourcePath, + Query: tt.sourceQuery, + }, + Transform: imagerequest.Transform{ + Size: imagerequest.Size{Width: 50, Height: 50}, + Format: imagerequest.FormatJPEG, + }, + }, time.Hour) - signedURL, err := fix.service.GenerateSignedURL("", &imagerequest.Request{ - Source: imagerequest.Source{ - Host: signedHost, - Path: tt.sourcePath, - Query: tt.sourceQuery, - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 50, Height: 50}, - Format: imagerequest.FormatJPEG, - }, - }, time.Hour) - if err != nil { - t.Fatalf("GenerateSignedURL() error = %v", err) - } - - status := getImage(t, fix, signedURL) + status := getImage(t, setupTestHandler(t), signedURL) if status != http.StatusBadGateway { t.Errorf("status = %d, want %d", status, http.StatusBadGateway) } diff --git a/internal/handlers/request_id_internal_test.go b/internal/handlers/request_id_internal_test.go index a785923..110d45c 100644 --- a/internal/handlers/request_id_internal_test.go +++ b/internal/handlers/request_id_internal_test.go @@ -17,11 +17,13 @@ import ( ) // signedPhotoURL returns a signed /v1/image/ URL, valid for a minute, for the -// JPEG at photoPath on signedHost at 50x50, made with h's image service. -func signedPhotoURL(t *testing.T, h *Handlers) string { +// JPEG at photoPath on signedHost at 50x50. It is signed with testSigningKey, +// the key of the image service newSignedHostServer builds. It takes a Handlers +// it does not use, so that it has the type of encPhotoURL. +func signedPhotoURL(t *testing.T, _ *Handlers) string { t.Helper() - signedURL, err := h.imgSvc.GenerateSignedURL("", &imagerequest.Request{ + return generateSignedURL(testSigningKey, &imagerequest.Request{ Source: imagerequest.Source{ Host: signedHost, Path: photoPath, @@ -31,11 +33,6 @@ func signedPhotoURL(t *testing.T, h *Handlers) string { Format: imagerequest.FormatJPEG, }, }, time.Minute) - if err != nil { - t.Fatalf("GenerateSignedURL() error = %v", err) - } - - return signedURL } // encPhotoURL returns an encrypted /v1/e/ URL, which never expires, for the diff --git a/internal/httpfetcher/fetch_internal_test.go b/internal/httpfetcher/fetch_internal_test.go index c443987..b6523a8 100644 --- a/internal/httpfetcher/fetch_internal_test.go +++ b/internal/httpfetcher/fetch_internal_test.go @@ -524,7 +524,7 @@ func TestFetchSemaphoreReleasedOnPartialReadClose(t *testing.T) { }) } -// The dial-time re-resolution in ssrfSafeDialer is what closes the DNS +// The dial-time re-resolution in dialSSRFSafe is what closes the DNS // rebinding window: even if validateURL saw a public answer earlier, the // dialer independently re-checks the address it is about to connect to. A full // rebinding simulation (a resolver returning public, then private) would mean @@ -544,9 +544,9 @@ func TestSSRFSafeDialerBlocksPrivateTarget(t *testing.T) { t.Run(addr, func(t *testing.T) { t.Parallel() - _, err := ssrfSafeDialer(context.Background(), "tcp", addr) + _, err := dialSSRFSafe(context.Background(), "tcp", addr, nil) if !errors.Is(err, ErrSSRFBlocked) { - t.Errorf("ssrfSafeDialer(%q) = %v, want ErrSSRFBlocked", addr, err) + t.Errorf("dialSSRFSafe(%q) = %v, want ErrSSRFBlocked", addr, err) } }) } @@ -560,7 +560,7 @@ func TestSSRFSafeDialerAllowsPublicTarget(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() - _, err := ssrfSafeDialer(ctx, "tcp", testPublicHost+":80") + _, err := dialSSRFSafe(ctx, "tcp", testPublicHost+":80", nil) if err == nil { t.Fatal("expected a dial error for an unreachable public target") } diff --git a/internal/httpfetcher/httpfetcher.go b/internal/httpfetcher/httpfetcher.go index 29373f4..8988d55 100644 --- a/internal/httpfetcher/httpfetcher.go +++ b/internal/httpfetcher/httpfetcher.go @@ -639,13 +639,6 @@ func ipv4CompatibleToIPv4(ip net.IP) net.IP { return ip } -// ssrfSafeDialer validates IP addresses against the built-in blocked ranges -// before connecting. New wraps dialSSRFSafe with the operator-supplied -// blocklist; this entry point enforces the built-in ranges alone. -func ssrfSafeDialer(ctx context.Context, network, addr string) (net.Conn, error) { - return dialSSRFSafe(ctx, network, addr, nil) -} - // dialSSRFSafe re-resolves addr and refuses to connect to any built-in // internal range or operator-supplied blocked prefix, closing the // DNS-rebinding window at connect time. diff --git a/internal/imageprocessor/imageprocessor.go b/internal/imageprocessor/imageprocessor.go index c344eea..c66148f 100644 --- a/internal/imageprocessor/imageprocessor.go +++ b/internal/imageprocessor/imageprocessor.go @@ -322,30 +322,6 @@ const ( mimeJXL = "image/jxl" ) -// SupportedInputFormats returns MIME types this processor can read. -func (p *ImageProcessor) SupportedInputFormats() []string { - return []string{ - mimeJPEG, - mimePNG, - mimeGIF, - mimeWebP, - mimeAVIF, - mimeJXL, - } -} - -// SupportedOutputFormats returns formats this processor can write. -func (p *ImageProcessor) SupportedOutputFormats() []imagerequest.Format { - return []imagerequest.Format{ - imagerequest.FormatJPEG, - imagerequest.FormatPNG, - imagerequest.FormatGIF, - imagerequest.FormatWebP, - imagerequest.FormatAVIF, - imagerequest.FormatJXL, - } -} - // FormatToMIME converts a Format to its MIME type string. func FormatToMIME(format imagerequest.Format) string { switch format { diff --git a/internal/imageprocessor/imageprocessor_internal_test.go b/internal/imageprocessor/imageprocessor_internal_test.go index f5a865e..90c45b9 100644 --- a/internal/imageprocessor/imageprocessor_internal_test.go +++ b/internal/imageprocessor/imageprocessor_internal_test.go @@ -312,22 +312,6 @@ func TestImageProcessor_ProcessPNG(t *testing.T) { } } -func TestImageProcessor_SupportedFormats(t *testing.T) { - t.Parallel() - - proc := New(Params{}) - - inputFormats := proc.SupportedInputFormats() - if len(inputFormats) == 0 { - t.Error("SupportedInputFormats() returned empty slice") - } - - outputFormats := proc.SupportedOutputFormats() - if len(outputFormats) == 0 { - t.Error("SupportedOutputFormats() returned empty slice") - } -} - func TestImageProcessor_RejectsOversizedInput(t *testing.T) { t.Parallel() diff --git a/internal/imagerequest/urlparser.go b/internal/imagerequest/urlparser.go index ce96231..9e8cc4c 100644 --- a/internal/imagerequest/urlparser.go +++ b/internal/imagerequest/urlparser.go @@ -66,23 +66,6 @@ var queryEscapeRegex = regexp.MustCompile(`%[0-9A-Fa-f]{2}|[^!-~]|["#'<>%]`) var hostRegex = regexp.MustCompile( `^(?:[A-Za-z0-9_.-]+|\[([0-9A-Fa-f:.]+)\])(?::[0-9]*)?$`) -// ParseImagePath parses an image URL's path after /v1/image/, decoded once: -// //., or // for JPEG XL. -// Examples: -// - cdn.example.com/photos/cat.jpg/800x600.webp -// - cdn.example.com/photos/cat.jpg/0x0.jpeg -// - cdn.example.com/photos/cat.jpg/orig.png -// - cdn.example.com/photos/cat.jpg/800x600 -func ParseImagePath(path string) (*Request, error) { - // Strip leading slash if present (chi may include it) - path = strings.TrimPrefix(path, "/") - if path == "" { - return nil, ErrMissingHost - } - - return parseImageComponents(path) -} - // ParseImageURL parses an image URL's path, decoded once as r.URL.Path holds // it: /v1/image///., or /v1/image/// // for JPEG XL. A source query follows the source path after a "?", which the diff --git a/internal/imagerequest/urlparser_internal_test.go b/internal/imagerequest/urlparser_internal_test.go index 83742ca..f80ebfd 100644 --- a/internal/imagerequest/urlparser_internal_test.go +++ b/internal/imagerequest/urlparser_internal_test.go @@ -192,76 +192,16 @@ func TestParseImageURL_Errors(t *testing.T) { } } -func TestParseImagePath(t *testing.T) { - t.Parallel() - - // ParseImagePath is for chi wildcard capture (no /v1/image/ prefix) - tests := []struct { - name string - input string - want *Request - wantErr bool - }{ - { - name: "chi wildcard capture", - input: "cdn.example.com/photos/cat.jpg/800x600.webp", - want: &Request{ - Source: Source{ - Host: testHostCDN, - Path: testPathCat, - }, - Transform: Transform{ - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, - }, - }, - }, - { - name: "with leading slash from chi", - input: "/cdn.example.com/photos/cat.jpg/800x600.webp", - want: &Request{ - Source: Source{ - Host: testHostCDN, - Path: testPathCat, - }, - Transform: Transform{ - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, - }, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - got, err := ParseImagePath(tt.input) - if (err != nil) != tt.wantErr { - t.Errorf("ParseImagePath() error = %v, wantErr %v", err, tt.wantErr) - - return - } - - if err != nil { - return - } - - assertRequest(t, got, tt.want) - }) - } -} - -// TestParseImagePath_LowerCasesHost verifies that the host is lower-cased, so +// TestParseImageURL_LowerCasesHost verifies that the host is lower-cased, so // that every spelling of its letter case is the same host to the allowlist, // the upstream connection limits and the cache, and that the path keeps its // case. -func TestParseImagePath_LowerCasesHost(t *testing.T) { +func TestParseImageURL_LowerCasesHost(t *testing.T) { t.Parallel() - got, err := ParseImagePath("CDN.Example.COM/Photos/Cat.jpg/800x600.webp") + got, err := ParseImageURL("/v1/image/CDN.Example.COM/Photos/Cat.jpg/800x600.webp") if err != nil { - t.Fatalf("ParseImagePath() error = %v", err) + t.Fatalf("ParseImageURL() error = %v", err) } t.Logf("parsed host %q, path %q", got.Source.Host, got.Source.Path) @@ -275,21 +215,21 @@ func TestParseImagePath_LowerCasesHost(t *testing.T) { } } -// TestParseImagePath_RefusesUserinfo verifies that a host holding "@" is +// TestParseImageURL_RefusesUserinfo verifies that a host holding "@" is // refused: a URL reads what comes before the "@" as a user name and password // and the rest as the host. -func TestParseImagePath_RefusesUserinfo(t *testing.T) { +func TestParseImageURL_RefusesUserinfo(t *testing.T) { t.Parallel() for _, input := range []string{ - "user@cdn.example.com/photos/cat.jpg/800x600.webp", - "user:password@cdn.example.com/photos/cat.jpg/800x600.webp", + "/v1/image/user@cdn.example.com/photos/cat.jpg/800x600.webp", + "/v1/image/user:password@cdn.example.com/photos/cat.jpg/800x600.webp", } { - got, err := ParseImagePath(input) - t.Logf("ParseImagePath(%q) = %+v, error = %v", input, got, err) + got, err := ParseImageURL(input) + t.Logf("ParseImageURL(%q) = %+v, error = %v", input, got, err) if !errors.Is(err, ErrInvalidHost) { - t.Errorf("ParseImagePath(%q) error = %v, want %v", + t.Errorf("ParseImageURL(%q) error = %v, want %v", input, err, ErrInvalidHost) } } @@ -315,6 +255,10 @@ func TestParseImageURL_PathTraversal(t *testing.T) { name: "parent in middle of path", input: "/v1/image/cdn.example.com/photos/../../../etc/passwd/800x600.jpeg", }, + { + name: "parent directory in path", + input: "/v1/image/cdn.example.com/photos/../secret/800x600.jpeg", + }, { name: "encoded parent directory", input: "/v1/image/cdn.example.com/photos/%2e%2e/secret/800x600.jpeg", @@ -357,40 +301,6 @@ func TestParseImageURL_PathTraversal(t *testing.T) { } } -func TestParseImagePath_PathTraversal(t *testing.T) { - t.Parallel() - - // Test path traversal via ParseImagePath (chi wildcard) - tests := []struct { - name string - input string - }{ - { - name: "parent directory in path", - input: "cdn.example.com/photos/../secret/800x600.jpeg", - }, - { - name: "encoded traversal", - input: "cdn.example.com/photos/%2e%2e/secret/800x600.jpeg", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - _, err := ParseImagePath(tt.input) - if err == nil { - t.Error("ParseImagePath() should reject path traversal attempts") - } - - if !errors.Is(err, ErrPathTraversal) { - t.Errorf("ParseImagePath() error = %v, want ErrPathTraversal", err) - } - }) - } -} - // TestParseImageURL_SourceURLHoldingPercent25 verifies that a source path and // a source query holding %25, the percent-escape of a literal "%", are // accepted as they are, although decoding them once more leaves a "%" that diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index 5342080..08ccd9a 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -14,12 +14,6 @@ import ( "sneak.berlin/go/pixa/internal/imagerequest" ) -// Cache errors. -var ( - ErrCacheMiss = errors.New("cache miss") - ErrNegativeCache = errors.New("negative cache hit") -) - // maxPendingAccessTimes is how many access times of variants served from // the cache, and how many of cached sources a miss used, may each wait to be // written, at about 80 bytes each. An access time handed over while that diff --git a/internal/imgcache/imgcache.go b/internal/imgcache/imgcache.go index 6eb082f..83dd0d8 100644 --- a/internal/imgcache/imgcache.go +++ b/internal/imgcache/imgcache.go @@ -1,13 +1,7 @@ -// Package imgcache provides interfaces and types for the image caching proxy. +// Package imgcache provides types for the image caching proxy. package imgcache -import ( - "context" - "io" - "time" - - "sneak.berlin/go/pixa/internal/imagerequest" -) +import "io" // ImageResponse represents a processed image ready to serve type ImageResponse struct { @@ -19,9 +13,7 @@ type ImageResponse struct { ContentType string // ETag is the entity tag for caching ETag string - // LastModified is when the content was last modified - LastModified time.Time - // CacheStatus indicates HIT, MISS, or STALE + // CacheStatus indicates HIT or MISS CacheStatus CacheStatus // FetchedBytes is the number of bytes fetched from upstream (0 if cache hit) FetchedBytes int64 @@ -32,16 +24,6 @@ type CacheStatus string // Cache status values for response headers. const ( - CacheHit CacheStatus = "HIT" - CacheMiss CacheStatus = "MISS" - CacheStale CacheStatus = "STALE" + CacheHit CacheStatus = "HIT" + CacheMiss CacheStatus = "MISS" ) - -// ImageCache is the main interface for the image caching proxy -type ImageCache interface { - // Get retrieves a processed image, fetching and processing if necessary - Get(ctx context.Context, req *imagerequest.Request) (*ImageResponse, error) - - // Warm pre-fetches and caches an image without returning it - Warm(ctx context.Context, req *imagerequest.Request) error -} diff --git a/internal/imgcache/partial_cache_files_internal_test.go b/internal/imgcache/partial_cache_files_internal_test.go index 647cf2c..2179396 100644 --- a/internal/imgcache/partial_cache_files_internal_test.go +++ b/internal/imgcache/partial_cache_files_internal_test.go @@ -290,9 +290,9 @@ func TestContentStorage_StoreReplacesFileOfAnotherSize(t *testing.T) { t.Fatalf("Store() error = %v", err) } - stored, err := storage.Load(key) + stored, _, err := storage.LoadWithSize(key) if err != nil { - t.Fatalf("Load() error = %v", err) + t.Fatalf("LoadWithSize() error = %v", err) } defer func() { _ = stored.Close() }() diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index fa564cd..7d03c0a 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -24,8 +24,7 @@ import ( "sneak.berlin/go/pixa/internal/signature" ) -// Service implements the ImageCache interface, orchestrating cache, -// fetcher, and processor. +// Service orchestrates the cache, fetcher, and processor. type Service struct { cache *Cache fetcher httpfetcher.Fetcher @@ -189,13 +188,6 @@ func (s *Service) Get( return response, nil } -// Warm pre-fetches and caches an image without returning it. -func (s *Service) Warm(ctx context.Context, req *imagerequest.Request) error { - _, err := s.Get(ctx, req) - - return err -} - // WaitForProcessing waits until no image is being processed, or until ctx // ends, and returns how many images were still being processed then. func (s *Service) WaitForProcessing(ctx context.Context) int { @@ -229,30 +221,6 @@ func (s *Service) ValidateRequest(req *imagerequest.Request) error { return s.signer.Verify(req) } -// GenerateSignedURL generates a signed URL for the given request. The URL -// carries q and fit next to sig and exp, so the image route verifies it for -// the quality and fit it was signed with. An unset quality or fit is first -// set to 85 or cover, the values the route uses when a URL has no q or fit. -func (s *Service) GenerateSignedURL( - baseURL string, - req *imagerequest.Request, - ttl time.Duration, -) (string, error) { - if req.Transform.Quality == 0 { - req.Transform.Quality = 85 - } - - if req.Transform.FitMode == "" { - req.Transform.FitMode = imagerequest.FitCover - } - - // Sets the request's Expires and Signature - path, sig, exp := s.signer.GenerateSignedURL(req, ttl) - - return fmt.Sprintf("%s%s?sig=%s&exp=%d&q=%d&fit=%s", - baseURL, path, sig, exp, req.Transform.Quality, req.Transform.FitMode), nil -} - // errPanicked is returned when processing a variant panicked. var errPanicked = errors.New("panic while processing image") diff --git a/internal/imgcache/storage.go b/internal/imgcache/storage.go index 4cfe4cd..d0da00f 100644 --- a/internal/imgcache/storage.go +++ b/internal/imgcache/storage.go @@ -62,22 +62,6 @@ func (s *ContentStorage) Store( return int64(len(data)), nil } -// Load returns a reader for the content with the given key. -func (s *ContentStorage) Load(key imagerequest.SourceKey) (io.ReadCloser, error) { - path := s.keyToPath(key) - - f, err := os.Open(path) //nolint:gosec // path derived from source key - if err != nil { - if os.IsNotExist(err) { - return nil, ErrNotFound - } - - return nil, fmt.Errorf("failed to open content: %w", err) - } - - return f, nil -} - // LoadWithSize returns a reader and file size for the content with the given key. func (s *ContentStorage) LoadWithSize( key imagerequest.SourceKey, @@ -251,22 +235,6 @@ func (s *VariantStorage) Store( return int64(len(data)), nil } -// Load returns a reader for the content at the given key. -func (s *VariantStorage) Load(key imagerequest.VariantKey) (io.ReadCloser, error) { - path := s.keyToPath(key) - - f, err := os.Open(path) //nolint:gosec // path derived from cache key - if err != nil { - if os.IsNotExist(err) { - return nil, ErrNotFound - } - - return nil, fmt.Errorf("failed to open content: %w", err) - } - - return f, nil -} - // LoadWithContentType opens the content at the given key and returns a // reader, the file size and the content type, which is read from the // file's first bytes. A missing file is ErrNotFound. A file whose first diff --git a/internal/imgcache/storage_internal_test.go b/internal/imgcache/storage_internal_test.go index dc88cc9..a3e0e78 100644 --- a/internal/imgcache/storage_internal_test.go +++ b/internal/imgcache/storage_internal_test.go @@ -11,7 +11,7 @@ import ( "sneak.berlin/go/pixa/internal/imagerequest" ) -func TestContentStorage_StoreAndLoad(t *testing.T) { +func TestContentStorage_StoreAndLoadWithSize(t *testing.T) { t.Parallel() tmpDir := t.TempDir() @@ -44,9 +44,9 @@ func TestContentStorage_StoreAndLoad(t *testing.T) { } // Load and verify content - r, err := storage.Load(key) + r, _, err := storage.LoadWithSize(key) if err != nil { - t.Fatalf("Load() error = %v", err) + t.Fatalf("LoadWithSize() error = %v", err) } defer func() { _ = r.Close() }() @@ -57,11 +57,11 @@ func TestContentStorage_StoreAndLoad(t *testing.T) { } if !bytes.Equal(loaded, content) { - t.Errorf("Load() content = %q, want %q", loaded, content) + t.Errorf("LoadWithSize() content = %q, want %q", loaded, content) } } -func TestContentStorage_LoadNotFound(t *testing.T) { +func TestContentStorage_LoadWithSizeNotFound(t *testing.T) { t.Parallel() tmpDir := t.TempDir() @@ -71,9 +71,9 @@ func TestContentStorage_LoadNotFound(t *testing.T) { t.Fatalf("NewContentStorage() error = %v", err) } - _, err = storage.Load(imagerequest.SourceKey("nonexistent")) + _, _, err = storage.LoadWithSize(imagerequest.SourceKey("nonexistent")) if !errors.Is(err, ErrNotFound) { - t.Errorf("Load() error = %v, want ErrNotFound", err) + t.Errorf("LoadWithSize() error = %v, want ErrNotFound", err) } } diff --git a/internal/magic/jpegxl_internal_test.go b/internal/magic/jpegxl_internal_test.go index 7592137..45d9197 100644 --- a/internal/magic/jpegxl_internal_test.go +++ b/internal/magic/jpegxl_internal_test.go @@ -37,22 +37,13 @@ func TestDetectFormatJPEGXL(t *testing.T) { } } -// TestJPEGXLMIMEType verifies that image/jxl is a supported input type and -// maps to and from the format jxl. +// TestJPEGXLMIMEType verifies that image/jxl maps to the format jxl. func TestJPEGXLMIMEType(t *testing.T) { t.Parallel() - if !IsSupportedMIMEType(testMIMEJXL) { - t.Errorf("IsSupportedMIMEType(%q) = false", testMIMEJXL) - } - format, ok := MIMEToImageFormat(testMIMEJXL) if format != FormatJXL || !ok { t.Errorf("MIMEToImageFormat(%q) = %q, %v, want %q, true", testMIMEJXL, format, ok, FormatJXL) } - - if got := ImageFormatToMIME(FormatJXL); got != testMIMEJXL { - t.Errorf("ImageFormatToMIME(%q) = %q, want %q", FormatJXL, got, testMIMEJXL) - } } diff --git a/internal/magic/magic.go b/internal/magic/magic.go index 922316a..db7781a 100644 --- a/internal/magic/magic.go +++ b/internal/magic/magic.go @@ -5,7 +5,6 @@ package magic import ( "bytes" "errors" - "io" "strings" ) @@ -36,22 +35,17 @@ type ImageFormat string // Supported image output formats. const ( - FormatOriginal ImageFormat = "orig" - FormatJPEG ImageFormat = "jpeg" - FormatPNG ImageFormat = "png" - FormatWebP ImageFormat = "webp" - FormatAVIF ImageFormat = "avif" - FormatJXL ImageFormat = "jxl" - FormatGIF ImageFormat = "gif" + FormatJPEG ImageFormat = "jpeg" + FormatPNG ImageFormat = "png" + FormatWebP ImageFormat = "webp" + FormatAVIF ImageFormat = "avif" + FormatJXL ImageFormat = "jxl" + FormatGIF ImageFormat = "gif" ) // MinMagicBytes is the minimum number of bytes needed to detect format. const MinMagicBytes = 12 -// mimeOctetStream is the fallback MIME type for formats without a -// specific MIME type. -const mimeOctetStream = "application/octet-stream" - // Magic byte signatures for supported formats. // These are effectively constants but Go doesn't support const slices. // @@ -190,42 +184,6 @@ func normalizeMIMEType(mimeType string) string { return strings.TrimSpace(strings.ToLower(mimeType)) } -// IsSupportedMIMEType checks if a MIME type is supported for input. -func IsSupportedMIMEType(mimeType string) bool { - normalized := normalizeMIMEType(mimeType) - switch MIMEType(normalized) { - case MIMETypeJPEG, MIMETypePNG, MIMETypeWebP, MIMETypeGIF, MIMETypeAVIF, - MIMETypeJXL, MIMETypeSVG: - return true - default: - return false - } -} - -// PeekAndValidate reads the minimum bytes needed for format detection, -// validates against the declared type, and returns a reader that includes -// those bytes for subsequent reading. -func PeekAndValidate(r io.Reader, declaredType string) (io.Reader, error) { - // Read minimum bytes for detection - buf := make([]byte, MinMagicBytes) - - n, err := io.ReadFull(r, buf) - if err != nil && !errors.Is(err, io.ErrUnexpectedEOF) { - return nil, err - } - - buf = buf[:n] - - // Validate magic bytes - err = ValidateMagicBytes(buf, declaredType) - if err != nil { - return nil, err - } - - // Return a reader that includes the peeked bytes - return io.MultiReader(bytes.NewReader(buf), r), nil -} - // MIMEToImageFormat converts a MIME type to an ImageFormat. func MIMEToImageFormat(mimeType string) (ImageFormat, bool) { normalized := normalizeMIMEType(mimeType) @@ -249,26 +207,3 @@ func MIMEToImageFormat(mimeType string) (ImageFormat, bool) { return "", false } } - -// ImageFormatToMIME converts an ImageFormat to a MIME type string. -func ImageFormatToMIME(format ImageFormat) string { - switch format { - case FormatJPEG: - return string(MIMETypeJPEG) - case FormatPNG: - return string(MIMETypePNG) - case FormatWebP: - return string(MIMETypeWebP) - case FormatGIF: - return string(MIMETypeGIF) - case FormatAVIF: - return string(MIMETypeAVIF) - case FormatJXL: - return string(MIMETypeJXL) - case FormatOriginal: - // Original format passes content through unchanged. - return mimeOctetStream - default: - return mimeOctetStream - } -} diff --git a/internal/magic/magic_internal_test.go b/internal/magic/magic_internal_test.go index e0295ee..fc081ea 100644 --- a/internal/magic/magic_internal_test.go +++ b/internal/magic/magic_internal_test.go @@ -3,9 +3,6 @@ package magic import ( "bytes" "errors" - "io" - "slices" - "strings" "testing" ) @@ -155,119 +152,6 @@ func TestValidateMagicBytes(t *testing.T) { } } -func TestIsSupportedMIMEType(t *testing.T) { - t.Parallel() - - tests := []struct { - mimeType string - want bool - }{ - {testMIMEJPEG, true}, - {testMIMEPNG, true}, - {testMIMEWebP, true}, - {testMIMEGIF, true}, - {testMIMEAVIF, true}, - {"image/svg+xml", true}, - {"IMAGE/JPEG", true}, - {testMIMEJPEGParams, true}, - {"image/tiff", false}, - {"image/bmp", false}, - {mimeOctetStream, false}, - {"text/plain", false}, - {"", false}, - } - - for _, tt := range tests { - t.Run(tt.mimeType, func(t *testing.T) { - t.Parallel() - - if got := IsSupportedMIMEType(tt.mimeType); got != tt.want { - t.Errorf("IsSupportedMIMEType(%q) = %v, want %v", tt.mimeType, got, tt.want) - } - }) - } -} - -func TestPeekAndValidate(t *testing.T) { - t.Parallel() - - jpegMagic := []byte{ - 0xFF, 0xD8, 0xFF, 0xE0, 0x00, 0x10, 0x4A, 0x46, 0x49, 0x46, 0x00, 0x01, - } - pngMagic := []byte{ - 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, 0x00, 0x00, 0x00, 0x0D, - } - jpegData := slices.Concat(jpegMagic, []byte("rest of jpeg data")) - pngData := slices.Concat(pngMagic, []byte("rest of png data")) - - tests := []struct { - name string - data []byte - declaredType string - wantErr bool - wantData []byte - }{ - { - name: "valid JPEG", - data: jpegData, - declaredType: testMIMEJPEG, - wantErr: false, - wantData: jpegData, - }, - { - name: "valid PNG", - data: pngData, - declaredType: testMIMEPNG, - wantErr: false, - wantData: pngData, - }, - { - name: "mismatched type", - data: jpegData, - declaredType: testMIMEPNG, - wantErr: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - r := bytes.NewReader(tt.data) - - result, err := PeekAndValidate(r, tt.declaredType) - if tt.wantErr { - if err == nil { - t.Error("PeekAndValidate() expected error, got nil") - } - - return - } - - if err != nil { - t.Errorf("PeekAndValidate() unexpected error = %v", err) - - return - } - - // Read all data from result reader - got, err := io.ReadAll(result) - if err != nil { - t.Errorf("Failed to read result: %v", err) - - return - } - - if !bytes.Equal(got, tt.wantData) { - t.Errorf( - "PeekAndValidate() data mismatch: got %d bytes, want %d bytes", - len(got), len(tt.wantData), - ) - } - }) - } -} - func TestMIMEToImageFormat(t *testing.T) { t.Parallel() @@ -303,35 +187,6 @@ func TestMIMEToImageFormat(t *testing.T) { } } -func TestImageFormatToMIME(t *testing.T) { - t.Parallel() - - tests := []struct { - format ImageFormat - wantMIME string - }{ - {FormatJPEG, testMIMEJPEG}, - {FormatPNG, testMIMEPNG}, - {FormatWebP, testMIMEWebP}, - {FormatGIF, testMIMEGIF}, - {FormatAVIF, testMIMEAVIF}, - {FormatOriginal, mimeOctetStream}, - {"unknown", mimeOctetStream}, - } - - for _, tt := range tests { - t.Run(string(tt.format), func(t *testing.T) { - t.Parallel() - - got := ImageFormatToMIME(tt.format) - - if got != tt.wantMIME { - t.Errorf("ImageFormatToMIME(%q) = %v, want %v", tt.format, got, tt.wantMIME) - } - }) - } -} - func TestNormalizeMIMEType(t *testing.T) { t.Parallel() @@ -493,34 +348,3 @@ func TestDetectFormatFtypNotAVIF(t *testing.T) { t.Errorf("DetectFormat(MP4) error = %v, want %v", err, ErrUnknownFormat) } } - -func TestPeekAndValidatePreservesReader(t *testing.T) { - t.Parallel() - - // Ensure that after PeekAndValidate, we can read the complete - // original content - originalContent := append( - []byte{0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, 0x00, 0x00, 0x00, 0x0D}, - []byte(strings.Repeat("PNG IDAT chunk data here ", 100))..., - ) - - r := bytes.NewReader(originalContent) - - validated, err := PeekAndValidate(r, testMIMEPNG) - if err != nil { - t.Fatalf("PeekAndValidate() error = %v", err) - } - - // Read everything from the validated reader - got, err := io.ReadAll(validated) - if err != nil { - t.Fatalf("io.ReadAll() error = %v", err) - } - - if !bytes.Equal(got, originalContent) { - t.Errorf( - "Content mismatch: got %d bytes, want %d bytes", - len(got), len(originalContent), - ) - } -} diff --git a/internal/server/source_url_internal_test.go b/internal/server/source_url_internal_test.go index 05e8ef4..550b8e5 100644 --- a/internal/server/source_url_internal_test.go +++ b/internal/server/source_url_internal_test.go @@ -513,20 +513,27 @@ func TestImageRoutesFetchIPv6SourceHostWithOrWithoutPort(t *testing.T) { func signedImageURL(t *testing.T, sourceHost string) string { t.Helper() - path, sig, exp := signature.New(testSigningKey).GenerateSignedURL( - &imagerequest.Request{ - Source: imagerequest.Source{ - Host: sourceHost, - Path: ipv6SourcePath, - }, - Transform: imagerequest.Transform{ - Format: imagerequest.FormatPNG, - Quality: 85, - FitMode: imagerequest.FitCover, - }, - }, time.Hour) + expires := time.Now().Add(time.Hour) + sig := signature.New(testSigningKey).Sign(&imagerequest.Request{ + Source: imagerequest.Source{ + Host: sourceHost, + Path: ipv6SourcePath, + }, + Transform: imagerequest.Transform{ + Format: imagerequest.FormatPNG, + Quality: 85, + FitMode: imagerequest.FitCover, + }, + Expires: expires, + }) - return path + "?sig=" + sig + "&exp=" + strconv.FormatInt(exp, 10) + // Go's percent-encoding of a URL path writes the brackets of the IPv6 + // host as %5B and %5D. + path := (&url.URL{ + Path: "/v1/image/" + sourceHost + ipv6SourcePath + "/orig.png", + }).EscapedPath() + + return path + "?sig=" + sig + "&exp=" + strconv.FormatInt(expires.Unix(), 10) } // encryptedImageURL returns the encrypted URL of ipv6SourcePath on sourceHost diff --git a/internal/signature/golden_test.go b/internal/signature/golden_test.go index bb8eda2..f3d6e16 100644 --- a/internal/signature/golden_test.go +++ b/internal/signature/golden_test.go @@ -22,11 +22,6 @@ type goldenVector struct { // padded) HMAC-SHA256 signature for the request with Expires // set to goldenExpiresUnix. wantSignature string - // wantSignedPath is the exact path returned by - // GenerateSignedURL for the request. The signature and - // expiration are returned separately by GenerateSignedURL and - // are not embedded in the path. - wantSignedPath string } // goldenVectors returns the known-answer vectors. The expected values @@ -50,8 +45,7 @@ func goldenVectors() []goldenVector { }, // Signed data: // "cdn.example.com:/photos/cat.jpg::800:600:webp:1704067200:85:cover" - wantSignature: "kdqeGoW2SX7qnaYtoB970wEnLydn0UnIgQYQLfAnjXQ=", - wantSignedPath: testSignedPath, + wantSignature: "kdqeGoW2SX7qnaYtoB970wEnLydn0UnIgQYQLfAnjXQ=", }, { name: "resized with query string", @@ -71,8 +65,6 @@ func goldenVectors() []goldenVector { // Signed data: // "cdn.example.com:/photos/cat.jpg:token=abc&v=2:800:600:webp:1704067200:85:cover" wantSignature: "pKgVBOTd_Q_EikI7MNQLC9Q8Hurdxzyv3EIYvVhqc2I=", - wantSignedPath: "/v1/image/cdn.example.com/photos/cat.jpg" + - "%3Ftoken=abc&v=2/800x600.webp", }, { name: "original size without query", @@ -91,8 +83,7 @@ func goldenVectors() []goldenVector { }, // Signed data: // "cdn.example.com:/photos/cat.jpg::0:0:png:1704067200:85:cover" - wantSignature: "6_rZ0yyVbGZRs8kG7n7HLgLi5Jt8vjiWQljIEL1jbIs=", - wantSignedPath: "/v1/image/cdn.example.com/photos/cat.jpg/orig.png", + wantSignature: "6_rZ0yyVbGZRs8kG7n7HLgLi5Jt8vjiWQljIEL1jbIs=", }, { name: "non-default quality and fit", @@ -112,22 +103,17 @@ func goldenVectors() []goldenVector { // Signed data: // "cdn.example.com:/photos/cat.jpg::800:600:webp:1704067200:40:contain" wantSignature: "pGaXpPUbI3A7nMx-4T9bfq9bYWBNL0kY4bxlcv3g1F8=", - // The path is the same as for the default quality and fit: - // q=40&fit=contain go in the query string next to sig and - // exp (imgcache.Service.GenerateSignedURL adds all four). - wantSignedPath: testSignedPath, }, } } -// TestSigner_GoldenVectors pins the exact HMAC-SHA256 signature output and -// the exact generated signed URL path for fully-specified requests with a -// hardcoded signing key. +// TestSigner_GoldenVectors pins the exact HMAC-SHA256 signature output for +// fully-specified requests with a hardcoded signing key. // // If any of these assertions fail, the signed byte format -// ("host:path:query:width:height:format:expiration:quality:fit"), the -// base64url encoding, or the signed URL layout has changed. Update these -// constants only when that change is intended. +// ("host:path:query:width:height:format:expiration:quality:fit") or the +// base64url encoding has changed. Update these constants only when that +// change is intended. func TestSigner_GoldenVectors(t *testing.T) { t.Parallel() @@ -145,14 +131,6 @@ func TestSigner_GoldenVectors(t *testing.T) { t.Errorf("Sign() = %q, want %q (signed byte format changed?)", gotSignature, tt.wantSignature) } - - urlReq := tt.req - - gotPath, _, _ := signer.GenerateSignedURL(&urlReq, time.Hour) - if gotPath != tt.wantSignedPath { - t.Errorf("GenerateSignedURL() path = %q, want %q (signed URL layout changed?)", - gotPath, tt.wantSignedPath) - } }) } } diff --git a/internal/signature/signature.go b/internal/signature/signature.go index beff9b4..6982756 100644 --- a/internal/signature/signature.go +++ b/internal/signature/signature.go @@ -8,8 +8,6 @@ import ( "encoding/base64" "errors" "fmt" - "net/url" - "strconv" "strings" "time" @@ -79,44 +77,6 @@ func (s *Signer) Verify(req *imagerequest.Request) error { return nil } -// GenerateSignedURL creates a complete URL with signature and expiration. -// Returns the path portion that should be appended to the base URL. -func (s *Signer) GenerateSignedURL( - req *imagerequest.Request, ttl time.Duration, -) (string, string, int64) { - // Set expiration - req.Expires = time.Now().Add(ttl) - exp := req.Expires.Unix() - - // Generate signature - sig := s.Sign(req) - req.Signature = sig - - // Build the size component - var sizeStr string - if req.Transform.Size.Width == 0 && req.Transform.Size.Height == 0 { - sizeStr = "orig" - } else { - sizeStr = fmt.Sprintf("%dx%d", - req.Transform.Size.Width, req.Transform.Size.Height) - } - - // Build the path. A source query follows the source path after a "?". - // The image route reads the path decoded once, so the whole path is - // written in Go's own percent-encoding of a URL path, which writes "?" - // as %3F and "%" as %25, and leaves "/", ";", ",", "=" and "&" as they - // are. - sourcePathAndQuery := req.Source.Path - if req.Source.Query != "" { - sourcePathAndQuery += "?" + req.Source.Query - } - - unescapedPath := fmt.Sprintf("/v1/image/%s%s/%s.%s", - req.Source.Host, sourcePathAndQuery, sizeStr, req.Transform.Format) - - return (&url.URL{Path: unescapedPath}).EscapedPath(), sig, exp -} - // buildSignatureData creates the string to be signed. // Format: "host:path:query:width:height:format:expiration:quality:fit" // The path and query go through escapePercentAndColon, so that a ":" in @@ -146,17 +106,3 @@ func escapePercentAndColon(s string) string { return strings.ReplaceAll(s, ":", "%3A") } - -// ParseParams extracts signature and expiration from query parameters. -func ParseParams(sig, expStr string) (string, time.Time, error) { - if expStr == "" { - return sig, time.Time{}, nil - } - - expUnix, err := strconv.ParseInt(expStr, 10, 64) - if err != nil { - return "", time.Time{}, fmt.Errorf("invalid expiration: %w", err) - } - - return sig, time.Unix(expUnix, 0), nil -} diff --git a/internal/signature/signature_test.go b/internal/signature/signature_test.go index 119fb14..dfe08a2 100644 --- a/internal/signature/signature_test.go +++ b/internal/signature/signature_test.go @@ -2,7 +2,6 @@ package signature_test import ( "errors" - "strings" "testing" "time" @@ -18,8 +17,6 @@ const ( testFormatPNG = "png" testFitCover = "cover" testFitContain = "contain" - testSignedPath = "/v1/image/cdn.example.com/photos/cat.jpg/800x600.webp" - testSig = "abc123" ) func TestSigner_Sign(t *testing.T) { @@ -378,140 +375,10 @@ func TestSigner_DifferentKeys(t *testing.T) { } } -func TestGenerateSignedURL(t *testing.T) { - t.Parallel() - - signer := signature.New("test-secret-key") - - req := &imagerequest.Request{ - Source: imagerequest.Source{ - Host: testHost, - Path: testPath, - Query: "", - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 800, Height: 600}, - Format: testFormatWebP, - }, - } - - ttl := 1 * time.Hour - path, sig, exp := signer.GenerateSignedURL(req, ttl) - - // Path should be correct format - if path != testSignedPath { - t.Errorf("GenerateSignedURL() path = %q, want %q", path, testSignedPath) - } - - // Signature should be non-empty - if sig == "" { - t.Error("GenerateSignedURL() produced empty signature") - } - - // Expiration should be approximately now + TTL - expTime := time.Unix(exp, 0) - - expectedExp := time.Now().Add(ttl) - if expTime.Sub(expectedExp) > time.Second { - t.Errorf("GenerateSignedURL() exp time off by too much") - } - - // Request should have been updated with signature and expiration - if req.Signature != sig { - t.Errorf("GenerateSignedURL() didn't update request signature") - } -} - -func TestGenerateSignedURL_OrigSize(t *testing.T) { - t.Parallel() - - signer := signature.New("test-secret-key") - - req := &imagerequest.Request{ - Source: imagerequest.Source{ - Host: testHost, - Path: testPath, - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 0, Height: 0}, // Original size - Format: testFormatPNG, - }, - } - - path, _, _ := signer.GenerateSignedURL(req, time.Hour) - - expectedPath := "/v1/image/cdn.example.com/photos/cat.jpg/orig.png" - if path != expectedPath { - t.Errorf("GenerateSignedURL() path = %q, want %q", path, expectedPath) - } -} - -func TestGenerateSignedURL_WithQueryString(t *testing.T) { - t.Parallel() - - signer := signature.New("test-secret-key-for-testing!") - - req := &imagerequest.Request{ - Source: imagerequest.Source{ - Host: testHost, - Path: testPath, - Query: "token=abc&v=2", - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 800, Height: 600}, - Format: testFormatWebP, - }, - } - - path, _, _ := signer.GenerateSignedURL(req, time.Hour) - - // The path must NOT contain a bare "?" that would be interpreted as - // a query string delimiter. The size segment must appear as the last - // path component. - if strings.Contains(path, "?token=abc") { - t.Errorf("GenerateSignedURL() produced bare query string in path: %q", path) - } - - // The size segment must be present in the path - if !strings.Contains(path, "/800x600.webp") { - t.Errorf("GenerateSignedURL() missing size segment in path: %q", path) - } - - // Path should end with the size.format, not with query params - if !strings.HasSuffix(path, "/800x600.webp") { - t.Errorf("GenerateSignedURL() path should end with size.format: %q", path) - } -} - -func TestGenerateSignedURL_WithoutQueryString(t *testing.T) { - t.Parallel() - - signer := signature.New("test-secret-key-for-testing!") - - req := &imagerequest.Request{ - Source: imagerequest.Source{ - Host: testHost, - Path: testPath, - }, - Transform: imagerequest.Transform{ - Size: imagerequest.Size{Width: 800, Height: 600}, - Format: testFormatWebP, - }, - } - - path, _, _ := signer.GenerateSignedURL(req, time.Hour) - - if path != testSignedPath { - t.Errorf("GenerateSignedURL() path = %q, want %q", path, testSignedPath) - } -} - -// TestGenerateSignedURL_ReadmeQueryExample checks the README's example of a -// source URL with a query, the image at -// https://cdn.example.com/img?url=https://origin.example.com/my%20cat.jpg: its -// signature with the README's key, and the path GenerateSignedURL writes for -// it, with the "?" written as %3F and the "%" as %25. -func TestGenerateSignedURL_ReadmeQueryExample(t *testing.T) { +// TestSign_ReadmeQueryExample checks the signature, with the README's key, of +// the README's example of a source URL with a query, the image at +// https://cdn.example.com/img?url=https://origin.example.com/my%20cat.jpg. +func TestSign_ReadmeQueryExample(t *testing.T) { t.Parallel() signer := signature.New("example-signing-key-for-documentation") @@ -537,76 +404,4 @@ func TestGenerateSignedURL_ReadmeQueryExample(t *testing.T) { if gotSig != wantSig { t.Errorf("Sign() = %q, want %q", gotSig, wantSig) } - - const wantPath = "/v1/image/cdn.example.com" + - "/img%3Furl=https://origin.example.com/my%2520cat.jpg/800x600.webp" - - gotPath, _, _ := signer.GenerateSignedURL(&req, time.Hour) - if gotPath != wantPath { - t.Errorf("GenerateSignedURL() path = %q, want %q", gotPath, wantPath) - } -} - -func TestParseParams(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - sig string - expStr string - wantSig string - wantErr bool - checkTime bool - }{ - { - name: "valid params", - sig: testSig, - expStr: "1704067200", - wantSig: testSig, - wantErr: false, - }, - { - name: "empty expiration", - sig: testSig, - expStr: "", - wantSig: testSig, - wantErr: false, - }, - { - name: "invalid expiration", - sig: testSig, - expStr: "not-a-number", - wantErr: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - sig, exp, err := signature.ParseParams(tt.sig, tt.expStr) - - if tt.wantErr { - if err == nil { - t.Error("ParseParams() expected error, got nil") - } - - return - } - - if err != nil { - t.Errorf("ParseParams() unexpected error = %v", err) - - return - } - - if sig != tt.wantSig { - t.Errorf("sig = %q, want %q", sig, tt.wantSig) - } - - if tt.expStr != "" && exp.IsZero() { - t.Error("exp should not be zero when expStr is provided") - } - }) - } } -- 2.54.0