Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: set csp and content-type-options headers #1293

Merged
merged 3 commits into from
Jan 27, 2023
Merged

feat: set csp and content-type-options headers #1293

merged 3 commits into from
Jan 27, 2023

Conversation

markphelps
Copy link
Collaborator

Fixes: FLI-178

Sets Content-Security-Policy header to "default-src 'self'; img-src *;" which defaults to only allow resources, scripts, etc from this host, but allows images from anywhere per: https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP#examples_common_use_cases

See also: https://infosec.mozilla.org/guidelines/web_security#content-security-policy

Also sets X-Content-Type-Options as well

I am a bit concerned about breaking some functionality since there are some warnings in the browser, however it doesn't seem to actually affect anything during the light testing I've done:

CleanShot 2023-01-26 at 16 23 50

One option is it make these opt-in, or maybe opt out? Although there's a multitude of ways to configure CSP, so I'm not sure if we can come up with a default that works for everyone. A final option is to simply not do this work ourselves

@markphelps markphelps requested a review from a team as a code owner January 26, 2023 21:27
@markphelps markphelps changed the title fix: set csp and content-type-options headers feat: set csp and content-type-options headers Jan 26, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2023

Codecov Report

Merging #1293 (eb12bc7) into main (9eef7e5) will decrease coverage by 0.19%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1293      +/-   ##
==========================================
- Coverage   79.88%   79.70%   -0.19%     
==========================================
  Files          43       43              
  Lines        3267     3267              
==========================================
- Hits         2610     2604       -6     
- Misses        527      531       +4     
- Partials      130      132       +2     
Impacted Files Coverage Δ
internal/storage/oplock/sql/sql.go 90.82% <0.00%> (-5.51%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dug into this a bit this morning.

Screenshot 2023-01-27 at 11 12 19

I am fairly certain in the first case we're just seeing React Dev tools getting rejected by the new CSP rules:
(prepareInjection.js)

facebook/react#17997
facebook/react#26051

https://github.com/facebook/react/blob/main/packages/react-devtools-extensions/firefox/manifest.json#L55

This is how the extensions (Chrome + Firefox) get side-loaded into the page.
They fetch the source for the script tag and then inline it directly into the page.
The CSP rule for that would have to be something like unsafe-inline which we dont want as that defeats the purpose. Since this is for devtools, I think we can just get away with going through vite and for frontend dev without the CSP rules enabled there.

The second eludes me a little. zooming-in a bit it appears to be side-loading styles:

// snippet taken from bundled source
let o=document.createElement("style");o.setAttribute("data-n-href",r),o.setAttribute("media","x"),n&&o.setAttribute("nonce",n)

Update:

I believe there is a clue in there (the nonce) which makes me think of the CSP nonce base64 value:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/Sources#nonce-base64-value

This led me to here: vercel/next.js#18451

And that led me to here: https://reesmorris.co.uk/blog/implementing-proper-csp-nextjs-styled-components

That is obviously Next js which we have moved away from. However, I wonder if something similar is happening and we might somehow find a way to set the nonce value server side to support whatever this is attempting to load.

Update (2): I think this is font-awesome related

FortAwesome/react-fontawesome#92
https://fontawesome.com/v5/docs/web/other-topics/security#content-security-policy

@GeorgeMac
Copy link
Contributor

GeorgeMac commented Jan 27, 2023

Update (3): Following that last blog post I linked I cut a branch on the UI which removes the second fontawesome related CSP error:

flipt-io/flipt-ui#51

Screenshot 2023-01-27 at 11 59 35

So it is just borking chrome dev tools when you run the UI embedded like we do in production Flipt.
Which we can ignore.

P.S. I confirmed disabling devtools removed that last error.

SHIPIT :shipit:

@markphelps markphelps enabled auto-merge (squash) January 27, 2023 13:41
@markphelps markphelps merged commit 9286fff into main Jan 27, 2023
@markphelps markphelps deleted the csp-header branch January 27, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants