From ddbc950f466ff7fa4c0b2dfb11489311b0d384f2 Mon Sep 17 00:00:00 2001 From: Andrew Lytvynov Date: Thu, 31 Oct 2024 14:13:29 -0500 Subject: [PATCH] safeweb: add support for custom CSP (#13975) To allow more flexibility with CSPs, add a fully customizable `CSP` type that can be provided in `Config` and encodes itself into the correct format. Preserve the `CSPAllowInlineStyles` option as is today, but maybe that'll get deprecated later in favor of the new CSP field. In particular, this allows for pages loading external JS, or inline JS with nonces or hashes (see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#unsafe_inline_script) Updates https://github.com/tailscale/corp/issues/8027 Signed-off-by: Andrew Lytvynov --- safeweb/http.go | 88 +++++++++++++++++++++++++++++++++++++------- safeweb/http_test.go | 28 +++++++++----- 2 files changed, 92 insertions(+), 24 deletions(-) diff --git a/safeweb/http.go b/safeweb/http.go index d8818d386..bd53eca5b 100644 --- a/safeweb/http.go +++ b/safeweb/http.go @@ -74,25 +74,74 @@ crand "crypto/rand" "fmt" "log" + "maps" "net" "net/http" "net/url" "path" + "slices" "strings" "github.com/gorilla/csrf" ) -// The default Content-Security-Policy header. -var defaultCSP = strings.Join([]string{ - `default-src 'self'`, // origin is the only valid source for all content types - `script-src 'self'`, // disallow inline javascript - `frame-ancestors 'none'`, // disallow framing of the page - `form-action 'self'`, // disallow form submissions to other origins - `base-uri 'self'`, // disallow base URIs from other origins - `block-all-mixed-content`, // disallow mixed content when serving over HTTPS - `object-src 'self'`, // disallow embedding of resources from other origins -}, "; ") +// CSP is the value of a Content-Security-Policy header. Keys are CSP +// directives (like "default-src") and values are source expressions (like +// "'self'" or "https://tailscale.com"). A nil slice value is allowed for some +// directives like "upgrade-insecure-requests" that don't expect a list of +// source definitions. +type CSP map[string][]string + +// DefaultCSP is the recommended CSP to use when not loading resources from +// other domains and not embedding the current website. If you need to tweak +// the CSP, it is recommended to extend DefaultCSP instead of writing your own +// from scratch. +func DefaultCSP() CSP { + return CSP{ + "default-src": {"self"}, // origin is the only valid source for all content types + "frame-ancestors": {"none"}, // disallow framing of the page + "form-action": {"self"}, // disallow form submissions to other origins + "base-uri": {"self"}, // disallow base URIs from other origins + // TODO(awly): consider upgrade-insecure-requests in SecureContext + // instead, as this is deprecated. + "block-all-mixed-content": nil, // disallow mixed content when serving over HTTPS + } +} + +// Set sets the values for a given directive. Empty values are allowed, if the +// directive doesn't expect any (like "upgrade-insecure-requests"). +func (csp CSP) Set(directive string, values ...string) { + csp[directive] = values +} + +// Add adds a source expression to an existing directive. +func (csp CSP) Add(directive, value string) { + csp[directive] = append(csp[directive], value) +} + +// Del deletes a directive and all its values. +func (csp CSP) Del(directive string) { + delete(csp, directive) +} + +func (csp CSP) String() string { + keys := slices.Collect(maps.Keys(csp)) + slices.Sort(keys) + var s strings.Builder + for _, k := range keys { + s.WriteString(k) + for _, v := range csp[k] { + // Special values like 'self', 'none', 'unsafe-inline', etc., must + // be quoted. Do it implicitly as a convenience here. + if !strings.Contains(v, ".") && len(v) > 1 && v[0] != '\'' && v[len(v)-1] != '\'' { + v = "'" + v + "'" + } + s.WriteString(" " + v) + } + s.WriteString("; ") + } + return strings.TrimSpace(s.String()) +} // The default Strict-Transport-Security header. This header tells the browser // to exclusively use HTTPS for all requests to the origin for the next year. @@ -130,6 +179,9 @@ type Config struct { // startup. CSRFSecret []byte + // CSP is the Content-Security-Policy header to return with BrowserMux + // responses. + CSP CSP // CSPAllowInlineStyles specifies whether to include `style-src: // unsafe-inline` in the Content-Security-Policy header to permit the use of // inline CSS. @@ -168,6 +220,10 @@ func (c *Config) setDefaults() error { } } + if c.CSP == nil { + c.CSP = DefaultCSP() + } + return nil } @@ -199,16 +255,20 @@ func NewServer(config Config) (*Server, error) { if config.CookiesSameSiteLax { sameSite = csrf.SameSiteLaxMode } + if config.CSPAllowInlineStyles { + if _, ok := config.CSP["style-src"]; ok { + config.CSP.Add("style-src", "unsafe-inline") + } else { + config.CSP.Set("style-src", "self", "unsafe-inline") + } + } s := &Server{ Config: config, - csp: defaultCSP, + csp: config.CSP.String(), // only set Secure flag on CSRF cookies if we are in a secure context // as otherwise the browser will reject the cookie csrfProtect: csrf.Protect(config.CSRFSecret, csrf.Secure(config.SecureContext), csrf.SameSite(sameSite)), } - if config.CSPAllowInlineStyles { - s.csp = defaultCSP + `; style-src 'self' 'unsafe-inline'` - } s.h = cmp.Or(config.HTTPServer, &http.Server{}) if s.h.Handler != nil { return nil, fmt.Errorf("use safeweb.Config.APIMux and safeweb.Config.BrowserMux instead of http.Server.Handler") diff --git a/safeweb/http_test.go b/safeweb/http_test.go index a2e2d7644..852ce326b 100644 --- a/safeweb/http_test.go +++ b/safeweb/http_test.go @@ -241,18 +241,26 @@ func TestCSRFProtection(t *testing.T) { func TestContentSecurityPolicyHeader(t *testing.T) { tests := []struct { name string + csp CSP apiRoute bool - wantCSP bool + wantCSP string }{ { - name: "default routes get CSP headers", - apiRoute: false, - wantCSP: true, + name: "default CSP", + wantCSP: `base-uri 'self'; block-all-mixed-content; default-src 'self'; form-action 'self'; frame-ancestors 'none';`, + }, + { + name: "custom CSP", + csp: CSP{ + "default-src": {"'self'", "https://tailscale.com"}, + "upgrade-insecure-requests": nil, + }, + wantCSP: `default-src 'self' https://tailscale.com; upgrade-insecure-requests;`, }, { name: "`/api/*` routes do not get CSP headers", apiRoute: true, - wantCSP: false, + wantCSP: "", }, } @@ -265,9 +273,9 @@ func TestContentSecurityPolicyHeader(t *testing.T) { var s *Server var err error if tt.apiRoute { - s, err = NewServer(Config{APIMux: h}) + s, err = NewServer(Config{APIMux: h, CSP: tt.csp}) } else { - s, err = NewServer(Config{BrowserMux: h}) + s, err = NewServer(Config{BrowserMux: h, CSP: tt.csp}) } if err != nil { t.Fatal(err) @@ -279,8 +287,8 @@ func TestContentSecurityPolicyHeader(t *testing.T) { s.h.Handler.ServeHTTP(w, req) resp := w.Result() - if (resp.Header.Get("Content-Security-Policy") == "") == tt.wantCSP { - t.Fatalf("content security policy want: %v; got: %v", tt.wantCSP, resp.Header.Get("Content-Security-Policy")) + if got := resp.Header.Get("Content-Security-Policy"); got != tt.wantCSP { + t.Fatalf("content security policy want: %q; got: %q", tt.wantCSP, got) } }) } @@ -397,7 +405,7 @@ func TestCSPAllowInlineStyles(t *testing.T) { csp := resp.Header.Get("Content-Security-Policy") allowsStyles := strings.Contains(csp, "style-src 'self' 'unsafe-inline'") if allowsStyles != allow { - t.Fatalf("CSP inline styles want: %v; got: %v", allow, allowsStyles) + t.Fatalf("CSP inline styles want: %v, got: %v in %q", allow, allowsStyles, csp) } }) }