From 01d2548ab37ae03ac57c9c5df2d02d9480aa4a4d Mon Sep 17 00:00:00 2001
From: Jakub Zych <jakub@golem15.com>
Date: Mon, 12 Oct 2026 11:21:38 +0200
Subject: [PATCH] fix(14.2): WR-04 validate storage.uploads.public_path_prefix
 at boot

The prefix was only trimmed. Without a leading slash ServeMux read its first
segment as a host pattern, so every upload silently answered 404, and a CDN
URL panicked in ServeMux and surfaced as a misleading route conflict. With
storage.uploads.serve on, BuildRouter now refuses a prefix that is not an
absolute URL path (scheme, host, query, fragment, whitespace, wildcard
braces, empty, . or .. segments, or a bare /) with an error naming the key.
---
 docs/services/storage.md     |  2 ++
 modules/surf/README.md       |  4 ++--
 modules/surf/uploads.go      | 34 +++++++++++++++++++++++++++++++---
 modules/surf/uploads_test.go | 35 +++++++++++++++++++++++++++++++++++
 4 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/docs/services/storage.md b/docs/services/storage.md
index 7471ae3..d6d1686 100644
--- a/docs/services/storage.md
+++ b/docs/services/storage.md
@@ -57,6 +57,8 @@ The route serves originals and thumbnails and answers 404 for a missing file, an
 - 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.
 
+With `serve` on, `public_path_prefix` must be an absolute URL path such as `/storage/uploads`: a leading slash, no scheme or host, no query, no empty, `.` or `..` segment and no `{` or `}`. Start-up fails with an error naming the key otherwise, for example for the WinterCMS disk path `storage/app/uploads/public` or a CDN URL. With `serve` off the key only builds file URLs, so a CDN origin is fine there.
+
 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/modules/surf/README.md b/modules/surf/README.md
index c93e815..81a92a9 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. 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 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, and when the prefix is not an absolute URL path (no leading slash, a scheme or host, a query, an empty, `.` or `..` segment, or `{`/`}`).
 - 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`, 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`. |
+| `storage.uploads.serve` | `false` | Mounts the uploads route `GET <storage.uploads.public_path_prefix>/{path...}` (prefix default `/storage/uploads`; it must then be an absolute URL path, or start-up fails) 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 7b54c5d..c22af91 100644
--- a/modules/surf/uploads.go
+++ b/modules/surf/uploads.go
@@ -57,9 +57,9 @@ func mountUploads(r *Router, app *backpack.App) error {
 	if !app.Config.Bool("storage.uploads.serve") {
 		return nil
 	}
-	prefix := strings.TrimSuffix(strings.TrimSpace(app.Config.String("storage.uploads.public_path_prefix")), "/")
-	if prefix == "" {
-		prefix = defaultUploadsPrefix
+	prefix, err := uploadsPrefix(app.Config.String("storage.uploads.public_path_prefix"))
+	if err != nil {
+		return err
 	}
 	for _, rt := range r.routes {
 		if rt.path == prefix || strings.HasPrefix(rt.path, prefix+"/") {
@@ -74,6 +74,34 @@ func mountUploads(r *Router, app *backpack.App) error {
 	return nil
 }
 
+// uploadsPrefix returns the route prefix for a storage.uploads.public_path_prefix
+// value: trimmed, without a trailing slash, defaultUploadsPrefix when empty.
+// The route needs an absolute URL path: a leading slash, no scheme or host,
+// no empty, "." or ".." segment, no query, fragment, whitespace or ServeMux
+// wildcard braces. Anything else is refused, because ServeMux would read a
+// value without a leading slash as a host pattern (every upload then 404s)
+// and panic on a URL.
+func uploadsPrefix(raw string) (string, error) {
+	value := strings.TrimSpace(raw)
+	if value == "" {
+		return defaultUploadsPrefix, nil
+	}
+	prefix := strings.TrimSuffix(value, "/")
+	bad := !strings.HasPrefix(prefix, "/") || strings.ContainsAny(prefix, "{}?# \t\r\n\\")
+	if !bad {
+		for _, seg := range strings.Split(prefix[1:], "/") {
+			if seg == "" || seg == "." || seg == ".." {
+				bad = true
+				break
+			}
+		}
+	}
+	if bad {
+		return "", fmt.Errorf("surf: storage.uploads.serve needs storage.uploads.public_path_prefix to be an absolute URL path such as /storage/uploads (no scheme, host, query or wildcard), got %q", raw)
+	}
+	return prefix, nil
+}
+
 // uploadsHandler resolves the published bucket and database on every request
 // and answers 404 while either is missing.
 func uploadsHandler(app *backpack.App, prefix string) http.HandlerFunc {
diff --git a/modules/surf/uploads_test.go b/modules/surf/uploads_test.go
index 6ded316..472cd0f 100644
--- a/modules/surf/uploads_test.go
+++ b/modules/surf/uploads_test.go
@@ -159,6 +159,41 @@ func TestUploadsMount(t *testing.T) {
 		}
 	})
 
+	t.Run("a prefix that is not an absolute URL path fails BuildRouter", func(t *testing.T) {
+		for _, prefix := range []string{
+			"storage/uploads",                 // no leading slash: ServeMux reads "storage" as a host
+			"storage/app/uploads/public",      // the WinterCMS disk path, not its URL
+			"https://cdn.example.com/uploads", // a CDN origin
+			"//cdn.example.com/uploads",       // a scheme-relative URL
+			"/storage//uploads",
+			"/storage/{id}/uploads",
+			"/storage/uploads?x=1",
+			"/storage/uploads#top",
+			"/storage/../uploads",
+			"/storage/./uploads",
+			"/storage/up loads",
+			"/",
+		} {
+			app := backpack.New(uploadsConfig(t, map[string]any{"storage.uploads.serve": true, "storage.uploads.public_path_prefix": prefix}))
+			_, err := BuildRouter(app, nil)
+			if err == nil || !strings.Contains(err.Error(), "storage.uploads.public_path_prefix") || !strings.Contains(err.Error(), "absolute URL path") {
+				t.Fatalf("prefix %q: err = %v, want a refusal naming storage.uploads.public_path_prefix", prefix, err)
+			}
+		}
+		// Valid prefixes, with or without a trailing slash, and an invalid
+		// one while serve is off (the key then only feeds attach.PublicURL).
+		for _, prefix := range []string{"/storage/uploads", "/storage/app/uploads/public/", "/media", " /files "} {
+			app := backpack.New(uploadsConfig(t, map[string]any{"storage.uploads.serve": true, "storage.uploads.public_path_prefix": prefix}))
+			if _, err := BuildRouter(app, nil); err != nil {
+				t.Fatalf("prefix %q: %v", prefix, err)
+			}
+		}
+		app := backpack.New(uploadsConfig(t, map[string]any{"storage.uploads.public_path_prefix": "https://cdn.example.com/uploads"}))
+		if _, err := BuildRouter(app, nil); err != nil {
+			t.Fatalf("serve off with a CDN prefix: %v", err)
+		}
+	})
+
 	t.Run("a plugin route at or under the prefix fails BuildRouter", func(t *testing.T) {
 		for _, path := range []string{"/storage/uploads/{path...}", "/storage/uploads/avatars/{id}", "/storage/uploads"} {
 			app := backpack.New(uploadsConfig(t, map[string]any{"storage.uploads.serve": true}))