use a content-security-policy in development

[iliana]

In preparation for adding content-security-policy and other security headers to console endpoints in Nexus, we can add them here too to ensure they're applied when developing / running tests / etc.

The base headers are defined in vercel.json and then imported into vite.config.ts to avoid repeating them.

The content-security-policy is based on the recommendation by the OWASP Secure Headers Project (click the "Best Practices" tab). I am still tweaking this as I look at other sites for recommendations, but I wanted to see if this breaks any tests. The directives:

• default-src 'self': By default, restrict all resources to same-origin.
• style-src 'unsafe-inline' 'self': Restrict CSS to same-origin and inline use. We've fixed a number of these issues as part of this PR but we still have some, so we'll track those in Tracking: remove style-src: 'unsafe-inline' from CSP #2183.
• frame-src 'none': Disallow nested browsing contexts (<frame> and <iframe>).
• object-src 'none': Disallow <object> and <embed>.
• form-action 'none': Disallow submitting any forms with an action attribute (none of our forms appear to be the traditional kind and instead use events).
• frame-ancestors 'none': Disallow embedding this site with things like <iframe>; used to prevent click-jacking attacks.

In development mode, an additional script-src CSP directive is added which references a randomly-generated nonce. Vite injects this in the generated index.html so that the dev-mode scripts can load. We do this instead of allowing 'unsafe-inline' because I'm not sure whether tests run against dev bits or not, and this helps get dev builds much closer to production.

Also set are x-content-type-options: nosniff and x-frame-options: DENY.

It would be nice to write tests (both vitest and playwright?) that verify this header is set in those environments so we know we're actually testing the CSP.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Apr 23, 2024 8:10pm

[benjaminleonard]

We might not need the headers in vercel.json since all we're really using it for is the mock preview, though there's probably no harm in keeping them there.

[david-crespo]

I was thinking the same thing. Will think about it.

[iliana]

Yeah, it's certainly optional for us to add them to vercel.json, but I also figure why not (apart from the hot mess of importing the JSON file when loading the Vite config). My thinking was, if we see some regular usage out of that, that's more incidental testing on the CSP... but I don't know if we get bug reports about the console preview.

[david-crespo]

Still looking through all the values but I think this is good. I don't see any errors, everything seems fine. Good idea using the vercel config as the single source of truth.

[iliana]

oh, lol, this was me testing to see if prettier or eslint would fix this (it apparently didn't)

[iliana]

Do we have some way of knowing that the Playwright tests are actually using this header? (Does it use Vite to spin up the server?)

[david-crespo]

One thing I probably want to do is find a way in the code itself to comment on each of the values and why they are what they are. It looks like we can't convert vercel.json to .jsonc and we probably can't jam comments in there, so I might add it as a markdown thing in the docs dir.

[david-crespo]

I believe we can pretty easily assert in playwright about the headers on a response. I'll try that now. And yes, the tests run against the dev server both in CI and locally:

console/playwright.config.ts

Lines 48 to 51 in f1e8f2e

	webServer: {
	command: 'npm run start:msw -- --port 4009',
	port: 4009,
	},

[david-crespo]

The vite doc says:

Ensure that you replace the placeholder with a unique value for each request. This is important to prevent bypassing a resource's policy, which can otherwise be easily done.

I assume that's relevant in production — maybe we don't need to bother with the nonce at all in local dev?

[iliana]

Yes, that's primarily relevant for production. The reason I went ahead and randomized it for each config load is so that someone can't add a <script nonce="fixed-dev-string"> and have it work in dev, but that's a bit silly (it wouldn't work on the Vercel preview or the real control plane). For instance doing it this way in dev mode means the nonce is fixed on each page load for the existence of that dev server.

The real purpose of cspNonce is to put a placeholder string there, which can be replaced and randomized by the actual web server on each request it serves. We don't need that because our prod builds don't have any inline scripts, they all come from 'self' which is reasonably safe for our purposes.

(from https://csp-evaluator.withgoogle.com/)

But we could pretty easily have Nexus generate a nonce, if we want. I guess we wouldn't be able to apply the correct nonce in vercel.json though.

[iliana]

I suppose how that would look is:

• We set cspNonce to always be something like NexusPlaceholder.
• We update the CSP to always have script-src 'nonce-NexusPlaceholder', in server/preview/vercel.
• Over in Nexus we change the code to search and replace NexusPlaceholder in index.html with a random hex value and also put that in the CSP header on each request.

I haven't yet researched if that breaks any browser addons.

[david-crespo]

Ok, I think the placeholder thing makes more sense than what we have now, but I suspect, as you say, script-src 'self' is sufficient and we can skip the nonce thing altogether both here and in omicron.

---

I'm thinking of adding more explanation to the doc of why we're doing this client-side. How close is this? Do you think there is much of a direct security problem we're solving? The only attack vector I can think of is a malicious npm dependency.

The reason we are setting CSP headers not just in Nexus, where they affect production deployments, but also in local dev (which Playwright runs against) and in the Vercel previews is not primarily the security of those environments but rather so we can know as early as possible in the development process if a given CSP directive breaks the web console.

[iliana]

I don't believe we're solving a security problem directly by putting the CSP here, so your writeup is correct to me. The primary reason for starting here is to catch issues early (and before we turn it on in Nexus, where clickjacking/XSS/etc are far more of a potential threat).

Given that the Playwright tests run against the dev server, I definitely want to keep the nonce setup in non-production mode; the alternative is that we must allow script-src 'unsafe-inline' and that would mean we're not testing that the console works without that.

[iliana]

Added this because I added more content to an existing patch in a commit, which made Vercel break. Turns out it caches your node_modules at the end of the run, and patch-package only knows not to apply a patch if it's entirely applied.

ds300/patch-package#37 has some related chatter.

[iliana]

I removed this so we could remove react-style-singleton from the bundle in its entirety.

Also, a much simpler patch to react-remove-scroll should be possible and have the same effect, without removing the offending code from our bundle, if a smaller patch is preferred:

diff --git a/node_modules/react-remove-scroll/dist/es2015/UI.js b/node_modules/react-remove-scroll/dist/es2015/UI.js
index 26c94a8..5991c96 100644
--- a/node_modules/react-remove-scroll/dist/es2015/UI.js
+++ b/node_modules/react-remove-scroll/dist/es2015/UI.js
@@ -26,7 +26,7 @@ var RemoveScroll = React.forwardRef(function (props, parentRef) {
 });
 RemoveScroll.defaultProps = {
     enabled: true,
-    removeScrollBar: true,
+    removeScrollBar: false,
     inert: false,
 };
 RemoveScroll.classNames = {

[iliana]

I just remembered that I need to verify that WebSockets (serial console) still work...

[iliana]

The console websocket works fine with this CSP on Firefox. To-spec source matching was apparently wrong in WebKit (Safari) until two years ago. Not sure what Safari versions we consider supported? https://bugs.webkit.org/show_bug.cgi?id=235873

Unfortunately there appear to be some inline styles in xterm.js that I'll need to hunt down. I didn't realize that was in a separate bundle.

[iliana]

If we want this to land in R8, we should add 'unsafe-inline' back to style-src, I think (and do the same over on the Omicron PR). Getting xterm.js to play nicely will take a bit more time than the patches above did.

[david-crespo]

I think that makes sense. Everything else is a big improvement on the status quo.

[iliana]

Cool. I'll get that done today so we can land it!

[david-crespo]

Think it's worth getting rid of the patches and redoing them later? Keeps things cleaner here.

[iliana]

Personally I'd like to keep them so we can see if/when they break with upstream changes? I don't think the behavior we're patching out is something we really want anyway. Unless the reason they're pulled in (e.g. radix-ui) are being dropped at some point.

[david-crespo]

Yeah, that's fine. We'll see conflicts if we bump the dep and it conflicts with changes they've made.

[david-crespo]

Thank you for doing this! Putting it on auto-merge and I'll go give a quick last look to the Omicron PR.