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

secureheaders recipe: add Content-Security-Policy header and includeSubDomains; preload to Strict-Transport-Security #3177

Closed
wants to merge 9 commits into from

Conversation

plbstl
Copy link

@plbstl plbstl commented Feb 19, 2022

Related: #2019

What are the changes and their implications?

Bug Checklist

  • Integration test added (see test docs if needed)

Feature Checklist

- add img-src policy for images and gifs
- move scriptHash to production only so that unsafe-inline works in firefox
- add `Paul Ebose` to list of contributors
@plbstl
Copy link
Author

plbstl commented Feb 19, 2022

@blitzjs/next: 11.1.0-0.45.3

blitz.config.ts before and after these changes:

Screenshot_20220219_025501

When I installed the secureheaders recipe, the dev server could not start because of an invalid blitz config structure. 9e1ca0d

Also, data URI images failed to load in Firefox due to security restrictions. d7ea7e7

Content-Security-Policy.frame-ancestors compliments X-Frame-Options header and is ignored if put in a meta tag.

return `default-src 'self'; style-src 'self' 'unsafe-inline' fonts.googleapis.com; font-src 'self' data: fonts.gstatic.com; script-src 'self' ${
process.env.NODE_ENV === "production" ? "" : "'unsafe-eval'"
} ${scriptHash}`
return `default-src 'self'; img-src 'self' res.cloudinary.com data:; style-src 'self' 'unsafe-inline' fonts.googleapis.com; font-src 'self' data: fonts.gstatic.com; script-src 'self' ${
Copy link
Member

Choose a reason for hiding this comment

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

Is this addition necessary? Not every app requires cloudinary, and allowing the data: protocol gives a malicious user a method to inject XSS attacks https://www.w3.org/TR/CSP3/#csp-directives

Copy link
Author

@plbstl plbstl Feb 20, 2022

Choose a reason for hiding this comment

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

Not every app requires cloudinary

It can be removed. I put it in there as an example, since also not every app require Google fonts.

allowing the data: protocol gives a malicious user a method to inject XSS attacks https://www.w3.org/TR/CSP3/#csp-directives

I'm adding data: protocol to the img-src directive. From the link you sent:

To mitigate the risk of cross-site scripting attacks, web developers SHOULD include directives that regulate sources of script and plugins. They can do so by including:

• Both the [script-src](https://www.w3.org/TR/CSP3/#script-src) and [object-src](https://www.w3.org/TR/CSP3/#object-src) directives, or
• a [default-src](https://www.w3.org/TR/CSP3/#default-src) directive

This web.dev article hints that the script-src, object-src, and base-uri directives are relevant for XSS

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with keeping the data: protocol for img-src then, but I think cloudinary can be left out. Unlike cloudinary, Google fonts is used by the default app template and the secureheaders recipe should generate a policy that doesn't produce errors on a fresh blitz app.

It would be a good idea to add a comment next to that line letting users know they can remove the Google font if they've replaced the default CSS though.

- add comment to remove Google fonts domains, if need be
- update recipe repo link
- add more helpful links in readme
@plbstl plbstl requested a review from MrLeebo February 23, 2022 18:13
Copy link
Member

@MrLeebo MrLeebo left a comment

Choose a reason for hiding this comment

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

Great work on this 👍

@beerose beerose changed the title improve secureheaders recipe secureheaders recipe: add Content-Security-Policy header and includeSubDomains; preload to Strict-Transport-Security Feb 28, 2022
Copy link
Contributor

@beerose beerose left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

@itsdillon itsdillon changed the title secureheaders recipe: add Content-Security-Policy header and includeSubDomains; preload to Strict-Transport-Security [legacy-framework] secureheaders recipe: add Content-Security-Policy header and includeSubDomains; preload to Strict-Transport-Security Jul 7, 2022
@itsdillon itsdillon changed the title [legacy-framework] secureheaders recipe: add Content-Security-Policy header and includeSubDomains; preload to Strict-Transport-Security [legacy-framework] [legacy-framework] secureheaders recipe: add Content-Security-Policy header and includeSubDomains; preload to Strict-Transport-Security Jul 7, 2022
@itsdillon itsdillon changed the title [legacy-framework] [legacy-framework] secureheaders recipe: add Content-Security-Policy header and includeSubDomains; preload to Strict-Transport-Security [legacy-framework] secureheaders recipe: add Content-Security-Policy header and includeSubDomains; preload to Strict-Transport-Security Jul 7, 2022
@beerose beerose changed the title [legacy-framework] secureheaders recipe: add Content-Security-Policy header and includeSubDomains; preload to Strict-Transport-Security secureheaders recipe: add Content-Security-Policy header and includeSubDomains; preload to Strict-Transport-Security Aug 1, 2022
@siddhsuresh
Copy link
Member

Closing since required headers added in #3813

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants