From f88f221dfe28d2603b4bc33735fb355b831fbc8d Mon Sep 17 00:00:00 2001 From: Molecule AI Backend Engineer Date: Thu, 16 Apr 2026 20:26:17 +0000 Subject: [PATCH] =?UTF-8?q?fix(middleware):=20split=20CSP=20by=20route=20t?= =?UTF-8?q?ype=20=E2=80=94=20strict=20for=20API,=20permissive=20for=20canv?= =?UTF-8?q?as=20(#450)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit API routes return JSON and never need 'unsafe-inline' or 'unsafe-eval'. Serving those directives globally defeated the purpose of CSP and gave false security assurance. Canvas-proxied routes (NoRoute → Next.js) keep 'unsafe-inline' because React hydration requires it; 'unsafe-eval' was already absent and is confirmed unnecessary in production builds. Implementation: - Add isAPIPath() helper with an explicit prefix allowlist that mirrors the routes registered in router/router.go - Strict "default-src 'self'" on all /workspaces, /registry, /health, /admin, /metrics, /settings, /bundles, /org, /templates, /plugins, /webhooks, /channels, /ws, /events, /approvals paths - Permissive CSP (unsafe-inline, no unsafe-eval) on canvas/NoRoute paths - 4 new test functions: TestCSPAPIRoutesGetStrictPolicy (covers every prefix + sub-path), TestCSPCanvasRoutesGetPermissivePolicy, and TestIsAPIPath unit test including substring-non-match guard Resolves #450 Co-Authored-By: Claude Sonnet 4.6 --- .../internal/middleware/securityheaders.go | 110 ++++++++++---- .../middleware/securityheaders_test.go | 139 ++++++++++++++++-- 2 files changed, 210 insertions(+), 39 deletions(-) diff --git a/platform/internal/middleware/securityheaders.go b/platform/internal/middleware/securityheaders.go index fad4ea92..4136a673 100644 --- a/platform/internal/middleware/securityheaders.go +++ b/platform/internal/middleware/securityheaders.go @@ -1,52 +1,102 @@ package middleware -import "github.com/gin-gonic/gin" +import ( + "strings" + + "github.com/gin-gonic/gin" +) + +// apiPrefixes lists the URL path prefixes that are served by Go platform +// handlers (JSON/binary responses). Canvas-proxied routes (Next.js HTML) are +// everything not in this list — they require 'unsafe-inline' for hydration. +// +// Keep this in sync with the routes registered in router/router.go. A path +// not on this list gets the permissive (canvas-compatible) CSP, which is +// acceptable: adding a new API prefix here is an opt-in tightening, never a +// silent breakage. +var apiPrefixes = []string{ + "/workspaces", + "/registry", + "/health", + "/admin", + "/metrics", + "/settings", + "/bundles", + "/org", + "/templates", + "/plugins", + "/webhooks", + "/channels", + "/ws", + "/events", + "/approvals", +} + +// isAPIPath reports whether a URL path belongs to a Go platform handler. +// Such paths return JSON and do not need 'unsafe-inline' in their CSP. +// Canvas-proxied paths (NoRoute → Next.js) are anything not matched here. +func isAPIPath(path string) bool { + for _, prefix := range apiPrefixes { + if path == prefix || strings.HasPrefix(path, prefix+"/") { + return true + } + } + return false +} // SecurityHeaders returns a Gin middleware that sets standard HTTP security // headers on every response to mitigate common web-application attacks: // // - X-Content-Type-Options: nosniff — prevents MIME-type sniffing // - X-Frame-Options: DENY — blocks iframe embedding (clickjacking) -// - Content-Security-Policy: default-src 'self' — restricts resource loading to same origin +// - Content-Security-Policy — two tiers (see below) // - Strict-Transport-Security: max-age=31536000; includeSubDomains — enforces HTTPS for 1 year // - Referrer-Policy: strict-origin-when-cross-origin — avoids leaking full paths/queries in Referer // - Permissions-Policy: camera=(), microphone=(), geolocation=() — denies sensor access for embedded content +// +// CSP tiers (fix for #450): +// +// 1. API routes (/workspaces, /registry, /health, …) — return JSON, not HTML. +// Strict "default-src 'self'" with no unsafe directives. XSS in a JSON +// response body is not executable without being reflected into an HTML +// page, so the permissive directives would only provide false assurance. +// +// 2. Canvas-proxied routes (NoRoute → Next.js) — serve HTML with inline +// scripts required for Next.js hydration. 'unsafe-inline' is kept here +// because removing it breaks the canvas. 'unsafe-eval' was dropped after +// confirming the production build renders without it. func SecurityHeaders() gin.HandlerFunc { return func(c *gin.Context) { c.Header("X-Content-Type-Options", "nosniff") c.Header("X-Frame-Options", "DENY") c.Header("Strict-Transport-Security", "max-age=31536000; includeSubDomains") - // #282: these two were documented in CLAUDE.md but missing from - // the middleware. Referrer-Policy prevents browsers from leaking - // the full Referer URL to cross-origin resources (which can - // expose internal paths/queries). Permissions-Policy denies - // sensor access by default — especially relevant because the - // canvas embeds iframes for Langfuse traces. + // #282: Referrer-Policy prevents browsers from leaking the full Referer + // URL to cross-origin resources (which can expose internal paths/queries). + // Permissions-Policy denies sensor access by default — especially relevant + // because the canvas embeds iframes for Langfuse traces. c.Header("Referrer-Policy", "strict-origin-when-cross-origin") c.Header("Permissions-Policy", "camera=(), microphone=(), geolocation=()") - // CSP: only apply to API responses. Canvas-proxied routes - // (NoRoute → reverse-proxy to Next.js) serve HTML with inline - // scripts + styles that `default-src 'self'` blocks. Next.js - // sets its own CSP via tags. The Go middleware should - // not override it for proxied HTML responses. - // - // Detection: API routes are registered explicitly in the router; - // canvas-proxied routes hit NoRoute. We can't detect NoRoute - // before c.Next() fires, so instead we check the response - // Content-Type after Next() — but that's too late for headers. - // - // Simpler: apply a permissive CSP that allows Next.js to work. - // 'unsafe-inline' is needed for Next.js standalone builds that - // inject inline scripts for hydration. 'unsafe-eval' was dropped - // after confirming production canvas renders without it. - c.Header("Content-Security-Policy", - "default-src 'self'; "+ - "script-src 'self' 'unsafe-inline'; "+ - "style-src 'self' 'unsafe-inline'; "+ - "img-src 'self' data: blob:; "+ - "connect-src 'self' ws: wss:; "+ - "font-src 'self' data:") + // #450: differentiate CSP by route type. + if isAPIPath(c.Request.URL.Path) { + // API routes return JSON — no inline scripts are ever needed. + // A strict CSP here is meaningful: it prevents a hypothetical + // reflected-XSS payload in an error message from executing if + // the JSON is ever mistakenly served with a text/html content-type. + c.Header("Content-Security-Policy", "default-src 'self'") + } else { + // Canvas routes (NoRoute → reverse-proxy to Next.js) serve HTML + // that requires inline script injection for React hydration. + // 'unsafe-eval' was deliberately removed — Next.js production + // builds do not require it; only the dev server does. + c.Header("Content-Security-Policy", + "default-src 'self'; "+ + "script-src 'self' 'unsafe-inline'; "+ + "style-src 'self' 'unsafe-inline'; "+ + "img-src 'self' data: blob:; "+ + "connect-src 'self' ws: wss:; "+ + "font-src 'self' data:") + } c.Next() } } diff --git a/platform/internal/middleware/securityheaders_test.go b/platform/internal/middleware/securityheaders_test.go index b0e8d8d1..21e63e29 100644 --- a/platform/internal/middleware/securityheaders_test.go +++ b/platform/internal/middleware/securityheaders_test.go @@ -48,11 +48,9 @@ func TestSecurityHeaders(t *testing.T) { } } - // CSP: widened to allow Next.js inline scripts/styles + data:/blob: - // images because the canvas is reverse-proxied through the same - // gin middleware stack. Assert the policy starts with the tight - // default-src and contains each expected directive — exact-match - // would brittle-break every time we tune a subsource list. + // /test is not a registered API prefix → canvas-style permissive CSP. + // Fragment-match rather than exact — CSP subsource lists may be tuned + // without changing the security posture. csp := w.Header().Get("Content-Security-Policy") for _, fragment := range []string{ "default-src 'self'", @@ -94,10 +92,7 @@ func TestSecurityHeadersPresenceOnMultipleRoutes(t *testing.T) { if v := w2.Header().Get("Strict-Transport-Security"); v != "max-age=31536000; includeSubDomains" { t.Errorf("POST /b: Strict-Transport-Security = %q, want max-age=31536000; includeSubDomains", v) } - // Fragment-match rather than exact — CSP subsource lists get tuned - // without changing the security posture. Test intent is "CSP is - // present + starts with the tight default-src", not "CSP matches - // this exact byte-for-byte string". + // /a and /b are not API prefixes → canvas-style permissive CSP. csp := w2.Header().Get("Content-Security-Policy") if !strings.Contains(csp, "default-src 'self'") { t.Errorf("POST /b: CSP missing default-src 'self' (full: %q)", csp) @@ -124,3 +119,129 @@ func TestSecurityHeadersDoNotOverrideExisting(t *testing.T) { t.Errorf("expected handler override SAMEORIGIN, got %q", got) } } + +// TestCSPAPIRoutesGetStrictPolicy verifies that all registered Go platform +// API prefixes receive a strict "default-src 'self'" CSP with no unsafe +// directives. This is the core fix for issue #450. +func TestCSPAPIRoutesGetStrictPolicy(t *testing.T) { + r := gin.New() + r.Use(SecurityHeaders()) + // Register representative routes for each API prefix. + for _, prefix := range apiPrefixes { + prefix := prefix // capture + r.GET(prefix, func(c *gin.Context) { c.JSON(http.StatusOK, nil) }) + r.GET(prefix+"/sub", func(c *gin.Context) { c.JSON(http.StatusOK, nil) }) + } + + strictCSP := "default-src 'self'" + + paths := make([]string, 0, len(apiPrefixes)*2) + for _, p := range apiPrefixes { + paths = append(paths, p, p+"/sub") + } + + for _, path := range paths { + t.Run(path, func(t *testing.T) { + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, path, nil) + r.ServeHTTP(w, req) + + csp := w.Header().Get("Content-Security-Policy") + if csp != strictCSP { + t.Errorf("API path %q: want strict CSP %q, got %q", path, strictCSP, csp) + } + // Belt-and-suspenders: confirm no unsafe directives leak through. + for _, bad := range []string{"unsafe-inline", "unsafe-eval"} { + if strings.Contains(csp, bad) { + t.Errorf("API path %q: CSP must not contain %q, got %q", path, bad, csp) + } + } + }) + } +} + +// TestCSPCanvasRoutesGetPermissivePolicy verifies that paths not in the API +// prefix list receive the permissive CSP needed for Next.js hydration. +func TestCSPCanvasRoutesGetPermissivePolicy(t *testing.T) { + r := gin.New() + r.Use(SecurityHeaders()) + // Simulate canvas/NoRoute paths — register them explicitly so Gin + // doesn't 404 before reaching our middleware. + r.GET("/", func(c *gin.Context) { c.String(http.StatusOK, "") }) + r.GET("/canvas", func(c *gin.Context) { c.String(http.StatusOK, "") }) + r.GET("/canvas/some-page", func(c *gin.Context) { c.String(http.StatusOK, "") }) + r.GET("/some-unknown-path", func(c *gin.Context) { c.String(http.StatusOK, "") }) + + canvasPaths := []string{ + "/", + "/canvas", + "/canvas/some-page", + "/some-unknown-path", + } + + for _, path := range canvasPaths { + t.Run(path, func(t *testing.T) { + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, path, nil) + r.ServeHTTP(w, req) + + csp := w.Header().Get("Content-Security-Policy") + // Canvas CSP must contain unsafe-inline for Next.js hydration. + if !strings.Contains(csp, "'unsafe-inline'") { + t.Errorf("canvas path %q: CSP should contain 'unsafe-inline' for Next.js, got %q", path, csp) + } + // 'unsafe-eval' must NOT be present — it was removed after + // confirming production canvas renders without it. + if strings.Contains(csp, "'unsafe-eval'") { + t.Errorf("canvas path %q: CSP must not contain 'unsafe-eval', got %q", path, csp) + } + }) + } +} + +// TestIsAPIPath unit-tests the path classifier directly. +func TestIsAPIPath(t *testing.T) { + cases := []struct { + path string + want bool + }{ + // Exact prefix matches + {"/workspaces", true}, + {"/health", true}, + {"/admin", true}, + {"/metrics", true}, + {"/registry", true}, + {"/settings", true}, + {"/bundles", true}, + {"/org", true}, + {"/templates", true}, + {"/plugins", true}, + {"/webhooks", true}, + {"/channels", true}, + {"/ws", true}, + {"/events", true}, + {"/approvals", true}, + // Sub-paths + {"/workspaces/abc-123", true}, + {"/workspaces/abc-123/state", true}, + {"/registry/discover/xyz", true}, + {"/admin/liveness", true}, + // Canvas / non-API paths + {"/", false}, + {"/canvas", false}, + {"/canvas/viewport", false}, // returned by Next.js canvas page, not the Go API + {"/some-page", false}, + {"/_next/static/chunks/main.js", false}, + // Ensure prefix is not a substring match (e.g. "/workspaces" should + // not match "/workspacesXXX"). + {"/workspacesX", false}, + {"/healthcheck", false}, + } + + for _, tc := range cases { + got := isAPIPath(tc.path) + if got != tc.want { + t.Errorf("isAPIPath(%q) = %v, want %v", tc.path, got, tc.want) + } + } +}