Skip to content

Commit 82c14e0

Browse files
code-ashercode-asher
authored
feat: add csp headers for embedded apps (#18374)
I modified the proxy host cache we already had and were using for websocket csp headers to also include the wildcard app host, then used those for frame-src policies. I did not add frame-ancestors, since if I understand correctly, those would go on the app, and this middleware does not come into play there. Maybe we will want to add it on workspace apps like we do with cors, if we find apps are setting it to `none` or something. Closes coder/internal#684
1 parent aee96c9 commit 82c14e0

File tree

8 files changed

+180
-57
lines changed

8 files changed

+180
-57
lines changed

‎coderd/coderd.go

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ import (
7676
"github.com/coder/coder/v2/coderd/portsharing"
7777
"github.com/coder/coder/v2/coderd/prometheusmetrics"
7878
"github.com/coder/coder/v2/coderd/provisionerdserver"
79+
"github.com/coder/coder/v2/coderd/proxyhealth"
7980
"github.com/coder/coder/v2/coderd/rbac"
8081
"github.com/coder/coder/v2/coderd/rbac/policy"
8182
"github.com/coder/coder/v2/coderd/rbac/rolestore"
@@ -85,6 +86,7 @@ import (
8586
"github.com/coder/coder/v2/coderd/updatecheck"
8687
"github.com/coder/coder/v2/coderd/util/slice"
8788
"github.com/coder/coder/v2/coderd/workspaceapps"
89+
"github.com/coder/coder/v2/coderd/workspaceapps/appurl"
8890
"github.com/coder/coder/v2/coderd/workspacestats"
8991
"github.com/coder/coder/v2/codersdk"
9092
"github.com/coder/coder/v2/codersdk/healthsdk"
@@ -1534,16 +1536,27 @@ func New(options *Options) *API {
15341536
// browsers, so these don't make sense on api routes.
15351537
cspMW := httpmw.CSPHeaders(
15361538
api.Experiments,
1537-
options.Telemetry.Enabled(), func() []string {
1539+
options.Telemetry.Enabled(), func() []*proxyhealth.ProxyHost {
15381540
if api.DeploymentValues.Dangerous.AllowAllCors {
1539-
// In this mode, allow all external requests
1540-
return []string{"*"}
1541+
// In this mode, allow all external requests.
1542+
return []*proxyhealth.ProxyHost{
1543+
{
1544+
Host: "*",
1545+
AppHost: "*",
1546+
},
1547+
}
1548+
}
1549+
// Always add the primary, since the app host may be on a sub-domain.
1550+
proxies := []*proxyhealth.ProxyHost{
1551+
{
1552+
Host: api.AccessURL.Host,
1553+
AppHost: appurl.ConvertAppHostForCSP(api.AccessURL.Host, api.AppHostname),
1554+
},
15411555
}
15421556
if f := api.WorkspaceProxyHostsFn.Load(); f != nil {
1543-
return (*f)()
1557+
proxies = append(proxies, (*f)()...)
15441558
}
1545-
// By default we do not add extra websocket connections to the CSP
1546-
return []string{}
1559+
return proxies
15471560
}, additionalCSPHeaders)
15481561

15491562
// Static file handler must be wrapped with HSTS handler if the
@@ -1582,7 +1595,7 @@ type API struct {
15821595
AppearanceFetcher atomic.Pointer[appearance.Fetcher]
15831596
// WorkspaceProxyHostsFn returns the hosts of healthy workspace proxies
15841597
// for header reasons.
1585-
WorkspaceProxyHostsFn atomic.Pointer[func() []string]
1598+
WorkspaceProxyHostsFn atomic.Pointer[func() []*proxyhealth.ProxyHost]
15861599
// TemplateScheduleStore is a pointer to an atomic pointer because this is
15871600
// passed to another struct, and we want them all to be the same reference.
15881601
TemplateScheduleStore *atomic.Pointer[schedule.TemplateScheduleStore]

‎coderd/httpmw/csp.go

Lines changed: 20 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"net/http"
66
"strings"
77

8+
"github.com/coder/coder/v2/coderd/proxyhealth"
89
"github.com/coder/coder/v2/codersdk"
910
)
1011

@@ -47,18 +48,18 @@ const (
4748
// for coderd.
4849
//
4950
// Arguments:
50-
// - websocketHosts: a function that returns a list of supported external websocket hosts.
51-
// This is to support the terminal connecting to a workspace proxy.
52-
// The origin of the terminal request does not match the url of the proxy,
53-
// so the CSP list of allowed hosts must be dynamic and match the current
54-
// available proxy urls.
51+
// - proxyHosts: a function that returns a list of supported proxy hosts
52+
// (including the primary). This is to support the terminal connecting to a
53+
// workspace proxy and for embedding apps in an iframe. The origin of the
54+
// requests do not match the url of the proxy, so the CSP list of allowed
55+
// hosts must be dynamic and match the current available proxy urls.
5556
// - staticAdditions: a map of CSP directives to append to the default CSP headers.
5657
// Used to allow specific static additions to the CSP headers. Allows some niche
5758
// use cases, such as embedding Coder in an iframe.
5859
// Example: https://github.com/coder/coder/issues/15118
5960
//
6061
//nolint:revive
61-
func CSPHeaders(experiments codersdk.Experiments, telemetry bool, websocketHosts func() []string, staticAdditions map[CSPFetchDirective][]string) func(next http.Handler) http.Handler {
62+
func CSPHeaders(experiments codersdk.Experiments, telemetry bool, proxyHosts func() []*proxyhealth.ProxyHost, staticAdditions map[CSPFetchDirective][]string) func(next http.Handler) http.Handler {
6263
return func(next http.Handler) http.Handler {
6364
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
6465
// 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
9798
// "require-trusted-types-for" : []string{"'script'"},
9899
}
99100

100-
if experiments.Enabled(codersdk.ExperimentAITasks) {
101-
// AI tasks use iframe embeds of local apps.
102-
// TODO: Handle region domains too, not just path based apps
103-
cspSrcs.Append(CSPFrameAncestors, `'self'`)
104-
cspSrcs.Append(CSPFrameSource, `'self'`)
105-
} else {
106-
cspSrcs.Append(CSPFrameAncestors, `'none'`)
107-
}
108-
109101
if telemetry {
110102
// If telemetry is enabled, we report to coder.com.
111103
cspSrcs.Append(CSPDirectiveConnectSrc, "https://coder.com")
@@ -126,19 +118,26 @@ func CSPHeaders(experiments codersdk.Experiments, telemetry bool, websocketHosts
126118
cspSrcs.Append(CSPDirectiveConnectSrc, fmt.Sprintf("wss://%[1]s ws://%[1]s", host))
127119
}
128120

129-
// The terminal requires a websocket connection to the workspace proxy.
130-
// Make sure we allow this connection to healthy proxies.
131-
extraConnect := websocketHosts()
121+
// The terminal and iframed apps can use workspace proxies (which includes
122+
// the primary). Make sure we allow connections to healthy proxies.
123+
extraConnect := proxyHosts()
132124
if len(extraConnect) > 0 {
133125
for _, extraHost := range extraConnect {
134-
if extraHost == "*" {
126+
// Allow embedding the app host.
127+
if experiments.Enabled(codersdk.ExperimentAITasks) {
128+
cspSrcs.Append(CSPDirectiveFrameSrc, extraHost.AppHost)
129+
}
130+
if extraHost.Host == "*" {
135131
// '*' means all
136132
cspSrcs.Append(CSPDirectiveConnectSrc, "*")
137133
continue
138134
}
139-
cspSrcs.Append(CSPDirectiveConnectSrc, fmt.Sprintf("wss://%[1]s ws://%[1]s", extraHost))
135+
// Avoid double-adding r.Host.
136+
if extraHost.Host != r.Host {
137+
cspSrcs.Append(CSPDirectiveConnectSrc, fmt.Sprintf("wss://%[1]s ws://%[1]s", extraHost.Host))
138+
}
140139
// We also require this to make http/https requests to the workspace proxy for latency checking.
141-
cspSrcs.Append(CSPDirectiveConnectSrc, fmt.Sprintf("https://%[1]s http://%[1]s", extraHost))
140+
cspSrcs.Append(CSPDirectiveConnectSrc, fmt.Sprintf("https://%[1]s http://%[1]s", extraHost.Host))
142141
}
143142
}
144143

‎coderd/httpmw/csp_test.go

Lines changed: 37 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,59 @@
11
package httpmw_test
22

33
import (
4-
"fmt"
54
"net/http"
65
"net/http/httptest"
6+
"strings"
77
"testing"
88

99
"github.com/stretchr/testify/require"
1010

1111
"github.com/coder/coder/v2/coderd/httpmw"
12+
"github.com/coder/coder/v2/coderd/proxyhealth"
1213
"github.com/coder/coder/v2/codersdk"
1314
)
1415

15-
func TestCSPConnect(t *testing.T) {
16+
func TestCSP(t *testing.T) {
1617
t.Parallel()
1718

18-
expected := []string{"example.com", "coder.com"}
19+
proxyHosts := []*proxyhealth.ProxyHost{
20+
{
21+
Host: "test.com",
22+
AppHost: "*.test.com",
23+
},
24+
{
25+
Host: "coder.com",
26+
AppHost: "*.coder.com",
27+
},
28+
{
29+
// Host is not added because it duplicates the host header.
30+
Host: "example.com",
31+
AppHost: "*.coder2.com",
32+
},
33+
}
1934
expectedMedia := []string{"media.com", "media2.com"}
2035

36+
expected := []string{
37+
"frame-src 'self' *.test.com *.coder.com *.coder2.com",
38+
"media-src 'self' media.com media2.com",
39+
strings.Join([]string{
40+
"connect-src", "'self'",
41+
// Added from host header.
42+
"wss://example.com", "ws://example.com",
43+
// Added via proxy hosts.
44+
"wss://test.com", "ws://test.com", "https://test.com", "http://test.com",
45+
"wss://coder.com", "ws://coder.com", "https://coder.com", "http://coder.com",
46+
}, " "),
47+
}
48+
49+
// When the host is empty, it uses example.com.
2150
r := httptest.NewRequest(http.MethodGet, "/", nil)
2251
rw := httptest.NewRecorder()
2352

24-
httpmw.CSPHeaders(codersdk.Experiments{}, false, func() []string {
25-
return expected
53+
httpmw.CSPHeaders(codersdk.Experiments{
54+
codersdk.ExperimentAITasks,
55+
}, false, func() []*proxyhealth.ProxyHost {
56+
return proxyHosts
2657
}, map[httpmw.CSPFetchDirective][]string{
2758
httpmw.CSPDirectiveMediaSrc: expectedMedia,
2859
})(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
@@ -31,10 +62,6 @@ func TestCSPConnect(t *testing.T) {
3162

3263
require.NotEmpty(t, rw.Header().Get("Content-Security-Policy"), "Content-Security-Policy header should not be empty")
3364
for _, e := range expected {
34-
require.Containsf(t, rw.Header().Get("Content-Security-Policy"), fmt.Sprintf("ws://%s", e), "Content-Security-Policy header should contain ws://%s", e)
35-
require.Containsf(t, rw.Header().Get("Content-Security-Policy"), fmt.Sprintf("wss://%s", e), "Content-Security-Policy header should contain wss://%s", e)
36-
}
37-
for _, e := range expectedMedia {
38-
require.Containsf(t, rw.Header().Get("Content-Security-Policy"), e, "Content-Security-Policy header should contain %s", e)
65+
require.Contains(t, rw.Header().Get("Content-Security-Policy"), e)
3966
}
4067
}

‎coderd/proxyhealth/proxyhealth.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
package proxyhealth
2+
3+
type ProxyHost struct {
4+
// Host is the root host of the proxy.
5+
Host string
6+
// AppHost is the wildcard host where apps are hosted.
7+
AppHost string
8+
}

‎coderd/workspaceapps/appurl/appurl.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,3 +289,23 @@ func ExecuteHostnamePattern(pattern *regexp.Regexp, hostname string) (string, bo
289289

290290
return matches[1], true
291291
}
292+
293+
// ConvertAppHostForCSP converts the wildcard host to a format accepted by CSP.
294+
// For example *--apps.coder.com must become *.coder.com. If there is no
295+
// wildcard host, or it cannot be converted, return the base host.
296+
func ConvertAppHostForCSP(host, wildcard string) string {
297+
if wildcard == "" {
298+
return host
299+
}
300+
parts := strings.Split(wildcard, ".")
301+
for i, part := range parts {
302+
if strings.Contains(part, "*") {
303+
// The wildcard can only be in the first section.
304+
if i != 0 {
305+
return host
306+
}
307+
parts[i] = "*"
308+
}
309+
}
310+
return strings.Join(parts, ".")
311+
}

‎coderd/workspaceapps/appurl/appurl_test.go

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,3 +410,59 @@ func TestCompileHostnamePattern(t *testing.T) {
410410
})
411411
}
412412
}
413+
414+
func TestConvertAppURLForCSP(t *testing.T) {
415+
t.Parallel()
416+
417+
testCases := []struct {
418+
name string
419+
host string
420+
wildcard string
421+
expected string
422+
}{
423+
{
424+
name: "Empty",
425+
host: "example.com",
426+
wildcard: "",
427+
expected: "example.com",
428+
},
429+
{
430+
name: "NoAsterisk",
431+
host: "example.com",
432+
wildcard: "coder.com",
433+
expected: "coder.com",
434+
},
435+
{
436+
name: "Asterisk",
437+
host: "example.com",
438+
wildcard: "*.coder.com",
439+
expected: "*.coder.com",
440+
},
441+
{
442+
name: "FirstPrefix",
443+
host: "example.com",
444+
wildcard: "*--apps.coder.com",
445+
expected: "*.coder.com",
446+
},
447+
{
448+
name: "FirstSuffix",
449+
host: "example.com",
450+
wildcard: "apps--*.coder.com",
451+
expected: "*.coder.com",
452+
},
453+
{
454+
name: "Middle",
455+
host: "example.com",
456+
wildcard: "apps.*.com",
457+
expected: "example.com",
458+
},
459+
}
460+
461+
for _, c := range testCases {
462+
c := c
463+
t.Run(c.name, func(t *testing.T) {
464+
t.Parallel()
465+
require.Equal(t, c.expected, appurl.ConvertAppHostForCSP(c.host, c.wildcard))
466+
})
467+
}
468+
}

‎enterprise/coderd/proxyhealth/proxyhealth.go

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ import (
2121
"github.com/coder/coder/v2/coderd/database"
2222
"github.com/coder/coder/v2/coderd/database/dbauthz"
2323
"github.com/coder/coder/v2/coderd/prometheusmetrics"
24+
agplproxyhealth "github.com/coder/coder/v2/coderd/proxyhealth"
25+
"github.com/coder/coder/v2/coderd/workspaceapps/appurl"
2426
"github.com/coder/coder/v2/codersdk"
2527
)
2628

@@ -63,7 +65,7 @@ type ProxyHealth struct {
6365

6466
// Cached values for quick access to the health of proxies.
6567
cache *atomic.Pointer[map[uuid.UUID]ProxyStatus]
66-
proxyHosts *atomic.Pointer[[]string]
68+
proxyHosts *atomic.Pointer[[]*agplproxyhealth.ProxyHost]
6769

6870
// PromMetrics
6971
healthCheckDuration prometheus.Histogram
@@ -116,7 +118,7 @@ func New(opts *Options) (*ProxyHealth, error) {
116118
logger: opts.Logger,
117119
client: client,
118120
cache: &atomic.Pointer[map[uuid.UUID]ProxyStatus]{},
119-
proxyHosts: &atomic.Pointer[[]string]{},
121+
proxyHosts: &atomic.Pointer[[]*agplproxyhealth.ProxyHost]{},
120122
healthCheckDuration: healthCheckDuration,
121123
healthCheckResults: healthCheckResults,
122124
}, nil
@@ -144,9 +146,9 @@ func (p *ProxyHealth) Run(ctx context.Context) {
144146
}
145147

146148
func (p *ProxyHealth) storeProxyHealth(statuses map[uuid.UUID]ProxyStatus) {
147-
var proxyHosts []string
149+
var proxyHosts []*agplproxyhealth.ProxyHost
148150
for _, s := range statuses {
149-
if s.ProxyHost != "" {
151+
if s.ProxyHost != nil {
150152
proxyHosts = append(proxyHosts, s.ProxyHost)
151153
}
152154
}
@@ -190,23 +192,22 @@ type ProxyStatus struct {
190192
// then the proxy in hand. AKA if the proxy was updated, and the status was for
191193
// an older proxy.
192194
Proxy database.WorkspaceProxy
193-
// ProxyHost is the host:port of the proxy url. This is included in the status
194-
// to make sure the proxy url is a valid URL. It also makes it easier to
195-
// escalate errors if the url.Parse errors (should never happen).
196-
ProxyHost string
195+
// ProxyHost is the base host:port and app host of the proxy. This is included
196+
// in the status to make sure the proxy url is a valid URL. It also makes it
197+
// easier to escalate errors if the url.Parse errors (should never happen).
198+
ProxyHost *agplproxyhealth.ProxyHost
197199
Status Status
198200
Report codersdk.ProxyHealthReport
199201
CheckedAt time.Time
200202
}
201203

202-
// ProxyHosts returns the host:port of all healthy proxies.
203-
// This can be computed from HealthStatus, but is cached to avoid the
204-
// caller needing to loop over all proxies to compute this on all
205-
// static web requests.
206-
func (p *ProxyHealth) ProxyHosts() []string {
204+
// ProxyHosts returns the host:port and wildcard host of all healthy proxies.
205+
// This can be computed from HealthStatus, but is cached to avoid the caller
206+
// needing to loop over all proxies to compute this on all static web requests.
207+
func (p *ProxyHealth) ProxyHosts() []*agplproxyhealth.ProxyHost {
207208
ptr := p.proxyHosts.Load()
208209
if ptr == nil {
209-
return []string{}
210+
return []*agplproxyhealth.ProxyHost{}
210211
}
211212
return *ptr
212213
}
@@ -350,7 +351,10 @@ func (p *ProxyHealth) runOnce(ctx context.Context, now time.Time) (map[uuid.UUID
350351
status.Report.Errors = append(status.Report.Errors, fmt.Sprintf("failed to parse proxy url: %s", err.Error()))
351352
status.Status = Unhealthy
352353
}
353-
status.ProxyHost = u.Host
354+
status.ProxyHost = &agplproxyhealth.ProxyHost{
355+
Host: u.Host,
356+
AppHost: appurl.ConvertAppHostForCSP(u.Host, proxy.WildcardHostname),
357+
}
354358

355359
// Set the prometheus metric correctly.
356360
switch status.Status {

0 commit comments

Comments
 (0)