feat: optional S3-compatible storage for uploaded images, instead of the local upload directory

test/s3_image_storage_http_test.go

@@ -0,0 +1,441 @@
+package test
+
+import (
+	"RenewCMS/api/handlers/staticimages"
+	validator "RenewCMS/api/pkg/validator"
+	domainImage "RenewCMS/internal/domain/image"
+	"RenewCMS/internal/infrastructure/config"
+	"RenewCMS/internal/infrastructure/storage"
+	"RenewCMS/internal/infrastructure/useCases"
+	"bytes"
+	"errors"
+	"log"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/go-chi/chi/v5"
+)
+
+const s3ObjectBody = "0123456789"
+
+func newS3StorageRouter(store domainImage.Storage) *chi.Mux {
+	router := chi.NewRouter()
+	router.Mount("/static/uploadedImages", staticimages.NewHandler(useCases.NewOpenMediaUseCase(store)))
+	return router
+}
+
+func serveS3Storage(router http.Handler, method string, target string, headers map[string]string) *httptest.ResponseRecorder {
+	request := httptest.NewRequest(method, target, nil)
+	for name, value := range headers {
+		request.Header.Set(name, value)
+	}
+	recorder := httptest.NewRecorder()
+	router.ServeHTTP(recorder, request)
+	return recorder
+}
+
+func clearStorageEnv(t *testing.T) {
+	t.Helper()
+	setBaseSettingsEnv(t)
+	for _, name := range []string{"STORAGE_DRIVER", "S3_BUCKET", "S3_REGION", "S3_ENDPOINT", "S3_ACCESS_KEY_ID", "S3_SECRET_ACCESS_KEY", "S3_KEY_PREFIX", "S3_PATH_STYLE", "S3_TLS"} {
+		t.Setenv(name, "")
+	}
+}
+
+func TestS3StorageAcceptance(t *testing.T) {
+	validator.InitValidator()
+
+	t.Run("Startup configuration validation", func(t *testing.T) {
+		examples := []struct {
+			name      string
+			env       map[string]string
+			wantError string
+			check     func(t *testing.T, settings config.Settings)
+		}{
+			{
+				name: "(unset) with no S3 variable",
+				env:  map[string]string{},
+				check: func(t *testing.T, settings config.Settings) {
+					if settings.Storage.Driver != "local" {
+						t.Errorf("driver = %q, want local", settings.Storage.Driver)
+					}
+				},
+			},
+			{
+				name: "local with S3_BUCKET set",
+				env:  map[string]string{"STORAGE_DRIVER": "local", "S3_BUCKET": "media"},
+				check: func(t *testing.T, settings config.Settings) {
+					if settings.Storage.Driver != "local" || settings.Storage.S3.Bucket != "" {
+						t.Errorf("S3 variables must be ignored, got %+v", settings.Storage)
+					}
+				},
+			},
+			{
+				name: "s3 with S3_BUCKET and S3_REGION set",
+				env:  map[string]string{"STORAGE_DRIVER": "s3", "S3_BUCKET": "media", "S3_REGION": "eu-west-3"},
+				check: func(t *testing.T, settings config.Settings) {
+					s3 := settings.Storage.S3
+					if s3.Bucket != "media" || s3.Region != "eu-west-3" || s3.AccessKeyID != "" || s3.SecretAccessKey != "" {
+						t.Errorf("unexpected settings %+v", s3)
+					}
+				},
+			},
+			{
+				name: "s3 with S3_BUCKET set and S3_REGION unset",
+				env:  map[string]string{"STORAGE_DRIVER": "s3", "S3_BUCKET": "media"},
+				check: func(t *testing.T, settings config.Settings) {
+					if settings.Storage.S3.Region != "us-east-1" {
+						t.Errorf("region = %q, want us-east-1", settings.Storage.S3.Region)
+					}
+				},
+			},
+			{
+				name:      "s3 with S3_REGION set and S3_BUCKET unset",
+				env:       map[string]string{"STORAGE_DRIVER": "s3", "S3_REGION": "eu-west-3"},
+				wantError: "Environment variable S3_BUCKET is not set",
+			},
+			{
+				name:      "s3 with key id set and secret unset",
+				env:       map[string]string{"STORAGE_DRIVER": "s3", "S3_BUCKET": "media", "S3_ACCESS_KEY_ID": "id"},
+				wantError: "S3_SECRET_ACCESS_KEY",
+			},
+			{
+				name:      "s3 with secret set and key id unset",
+				env:       map[string]string{"STORAGE_DRIVER": "s3", "S3_BUCKET": "media", "S3_SECRET_ACCESS_KEY": "secret"},
+				wantError: "S3_ACCESS_KEY_ID",
+			},
+			{
+				name: "s3 with UPLOAD_DIR set",
+				env:  map[string]string{"STORAGE_DRIVER": "s3", "S3_BUCKET": "media", "UPLOAD_DIR": "./data/uploads"},
+				check: func(t *testing.T, settings config.Settings) {
+					if settings.Storage.Driver != "s3" {
+						t.Errorf("driver = %q, want s3", settings.Storage.Driver)
+					}
+				},
+			},
+			{
+				name:      "gcs with no variable",
+				env:       map[string]string{"STORAGE_DRIVER": "gcs"},
+				wantError: `unsupported storage driver "gcs"`,
+			},
+		}
+		for _, example := range examples {
+			t.Run(example.name, func(t *testing.T) {
+				clearStorageEnv(t)
+				for name, value := range example.env {
+					t.Setenv(name, value)
+				}
+				settings, err := config.NewSettingsFromEnv()
+				if example.wantError != "" {
+					if err == nil || !strings.Contains(err.Error(), example.wantError) {
+						t.Fatalf("expected error containing %q, got %v", example.wantError, err)
+					}
+					return
+				}
+				if err != nil {
+					t.Fatalf("unexpected error %v", err)
+				}
+				example.check(t, settings)
+			})
+		}
+	})
+
+	t.Run("The registry opens the S3 storage after a bucket check", func(t *testing.T) {
+		fake := newFakeS3Server(t)
+		store, err := storage.Open(storage.Config{Driver: "s3", S3: fake.config("")})
+		if err != nil || store == nil {
+			t.Fatalf("expected a storage and no error, got %v, %v", store, err)
+		}
+	})
+
+	t.Run("Startup fails fast with a clear secret-free error", func(t *testing.T) {
+		for _, status := range []int{http.StatusNotFound, http.StatusForbidden, http.StatusInternalServerError} {
+			fake := newFakeS3Server(t)
+			fake.setBucketStatus(status)
+			_, err := storage.Open(storage.Config{Driver: "s3", S3: fake.config("")})
+			if err == nil {
+				t.Fatalf("status %d: expected an error", status)
+			}
+			if !strings.Contains(err.Error(), s3TestBucket) {
+				t.Errorf("status %d: error must mention the bucket, got %q", status, err.Error())
+			}
+			if strings.Contains(err.Error(), s3TestSecret) {
+				t.Errorf("status %d: error leaks the secret: %q", status, err.Error())
+			}
+		}
+	})
+
+	t.Run("Put stores the object under the prefix with its content type", func(t *testing.T) {
+		fake := newFakeS3Server(t)
+		store := openFakeS3Storage(t, fake, "uploads/")
+		name, err := store.Put("image/png", bytes.NewReader([]byte{1, 2, 3}))
+		if err != nil || !strings.HasSuffix(name, ".png") {
+			t.Fatalf("Put returned %q, %v", name, err)
+		}
+		object, ok := fake.object("uploads/" + name)
+		if !ok || !bytes.Equal(object.data, []byte{1, 2, 3}) || object.contentType != "image/png" {
+			t.Errorf("unexpected stored object %+v (found %v)", object, ok)
+		}
+	})
+
+	t.Run("Put failure is returned as an error", func(t *testing.T) {
+		fake := newFakeS3Server(t)
+		store := openFakeS3Storage(t, fake, "")
+		fake.failMethod(http.MethodPut, http.StatusInternalServerError)
+		_, err := store.Put("image/png", bytes.NewReader([]byte{1}))
+		if err == nil || errors.Is(err, domainImage.ErrBlobNotFound) {
+			t.Errorf("expected a plain error, got %v", err)
+		}
+	})
+
+	t.Run("Open returns content, size and modification time", func(t *testing.T) {
+		fake := newFakeS3Server(t)
+		fake.put("uploads/a1b2.png", []byte(s3ObjectBody), "image/png")
+		store := openFakeS3Storage(t, fake, "uploads/")
+		blob, err := store.Open("a1b2.png")
+		if err != nil {
+			t.Fatalf("Open failed: %v", err)
+		}
+		if blob.Size != 10 || !blob.ModTime.Equal(s3TestModTime) {
+			t.Errorf("unexpected size %d or modtime %v", blob.Size, blob.ModTime)
+		}
+		if got := readAllBlob(t, blob); string(got) != s3ObjectBody {
+			t.Errorf("unexpected content %q", got)
+		}
+	})
+
+	t.Run("Open returns a Content that implements io.ReadSeeker", func(t *testing.T) {
+		fake := newFakeS3Server(t)
+		fake.put("a1b2.png", []byte(s3ObjectBody), "image/png")
+		blob, err := openFakeS3Storage(t, fake, "").Open("a1b2.png")
+		if err != nil {
+			t.Fatalf("Open failed: %v", err)
+		}
+		defer blob.Content.Close()
+		if _, ok := blob.Content.(interface {
+			Seek(int64, int) (int64, error)
+		}); !ok {
+			t.Error("Content must be seekable")
+		}
+	})
+
+	t.Run("Open of a missing key is not found", func(t *testing.T) {
+		fake := newFakeS3Server(t)
+		_, err := openFakeS3Storage(t, fake, "").Open("missing.png")
+		if !errors.Is(err, domainImage.ErrBlobNotFound) {
+			t.Errorf("expected ErrBlobNotFound, got %v", err)
+		}
+	})
+
+	t.Run("Open with a backend failure is not a not-found", func(t *testing.T) {
+		fake := newFakeS3Server(t)
+		store := openFakeS3Storage(t, fake, "")
+		fake.failMethod(http.MethodGet, http.StatusInternalServerError)
+		_, err := store.Open("a1b2.png")
+		if err == nil || errors.Is(err, domainImage.ErrBlobNotFound) {
+			t.Errorf("expected a plain error, got %v", err)
+		}
+	})
+
+	t.Run("Delete is idempotent", func(t *testing.T) {
+		fake := newFakeS3Server(t)
+		fake.put("a1b2.png", []byte(s3ObjectBody), "image/png")
+		store := openFakeS3Storage(t, fake, "")
+		if err := store.Delete("a1b2.png"); err != nil {
+			t.Fatalf("deleting an existing key failed: %v", err)
+		}
+		if _, ok := fake.object("a1b2.png"); ok {
+			t.Error("the key must be gone")
+		}
+		fake.failMethod(http.MethodDelete, http.StatusNotFound)
+		if err := store.Delete("a1b2.png"); err != nil {
+			t.Errorf("deleting a missing key must not fail, got %v", err)
+		}
+	})
+
+	t.Run("A backend failure on Delete is returned", func(t *testing.T) {
+		fake := newFakeS3Server(t)
+		store := openFakeS3Storage(t, fake, "")
+		fake.failMethod(http.MethodDelete, http.StatusInternalServerError)
+		if err := store.Delete("a1b2.png"); err == nil {
+			t.Error("expected an error")
+		}
+	})
+
+	t.Run("Serving /static/uploadedImages/<name> from the bucket", func(t *testing.T) {
+		fake := newFakeS3Server(t)
+		fake.put("a1b2.png", []byte(s3ObjectBody), "image/png")
+		router := newS3StorageRouter(openFakeS3Storage(t, fake, ""))
+		lastModified := s3TestModTime.UTC().Format(http.TimeFormat)
+
+		assertCommonHeaders := func(t *testing.T, recorder *httptest.ResponseRecorder) {
+			t.Helper()
+			header := recorder.Header()
+			if header.Get("Content-Type") != "image/png" || header.Get("Last-Modified") != lastModified ||
+				header.Get("Accept-Ranges") != "bytes" || header.Get("Cache-Control") != "public, no-cache" ||
+				header.Get("X-Content-Type-Options") != "nosniff" || header.Get("Content-Length") != "10" {
+				t.Errorf("unexpected headers %v", header)
+			}
+			if header.Get("ETag") != "" {
+				t.Errorf("no ETag expected, got %q", header.Get("ETag"))
+			}
+		}
+
+		t.Run("GET with no extra header", func(t *testing.T) {
+			recorder := serveS3Storage(router, http.MethodGet, "/static/uploadedImages/a1b2.png", nil)
+			if recorder.Code != http.StatusOK || recorder.Body.String() != s3ObjectBody {
+				t.Fatalf("got %d %q", recorder.Code, recorder.Body.String())
+			}
+			assertCommonHeaders(t, recorder)
+		})
+
+		t.Run("HEAD with no extra header", func(t *testing.T) {
+			recorder := serveS3Storage(router, http.MethodHead, "/static/uploadedImages/a1b2.png", nil)
+			if recorder.Code != http.StatusOK || recorder.Body.Len() != 0 {
+				t.Fatalf("got %d with %d body bytes", recorder.Code, recorder.Body.Len())
+			}
+			assertCommonHeaders(t, recorder)
+		})
+
+		t.Run("GET with If-Modified-Since equal to Last-Modified", func(t *testing.T) {
+			recorder := serveS3Storage(router, http.MethodGet, "/static/uploadedImages/a1b2.png", map[string]string{"If-Modified-Since": lastModified})
+			if recorder.Code != http.StatusNotModified || recorder.Body.Len() != 0 {
+				t.Errorf("got %d with %d body bytes", recorder.Code, recorder.Body.Len())
+			}
+		})
+
+		t.Run("GET with Range bytes=0-3", func(t *testing.T) {
+			recorder := serveS3Storage(router, http.MethodGet, "/static/uploadedImages/a1b2.png", map[string]string{"Range": "bytes=0-3"})
+			if recorder.Code != http.StatusPartialContent || recorder.Body.String() != "0123" {
+				t.Fatalf("got %d %q", recorder.Code, recorder.Body.String())
+			}
+			if recorder.Header().Get("Content-Range") != "bytes 0-3/10" || recorder.Header().Get("Content-Length") != "4" {
+				t.Errorf("unexpected headers %v", recorder.Header())
+			}
+		})
+
+		t.Run("GET with Range bytes=4-6 reads the middle through a ranged request", func(t *testing.T) {
+			recorder := serveS3Storage(router, http.MethodGet, "/static/uploadedImages/a1b2.png", map[string]string{"Range": "bytes=4-6"})
+			if recorder.Code != http.StatusPartialContent || recorder.Body.String() != "456" {
+				t.Fatalf("got %d %q", recorder.Code, recorder.Body.String())
+			}
+		})
+
+		t.Run("GET with Range bytes=50-60", func(t *testing.T) {
+			recorder := serveS3Storage(router, http.MethodGet, "/static/uploadedImages/a1b2.png", map[string]string{"Range": "bytes=50-60"})
+			if recorder.Code != http.StatusRequestedRangeNotSatisfiable || recorder.Header().Get("Content-Range") != "bytes */10" {
+				t.Errorf("got %d %v", recorder.Code, recorder.Header())
+			}
+		})
+	})
+
+	t.Run("SVG from the bucket keeps its security headers", func(t *testing.T) {
+		fake := newFakeS3Server(t)
+		fake.put("logo.svg", []byte("<svg/>"), "image/svg+xml")
+		router := newS3StorageRouter(openFakeS3Storage(t, fake, ""))
+		recorder := serveS3Storage(router, http.MethodGet, "/static/uploadedImages/logo.svg", nil)
+		if recorder.Header().Get("Content-Type") != "image/svg+xml" {
+			t.Errorf("unexpected Content-Type %q", recorder.Header().Get("Content-Type"))
+		}
+		if recorder.Header().Get("Content-Security-Policy") != "default-src 'none'; style-src 'unsafe-inline'; sandbox" {
+			t.Errorf("unexpected CSP %q", recorder.Header().Get("Content-Security-Policy"))
+		}
+	})
+
+	t.Run("Missing object is 404 and a backend failure is 500", func(t *testing.T) {
+		fake := newFakeS3Server(t)
+		fake.put("a1b2.png", []byte(s3ObjectBody), "image/png")
+		router := newS3StorageRouter(openFakeS3Storage(t, fake, ""))
+		if recorder := serveS3Storage(router, http.MethodGet, "/static/uploadedImages/missing.png", nil); recorder.Code != http.StatusNotFound {
+			t.Errorf("expected 404, got %d", recorder.Code)
+		}
+		fake.failMethod(http.MethodGet, http.StatusInternalServerError)
+		recorder := serveS3Storage(router, http.MethodGet, "/static/uploadedImages/a1b2.png", nil)
+		if recorder.Code != http.StatusInternalServerError || strings.TrimSpace(recorder.Body.String()) != "Unable to read the file" {
+			t.Errorf("got %d %q", recorder.Code, recorder.Body.String())
+		}
+	})
+
+	t.Run("Path traversal never reaches another key", func(t *testing.T) {
+		fake := newFakeS3Server(t)
+		router := newS3StorageRouter(openFakeS3Storage(t, fake, "uploads/"))
+		serveS3Storage(router, http.MethodGet, "/static/uploadedImages/../secret.png", nil)
+		keys := fake.requestedKeys(http.MethodGet)
+		if len(keys) != 1 || keys[0] != "uploads/secret.png" {
+			t.Errorf("unexpected requested keys %v", keys)
+		}
+	})
+
+	t.Run("Handler as-is for a non-seekable blob", func(t *testing.T) {
+		store := streamOnlyStorage{data: []byte(s3ObjectBody)}
+		get := serveFrom(t, store, http.MethodGet, "/remote.png")
+		if get.Code != http.StatusOK || get.Body.String() != s3ObjectBody || get.Header().Get("Content-Length") != "10" || get.Header().Get("Last-Modified") == "" {
+			t.Errorf("unexpected GET %d %q %v", get.Code, get.Body.String(), get.Header())
+		}
+		head := serveFrom(t, store, http.MethodHead, "/remote.png")
+		if head.Code != http.StatusOK || head.Body.Len() != 0 || head.Header().Get("Content-Length") != "10" || head.Header().Get("Last-Modified") == "" {
+			t.Errorf("unexpected HEAD %d %q %v", head.Code, head.Body.String(), head.Header())
+		}
+	})
+
+	t.Run("Boot backfill logs how many images it read from the bucket", func(t *testing.T) {
+		repo, store := newMockImageRepo(), newMockImageStorage()
+		for id, name := range []string{"one.png", "two.png", "three.png"} {
+			store.blobs[name] = pngBytes(t, id+1, 1)
+			repo.images[uint32(id+1)] = domainImage.Image{ID: uint32(id + 1), Path: domainImage.PublicPath(name)}
+		}
+		repo.images[4] = domainImage.Image{ID: 4, Path: domainImage.PublicPath("missing.png")}
+
+		var logged bytes.Buffer
+		err := storage.RunBackfillReportingReads(store, log.New(&logged, "", 0), func(readable domainImage.Storage) error {
+			return useCases.NewBackfillImageMetadataUseCase(repo, readable).Run()
+		})
+		if err != nil {
+			t.Fatalf("backfill failed: %v", err)
+		}
+		if !strings.Contains(logged.String(), "read 3 images") || strings.Contains(logged.String(), s3TestSecret) {
+			t.Errorf("unexpected log %q", logged.String())
+		}
+		if repo.images[1].Filename != "one.png" || repo.images[4].Filename != "" {
+			t.Errorf("backfill behaviour changed: %+v", repo.images)
+		}
+
+		failure := errors.New("repository down")
+		repo.fail.failNextWith("ListMissingMetadata", failure)
+		var silent bytes.Buffer
+		err = storage.RunBackfillReportingReads(store, log.New(&silent, "", 0), func(readable domainImage.Storage) error {
+			return useCases.NewBackfillImageMetadataUseCase(repo, readable).Run()
+		})
+		if !errors.Is(err, failure) || silent.Len() != 0 {
+			t.Errorf("errors must be returned unchanged and nothing logged for zero reads, got %v %q", err, silent.String())
+		}
+	})
+
+	t.Run("The local driver is unchanged", func(t *testing.T) {
+		clearStorageEnv(t)
+		settings, err := config.NewSettingsFromEnv()
+		if err != nil || settings.Storage.Driver != "local" {
+			t.Fatalf("expected the local driver, got %+v, %v", settings.Storage, err)
+		}
+		uploadDir := t.TempDir()
+		store, err := storage.Open(storage.Config{Driver: storage.DriverLocal, Location: uploadDir})
+		if err != nil {
+			t.Fatalf("Open failed: %v", err)
+		}
+		name, err := store.Put("image/png", bytes.NewReader([]byte(s3ObjectBody)))
+		if err != nil {
+			t.Fatalf("Put failed: %v", err)
+		}
+		if data, err := os.ReadFile(filepath.Join(uploadDir, name)); err != nil || string(data) != s3ObjectBody {
+			t.Fatalf("expected the file in UPLOAD_DIR, got %q, %v", data, err)
+		}
+		recorder := serveS3Storage(newS3StorageRouter(store), http.MethodGet, "/static/uploadedImages/"+name, nil)
+		if recorder.Code != http.StatusOK || recorder.Body.String() != s3ObjectBody || recorder.Header().Get("Content-Type") != "image/png" {
+			t.Errorf("got %d %q %v", recorder.Code, recorder.Body.String(), recorder.Header())
+		}
+	})
+}

test/s3_image_storage_integration_test.go

@@ -0,0 +1,71 @@
+package test
+
+import (
+	domainImage "RenewCMS/internal/domain/image"
+	"RenewCMS/internal/infrastructure/storage"
+	"bytes"
+	"errors"
+	"io"
+	"os"
+	"testing"
+)
+
+const (
+	envTestS3Endpoint  = "RENEWCMS_TEST_S3_ENDPOINT"
+	envTestS3Bucket    = "RENEWCMS_TEST_S3_BUCKET"
+	envTestS3AccessKey = "RENEWCMS_TEST_S3_ACCESS_KEY"
+	envTestS3SecretKey = "RENEWCMS_TEST_S3_SECRET_KEY"
+)
+
+func TestS3StorageIntegration(t *testing.T) {
+	endpoint := os.Getenv(envTestS3Endpoint)
+	if endpoint == "" {
+		t.Skipf("%s is not set, run task test:s3 to start a throwaway S3 server", envTestS3Endpoint)
+	}
+	store, err := storage.Open(storage.Config{Driver: storage.DriverS3, S3: storage.S3Config{
+		Bucket:          os.Getenv(envTestS3Bucket),
+		Region:          "us-east-1",
+		Endpoint:        endpoint,
+		AccessKeyID:     os.Getenv(envTestS3AccessKey),
+		SecretAccessKey: os.Getenv(envTestS3SecretKey),
+		KeyPrefix:       "integration/",
+		PathStyle:       true,
+	}})
+	if err != nil {
+		t.Fatalf("storage.Open failed: %v", err)
+	}
+
+	name, err := store.Put("image/png", bytes.NewReader([]byte("0123456789")))
+	if err != nil {
+		t.Fatalf("Put failed: %v", err)
+	}
+	blob, err := store.Open(name)
+	if err != nil {
+		t.Fatalf("Open failed: %v", err)
+	}
+	if blob.Size != 10 || blob.ModTime.IsZero() {
+		t.Errorf("unexpected blob metadata size=%d modtime=%v", blob.Size, blob.ModTime)
+	}
+	seeker, ok := blob.Content.(io.ReadSeeker)
+	if !ok {
+		t.Fatal("Content is not an io.ReadSeeker")
+	}
+	if _, err := seeker.Seek(4, io.SeekStart); err != nil {
+		t.Fatalf("Seek failed: %v", err)
+	}
+	chunk := make([]byte, 3)
+	if _, err := io.ReadFull(seeker, chunk); err != nil || string(chunk) != "456" {
+		t.Errorf("ranged read = %q, %v", chunk, err)
+	}
+	_ = blob.Content.Close()
+
+	if err := store.Delete(name); err != nil {
+		t.Fatalf("Delete failed: %v", err)
+	}
+	if err := store.Delete(name); err != nil {
+		t.Errorf("deleting a missing key must not fail, got %v", err)
+	}
+	if _, err := store.Open(name); !errors.Is(err, domainImage.ErrBlobNotFound) {
+		t.Errorf("expected ErrBlobNotFound after delete, got %v", err)
+	}
+}

test/s3_image_storage_test.go

@@ -0,0 +1,547 @@
+package test
+
+import (
+	domainImage "RenewCMS/internal/domain/image"
+	"RenewCMS/internal/infrastructure/config"
+	"RenewCMS/internal/infrastructure/storage"
+	"bytes"
+	"errors"
+	"fmt"
+	"io"
+	"net"
+	"net/http"
+	"net/http/httptest"
+	"strconv"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+)
+
+const (
+	s3TestBucket    = "media"
+	s3TestSecret    = "TOPSECRET"
+	s3TestAccessKey = "AKIATESTKEY"
+)
+
+var s3TestModTime = time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC)
+
+type fakeS3Object struct {
+	data        []byte
+	contentType string
+	modTime     time.Time
+}
+
+type fakeS3Request struct {
+	method string
+	key    string
+	rng    string
+}
+
+type fakeS3Server struct {
+	server       *httptest.Server
+	mu           sync.Mutex
+	objects      map[string]fakeS3Object
+	bucketStatus int
+	failures     map[string]int
+	requests     []fakeS3Request
+}
+
+func newFakeS3Server(t *testing.T) *fakeS3Server {
+	t.Helper()
+	fake := &fakeS3Server{
+		objects:      map[string]fakeS3Object{},
+		bucketStatus: http.StatusOK,
+		failures:     map[string]int{},
+	}
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+	fake.server = httptest.NewUnstartedServer(http.HandlerFunc(fake.handle))
+	fake.server.Listener = listener
+	fake.server.Start()
+	t.Cleanup(fake.server.Close)
+	return fake
+}
+
+func (f *fakeS3Server) put(key string, data []byte, contentType string) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	f.objects[key] = fakeS3Object{data: data, contentType: contentType, modTime: s3TestModTime}
+}
+
+func (f *fakeS3Server) object(key string) (fakeS3Object, bool) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	object, ok := f.objects[key]
+	return object, ok
+}
+
+func (f *fakeS3Server) failMethod(method string, status int) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	f.failures[method] = status
+}
+
+func (f *fakeS3Server) setBucketStatus(status int) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	f.bucketStatus = status
+}
+
+func (f *fakeS3Server) recorded() []fakeS3Request {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	return append([]fakeS3Request(nil), f.requests...)
+}
+
+func (f *fakeS3Server) requestedKeys(method string) []string {
+	var keys []string
+	for _, request := range f.recorded() {
+		if request.method == method {
+			keys = append(keys, request.key)
+		}
+	}
+	return keys
+}
+
+func (f *fakeS3Server) handle(w http.ResponseWriter, r *http.Request) {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	trimmed := strings.TrimPrefix(r.URL.Path, "/"+s3TestBucket)
+	if r.URL.Path != "/"+s3TestBucket && trimmed == r.URL.Path {
+		http.Error(w, "unknown bucket", http.StatusNotFound)
+		return
+	}
+	key := strings.TrimPrefix(trimmed, "/")
+	if key == "" {
+		w.WriteHeader(f.bucketStatus)
+		return
+	}
+	f.requests = append(f.requests, fakeS3Request{method: r.Method, key: key, rng: r.Header.Get("Range")})
+	if status, failing := f.failures[r.Method]; failing {
+		writeFakeS3Error(w, r, status, "InternalError")
+		return
+	}
+	switch r.Method {
+	case http.MethodPut:
+		data, _ := io.ReadAll(r.Body)
+		f.objects[key] = fakeS3Object{data: data, contentType: r.Header.Get("Content-Type"), modTime: s3TestModTime}
+		w.WriteHeader(http.StatusOK)
+	case http.MethodDelete:
+		delete(f.objects, key)
+		w.WriteHeader(http.StatusNoContent)
+	case http.MethodGet, http.MethodHead:
+		object, ok := f.objects[key]
+		if !ok {
+			writeFakeS3Error(w, r, http.StatusNotFound, "NoSuchKey")
+			return
+		}
+		writeFakeS3Object(w, r, object)
+	default:
+		w.WriteHeader(http.StatusMethodNotAllowed)
+	}
+}
+
+func writeFakeS3Error(w http.ResponseWriter, r *http.Request, status int, code string) {
+	w.Header().Set("Content-Type", "application/xml")
+	w.WriteHeader(status)
+	if r.Method != http.MethodHead {
+		_, _ = fmt.Fprintf(w, `<?xml version="1.0" encoding="UTF-8"?><Error><Code>%s</Code><Message>%s</Message></Error>`, code, code)
+	}
+}
+
+func writeFakeS3Object(w http.ResponseWriter, r *http.Request, object fakeS3Object) {
+	data := object.data
+	status := http.StatusOK
+	w.Header().Set("Content-Type", object.contentType)
+	w.Header().Set("Last-Modified", object.modTime.UTC().Format(http.TimeFormat))
+	if rng := r.Header.Get("Range"); rng != "" {
+		var start int
+		spec := strings.TrimPrefix(rng, "bytes=")
+		bounds := strings.SplitN(spec, "-", 2)
+		start, _ = strconv.Atoi(bounds[0])
+		end := len(data) - 1
+		if bounds[1] != "" {
+			end, _ = strconv.Atoi(bounds[1])
+		}
+		w.Header().Set("Content-Range", fmt.Sprintf("bytes %d-%d/%d", start, end, len(data)))
+		data = data[start : end+1]
+		status = http.StatusPartialContent
+	}
+	w.Header().Set("Content-Length", strconv.Itoa(len(data)))
+	w.WriteHeader(status)
+	if r.Method != http.MethodHead {
+		_, _ = w.Write(data)
+	}
+}
+
+func (f *fakeS3Server) config(prefix string) storage.S3Config {
+	return storage.S3Config{
+		Bucket:          s3TestBucket,
+		Region:          "us-east-1",
+		Endpoint:        f.server.URL,
+		AccessKeyID:     s3TestAccessKey,
+		SecretAccessKey: s3TestSecret,
+		KeyPrefix:       prefix,
+		PathStyle:       true,
+		MaxAttempts:     1,
+	}
+}
+
+func openFakeS3Storage(t *testing.T, fake *fakeS3Server, prefix string) domainImage.Storage {
+	t.Helper()
+	store, err := storage.Open(storage.Config{Driver: storage.DriverS3, S3: fake.config(prefix)})
+	if err != nil {
+		t.Fatalf("storage.Open failed: %v", err)
+	}
+	return store
+}
+
+func readAllBlob(t *testing.T, blob domainImage.Blob) []byte {
+	t.Helper()
+	defer blob.Content.Close()
+	data, err := io.ReadAll(blob.Content)
+	if err != nil {
+		t.Fatalf("read failed: %v", err)
+	}
+	return data
+}
+
+func TestS3StorageSeekThenReadUsesRangedRequests(t *testing.T) {
+	fake := newFakeS3Server(t)
+	fake.put("a1b2.png", []byte("0123456789"), "image/png")
+	store := openFakeS3Storage(t, fake, "")
+
+	blob, err := store.Open("a1b2.png")
+	if err != nil {
+		t.Fatalf("Open failed: %v", err)
+	}
+	defer blob.Content.Close()
+	seeker, ok := blob.Content.(io.ReadSeeker)
+	if !ok {
+		t.Fatal("Content is not an io.ReadSeeker")
+	}
+
+	end, err := seeker.Seek(0, io.SeekEnd)
+	if err != nil || end != 10 {
+		t.Fatalf("Seek to end = %d, %v", end, err)
+	}
+	if _, err := seeker.Seek(4, io.SeekStart); err != nil {
+		t.Fatalf("Seek failed: %v", err)
+	}
+	chunk := make([]byte, 3)
+	if _, err := io.ReadFull(seeker, chunk); err != nil || string(chunk) != "456" {
+		t.Fatalf("read after seek = %q, %v", chunk, err)
+	}
+	rest, _ := io.ReadAll(seeker)
+	if string(rest) != "789" {
+		t.Errorf("remaining bytes = %q", rest)
+	}
+
+	var ranges []string
+	for _, request := range fake.recorded() {
+		if request.method == http.MethodGet {
+			ranges = append(ranges, request.rng)
+		}
+	}
+	if len(ranges) != 2 || ranges[0] != "" || ranges[1] != "bytes=4-" {
+		t.Errorf("unexpected GET ranges %q", ranges)
+	}
+
+	if _, err := seeker.Seek(-1, io.SeekStart); err == nil {
+		t.Error("a negative seek must fail")
+	}
+}
+
+func TestS3StorageNamesThatAreNotPlainAreNeverRequested(t *testing.T) {
+	fake := newFakeS3Server(t)
+	store := openFakeS3Storage(t, fake, "uploads/")
+	for _, name := range []string{"", ".", "..", "../x.png", "a/b.png"} {
+		if _, err := store.Open(name); !errors.Is(err, domainImage.ErrBlobNotFound) {
+			t.Errorf("Open(%q) = %v, want ErrBlobNotFound", name, err)
+		}
+		if err := store.Delete(name); err != nil {
+			t.Errorf("Delete(%q) = %v, want nil", name, err)
+		}
+	}
+	if requests := fake.recorded(); len(requests) != 0 {
+		t.Errorf("no object request expected, got %v", requests)
+	}
+}
+
+func TestS3StorageErrorsNeverContainTheSecret(t *testing.T) {
+	fake := newFakeS3Server(t)
+	store := openFakeS3Storage(t, fake, "")
+	fake.failMethod(http.MethodPut, http.StatusInternalServerError)
+	fake.failMethod(http.MethodGet, http.StatusInternalServerError)
+	fake.failMethod(http.MethodDelete, http.StatusInternalServerError)
+
+	_, putErr := store.Put("image/png", bytes.NewReader([]byte("abc")))
+	_, openErr := store.Open("a1b2.png")
+	deleteErr := store.Delete("a1b2.png")
+	for name, err := range map[string]error{"Put": putErr, "Open": openErr, "Delete": deleteErr} {
+		if err == nil {
+			t.Fatalf("%s must fail", name)
+		}
+		if strings.Contains(err.Error(), s3TestSecret) {
+			t.Errorf("%s error leaks the secret: %v", name, err)
+		}
+	}
+}
+
+func TestS3StorageOpenWithUnreachableEndpointIsSecretFree(t *testing.T) {
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+	endpoint := "http://" + listener.Addr().String()
+	_ = listener.Close()
+
+	_, err = storage.Open(storage.Config{Driver: storage.DriverS3, S3: storage.S3Config{
+		Bucket: s3TestBucket, Region: "us-east-1", Endpoint: endpoint, PathStyle: true, MaxAttempts: 1,
+		AccessKeyID: s3TestAccessKey, SecretAccessKey: s3TestSecret,
+	}})
+	if err == nil || !strings.Contains(err.Error(), s3TestBucket) || strings.Contains(err.Error(), s3TestSecret) {
+		t.Errorf("expected a bucket-named, secret-free error, got %v", err)
+	}
+}
+
+func TestS3StorageEndpointWithoutSchemeFollowsTheTLSSetting(t *testing.T) {
+	fake := newFakeS3Server(t)
+	settings := fake.config("")
+	settings.Endpoint = strings.TrimPrefix(fake.server.URL, "http://")
+	settings.TLS = false
+	store, err := storage.Open(storage.Config{Driver: storage.DriverS3, S3: settings})
+	if err != nil {
+		t.Fatalf("plain HTTP endpoint without scheme must open, got %v", err)
+	}
+	if _, err := store.Open("missing.png"); !errors.Is(err, domainImage.ErrBlobNotFound) {
+		t.Errorf("expected not found, got %v", err)
+	}
+}
+
+func TestS3StorageSettingsFromEnv(t *testing.T) {
+	setBaseSettingsEnv(t)
+	for _, name := range []string{"STORAGE_DRIVER", "S3_BUCKET", "S3_REGION", "S3_ENDPOINT", "S3_ACCESS_KEY_ID", "S3_SECRET_ACCESS_KEY", "S3_KEY_PREFIX", "S3_PATH_STYLE", "S3_TLS"} {
+		t.Setenv(name, "")
+	}
+
+	t.Run("Every S3 variable is read", func(t *testing.T) {
+		t.Setenv("STORAGE_DRIVER", "s3")
+		t.Setenv("S3_BUCKET", "media")
+		t.Setenv("S3_REGION", "eu-west-3")
+		t.Setenv("S3_ENDPOINT", "minio:9000")
+		t.Setenv("S3_ACCESS_KEY_ID", "id")
+		t.Setenv("S3_SECRET_ACCESS_KEY", "secret")
+		t.Setenv("S3_KEY_PREFIX", "cms/")
+		t.Setenv("S3_PATH_STYLE", "true")
+		t.Setenv("S3_TLS", "false")
+		settings, err := config.NewSettingsFromEnv()
+		if err != nil {
+			t.Fatalf("unexpected error %v", err)
+		}
+		want := config.S3Settings{Bucket: "media", Region: "eu-west-3", Endpoint: "minio:9000", AccessKeyID: "id", SecretAccessKey: "secret", KeyPrefix: "cms/", PathStyle: true, TLS: false}
+		if settings.Storage.Driver != "s3" || settings.Storage.S3 != want {
+			t.Errorf("unexpected storage settings %+v", settings.Storage)
+		}
+	})
+
+	t.Run("Defaults apply", func(t *testing.T) {
+		t.Setenv("STORAGE_DRIVER", "s3")
+		t.Setenv("S3_BUCKET", "media")
+		settings, err := config.NewSettingsFromEnv()
+		if err != nil {
+			t.Fatalf("unexpected error %v", err)
+		}
+		s3 := settings.Storage.S3
+		if s3.Region != "us-east-1" || s3.PathStyle || !s3.TLS || s3.KeyPrefix != "" || s3.Endpoint != "" {
+			t.Errorf("unexpected defaults %+v", s3)
+		}
+	})
+
+	t.Run("Invalid booleans are rejected", func(t *testing.T) {
+		t.Setenv("STORAGE_DRIVER", "s3")
+		t.Setenv("S3_BUCKET", "media")
+		t.Setenv("S3_PATH_STYLE", "maybe")
+		if _, err := config.NewSettingsFromEnv(); err == nil || !strings.Contains(err.Error(), "S3_PATH_STYLE") {
+			t.Errorf("expected an S3_PATH_STYLE error, got %v", err)
+		}
+		t.Setenv("S3_PATH_STYLE", "")
+		t.Setenv("S3_TLS", "maybe")
+		if _, err := config.NewSettingsFromEnv(); err == nil || !strings.Contains(err.Error(), "S3_TLS") {
+			t.Errorf("expected an S3_TLS error, got %v", err)
+		}
+	})
+
+	t.Run("Required variables by driver", func(t *testing.T) {
+		if got := config.StorageRequiredEnvVars("s3"); len(got) != 1 || got[0] != "S3_BUCKET" {
+			t.Errorf("unexpected required variables %v", got)
+		}
+		if got := config.StorageRequiredEnvVars("local"); len(got) != 0 {
+			t.Errorf("local requires nothing, got %v", got)
+		}
+	})
+}
+
+func countS3Gets(fake *fakeS3Server) int {
+	return len(fake.requestedKeys(http.MethodGet))
+}
+
+func openFakeS3Seeker(t *testing.T, fake *fakeS3Server, name string) io.ReadSeekCloser {
+	t.Helper()
+	store := openFakeS3Storage(t, fake, "")
+	blob, err := store.Open(name)
+	if err != nil {
+		t.Fatalf("Open failed: %v", err)
+	}
+	seeker, ok := blob.Content.(io.ReadSeekCloser)
+	if !ok {
+		t.Fatal("Content is not an io.ReadSeekCloser")
+	}
+	return seeker
+}
+
+func TestS3StorageSequentialReadUsesASingleRequest(t *testing.T) {
+	fake := newFakeS3Server(t)
+	fake.put("a1b2.png", []byte("0123456789"), "image/png")
+	seeker := openFakeS3Seeker(t, fake, "a1b2.png")
+	defer seeker.Close()
+
+	first := make([]byte, 4)
+	if _, err := io.ReadFull(seeker, first); err != nil || string(first) != "0123" {
+		t.Fatalf("first read = %q, %v", first, err)
+	}
+	rest, err := io.ReadAll(seeker)
+	if err != nil || string(rest) != "456789" {
+		t.Fatalf("rest = %q, %v", rest, err)
+	}
+	if gets := countS3Gets(fake); gets != 1 {
+		t.Errorf("sequential read must use 1 GET, got %d", gets)
+	}
+}
+
+func TestS3StorageReadAtEndReturnsEOFWithoutRequest(t *testing.T) {
+	fake := newFakeS3Server(t)
+	fake.put("a1b2.png", []byte("0123456789"), "image/png")
+	seeker := openFakeS3Seeker(t, fake, "a1b2.png")
+	defer seeker.Close()
+
+	if _, err := seeker.Seek(0, io.SeekEnd); err != nil {
+		t.Fatalf("Seek failed: %v", err)
+	}
+	n, err := seeker.Read(make([]byte, 4))
+	if n != 0 || err != io.EOF {
+		t.Errorf("Read at end = %d, %v, want 0, EOF", n, err)
+	}
+	if gets := countS3Gets(fake); gets != 1 {
+		t.Errorf("reading at the end must not issue a request, got %d GETs", gets)
+	}
+}
+
+func TestS3StorageSeekWhenceVariants(t *testing.T) {
+	fake := newFakeS3Server(t)
+	fake.put("a1b2.png", []byte("0123456789"), "image/png")
+	seeker := openFakeS3Seeker(t, fake, "a1b2.png")
+	defer seeker.Close()
+
+	if pos, err := seeker.Seek(-3, io.SeekEnd); err != nil || pos != 7 {
+		t.Fatalf("SeekEnd(-3) = %d, %v", pos, err)
+	}
+	if pos, err := seeker.Seek(-2, io.SeekCurrent); err != nil || pos != 5 {
+		t.Fatalf("SeekCurrent(-2) = %d, %v", pos, err)
+	}
+	chunk := make([]byte, 2)
+	if _, err := io.ReadFull(seeker, chunk); err != nil || string(chunk) != "56" {
+		t.Errorf("read after seeks = %q, %v", chunk, err)
+	}
+	if _, err := seeker.Seek(0, 99); err == nil {
+		t.Error("an invalid whence must fail")
+	}
+}
+
+func TestS3StorageReadAfterCloseReopensTheObject(t *testing.T) {
+	fake := newFakeS3Server(t)
+	fake.put("a1b2.png", []byte("0123456789"), "image/png")
+	seeker := openFakeS3Seeker(t, fake, "a1b2.png")
+
+	head := make([]byte, 3)
+	if _, err := io.ReadFull(seeker, head); err != nil {
+		t.Fatalf("read failed: %v", err)
+	}
+	if err := seeker.Close(); err != nil {
+		t.Fatalf("Close failed: %v", err)
+	}
+	rest, err := io.ReadAll(seeker)
+	if err != nil || string(rest) != "3456789" {
+		t.Errorf("read after Close = %q, %v", rest, err)
+	}
+	_ = seeker.Close()
+	if gets := countS3Gets(fake); gets != 2 {
+		t.Errorf("expected 2 GETs, got %d", gets)
+	}
+}
+
+func TestS3StorageMaxAttemptsLimitsRetries(t *testing.T) {
+	fake := newFakeS3Server(t)
+	store := openFakeS3Storage(t, fake, "")
+	fake.failMethod(http.MethodGet, http.StatusInternalServerError)
+
+	if _, err := store.Open("a1b2.png"); err == nil {
+		t.Fatal("Open must fail")
+	}
+	if gets := countS3Gets(fake); gets != 1 {
+		t.Errorf("MaxAttempts=1 must issue exactly 1 GET, got %d", gets)
+	}
+}
+
+func TestS3StorageTLSEndpointWithoutSchemeUsesHTTPS(t *testing.T) {
+	fake := newFakeS3Server(t)
+	settings := fake.config("")
+	settings.Endpoint = strings.TrimPrefix(fake.server.URL, "http://")
+	settings.TLS = true
+	_, err := storage.Open(storage.Config{Driver: storage.DriverS3, S3: settings})
+	if err == nil || !strings.Contains(err.Error(), s3TestBucket) {
+		t.Errorf("https against a plain HTTP server must fail naming the bucket, got %v", err)
+	}
+}
+
+func TestS3StorageRedactsSecretsFoundInTransportErrors(t *testing.T) {
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+	endpoint := "http://" + listener.Addr().String() + "/" + s3TestAccessKey + "/" + s3TestSecret
+	_ = listener.Close()
+
+	_, err = storage.Open(storage.Config{Driver: storage.DriverS3, S3: storage.S3Config{
+		Bucket: s3TestBucket, Region: "us-east-1", Endpoint: endpoint, PathStyle: true, MaxAttempts: 1,
+		AccessKeyID: s3TestAccessKey, SecretAccessKey: s3TestSecret,
+	}})
+	if err == nil {
+		t.Fatal("expected an error")
+	}
+	if strings.Contains(err.Error(), s3TestSecret) || strings.Contains(err.Error(), s3TestAccessKey) {
+		t.Errorf("credentials leaked: %v", err)
+	}
+	if !strings.Contains(err.Error(), "***") {
+		t.Errorf("expected redaction markers, got %v", err)
+	}
+}
+
+func TestS3StorageReadFailsWhenTheRangedRequestFails(t *testing.T) {
+	fake := newFakeS3Server(t)
+	fake.put("a1b2.png", []byte("0123456789"), "image/png")
+	seeker := openFakeS3Seeker(t, fake, "a1b2.png")
+	defer seeker.Close()
+
+	if _, err := seeker.Seek(4, io.SeekStart); err != nil {
+		t.Fatalf("Seek failed: %v", err)
+	}
+	fake.failMethod(http.MethodGet, http.StatusInternalServerError)
+	n, err := seeker.Read(make([]byte, 3))
+	if err == nil || n != 0 {
+		t.Errorf("Read = %d, %v, want a failure with no bytes", n, err)
+	}
+}