diff --git a/README.md b/README.md index ed5b3b0..6129f36 100644 --- a/README.md +++ b/README.md @@ -238,7 +238,7 @@ variables set by the file's `env:` section are checked the same way. | `PIXA_UPSTREAM_FETCH_TIMEOUT` | `upstream_fetch_timeout` | Time allowed for one fetch from an upstream host; default `30s` | | `PIXA_UPSTREAM_MAX_RESPONSE_SIZE` | `upstream_max_response_size` | Largest upstream response accepted, in bytes; default 50 MiB | | `PIXA_DOWNSTREAM_TIMEOUT` | `downstream_timeout` | Time allowed for answering one client request; default `60s` | -| `PIXA_ACCESS_CONTROL_ALLOW_ORIGIN` | `access_control_allow_origin` | CORS origin allowed to read responses: `*` or one origin; default `*` | +| `PIXA_ACCESS_CONTROL_ALLOW_ORIGIN` | `access_control_allow_origin` | CORS origin allowed to read image responses: `*` or one origin; default `*` | | `PIXA_METRICS_USERNAME` | `metrics.username` | Username for `/metrics`, which is served only when both are set | | `PIXA_METRICS_PASSWORD` | `metrics.password` | Password for `/metrics`; set together with the username | | `PIXA_SENTRY_DSN` | `sentry_dsn` | Sentry DSN for error reporting; empty disables it | @@ -247,8 +247,9 @@ variables set by the file's `env:` section are checked the same way. Key settings in more detail: -- `access_control_allow_origin` — the origin a browser lets read pixa's - responses, sent as the CORS `Access-Control-Allow-Origin` header: `*`, the +- `access_control_allow_origin` — the origin a browser lets read the responses + of the image routes, `/v1/image/` and `/v1/e/`, sent as the CORS + `Access-Control-Allow-Origin` header; no other route sends it. `*`, the default, is any site; otherwise one `http` or `https` origin such as `https://example.com`, whose host is a lowercase host name (letters, digits, hyphens and dots, with a letter in its last part) or an IP address diff --git a/TODO.md b/TODO.md index 637e3a5..3f394f9 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,12 @@ P2: security: referer blacklist # Completed Steps +- 2026-09-29 only the image routes send CORS headers (closes #98): the CORS + middleware, with the `access_control_allow_origin` origin, moved from the + router root onto a `/v1` subrouter holding `/v1/image/` and `/v1/e/`, where it + still answers a preflight `OPTIONS` request; the login and URL generator + pages, `/metrics` and the other routes send no `Access-Control-Allow-Origin`; + documented in `README.md` and `config.example.yml`. - 2026-09-29 the container makes `/var/lib/pixa` usable by itself (closes #159): `deploy/docker-entrypoint.sh` creates the directory if it is missing, gives the directory and everything in it to `pixad` when the directory or one diff --git a/config.example.yml b/config.example.yml index ebbc833..ac32c98 100644 --- a/config.example.yml +++ b/config.example.yml @@ -103,8 +103,9 @@ upstream_max_response_size: 52428800 # longer than upstream_fetch_timeout plus 20 seconds. downstream_timeout: 60s -# The origin a browser lets read pixa's responses, sent as the CORS -# Access-Control-Allow-Origin header: "*" (the default) is any site; +# The origin a browser lets read the responses of the image routes, +# /v1/image/ and /v1/e/, sent as the CORS Access-Control-Allow-Origin +# header; no other route sends it. "*" (the default) is any site; # otherwise one http or https origin such as https://example.com, whose # host is a lowercase host name (letters, digits, hyphens and dots, with a # letter in its last part) or an IP address (IPv6 in brackets, in its diff --git a/internal/server/cors_internal_test.go b/internal/server/cors_internal_test.go new file mode 100644 index 0000000..6232588 --- /dev/null +++ b/internal/server/cors_internal_test.go @@ -0,0 +1,64 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// TestCORSOnlyOnImageRoutes verifies that the image routes answer with the +// configured access_control_allow_origin, a preflight request included, and +// that the login and URL generator pages send no Access-Control-Allow-Origin, +// so no other site can read them. /metrics is left out: its middleware +// registers with the process-wide Prometheus registry, which only one test +// in this package can do. +func TestCORSOnlyOnImageRoutes(t *testing.T) { + t.Parallel() + + const appOrigin = "https://app.example.com" + + s := newTestServer(t) + s.config.AccessControlAllowOrigin = appOrigin + s.SetupRoutes() + + requests := []struct { + method string + path string + want string + }{ + {http.MethodGet, unsignedImagePath, appOrigin}, + {http.MethodHead, unsignedImagePath, appOrigin}, + {http.MethodOptions, unsignedImagePath, appOrigin}, + {http.MethodGet, encryptedImagePath, appOrigin}, + {http.MethodGet, "/", ""}, + {http.MethodOptions, "/", ""}, + {http.MethodPost, "/generate", ""}, + {http.MethodGet, "/logout", ""}, + } + + for _, tc := range requests { + t.Run(tc.method+" "+tc.path, func(t *testing.T) { + t.Parallel() + + req := httptest.NewRequestWithContext( + t.Context(), tc.method, tc.path, nil) + req.Header.Set("Origin", appOrigin) + + // An OPTIONS request naming the method it asks about is the + // preflight a browser sends before some cross-origin requests. + if tc.method == http.MethodOptions { + req.Header.Set("Access-Control-Request-Method", http.MethodGet) + } + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + t.Logf("status %d", rec.Code) + + got := rec.Header().Get("Access-Control-Allow-Origin") + if got != tc.want { + t.Errorf("Access-Control-Allow-Origin = %q, want %q", + got, tc.want) + } + }) + } +} diff --git a/internal/server/maintenance_internal_test.go b/internal/server/maintenance_internal_test.go index 96b7272..f800005 100644 --- a/internal/server/maintenance_internal_test.go +++ b/internal/server/maintenance_internal_test.go @@ -13,6 +13,10 @@ import ( // unsignedImagePath is an image URL that carries no signature. const unsignedImagePath = "/v1/image/cdn.example.com/cat.jpg/100x100.jpeg" +// encryptedImagePath is an encrypted image URL whose token cannot be +// decrypted. +const encryptedImagePath = "/v1/e/token/cat.jpg" + // TestMaintenanceModeRefusesImageRequests verifies that while maintenance // mode is on, both image routes answer 503 Service Unavailable with a // Retry-After header and the JSON error body the image handlers send. @@ -28,7 +32,7 @@ func TestMaintenanceModeRefusesImageRequests(t *testing.T) { }{ {http.MethodGet, unsignedImagePath}, {http.MethodHead, unsignedImagePath}, - {http.MethodGet, "/v1/e/token/cat.jpg"}, + {http.MethodGet, encryptedImagePath}, } for _, tc := range requests { @@ -95,7 +99,7 @@ func TestImageRequestsServedWithoutMaintenanceMode(t *testing.T) { }{ {http.MethodGet, unsignedImagePath, http.StatusUnauthorized}, {http.MethodHead, unsignedImagePath, http.StatusUnauthorized}, - {http.MethodGet, "/v1/e/token/cat.jpg", http.StatusBadRequest}, + {http.MethodGet, encryptedImagePath, http.StatusBadRequest}, } for _, tc := range requests { diff --git a/internal/server/routes.go b/internal/server/routes.go index ade30ee..f341e8a 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -38,7 +38,6 @@ func (s *Server) SetupRoutes() { s.router.Use(s.mw.Metrics()) } - s.router.Use(s.mw.CORS()) s.router.Use(middleware.Timeout(s.config.DownstreamTimeout)) if s.sentryEnabled { @@ -74,22 +73,31 @@ func (s *Server) SetupRoutes() { s.router.Get("/logout", s.h.HandleLogout()) - // Image routes, refused while maintenance mode is on. Only these: the - // image's Docker HEALTHCHECK requests the health check, a 503 there - // would make the container unhealthy, and upaas marks a deploy failed - // when its container is unhealthy. - s.router.Group(func(r chi.Router) { - r.Use(s.refuseDuringMaintenance) + // Image routes, the only ones that send CORS headers, as pages on other + // sites read them. They are a subrouter rather than a group: a group's + // middleware runs only for a request that matches one of its routes, + // and a browser's preflight OPTIONS request matches none, so the CORS + // middleware could not answer it. + s.router.Route("/v1", func(r chi.Router) { + r.Use(s.mw.CORS()) - // Main image proxy route - // /v1/image///x. - r.Get("/v1/image/*", s.h.HandleImage()) - r.Head("/v1/image/*", s.h.HandleImage()) + // Refused while maintenance mode is on. Only these: the image's + // Docker HEALTHCHECK requests the health check, a 503 there would + // make the container unhealthy, and upaas marks a deploy failed + // when its container is unhealthy. + r.Group(func(r chi.Router) { + r.Use(s.refuseDuringMaintenance) - // Encrypted image URL route - // The trailing filename (e.g., /img.jpg) is ignored but helps - // browsers with content type - r.Get("/v1/e/{token}/*", s.h.HandleImageEnc()) + // Main image proxy route + // /v1/image///x. + r.Get("/image/*", s.h.HandleImage()) + r.Head("/image/*", s.h.HandleImage()) + + // Encrypted image URL route + // The trailing filename (e.g., /img.jpg) is ignored but helps + // browsers with content type + r.Get("/e/{token}/*", s.h.HandleImageEnc()) + }) }) // Metrics endpoint with auth