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

Add security headers and policies #3540

Merged
merged 8 commits into from
Sep 8, 2021
Merged

Conversation

parlough
Copy link
Member

@parlough parlough commented Sep 1, 2021

Closes #1521

@google-cla google-cla bot added the cla: yes Contributor has signed the Contributor License Agreement label Sep 1, 2021
@github-actions
Copy link

github-actions bot commented Sep 1, 2021

Visit the preview URL for this PR (updated for commit 5c75c04):

https://dart-dev--pr3540-misc-add-some-securi-pt7d3ziq.web.app

(expires Sun, 12 Sep 2021 22:09:53 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: d851bc446d3c4d7394c5406c6f07255afc7075f3

Copy link
Member

@jonasfj jonasfj left a comment

Choose a reason for hiding this comment

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

This seems reasonable..

firebase.json Outdated Show resolved Hide resolved
firebase.json Outdated
{
"source": "**",
"headers": [
{ "key": "Content-Security-Policy", "value": "default-src https:; script-src 'self' 'unsafe-inline' 'unsafe-eval' https://www.googletagmanager.com https://www.google-analytics.com https://ssl.google-analytics.com https://gstatic.com https://*.gstatic.com https://dartpad.dev https://*.dartpad.dev https://youtube.com https://fonts.googleapis.com https://doubleclick.net https://*.doubleclick.net https://google.com https://*.google.com https://storage.googleapis.com; object-src 'none'; base-uri 'none'; style-src https: 'unsafe-inline'"},
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need: 'unsafe-inline' 'unsafe-eval'

I'm guess there is an actual reason :D

Perhaps we file a follow-up to fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The search page and the Google Tag Manager script on every page relied on unsafe-inline. Adding the hashes, I was able to remove unsafe-inline, and as far as I can there are no new problems popping up from that change.

unsafe-eval seems trickier as it is currently used by Google CSP for the search page as well as on the Dart SDK Archive page.

Copy link
Member

Choose a reason for hiding this comment

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

@parlough another option, instead of adding hashes is to embed the Google Tag Manager script from a .js file.

See:
https://github.com/dart-lang/pub-dev/blob/master/static/js/gtm.js

That's how we did on pub.dev

Copy link
Member

@jonasfj jonasfj left a comment

Choose a reason for hiding this comment

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

This looks fine.

@kwalrath you'll want to check if this creates issues when rolling it out, and then revert if it does.

@parlough parlough changed the title Add simple security headers Add security headers and policies Sep 5, 2021
Copy link
Contributor

@kwalrath kwalrath left a comment

Choose a reason for hiding this comment

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

LGTM, but let's wait just a bit before merging this.

@jonasfj
Copy link
Member

jonasfj commented Sep 8, 2021

It's worth shipping even with unsafe-eval, because it points out something that can be fixed later :D

@kwalrath kwalrath merged commit ca0c7b6 into master Sep 8, 2021
@kwalrath kwalrath deleted the misc/add-some-security-headers branch September 8, 2021 23:00
atsansone added a commit to flutter/website that referenced this pull request Feb 21, 2024
The Flutter docs site scores a [C for
security](https://securityheaders.com/?q=https%3A%2F%2Fdocs.flutter.dev).
One factor is the Content Security Policy, an HTTP header that can
prevent [Cross Site Scripting
(XSS)](https://securityheaders.com/?q=https%3A%2F%2Fdocs.flutter.dev)
attacks.

This PR adds the CSP to the Flutter docs site HTTP headers. It would
resemble the fix applied to the Dart homepage in
[Dart PR #3540](dart-lang/site-www#3540) based
on [Dart issue #1521](dart-lang/site-www#1521)

This change is based on the pub.dev site. Fixes #6381

Co-authored-by: Brett Morgan <brettmorgan@google.com>
atsansone added a commit to atsansone/website that referenced this pull request Apr 5, 2024
The Flutter docs site scores a [C for
security](https://securityheaders.com/?q=https%3A%2F%2Fdocs.flutter.dev).
One factor is the Content Security Policy, an HTTP header that can
prevent [Cross Site Scripting
(XSS)](https://securityheaders.com/?q=https%3A%2F%2Fdocs.flutter.dev)
attacks.

This PR adds the CSP to the Flutter docs site HTTP headers. It would
resemble the fix applied to the Dart homepage in
[Dart PR flutter#3540](dart-lang/site-www#3540) based
on [Dart issue flutter#1521](dart-lang/site-www#1521)

This change is based on the pub.dev site. Fixes flutter#6381

Co-authored-by: Brett Morgan <brettmorgan@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Contributor has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply security headers
3 participants