chore: Dynamic CSP connect-src to support terminals connecting to workspace proxies (#7352)

* chore: Expose proxy hostnames to csp header

coderd/coderd.go

@@ -793,7 +793,16 @@ func New(options *Options) *API {
 		r.Get("/swagger/*", globalHTTPSwaggerHandler)
 	}
 
-	r.NotFound(compressHandler(http.HandlerFunc(api.siteHandler.ServeHTTP)).ServeHTTP)
+	// 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(func() []string {
+		if f := api.WorkspaceProxyHostsFn.Load(); f != nil {
+			return (*f)()
+		}
+		// By default we do not add extra websocket connections to the CSP
+		return []string{}
+	})
+	r.NotFound(cspMW(compressHandler(http.HandlerFunc(api.siteHandler.ServeHTTP))).ServeHTTP)
 	return api
 }
 
@@ -813,7 +822,12 @@ type API struct {
 	WorkspaceClientCoordinateOverride atomic.Pointer[func(rw http.ResponseWriter) bool]
 	TailnetCoordinator                atomic.Pointer[tailnet.Coordinator]
 	QuotaCommitter                    atomic.Pointer[proto.QuotaCommitter]
-	TemplateScheduleStore             *atomic.Pointer[schedule.TemplateScheduleStore]
+	// WorkspaceProxyHostsFn returns the hosts of healthy workspace proxies
+	// for header reasons.
+	WorkspaceProxyHostsFn atomic.Pointer[func() []string]
+	// TemplateScheduleStore is a pointer to an atomic pointer because this is
+	// passed to another struct, and we want them all to be the same reference.
+	TemplateScheduleStore *atomic.Pointer[schedule.TemplateScheduleStore]
 
 	HTTPAuth *HTTPAuthorizer
 

coderd/httpmw/csp.go

@@ -0,0 +1,119 @@
+package httpmw
+
+import (
+	"fmt"
+	"net/http"
+	"strings"
+)
+
+// cspDirectives is a map of all csp fetch directives to their values.
+// Each directive is a set of values that is joined by a space (' ').
+// All directives are semi-colon separated as a single string for the csp header.
+type cspDirectives map[CSPFetchDirective][]string
+
+func (s cspDirectives) Append(d CSPFetchDirective, values ...string) {
+	if _, ok := s[d]; !ok {
+		s[d] = make([]string, 0)
+	}
+	s[d] = append(s[d], values...)
+}
+
+// CSPFetchDirective is the list of all constant fetch directives that
+// can be used/appended to.
+type CSPFetchDirective string
+
+const (
+	cspDirectiveDefaultSrc  = "default-src"
+	cspDirectiveConnectSrc  = "connect-src"
+	cspDirectiveChildSrc    = "child-src"
+	cspDirectiveScriptSrc   = "script-src"
+	cspDirectiveFontSrc     = "font-src"
+	cspDirectiveStyleSrc    = "style-src"
+	cspDirectiveObjectSrc   = "object-src"
+	cspDirectiveManifestSrc = "manifest-src"
+	cspDirectiveFrameSrc    = "frame-src"
+	cspDirectiveImgSrc      = "img-src"
+	cspDirectiveReportURI   = "report-uri"
+	cspDirectiveFormAction  = "form-action"
+	cspDirectiveMediaSrc    = "media-src"
+	cspFrameAncestors       = "frame-ancestors"
+	cspDirectiveWorkerSrc   = "worker-src"
+)
+
+// CSPHeaders returns a middleware that sets the Content-Security-Policy header
+// for coderd. It takes a function that allows adding supported external websocket
+// hosts. This is primarily to support the terminal connecting to a workspace proxy.
+func CSPHeaders(websocketHosts func() []string) func(next http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			// Content-Security-Policy disables loading certain content types and can prevent XSS injections.
+			// This site helps eval your policy for syntax and other common issues: https://csp-evaluator.withgoogle.com/
+			// If we ever want to render something like a PDF, we need to adjust "object-src"
+			//
+			//	The list of CSP options: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/default-src
+			cspSrcs := cspDirectives{
+				// All omitted fetch csp srcs default to this.
+				cspDirectiveDefaultSrc: {"'self'"},
+				cspDirectiveConnectSrc: {"'self'"},
+				cspDirectiveChildSrc:   {"'self'"},
+				// https://github.com/suren-atoyan/monaco-react/issues/168
+				cspDirectiveScriptSrc: {"'self'"},
+				cspDirectiveStyleSrc:  {"'self' 'unsafe-inline'"},
+				// data: is used by monaco editor on FE for Syntax Highlight
+				cspDirectiveFontSrc:   {"'self' data:"},
+				cspDirectiveWorkerSrc: {"'self' blob:"},
+				// object-src is needed to support code-server
+				cspDirectiveObjectSrc: {"'self'"},
+				// blob: for loading the pwa manifest for code-server
+				cspDirectiveManifestSrc: {"'self' blob:"},
+				cspDirectiveFrameSrc:    {"'self'"},
+				// data: for loading base64 encoded icons for generic applications.
+				// https: allows loading images from external sources. This is not ideal
+				// 	but is required for the templates page that renders readmes.
+				//	We should find a better solution in the future.
+				cspDirectiveImgSrc:     {"'self' https: data:"},
+				cspDirectiveFormAction: {"'self'"},
+				cspDirectiveMediaSrc:   {"'self'"},
+				// Report all violations back to the server to log
+				cspDirectiveReportURI: {"/api/v2/csp/reports"},
+				cspFrameAncestors:     {"'none'"},
+
+				// Only scripts can manipulate the dom. This prevents someone from
+				// naming themselves something like '<svg onload="alert(/cross-site-scripting/)" />'.
+				// "require-trusted-types-for" : []string{"'script'"},
+			}
+
+			// This extra connect-src addition is required to support old webkit
+			// based browsers (Safari).
+			// See issue: https://github.com/w3c/webappsec-csp/issues/7
+			// Once webkit browsers support 'self' on connect-src, we can remove this.
+			// When we remove this, the csp header can be static, as opposed to being
+			// dynamically generated for each request.
+			host := r.Host
+			// It is important r.Host is not an empty string.
+			if host != "" {
+				// We can add both ws:// and wss:// as browsers do not let https
+				// pages to connect to non-tls websocket connections. So this
+				// supports both http & https webpages.
+				cspSrcs.Append(cspDirectiveConnectSrc, fmt.Sprintf("wss://%[1]s ws://%[1]s", host))
+			}
+
+			// The terminal requires a websocket connection to the workspace proxy.
+			// Make sure we allow this connection to healthy proxies.
+			extraConnect := websocketHosts()
+			if len(extraConnect) > 0 {
+				for _, extraHost := range extraConnect {
+					cspSrcs.Append(cspDirectiveConnectSrc, fmt.Sprintf("wss://%[1]s ws://%[1]s", extraHost))
+				}
+			}
+
+			var csp strings.Builder
+			for src, vals := range cspSrcs {
+				_, _ = fmt.Fprintf(&csp, "%s %s; ", src, strings.Join(vals, " "))
+			}
+
+			w.Header().Set("Content-Security-Policy", csp.String())
+			next.ServeHTTP(w, r)
+		})
+	}
+}

coderd/httpmw/csp_test.go

@@ -0,0 +1,33 @@
+package httpmw_test
+
+import (
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/coder/coder/coderd/httpmw"
+)
+
+func TestCSPConnect(t *testing.T) {
+	t.Parallel()
+
+	expected := []string{"example.com", "coder.com"}
+
+	r := httptest.NewRequest(http.MethodGet, "/", nil)
+	rw := httptest.NewRecorder()
+
+	httpmw.CSPHeaders(func() []string {
+		return expected
+	})(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+		rw.WriteHeader(http.StatusOK)
+	})).ServeHTTP(rw, r)
+
+	require.NotEmpty(t, rw.Header().Get("Content-Security-Policy"), "Content-Security-Policy header should not be empty")
+	for _, e := range expected {
+		require.Containsf(t, rw.Header().Get("Content-Security-Policy"), fmt.Sprintf("ws://%s", e), "Content-Security-Policy header should contain ws://%s", e)
+		require.Containsf(t, rw.Header().Get("Content-Security-Policy"), fmt.Sprintf("wss://%s", e), "Content-Security-Policy header should contain wss://%s", e)
+	}
+}

enterprise/coderd/coderd.go

@@ -250,6 +250,10 @@ func New(ctx context.Context, options *Options) (*API, error) {
 		// Force the initial loading of the cache. Do this in a go routine in case
 		// the calls to the workspace proxies hang and this takes some time.
 		go api.forceWorkspaceProxyHealthUpdate(ctx)
+
+		// Use proxy health to return the healthy workspace proxy hostnames.
+		f := api.ProxyHealth.ProxyHosts
+		api.AGPL.WorkspaceProxyHostsFn.Store(&f)
 	}
 
 	err = api.updateEntitlements(ctx)

enterprise/coderd/proxyhealth/proxyhealth.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"net/url"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -59,7 +60,9 @@ type ProxyHealth struct {
 	logger   slog.Logger
 	client   *http.Client
 
-	cache *atomic.Pointer[map[uuid.UUID]ProxyStatus]
+	// Cached values for quick access to the health of proxies.
+	cache      *atomic.Pointer[map[uuid.UUID]ProxyStatus]
+	proxyHosts *atomic.Pointer[[]string]
 
 	// PromMetrics
 	healthCheckDuration prometheus.Histogram
@@ -112,6 +115,7 @@ func New(opts *Options) (*ProxyHealth, error) {
 		logger:              opts.Logger,
 		client:              client,
 		cache:               &atomic.Pointer[map[uuid.UUID]ProxyStatus]{},
+		proxyHosts:          &atomic.Pointer[[]string]{},
 		healthCheckDuration: healthCheckDuration,
 		healthCheckResults:  healthCheckResults,
 	}, nil
@@ -133,12 +137,24 @@ func (p *ProxyHealth) Run(ctx context.Context) {
 				p.logger.Error(ctx, "proxy health check failed", slog.Error(err))
 				continue
 			}
-			// Store the statuses in the cache.
-			p.cache.Store(&statuses)
+			p.storeProxyHealth(statuses)
 		}
 	}
 }
 
+func (p *ProxyHealth) storeProxyHealth(statuses map[uuid.UUID]ProxyStatus) {
+	var proxyHosts []string
+	for _, s := range statuses {
+		if s.ProxyHost != "" {
+			proxyHosts = append(proxyHosts, s.ProxyHost)
+		}
+	}
+
+	// Store the statuses in the cache before any other quick values.
+	p.cache.Store(&statuses)
+	p.proxyHosts.Store(&proxyHosts)
+}
+
 // ForceUpdate runs a single health check and updates the cache. If the health
 // check fails, the cache is not updated and an error is returned. This is useful
 // to trigger an update when a proxy is created or deleted.
@@ -148,8 +164,7 @@ func (p *ProxyHealth) ForceUpdate(ctx context.Context) error {
 		return err
 	}
 
-	// Store the statuses in the cache.
-	p.cache.Store(&statuses)
+	p.storeProxyHealth(statuses)
 	return nil
 }
 
@@ -168,12 +183,28 @@ type ProxyStatus struct {
 	// useful to know as it helps determine if the proxy checked has different values
 	// then the proxy in hand. AKA if the proxy was updated, and the status was for
 	// an older proxy.
-	Proxy     database.WorkspaceProxy
+	Proxy database.WorkspaceProxy
+	// ProxyHost is the host:port of the proxy url. This is included in the status
+	// to make sure the proxy url is a valid URL. It also makes it easier to
+	// escalate errors if the url.Parse errors (should never happen).
+	ProxyHost string
 	Status    Status
 	Report    codersdk.ProxyHealthReport
 	CheckedAt time.Time
 }
 
+// ProxyHosts returns the host:port of all healthy proxies.
+// This can be computed from HealthStatus, but is cached to avoid the
+// caller needing to loop over all proxies to compute this on all
+// static web requests.
+func (p *ProxyHealth) ProxyHosts() []string {
+	ptr := p.proxyHosts.Load()
+	if ptr == nil {
+		return []string{}
+	}
+	return *ptr
+}
+
 // runOnce runs the health check for all workspace proxies. If there is an
 // unexpected error, an error is returned. Expected errors will mark a proxy as
 // unreachable.
@@ -248,6 +279,7 @@ func (p *ProxyHealth) runOnce(ctx context.Context, now time.Time) (map[uuid.UUID
 					status.Status = Unhealthy
 					break
 				}
+
 				status.Status = Healthy
 			case err == nil && resp.StatusCode != http.StatusOK:
 				// Unhealthy as we did reach the proxy but it got an unexpected response.
@@ -262,6 +294,15 @@ func (p *ProxyHealth) runOnce(ctx context.Context, now time.Time) (map[uuid.UUID
 				status.Status = Unknown
 			}
 
+			u, err := url.Parse(proxy.Url)
+			if err != nil {
+				// This should never happen. This would mean the proxy sent
+				// us an invalid url?
+				status.Report.Errors = append(status.Report.Errors, fmt.Sprintf("failed to parse proxy url: %s", err.Error()))
+				status.Status = Unhealthy
+			}
+			status.ProxyHost = u.Host
+
 			// Set the prometheus metric correctly.
 			switch status.Status {
 			case Healthy:

site/site.go

@@ -125,11 +125,6 @@ func Handler(siteFS fs.FS, binFS http.FileSystem, binHashes map[string]string) h
 type handler struct {
 	fs fs.FS
 	// htmlFiles is the text/template for all *.html files.
-	// This is needed to support Content Security Policy headers.
-	// Due to material UI, we are forced to use a nonce to allow inline
-	// scripts, and that nonce is passed through a template.
-	// We only do this for html files to reduce the amount of in memory caching
-	// of duplicate files as `fs`.
 	htmlFiles     *htmlTemplates
 	h             http.Handler
 	buildInfoJSON string
@@ -152,15 +147,10 @@ func (h *handler) exists(filePath string) bool {
 }
 
 type htmlState struct {
-	CSP       cspState
 	CSRF      csrfState
 	BuildInfo string
 }
 
-type cspState struct {
-	Nonce string
-}
-
 type csrfState struct {
 	Token string
 }
@@ -275,104 +265,6 @@ func (t *htmlTemplates) renderWithState(filePath string, state htmlState) ([]byt
 	return buf.Bytes(), nil
 }
 
-// CSPDirectives is a map of all csp fetch directives to their values.
-// Each directive is a set of values that is joined by a space (' ').
-// All directives are semi-colon separated as a single string for the csp header.
-type CSPDirectives map[CSPFetchDirective][]string
-
-func (s CSPDirectives) Append(d CSPFetchDirective, values ...string) {
-	if _, ok := s[d]; !ok {
-		s[d] = make([]string, 0)
-	}
-	s[d] = append(s[d], values...)
-}
-
-// CSPFetchDirective is the list of all constant fetch directives that
-// can be used/appended to.
-type CSPFetchDirective string
-
-const (
-	CSPDirectiveDefaultSrc  = "default-src"
-	CSPDirectiveConnectSrc  = "connect-src"
-	CSPDirectiveChildSrc    = "child-src"
-	CSPDirectiveScriptSrc   = "script-src"
-	CSPDirectiveFontSrc     = "font-src"
-	CSPDirectiveStyleSrc    = "style-src"
-	CSPDirectiveObjectSrc   = "object-src"
-	CSPDirectiveManifestSrc = "manifest-src"
-	CSPDirectiveFrameSrc    = "frame-src"
-	CSPDirectiveImgSrc      = "img-src"
-	CSPDirectiveReportURI   = "report-uri"
-	CSPDirectiveFormAction  = "form-action"
-	CSPDirectiveMediaSrc    = "media-src"
-	CSPFrameAncestors       = "frame-ancestors"
-	CSPDirectiveWorkerSrc   = "worker-src"
-)
-
-func cspHeaders(next http.Handler) http.Handler {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		// Content-Security-Policy disables loading certain content types and can prevent XSS injections.
-		// This site helps eval your policy for syntax and other common issues: https://csp-evaluator.withgoogle.com/
-		// If we ever want to render something like a PDF, we need to adjust "object-src"
-		//
-		//	The list of CSP options: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/default-src
-		cspSrcs := CSPDirectives{
-			// All omitted fetch csp srcs default to this.
-			CSPDirectiveDefaultSrc: {"'self'"},
-			CSPDirectiveConnectSrc: {"'self'"},
-			CSPDirectiveChildSrc:   {"'self'"},
-			// https://github.com/suren-atoyan/monaco-react/issues/168
-			CSPDirectiveScriptSrc: {"'self'"},
-			CSPDirectiveStyleSrc:  {"'self' 'unsafe-inline'"},
-			// data: is used by monaco editor on FE for Syntax Highlight
-			CSPDirectiveFontSrc:   {"'self' data:"},
-			CSPDirectiveWorkerSrc: {"'self' blob:"},
-			// object-src is needed to support code-server
-			CSPDirectiveObjectSrc: {"'self'"},
-			// blob: for loading the pwa manifest for code-server
-			CSPDirectiveManifestSrc: {"'self' blob:"},
-			CSPDirectiveFrameSrc:    {"'self'"},
-			// data: for loading base64 encoded icons for generic applications.
-			// https: allows loading images from external sources. This is not ideal
-			// 	but is required for the templates page that renders readmes.
-			//	We should find a better solution in the future.
-			CSPDirectiveImgSrc:     {"'self' https: data:"},
-			CSPDirectiveFormAction: {"'self'"},
-			CSPDirectiveMediaSrc:   {"'self'"},
-			// Report all violations back to the server to log
-			CSPDirectiveReportURI: {"/api/v2/csp/reports"},
-			CSPFrameAncestors:     {"'none'"},
-
-			// Only scripts can manipulate the dom. This prevents someone from
-			// naming themselves something like '<svg onload="alert(/cross-site-scripting/)" />'.
-			// "require-trusted-types-for" : []string{"'script'"},
-		}
-
-		// This extra connect-src addition is required to support old webkit
-		// based browsers (Safari).
-		// See issue: https://github.com/w3c/webappsec-csp/issues/7
-		// Once webkit browsers support 'self' on connect-src, we can remove this.
-		// When we remove this, the csp header can be static, as opposed to being
-		// dynamically generated for each request.
-		host := r.Host
-		// It is important r.Host is not an empty string.
-		if host != "" {
-			// We can add both ws:// and wss:// as browsers do not let https
-			// pages to connect to non-tls websocket connections. So this
-			// supports both http & https webpages.
-			cspSrcs.Append(CSPDirectiveConnectSrc, fmt.Sprintf("wss://%[1]s ws://%[1]s", host))
-		}
-
-		var csp strings.Builder
-		for src, vals := range cspSrcs {
-			_, _ = fmt.Fprintf(&csp, "%s %s; ", src, strings.Join(vals, " "))
-		}
-
-		w.Header().Set("Content-Security-Policy", csp.String())
-		next.ServeHTTP(w, r)
-	})
-}
-
 // secureHeaders is only needed for statically served files. We do not need this for api endpoints.
 // It adds various headers to enforce browser security features.
 func secureHeaders(next http.Handler) http.Handler {
@@ -404,7 +296,7 @@ func secureHeaders(next http.Handler) http.Handler {
 
 		// Prevent the browser from sending Referrer header with requests
 		ReferrerPolicy: "no-referrer",
-	}).Handler(cspHeaders(next))
+	}).Handler(next)
 }
 
 // htmlFiles recursively walks the file system passed finding all *.html files.