From 0ff30713375ba6dfbeca47e2280a26a792a79c4f Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 8 Feb 2026 15:58:32 -0800 Subject: [PATCH 1/3] fix: encode source query in GenerateSignedURL to avoid malformed URLs When a source URL has query parameters, GenerateSignedURL() was embedding a bare '?' in the path, causing everything after it to be parsed as the HTTP query string instead of as path segments. This made the size/format segment unreachable by the URL parser. Percent-encode the query string in the path segment so it remains part of the path and can be correctly extracted by ParseImagePath. Fixes #2 --- internal/imgcache/signature.go | 29 ++++++------ internal/imgcache/signature_query_test.go | 55 +++++++++++++++++++++++ 2 files changed, 71 insertions(+), 13 deletions(-) create mode 100644 internal/imgcache/signature_query_test.go diff --git a/internal/imgcache/signature.go b/internal/imgcache/signature.go index 892c496..1795b1f 100644 --- a/internal/imgcache/signature.go +++ b/internal/imgcache/signature.go @@ -6,6 +6,7 @@ import ( "encoding/base64" "errors" "fmt" + "net/url" "strconv" "time" ) @@ -96,22 +97,24 @@ func (s *Signer) GenerateSignedURL(req *ImageRequest, ttl time.Duration) (path s sizeStr = fmt.Sprintf("%dx%d", req.Size.Width, req.Size.Height) } - // Build the path - path = fmt.Sprintf("/v1/image/%s%s/%s.%s", - req.SourceHost, - req.SourcePath, - sizeStr, - req.Format, - ) - - // Append query if present + // 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 + // 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. if req.SourceQuery != "" { - // The query goes before the size segment in our URL scheme - // So we need to rebuild the path - path = fmt.Sprintf("/v1/image/%s%s?%s/%s.%s", + path = fmt.Sprintf("/v1/image/%s%s%%3F%s/%s.%s", + req.SourceHost, + req.SourcePath, + url.PathEscape(req.SourceQuery), + sizeStr, + req.Format, + ) + } else { + path = fmt.Sprintf("/v1/image/%s%s/%s.%s", req.SourceHost, req.SourcePath, - req.SourceQuery, sizeStr, req.Format, ) diff --git a/internal/imgcache/signature_query_test.go b/internal/imgcache/signature_query_test.go new file mode 100644 index 0000000..da05d6f --- /dev/null +++ b/internal/imgcache/signature_query_test.go @@ -0,0 +1,55 @@ +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) + } +} From e3b346e88133f5d4c12c42fb60e458167bc34412 Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 8 Feb 2026 15:59:27 -0800 Subject: [PATCH 2/3] fix: correct Stats() to scan only hit/miss counts, compute HitRate properly Stats() was scanning 5 SQL columns (hit_count, miss_count, upstream_fetch_count, upstream_fetch_bytes, transform_count) into mismatched struct fields, causing HitRate to contain the integer transform_count instead of a 0.0-1.0 ratio. Simplify the query to only fetch hit_count and miss_count, then compute TotalItems, TotalSizeBytes, and HitRate correctly. Fixes #4 --- internal/imgcache/cache.go | 8 +-- internal/imgcache/stats_test.go | 90 +++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 3 deletions(-) create mode 100644 internal/imgcache/stats_test.go diff --git a/internal/imgcache/cache.go b/internal/imgcache/cache.go index 6d5d4dc..f644fa7 100644 --- a/internal/imgcache/cache.go +++ b/internal/imgcache/cache.go @@ -297,19 +297,21 @@ func (c *Cache) CleanExpired(ctx context.Context) error { func (c *Cache) Stats(ctx context.Context) (*CacheStats, error) { var stats CacheStats + // Fetch hit/miss counts from the stats table err := c.db.QueryRowContext(ctx, ` - SELECT hit_count, miss_count, upstream_fetch_count, upstream_fetch_bytes, transform_count + SELECT hit_count, miss_count FROM cache_stats WHERE id = 1 - `).Scan(&stats.HitCount, &stats.MissCount, &stats.TotalItems, &stats.TotalSizeBytes, &stats.HitRate) + `).Scan(&stats.HitCount, &stats.MissCount) if err != nil && !errors.Is(err, sql.ErrNoRows) { return nil, fmt.Errorf("failed to get cache stats: %w", err) } - // Get actual counts + // Get actual item count and total size from content tables _ = c.db.QueryRowContext(ctx, `SELECT COUNT(*) FROM request_cache`).Scan(&stats.TotalItems) _ = c.db.QueryRowContext(ctx, `SELECT COALESCE(SUM(size_bytes), 0) FROM output_content`).Scan(&stats.TotalSizeBytes) + // Compute hit rate as a ratio if stats.HitCount+stats.MissCount > 0 { stats.HitRate = float64(stats.HitCount) / float64(stats.HitCount+stats.MissCount) } diff --git a/internal/imgcache/stats_test.go b/internal/imgcache/stats_test.go new file mode 100644 index 0000000..5b36a3d --- /dev/null +++ b/internal/imgcache/stats_test.go @@ -0,0 +1,90 @@ +package imgcache + +import ( + "context" + "database/sql" + "math" + "testing" + "time" + + "sneak.berlin/go/pixa/internal/database" +) + +func setupStatsTestDB(t *testing.T) *sql.DB { + t.Helper() + db, err := sql.Open("sqlite", ":memory:") + if err != nil { + t.Fatal(err) + } + if err := database.ApplyMigrations(db); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { db.Close() }) + return db +} + +func TestStats_HitRateIsRatio(t *testing.T) { + db := setupStatsTestDB(t) + dir := t.TempDir() + + cache, err := NewCache(db, CacheConfig{ + StateDir: dir, + CacheTTL: time.Hour, + NegativeTTL: 5 * time.Minute, + }) + if err != nil { + t.Fatal(err) + } + + ctx := context.Background() + + // Set some hit/miss counts and a transform_count + _, err = db.ExecContext(ctx, ` + UPDATE cache_stats SET hit_count = 75, miss_count = 25, transform_count = 9999 WHERE id = 1 + `) + if err != nil { + t.Fatal(err) + } + + stats, err := cache.Stats(ctx) + if err != nil { + t.Fatal(err) + } + + if stats.HitCount != 75 { + t.Errorf("HitCount = %d, want 75", stats.HitCount) + } + if stats.MissCount != 25 { + t.Errorf("MissCount = %d, want 25", stats.MissCount) + } + + // HitRate should be 0.75, NOT 9999 (transform_count) + expectedRate := 0.75 + if math.Abs(stats.HitRate-expectedRate) > 0.001 { + t.Errorf("HitRate = %f, want %f (was it scanning transform_count?)", stats.HitRate, expectedRate) + } +} + +func TestStats_ZeroCounts(t *testing.T) { + db := setupStatsTestDB(t) + dir := t.TempDir() + + cache, err := NewCache(db, CacheConfig{ + StateDir: dir, + CacheTTL: time.Hour, + NegativeTTL: 5 * time.Minute, + }) + if err != nil { + t.Fatal(err) + } + + stats, err := cache.Stats(context.Background()) + if err != nil { + t.Fatal(err) + } + + // With zero hits and misses, HitRate should be 0, not some garbage value + if stats.HitRate != 0.0 { + t.Errorf("HitRate = %f, want 0.0 for zero counts", stats.HitRate) + } +} From 79ceed2ee49071ac8c246cfcf5042653083a5779 Mon Sep 17 00:00:00 2001 From: clawbot Date: Sun, 8 Feb 2026 15:59:51 -0800 Subject: [PATCH 3/3] fix: guard against division by zero when fetchBytes is 0 processAndStore() computed sizePercent as outputSize/fetchBytes*100 without checking for zero, producing Inf/NaN in logs and metrics. Also treat empty cached source data the same as missing (re-fetch from upstream) since zero-byte images can't be processed. Fixes #5 --- internal/imgcache/divzero_test.go | 43 +++++++++++++++++++++++++++++++ internal/imgcache/service.go | 10 ++++--- 2 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 internal/imgcache/divzero_test.go diff --git a/internal/imgcache/divzero_test.go b/internal/imgcache/divzero_test.go new file mode 100644 index 0000000..160d480 --- /dev/null +++ b/internal/imgcache/divzero_test.go @@ -0,0 +1,43 @@ +package imgcache + +import ( + "fmt" + "math" + "testing" +) + +func TestSizePercentSafeWithZeroFetchBytes(t *testing.T) { + // Simulate the calculation from processAndStore + fetchBytes := int64(0) + outputSize := int64(100) + + var sizePercent float64 + if fetchBytes > 0 { + sizePercent = float64(outputSize) / float64(fetchBytes) * 100.0 + } + + // sizePercent should be 0, not Inf or NaN + if math.IsInf(sizePercent, 0) || math.IsNaN(sizePercent) { + t.Errorf("sizePercent = %f, should not be Inf/NaN", sizePercent) + } + + // Should produce valid log output + result := fmt.Sprintf("%.1f%%", sizePercent) + if result != "0.0%" { + t.Errorf("formatted size ratio = %q, want %q", result, "0.0%") + } +} + +func TestSizePercentNormalCase(t *testing.T) { + fetchBytes := int64(1000) + outputSize := int64(500) + + var sizePercent float64 + if fetchBytes > 0 { + sizePercent = float64(outputSize) / float64(fetchBytes) * 100.0 + } + + if math.Abs(sizePercent-50.0) > 0.001 { + t.Errorf("sizePercent = %f, want 50.0", sizePercent) + } +} diff --git a/internal/imgcache/service.go b/internal/imgcache/service.go index d3a5c15..7734f96 100644 --- a/internal/imgcache/service.go +++ b/internal/imgcache/service.go @@ -153,8 +153,8 @@ func (s *Service) processFromSourceOrFetch( } } - // Fetch from upstream if we don't have source data - if sourceData == nil { + // Fetch from upstream if we don't have source data or it's empty + if len(sourceData) == 0 { resp, err := s.fetchAndProcess(ctx, req, cacheKey) if err != nil { return nil, err @@ -264,7 +264,11 @@ func (s *Service) processAndStore( // Log conversion details outputSize := int64(len(processedData)) - sizePercent := float64(outputSize) / float64(fetchBytes) * 100.0 //nolint:mnd // percentage calculation + + var sizePercent float64 + if fetchBytes > 0 { + sizePercent = float64(outputSize) / float64(fetchBytes) * 100.0 //nolint:mnd // percentage calculation + } s.log.Info("image converted", "host", req.SourceHost,