From 6526b717dd7090f9b684b8608242a0722306ebed Mon Sep 17 00:00:00 2001 From: sneak Date: Sat, 25 Jul 2026 17:51:30 +0700 Subject: [PATCH] refactor: extract signature package from imgcache Move HMAC-SHA256 request signing out of internal/imgcache/ into its own internal/signature/ package, per the plan in issue #39. This is one of the remaining "easily separable" extractions; only the signer is moved here so the diff stays reviewable. What moved (from internal/imgcache/signature.go): - Signer type, its New constructor, Sign, Verify, GenerateSignedURL - ParseParams (query-string signature/expiration parsing) - Signature error sentinels To keep the import edge one-way (imgcache depends on signature, never the reverse), the package defines a standalone Request type carrying just the fields the signature covers, instead of importing imgcache.ImageRequest. imgcache projects its ImageRequest onto signature.Request via a small unexported helper. Renames (no stuttering): - NewSigner -> signature.New - ParseSignatureParams -> signature.ParseParams - ErrSignatureRequired/Invalid/Expired -> signature.ErrRequired/Invalid/Expired The "non-whitelisted host" error message is updated to "non-allowlisted" for inclusive terminology, consistent with the allowlist rename. Pure refactor. The bytes fed to the HMAC are unchanged, so signatures remain compatible. All existing tests move with the package; docker build passes (fmt-check, lint, test, build). refs #39 --- internal/imgcache/service.go | 30 ++- internal/imgcache/service_test.go | 13 +- internal/imgcache/signature_query_test.go | 55 ----- internal/{imgcache => signature}/signature.go | 70 ++++-- .../{imgcache => signature}/signature_test.go | 223 +++++++++++------- 5 files changed, 223 insertions(+), 168 deletions(-) delete mode 100644 internal/imgcache/signature_query_test.go rename internal/{imgcache => signature}/signature.go (59%) rename internal/{imgcache => signature}/signature_test.go (68%) diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index 4229911..f2f6b26 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -15,6 +15,7 @@ import ( "sneak.berlin/go/pixa/internal/httpfetcher" "sneak.berlin/go/pixa/internal/imageprocessor" "sneak.berlin/go/pixa/internal/magic" + "sneak.berlin/go/pixa/internal/signature" ) // Service implements the ImageCache interface, orchestrating cache, fetcher, and processor. @@ -22,7 +23,7 @@ type Service struct { cache *Cache fetcher httpfetcher.Fetcher processor *imageprocessor.ImageProcessor - signer *Signer + signer *signature.Signer allowlist *allowlist.HostAllowList log *slog.Logger allowHTTP bool @@ -69,7 +70,7 @@ func NewService(cfg *ServiceConfig) (*Service, error) { fetcher = httpfetcher.New(fetcherCfg) } - signer := NewSigner(cfg.SigningKey) + signer := signature.New(cfg.SigningKey) log := cfg.Logger if log == nil { @@ -397,7 +398,7 @@ func (s *Service) ValidateRequest(req *ImageRequest) error { } // Signature required for non-allowed hosts - return s.signer.Verify(req) + return s.signer.Verify(signatureRequest(req)) } // GenerateSignedURL generates a signed URL for the given request. @@ -406,11 +407,32 @@ func (s *Service) GenerateSignedURL( req *ImageRequest, ttl time.Duration, ) (string, error) { - path, sig, exp := s.signer.GenerateSignedURL(req, ttl) + sigReq := signatureRequest(req) + path, sig, exp := s.signer.GenerateSignedURL(sigReq, ttl) + + // Propagate the generated signature and expiration back onto the request. + req.Expires = sigReq.Expires + req.Signature = sigReq.Signature return fmt.Sprintf("%s%s?sig=%s&exp=%d", baseURL, path, sig, exp), nil } +// signatureRequest projects an ImageRequest onto the standalone +// signature.Request type used by the signature package. This keeps the +// import edge one-way: imgcache depends on signature, never the reverse. +func signatureRequest(req *ImageRequest) *signature.Request { + return &signature.Request{ + SourceHost: req.SourceHost, + SourcePath: req.SourcePath, + SourceQuery: req.SourceQuery, + Width: req.Size.Width, + Height: req.Size.Height, + Format: string(req.Format), + Signature: req.Signature, + Expires: req.Expires, + } +} + // HTTP status codes for error responses. const ( httpStatusBadGateway = 502 diff --git a/internal/imgcache/service_test.go b/internal/imgcache/service_test.go index 5e0b7be..9fe05c6 100644 --- a/internal/imgcache/service_test.go +++ b/internal/imgcache/service_test.go @@ -7,6 +7,7 @@ import ( "time" "sneak.berlin/go/pixa/internal/magic" + "sneak.berlin/go/pixa/internal/signature" ) func TestService_Get_WhitelistedHost(t *testing.T) { @@ -77,9 +78,9 @@ func TestService_Get_NonWhitelistedHost_ValidSignature(t *testing.T) { } // Generate a valid signature - signer := NewSigner(signingKey) + signer := signature.New(signingKey) req.Expires = time.Now().Add(time.Hour) - req.Signature = signer.Sign(req) + req.Signature = signer.Sign(signatureRequest(req)) // Should pass validation err := svc.ValidateRequest(req) @@ -118,9 +119,9 @@ func TestService_Get_NonWhitelistedHost_ExpiredSignature(t *testing.T) { } // Generate an expired signature - signer := NewSigner(signingKey) + signer := signature.New(signingKey) req.Expires = time.Now().Add(-time.Hour) // Already expired - req.Signature = signer.Sign(req) + req.Signature = signer.Sign(signatureRequest(req)) // Should fail validation err := svc.ValidateRequest(req) @@ -164,7 +165,7 @@ func TestService_ValidateRequest_SignatureExactHostMatch(t *testing.T) { WithNoWhitelist(), ) - signer := NewSigner(signingKey) + signer := signature.New(signingKey) // Sign a request for "cdn.example.com" signedReq := &ImageRequest{ @@ -176,7 +177,7 @@ func TestService_ValidateRequest_SignatureExactHostMatch(t *testing.T) { FitMode: FitCover, Expires: time.Now().Add(time.Hour), } - signedReq.Signature = signer.Sign(signedReq) + signedReq.Signature = signer.Sign(signatureRequest(signedReq)) // The original request should pass validation t.Run("exact host passes", func(t *testing.T) { diff --git a/internal/imgcache/signature_query_test.go b/internal/imgcache/signature_query_test.go deleted file mode 100644 index da05d6f..0000000 --- a/internal/imgcache/signature_query_test.go +++ /dev/null @@ -1,55 +0,0 @@ -package imgcache - -import ( - "strings" - "testing" - "time" -) - -func TestGenerateSignedURL_WithQueryString(t *testing.T) { - signer := NewSigner("test-secret-key-for-testing!") - - req := &ImageRequest{ - SourceHost: "cdn.example.com", - SourcePath: "/photos/cat.jpg", - SourceQuery: "token=abc&v=2", - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, - } - - 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) { - signer := NewSigner("test-secret-key-for-testing!") - - req := &ImageRequest{ - SourceHost: "cdn.example.com", - SourcePath: "/photos/cat.jpg", - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, - } - - path, _, _ := signer.GenerateSignedURL(req, time.Hour) - - expected := "/v1/image/cdn.example.com/photos/cat.jpg/800x600.webp" - if path != expected { - t.Errorf("GenerateSignedURL() path = %q, want %q", path, expected) - } -} diff --git a/internal/imgcache/signature.go b/internal/signature/signature.go similarity index 59% rename from internal/imgcache/signature.go rename to internal/signature/signature.go index ebda10a..092f067 100644 --- a/internal/imgcache/signature.go +++ b/internal/signature/signature.go @@ -1,4 +1,6 @@ -package imgcache +// Package signature provides HMAC-SHA256 signing and verification of image +// requests. +package signature import ( "crypto/hmac" @@ -13,27 +15,49 @@ import ( // Signature errors. var ( - ErrSignatureRequired = errors.New("signature required for non-whitelisted host") - ErrSignatureInvalid = errors.New("invalid signature") - ErrSignatureExpired = errors.New("signature has expired") + ErrRequired = errors.New("signature required for non-allowlisted host") + ErrInvalid = errors.New("invalid signature") + ErrExpired = errors.New("signature has expired") ErrMissingExpiration = errors.New("signature expiration is required") ) +// Request carries the components an image request signature covers. It is a +// standalone type so that this package does not depend on imgcache, keeping +// the import edge one-way (imgcache depends on signature, never the reverse). +type Request struct { + // SourceHost is the origin host (e.g. "cdn.example.com"). + SourceHost string + // SourcePath is the path on the origin (e.g. "/photos/cat.jpg"). + SourcePath string + // SourceQuery is the optional query string for the origin URL. + SourceQuery string + // Width is the requested output width in pixels. + Width int + // Height is the requested output height in pixels. + Height int + // Format is the requested output format (e.g. "webp"). + Format string + // Signature is the HMAC signature to verify. + Signature string + // Expires is the signature expiration timestamp. + Expires time.Time +} + // Signer handles HMAC-SHA256 signature generation and verification. type Signer struct { secretKey []byte } -// NewSigner creates a new Signer with the given secret key. -func NewSigner(secretKey string) *Signer { +// New creates a new Signer with the given secret key. +func New(secretKey string) *Signer { return &Signer{ secretKey: []byte(secretKey), } } -// Sign generates an HMAC-SHA256 signature for the given image request. +// Sign generates an HMAC-SHA256 signature for the given request. // The signature covers: host + path + query + width + height + format + expiration. -func (s *Signer) Sign(req *ImageRequest) string { +func (s *Signer) Sign(req *Request) string { data := s.buildSignatureData(req) mac := hmac.New(sha256.New, s.secretKey) mac.Write([]byte(data)) @@ -48,14 +72,14 @@ func (s *Signer) Sign(req *ImageRequest) string { // No suffix matching, wildcard matching, or partial matching is supported. // A signature for "cdn.example.com" will NOT verify for "example.com" or // "other.cdn.example.com", and vice versa. -func (s *Signer) Verify(req *ImageRequest) error { +func (s *Signer) Verify(req *Request) error { // Check expiration first if req.Expires.IsZero() { return ErrMissingExpiration } if time.Now().After(req.Expires) { - return ErrSignatureExpired + return ErrExpired } // Compute expected signature @@ -63,7 +87,7 @@ func (s *Signer) Verify(req *ImageRequest) error { // Constant-time comparison to prevent timing attacks if !hmac.Equal([]byte(req.Signature), []byte(expected)) { - return ErrSignatureInvalid + return ErrInvalid } return nil @@ -73,13 +97,13 @@ func (s *Signer) Verify(req *ImageRequest) error { // Format: "host:path:query:width:height:format:expiration" // All components are used verbatim (exact match). No normalization, // suffix matching, or wildcard expansion is performed. -func (s *Signer) buildSignatureData(req *ImageRequest) string { +func (s *Signer) buildSignatureData(req *Request) string { return fmt.Sprintf("%s:%s:%s:%d:%d:%s:%d", req.SourceHost, req.SourcePath, req.SourceQuery, - req.Size.Width, - req.Size.Height, + req.Width, + req.Height, req.Format, req.Expires.Unix(), ) @@ -87,7 +111,7 @@ func (s *Signer) buildSignatureData(req *ImageRequest) string { // 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, ttl time.Duration) (path string, sig string, exp int64) { +func (s *Signer) GenerateSignedURL(req *Request, ttl time.Duration) (path string, sig string, exp int64) { // Set expiration req.Expires = time.Now().Add(ttl) exp = req.Expires.Unix() @@ -98,15 +122,15 @@ func (s *Signer) GenerateSignedURL(req *ImageRequest, ttl time.Duration) (path s // Build the size component var sizeStr string - if req.Size.OriginalSize() { + if req.Width == 0 && req.Height == 0 { sizeStr = "orig" } else { - sizeStr = fmt.Sprintf("%dx%d", req.Size.Width, req.Size.Height) + sizeStr = fmt.Sprintf("%dx%d", req.Width, req.Height) } // Build the path. // When a source query is present, it is embedded as a path segment - // (e.g. /host/path?query/size.fmt) so that ParseImagePath can extract + // (e.g. /host/path?query/size.fmt) so that the URL parser can extract // it from the last-slash split. The "?" inside a path segment is // percent-encoded by clients but chi delivers it decoded, which is // exactly what the URL parser expects. @@ -130,12 +154,12 @@ func (s *Signer) GenerateSignedURL(req *ImageRequest, ttl time.Duration) (path s return path, sig, exp } -// ParseSignatureParams extracts signature and expiration from query parameters. -func ParseSignatureParams(sig, expStr string) (signature string, expires time.Time, err error) { - signature = sig +// ParseParams extracts signature and expiration from query parameters. +func ParseParams(sig, expStr string) (parsed string, expires time.Time, err error) { + parsed = sig if expStr == "" { - return signature, time.Time{}, nil + return parsed, time.Time{}, nil } expUnix, err := strconv.ParseInt(expStr, 10, 64) @@ -145,5 +169,5 @@ func ParseSignatureParams(sig, expStr string) (signature string, expires time.Ti expires = time.Unix(expUnix, 0) - return signature, expires, nil + return parsed, expires, nil } diff --git a/internal/imgcache/signature_test.go b/internal/signature/signature_test.go similarity index 68% rename from internal/imgcache/signature_test.go rename to internal/signature/signature_test.go index 6dc3632..0f2f223 100644 --- a/internal/imgcache/signature_test.go +++ b/internal/signature/signature_test.go @@ -1,19 +1,21 @@ -package imgcache +package signature import ( + "strings" "testing" "time" ) func TestSigner_Sign(t *testing.T) { - signer := NewSigner("test-secret-key") + signer := New("test-secret-key") - req := &ImageRequest{ + req := &Request{ SourceHost: "cdn.example.com", SourcePath: "/photos/cat.jpg", SourceQuery: "", - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, + Width: 800, + Height: 600, + Format: "webp", Expires: time.Unix(1704067200, 0), // Fixed timestamp for reproducibility } @@ -31,12 +33,13 @@ func TestSigner_Sign(t *testing.T) { } // Different input should produce different signature - req2 := &ImageRequest{ + req2 := &Request{ SourceHost: "cdn.example.com", SourcePath: "/photos/dog.jpg", // Different path SourceQuery: "", - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, + Width: 800, + Height: 600, + Format: "webp", Expires: time.Unix(1704067200, 0), } @@ -47,21 +50,22 @@ func TestSigner_Sign(t *testing.T) { } func TestSigner_Verify(t *testing.T) { - signer := NewSigner("test-secret-key") + signer := New("test-secret-key") tests := []struct { name string - setup func() *ImageRequest + setup func() *Request wantErr error }{ { name: "valid signature", - setup: func() *ImageRequest { - req := &ImageRequest{ + setup: func() *Request { + req := &Request{ SourceHost: "cdn.example.com", SourcePath: "/photos/cat.jpg", - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, + Width: 800, + Height: 600, + Format: "webp", Expires: time.Now().Add(1 * time.Hour), } req.Signature = signer.Sign(req) @@ -72,42 +76,45 @@ func TestSigner_Verify(t *testing.T) { }, { name: "expired signature", - setup: func() *ImageRequest { - req := &ImageRequest{ + setup: func() *Request { + req := &Request{ SourceHost: "cdn.example.com", SourcePath: "/photos/cat.jpg", - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, + Width: 800, + Height: 600, + Format: "webp", Expires: time.Now().Add(-1 * time.Hour), // Expired } req.Signature = signer.Sign(req) return req }, - wantErr: ErrSignatureExpired, + wantErr: ErrExpired, }, { name: "invalid signature", - setup: func() *ImageRequest { - return &ImageRequest{ + setup: func() *Request { + return &Request{ SourceHost: "cdn.example.com", SourcePath: "/photos/cat.jpg", - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, + Width: 800, + Height: 600, + Format: "webp", Expires: time.Now().Add(1 * time.Hour), Signature: "invalid-signature", } }, - wantErr: ErrSignatureInvalid, + wantErr: ErrInvalid, }, { name: "missing expiration", - setup: func() *ImageRequest { - return &ImageRequest{ + setup: func() *Request { + return &Request{ SourceHost: "cdn.example.com", SourcePath: "/photos/cat.jpg", - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, + Width: 800, + Height: 600, + Format: "webp", Signature: "some-signature", // Expires is zero } @@ -116,12 +123,13 @@ func TestSigner_Verify(t *testing.T) { }, { name: "tampered request", - setup: func() *ImageRequest { - req := &ImageRequest{ + setup: func() *Request { + req := &Request{ SourceHost: "cdn.example.com", SourcePath: "/photos/cat.jpg", - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, + Width: 800, + Height: 600, + Format: "webp", Expires: time.Now().Add(1 * time.Hour), } req.Signature = signer.Sign(req) @@ -130,7 +138,7 @@ func TestSigner_Verify(t *testing.T) { return req }, - wantErr: ErrSignatureInvalid, + wantErr: ErrInvalid, }, } @@ -156,16 +164,17 @@ func TestSigner_Verify(t *testing.T) { // matching on every URL component. No suffix matching, wildcard matching, // or partial matching is supported. func TestSigner_Verify_ExactMatchOnly(t *testing.T) { - signer := NewSigner("test-secret-key") + signer := New("test-secret-key") // Base request that we'll sign, then tamper with individual fields. - baseReq := func() *ImageRequest { - req := &ImageRequest{ + baseReq := func() *Request { + req := &Request{ SourceHost: "cdn.example.com", SourcePath: "/photos/cat.jpg", SourceQuery: "token=abc", - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, + Width: 800, + Height: 600, + Format: "webp", Expires: time.Now().Add(1 * time.Hour), } req.Signature = signer.Sign(req) @@ -175,95 +184,95 @@ func TestSigner_Verify_ExactMatchOnly(t *testing.T) { tests := []struct { name string - tamper func(req *ImageRequest) + tamper func(req *Request) }{ { name: "parent domain does not match subdomain", - tamper: func(req *ImageRequest) { + tamper: func(req *Request) { // Signed for cdn.example.com, try example.com req.SourceHost = "example.com" }, }, { name: "subdomain does not match parent domain", - tamper: func(req *ImageRequest) { + tamper: func(req *Request) { // Signed for cdn.example.com, try images.cdn.example.com req.SourceHost = "images.cdn.example.com" }, }, { name: "sibling subdomain does not match", - tamper: func(req *ImageRequest) { + tamper: func(req *Request) { // Signed for cdn.example.com, try images.example.com req.SourceHost = "images.example.com" }, }, { name: "host with suffix appended does not match", - tamper: func(req *ImageRequest) { + tamper: func(req *Request) { // Signed for cdn.example.com, try cdn.example.com.evil.com req.SourceHost = "cdn.example.com.evil.com" }, }, { name: "host with prefix does not match", - tamper: func(req *ImageRequest) { + tamper: func(req *Request) { // Signed for cdn.example.com, try evilcdn.example.com req.SourceHost = "evilcdn.example.com" }, }, { name: "different path does not match", - tamper: func(req *ImageRequest) { + tamper: func(req *Request) { req.SourcePath = "/photos/dog.jpg" }, }, { name: "path suffix does not match", - tamper: func(req *ImageRequest) { + tamper: func(req *Request) { req.SourcePath = "/photos/cat.jpg/extra" }, }, { name: "path prefix does not match", - tamper: func(req *ImageRequest) { + tamper: func(req *Request) { req.SourcePath = "/other/photos/cat.jpg" }, }, { name: "different query does not match", - tamper: func(req *ImageRequest) { + tamper: func(req *Request) { req.SourceQuery = "token=xyz" }, }, { name: "added query does not match empty query", - tamper: func(req *ImageRequest) { + tamper: func(req *Request) { req.SourceQuery = "extra=1" }, }, { name: "removed query does not match", - tamper: func(req *ImageRequest) { + tamper: func(req *Request) { req.SourceQuery = "" }, }, { name: "different width does not match", - tamper: func(req *ImageRequest) { - req.Size.Width = 801 + tamper: func(req *Request) { + req.Width = 801 }, }, { name: "different height does not match", - tamper: func(req *ImageRequest) { - req.Size.Height = 601 + tamper: func(req *Request) { + req.Height = 601 }, }, { name: "different format does not match", - tamper: func(req *ImageRequest) { - req.Format = FormatPNG + tamper: func(req *Request) { + req.Format = "png" }, }, } @@ -274,8 +283,8 @@ func TestSigner_Verify_ExactMatchOnly(t *testing.T) { tt.tamper(req) err := signer.Verify(req) - if err != ErrSignatureInvalid { - t.Errorf("Verify() = %v, want %v", err, ErrSignatureInvalid) + if err != ErrInvalid { + t.Errorf("Verify() = %v, want %v", err, ErrInvalid) } }) } @@ -293,7 +302,7 @@ func TestSigner_Verify_ExactMatchOnly(t *testing.T) { // string in the signature data, producing different signatures for // suffix-related hosts. func TestSigner_Sign_ExactHostInData(t *testing.T) { - signer := NewSigner("test-secret-key") + signer := New("test-secret-key") hosts := []string{ "cdn.example.com", @@ -306,12 +315,13 @@ func TestSigner_Sign_ExactHostInData(t *testing.T) { sigs := make(map[string]string) for _, host := range hosts { - req := &ImageRequest{ + req := &Request{ SourceHost: host, SourcePath: "/photos/cat.jpg", SourceQuery: "", - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, + Width: 800, + Height: 600, + Format: "webp", Expires: time.Unix(1704067200, 0), } @@ -325,14 +335,15 @@ func TestSigner_Sign_ExactHostInData(t *testing.T) { } func TestSigner_DifferentKeys(t *testing.T) { - signer1 := NewSigner("secret-key-1") - signer2 := NewSigner("secret-key-2") + signer1 := New("secret-key-1") + signer2 := New("secret-key-2") - req := &ImageRequest{ + req := &Request{ SourceHost: "cdn.example.com", SourcePath: "/photos/cat.jpg", - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, + Width: 800, + Height: 600, + Format: "webp", Expires: time.Now().Add(1 * time.Hour), } @@ -345,20 +356,21 @@ func TestSigner_DifferentKeys(t *testing.T) { } // Verify with key 2 should fail - if err := signer2.Verify(req); err != ErrSignatureInvalid { + if err := signer2.Verify(req); err != ErrInvalid { t.Errorf("Verify() with different key should fail, got: %v", err) } } func TestGenerateSignedURL(t *testing.T) { - signer := NewSigner("test-secret-key") + signer := New("test-secret-key") - req := &ImageRequest{ + req := &Request{ SourceHost: "cdn.example.com", SourcePath: "/photos/cat.jpg", SourceQuery: "", - Size: Size{Width: 800, Height: 600}, - Format: FormatWebP, + Width: 800, + Height: 600, + Format: "webp", } ttl := 1 * time.Hour @@ -389,13 +401,14 @@ func TestGenerateSignedURL(t *testing.T) { } func TestGenerateSignedURL_OrigSize(t *testing.T) { - signer := NewSigner("test-secret-key") + signer := New("test-secret-key") - req := &ImageRequest{ + req := &Request{ SourceHost: "cdn.example.com", SourcePath: "/photos/cat.jpg", - Size: Size{Width: 0, Height: 0}, // Original size - Format: FormatPNG, + Width: 0, // Original size + Height: 0, + Format: "png", } path, _, _ := signer.GenerateSignedURL(req, time.Hour) @@ -406,7 +419,57 @@ func TestGenerateSignedURL_OrigSize(t *testing.T) { } } -func TestParseSignatureParams(t *testing.T) { +func TestGenerateSignedURL_WithQueryString(t *testing.T) { + signer := New("test-secret-key-for-testing!") + + req := &Request{ + SourceHost: "cdn.example.com", + SourcePath: "/photos/cat.jpg", + SourceQuery: "token=abc&v=2", + Width: 800, + Height: 600, + Format: "webp", + } + + 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) { + signer := New("test-secret-key-for-testing!") + + req := &Request{ + SourceHost: "cdn.example.com", + SourcePath: "/photos/cat.jpg", + Width: 800, + Height: 600, + Format: "webp", + } + + path, _, _ := signer.GenerateSignedURL(req, time.Hour) + + expected := "/v1/image/cdn.example.com/photos/cat.jpg/800x600.webp" + if path != expected { + t.Errorf("GenerateSignedURL() path = %q, want %q", path, expected) + } +} + +func TestParseParams(t *testing.T) { tests := []struct { name string sig string @@ -439,18 +502,18 @@ func TestParseSignatureParams(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - sig, exp, err := ParseSignatureParams(tt.sig, tt.expStr) + sig, exp, err := ParseParams(tt.sig, tt.expStr) if tt.wantErr { if err == nil { - t.Error("ParseSignatureParams() expected error, got nil") + t.Error("ParseParams() expected error, got nil") } return } if err != nil { - t.Errorf("ParseSignatureParams() unexpected error = %v", err) + t.Errorf("ParseParams() unexpected error = %v", err) return }