From ac581e9c4c95f36e7981ad84178f080a886ddbce Mon Sep 17 00:00:00 2001 From: Asher Date: Fri, 13 Jun 2025 16:43:57 -0800 Subject: [PATCH 1/2] feat: add csp headers for embedded apps --- coderd/coderd.go | 27 +++++++--- coderd/httpmw/csp.go | 41 +++++++------- coderd/httpmw/csp_test.go | 47 ++++++++++++---- coderd/proxyhealth/proxyhealth.go | 8 +++ coderd/workspaceapps/appurl/appurl.go | 20 +++++++ coderd/workspaceapps/appurl/appurl_test.go | 56 ++++++++++++++++++++ enterprise/coderd/proxyhealth/proxyhealth.go | 34 ++++++------ enterprise/coderd/workspaceproxy.go | 4 -- 8 files changed, 180 insertions(+), 57 deletions(-) create mode 100644 coderd/proxyhealth/proxyhealth.go diff --git a/coderd/coderd.go b/coderd/coderd.go index 8cc5435542189..a8f6fc427c822 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -76,6 +76,7 @@ import ( "github.com/coder/coder/v2/coderd/portsharing" "github.com/coder/coder/v2/coderd/prometheusmetrics" "github.com/coder/coder/v2/coderd/provisionerdserver" + "github.com/coder/coder/v2/coderd/proxyhealth" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/coderd/rbac/rolestore" @@ -85,6 +86,7 @@ import ( "github.com/coder/coder/v2/coderd/updatecheck" "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/coderd/workspaceapps" + "github.com/coder/coder/v2/coderd/workspaceapps/appurl" "github.com/coder/coder/v2/coderd/workspacestats" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/healthsdk" @@ -1534,16 +1536,27 @@ func New(options *Options) *API { // browsers, so these don't make sense on api routes. cspMW := httpmw.CSPHeaders( api.Experiments, - options.Telemetry.Enabled(), func() []string { + options.Telemetry.Enabled(), func() []*proxyhealth.ProxyHost { if api.DeploymentValues.Dangerous.AllowAllCors { - // In this mode, allow all external requests - return []string{"*"} + // 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{ + { + Host: api.AccessURL.Host, + AppHost: appurl.ConvertAppHostForCSP(api.AccessURL.String(), api.AppHostname), + }, } if f := api.WorkspaceProxyHostsFn.Load(); f != nil { - return (*f)() + proxies = append(proxies, (*f)()...) } - // By default we do not add extra websocket connections to the CSP - return []string{} + return proxies }, additionalCSPHeaders) // Static file handler must be wrapped with HSTS handler if the @@ -1582,7 +1595,7 @@ type API struct { AppearanceFetcher atomic.Pointer[appearance.Fetcher] // WorkspaceProxyHostsFn returns the hosts of healthy workspace proxies // for header reasons. - WorkspaceProxyHostsFn atomic.Pointer[func() []string] + WorkspaceProxyHostsFn atomic.Pointer[func() []*proxyhealth.ProxyHost] // 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] diff --git a/coderd/httpmw/csp.go b/coderd/httpmw/csp.go index afc19ddaf0c1f..06897a45afd01 100644 --- a/coderd/httpmw/csp.go +++ b/coderd/httpmw/csp.go @@ -5,6 +5,7 @@ import ( "net/http" "strings" + "github.com/coder/coder/v2/coderd/proxyhealth" "github.com/coder/coder/v2/codersdk" ) @@ -47,18 +48,18 @@ const ( // for coderd. // // Arguments: -// - websocketHosts: a function that returns a list of supported external websocket hosts. -// This is to support the terminal connecting to a workspace proxy. -// The origin of the terminal request does not match the url of the proxy, -// so the CSP list of allowed hosts must be dynamic and match the current -// available proxy urls. +// - proxyHosts: a function that returns a list of supported proxy hosts +// (including the primary). This is to support the terminal connecting to a +// workspace proxy and for embedding apps in an iframe. The origin of the +// requests do not match the url of the proxy, so the CSP list of allowed +// hosts must be dynamic and match the current available proxy urls. // - staticAdditions: a map of CSP directives to append to the default CSP headers. // Used to allow specific static additions to the CSP headers. Allows some niche // use cases, such as embedding Coder in an iframe. // Example: https://github.com/coder/coder/issues/15118 // //nolint:revive -func CSPHeaders(experiments codersdk.Experiments, telemetry bool, websocketHosts func() []string, staticAdditions map[CSPFetchDirective][]string) func(next http.Handler) http.Handler { +func CSPHeaders(experiments codersdk.Experiments, telemetry bool, proxyHosts func() []*proxyhealth.ProxyHost, staticAdditions map[CSPFetchDirective][]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. @@ -97,15 +98,6 @@ func CSPHeaders(experiments codersdk.Experiments, telemetry bool, websocketHosts // "require-trusted-types-for" : []string{"'script'"}, } - if experiments.Enabled(codersdk.ExperimentAITasks) { - // AI tasks use iframe embeds of local apps. - // TODO: Handle region domains too, not just path based apps - cspSrcs.Append(CSPFrameAncestors, `'self'`) - cspSrcs.Append(CSPFrameSource, `'self'`) - } else { - cspSrcs.Append(CSPFrameAncestors, `'none'`) - } - if telemetry { // If telemetry is enabled, we report to coder.com. cspSrcs.Append(CSPDirectiveConnectSrc, "https://coder.com") @@ -126,19 +118,26 @@ func CSPHeaders(experiments codersdk.Experiments, telemetry bool, websocketHosts 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() + // The terminal and iframed apps can use workspace proxies (which includes + // the primary). Make sure we allow connections to healthy proxies. + extraConnect := proxyHosts() if len(extraConnect) > 0 { for _, extraHost := range extraConnect { - if extraHost == "*" { + // Allow embedding the app host. + if experiments.Enabled(codersdk.ExperimentAITasks) { + cspSrcs.Append(CSPDirectiveFrameSrc, extraHost.AppHost) + } + if extraHost.Host == "*" { // '*' means all cspSrcs.Append(CSPDirectiveConnectSrc, "*") continue } - cspSrcs.Append(CSPDirectiveConnectSrc, fmt.Sprintf("wss://%[1]s ws://%[1]s", extraHost)) + // Avoid double-adding r.Host. + if extraHost.Host != r.Host { + cspSrcs.Append(CSPDirectiveConnectSrc, fmt.Sprintf("wss://%[1]s ws://%[1]s", extraHost.Host)) + } // We also require this to make http/https requests to the workspace proxy for latency checking. - cspSrcs.Append(CSPDirectiveConnectSrc, fmt.Sprintf("https://%[1]s http://%[1]s", extraHost)) + cspSrcs.Append(CSPDirectiveConnectSrc, fmt.Sprintf("https://%[1]s http://%[1]s", extraHost.Host)) } } diff --git a/coderd/httpmw/csp_test.go b/coderd/httpmw/csp_test.go index bef6ab196eb6e..5fd4b5bbd38aa 100644 --- a/coderd/httpmw/csp_test.go +++ b/coderd/httpmw/csp_test.go @@ -1,28 +1,59 @@ package httpmw_test import ( - "fmt" "net/http" "net/http/httptest" + "strings" "testing" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/proxyhealth" "github.com/coder/coder/v2/codersdk" ) -func TestCSPConnect(t *testing.T) { +func TestCSP(t *testing.T) { t.Parallel() - expected := []string{"example.com", "coder.com"} + proxyHosts := []*proxyhealth.ProxyHost{ + { + Host: "test.com", + AppHost: "*.test.com", + }, + { + Host: "coder.com", + AppHost: "*.coder.com", + }, + { + // Host is not added because it duplicates the host header. + Host: "example.com", + AppHost: "*.coder2.com", + }, + } expectedMedia := []string{"media.com", "media2.com"} + expected := []string{ + "frame-src 'self' *.test.com *.coder.com *.coder2.com", + "media-src 'self' media.com media2.com", + strings.Join([]string{ + "connect-src", "'self'", + // Added from host header. + "wss://example.com", "ws://example.com", + // Added via proxy hosts. + "wss://test.com", "ws://test.com", "https://test.com", "http://test.com", + "wss://coder.com", "ws://coder.com", "https://coder.com", "http://coder.com", + }, " "), + } + + // When the host is empty, it uses example.com. r := httptest.NewRequest(http.MethodGet, "/", nil) rw := httptest.NewRecorder() - httpmw.CSPHeaders(codersdk.Experiments{}, false, func() []string { - return expected + httpmw.CSPHeaders(codersdk.Experiments{ + codersdk.ExperimentAITasks, + }, false, func() []*proxyhealth.ProxyHost { + return proxyHosts }, map[httpmw.CSPFetchDirective][]string{ httpmw.CSPDirectiveMediaSrc: expectedMedia, })(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { @@ -31,10 +62,6 @@ func TestCSPConnect(t *testing.T) { 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) - } - for _, e := range expectedMedia { - require.Containsf(t, rw.Header().Get("Content-Security-Policy"), e, "Content-Security-Policy header should contain %s", e) + require.Contains(t, rw.Header().Get("Content-Security-Policy"), e) } } diff --git a/coderd/proxyhealth/proxyhealth.go b/coderd/proxyhealth/proxyhealth.go new file mode 100644 index 0000000000000..ac6dd5de59f9b --- /dev/null +++ b/coderd/proxyhealth/proxyhealth.go @@ -0,0 +1,8 @@ +package proxyhealth + +type ProxyHost struct { + // Host is the root host of the proxy. + Host string + // AppHost is the wildcard host where apps are hosted. + AppHost string +} diff --git a/coderd/workspaceapps/appurl/appurl.go b/coderd/workspaceapps/appurl/appurl.go index 1b1be9197b958..2676c07164a29 100644 --- a/coderd/workspaceapps/appurl/appurl.go +++ b/coderd/workspaceapps/appurl/appurl.go @@ -289,3 +289,23 @@ func ExecuteHostnamePattern(pattern *regexp.Regexp, hostname string) (string, bo return matches[1], true } + +// ConvertAppHostForCSP converts the wildcard host to a format accepted by CSP. +// For example *--apps.coder.com must become *.coder.com. If there is no +// wildcard host, or it cannot be converted, return the base host. +func ConvertAppHostForCSP(host, wildcard string) string { + if wildcard == "" { + return host + } + parts := strings.Split(wildcard, ".") + for i, part := range parts { + if strings.Contains(part, "*") { + // The wildcard can only be in the first section. + if i != 0 { + return host + } + parts[i] = "*" + } + } + return strings.Join(parts, ".") +} diff --git a/coderd/workspaceapps/appurl/appurl_test.go b/coderd/workspaceapps/appurl/appurl_test.go index 8353768de1d33..3924949cb30ad 100644 --- a/coderd/workspaceapps/appurl/appurl_test.go +++ b/coderd/workspaceapps/appurl/appurl_test.go @@ -410,3 +410,59 @@ func TestCompileHostnamePattern(t *testing.T) { }) } } + +func TestConvertAppURLForCSP(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + host string + wildcard string + expected string + }{ + { + name: "Empty", + host: "example.com", + wildcard: "", + expected: "example.com", + }, + { + name: "NoAsterisk", + host: "example.com", + wildcard: "coder.com", + expected: "coder.com", + }, + { + name: "Asterisk", + host: "example.com", + wildcard: "*.coder.com", + expected: "*.coder.com", + }, + { + name: "FirstPrefix", + host: "example.com", + wildcard: "*--apps.coder.com", + expected: "*.coder.com", + }, + { + name: "FirstSuffix", + host: "example.com", + wildcard: "apps--*.coder.com", + expected: "*.coder.com", + }, + { + name: "Middle", + host: "example.com", + wildcard: "apps.*.com", + expected: "example.com", + }, + } + + for _, c := range testCases { + c := c + t.Run(c.name, func(t *testing.T) { + t.Parallel() + require.Equal(t, c.expected, appurl.ConvertAppHostForCSP(c.host, c.wildcard)) + }) + } +} diff --git a/enterprise/coderd/proxyhealth/proxyhealth.go b/enterprise/coderd/proxyhealth/proxyhealth.go index 33a5da7d269a8..7faac6a9e8147 100644 --- a/enterprise/coderd/proxyhealth/proxyhealth.go +++ b/enterprise/coderd/proxyhealth/proxyhealth.go @@ -21,6 +21,8 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/prometheusmetrics" + agplproxyhealth "github.com/coder/coder/v2/coderd/proxyhealth" + "github.com/coder/coder/v2/coderd/workspaceapps/appurl" "github.com/coder/coder/v2/codersdk" ) @@ -63,7 +65,7 @@ type ProxyHealth struct { // Cached values for quick access to the health of proxies. cache *atomic.Pointer[map[uuid.UUID]ProxyStatus] - proxyHosts *atomic.Pointer[[]string] + proxyHosts *atomic.Pointer[[]*agplproxyhealth.ProxyHost] // PromMetrics healthCheckDuration prometheus.Histogram @@ -116,7 +118,7 @@ func New(opts *Options) (*ProxyHealth, error) { logger: opts.Logger, client: client, cache: &atomic.Pointer[map[uuid.UUID]ProxyStatus]{}, - proxyHosts: &atomic.Pointer[[]string]{}, + proxyHosts: &atomic.Pointer[[]*agplproxyhealth.ProxyHost]{}, healthCheckDuration: healthCheckDuration, healthCheckResults: healthCheckResults, }, nil @@ -144,9 +146,9 @@ func (p *ProxyHealth) Run(ctx context.Context) { } func (p *ProxyHealth) storeProxyHealth(statuses map[uuid.UUID]ProxyStatus) { - var proxyHosts []string + var proxyHosts []*agplproxyhealth.ProxyHost for _, s := range statuses { - if s.ProxyHost != "" { + if s.ProxyHost != nil { proxyHosts = append(proxyHosts, s.ProxyHost) } } @@ -190,23 +192,22 @@ type ProxyStatus struct { // then the proxy in hand. AKA if the proxy was updated, and the status was for // an older proxy. 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 + // ProxyHost is the base host:port and app host of the proxy. 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 *agplproxyhealth.ProxyHost 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 { +// ProxyHosts returns the host:port and wildcard host 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() []*agplproxyhealth.ProxyHost { ptr := p.proxyHosts.Load() if ptr == nil { - return []string{} + return []*agplproxyhealth.ProxyHost{} } return *ptr } @@ -350,7 +351,10 @@ func (p *ProxyHealth) runOnce(ctx context.Context, now time.Time) (map[uuid.UUID status.Report.Errors = append(status.Report.Errors, fmt.Sprintf("failed to parse proxy url: %s", err.Error())) status.Status = Unhealthy } - status.ProxyHost = u.Host + status.ProxyHost = &agplproxyhealth.ProxyHost{ + Host: u.Host, + AppHost: appurl.ConvertAppHostForCSP(u.Host, proxy.WildcardHostname), + } // Set the prometheus metric correctly. switch status.Status { diff --git a/enterprise/coderd/workspaceproxy.go b/enterprise/coderd/workspaceproxy.go index f495f1091a336..16fe079d20eb6 100644 --- a/enterprise/coderd/workspaceproxy.go +++ b/enterprise/coderd/workspaceproxy.go @@ -965,12 +965,8 @@ func convertRegion(proxy database.WorkspaceProxy, status proxyhealth.ProxyStatus func convertProxy(p database.WorkspaceProxy, status proxyhealth.ProxyStatus) codersdk.WorkspaceProxy { now := dbtime.Now() if p.IsPrimary() { - // Primary is always healthy since the primary serves the api that this - // is returned from. - u, _ := url.Parse(p.Url) status = proxyhealth.ProxyStatus{ Proxy: p, - ProxyHost: u.Host, Status: proxyhealth.Healthy, Report: codersdk.ProxyHealthReport{}, CheckedAt: now, From 4cd147cda5c04144c354ba9d4dca83f5381f4809 Mon Sep 17 00:00:00 2001 From: Asher Date: Mon, 16 Jun 2025 12:38:58 -0800 Subject: [PATCH 2/2] Use Host() not String() --- coderd/coderd.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index a8f6fc427c822..50565ed86d4ef 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1550,7 +1550,7 @@ func New(options *Options) *API { proxies := []*proxyhealth.ProxyHost{ { Host: api.AccessURL.Host, - AppHost: appurl.ConvertAppHostForCSP(api.AccessURL.String(), api.AppHostname), + AppHost: appurl.ConvertAppHostForCSP(api.AccessURL.Host, api.AppHostname), }, } if f := api.WorkspaceProxyHostsFn.Load(); f != nil {