From 41a03b9987f52244fb75df2efd96540321550971 Mon Sep 17 00:00:00 2001
From: Jakub Zych <jakub@golem15.com>
Date: Mon, 12 Oct 2026 11:20:43 +0200
Subject: [PATCH] fix(14.2): WR-03 uploads route sandboxes files and downloads
 non-images

The route answered with the blob's stored Content-Type, which attach.Store
sniffs from the bytes, so a public text upload starting with <html> was
served as text/html on the application origin; nosniff does not help when
the declared type is already HTML. Every response now carries a sandboxing
Content-Security-Policy, and a successful response whose type is not a
raster image (JPEG, PNG, GIF, WebP, AVIF) gets Content-Disposition:
attachment, on GET and HEAD. Covers stay inline.
---
 docs/services/storage.md     |   8 ++-
 docs/setup/configuration.md  |   2 +-
 modules/surf/README.md       |   4 +-
 modules/surf/uploads.go      | 101 +++++++++++++++++++++++++++++------
 modules/surf/uploads_test.go |  76 ++++++++++++++++++++++++++
 5 files changed, 172 insertions(+), 19 deletions(-)

diff --git a/docs/services/storage.md b/docs/services/storage.md
index a477539..7471ae3 100644
--- a/docs/services/storage.md
+++ b/docs/services/storage.md
@@ -51,7 +51,13 @@ storage:
     serve: true
 ```
 
-The route serves originals and thumbnails and answers 404 for a missing file, an invalid path and a file whose `system_files` row is not public (`is_public` false). Every response carries `X-Content-Type-Options: nosniff`, so a browser does not sniff a stored file into HTML or script. The route looks up the bucket and the database on each request: `route:list` lists it without opening either, and it answers 404 until `serve` has published them. The route owns the whole prefix, so start-up fails when a plugin registers a route at or under it.
+The route serves originals and thumbnails and answers 404 for a missing file, an invalid path and a file whose `system_files` row is not public (`is_public` false). The stored content type is the one sniffed from the file's bytes at upload, so an uploaded text file that starts with `<html>` is stored as `text/html`; `X-Content-Type-Options: nosniff` alone does not stop a browser from rendering a file whose declared type is already HTML. The route therefore protects the application origin in three ways:
+
+- Every response carries `X-Content-Type-Options: nosniff`.
+- Every response carries `Content-Security-Policy: default-src 'none'; img-src 'self'; media-src 'self'; sandbox`. A file opened as a page runs no script and gets an opaque origin, so it cannot read the application's cookies or storage. The policy does not affect an `<img>` or `<video>` that embeds the file.
+- A file whose type is not a raster image (`image/jpeg`, `image/png`, `image/gif`, `image/webp` or `image/avif`) is sent with `Content-Disposition: attachment`, so the browser downloads it instead of showing it. That includes HTML, SVG, PDF and plain text.
+
+The route looks up the bucket and the database on each request: `route:list` lists it without opening either, and it answers 404 until `serve` has published them. The route owns the whole prefix, so start-up fails when a plugin registers a route at or under it.
 
 Without the key, the application decides where files are served: mount `attach.StaticHandlerPublic` itself, or put a web server or CDN in front of the bucket directory. Either way, serve uploads from a separate origin when you can; the [Attachments](../database/attachments.md) page explains why. The built-in route is meant for development and for deployments that cannot split the origin yet.
 
diff --git a/docs/setup/configuration.md b/docs/setup/configuration.md
index 8a8a509..6a9e760 100644
--- a/docs/setup/configuration.md
+++ b/docs/setup/configuration.md
@@ -55,7 +55,7 @@ These are the keys most applications set. Each module's reference page lists all
 | `http.cors.*`, `http.trusted_proxies` | CORS and the proxies whose forwarded client IP is trusted. | [surf](../../modules/surf/README.md) |
 | `http.access_log` | Logs one record per request with method, matched route pattern, status and duration. Defaults to `false`. | [surf](../../modules/surf/README.md) |
 | `storage.uploads.bucket_url` | Uploads bucket, `file://` or `mem://`. Required by `serve`. | [lagoon](../../modules/lagoon/README.md) |
-| `storage.uploads.serve` | Mounts `attach.StaticHandlerPublic` under `storage.uploads.public_path_prefix` with `X-Content-Type-Options: nosniff`. Defaults to `false`. | [surf](../../modules/surf/README.md) |
+| `storage.uploads.serve` | Mounts `attach.StaticHandlerPublic` under `storage.uploads.public_path_prefix` with `X-Content-Type-Options: nosniff`, a sandboxing CSP and `Content-Disposition: attachment` for anything but a raster image. Defaults to `false`. | [surf](../../modules/surf/README.md) |
 | `admin.jwt.secret` | Secret for admin tokens. Required as soon as a plugin registers an admin controller. | [cabana](../../modules/cabana/README.md) |
 | `backend.uri` | Path the admin is served under. Defaults to `/backend`. | [cabana](../../modules/cabana/README.md) |
 | `mail.driver`, `mail.from`, `mail.smtp.*` | Mail delivery: `memory`, `log` or `smtp`. | [postcard](../../modules/postcard/README.md) |
diff --git a/modules/surf/README.md b/modules/surf/README.md
index 07ded62..c93e815 100644
--- a/modules/surf/README.md
+++ b/modules/surf/README.md
@@ -34,7 +34,7 @@ Use cases:
 - Path-scoped CORS configured with the same keys as Laravel's `config/cors.php` (`surf.CORSConfig`), including preflight handling. Every `OPTIONS` request on a CORS path is answered with 204 before routing, with the headers Laravel's `HandleCors` sends: `Cache-Control: no-cache, private` always and, on a preflight (an `Origin` and an `Access-Control-Request-Method`), the requested method (upper-cased) and headers echoed in `Access-Control-Allow-Methods` and `Access-Control-Allow-Headers` when `*` allows any, `Vary` on the request headers and PHP's default `Content-Type: text/html; charset=UTF-8`.
 - Optional `surf.LocaleResolver`: a compiled plugin publishes the interface through backpack; surf looks it up without importing the plugin. When present, it may strip an enabled locale prefix before routing and writes a validated code with `towel.WithLocale`. When absent, hosts keep the raw `Accept-Language` header and `surf.LocaleFromPrincipal` overlay.
 - `surf.LocaleFromPrincipal` switches the request locale to the signed-in user's preferred locale.
-- Opt-in uploads route: with `storage.uploads.serve` true, `surf.BuildRouter` mounts `GET <storage.uploads.public_path_prefix>/{path...}` (owned by `summercms.surf`) on `attach.StaticHandlerPublic`, so a file is served only when its `system_files` row is public, and every response carries `X-Content-Type-Options: nosniff`. The bucket and the database are looked up per request, so `route:list` lists the route without opening either; until `serve` publishes them it answers 404. The route owns the whole prefix: `surf.BuildRouter` fails when a plugin route sits at or under it.
+- Opt-in uploads route: with `storage.uploads.serve` true, `surf.BuildRouter` mounts `GET <storage.uploads.public_path_prefix>/{path...}` (owned by `summercms.surf`) on `attach.StaticHandlerPublic`, so a file is served only when its `system_files` row is public. Every response carries `X-Content-Type-Options: nosniff` and a sandboxing `Content-Security-Policy`, and a file whose type is not a raster image (JPEG, PNG, GIF, WebP, AVIF) is sent with `Content-Disposition: attachment`, so a stored HTML or SVG file never renders on the app origin. The bucket and the database are looked up per request, so `route:list` lists the route without opening either; until `serve` publishes them it answers 404. The route owns the whole prefix: `surf.BuildRouter` fails when a plugin route sits at or under it.
 - Opt-in request log: with `http.access_log` true, every request writes one Info record `http request` after it is served, with `method`, `pattern` (the matched route, such as `GET /posts/{id}`), `status` and `duration_ms`. A request whose client went away adds `client_gone=true`; its 5xx is logged as 499 (client closed request) only when the client had already gone when the status was written and the handler did not panic, so a 5xx written before the cancellation and a recovered panic keep their real status. An unmatched request (a 404 or 405 from the mux, or a CORS preflight answered before routing) has an empty `pattern`. The URL path, the query string, headers and the client address are never logged, for an unmatched request too: a token route requested with another method answers 405 before any route handler runs. Records go to the app's published `*slog.Logger`, else `slog.Default`, so the [sunscreen](../sunscreen/README.md) redaction still applies.
 - A read-only route table (`surf.Router.Routes`), the admin mount status (`surf.Router.AdminPrefix`) and the `serve` and `route:list` commands.
 
@@ -157,7 +157,7 @@ The generated application `main` wires surf in with `surf.ServeCommand` and `sur
 | `http.cors.max_age` | `0` | Preflight cache time in seconds. |
 | `http.cors.supports_credentials` | `false` | Sends `Access-Control-Allow-Credentials: true`. |
 | `http.access_log` | `false` | Logs one `http request` record per request (method, matched pattern, status, duration in milliseconds; never the path or query). Environment: `SUMMER_HTTP__ACCESS_LOG`. |
-| `storage.uploads.serve` | `false` | Mounts the uploads route `GET <storage.uploads.public_path_prefix>/{path...}` (prefix default `/storage/uploads`) on `attach.StaticHandlerPublic` with `X-Content-Type-Options: nosniff`. Prefer a separate origin for uploads in production. Environment: `SUMMER_STORAGE__UPLOADS__SERVE`. |
+| `storage.uploads.serve` | `false` | Mounts the uploads route `GET <storage.uploads.public_path_prefix>/{path...}` (prefix default `/storage/uploads`) on `attach.StaticHandlerPublic` with `X-Content-Type-Options: nosniff`, a sandboxing CSP and `Content-Disposition: attachment` for anything but a raster image. Prefer a separate origin for uploads in production. Environment: `SUMMER_STORAGE__UPLOADS__SERVE`. |
 
 ```yaml
 http:
diff --git a/modules/surf/uploads.go b/modules/surf/uploads.go
index 553c434..7b54c5d 100644
--- a/modules/surf/uploads.go
+++ b/modules/surf/uploads.go
@@ -2,6 +2,7 @@ package surf
 
 import (
 	"fmt"
+	"mime"
 	"net/http"
 	"strings"
 
@@ -19,11 +20,32 @@ const uploadsPluginID = "summercms.surf"
 // storage.uploads.public_path_prefix is empty.
 const defaultUploadsPrefix = "/storage/uploads"
 
+// uploadsContentSecurityPolicy goes on every uploads response: a stored
+// file opened as a document runs no script, loads nothing but same-origin
+// images and media, and gets an opaque origin (sandbox), so it cannot read
+// the app's cookies or storage. It does not affect an <img> or <video> that
+// embeds the file.
+const uploadsContentSecurityPolicy = "default-src 'none'; img-src 'self'; media-src 'self'; sandbox"
+
+// uploadsInlineTypes are the content types the uploads route lets a browser
+// show inline: raster images. Every other successful response, SVG and PDF
+// included, is served with Content-Disposition: attachment.
+var uploadsInlineTypes = map[string]bool{
+	"image/avif": true,
+	"image/gif":  true,
+	"image/jpeg": true,
+	"image/png":  true,
+	"image/webp": true,
+}
+
 // mountUploads registers GET {storage.uploads.public_path_prefix}/{path...}
 // when storage.uploads.serve is true. The handler delegates to
-// attach.StaticHandlerPublic, so missing rows and is_public=false rows 404,
-// and adds X-Content-Type-Options: nosniff because the files are served on
-// the app origin. The bucket and the database are looked up per request:
+// attach.StaticHandlerPublic, so missing rows and is_public=false rows 404.
+// Because the files are served on the app origin, every response carries
+// X-Content-Type-Options: nosniff and uploadsContentSecurityPolicy, and a
+// file whose stored type is not an inline raster image (uploadsInlineTypes)
+// is served as an attachment: attach.Store records the type sniffed from
+// the bytes, so an uploaded text file that starts with <html> is text/html. The bucket and the database are looked up per request:
 // boot and route:list build the router before serve publishes either.
 // The route owns the whole prefix: a plugin route at or under it fails
 // BuildRouter, as a route under the admin prefix does. With the key false
@@ -56,17 +78,66 @@ func mountUploads(r *Router, app *backpack.App) error {
 // and answers 404 while either is missing.
 func uploadsHandler(app *backpack.App, prefix string) http.HandlerFunc {
 	return func(w http.ResponseWriter, req *http.Request) {
-		w.Header().Set("X-Content-Type-Options", "nosniff")
-		bucket, ok := app.Lookup[*blob.Bucket]()
-		if !ok || bucket == nil {
-			http.NotFound(w, req)
-			return
-		}
-		gdb, ok := app.Lookup[*gorm.DB]()
-		if !ok || gdb == nil {
-			http.NotFound(w, req)
-			return
-		}
-		attach.StaticHandlerPublic(bucket, prefix, gdb).ServeHTTP(w, req)
+		serveUploads(w, req, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+			bucket, ok := app.Lookup[*blob.Bucket]()
+			if !ok || bucket == nil {
+				http.NotFound(w, req)
+				return
+			}
+			gdb, ok := app.Lookup[*gorm.DB]()
+			if !ok || gdb == nil {
+				http.NotFound(w, req)
+				return
+			}
+			attach.StaticHandlerPublic(bucket, prefix, gdb).ServeHTTP(w, req)
+		}))
 	}
 }
+
+// serveUploads runs next with the uploads response headers: nosniff and the
+// sandboxing CSP always, and Content-Disposition: attachment on a successful
+// response whose Content-Type is not in uploadsInlineTypes. The disposition
+// is decided when the status is written (or, for a HEAD that writes
+// nothing, when next returns), after next has set the Content-Type.
+func serveUploads(w http.ResponseWriter, req *http.Request, next http.Handler) {
+	w.Header().Set("X-Content-Type-Options", "nosniff")
+	w.Header().Set("Content-Security-Policy", uploadsContentSecurityPolicy)
+	dw := &uploadsDispositionWriter{ResponseWriter: w}
+	next.ServeHTTP(dw, req)
+	dw.decide(http.StatusOK)
+}
+
+// uploadsDispositionWriter adds Content-Disposition: attachment before the
+// first status of a non-inline successful response goes out.
+type uploadsDispositionWriter struct {
+	http.ResponseWriter
+	decided bool
+}
+
+func (w *uploadsDispositionWriter) decide(status int) {
+	if w.decided {
+		return
+	}
+	w.decided = true
+	if status >= 200 && status < 300 && !uploadsInline(w.Header().Get("Content-Type")) {
+		w.Header().Set("Content-Disposition", "attachment")
+	}
+}
+
+func (w *uploadsDispositionWriter) WriteHeader(status int) {
+	w.decide(status)
+	w.ResponseWriter.WriteHeader(status)
+}
+
+func (w *uploadsDispositionWriter) Write(p []byte) (int, error) {
+	w.decide(http.StatusOK)
+	return w.ResponseWriter.Write(p)
+}
+
+func (w *uploadsDispositionWriter) Unwrap() http.ResponseWriter { return w.ResponseWriter }
+
+// uploadsInline reports whether contentType is an inline raster image.
+func uploadsInline(contentType string) bool {
+	mediaType, _, err := mime.ParseMediaType(contentType)
+	return err == nil && uploadsInlineTypes[strings.ToLower(mediaType)]
+}
diff --git a/modules/surf/uploads_test.go b/modules/surf/uploads_test.go
index fc2b88c..6ded316 100644
--- a/modules/surf/uploads_test.go
+++ b/modules/surf/uploads_test.go
@@ -122,6 +122,7 @@ const (
 	uploadsPublicDisk  = "abc123xyz.png"
 	uploadsPrivateDisk = "def456uvw.png"
 	uploadsOrphanDisk  = "ghi789rst.png"
+	uploadsHTMLDisk    = "jkl012mno.txt"
 	uploadsPublicBody  = "public-png-bytes"
 )
 
@@ -208,6 +209,7 @@ func TestUploadsMount(t *testing.T) {
 	for _, f := range []attach.File{
 		{DiskName: uploadsPublicDisk, FileName: "cover.png", FileSize: 1, ContentType: "image/png", IsPublic: &pub},
 		{DiskName: uploadsPrivateDisk, FileName: "secret.png", FileSize: 1, ContentType: "image/png", IsPublic: &priv},
+		{DiskName: uploadsHTMLDisk, FileName: "notes.txt", FileSize: 1, ContentType: "text/html; charset=utf-8", IsPublic: &pub},
 	} {
 		if err := gdb.Create(&f).Error; err != nil {
 			t.Fatal(err)
@@ -222,6 +224,10 @@ func TestUploadsMount(t *testing.T) {
 			t.Fatal(err)
 		}
 	}
+	if err := bucket.WriteAll(ctx, attach.BlobKey(uploadsHTMLDisk), []byte("<html><script>alert(1)</script></html>"), &blob.WriterOptions{ContentType: "text/html; charset=utf-8"}); err != nil {
+		t.Fatal(err)
+	}
+	htmlPath := "/storage/uploads/" + attach.BlobKey(uploadsHTMLDisk)
 
 	t.Run("database without a bucket answers 404", func(t *testing.T) {
 		app := backpack.New(uploadsConfig(t, map[string]any{"storage.uploads.serve": true}))
@@ -269,6 +275,15 @@ func TestUploadsMount(t *testing.T) {
 		if got := r.header.Get("X-Content-Type-Options"); got != "nosniff" {
 			t.Fatalf("X-Content-Type-Options = %q, want nosniff", got)
 		}
+		if r.header.Get("Content-Disposition") != "" || r.header.Get("Content-Security-Policy") != uploadsContentSecurityPolicy {
+			t.Fatalf("image/png: Content-Disposition=%q CSP=%q, want inline with the sandbox CSP", r.header.Get("Content-Disposition"), r.header.Get("Content-Security-Policy"))
+		}
+		// A public file whose stored type is text/html (a .txt upload whose
+		// bytes start with <html>) is served as an attachment, sandboxed.
+		html := uploadsDo(h, http.MethodGet, htmlPath)
+		if html.code != http.StatusOK || html.header.Get("Content-Disposition") != "attachment" || html.header.Get("Content-Security-Policy") != uploadsContentSecurityPolicy {
+			t.Fatalf("GET %s = %d Content-Disposition=%q CSP=%q, want 200 attachment with the sandbox CSP", htmlPath, html.code, html.header.Get("Content-Disposition"), html.header.Get("Content-Security-Policy"))
+		}
 		head := uploadsDo(h, http.MethodHead, publicPath)
 		if head.code != http.StatusOK || head.body != "" || head.header.Get("Content-Type") != "image/png" || head.header.Get("X-Content-Type-Options") != "nosniff" {
 			t.Fatalf("HEAD %s = %d %q %v", publicPath, head.code, head.body, head.header)
@@ -329,3 +344,64 @@ func TestUploadsMount(t *testing.T) {
 		}
 	})
 }
+
+// TestUploadsResponseHeaders: every uploads response carries nosniff and the
+// sandboxing Content-Security-Policy; a successful response whose type is
+// not an inline raster image (HTML, SVG, PDF, text, a missing type) gets
+// Content-Disposition: attachment, on GET and HEAD; images stay inline and
+// error responses get no disposition.
+func TestUploadsResponseHeaders(t *testing.T) {
+	serve := func(ct string, status int) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+			if status == http.StatusNotFound {
+				http.NotFound(w, req)
+				return
+			}
+			if ct != "" {
+				w.Header().Set("Content-Type", ct)
+			}
+			if req.Method == http.MethodHead {
+				return
+			}
+			_, _ = w.Write([]byte("<html><script>alert(1)</script></html>"))
+		})
+	}
+	for _, tc := range []struct {
+		ct         string
+		status     int
+		attachment bool
+	}{
+		{"image/png", 200, false},
+		{"image/jpeg", 200, false},
+		{"IMAGE/WEBP; charset=binary", 200, false},
+		{"image/gif", 200, false},
+		{"image/avif", 200, false},
+		{"text/html; charset=utf-8", 200, true},
+		{"image/svg+xml", 200, true},
+		{"application/pdf", 200, true},
+		{"text/plain; charset=utf-8", 200, true},
+		{"application/octet-stream", 200, true},
+		{"", 200, true},
+		{"not a media type;;", 200, true},
+		{"text/plain", http.StatusNotFound, false},
+	} {
+		for _, method := range []string{http.MethodGet, http.MethodHead} {
+			rec := httptest.NewRecorder()
+			serveUploads(rec, httptest.NewRequest(method, "/storage/uploads/abc/123/xyz/abc123xyz.png", nil), serve(tc.ct, tc.status))
+			h := rec.Header()
+			if h.Get("X-Content-Type-Options") != "nosniff" || h.Get("Content-Security-Policy") != uploadsContentSecurityPolicy {
+				t.Fatalf("%s %q: nosniff=%q csp=%q", method, tc.ct, h.Get("X-Content-Type-Options"), h.Get("Content-Security-Policy"))
+			}
+			got := h.Get("Content-Disposition")
+			if tc.attachment && got != "attachment" {
+				t.Fatalf("%s %q (%d): Content-Disposition = %q, want attachment", method, tc.ct, tc.status, got)
+			}
+			if !tc.attachment && got != "" {
+				t.Fatalf("%s %q (%d): Content-Disposition = %q, want none", method, tc.ct, tc.status, got)
+			}
+		}
+	}
+	if !strings.Contains(uploadsContentSecurityPolicy, "sandbox") || !strings.Contains(uploadsContentSecurityPolicy, "default-src 'none'") {
+		t.Fatalf("uploads CSP %q does not sandbox", uploadsContentSecurityPolicy)
+	}
+}