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") - } - }) - } }