Skip to content

Enable subresource integrity for JavaScript packs#6803

Merged
aduth merged 2 commits intomainfrom
aduth-sri
Aug 19, 2022
Merged

Enable subresource integrity for JavaScript packs#6803
aduth merged 2 commits intomainfrom
aduth-sri

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Aug 19, 2022

Why: For additional protection against script manipulation, particularly in combination with CDN.

See: https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity

Testing Instructions:

  1. NODE_ENV=production yarn build
  2. rails s
  3. Go to http://localhost:3000
  4. Observe:
    a. No errors in console
    b. JavaScript still works

**Why**: For additional protection against script manipulation, particularly in combination with CDN.

See: https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity

changelog: Improvements, Security, Enable subresource integrity for JavaScript
integrity: isProductionEnv,
output: 'manifest.json',
transform(manifest) {
const srcIntegrity = {};
Copy link
Contributor Author

@aduth aduth Aug 19, 2022

Choose a reason for hiding this comment

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

This changes the output from something like...

{
  "whatever.js": {
    "src": "/packs/whatever-u09n1lo.js",
    "integrity": "sha256-..."
  }
}

...to a more consumable...

{
  "integrity": {
    "/packs/whatever-u09n1lo.js": "sha256-..."
  }
}

@aduth aduth marked this pull request as ready for review August 19, 2022 16:06
Copy link
Contributor

@mitchellhenke mitchellhenke left a comment

Choose a reason for hiding this comment

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

👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼
👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼
👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼
👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼
👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼
👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼
👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼
👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼 👏🏼

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

javascript_include_tag(
source,
crossorigin: local_crossorigin_sources? ? true : nil,
integrity: AssetSources.get_integrity(source),
Copy link
Contributor

@zachmargolis zachmargolis Aug 19, 2022

Choose a reason for hiding this comment

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

just to check my understanding

  1. we fingerprint each asset in its filename so if the content changes, we'll have new URLs
  2. in a 50-50 deploy state, new boxes will look for new code
  3. the CDN should have cached old URLs, so it will servic old code for old boxes

so this should be pretty safe in our deploy setup

Copy link
Contributor Author

@aduth aduth Aug 19, 2022

Choose a reason for hiding this comment

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

Yeah, I chatted about this a bit with @mitchellhenke and I think it should be fine. Since the manifest includes both the fingerprinted JavaScript and the corresponding integrity value for that file, it should always be consistent with itself. We're not really changing anything with regards to the availability of old assets in the CDN, so I'm assuming that would continue to work as-is. And the machines reference their own local copy of the manifest, not one in the CDN.

@aduth
Copy link
Contributor Author

aduth commented Aug 19, 2022

CodeClimate seems to be stuck, so I'm going to admin merge to bypass.

@aduth aduth merged commit 59a0a19 into main Aug 19, 2022
@aduth aduth deleted the aduth-sri branch August 19, 2022 17:38
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