feat: enable SvelteKit CSP policy with hash-based inline script/style protection#26604
feat: enable SvelteKit CSP policy with hash-based inline script/style protection#26604
Conversation
|
Preview environment has been removed. |
meesfrensel
left a comment
There was a problem hiding this comment.
Nice! I believe font-src and base-uri set to self are redundant since we already have default-src: self.
How do the directives flow from svelte config? Do they already automatically appear in the thing?
From a quick smoke test:
- Navigating to /map doesn't load the map tiles, and gives in this error in the console, but I don't see failed requests in the network tab:
Content-Security-Policy: The page’s settings blocked a worker script (worker-src) at blob:https://pr-26604.preview.internal.immich.build/ee2903a1-3a59-4637-9b29-6bd425e1289b from being executed because it violates the following directive: “worker-src 'self'” - Small map in detail panel of asset viewer: tiles don't load.
- Panorama viewer does not load. I think the image is loaded in a script and sent to the viewer library as a
blob:URL butblob:is missing from connect-src:ontent-Security-Policy: The page’s settings blocked the loading of a resource (connect-src) at blob:https://pr-26604.preview.internal.immich.build/9746f882-b47a-4ede-8b4a-849c27725037 because it violates the following directive: “connect-src 'self' https: wss: https://pay.futo.org/ https://buy.immich.app/”
| const { csp: baseCsp, html: indexWithoutCspMeta } = extractCsp(index); | ||
| index = indexWithoutCspMeta; | ||
| const csp = augmentCsp(baseCsp, { | ||
| 'connect-src': ['wss:', 'https://pay.futo.org', 'https://buy.immich.app'], |
There was a problem hiding this comment.
Why not just put these in the svelte config? It doesn't really hurt to have these in the maintenance mode pages.
There was a problem hiding this comment.
I agree. You'd be able to remove that entire csp utils file, too, I believe
There was a problem hiding this comment.
yes, we don't need to promote the defined CSP to a header either, since we're not using header-only CSP directives, like report-to or sandbox. I'll revert the entire CSP. I'll leave the scrip that generates the hashes for inline scripts in app.html, since that is still useful to have.
There was a problem hiding this comment.
Note - the tile URLs are now also hardcoded, instead of trying to parse out the domains from the dark/light tile .json configuration. So if the tile servers changes, the CSP will need to be manually updated.
There was a problem hiding this comment.
I'll leave the scrip that generates the hashes for inline scripts in app.html, since that is still useful to have.
I would prefer not to. I don't see any reason why we should leave unused code in there. We can almost grab it from this PR again if we do need it
There was a problem hiding this comment.
Note - the tile URLs are now also hardcoded, instead of trying to parse out the domains from the dark/light tile .json configuration. So if the tile servers changes, the CSP will need to be manually updated.
You're aware that we let users customize their tile servers? (https://my.immich.app/admin/system-settings?isOpen=location+map)
There was a problem hiding this comment.
Oh - missed that.
Its slightly problematic to parse out all the domains from a mapbox json. It can contain 'sources' that have 'urls' that fetch other domains. i.e. for immich, this is where 'static.immich.cloud' comes from - in addition to the tiles.immich.cloud for the main .json file.
An easy and reliable way to solve this problem is to not have the browser directly contact the map server. Instead, create a rest endpoint on the api server itself to proxy the tile requests. This way, the browser CSP does not need to carve out an exception - as all requests will go back to the same server (self:).
Downsides:
- May add some latency - uses your servers upstream bandwidth.
Upsides:
- potentially have the server do some caching
- you increase some privacy - the map server only sees the ip address of the server, not every browser/mobile device your using.
In general, having every 3rd party request go through the api server would be my preferred solution - mostly for privacy - and for observability - knowing that I (as an admin) am in control all all URLs the browser touches.
Not sure if its possible for gcast/streaming - but it would be awesome if that could be done there too - so google won't see my client IP.
There was a problem hiding this comment.
In general, having every 3rd party request go through the api server would be my preferred solution
I think this is a bit of a double edged sword. On the one hand I agree, yes, but then again some people like to air-gap their instances. Right now, Immich server does not need internet access (the only traffic there is ML downloading the models, which you can also download yourself and put in the directories). I think the point is basically that a web browser already has internet, whereas the server doesn't necessarily has to.
There was a problem hiding this comment.
I am one of those "some people", I try to deny outgoing traffic from as many containers as possible.
having a CSP header would be nice :)
Couldn't it be as open as it needs for now and be adjusted in future versions?
c608ba1 to
2fc2ac5
Compare
5704ccb to
2de9f20
Compare
jrasm91
left a comment
There was a problem hiding this comment.
What impact does adding csp to the svelte config have on the final production build?
When csp: { mode: 'hash', directives: {...} } is added to the SvelteKit config with adapter-static, the build output is altered like so:
In our case, since we're a SPA, and using the static adapter, with no SSR - this is effectively only a single 'fallback' page, which is used for every route.
The only remaining gap was the theme/FOUC script in the fallback/index template - svelte only hashes its own script blocks. Since this is one Immich added, it needs to be hashed manually - which is what that script in this PR does. We can remove that script though, per Daniel's suggestion. In short: the only build artifact change is the addition of a CSP in general protect against XSS, clickjacking, and other security things. |
|
Right. Since there are still so many things that aren't covered with this approach I'm failing to see what value it actually brings. What use case does accepting this PR in it's current state address? |
|
The main thing this gets us is protection against the most common and dangerous web attack — XSS via injected scripts. Without any CSP, if someone finds a way to inject HTML into the page (say through album names, descriptions, or any user-provided content), the browser will happily run whatever Specifically it:
You're right that some areas are still permissive — The remaining gaps (especially tiles) would be great follow-ups, but I don't think they should block shipping the core protection. Happy to discuss though! |
|
The XSS risk is fairly low in modern web frameworks like Svelte. The main concerns are user input into href/src attributes or values being rendered through There are some issues:
|
|
According to multiple CSP evaluators, this current CSP can be bypassed because It can be mitigated by making the source more specific ( It could also be mitigated with a hash-based approach. and also adding As a safeguard for shipping changes that break CSP, I set up CSP reporting endpoint in my dev environments. It logs detected CSP violations as JSON, saving me from shipping CSP violations that might be hard to see at first glance. I do agree that it's still a good idea to ship an imperfect CSP initially, and improve it with time. See: |
|
I'm not against adding CSP, but as I understand it, the current state of the PR doesn't actually add it. It just configures the static build to include metadata related to CSP in the build output. How are users supposed to use that? |
|
In this case, CSP isn't sent as a header, but is injected as an HTML meta tag ( |
|
The current policy would likely break the existing, custom, functionality that we have in the application. Specifically today we support custom css, custom tiling maps, and allow Immich to be embedded in iframes for dashboards, portals, etc. I think the goal of a CSP related change should be to enable better security by default while still giving the user full control over the policy. Probably the way to go here is to add CSP headers on the server and have it use some sensible defaults, while providing an option to overwrite it completely. |
See #25389
This adds a CSP - using HASH based script protection (vs the nonce based on in referenced PR) - and also serving the CSP via express (so it works in prod mode)
Not using nonces because the content never changes (since using adapter-static). Svelte-kit also creates the bootstrap script as part of app.html - and since this is created statically, the nonce will always be the same for each request - which defeats the purpose of a nonce.
So, this uses hashes instead. Sveltekit actually creates hashes for all the inline scripts it generates (including the bootstrap script) and adds it as a header.
At runtime, ApiService.ssr() and MaintanceWorkerService.ssr() will extract/remove the CSP from the file - and augment it with additional policies (websockets, payment endpoints) and sets it as a CSP header which is more secure.
specifics of this CSP:
script-src -
www.gstatic.comis included for google cast support (didn't test this)style-src has 'unsafe-inline' - svelte components that use style directives use inline styles - there is no other option here.
connect-src -
https: this is super permissive right now - because of admin-configurable tile servers - personally, I think we should proxy tiles via the backend-api.img-src -
data:for JXL detection,blob:for canvas, andhttps:for map tilesworker-src
self- local onlyfont-src -
self- local onlyframe-src -
none- no framesobject-src -
none- no pluginsbase-uri -
self- best practiceAlso included is a script to generate hash for inline scripts (the theme parsing/setting inline script)
As a followup - the tiles stuff should be locked down a little more