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 Subresource Integrity for JS and CSS files #9933

Merged
merged 4 commits into from Jul 3, 2018

Conversation

Projects
None yet
3 participants
@morloy
Contributor

morloy commented Jun 3, 2018

After moving our Meteor assets out to a CDN, the Mozilla Observatory kept complaining about missing Subresouce Integrity in our JavaScript files.
This PR adds a sha512 hash to all basic Meteor files served to the client.

Solves meteor/meteor-feature-requests#35.

@benjamn benjamn added this to the Release 1.7.1 milestone Jun 13, 2018

if (item.sha512) {
itemObj.hash = item.sha512;
} else {
const hash = createHash('sha512');

This comment has been minimized.

@abernix

abernix Jun 14, 2018

Member

If we were to update tools/isobuild/bundler.js, here to also generate and store a sha512 into the manifest (thus ensuring that item.sha512 is defined above) — what would be the use-case for needing to generate this hash at run-time (on each request), as would be occurring here?

(It seems like this would be the way to go, but I'm curious if I'm missing something.)

This comment has been minimized.

@morloy

morloy Jun 15, 2018

Contributor

As explained below, using SHA-512 for the hash in the manifest and truncating it for filenames would be a good approach in my opinion.

@@ -19,7 +20,8 @@ export async function generateHTMLForArch(arch, includeHead) {
cacheable: true,
url: '/packages/templating-runtime.js?hash=c18de19afda6e9f0db7faf3d4382a4c953cabe18&v="1"',
size: 24132,
hash: 'c18de19afda6e9f0db7faf3d4382a4c953cabe18'
hash: 'c18de19afda6e9f0db7faf3d4382a4c953cabe18',
sha512: 'mJ9OEOi4HbVMzqiVGOjxCiIIImgnzEg6QhorPno46yULMu/3tKmENYriasWP9RrM+wLNwlTzw6+9d+nWY9/A0A==',

This comment has been minimized.

@abernix

abernix Jun 14, 2018

Member

Perhaps using sri would be a more appropriate name here, since the suggested algorithm might update over time? (And has already changed since when we first started discussing this — for example, SHA256 was no longer recommended by the NSA, if I recall)

@@ -19,7 +20,8 @@ export async function generateHTMLForArch(arch, includeHead) {
cacheable: true,
url: '/packages/templating-runtime.js?hash=c18de19afda6e9f0db7faf3d4382a4c953cabe18&v="1"',
size: 24132,
hash: 'c18de19afda6e9f0db7faf3d4382a4c953cabe18'
hash: 'c18de19afda6e9f0db7faf3d4382a4c953cabe18',

This comment has been minimized.

@abernix

abernix Jun 14, 2018

Member

I had been tempted (both today and in the past) to suggest that we switch to using a newer cipher for hash (which currently uses SHA-1, 40-bytes in length — in hex) and just use that for the SRI — relying on only a single hash! 🙌🤔

That temptation was originally based on me thinking we could go with SHA-256, which has a relatively short hexadecimal representation (64-bytes) and would still work fine as a filename (which hash is frequently used for in builds and when serving files to clients!).

Unfortunately, as hashes become more complex, they become longer and the hex representation of a SHA-512 hash would be 128-bytes — a super long filename. (Maybe too long?)

Most problematically though, SRI requires base64 encoding which, while shorter than 128-bytes for SHA-512, fails to be useful as a filename because of the base64 encoding (/, in particular), so my temptation seems to be well-prevented.

This comment has been minimized.

@morloy

morloy Jun 15, 2018

Contributor

I‘d recommend switching to SHA-512 and simply truncate it for the filenames. This seems to be totally fine. If more information needs to go into the filename, there’s Base58 or UNMISTAKABLE_CHARS, that would give Base55.

Recoding the hash from hex to Base64 for SRI would be trivial.

@@ -9,8 +9,9 @@ export const headTemplate = ({
}) => {
var headSections = head.split(/<meteor-bundled-css[^<>]*>/, 2);
var cssBundle = [...(css || []).map(file =>
template(' <link rel="stylesheet" type="text/css" class="__meteor-css__" href="<%- href %>">')({
template(' <link rel="stylesheet" type="text/css" class="__meteor-css__" href="<%- href %>" integrity="sha512-<%- hash %>" crossorigin="anonymous">')({

This comment has been minimized.

@abernix

abernix Jun 14, 2018

Member

I'm curious if this crossorigin="anonymous" might potentially cause problems for existing deployments which might necessitate the presence of user credentials (as defined here; e.g. HTTP Auth, Cookies, etc.) in order to serve the JS or CSS files (let's say the Meteor's deployment was fully behind that forced HTTP Authentication).

From what I understand, it's mandatory for crossorigin to be set for SRI to work (See 0, 1), but I'm afraid this might need to be toggleable between the valid options, anonymous and use-credentials.

Any thoughts?

This comment has been minimized.

@morloy

morloy Jun 15, 2018

Contributor

Having this configurable sounds good to me. But where would this config go ideally? Any ideas?

@@ -54,8 +55,9 @@ export const closeTemplate = ({
'',
...(js || []).map(file =>
template(' <script type="text/javascript" src="<%- src %>"></script>')({
template(' <script type="text/javascript" src="<%- src %>" integrity="sha512-<%- hash %>" crossorigin="anonymous"></script>')({

This comment has been minimized.

@abernix

abernix Jun 14, 2018

Member

Same question as above!

morloy added some commits Jun 26, 2018

@morloy

This comment has been minimized.

Contributor

morloy commented Jun 28, 2018

@abernix I added WebAppInternals.enableSubresourceIntegrity(use_credentials = false) to make the whole SRI optional and allow to select either anonymous or use-credentials for the crossorigin.
By default, SRI is disabled to make sure that we don’t break any existing setups. Hope this fine now.

@benjamn

benjamn approved these changes Jul 3, 2018

@benjamn

benjamn approved these changes Jul 3, 2018

@abernix

abernix approved these changes Jul 3, 2018

LGTM!

@benjamn benjamn merged commit ef3af68 into meteor:devel Jul 3, 2018

18 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci: Clean Up Your tests passed on CircleCI!
Details
ci/circleci: Docs Your tests passed on CircleCI!
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Isolated Tests Your tests passed on CircleCI!
Details
ci/circleci: Test Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 10 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 7 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 8 Your tests passed on CircleCI!
Details
ci/circleci: Test Group 9 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

benjamn added a commit that referenced this pull request Jul 3, 2018

Improve File hashing logic in bundler.js.
Follow-up to #9933.

As recommended by @abernix, the sha1 hash of every file is now computed
from the file's sha512 hash, so we don't have to hash the entire contents
of the file twice with two different algorithms.

Other changes/improvements:

* Invalidate the hashes when/if `File#setContents` is called.

* Ignore `options.hash` and just compute hashes from actual file contents.
  Disagreement here would be worse than any performance benefits from
  precomputing the hash.

benjamn added a commit that referenced this pull request Jul 3, 2018

Improve File hashing logic in bundler.js. (#10050)
Follow-up to #9933.

As recommended by @abernix, the sha1 hash of every file is now computed
from the file's sha512 hash, so we don't have to hash the entire contents
of the file twice with two different algorithms.

Other changes/improvements:

* Invalidate the hashes when/if `File#setContents` is called.

* Ignore `options.hash` and just compute hashes from actual file contents.
  Disagreement here would be worse than any performance benefits from
  precomputing the hash.

benjamn added a commit that referenced this pull request Jul 3, 2018

Bump minor versions of webapp and boilerplate-generator packages.
Since recent changes in these packages (#9933) depend on changes to the
build tool, it seems wise to tie these updates to Meteor 1.7.1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment