Content Security Policy support

benmccann commented 4 years ago

I don't see anything in the code related to CSP support, so I'm guessing that doesn't exist yet unless I missed it.

Sapper supports it via %sapper.cspnonce%: https://sapper.svelte.dev/docs#Content_Security_Policy_CSP

dominikg commented 4 years ago

I stumbled over nonce-based csp support in sapper before: https://github.com/sveltejs/sapper/issues/1175

May need a different approach in kit depending on the adapter being used (capability to generate nonce for every request?) Imho nonce should only be used where a) it has to be an inline item and b) the data contained does not only change on build but at runtime (dynamic ssr). Whereever possible inline items should either be properly hashed or extracted.

That being said, what level of csp support are we looking at? No unsafe-* required?

acoyfellow commented 3 years ago

Hi - it looks like this (mostly) can be solved with hooks now, in the handle function.

no nonce's yet but i'm working on it, maybe someone can see that solution

https://gist.github.com/acoyfellow/d8e86979c66ebea25e1643594e38be73

furudean commented 3 years ago

~~Technically a duplicate of #72, but this issue has better info so lets go with this one?~~

Oops, CSP is not the same as CSRF!

seanlail commented 3 years ago

@acoyfellow Great work so far!

I just want to highlight that it's really important that eval is not used in SvelteKit. Can we ensure that there is no unsafe use, as @dominikg has suggested.

I've just been looking at an issue in Sapper here where the issue is that we have to allow unsafe-eval, which defeats the purpose of using CSP.

cc/ @Conduitry (as we were talking about this in the other ticket).

Rich-Harris commented 3 years ago

Noting this, from Twitter:

is there any way to move the inline script for SvelteKit to an external one? I’d like to enforce a stricter CSP, but currently need unsafe-inline to accommodate. Thanks!

For server-rendered pages we could just use a nonce, I think? For pre-rendered pages that's no good, we would need to generate a hashed file (say /boot/xyz123.js) for each unique script.

dominikg commented 3 years ago

could still hash inline content: https://content-security-policy.com/hash/

and pass the hash in a policy via meta in head https://content-security-policy.com/examples/meta/

I'm not sure if meta policies are additive though so may not be ideal. external scripts would work best as those are covered by self, no hash required

Karlinator commented 3 years ago

As far as I can see it's only the init script in the head, right? Nothing else is inline? Or does this depend on build configurations/adapters?

In any case meta policies are allowed alongside HTTP policies. However, any source must pass all specified policies, meaning if you wanted to load scripts from a CDN it must be specified both places and the HTTP header must still send unsafe-inline. Basically, generating CSP in more than one place is kind of a mess.

See: https://www.w3.org/TR/CSP3/#multiple-policies

Putting the init script in a separate file is a much better option imo.

pzuraq commented 3 years ago

@Rich-Harris

For pre-rendered pages that's no good, we would need to generate a hashed file (say /boot/xyz123.js) for each unique script.

I think one of the advantages of nonces is that with them and strict-dynamic you can load arbitrary scripts (say, Google Analytics, or a some other source) and since they're trusted, they can load anything they need, without needing to allowlist them. This is helpful for scripts distributed and loaded via CDN where the hashes might change frequently, and reduces maintenance cost.

I'm not an expert here, but I think if the page is guaranteed to be static, then a static nonce would also work and not pose a security risk. Alternatively, the pages could be build with a static, known nonce placeholder (ideally a build/deploy time secret) which could then be replaced on each request with a dynamic nonce. We've used this approach in our cloudflare workers app and it seems to be working pretty well so far.

Karlinator commented 3 years ago

@pzuraq Nonces must always be unique for each HTTP request. If you send the same one twice then an attacker can inject inline script with that nonce simply by checking the nonce themselves previously.

Nonces also don't work for static hosts where you don't have the ability to generate them.

Additionally, we have the problem described above that adding the nonce (or a hash) in a <meta> tag is hard to combine with a CSP set outside it (and is therefore hard to configure and use). If we set script-src nonce-randomString then any script-src policy defined in the HTTP header must include 'unsafe-inline'. If a resource fails either policy it will be blocked. I think this also means that any domains listed in the HTTP header must also be replicated.

It'd be nice if CSPs were just combined additively, but according to the spec they're not.

I think it would generally be a good idea to not rely on the adapters and/or runtime modification to avoid unsafe-inline. Moving the current inline script to a different file would work on all adapters without any extra work, and avoids adding any CSPs. If you need nonces to run Analytics you can still do that, but it doesn't seem like SvelteKit should need it honestly.

Hashing the script would work, as it's all build-time (and therefore works for static as well), but we'd still need 'unsafe-inline' in any HTTP headers if we're adding it in a <meta> tag.

Additionally, as discussed in #1776, using nonces and hashes don't work with manifest V3. It'd be a bit of a shame to not support writing browser extensions at all.

The only downside to moving the script to an external file is in implementation. I've started looking at it (I'm writing an app that really should have a strict CSP), but I'm having a hard time figuring how to do it. Speaking of, if @benmccann or someone has a tip to how one could modify the page render pipeline to emit a script file I'm all ears. I've found the script itself in server/page/render.js, but I'm not sure how I would create a separate file and have it be served and stuff?

pzuraq commented 3 years ago

If you send the same one twice then an attacker can inject inline script with that nonce simply by checking the nonce themselves previously.

Right, but that only works if the contents of the page itself are dynamic. If they are not, there is no way to attack, unless I'm missing something. The alternative that allows you to still use strict-dynamic and include external sources is to have a script dynamically add them inline and make that script safe with a hash: https://csp.withgoogle.com/docs/faq.html#static-content

But that's unfortunate because the script has to start executing in order to begin loading all of the resources. Maybe it'll execute quickly enough that it wouldn't make a difference?

Edit: I guess maybe you could MITM with a static nonce value?

Karlinator commented 3 years ago

The point where you're relying on CSP to save you you've already assumed someone is able to inject <script> tags in your html. If we're assuming that the HTML can't be altered then we might as well ignore CSP.

Regardless, static nonces are against spec and we shouldn't use it to essentially circumvent CSP. I strongly believe that SvelteKit should not by default violate the CSP specification. https://www.w3.org/TR/CSP3/#security-nonces

If I have the ability to inject a <script> tag, then the point of nonces is that they stop me from doing so. If I know what the nonce is then it becomes essentially meaningless and you might as well use unsafe-inline.

I'm mostly concerned about the inline snippet which SvelteKit generates in the head of the document which is required to start hydration. If that script was instead loaded from /boot/xyz123.js as suggested earlier then it's allowed under script-src 'self' and no further action is required. That means we're just adding <script type="module" src="/boot/xyz123.js"></script> to the head, and the CSP will allow it. The strategy with loading from a script is only necessary if you're loading cross-origin scripts and don't want to include those origins in the script-src.

My ambition right now is that a SvelteKit app by default should require only script-src 'self' on the webserver level (which allows the use of CSP set either in hooks or in a reverse proxy), and I think to achieve that we need to move the script currently in the <head> to a separate file. Nonces and hashes still require that the "outer" CSP allows them, which requires either unsafe-inline or no script-src directive at all. For my use case in particular that would be inconvenient.

We would be waiting for an additional document load (it's a very small document, but it's at least another round trip, several over HTTP/1), but given that the rest of the scripts are preloaded (and also necessary for hydration if I've understood correctly) I don't think it has any real impact on loading time/performance.

Karlinator commented 3 years ago

So I've just realised that I was testing stuff in a repo without any styling. SvelteKit currently also requires style-src 'unsafe-inline' due to embedding the styling in the header as well.

seanlail commented 3 years ago

So I've just realised that I was testing stuff in a repo without any styling. SvelteKit currently also requires style-src 'unsafe-inline' due to embedding the styling in the header as well.

Is this only required in Dev mode?

Karlinator commented 3 years ago

Is this only required in Dev mode?

Just came back to say this, looks like it is. When building all the styles are extracted.

Karlinator commented 3 years ago

I've run into a new style issue.

Transitions rely on inline styles (the kind which is injected in the tag itself). I understand why this is done (it's a very natural place to put it), but sadly it requires 'unsafe-inline'.

These transitions are generated at compile time, so it is possible to get them into a CSP. Hashing is possible, but with the caveats above (as well as requiring 'unsafe-hashes'). We could put them in CSS files (and activate them with classes?), I think?

EDIT: Though, come to think of it, the transitions are managed by Svelte and not SvelteKit I believe, and should be raised there (https://github.com/sveltejs/svelte/issues/6662)

lberezy commented 3 years ago

I am running into this issue when attempting to use SvelteKit to produce a SPA webextension (which explicitly disallows unsafe-inline). Any suggestions on what I can do today to work around this, or must I wait for the small piece of inline JS to be moved to a file?

benbender commented 3 years ago

@benmccann I would like to propose to shift this issue from "post-1.0" to "pre-1.0" as security should be a day 1-feature.

Therefor it would be great to target support - at least - for CSP with a "strict-dynamic"-setting (where nonce'd scripts can generate inlined script-tags at will) for 1.0 and go from there.

The path from there, post-1.0, could be enhanced support for things like Permission-Policy (which is the base for the integrated google-floc-optout) or even emerging standards like trusted types.

Integrations like those above could help mitigate many common errors and attacks for many users by default and make sveltekit a privacy- & security-by-design framework for the years to come. Would love to see steps in this direction!

benmccann commented 3 years ago

You are welcome to send a PR for it to make it pre-1.0 :smile:

benbender commented 3 years ago

I generally would love to dig in, but as I'm not familiar with the build-system of sveltekit at all and there is, as far as I know, no documentation, but plans and implicit knowledge for pre-1.0 steps, two bundlers and quite some rough edges involved, I'll have to miss that shot.

In the end I'll take this, sadly, as a "no"... fair enough... Even if I had hoped, my request for rethinking priorities here, would have been considered a bit more seriously.

seanlail commented 3 years ago

Perhaps @acoyfellow has an insight to the work done in their comment? Sapper had nonce support, how difficult would it be to port that and is it even possible?

benmccann commented 3 years ago

Here's where the code is for anyone who'd like to take a stab at it:

https://github.com/sveltejs/kit/blob/bb4d3731f7fb42ed301633f5e07842dce472b8c8/packages/kit/src/runtime/server/page/render.js#L116

benbender commented 3 years ago

If it would be that simple, I would generate a hash, add the header where floc is handled and call it a day... But what's about AMP, SW, injected styles, (pre-)loaded modules, adapters... etc?

Karlinator commented 3 years ago

I'm starting to coalesce around a split solution, which is not something I'm a fan of but externalising the script seems really difficult for non-static settings.

Add an option (say "noncePlaceholders") which adds a placeholder (say "%svelte.CSPNonce") to all nonceable inline elements at render time (so <script type="module" %svelte.CSPNonce%>--init script here--</script>. This allows the developer to do a simple string replace to insert their nonce in a hook (which is presumably where they are adding the CSP directives anyway). This would work for all dynamic hosts.
Add an option to adapter-static to move the init script to a different file. This is by far the easiest adapter for which to do this. When toggled it would, after rendering, grab every inline script element and write them to an external file instead.

The first one is pretty easy, the second one I'm not sure about but shouldn't be too hard.

Both are things I've seen or used myself in more hacky ways using purely hooks, but adding this (and documenting it) would i think be a real boon.

benmccann commented 3 years ago

externalising the script seems really difficult for non-static settings

To expound on this difficulty, session and hydrate contain data blobs calculated for each request and route and spa can be set on a page-level by doing an export of those settings: https://github.com/sveltejs/kit/blob/bb4d3731f7fb42ed301633f5e07842dce472b8c8/packages/kit/src/runtime/server/page/render.js#L116

benbender commented 3 years ago

There also seems to be a generic one in the making which might be useful: https://github.com/josh-hemphill/vite-plugin-csp

Karlinator commented 3 years ago

To expound on this difficulty, session and hydrate contain data blobs calculated for each request

And so does page.query, which is the URLSearchParams used for that request page.

I do think it's a pretty reasonable assumption, by the way, that if you can't use nonces then you are using adapter-static, in which there is no runtime anyway so these problems don't exist.

RomanistHere commented 3 years ago

How reasonable is it to move this block manually to a separate file for production? Also what do you think about post-build script to do it?

antony commented 3 years ago

I've done a really ugly hacky version of a CSP here in case anybody needs a sticky-tape solution until we get around to implementing this.

https://github.com/antony/sveltekit-adapter-browser-extension

michmich112 commented 2 years ago

I made an adapter based on the static adapter that removes inline scripts for use cases for chrome extensions (this includes manifest v3). Its really more of a band-aid for now but hopefully it helps some of your which have this problem: sveltekit-adapter-chrome-exentsion

example using the extension

RomanistHere commented 2 years ago

I made an adapter based on the static adapter that removes inline scripts for use cases for chrome extensions (this includes manifest v3). Its really more of a band-aid for now but hopefully it helps some of your which have this problem: sveltekit-adapter-chrome-exentsion

404

michmich112 commented 2 years ago

I made an adapter based on the static adapter that removes inline scripts for use cases for chrome extensions (this includes manifest v3). Its really more of a band-aid for now but hopefully it helps some of your which have this problem: sveltekit-adapter-chrome-exentsion

404

fixed

Rich-Harris commented 2 years ago

I'm taking a long overdue look at this and #2394 (thank you @Karlinator). I agree with the sentiment that this should ideally be a pre-1.0 consideration, if we're able to figure out the right API.

We need to support inline scripts and styles for a couple of reasons:

Performance. Having the init script be inline will likely yield a better Time To Interactive than an external script. Similarly, we recently added a performance optimisation that inlines stylesheets below a certain size thresold
We have to assume that the server is stateless, which means creating external scripts dynamically becomes tricky (we'd basically need to encode the contents in the script URL — yikes)

(In the case of Chrome extensions clearly we do need to be able to externalise scripts; whether that needs built-in support or can be implemented using the existing trickery is up for debate.)

Since prerendering is something supported by all adapters, not just adapter-static, I lean towards using hashes rather than nonces. One possible idea would be to have Kit add CSP headers automatically, if enabled, with the ability to add additional directives. A config like this...

// svelte.config.js
export default {
  kit: {
    csp: {
      directives: {
        'img-src': ['https://example.com/']
      }
    }
  }
};

...might result in the following header...

Content-Security-Policy: script-src acd123==; style-src def456==; img-src: https://example.com

...because Kit computes hashes for the inline init script and any inlined styles, and adds custom directives specified in the config.

Pros:

easy to configure
the options can be typed, helping to ensure that the header is well-formed
works with both prerendered and dynamically rendered pages

Cons:

No control over individual pages (other than by manipulating the headers in handle)
Additional scripts like analytics are left as an exercise to the reader (though that seems solvable — just throw those in static rather than inlining them. bonus: they load lazily)

Thoughts?

Re inline styles added by Svelte itself — I'm hoping that we can transition (geddit?) to use the Web Animations API before too long, so that we no longer need to use unsafe-inline.

RomanistHere commented 2 years ago

Performance. Having the init script be inline will likely yield a better Time To Interactive than an external script. Similarly, we recently added a performance optimisation that inlines stylesheets below a certain size thresold

Well, performance is good. But it's going to be a security flow, isn't it? Some security-based companies would not want to use SvelteKit because of it or will need to write a custom solution for extracting the scripts and styles.

Ideally I'd like to see a flag in config, based on which important (for performance) scripts and styles can be either inline or in external files. What do you think?

pzuraq commented 2 years ago

@Rich-Harris we're currently using nonces, and it impacts every page load since we have to replace the nonce value. So I think I would also lean toward hashes, since it works with prerendering and will generally be more performant.

One additional consideration would be supporting strict-dynamic, which I mentioned above. This would mean hashing not just inline scripts, but all scripts loaded on the page, because when enabled it causes 'self' and all URL based entries in the allowlist to be ignored in favor of either hashes or nonces. I think there are tradeoffs here, for instance it's difficult to use strict-dynamic with hashes and include external script tags (e.g. Google Analytics), so maybe it doesn't need to be a v1 feature, but it would be nice to have it as an option.

Karlinator commented 2 years ago

@Rich-Harris I like the API you propose for this. I've stated elsewhere that the "generate a nonce and pass it to a hook" approach I took in the PR, leaving the actual injection of the headers up to the application, is a stopgap measure and not my preferred API. I have a couple of points on this specifically:

Typing CSP directives is not entirely trivial (which directives can take what kinds of contents etc). Some inspiration: https://github.com/josh-hemphill/csp-typed-directives
Use arrays, and none of that string | string[] stuff (I seem to recall you hold this opinion as well)
We need to be consistent about e.g script-src 'self' and script-src 'strict-dynamic'. In the spec these have quotes. Do we enforce that in the config (so 'script-src': ["'self'", "'strict-dynamic'"]) or omit them ('script-src': ["self", "strict-dynamic"])?
In a strict-dynamic setting, the user could manually hash external scripts and add those hashes to the header here (versioned links to external scripts should already be hash-stable).

As for the actual substance of the implementation, I think a one-size-fits-all solution might be hard to come by. The woes of being platform agnostic has already revealed that e.g Chrome extensions need to be able to externalise scripts, and I would suggest that this should somehow be an option in all static contexts—prerendered pages should ideally be able to externalise the init script. Attaching this to the prerender covers almost all the situations where I would want it I think and without requiring some nasty workaround for the sateless runtime (like encoding the script in the url which, yeah, yuck).

On hash vs nonce I'm a bit uncertain. Hashing can often come with the security benefit of only manually approved scripts being allowed—blocking some vectors where a script is modified maliciously. Of course, by generating the hash at request time we are essentially bypassing that, making the function more or less identical with nonces, so really I think implementation concerns should decide here. Usually nonces are preferred for dynamic script tags, but there's no specific reason against hashes I think.

I originally made the PR with nonces because that seemed easier, and I wasn't really considering prerendering. Generate a string in the adapter, and pass it inwards. Hashing requires passing a hash function from the adapter all the way in through the render, hashing the script string, then passing the hash back out again to the header. But both are doable.

I have noticed that in my hooks-based workarounds hashes have been sometimes inconsistent. I never dug deep enough to find out why, but they would sometimes block the script anyway, something I don't see with nonces. It might be related to how hashes are sensitive to even a single whitespace character or trailing newline? (And it's not like my regex-based nonsene was the most stable thing ever). That's one reason I kind of prefer hashes only where I know the hash won't change so I can test and verify.

If we want there's also nothing stopping hashes and nonces from living together. We can collect hashes during prerendering, store them somewhere, and serve headers with hashes with prerendered pages. Then when we render dynamically we can use nonces instead. I think conceptually this feels the most "correct", but it may be a waste of effort.

@RomanistHere whether host-source directives or hash/nonce (with or wothout strict-dynamic) is the "better" way is as far as I understand an endless debate. But I think argument 2 is the killer here: externalising this script is basically impossible unless you're in some kind of static context (either on adapter-static or possibly serving prerendered pages). I think if you want to avoid all hashes and nonces then "you need to prerender all your pages" is a perfectly reasonable requirement.

@pzuraq Remember that the hash would need to be calculated for every page load too, since the inline init script changes on a per-request basis. Only if the page is prerendered will the hash remain the same.

I think strict-dynamic is possibly easier with nonces? If you're using hashes to allow external scripts then either you need to specify the hash at deploy time (possible but tedious) or Kit need to fetch and hash the script at request time (please no). Of course the right way to accomplish this is to load external scripts from a hashed inline tag:

    var s = document.createElement('script');
    s.src = "https://cdn.example.com/some-script-you-need.min.js";
    document.body.appendChild(s);

I actually don't think it's too big of an ask to support strict-dynamic? Not counting external scripts, my PR, imperfect as it is, already supports it by noncing every link in the HTML head. We could give the user somewhere to list script links to insert, and then either putting them in the HTML with a nonce or appending them from a hashed inline script. I'm thinking something like:

// svelte.config.js
export default {
  kit: {
    externalScripts: [
      {
        src: "https://cdn.example.com/some-script-you-need.min.js",
        tag: "script",
      },
      {
        href: "https://cdn.example.com/some-other-script-you-need.min.js",
        tag: "link",
        rel: "modulepreload",
      }
    ]
  }
};

pzuraq commented 2 years ago

strict-dynamic is definitely easier with nonces, but wouldn't work for pre-rendered pages. Also I think hashes would still be cacheable if the requests were identical, because the inline script would be the same right? Like, if the request is exactly the same then the response should be the same, and the hash should still be valid.

With nonces, you were correct in your original post above, they do have to be unique for every single request. Right now we solve this by caching the response but then find/replacing the nonce after each response, which is a bit of a pain. However, in those cases we never even call SvelteKit's render function, so it returns pretty quickly overall.

Re: appending via a hashed inline script, that would mean that the browser has to first parse and run some JavaScript before it can start loading external scripts, which is not ideal IMO. I think noncing/hashing every external script in <head> would be preferable, so the browser could start loading them as soon as it reads them. I also think if the user wants hashes, they probably should provide any hashes for external scripts themselves to keep things simpler in the framework - hashes should only be generated for code that SvelteKit is responsible for generating.

pzuraq commented 2 years ago

Another thing to note about nonces is that our setup requires a secret to be added to the deployment environment, which is a bit of a pain. We use this secret to be the nonce placeholder to target with the find/replace after each cache hit. It has to be a secret because if it were a well known value, you could target it for script injection in SSR.

Unsure if there would be an easy way to remedy that within SvelteKit itself, maybe the secret could just be generated by the framework on load? It would have to persist between different instances though on some providers, Cloudflare (our provider) shares a cache with multiple workers so the placeholder has to be the same across all workers.

Rich-Harris commented 2 years ago

Thanks for the valuable feedback everyone. I think we're close to squaring all the various circles.

Ideally I'd like to see a flag in config, based on which important (for performance) scripts and styles can be either inline or in external files. What do you think?

Yeah, it probably makes sense for this to live in Kit rather than adapters having to jump through lots of hoops themselves. Something like this perhaps:

// svelte.config.js
export default {
  kit: {
    prerender: {
      externaliseScriptsAndStyles: true
    }
  }
};

That's something that could happen independently of the CSP work, even though it's motivated by it.

One additional consideration would be supporting strict-dynamic

Totally doable — at build time we can hash all the assets, and use the hashes at render time if 'strict-dynamic' is enabled:

// svelte.config.js
export default {
  kit: {
    csp: {
      directives: {
        'script-src': ['strict-dynamic']
      }
    }
  }
};

Some inspiration: https://github.com/josh-hemphill/csp-typed-directives

Woah! This is very useful (I don't think I'd have been able to figure out how to get autocompletion for unsaf... while also supporting URLs etc).

Agree re string[] rather than string | string[]. I think ['self'] is preferable to ["'self'"], even if it would be more 'correct' to do the pedantic thing — I don't think there's any real potential for ambiguity, and the author of csp-typed-directives appears to agree.

Of course, by generating the hash at request time we are essentially bypassing that, making the function more or less identical with nonces, so really I think implementation concerns should decide here.

To clarify, I was only proposing hashing the scripts that SvelteKit generates — not injecting hashes (or nonces, for that matter) in cases like this:

<div class="comment">
  <p>someone forgot to escape user input <script>alert('lol')</script></p>
</div>

That would mean people were responsible for their own dynamic scripts (google analytics, etc), either by putting them in static and allowing self, or using strict-dynamic and a) generating the hash themselves, as suggested above, or b) creating the script dynamically when the app loads (right now that would probably happen in the root __layout, but we'll probably add a client-side equivalent of src/hooks.js at some point soon, where this sort of thing could happen).

Then when we render dynamically we can use nonces instead.

Yeah, I think this hybrid approach makes sense. Nonces for dynamic, hashes for static. And we could expose the nonce — %svelte.nonce% — for things like analytics snippets and external scripts with strict-dynamic, but disallow its usage during prerendering, perhaps.

For posterity, I'm going to recap a discussion I had with @antony earlier since he raised a very reasonable question — rather than having Kit generate CSP headers based on config, what if it just gave you the tools you need to generate them yourself? For example this:

<meta http-equiv="Content-Security-Policy" content="default-src 'self' %svelte.cspHashes% ">

On the face of it this makes a lot of sense, since it allows Kit to get out of the way, and provides app authors with maximum flexibility. But after discussing it we concluded it probably wasn't the right direction:

headers are preferable to <meta http-equiv> in situations where you control them (i.e. not prerendering)

if using headers, we'd need to change the handle API and you'd need to implement a custom hook (and be careful to generate well-formed headers):

const { response, hashes } = await resolve(request);
if (hashes) { // only true for pages
 response.headers.set('content-security-policy': `script-src: ${hashes.script}`);
}

prerendering would have to read the headers you'd generated in your custom hook, and inject them into the head using <meta http-equiv>. but this is tricky: normally the way to inject content into the head is to insert it right before </head, since otherwise you have to use brittle regexes, but with CSP the <meta> needs to go before your inline scripts and styles. So realistically we'd need to use cheerio or something to parse the HTML, which will slow prerendering down and make Kit heftier

So there'd be a lot more hoop-jumping, for both authors and the framework, and you'd lose the benefits of typechecked directives etc. The result would be that way fewer people would end up enabling CSP, unless someone made them do it. All in all it feels like this is one of those places where it makes sense for the framework to step in and say 'I got this'.

pzuraq commented 2 years ago

Yeah, I think this hybrid approach makes sense. Nonces for dynamic, hashes for static. And we could expose the nonce — %svelte.nonce% — for things like analytics snippets and external scripts with strict-dynamic, but disallow its usage during prerendering, perhaps.

@Rich-Harris Would this be optional, to allow people to choose between using hashes or nonces based on what makes more sense to their setup? I think we would prefer hashes for the reasons I mentioned above, would help us cache our responses more aggressively and not need to rerender just to update the nonce.

Rich-Harris commented 2 years ago

Yeah, perhaps we could have an option like this, where 'auto' is the default

// svelte.config.js
export default {
  kit: {
    csp: {
      mode: 'hash' // or 'nonce' or 'auto', meaning 'hash' for prerendered and 'nonce' for dynamic
    }
  }
};

Karlinator commented 2 years ago

To clarify, I was only proposing hashing the scripts that SvelteKit generates — not injecting hashes (or nonces, for that matter) in cases like this:

Absolutely! I was just saying the (admittedly minor) security differences between hashes and nonces kind of disappear for us. If you can compromise a nonced init script in Kit then you could do the same for a hashed script (though both would be difficult), when with only compile-time hashes you have a slight advantage.

On hashes for external scripts: This is a feature of CSP 3, and annoyingly not yet implemented in Firefox. That's a bit of a bummer. As far as I can tell other browsers support this. This is problematic for using hashes generated at compile time to allow our static scripts, which is otherwise an idea I really like. I'm not sure how much we should let this affect us though, since this is Firefox simply not being in line with spec and is likely to be fixed at some point.

And we could expose the nonce — %svelte.nonce% — for things like analytics snippets and external scripts with strict-dynamic, but disallow its usage during prerendering, perhaps.

We would need to be careful about this, no? As @pzuraq mentions this can be a target for attacking the SSR, and we don't want this to get through:

<div class="comment">
  <p>someone forgot to escape user input <script nonce="%svelte.nonce%">alert('lol')</script></p>
</div>

I forget exactly how Kit's render pipeline works, but I imagine we can search for this string in, say, only the static app.html before any other content is added?

Suggestion so far (in auto mode):

Kit builds all the static files, hashes them, and stores the hashes somewhere (manifest?)
The adapter calls prerender. Kit renders the files, hashes the inline scripts, adds those to the manifest. All script links are given integrity attributes (required for hashes of external scripts to work)
Runtime. If Kit serves a response from the prerender cache, it sends with the hash headers relevant (that page and relevant assets). If instead it renders the page, it works mostly like in my PR (adapter gives a nonce, inserted into all script tags).

In nonce mode it would ignore hashes completely (basically my PR). In hash mode I guess it would dynamically hash the contents when rendering? Insert integrity attributes like during prerendering, hash the inline script, and send CSP headers with hashes.

Do we hash/nonce all scripts even if we're not in sctrict-dynamic? I don't think it can hurt, and it saves us from another toggle.

Karlinator commented 2 years ago

Wait, I left out kit.externaliseScriptsAndStyles from that. That would only have effect during prerendering I think (we've discussed at length here how that's almost impossible to combine with SSR), and would cause the prerender to return separate html and js file which can be placed somewhere. Some predictable url is needed, which Kit can insert into the html at render and then later serve the JS file from. Anything rendered at runtime basically ignore this.

pzuraq commented 2 years ago

We would need to be careful about this, no? As @pzuraq mentions this can be a target for attacking the SSR, and we don't want this to get through:

My understanding was that %svelte.nonce% would only be replaced in sections of the code that are known to be safe, e.g. before user content that has been SSR'd is inserted. That's a fine setup, because there's no user content added before the replacement.

The problem comes if we want to cache the entire response, so we don't need to enter into SSR in the first place. When caching the response, we lose context so there's no way to tell which parts of the response are "safe" to replace in and which are not. Responses could potentially be cached in parts to avoid this, maybe even the nonce placeholder could be cached as part of the response and generated uniquely for each response 🤔 but that could be platform specific. Cloudflare uses the Cache API to cache responses, and it expects a full Response object, so you'd have to like, store this info in a header or something like that.

Karlinator commented 2 years ago

My understanding was that %svelte.nonce% would only be replaced in sections of the code that are known to be safe, e.g. before user content that has been SSR'd is inserted. That's a fine setup, because there's no user content added before the replacement.

Yeah, that was my assumption too, just wanted to make it explicit.

Rich-Harris commented 2 years ago

%svelte.nonce% in rendered content wouldn't be replaced, because in production no string replacement happens. But I opened #3490 anyway because I'm a completionist

pzuraq commented 2 years ago

Thought I'd mention something that just popped up in the Discord that's a bit related. Currently, we have a few places where we need to use dynamic styles in our templates, as things like the background color of certain elements are theme-able. This currently does not work with our CSP, as we do not allow unsafe-inline for styles, so there's a brief pop-in flash as the page rerenders from JS taking over, at which point the styles are set via a trusted script and render properly.

Not sure what the best way to solve this would be, and it's definitely an edge case so it doesn't necessarily have to be in the first pass. I believe style attributes can also be hashed, so maybe there could be a way for a user to indicate that a certain style tag should be hashed and included?

Karlinator commented 2 years ago

@pzuraq That seems like it touches on both Kit and Svelte itself, no? Svelte is responsible for generating those inline styles (this is one annoyance of CSP, that it often requires passing a nonce or a hash all the way through an application stack from "the thing that generates CSS" to "the thing that sets HTTP headers").

Anyways, this is controlled by the style-src-attr directive, and allows hashing if you also specify unsafe-hashes. Noncing is not available for attribute styling. I'm not sure if I like adding unsafe-hashes (though I think it isn't as unsafe as the name implies).

pzuraq commented 2 years ago

Yeah, agreed re: unsafe-hashes. Another option would be to somehow allow SvelteKit to generate a stylesheet or style tag dynamically, and generate the hash for that. Will have to think about what would be the best way to do that.

Rich-Harris commented 2 years ago

Closed via #3499. Opened #3555, #3556, #3557 and #3558 to track follow-up tasks. I'm not going to be working on CSP stuff in the near term, so if anyone needs these and is up for a PR then please feel welcome!

benmccann commented 2 years ago

CSP docs are here: https://kit.svelte.dev/docs/configuration#csp

sveltejs / kit

Content Security Policy support #93