Skip to content
This repository has been archived by the owner before Nov 9, 2022. It is now read-only.

Update Guide to make Helmet the official recommendation for HTTP Headers #750

Merged
merged 13 commits into from Apr 14, 2020

Conversation

toinevk
Copy link
Contributor

@toinevk toinevk commented Mar 3, 2018

This PR updates the Guide to make Helmet the official recommendation for HTTP Headers, in addition the guide provides recommend config for two headers.

TODOs:

  • Update CHANGELOG.md
  • Use <h2 id="foo"> instead of ## Foo for headers
  • Leave a blank line after each header

toinevk added 5 commits Mar 3, 2018
Add official recommendation for Helmet including information on HTTP
headers Meteor recommends.
Update mobile sector to reccomend Helmet.
Mention the new section and the change to the mobile section.
@apollo-cla
Copy link

apollo-cla commented Mar 3, 2018

@toinevk: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

toinevk added 3 commits Mar 3, 2018
Final tweaks
# Conflicts:
#	content/security.md
Fix deleted content
@hwillson
Copy link
Contributor

hwillson commented Mar 3, 2018

Thanks @toinevk! I haven't had a chance to review this yet, but will shortly.

Reference issue: meteor/meteor#9689

@@ -147,7 +147,7 @@ You can see that this Method does a _very specific thing_ - it just makes a sing
However, this doesn't mean you can't have any flexibility in your Methods. Let's look at an example:

```js
const Meteor.users.methods.setUserData = new ValidatedMethod({
Copy link
Member

@abernix abernix Apr 27, 2018

Choose a reason for hiding this comment

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

🤦‍♂️

Copy link
Member

@abernix abernix Apr 27, 2018

Choose a reason for hiding this comment

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

(Thanks for fixing this)

imgSrc: ["'self'"],
styleSrc: ["'self'", "'unsafe-inline'"],
},
browserSniff: false
Copy link
Member

@abernix abernix Apr 27, 2018

Choose a reason for hiding this comment

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

@dhrubins If you have a moment, do you think you could share your (likely more experienced) CSP eyes 👀 with this ruleset recommendation?

Copy link
Contributor Author

@toinevk toinevk Apr 27, 2018

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Yeah, I agree that the default policy there is a reasonable baseline, but unfortunately CSP can get complex very quickly, so perhaps there could also be some links to CSP tools to help folks through the tough parts. Here are a few that helped me: Google's CSP Evaluator, Report-URI's CSP Builder, CSP documentation from Mozilla, CSPValidator.org. Plus, the awesome SecurityHeaders.com since helmet also helps you set other headers aside from CSP.

One Meteor specific challenge we had @LegalRobot was with removing unsafe-inline, which seems like a reasonable first step from the baseline in this PR. We switched to a mixture of script hashes and origins in our script-src directive, but we quickly found that this causes the Meteor runtime config script to get blocked (I don't remember if it was the lack of unsafe-inline, or the CSP rule that once you use a hash that everything needs a hash). Anyway, it effectively blocks Meteor from loading anything to the client after that. The only way we've figured out around this is to take the hash of the runtime config upon startup (and we needed it in base64 so we used node's native crypto module rather than the Meteor sha package, as is standard in Meteor core):

import crypto from 'crypto'
import { Autoupdate } from 'meteor/autoupdate'

Meteor.startup(() => {
  const runtimeConfig = Object.assign(__meteor_runtime_config__, Autoupdate)
  runtimeConfig.accountsConfigCalled = true
  const runtimeConfigHash = crypto.createHash('sha256').update(`__meteor_runtime_config__ = JSON.parse(decodeURIComponent("${encodeURIComponent(JSON.stringify(runtimeConfig))}"))`).digest('base64')
  ...
})

Now our helmet config for the script-src directive includes 'sha256-${runtimeConfigHash}', which just feels kinda hack-y (but, hey, it works!). We also found that we had to run this in the startup callback, rather than during the build.

We also had to conditionally disable some directives when running locally. I'm documenting our whole helmet config line by line and will post it shortly to hopefully save some other folks some pain when implementing the same.

Choose a reason for hiding this comment

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

Here's a gist with our settings and how we use helmet: https://gist.github.com/dhrubins/31248506873a006ba5e6b459a81855c2

Writeup coming soon.

Copy link
Contributor Author

@toinevk toinevk May 2, 2018

Choose a reason for hiding this comment

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

Thanks for that input @dhrubins! Some really interesting insights there. I have updated based on feedback and added your gist in! Great example of how to get it functioning in Meteor. I do wonder whether there is way to load Meteor settings without calling a script...

toinevk added 2 commits May 2, 2018
List useful tools for further developing CSP including LegalRobot
Production CSP settings.
Clarify Legal Robot example
@toinevk
Copy link
Contributor Author

toinevk commented Oct 19, 2018

@abernix Are you happy to merge? We can then move to deprecate the browser policy packages.

@CLAassistant
Copy link

CLAassistant commented Oct 7, 2019

CLA assistant check
All committers have signed the CLA.

@filipenevola filipenevola merged commit 8b5aed4 into meteor:master Apr 14, 2020
1 check passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants