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

Accept additional attributes for assets to support SRI #436

Merged
merged 4 commits into from
Sep 25, 2019
Merged

Accept additional attributes for assets to support SRI #436

merged 4 commits into from
Sep 25, 2019

Conversation

ashudson23
Copy link
Contributor

@ashudson23 ashudson23 commented Sep 22, 2019

Summary

Potential fix for feature request in issue #344 which will add SRI integrity to be html tags before sending to client, if the value exists on the asset.

Test plan

Webpack

const SriPlugin = require('webpack-subresource-integrity');
const LoadablePlugin = require('@loadable/webpack-plugin');

module.exports = ({
  ...
  plugins: [
    ...
    new SriPlugin({
      hashFuncNames: ['sha256', 'sha384'],
    }),
    new LoadablePlugin(),
  ],
});

In the server renderer script

Also worth adding the crossorigin key (this functionality already existed and isn't being introduced by this PR)

chunkExtractor.getScriptTags({ crossorigin: 'anonymous' })

Start application

Confirm that during SSR run the tags have integrity key with correct value

<script
    async
    data-chunk="HelloWorld"
    src="/1.bundle.js"
    crossorigin="anonymous"
    integrity="sha256-mYmvbcbExknkCdg+X/j/8zsdpRcm29eJ3lpF4wfnl+4= sha384-gECBjN17ng81O/ueB9Dz8Vor95n36djp9M/oWAXg7+4qgCPArWkzSqlEchJlQK3j"
></script>

@ashudson23 ashudson23 changed the title Accept additional attributes for assets, and enabling SRI keys on scr… Accept additional attributes for assets to support SRI Sep 22, 2019
Copy link
Owner

@gregberge gregberge left a comment

Choose a reason for hiding this comment

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

It looks good but I don't get why we need to validate asset attributes.

@@ -26,6 +33,24 @@ class LoadablePlugin {
errorDetails: false,
timings: false,
})

// include extra asset keys if present
stats.assets = stats.assets.map((asset) => {
Copy link
Owner

Choose a reason for hiding this comment

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

I think we will be fine without that isn't it?

Copy link
Contributor Author

@ashudson23 ashudson23 Sep 24, 2019

Choose a reason for hiding this comment

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

Actually, you're absolute right! I was forcing loadable-stats.json to store integrity when it didn't need to be there.

assetToScriptTag(asset, extraProps),
assetToScriptTag({
...asset,
...this.getSriFromFileName(asset.filename),
Copy link
Owner

Choose a reason for hiding this comment

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

All of these is useless, the integrity should already be in asset. The only needed part is : getSriHtmlAttributes and all usage of it.

@gregberge gregberge merged commit 586ad0a into gregberge:master Sep 25, 2019
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.

2 participants