Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion app/helpers/script_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,13 @@ def render_javascript_pack_once_tags(*names)
[
javascript_assets_tag(*@scripts),
javascript_polyfill_pack_tag,
javascript_include_tag(*sources, crossorigin: local_crossorigin_sources? ? true : nil),
*sources.map do |source|
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.

)
end,
],
)
end
Expand Down
16 changes: 14 additions & 2 deletions lib/asset_sources.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def get_sources(*names)
# See: app/javascript/packages/rails-i18n-webpack-plugin/extract-keys-webpack-plugin.js
regexp_locale_suffix = %r{\.(#{I18n.available_locales.join('|')})\.js$}

load_manifest if !manifest || !cache_manifest
load_manifest_if_needed

locale_sources, sources = names.flat_map do |name|
manifest&.dig('entrypoints', name, 'assets', 'js')
Expand All @@ -22,19 +22,31 @@ def get_sources(*names)
end

def get_assets(*names)
load_manifest if !manifest || !cache_manifest
load_manifest_if_needed

names.flat_map do |name|
manifest&.dig('entrypoints', name, 'assets')&.except('js')&.values&.flatten
end.uniq.compact
end

def get_integrity(path)
load_manifest_if_needed

manifest&.dig('integrity', path)
end

def load_manifest
self.manifest = begin
JSON.parse(File.read(manifest_path))
rescue JSON::ParserError, Errno::ENOENT
nil
end
end

private

def load_manifest_if_needed
load_manifest if !manifest || !cache_manifest
end
end
end
19 changes: 19 additions & 0 deletions spec/helpers/script_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,25 @@
)
end

context 'with script integrity available' do
before do
allow(AssetSources).to receive(:get_integrity).and_return(nil)
allow(AssetSources).to receive(:get_integrity).with('/application.js').
and_return('sha256-aztp/wpATyjXXpigZtP8ZP/9mUCHDMaL7OKFRbmnUIazQ9ehNmg4CD5Ljzym/TyA')
end

it 'adds integrity attribute' do
output = render_javascript_pack_once_tags

expect(output).to have_css(
"script[src^='/polyfill.js']:not([integrity]) ~ \
script[src^='/application.js'][integrity^='sha256-']",
count: 1,
visible: :all,
)
end
end

context 'local development crossorigin sources' do
let(:webpack_port) { '3035' }

Expand Down
20 changes: 20 additions & 0 deletions spec/lib/asset_sources_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@
]
}
}
},
"integrity": {
"vendor.js": "sha256-aztp/wpATyjXXpigZtP8ZP/9mUCHDMaL7OKFRbmnUIazQ9ehNmg4CD5Ljzym/TyA"
}
}
STR
Expand Down Expand Up @@ -136,6 +139,23 @@
end
end

describe '.get_integrity' do
let(:path) { 'vendor.js' }
subject(:integrity) { AssetSources.get_integrity(path) }

it 'returns the integrity hash' do
expect(integrity).to start_with('sha256-')
end

context 'a path which does not exist in the manifest' do
let(:path) { 'missing.js' }

it 'returns nil' do
expect(integrity).to be_nil
end
end
end

describe '.load_manifest' do
it 'sets the manifest' do
AssetSources.load_manifest
Expand Down
13 changes: 13 additions & 0 deletions webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,20 @@ module.exports = /** @type {import('webpack').Configuration} */ ({
: filename;
},
writeToDisk: true,
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-..."
  }
}

for (const [key, { src, integrity }] of Object.entries(manifest)) {
if (integrity) {
srcIntegrity[src] = integrity;
delete manifest[key];
}
}

manifest.integrity = srcIntegrity;
return manifest;
},
}),
new RailsI18nWebpackPlugin({
onMissingString(key, locale) {
Expand Down