From 615be176b82070a729e51852ede41bc6f7151257 Mon Sep 17 00:00:00 2001 From: Jakub Domeracki Date: Mon, 20 Apr 2026 13:01:46 +0200 Subject: [PATCH] fix(coderd): add frame-ancestors CSP directive to prevent clickjacking (#24474) --- coderd/coderd.go | 57 +++++++++++++++++++++++++-------------- coderd/httpmw/csp.go | 7 +++++ coderd/httpmw/csp_test.go | 39 +++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 20 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 700a5d8a88..f07a085dc9 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -2002,29 +2002,46 @@ func New(options *Options) *API { // Add CSP headers to all static assets and pages. CSP headers only affect // browsers, so these don't make sense on api routes. - cspMW := httpmw.CSPHeaders( - options.Telemetry.Enabled(), func() []*proxyhealth.ProxyHost { - if api.DeploymentValues.Dangerous.AllowAllCors { - // In this mode, allow all external requests. - return []*proxyhealth.ProxyHost{ - { - Host: "*", - AppHost: "*", - }, - } - } - // Always add the primary, since the app host may be on a sub-domain. - proxies := []*proxyhealth.ProxyHost{ + cspProxyHosts := func() []*proxyhealth.ProxyHost { + if api.DeploymentValues.Dangerous.AllowAllCors { + // In this mode, allow all external requests. + return []*proxyhealth.ProxyHost{ { - Host: api.AccessURL.Host, - AppHost: appurl.ConvertAppHostForCSP(api.AccessURL.Host, api.AppHostname), + Host: "*", + AppHost: "*", }, } - if f := api.WorkspaceProxyHostsFn.Load(); f != nil { - proxies = append(proxies, (*f)()...) - } - return proxies - }, additionalCSPHeaders) + } + // Always add the primary, since the app host may be on a sub-domain. + proxies := []*proxyhealth.ProxyHost{ + { + Host: api.AccessURL.Host, + AppHost: appurl.ConvertAppHostForCSP(api.AccessURL.Host, api.AppHostname), + }, + } + if f := api.WorkspaceProxyHostsFn.Load(); f != nil { + proxies = append(proxies, (*f)()...) + } + return proxies + } + cspMW := httpmw.CSPHeaders(options.Telemetry.Enabled(), cspProxyHosts, additionalCSPHeaders) + + // Embed routes (e.g. VS Code extension chat) are designed to be + // loaded inside iframes, so they need a relaxed frame-ancestors + // policy instead of the default 'self'. However, if the operator + // explicitly configured frame-ancestors via CODER_ADDITIONAL_CSP_POLICY, + // respect that setting. + embedCSPHeaders := make(map[httpmw.CSPFetchDirective][]string, len(additionalCSPHeaders)) + for k, v := range additionalCSPHeaders { + embedCSPHeaders[k] = v + } + if _, ok := additionalCSPHeaders[httpmw.CSPFrameAncestors]; !ok { + embedCSPHeaders[httpmw.CSPFrameAncestors] = []string{"*"} + } + embedCSPMW := httpmw.CSPHeaders(options.Telemetry.Enabled(), cspProxyHosts, embedCSPHeaders) + embedHandler := embedCSPMW(compressHandler(httpmw.HSTS(api.SiteHandler, options.StrictTransportSecurityCfg))) + r.Get("/agents/{agentId}/embed", embedHandler.ServeHTTP) + r.Get("/agents/{agentId}/embed/*", embedHandler.ServeHTTP) // Static file handler must be wrapped with HSTS handler if the // StrictTransportSecurityAge is set. We only need to set this header on diff --git a/coderd/httpmw/csp.go b/coderd/httpmw/csp.go index f39781ad51..a42567e342 100644 --- a/coderd/httpmw/csp.go +++ b/coderd/httpmw/csp.go @@ -142,6 +142,13 @@ func CSPHeaders(telemetry bool, proxyHosts func() []*proxyhealth.ProxyHost, stat cspSrcs.Append(directive, values...) } + // Default to 'self' to prevent clickjacking unless + // explicitly overridden via staticAdditions (e.g. for + // embeddable routes). + if _, ok := cspSrcs[CSPFrameAncestors]; !ok { + cspSrcs[CSPFrameAncestors] = []string{"'self'"} + } + var csp strings.Builder for src, vals := range cspSrcs { _, _ = fmt.Fprintf(&csp, "%s %s; ", src, strings.Join(vals, " ")) diff --git a/coderd/httpmw/csp_test.go b/coderd/httpmw/csp_test.go index ba88320e6f..8af2755514 100644 --- a/coderd/httpmw/csp_test.go +++ b/coderd/httpmw/csp_test.go @@ -12,6 +12,45 @@ import ( "github.com/coder/coder/v2/coderd/proxyhealth" ) +func TestCSPFrameAncestors(t *testing.T) { + t.Parallel() + + t.Run("DefaultSelf", func(t *testing.T) { + t.Parallel() + + r := httptest.NewRequest(http.MethodGet, "/", nil) + rw := httptest.NewRecorder() + + httpmw.CSPHeaders(false, func() []*proxyhealth.ProxyHost { + return nil + }, nil)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusOK) + })).ServeHTTP(rw, r) + + csp := rw.Header().Get("Content-Security-Policy") + require.Contains(t, csp, "frame-ancestors 'self'") + }) + + t.Run("OverrideViaStaticAdditions", func(t *testing.T) { + t.Parallel() + + r := httptest.NewRequest(http.MethodGet, "/", nil) + rw := httptest.NewRecorder() + + httpmw.CSPHeaders(false, func() []*proxyhealth.ProxyHost { + return nil + }, map[httpmw.CSPFetchDirective][]string{ + httpmw.CSPFrameAncestors: {"*"}, + })(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + rw.WriteHeader(http.StatusOK) + })).ServeHTTP(rw, r) + + csp := rw.Header().Get("Content-Security-Policy") + require.Contains(t, csp, "frame-ancestors *") + require.NotContains(t, csp, "frame-ancestors 'self'") + }) +} + func TestCSP(t *testing.T) { t.Parallel()