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.
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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}))
|
||||
|
||||
Reference in New Issue
Block a user