Skip to content

Commit 5f343bc

Browse files
jdomeracki-coderjdomeracki-coder
authored
fix(coderd): backport frame-ancestors CSP fixes to 2.32 (#24474, #24529) (#24806)
Cherry-pick backport of #24474 and #24529 to `release/2.32`. - #24474: fix(coderd): add frame-ancestors CSP directive to prevent clickjacking - #24529: fix(coderd): omit frame-ancestors CSP for embed routes Both commits cherry-picked cleanly with no conflicts. > Generated by Coder Agents
1 parent d6e9344 commit 5f343bc

3 files changed

Lines changed: 114 additions & 20 deletions

File tree

‎coderd/coderd.go‎

Lines changed: 41 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1982,29 +1982,50 @@ func New(options *Options) *API {
19821982

19831983
// Add CSP headers to all static assets and pages. CSP headers only affect
19841984
// browsers, so these don't make sense on api routes.
1985-
cspMW := httpmw.CSPHeaders(
1986-
options.Telemetry.Enabled(), func() []*proxyhealth.ProxyHost {
1987-
if api.DeploymentValues.Dangerous.AllowAllCors {
1988-
// In this mode, allow all external requests.
1989-
return []*proxyhealth.ProxyHost{
1990-
{
1991-
Host: "*",
1992-
AppHost: "*",
1993-
},
1994-
}
1995-
}
1996-
// Always add the primary, since the app host may be on a sub-domain.
1997-
proxies := []*proxyhealth.ProxyHost{
1985+
cspProxyHosts := func() []*proxyhealth.ProxyHost {
1986+
if api.DeploymentValues.Dangerous.AllowAllCors {
1987+
// In this mode, allow all external requests.
1988+
return []*proxyhealth.ProxyHost{
19981989
{
1999-
Host: api.AccessURL.Host,
2000-
AppHost: appurl.ConvertAppHostForCSP(api.AccessURL.Host, api.AppHostname),
1990+
Host: "*",
1991+
AppHost: "*",
20011992
},
20021993
}
2003-
if f := api.WorkspaceProxyHostsFn.Load(); f != nil {
2004-
proxies = append(proxies, (*f)()...)
2005-
}
2006-
return proxies
2007-
}, additionalCSPHeaders)
1994+
}
1995+
// Always add the primary, since the app host may be on a sub-domain.
1996+
proxies := []*proxyhealth.ProxyHost{
1997+
{
1998+
Host: api.AccessURL.Host,
1999+
AppHost: appurl.ConvertAppHostForCSP(api.AccessURL.Host, api.AppHostname),
2000+
},
2001+
}
2002+
if f := api.WorkspaceProxyHostsFn.Load(); f != nil {
2003+
proxies = append(proxies, (*f)()...)
2004+
}
2005+
return proxies
2006+
}
2007+
cspMW := httpmw.CSPHeaders(options.Telemetry.Enabled(), cspProxyHosts, additionalCSPHeaders)
2008+
2009+
// Embed routes (e.g. VS Code extension chat) are designed to be
2010+
// loaded inside iframes, so they must not include frame-ancestors
2011+
// in their CSP. The CSP wildcard '*' only matches network schemes
2012+
// (http, https, ws, wss) and cannot cover custom schemes like
2013+
// vscode-webview://, so the only way to allow all embedders is
2014+
// to omit the directive entirely. If the operator explicitly
2015+
// configured frame-ancestors via CODER_ADDITIONAL_CSP_POLICY,
2016+
// respect that setting.
2017+
2018+
embedCSPHeaders := make(map[httpmw.CSPFetchDirective][]string, len(additionalCSPHeaders))
2019+
for k, v := range additionalCSPHeaders {
2020+
embedCSPHeaders[k] = v
2021+
}
2022+
if _, ok := additionalCSPHeaders[httpmw.CSPFrameAncestors]; !ok {
2023+
embedCSPHeaders[httpmw.CSPFrameAncestors] = []string{}
2024+
}
2025+
embedCSPMW := httpmw.CSPHeaders(options.Telemetry.Enabled(), cspProxyHosts, embedCSPHeaders)
2026+
embedHandler := embedCSPMW(compressHandler(httpmw.HSTS(api.SiteHandler, options.StrictTransportSecurityCfg)))
2027+
r.Get("/agents/{agentId}/embed", embedHandler.ServeHTTP)
2028+
r.Get("/agents/{agentId}/embed/*", embedHandler.ServeHTTP)
20082029

20092030
// Static file handler must be wrapped with HSTS handler if the
20102031
// StrictTransportSecurityAge is set. We only need to set this header on

‎coderd/httpmw/csp.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,22 @@ func CSPHeaders(telemetry bool, proxyHosts func() []*proxyhealth.ProxyHost, stat
142142
cspSrcs.Append(directive, values...)
143143
}
144144

145+
// Default to 'self' to prevent clickjacking unless
146+
// explicitly overridden via staticAdditions (e.g. for
147+
// embeddable routes).
148+
//
149+
// An explicit empty value means "omit frame-ancestors
150+
// entirely", which is needed for embed routes where
151+
// non-network-scheme parents (e.g. vscode-webview://)
152+
// must be able to frame the page. The CSP wildcard '*'
153+
// only matches network schemes (http, https, ws, wss)
154+
// so it cannot cover custom schemes.
155+
if vals, ok := cspSrcs[CSPFrameAncestors]; !ok {
156+
cspSrcs[CSPFrameAncestors] = []string{"'self'"}
157+
} else if len(vals) == 0 {
158+
delete(cspSrcs, CSPFrameAncestors)
159+
}
160+
145161
var csp strings.Builder
146162
for src, vals := range cspSrcs {
147163
_, _ = fmt.Fprintf(&csp, "%s %s; ", src, strings.Join(vals, " "))

‎coderd/httpmw/csp_test.go‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,63 @@ import (
1212
"github.com/coder/coder/v2/coderd/proxyhealth"
1313
)
1414

15+
func TestCSPFrameAncestors(t *testing.T) {
16+
t.Parallel()
17+
18+
t.Run("DefaultSelf", func(t *testing.T) {
19+
t.Parallel()
20+
21+
r := httptest.NewRequest(http.MethodGet, "/", nil)
22+
rw := httptest.NewRecorder()
23+
24+
httpmw.CSPHeaders(false, func() []*proxyhealth.ProxyHost {
25+
return nil
26+
}, nil)(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
27+
rw.WriteHeader(http.StatusOK)
28+
})).ServeHTTP(rw, r)
29+
30+
csp := rw.Header().Get("Content-Security-Policy")
31+
require.Contains(t, csp, "frame-ancestors 'self'")
32+
})
33+
34+
t.Run("OverrideViaStaticAdditions", func(t *testing.T) {
35+
t.Parallel()
36+
37+
r := httptest.NewRequest(http.MethodGet, "/", nil)
38+
rw := httptest.NewRecorder()
39+
40+
httpmw.CSPHeaders(false, func() []*proxyhealth.ProxyHost {
41+
return nil
42+
}, map[httpmw.CSPFetchDirective][]string{
43+
httpmw.CSPFrameAncestors: {"https://example.com"},
44+
})(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
45+
rw.WriteHeader(http.StatusOK)
46+
})).ServeHTTP(rw, r)
47+
48+
csp := rw.Header().Get("Content-Security-Policy")
49+
require.Contains(t, csp, "frame-ancestors https://example.com")
50+
require.NotContains(t, csp, "frame-ancestors 'self'")
51+
})
52+
53+
t.Run("OmitWhenEmpty", func(t *testing.T) {
54+
t.Parallel()
55+
56+
r := httptest.NewRequest(http.MethodGet, "/", nil)
57+
rw := httptest.NewRecorder()
58+
59+
httpmw.CSPHeaders(false, func() []*proxyhealth.ProxyHost {
60+
return nil
61+
}, map[httpmw.CSPFetchDirective][]string{
62+
httpmw.CSPFrameAncestors: {},
63+
})(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
64+
rw.WriteHeader(http.StatusOK)
65+
})).ServeHTTP(rw, r)
66+
67+
csp := rw.Header().Get("Content-Security-Policy")
68+
require.NotContains(t, csp, "frame-ancestors")
69+
})
70+
}
71+
1572
func TestCSP(t *testing.T) {
1673
t.Parallel()
1774

0 commit comments

Comments
 (0)