diff --git a/backend/app/rest/api/rest.go b/backend/app/rest/api/rest.go index 964fd00a..d2ae7aff 100644 --- a/backend/app/rest/api/rest.go +++ b/backend/app/rest/api/rest.go @@ -249,6 +249,7 @@ func (s *Rest) routes() chi.Router { // api routes router.Route("/api/v1", func(rapi chi.Router) { + rapi.Use(apiCSPMiddleware) rapi.Group(func(rava chi.Router) { rava.Use(middleware.Timeout(5 * time.Second)) rava.Use(rateLimiter(100)) @@ -269,7 +270,6 @@ func (s *Rest) routes() chi.Router { ropen.Post("/counts", s.pubRest.countMultiCtrl) ropen.Get("/list", s.pubRest.listCtrl) ropen.Get("/info", s.pubRest.infoCtrl) - ropen.Get("/img", s.ImageProxy.Handler) ropen.Route("/rss", func(rrss chi.Router) { rrss.Get("/post", s.rssRest.postCommentsCtrl) @@ -278,11 +278,17 @@ func (s *Rest) routes() chi.Router { }) }) - // open routes, cached + // open routes, cached. /img lives here (not in the NoCache group above) because + // middleware.NoCache strips If-None-Match from incoming requests, which would + // defeat the proxy handler's 304 short-circuit. The handler sets a 30-day + // max-age on validated success responses (with a versioned etag for cache + // invalidation on revalidation); error responses get Cache-Control: no-store + // so transient failures aren't pinned in the cache. rapi.Group(func(ropen chi.Router) { ropen.Use(middleware.Timeout(30 * time.Second)) ropen.Use(rateLimiter(10)) ropen.Use(authMiddleware.Trace, logInfoWithBody) + ropen.Get("/img", s.ImageProxy.Handler) ropen.Get("/picture/{user}/{id}", s.pubRest.loadPictureCtrl) ropen.Get("/qr/telegram", s.pubRest.telegramQrCtrl) }) @@ -620,6 +626,26 @@ func cacheControl(expiration time.Duration, version string) func(http.Handler) h } } +// apiCSPMiddleware overrides the global Content-Security-Policy on /api/v1 routes +// with a strict, default-deny policy. The global CSP (securityHeadersMiddleware) keeps +// 'self' 'unsafe-inline' for script-src/style-src because the widget HTML pages +// (/web/*.html) need inline bootstrap blocks. API responses serve JSON, XML/RSS, or +// images — none of those should ever execute scripts when rendered, so they get the +// strictest policy available as defense-in-depth against future trust-boundary bugs. +// +// Image-serving handlers (/api/v1/img, /api/v1/picture/{user}/{id}) re-apply the same +// rest.StrictImageCSP value at the handler level and additionally set Content-Disposition: +// inline; filename="image" (framing the response as a file rather than a renderable +// document) and X-Content-Type-Options: nosniff. The CSP re-apply is intentional belt-and- +// braces: if a future route refactor bypasses this middleware, the image handlers still +// emit the policy. +func apiCSPMiddleware(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Security-Policy", rest.StrictImageCSP) + next.ServeHTTP(w, r) + }) +} + // securityHeadersMiddleware sets security-related headers: // - Content-Security-Policy: controls which resources the browser is allowed to load // - Permissions-Policy: disables browser features (camera, mic, etc.) not needed by a comment widget @@ -639,7 +665,8 @@ func securityHeadersMiddleware(imageProxyEnabled bool, allowedAncestors []string if len(allowedAncestors) > 0 { frameAncestors = strings.Join(allowedAncestors, " ") } - w.Header().Set("Content-Security-Policy", fmt.Sprintf("default-src 'none'; base-uri 'none'; form-action 'none'; connect-src 'self'; frame-src 'self' mailto:; img-src %s; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; font-src data:; object-src 'none'; frame-ancestors %s;", imgSrc, frameAncestors)) + // font-src is set to 'none' (no @font-face / no base64 fonts in the bundle). + w.Header().Set("Content-Security-Policy", fmt.Sprintf("default-src 'none'; base-uri 'none'; form-action 'none'; connect-src 'self'; frame-src 'self' mailto:; img-src %s; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; font-src 'none'; object-src 'none'; frame-ancestors %s;", imgSrc, frameAncestors)) w.Header().Set("Permissions-Policy", "accelerometer=(), autoplay=(), camera=(), cross-origin-isolated=(), display-capture=(), encrypted-media=(), fullscreen=(), geolocation=(), gyroscope=(), keyboard-map=(), magnetometer=(), microphone=(), midi=(), payment=(), picture-in-picture=(), publickey-credentials-get=(), screen-wake-lock=(), sync-xhr=(), usb=(), xr-spatial-tracking=(), clipboard-read=(), clipboard-write=(), gamepad=(), hid=(), idle-detection=(), interest-cohort=(), serial=(), unload=(), window-management=()") w.Header().Set("X-Content-Type-Options", "nosniff") w.Header().Set("Referrer-Policy", "strict-origin-when-cross-origin") diff --git a/backend/app/rest/api/rest_public.go b/backend/app/rest/api/rest_public.go index 370929a1..e05d453b 100644 --- a/backend/app/rest/api/rest_public.go +++ b/backend/app/rest/api/rest_public.go @@ -390,33 +390,56 @@ func safePictureSegment(seg string) bool { return true } +// sendPictureError writes a no-store Cache-Control header and delegates to rest.SendErrorJSON. +// Used by every rejection branch in loadPictureCtrl so error responses never inherit the +// 7-day client cache of the success path. +func sendPictureError(w http.ResponseWriter, r *http.Request, status int, err error, details string, code int) { + w.Header().Set("Cache-Control", "no-store") + rest.SendErrorJSON(w, r, status, err, details, code) +} + // GET /picture/{user}/{id} - get picture func (s *public) loadPictureCtrl(w http.ResponseWriter, r *http.Request) { + rest.SetImageDefenseHeaders(w) + user, imgID := chi.URLParam(r, "user"), chi.URLParam(r, "id") if user == "" || imgID == "" || !safePictureSegment(user) || !safePictureSegment(imgID) { log.Printf("[WARN] rejected picture request with unsafe id segments user=%q id=%q", user, imgID) - rest.SendErrorJSON(w, r, http.StatusBadRequest, fmt.Errorf("invalid picture id"), "invalid picture id", rest.ErrAssetNotFound) + sendPictureError(w, r, http.StatusBadRequest, fmt.Errorf("invalid picture id"), "invalid picture id", rest.ErrAssetNotFound) return } id := user + "/" + imgID img, err := s.imageService.Load(id) if err != nil { log.Printf("[WARN] can't load image %s: %v", id, err) - rest.SendErrorJSON(w, r, http.StatusBadRequest, fmt.Errorf("image not found"), "can't get image", rest.ErrAssetNotFound) + sendPictureError(w, r, http.StatusBadRequest, fmt.Errorf("image not found"), "can't get image", rest.ErrAssetNotFound) return } - // enforce client-side caching + + contentType, err := rest.SafeImgContentType(img) + if err != nil { + log.Printf("[WARN] rejecting non-image picture %s: %v", id, err) + sendPictureError(w, r, http.StatusUnsupportedMediaType, err, "invalid image content", rest.ErrAssetNotFound) + return + } + + // /picture/ does not need a security-version etag prefix — the upload flow + // validates input format (readAndValidateImage) and the serve path re-validates + // the stored bytes via rest.SafeImgContentType. Bytes within the resize dimension + // limits ARE preserved verbatim by resize, so the browser defense relies on the + // response headers (validated Content-Type + nosniff + strict CSP + + // Content-Disposition: inline), not on byte normalization. Picture IDs are limited + // to safePictureSegment (alphanumeric xid-generated guids), so the comma split + // inside rest.EtagMatches cannot collide; if the ID format ever changes, revisit. etag := `"` + id + `"` w.Header().Set("Etag", etag) w.Header().Set("Cache-Control", "max-age=604800") // 7 days - if match := r.Header.Get("If-None-Match"); match != "" { - if strings.Contains(match, etag) { - w.WriteHeader(http.StatusNotModified) - return - } + if match := r.Header.Get("If-None-Match"); match != "" && rest.EtagMatches(match, etag) { + w.WriteHeader(http.StatusNotModified) + return } - w.Header().Set("Content-Type", s.imageService.ImgContentType(img)) + w.Header().Set("Content-Type", contentType) w.Header().Set("Content-Length", strconv.Itoa(len(img))) w.WriteHeader(http.StatusOK) if _, err = io.Copy(w, bytes.NewReader(img)); err != nil { diff --git a/backend/app/rest/api/rest_public_test.go b/backend/app/rest/api/rest_public_test.go index 54bca27a..67740732 100644 --- a/backend/app/rest/api/rest_public_test.go +++ b/backend/app/rest/api/rest_public_test.go @@ -1,9 +1,11 @@ package api import ( + "bytes" "encoding/json" "fmt" "io" + "mime/multipart" "net/http" "net/http/httptest" "os" @@ -12,6 +14,7 @@ import ( "testing" "time" + "github.com/go-chi/chi/v5" cache "github.com/go-pkgz/lcw/v2" R "github.com/go-pkgz/rest" "github.com/google/uuid" @@ -19,6 +22,7 @@ import ( "github.com/stretchr/testify/require" "github.com/umputun/remark42/backend/app/store" + "github.com/umputun/remark42/backend/app/store/image" "github.com/umputun/remark42/backend/app/store/service" ) @@ -1104,3 +1108,113 @@ func TestRest_LoadPictureRejectsControlCharsInSegment(t *testing.T) { }) } } + +// TestRest_LoadPictureDefenseHeaders saves a real PNG via the standard upload handler +// and asserts that GET /api/v1/picture/{user}/{id} carries the layered defense headers +// (strict CSP, nosniff, Content-Disposition with filename) and that the strict ETag +// matcher does not 304 on a substring-of-the-real-etag (the pre-fix matcher would). +func TestRest_LoadPictureDefenseHeaders(t *testing.T) { + ts, _, teardown := startupT(t) + defer teardown() + + // upload a real PNG via /api/v1/picture + bodyBuf := &bytes.Buffer{} + bodyWriter := multipart.NewWriter(bodyBuf) + fileWriter, err := bodyWriter.CreateFormFile("file", "picture.png") + require.NoError(t, err) + _, err = io.Copy(fileWriter, gopherPNG()) + require.NoError(t, err) + contentType := bodyWriter.FormDataContentType() + require.NoError(t, bodyWriter.Close()) + + client := http.Client{} + defer client.CloseIdleConnections() + req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("%s/api/v1/picture?site=remark42", ts.URL), bodyBuf) + require.NoError(t, err) + req.Header.Add("Content-Type", contentType) + req.Header.Add("X-JWT", devToken) + resp, err := client.Do(req) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + m := map[string]string{} + require.NoError(t, json.Unmarshal(body, &m)) + require.NotEmpty(t, m["id"]) + + // fetch the picture and assert defense headers + resp, err = http.Get(fmt.Sprintf("%s/api/v1/picture/%s", ts.URL, m["id"])) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, "default-src 'none'; sandbox; frame-ancestors 'none'", + resp.Header.Get("Content-Security-Policy")) + assert.Equal(t, "nosniff", resp.Header.Get("X-Content-Type-Options")) + assert.Equal(t, `inline; filename="image"`, resp.Header.Get("Content-Disposition")) + assert.Equal(t, "image/png", resp.Header.Get("Content-Type")) + realEtag := resp.Header.Get("Etag") + require.NotEmpty(t, realEtag) + + // strict matcher: an If-None-Match value that CONTAINS the real etag as a substring + // but is not equal to it must NOT trigger 304. The pre-fix matcher used + // strings.Contains(header, etag) and would have returned true here. + require.True(t, len(realEtag) > 4) + substringMatch := "prefix-" + realEtag + "-suffix" + req2, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/api/v1/picture/%s", ts.URL, m["id"]), http.NoBody) + require.NoError(t, err) + req2.Header.Set("If-None-Match", substringMatch) + resp2, err := client.Do(req2) + require.NoError(t, err) + defer resp2.Body.Close() + assert.Equal(t, http.StatusOK, resp2.StatusCode, + "strict etag matcher must NOT 304 when real etag appears only as a substring of If-None-Match; got %q vs real %q", substringMatch, realEtag) + + // sanity: the exact real etag DOES validate + req3, err := http.NewRequest(http.MethodGet, fmt.Sprintf("%s/api/v1/picture/%s", ts.URL, m["id"]), http.NoBody) + require.NoError(t, err) + req3.Header.Set("If-None-Match", realEtag) + resp3, err := client.Do(req3) + require.NoError(t, err) + defer resp3.Body.Close() + assert.Equal(t, http.StatusNotModified, resp3.StatusCode, "exact etag must round-trip as 304") +} + +// TestRest_LoadPictureRejectsNonImage proves the /picture/ handler rejects bytes that +// don't sniff as a real image — even when retrieved successfully from the image store. +// Uses a StoreMock so we can return arbitrary attacker bytes for a valid-looking id. +func TestRest_LoadPictureRejectsNonImage(t *testing.T) { + htmlBody := []byte("
") + + imageStore := image.StoreMock{LoadFunc: func(string) ([]byte, error) { + return htmlBody, nil + }} + // minimal public struct on purpose: the reject path only exercises imageService.Load + // (other fields like dataService, cache, commentFormatter are not touched here). + p := &public{imageService: image.NewService(&imageStore, image.ServiceParams{})} + + router := chi.NewRouter() + router.Get("/api/v1/picture/{user}/{id}", p.loadPictureCtrl) + ts := httptest.NewServer(router) + defer ts.Close() + + resp, err := http.Get(ts.URL + "/api/v1/picture/dev_user/abc.png") + require.NoError(t, err) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + + assert.Equal(t, http.StatusUnsupportedMediaType, resp.StatusCode, + "non-image bytes must be rejected as 415") + assert.False(t, strings.HasPrefix(resp.Header.Get("Content-Type"), "text/html"), + "reject response must not be text/html; got %q", resp.Header.Get("Content-Type")) + assert.NotContains(t, string(body), "