feat: add csp headers for embedded apps #18374
Conversation
coderd/coderd.go
Outdated
| proxies := []*proxyhealth.ProxyHost{ | ||
| { | ||
| Host: api.AccessURL.Host, | ||
| AppHost: appurl.ConvertAppHostForCSP(api.AccessURL.String(), api.AppHostname), | ||
| }, | ||
| } |
There was a problem hiding this comment.
I tacked on the primary here, but we could instead add the primary as part of the WorkspaceProxyHostsFn function.
The current method here also includes the primary for AGPL, so if we do that we would also need to make some changes for the AGPL version (assuming wildcard apps and tasks even work with AGPL).
There was a problem hiding this comment.
api.AppHostname can be "". We should probably handle empty string, and not add it to the csp header.
There was a problem hiding this comment.
ConvertAppHostForCSP uses the first argument if the second is "", so we end up adding the base host instead (my thinking here was that means we are using path-based apps, so the root domain is the app host).
There was a problem hiding this comment.
Actually I think my reasoning is incorrect because you can use both wildcard and path-based at the same time, right? Hmm...that means we always want to add the access URL? I guess self would cover that one already, but can you use path-based for the regions? Maybe I need to add both there.
There was a problem hiding this comment.
Also I need to use .Host instead of .String() since I think CSP does not like the protocol being in there, could have sworn I fixed that already.
Edit: wait no what am I talking about, apparently my brain is fried today, seems to work either way.
There was a problem hiding this comment.
you can use both wildcard and path-based at the same time
You can, and self covers that. I actually do not know if we support path based on proxies 🤔
0cd14d3 to
35ddc13
Compare
35ddc13 to
ac581e9
Compare
I checked that the primary's headers get added to Definitely open to ideas on how I can more fully test. Maybe I need to make a proper deployment somewhere. |
|
I tested regions on dev and the frames are embedding perfectly 😎 |
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
noneor something.Closes coder/internal#684