diff --git a/internal/gcscache/gcscache.go b/internal/gcscache/gcscache.go index 0de848b1e..9202d683d 100644 --- a/internal/gcscache/gcscache.go +++ b/internal/gcscache/gcscache.go @@ -19,8 +19,46 @@ import ( var ctx = context.Background() +// objectHandle interface wraps the methods we use from storage.ObjectHandle +type objectHandle interface { + NewReader(ctx context.Context) (io.ReadCloser, error) + NewWriter(ctx context.Context) io.WriteCloser + Delete(ctx context.Context) error +} + +// bucketHandle interface wraps the methods we use from storage.BucketHandle +type bucketHandle interface { + Object(name string) objectHandle +} + +// gcsObjectHandle wraps storage.ObjectHandle to implement our interface +type gcsObjectHandle struct { + *storage.ObjectHandle +} + +func (o *gcsObjectHandle) NewReader(ctx context.Context) (io.ReadCloser, error) { + return o.ObjectHandle.NewReader(ctx) +} + +func (o *gcsObjectHandle) NewWriter(ctx context.Context) io.WriteCloser { + return o.ObjectHandle.NewWriter(ctx) +} + +func (o *gcsObjectHandle) Delete(ctx context.Context) error { + return o.ObjectHandle.Delete(ctx) +} + +// gcsBucketHandle wraps storage.BucketHandle to implement our interface +type gcsBucketHandle struct { + *storage.BucketHandle +} + +func (b *gcsBucketHandle) Object(name string) objectHandle { + return &gcsObjectHandle{b.BucketHandle.Object(name)} +} + type cache struct { - bucket *storage.BucketHandle + bucket bucketHandle prefix string } @@ -40,6 +78,12 @@ func (c *cache) Get(key string) ([]byte, bool) { return nil, false } + // Treat empty objects as cache misses to prevent cache poisoning + // when objects are deleted by lifecycle rules + if len(value) == 0 { + return nil, false + } + return value, true } @@ -59,7 +103,7 @@ func (c *cache) Delete(key string) { } } -func (c *cache) object(key string) *storage.ObjectHandle { +func (c *cache) object(key string) objectHandle { name := path.Join(c.prefix, keyToFilename(key)) return c.bucket.Object(name) } @@ -70,6 +114,14 @@ func keyToFilename(key string) string { return hex.EncodeToString(h.Sum(nil)) } +// NewWithBucket constructs a cache using the provided bucket handle +func NewWithBucket(bucket bucketHandle, prefix string) *cache { + return &cache{ + bucket: bucket, + prefix: prefix, + } +} + // New constructs a Cache storing files in the specified GCS bucket. If prefix // is not empty, objects will be prefixed with that path. Credentials should // be specified using one of the mechanisms supported for Application Default @@ -82,6 +134,6 @@ func New(bucket, prefix string) (*cache, error) { return &cache{ prefix: prefix, - bucket: client.Bucket(bucket), + bucket: &gcsBucketHandle{client.Bucket(bucket)}, }, nil } diff --git a/internal/gcscache/gcscache_test.go b/internal/gcscache/gcscache_test.go new file mode 100644 index 000000000..ccce831a0 --- /dev/null +++ b/internal/gcscache/gcscache_test.go @@ -0,0 +1,360 @@ +// Copyright 2013 The imageproxy authors. +// SPDX-License-Identifier: Apache-2.0 + +package gcscache + +import ( + "bytes" + "context" + "errors" + "io" + "testing" + + "cloud.google.com/go/storage" +) + +// mockObjectHandle implements objectHandle for testing +type mockObjectHandle struct { + data []byte + exists bool + readErr error + writeErr error + deleteErr error + writeData *bytes.Buffer +} + +func (m *mockObjectHandle) NewReader(ctx context.Context) (io.ReadCloser, error) { + if m.readErr != nil { + return nil, m.readErr + } + if !m.exists { + return nil, storage.ErrObjectNotExist + } + return io.NopCloser(bytes.NewReader(m.data)), nil +} + +func (m *mockObjectHandle) NewWriter(ctx context.Context) io.WriteCloser { + if m.writeData == nil { + m.writeData = &bytes.Buffer{} + } + return &mockWriter{buf: m.writeData, err: m.writeErr} +} + +func (m *mockObjectHandle) Delete(ctx context.Context) error { + if m.deleteErr != nil { + return m.deleteErr + } + m.exists = false + return nil +} + +// mockWriter implements io.WriteCloser for testing +type mockWriter struct { + buf *bytes.Buffer + err error +} + +func (w *mockWriter) Write(p []byte) (n int, err error) { + if w.err != nil { + return 0, w.err + } + return w.buf.Write(p) +} + +func (w *mockWriter) Close() error { + return w.err +} + +// mockBucketHandle implements bucketHandle for testing +type mockBucketHandle struct { + objects map[string]objectHandle +} + +func (b *mockBucketHandle) Object(name string) objectHandle { + if b.objects == nil { + b.objects = make(map[string]objectHandle) + } + if obj, exists := b.objects[name]; exists { + return obj + } + // Create a new mock object + obj := &mockObjectHandle{exists: false} + b.objects[name] = obj + return obj +} + +func TestCacheGetEmptyObject(t *testing.T) { + bucket := &mockBucketHandle{ + objects: map[string]objectHandle{ + "test-prefix/" + keyToFilename("empty-key"): &mockObjectHandle{ + data: []byte{}, + exists: true, + }, + }, + } + + c := NewWithBucket(bucket, "test-prefix") + + // Test that empty objects are treated as cache misses + data, ok := c.Get("empty-key") + if data != nil { + t.Errorf("Get returned non-nil data for empty object") + } + if ok { + t.Errorf("Get returned ok = true for empty object, expected false") + } +} + +func TestCacheGetNonEmptyObject(t *testing.T) { + testData := []byte("test image data") + bucket := &mockBucketHandle{ + objects: map[string]objectHandle{ + "test-prefix/" + keyToFilename("test-key"): &mockObjectHandle{ + data: testData, + exists: true, + }, + }, + } + + c := NewWithBucket(bucket, "test-prefix") + + // Test that non-empty objects are returned correctly + data, ok := c.Get("test-key") + if !bytes.Equal(data, testData) { + t.Errorf("Get returned incorrect data, got %v, want %v", data, testData) + } + if !ok { + t.Errorf("Get returned ok = false for existing object, expected true") + } +} + +func TestCacheGetMissingObject(t *testing.T) { + bucket := &mockBucketHandle{ + objects: map[string]objectHandle{ + "test-prefix/" + keyToFilename("missing-key"): &mockObjectHandle{ + exists: false, + }, + }, + } + + c := NewWithBucket(bucket, "test-prefix") + + // Test that missing objects return cache miss + data, ok := c.Get("missing-key") + if data != nil { + t.Errorf("Get returned non-nil data for missing object") + } + if ok { + t.Errorf("Get returned ok = true for missing object, expected false") + } +} + +func TestCacheGetError(t *testing.T) { + bucket := &mockBucketHandle{ + objects: map[string]objectHandle{ + "test-prefix/" + keyToFilename("error-key"): &mockObjectHandle{ + readErr: errors.New("read error"), + }, + }, + } + + c := NewWithBucket(bucket, "test-prefix") + + // Test that read errors return cache miss + data, ok := c.Get("error-key") + if data != nil { + t.Errorf("Get returned non-nil data for error case") + } + if ok { + t.Errorf("Get returned ok = true for error case, expected false") + } +} + +func TestCacheGetReadError(t *testing.T) { + // Create a custom mock that returns a reader that fails + bucket := &mockBucketHandle{ + objects: map[string]objectHandle{ + "test-prefix/" + keyToFilename("read-error-key"): &mockObjectHandleWithReadError{}, + }, + } + + c := NewWithBucket(bucket, "test-prefix") + + data, ok := c.Get("read-error-key") + if data != nil { + t.Errorf("Get returned non-nil data for read error") + } + if ok { + t.Errorf("Get returned ok = true for read error, expected false") + } +} + +// mockObjectHandleWithReadError implements objectHandle with a failing reader +type mockObjectHandleWithReadError struct{} + +func (m *mockObjectHandleWithReadError) NewReader(ctx context.Context) (io.ReadCloser, error) { + return io.NopCloser(&errorReader{}), nil +} + +func (m *mockObjectHandleWithReadError) NewWriter(ctx context.Context) io.WriteCloser { + return &mockWriter{buf: &bytes.Buffer{}} +} + +func (m *mockObjectHandleWithReadError) Delete(ctx context.Context) error { + return nil +} + +// mockObjectHandleWithCloseError implements objectHandle with a writer that fails on close +type mockObjectHandleWithCloseError struct{} + +func (m *mockObjectHandleWithCloseError) NewReader(ctx context.Context) (io.ReadCloser, error) { + return nil, storage.ErrObjectNotExist +} + +func (m *mockObjectHandleWithCloseError) NewWriter(ctx context.Context) io.WriteCloser { + return &mockWriter{buf: &bytes.Buffer{}, err: errors.New("close error")} +} + +func (m *mockObjectHandleWithCloseError) Delete(ctx context.Context) error { + return nil +} + +type errorReader struct{} + +func (e *errorReader) Read(p []byte) (n int, err error) { + return 0, errors.New("read error") +} + +func TestCacheSet(t *testing.T) { + bucket := &mockBucketHandle{ + objects: make(map[string]objectHandle), + } + + c := NewWithBucket(bucket, "test-prefix") + + testData := []byte("test image data") + c.Set("test-key", testData) + + // Verify data was written + expectedKey := "test-prefix/" + keyToFilename("test-key") + obj := bucket.objects[expectedKey] + mockObj, ok := obj.(*mockObjectHandle) + if !ok || mockObj == nil || mockObj.writeData == nil { + t.Fatalf("Set did not create object") + } + if !bytes.Equal(mockObj.writeData.Bytes(), testData) { + t.Errorf("Set did not write correct data, got %v, want %v", mockObj.writeData.Bytes(), testData) + } +} + +func TestCacheSetWriteError(t *testing.T) { + bucket := &mockBucketHandle{ + objects: map[string]objectHandle{ + "test-prefix/" + keyToFilename("test-key"): &mockObjectHandle{ + writeErr: errors.New("write error"), + }, + }, + } + + c := NewWithBucket(bucket, "test-prefix") + + // Should not panic on error, just log it + c.Set("test-key", []byte("data")) +} + +func TestCacheSetCloseError(t *testing.T) { + bucket := &mockBucketHandle{ + objects: make(map[string]objectHandle), + } + + // Create object that will fail on close + bucket.objects["test-prefix/"+keyToFilename("test-key")] = &mockObjectHandleWithCloseError{} + + c := NewWithBucket(bucket, "test-prefix") + + // Should not panic on error, just log it + c.Set("test-key", []byte("data")) +} + +func TestCacheDelete(t *testing.T) { + bucket := &mockBucketHandle{ + objects: map[string]objectHandle{ + "test-prefix/" + keyToFilename("test-key"): &mockObjectHandle{ + exists: true, + data: []byte("test data"), + }, + }, + } + + c := NewWithBucket(bucket, "test-prefix") + + c.Delete("test-key") + + // Verify object was marked as deleted + expectedKey := "test-prefix/" + keyToFilename("test-key") + obj := bucket.objects[expectedKey] + mockObj, ok := obj.(*mockObjectHandle) + if !ok || mockObj.exists { + t.Errorf("Delete did not mark object as deleted") + } +} + +func TestCacheDeleteError(t *testing.T) { + bucket := &mockBucketHandle{ + objects: map[string]objectHandle{ + "test-prefix/" + keyToFilename("test-key"): &mockObjectHandle{ + deleteErr: errors.New("delete error"), + }, + }, + } + + c := NewWithBucket(bucket, "test-prefix") + + // Should not panic on error, just log it + c.Delete("test-key") +} + +func TestCacheWithoutPrefix(t *testing.T) { + bucket := &mockBucketHandle{ + objects: map[string]objectHandle{ + keyToFilename("no-prefix-key"): &mockObjectHandle{ + data: []byte("data"), + exists: true, + }, + }, + } + + c := NewWithBucket(bucket, "") + + data, ok := c.Get("no-prefix-key") + if !bytes.Equal(data, []byte("data")) { + t.Errorf("Get with no prefix returned incorrect data") + } + if !ok { + t.Errorf("Get with no prefix returned ok = false, expected true") + } +} + +func TestKeyToFilename(t *testing.T) { + // Test that keyToFilename produces consistent results + key1 := "test-key-1" + key2 := "test-key-2" + + filename1 := keyToFilename(key1) + filename2 := keyToFilename(key2) + + // Same key should produce same filename + if keyToFilename(key1) != filename1 { + t.Errorf("keyToFilename not consistent for same key") + } + + // Different keys should produce different filenames + if filename1 == filename2 { + t.Errorf("keyToFilename produced same filename for different keys") + } + + // Filename should be hex encoded MD5 + if len(filename1) != 32 { // MD5 produces 16 bytes = 32 hex chars + t.Errorf("keyToFilename produced unexpected length: %d", len(filename1)) + } +} diff --git a/internal/s3cache/s3cache.go b/internal/s3cache/s3cache.go index 4ec77f4d0..47c2145b4 100644 --- a/internal/s3cache/s3cache.go +++ b/internal/s3cache/s3cache.go @@ -20,10 +20,11 @@ import ( "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/s3" + "github.com/aws/aws-sdk-go/service/s3/s3iface" ) type cache struct { - *s3.S3 + s3 s3iface.S3API bucket, prefix string } @@ -34,7 +35,7 @@ func (c *cache) Get(key string) ([]byte, bool) { Key: &key, } - resp, err := c.GetObject(input) + resp, err := c.s3.GetObject(input) if err != nil { var aerr awserr.Error if errors.As(err, &aerr) && aerr.Code() != "NoSuchKey" { @@ -49,6 +50,12 @@ func (c *cache) Get(key string) ([]byte, bool) { return nil, false } + // Treat empty objects as cache misses to prevent cache poisoning + // when objects are deleted by lifecycle rules + if len(value) == 0 { + return nil, false + } + return value, true } func (c *cache) Set(key string, value []byte) { @@ -59,7 +66,7 @@ func (c *cache) Set(key string, value []byte) { Key: &key, } - _, err := c.PutObject(input) + _, err := c.s3.PutObject(input) if err != nil { log.Printf("error writing to s3: %v", err) } @@ -71,7 +78,7 @@ func (c *cache) Delete(key string) { Key: &key, } - _, err := c.DeleteObject(input) + _, err := c.s3.DeleteObject(input) if err != nil { log.Printf("error deleting from s3: %v", err) } @@ -83,6 +90,15 @@ func keyToFilename(key string) string { return hex.EncodeToString(h.Sum(nil)) } +// NewWithClient constructs a cache using the provided S3 client +func NewWithClient(s3Client s3iface.S3API, bucket, prefix string) *cache { + return &cache{ + s3: s3Client, + bucket: bucket, + prefix: prefix, + } +} + // New constructs a cache configured using the provided URL string. URL should // be of the form: "s3://region/bucket/optional-path-prefix". Credentials // should be specified using one of the mechanisms supported by aws-sdk-go (see @@ -121,7 +137,7 @@ func New(s string) (*cache, error) { } return &cache{ - S3: s3.New(sess), + s3: s3.New(sess), bucket: bucket, prefix: prefix, }, nil diff --git a/internal/s3cache/s3cache_test.go b/internal/s3cache/s3cache_test.go new file mode 100644 index 000000000..53d1d16a0 --- /dev/null +++ b/internal/s3cache/s3cache_test.go @@ -0,0 +1,319 @@ +// Copyright 2013 The imageproxy authors. +// SPDX-License-Identifier: Apache-2.0 + +package s3cache + +import ( + "bytes" + "errors" + "io" + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/awserr" + "github.com/aws/aws-sdk-go/service/s3" + "github.com/aws/aws-sdk-go/service/s3/s3iface" +) + +// mockS3Client implements s3iface.S3API for testing +type mockS3Client struct { + s3iface.S3API + objects map[string][]byte + getErr error + putErr error + delErr error +} + +func (m *mockS3Client) GetObject(input *s3.GetObjectInput) (*s3.GetObjectOutput, error) { + if m.getErr != nil { + return nil, m.getErr + } + + key := aws.StringValue(input.Key) + data, exists := m.objects[key] + if !exists { + return nil, awserr.New("NoSuchKey", "The specified key does not exist.", nil) + } + + return &s3.GetObjectOutput{ + Body: io.NopCloser(bytes.NewReader(data)), + }, nil +} + +func (m *mockS3Client) PutObject(input *s3.PutObjectInput) (*s3.PutObjectOutput, error) { + if m.putErr != nil { + return nil, m.putErr + } + + if m.objects == nil { + m.objects = make(map[string][]byte) + } + + key := aws.StringValue(input.Key) + data, _ := io.ReadAll(input.Body) + m.objects[key] = data + + return &s3.PutObjectOutput{}, nil +} + +func (m *mockS3Client) DeleteObject(input *s3.DeleteObjectInput) (*s3.DeleteObjectOutput, error) { + if m.delErr != nil { + return nil, m.delErr + } + + key := aws.StringValue(input.Key) + delete(m.objects, key) + + return &s3.DeleteObjectOutput{}, nil +} + +func TestCacheGetEmptyObject(t *testing.T) { + mock := &mockS3Client{ + objects: map[string][]byte{ + "test-prefix/" + keyToFilename("empty-key"): []byte{}, + }, + } + + c := NewWithClient(mock, "test-bucket", "test-prefix") + + // Test that empty objects are treated as cache misses + data, ok := c.Get("empty-key") + if data != nil { + t.Errorf("Get returned non-nil data for empty object") + } + if ok { + t.Errorf("Get returned ok = true for empty object, expected false") + } +} + +func TestCacheGetNonEmptyObject(t *testing.T) { + testData := []byte("test image data") + mock := &mockS3Client{ + objects: map[string][]byte{ + "test-prefix/" + keyToFilename("test-key"): testData, + }, + } + + c := NewWithClient(mock, "test-bucket", "test-prefix") + + // Test that non-empty objects are returned correctly + data, ok := c.Get("test-key") + if !bytes.Equal(data, testData) { + t.Errorf("Get returned incorrect data, got %v, want %v", data, testData) + } + if !ok { + t.Errorf("Get returned ok = false for existing object, expected true") + } +} + +func TestCacheGetMissingObject(t *testing.T) { + mock := &mockS3Client{ + objects: make(map[string][]byte), + } + + c := NewWithClient(mock, "test-bucket", "test-prefix") + + // Test that missing objects return cache miss + data, ok := c.Get("missing-key") + if data != nil { + t.Errorf("Get returned non-nil data for missing object") + } + if ok { + t.Errorf("Get returned ok = true for missing object, expected false") + } +} + +func TestCacheGetError(t *testing.T) { + mock := &mockS3Client{ + getErr: awserr.New("InternalError", "Internal server error", nil), + } + + c := NewWithClient(mock, "test-bucket", "test-prefix") + + // Test that S3 errors (other than NoSuchKey) return cache miss + data, ok := c.Get("error-key") + if data != nil { + t.Errorf("Get returned non-nil data for error case") + } + if ok { + t.Errorf("Get returned ok = true for error case, expected false") + } +} + +func TestCacheGetReadError(t *testing.T) { + // Create a custom mock that returns a reader that fails + mock := &mockS3ClientWithReadError{} + + c := NewWithClient(mock, "test-bucket", "test-prefix") + + data, ok := c.Get("read-error-key") + if data != nil { + t.Errorf("Get returned non-nil data for read error") + } + if ok { + t.Errorf("Get returned ok = true for read error, expected false") + } +} + +// mockS3ClientWithReadError implements s3iface.S3API with a failing reader +type mockS3ClientWithReadError struct { + s3iface.S3API +} + +func (m *mockS3ClientWithReadError) GetObject(input *s3.GetObjectInput) (*s3.GetObjectOutput, error) { + return &s3.GetObjectOutput{ + Body: io.NopCloser(&errorReader{}), + }, nil +} + +type errorReader struct{} + +func (e *errorReader) Read(p []byte) (n int, err error) { + return 0, errors.New("read error") +} + +func TestCacheSet(t *testing.T) { + mock := &mockS3Client{ + objects: make(map[string][]byte), + } + + c := NewWithClient(mock, "test-bucket", "test-prefix") + + testData := []byte("test image data") + c.Set("test-key", testData) + + // Verify data was stored + expectedKey := "test-prefix/" + keyToFilename("test-key") + storedData := mock.objects[expectedKey] + if !bytes.Equal(storedData, testData) { + t.Errorf("Set did not store correct data, got %v, want %v", storedData, testData) + } +} + +func TestCacheSetError(t *testing.T) { + mock := &mockS3Client{ + putErr: errors.New("put error"), + } + + c := NewWithClient(mock, "test-bucket", "test-prefix") + + // Should not panic on error, just log it + c.Set("test-key", []byte("data")) +} + +func TestCacheDelete(t *testing.T) { + mock := &mockS3Client{ + objects: map[string][]byte{ + "test-prefix/" + keyToFilename("test-key"): []byte("test data"), + }, + } + + c := NewWithClient(mock, "test-bucket", "test-prefix") + + c.Delete("test-key") + + // Verify data was deleted + expectedKey := "test-prefix/" + keyToFilename("test-key") + if _, exists := mock.objects[expectedKey]; exists { + t.Errorf("Delete did not remove object from cache") + } +} + +func TestCacheDeleteError(t *testing.T) { + mock := &mockS3Client{ + delErr: errors.New("delete error"), + } + + c := NewWithClient(mock, "test-bucket", "test-prefix") + + // Should not panic on error, just log it + c.Delete("test-key") +} + +func TestCacheWithoutPrefix(t *testing.T) { + mock := &mockS3Client{ + objects: map[string][]byte{ + keyToFilename("no-prefix-key"): []byte("data"), + }, + } + + c := NewWithClient(mock, "test-bucket", "") + + data, ok := c.Get("no-prefix-key") + if !bytes.Equal(data, []byte("data")) { + t.Errorf("Get with no prefix returned incorrect data") + } + if !ok { + t.Errorf("Get with no prefix returned ok = false, expected true") + } +} + +func TestKeyToFilename(t *testing.T) { + // Test that keyToFilename produces consistent results + key1 := "test-key-1" + key2 := "test-key-2" + + filename1 := keyToFilename(key1) + filename2 := keyToFilename(key2) + + // Same key should produce same filename + if keyToFilename(key1) != filename1 { + t.Errorf("keyToFilename not consistent for same key") + } + + // Different keys should produce different filenames + if filename1 == filename2 { + t.Errorf("keyToFilename produced same filename for different keys") + } + + // Filename should be hex encoded MD5 + if len(filename1) != 32 { // MD5 produces 16 bytes = 32 hex chars + t.Errorf("keyToFilename produced unexpected length: %d", len(filename1)) + } +} + +func TestNew(t *testing.T) { + tests := []struct { + name string + url string + wantErr bool + bucket string + prefix string + }{ + { + name: "basic URL", + url: "s3://us-east-1/my-bucket", + bucket: "my-bucket", + prefix: "", + }, + { + name: "URL with prefix", + url: "s3://us-west-2/my-bucket/cache/prefix", + bucket: "my-bucket", + prefix: "cache/prefix", + }, + { + name: "invalid URL", + url: "://invalid", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c, err := New(tt.url) + if (err != nil) != tt.wantErr { + t.Errorf("New() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !tt.wantErr { + if c.bucket != tt.bucket { + t.Errorf("New() bucket = %v, want %v", c.bucket, tt.bucket) + } + if c.prefix != tt.prefix { + t.Errorf("New() prefix = %v, want %v", c.prefix, tt.prefix) + } + } + }) + } +}