diff --git a/README.md b/README.md index 250463e..5fb6a49 100644 --- a/README.md +++ b/README.md @@ -419,8 +419,11 @@ HMAC-SHA256(secret, "host:path:query:width:height:format:expiration:quality:fit" Where: - `host` — source origin hostname (e.g. `cdn.example.com`) -- `path` — source path (e.g. `/photos/cat.jpg`) -- `query` — source query string, empty string if none +- `path` — source path (e.g. `/photos/cat.jpg`), with each `%` written as `%25` + and each `:` as `%3A`, so `/photos/a:b.jpg` is signed as `/photos/a%3Ab.jpg` +- `query` — source query string, empty string if none, with `%` and `:` written + as in `path`; neither then holds a `:`, so path `/a:b` with no query and path + `/a` with query `b:` are signed differently - `width` — requested width in pixels, `0` for original - `height` — requested height in pixels, `0` for original - `format` — output format, one of those listed under Routes, with `original` diff --git a/TODO.md b/TODO.md index c046779..7793ef4 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,14 @@ P2: security: per-origin rate limiting # Completed Steps +- 2026-10-15 a `:` in the source path or query no longer moves where the path + ends and the query begins (closes #247): the signed data and the cache key + joined host, path and query with `:` and no escaping, so a URL signed for + `https://h/a:b` verified for `https://h/a?b:`; the two also shared a cache + key, and whichever was fetched first was served for both. In both, the path + and the query now have each `%` written as `%25` and each `:` as `%3A`; a path + and query holding neither are signed and keyed as before. Signature + Specification in `README.md` says so. - 2026-10-15 the example config allowlists no host (closes #249): it allowlisted `github.com` and `user-images.githubusercontent.com`, where anyone can put an image, so a deployment set up as Getting Started says served those images to diff --git a/internal/imgcache/storage.go b/internal/imgcache/storage.go index 395178d..35ff2f1 100644 --- a/internal/imgcache/storage.go +++ b/internal/imgcache/storage.go @@ -10,6 +10,7 @@ import ( "log/slog" "os" "path/filepath" + "strings" "time" ) @@ -371,11 +372,13 @@ func HashPath(path string) PathHash { // CacheKey generates a unique key for a request variant. // Format: sha256(host:path:query:width:height:format:quality:fit_mode) +// The path and query go through escapePercentAndColon, so that a ":" in +// either cannot be taken for the one between them. func CacheKey(req *ImageRequest) VariantKey { data := fmt.Sprintf("%s:%s:%s:%d:%d:%s:%d:%s", req.SourceHost, - req.SourcePath, - req.SourceQuery, + escapePercentAndColon(req.SourcePath), + escapePercentAndColon(req.SourceQuery), req.Size.Width, req.Size.Height, req.Format, @@ -387,6 +390,14 @@ func CacheKey(req *ImageRequest) VariantKey { return VariantKey(hex.EncodeToString(h[:])) } +// escapePercentAndColon returns s with each "%" written as "%25" and each +// ":" as "%3A", so that the result holds no ":". +func escapePercentAndColon(s string) string { + s = strings.ReplaceAll(s, "%", "%25") + + return strings.ReplaceAll(s, ":", "%3A") +} + // VariantStorage handles key-based file storage for processed image variants. // Files are stored at: /// // Metadata is stored at: ///.meta diff --git a/internal/imgcache/storage_internal_test.go b/internal/imgcache/storage_internal_test.go index c57e1ee..1d3996d 100644 --- a/internal/imgcache/storage_internal_test.go +++ b/internal/imgcache/storage_internal_test.go @@ -407,6 +407,62 @@ func TestCacheKey(t *testing.T) { } } +// TestCacheKey_ColonOrPercentInPathOrQuery checks that two different source +// URLs whose path or query holds ":" or "%" get different keys. +func TestCacheKey_ColonOrPercentInPathOrQuery(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + path1, query1 string + path2, query2 string + }{ + { + name: "colon moved from path to query", + path1: "/a:b", query1: "", + path2: "/a", query2: "b:", + }, + { + name: "colon in path and its percent-encoding", + path1: "/a:b", query1: "", + path2: "/a%3Ab", query2: "", + }, + } + + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + key1 := CacheKey(&ImageRequest{ + SourceHost: testHostCDN, + SourcePath: tt.path1, + SourceQuery: tt.query1, + Size: Size{Width: 800, Height: 600}, + Format: FormatWebP, + Quality: 85, + FitMode: FitCover, + }) + key2 := CacheKey(&ImageRequest{ + SourceHost: testHostCDN, + SourcePath: tt.path2, + SourceQuery: tt.query2, + Size: Size{Width: 800, Height: 600}, + Format: FormatWebP, + Quality: 85, + FitMode: FitCover, + }) + + t.Logf("CacheKey() of path %q query %q = %s", tt.path1, tt.query1, key1) + t.Logf("CacheKey() of path %q query %q = %s", tt.path2, tt.query2, key2) + + if key1 == key2 { + t.Errorf("CacheKey() gave path %q query %q and path %q query %q "+ + "the same key", tt.path1, tt.query1, tt.path2, tt.query2) + } + }) + } +} + // TestVariantStorage_StoreLogsFailedMetaWrite verifies that a .meta write // that fails is logged at warn and the store still succeeds. func TestVariantStorage_StoreLogsFailedMetaWrite(t *testing.T) { diff --git a/internal/signature/escaping_test.go b/internal/signature/escaping_test.go new file mode 100644 index 0000000..550805c --- /dev/null +++ b/internal/signature/escaping_test.go @@ -0,0 +1,113 @@ +package signature_test + +import ( + "errors" + "testing" + "time" + + "sneak.berlin/go/pixa/internal/signature" +) + +// TestSigner_Verify_ColonCannotMovePathQueryBoundary checks that a URL +// signed for path "/a:b" with no query does not verify for path "/a" with +// query "b:", a different resource on the same host. +func TestSigner_Verify_ColonCannotMovePathQueryBoundary(t *testing.T) { + t.Parallel() + + signer := signature.New(goldenSigningKey) + + signed := &signature.Request{ + SourceHost: testHost, + SourcePath: "/a:b", + Width: 800, + Height: 600, + Format: testFormatWebP, + Quality: 85, + FitMode: testFitCover, + Expires: time.Now().Add(1 * time.Hour), + } + signed.Signature = signer.Sign(signed) + + replayed := *signed + replayed.SourcePath = "/a" + replayed.SourceQuery = "b:" + + t.Logf("signature for path %q query %q: %s", + signed.SourcePath, signed.SourceQuery, signed.Signature) + t.Logf("signature for path %q query %q: %s", + replayed.SourcePath, replayed.SourceQuery, signer.Sign(&replayed)) + + err := signer.Verify(&replayed) + if !errors.Is(err, signature.ErrInvalid) { + t.Errorf("Verify() of path %q query %q with the signature for "+ + "path %q = %v, want %v", + replayed.SourcePath, replayed.SourceQuery, signed.SourcePath, + err, signature.ErrInvalid) + } +} + +// TestSigner_EscapedGoldenVectors pins the signature of requests whose path +// or query holds ":" or "%", which are signed with "%" written as "%25" and +// ":" as "%3A". The expected values were computed once, outside pixa, from +// the signed data in each comment. +func TestSigner_EscapedGoldenVectors(t *testing.T) { + t.Parallel() + + signer := signature.New(goldenSigningKey) + + vectors := []struct { + name string + path string + query string + wantSignature string + }{ + { + name: "colon in path", + path: "/photos/a:b.jpg", + // Signed data: + // "cdn.example.com:/photos/a%3Ab.jpg::800:600:webp:1704067200:85:cover" + wantSignature: "WnEpZ7bQp1Zvw8WDatlj2vu-4wYKWE3BFBizV8m__6k=", + }, + { + name: "colon in query", + path: testPath, + query: "t=1:2", + // Signed data: + // "cdn.example.com:/photos/cat.jpg:t=1%3A2:800:600:webp:1704067200:85:cover" + wantSignature: "85kRoN8hhr4cCUkEUjNvUsvVYKjYYfWdeffx5TjJjqA=", + }, + { + name: "percent in path and query", + path: "/photos/a%20b.jpg", + query: "q=50%", + // Signed data: + // "cdn.example.com:/photos/a%2520b.jpg:q=50%25:800:600:webp:1704067200:85:cover" + wantSignature: "MjDiiRdgXMrsjqPMsENtD4aG1hp1e-a6zy67vKTX4CQ=", + }, + } + + for _, tt := range vectors { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + req := &signature.Request{ + SourceHost: testHost, + SourcePath: tt.path, + SourceQuery: tt.query, + Width: 800, + Height: 600, + Format: testFormatWebP, + Quality: 85, + FitMode: testFitCover, + Expires: time.Unix(goldenExpiresUnix, 0), + } + + gotSignature := signer.Sign(req) + t.Logf("Sign() of path %q query %q = %s", tt.path, tt.query, gotSignature) + + if gotSignature != tt.wantSignature { + t.Errorf("Sign() = %q, want %q", gotSignature, tt.wantSignature) + } + }) + } +} diff --git a/internal/signature/signature.go b/internal/signature/signature.go index 0b43032..a683e39 100644 --- a/internal/signature/signature.go +++ b/internal/signature/signature.go @@ -10,6 +10,7 @@ import ( "fmt" "net/url" "strconv" + "strings" "time" ) @@ -154,15 +155,17 @@ func (s *Signer) GenerateSignedURL( // buildSignatureData creates the string to be signed. // Format: "host:path:query:width:height:format:expiration:quality:fit" -// All components are used verbatim (exact match). No normalization, -// suffix matching, or wildcard expansion is performed. Quality and fit -// are the effective transform values, so replaying a signed URL with a -// different quality or fit mode fails verification. +// The path and query go through escapePercentAndColon, so that a ":" in +// either cannot be taken for the one between them; every other component +// is used verbatim. No normalization, suffix matching, or wildcard +// expansion is performed. Quality and fit are the effective transform +// values, so replaying a signed URL with a different quality or fit mode +// fails verification. func (s *Signer) buildSignatureData(req *Request) string { return fmt.Sprintf("%s:%s:%s:%d:%d:%s:%d:%d:%s", req.SourceHost, - req.SourcePath, - req.SourceQuery, + escapePercentAndColon(req.SourcePath), + escapePercentAndColon(req.SourceQuery), req.Width, req.Height, req.Format, @@ -172,6 +175,14 @@ func (s *Signer) buildSignatureData(req *Request) string { ) } +// escapePercentAndColon returns s with each "%" written as "%25" and each +// ":" as "%3A", so that the result holds no ":". +func escapePercentAndColon(s string) string { + s = strings.ReplaceAll(s, "%", "%25") + + return strings.ReplaceAll(s, ":", "%3A") +} + // ParseParams extracts signature and expiration from query parameters. func ParseParams(sig, expStr string) (string, time.Time, error) { if expStr == "" {