Add support for subresource integrity (#506) #570
Conversation
WalkthroughThis set of changes introduces Subresource Integrity (SRI) support to the asset pipeline, allowing cryptographic integrity hashes to be generated, configured, and included in asset tags. The update spans configuration, helper methods, manifest structure, documentation, and comprehensive test coverage for both code and configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RailsHelper
participant Manifest
participant Config
participant Browser
User->>RailsHelper: Calls javascript_pack_tag(...)
RailsHelper->>Config: Checks integrity settings
RailsHelper->>Manifest: Looks up asset (with/without integrity)
Manifest-->>RailsHelper: Returns asset path (+ integrity if enabled)
RailsHelper-->>User: Renders <script> tag (with integrity/crossorigin if enabled)
User->>Browser: Loads page with asset tags
Browser->>AssetServer: Requests asset
Browser->>Browser: Verifies integrity if attribute present
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thanks for this @panagiotisplytas Looking good on the first glance. Will be great to add some specs and confirm we see no regressions with the setting disabled and enabled integrity works as expected. You can reference #571 for changes to make one of the failing specs currently run, if it doesn't make it to the main branch before your changes |
There was a problem hiding this comment.
Looking good, only minor gripe is the defaulting of values scattered around. Would mean updates in few places if defaults were to ever change. Wonder if we could improve on that, though not a dealbreaker.
We're looking up keys from default config today, though only one-level deep 🤔
| @@ -55,6 +55,16 @@ default: &default | |||
| # SHAKAPACKER_ASSET_HOST will override both configurations. | |||
| # asset_host: custom-path | |||
|
|
|||
| # Utilizing webpack-subresource-integrity plugin, will generate integrity hashes for all entries in manifest.json | |||
| # https://github.com/waysact/webpack-subresource-integrity/tree/main/webpack-subresource-integrity | |||
| integrity: | |||
There was a problem hiding this comment.
note: this can be done as a follow-up, but should we consider shorthands such as integrity: false?
There was a problem hiding this comment.
Not sure i understand, can you elaborate a bit?
There was a problem hiding this comment.
so instead of this:
integrity:
disabled: false
you'd be able to do
integrity: false
There was a problem hiding this comment.
🤔 I can definitely try.
But just by brainstorming, it feels like we'd be adding more checks, for little to no benefit. I may be missing something obvious here.
I'll try to come back to this within a few days, if i'm still stuck on how to do it we can leave it as a follow up
| } | ||
| ``` | ||
|
|
||
| After: |
There was a problem hiding this comment.
question: should we just switch to always generating an object going forward? (either now or in the next major)
while that'd technically make the file a little bigger in all cases, we'll only be talking a few bytes and ultimately SRI should be used whenever possible...
We could do this as a follow up, but I'm thinking if so we could add a new migrate-now kind of option for having the manifest always generated with { src: ... }
There was a problem hiding this comment.
🤔 Eventually, yes, i think we should have the same kind of output in the manifest.json across all environments (+/- integrity hashes). For consistency, and also to make the implementation & tests of SRI (and other features like that ) a bit simpler.
Having said that, and looking at the webpack-assets-manifest -plugin i am not sure how can we achieve that. Tried playing a bit with customize function in the past, but it went comically bad 😅 .
I may be missing something obvious tho.
I'll be happy to try if you have any suggestion in mind .
panagiotisplytas
force-pushed
the
add-support-for-subresource-integrity
branch
from
2d5f54a to
1a9bd8d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
lib/install/config/shakapacker.yml (1)
58-67: Well-documented integrity configuration blockThe configuration block is:
- Well-documented with links to relevant resources
- Provides sensible defaults (disabled by default, SHA-384, anonymous CORS mode)
- Includes comments explaining each option's purpose and alternatives
This approach ensures that enabling SRI is opt-in and won't break existing implementations.
Consider supporting shorthand syntax as suggested in a previous review comment. This could be implemented as a follow-up PR:
integrity: enabled: false # Current verbose way # OR integrity: false # Potential shorthanddocs/subresource_integrity.md (1)
44-52: 🛠️ Refactor suggestionConfig sample uses a different key than the implementation
The sample shows
cross_origin_loading, whereasbase.jsexpectscross_origin.
Keeping both in sync avoids user confusion and support issues.Update the doc or the code so the key name is identical across the project.
🧹 Nitpick comments (9)
spec/shakapacker/test_app/config/shakapacker_integrity.yml (1)
1-20: Configuration looks good but file should end with a newlineThe integrity configuration structure properly provides defaults and environment-specific overrides, matching the pattern used throughout the project. The default configuration disables integrity, while production enables it with multiple hash functions.
This setup gives users flexibility to enable SRI only in environments where it's needed, with sensible defaults.
Add a newline at the end of file to fix YAMLlint errors:
cross_origin: "use-credentials" +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
package/environments/base.js (1)
110-124: SRI plugin disabled in non-production builds – confirm intent
enabled: isProductionmeans integrity hashes will be absent whenNODE_ENV≠ production even if the feature is turned on inshakapacker.yml.
That keeps dev builds fast but makes it harder to spot integrity-related regressions early.If you want hashes available in development too, consider:
- enabled: isProduction + enabled: trueAlternatively, document this behaviour so users understand why their dev manifest lacks
integrity.docs/subresource_integrity.md (1)
1-8: Minor Markdown polishBare URLs (lines 4 & 31) and list indentation (lines 7-8) violate common MD linters.
While harmless, wrapping the links and fixing indent improves rendered output.Example:
- More: https://developer.mozilla.org/... + [MDN – Subresource Integrity](https://developer.mozilla.org/...)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
4-4: Bare URL used
null(MD034, no-bare-urls)
7-7: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
spec/shakapacker/manifest_spec.rb (2)
43-48: Spec description typoThe example is labelled
#lookup_with_chunks!but callslookup_pack_with_chunks!.
Renaming the description prevents confusion when reading test output.- it "#lookup_with_chunks! returns an array of paths to the bundled bootstrap.js of type javascript" do + it "#lookup_pack_with_chunks! returns an array of paths to the bundled bootstrap.js of type javascript" do
6-101: High duplication between the two contextsThe “no integrity” and “with integrity” contexts duplicate 90 % of their expectations, differing only in return shape.
Extracting shared examples and parameterising by a small fixture would:
- reduce maintenance overhead,
- make intent clearer,
- keep the file size manageable.
Consider:
shared_examples "manifest lookups" do |prefix, expected_entry| it "returns the correct bootstrap path" do expect(Shakapacker.manifest.lookup!("#{prefix}bootstrap.js")).to eq expected_entry end ... endThen invoke with and without integrity.
spec/shakapacker/helper_spec.rb (4)
3-6: Avoid mutating framework modules directlyOpening up
ActionView::TestCase::Behaviorand adding an accessor every time the spec is evaluated could leak state across the global test process and surprise other specs that rely on the original behaviour. Prefer isolating the stub to the current example, e.g.before do view = ActionView::Base.new allow(view).to receive_message_chain(:controller, :request).and_return(fake_request) endor simply define
let(:request) { fake_request }and rely on regular helper instantiation.
This keeps Rails internals untouched and eliminates the risk of brittle cross-spec side-effects.
515-519: Rubocop: missing space after commaRuboCop flagged
Layout/SpaceAfterCommahere:- .map { |chunk| stylesheet_link_tag(chunk,crossorigin: "anonymous", integrity: "sha384-hash") } + .map { |chunk| stylesheet_link_tag(chunk, crossorigin: "anonymous", integrity: "sha384-hash") }Tidying this keeps the spec style-guide compliant and avoids future CI noise.
🧰 Tools
🪛 RuboCop (1.73)
[convention] 518-518: Space missing after comma.
(Layout/SpaceAfterComma)
227-244: Large duplication can be collapsed with shared examplesThe expectations for
stylesheet_pack_tag(and their SRI counterparts) are repeated almost verbatim. Extracting them intoshared_examples "stylesheet_pack_tag behaviour"and parameterising the integrity options would cut ~200 LOC and make future maintenance (e.g. adding a new option) far simpler.shared_examples "stylesheet_pack_tag behaviour" do |args, options = {}| it "generates the correct link tag" do expected = chunks.uniq.map { |c| stylesheet_link_tag(c, **options) }.join("\n") expect(stylesheet_pack_tag(*args, **options.slice(:media))).to eq expected end endReuse with
include_examplesin both contexts.
366-375: Whitespace-sensitive heredocs can mask real failuresThe
<<~HTMLtrick removes the minimal common indentation, which works here only because every line happens to share the same indent. If one of the generated lines gains an extra space the spec will silently start comparing" " + htmlto"html"and fail—even though the helper is still correct.
Consider normalising both sides before the assertion:expect(javascript_pack_tag(...).strip).to eq expected.stripor compare arrays of lines. This eliminates false negatives caused purely by indentation drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
CHANGELOG.md(1 hunks)docs/subresource_integrity.md(1 hunks)lib/install/config/shakapacker.yml(1 hunks)lib/shakapacker/configuration.rb(1 hunks)lib/shakapacker/helper.rb(3 hunks)lib/shakapacker/manifest.rb(1 hunks)package.json(2 hunks)package/config.js(1 hunks)package/environments/base.js(3 hunks)spec/dummy/package.json(1 hunks)spec/shakapacker/configuration_spec.rb(1 hunks)spec/shakapacker/helper_spec.rb(1 hunks)spec/shakapacker/manifest_spec.rb(1 hunks)spec/shakapacker/test_app/config/shakapacker_integrity.yml(1 hunks)spec/shakapacker/test_app/public/packs/manifest.json(2 hunks)test/package/config.test.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
lib/shakapacker/manifest.rb (1)
lib/shakapacker/configuration.rb (1)
data(107-109)
spec/shakapacker/manifest_spec.rb (3)
lib/shakapacker/configuration.rb (1)
manifest_path(55-61)lib/shakapacker/instance.rb (1)
manifest(37-39)lib/shakapacker/manifest.rb (4)
lookup!(48-50)lookup_pack_with_chunks!(31-33)lookup(41-45)lookup_pack_with_chunks(21-29)
lib/shakapacker/helper.rb (1)
lib/shakapacker/configuration.rb (1)
integrity(102-104)
spec/shakapacker/configuration_spec.rb (2)
spec/shakapacker/test_app/config/application.rb (1)
config(6-10)lib/shakapacker/configuration.rb (1)
integrity(102-104)
spec/shakapacker/helper_spec.rb (1)
lib/shakapacker/helper.rb (12)
asset_pack_path(16-18)asset_pack_url(27-29)image_pack_path(34-36)image_pack_url(42-44)image_pack_tag(55-63)favicon_pack_tag(71-73)preload_pack_asset(128-134)javascript_pack_tag(98-118)append_javascript_pack_tag(187-191)prepend_javascript_pack_tag(193-197)stylesheet_pack_tag(161-172)append_stylesheet_pack_tag(174-185)
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
12-12: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
docs/subresource_integrity.md
4-4: Bare URL used
null
(MD034, no-bare-urls)
7-7: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
31-31: Bare URL used
null
(MD034, no-bare-urls)
🪛 YAMLlint (1.35.1)
spec/shakapacker/test_app/config/shakapacker_integrity.yml
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
🪛 LanguageTool
docs/subresource_integrity.md
[uncategorized] ~38-~38: A comma might be missing here.
Context: ...hould be enabled by default. To enable it just add this in shakapacker.yml ```y...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🪛 RuboCop (1.73)
spec/shakapacker/helper_spec.rb
[convention] 518-518: Space missing after comma.
(Layout/SpaceAfterComma)
🔇 Additional comments (17)
spec/dummy/package.json (1)
33-33: Good addition of webpack-subresource-integrity packageThis is a good addition that enables the generation of integrity hashes for assets, which is necessary for implementing Subresource Integrity (SRI) support.
CHANGELOG.md (1)
11-12: Well-documented feature addition in changelogGood practice to document the SRI feature addition in the changelog with proper attribution to the contributor.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
12-12: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
lib/shakapacker/configuration.rb (1)
102-104: Clean implementation of integrity configuration methodThe
integritymethod follows the same pattern as other configuration accessors, providing a clean API to access SRI settings from the configuration file.package.json (1)
49-49: Correct addition of webpack-subresource-integrity to devDependenciesAdding the webpack-subresource-integrity package to devDependencies ensures it's available during development.
package/config.js (1)
53-54: Ensuring integrity configuration has unique hash functionsGood practice to deduplicate the hash functions array. This prevents redundant hashes from being calculated during the build process, which could impact performance.
spec/shakapacker/test_app/public/packs/manifest.json (2)
14-61: Test manifest correctly structured for SRI supportThe manifest now includes entries for assets with integrity hashes, using objects with both "src" and "integrity" fields. This structure is essential for the SRI feature.
Note that these are placeholder hashes ("sha384-hash") which should be sufficient for testing purposes. In a real build, the webpack-subresource-integrity plugin would generate actual cryptographic hashes.
105-176: Entrypoints correctly extended with integrity supportThe entrypoints section properly includes new entries with integrity support, maintaining the same structure as the individual assets. This ensures consistent handling in helper methods.
lib/shakapacker/manifest.rb (1)
70-75: Enhanced find method to handle nested integrity objectsThe updated
findmethod now correctly supports both the legacy string format and the new nested object format containing integrity hashes. This approach ensures backward compatibility while adding SRI support.The method now:
- Returns nil for missing or empty entries
- Returns values directly for string entries (legacy format)
- Extracts the "src" field from nested entries or returns the entire object
This helps the manifest lookup continue to work for both formats seamlessly.
spec/shakapacker/configuration_spec.rb (2)
185-209: Comprehensive tests for default integrity configurationGood test coverage for the new integrity configuration. Tests verify the existence of all required keys and their default values.
212-232: Tests for custom integrity configurationThese tests verify that custom integrity settings are correctly loaded from the configuration file. The test cases cover all configurable aspects of the integrity feature.
The tests ensure that:
- Integrity can be enabled
- Multiple hash functions can be configured
- The cross-origin attribute can be customized
test/package/config.test.js (2)
58-71: Default integrity settings tests look goodTests verify that the default configuration correctly sets:
- integrity disabled by default
- sha384 as the default hash function
- "anonymous" as the default crossorigin value
These tests establish the expected baseline for the integrity feature.
73-96: Override tests are comprehensiveTests verify that integrity settings can be overridden via a configuration file, including:
- enabling integrity
- configuring multiple hash functions
- setting a different crossorigin value
The alternating test and setup blocks improve readability by grouping related assertions.
lib/shakapacker/helper.rb (4)
112-117: Refactored to use centralized tag renderingThe refactoring makes good use of the new
render_tagshelper to generate JavaScript tags with different loading strategies (async, deferred, sync).
169-172: Simplified stylesheet tag renderingThe stylesheet rendering logic has been simplified to use the centralized
render_tagshelper, reducing code duplication.
244-250: Helper methods to extract source and integrity from manifestThese utility methods safely handle both string-based and hash-based manifest entries, providing a clean abstraction for extracting source and integrity information.
252-276: Well-structured SRI integrationThe
render_tagsmethod provides a clean implementation for generating tags with integrity attributes when enabled. The implementation:
- Handles both JavaScript and stylesheet tags
- Only applies integrity attributes when the feature is enabled
- Correctly extracts integrity hashes from manifest entries
- Applies the configured crossorigin attribute
- Maintains proper spacing between tags
The conditional approach ensures backward compatibility with existing manifests.
spec/shakapacker/manifest_spec.rb (1)
134-143: Expected path may be incorrectThe expectation for
bootstrap_with_integritypoints to/packs/bootstrap-…(without_with_integrity).
If the manifest actually stores the file under the suffixed name, this test will give a false positive/negative.Double-check the integrity manifest and adjust accordingly:
- expected = [{ "src" => "/packs/bootstrap-300631c4f0e0f9c865bc.js", "integrity" => "sha384-hash" }] + expected = [{ "src" => "/packs/bootstrap_with_integrity-300631c4f0e0f9c865bc.js", "integrity" => "sha384-hash" }]
| // This is required for SRI to work. | ||
| crossOriginLoading: config.integrity.enabled | ||
| ? config.integrity.cross_origin | ||
| : false |
There was a problem hiding this comment.
Config key mismatch will disable crossOriginLoading at runtime
crossOriginLoading is set from config.integrity.cross_origin, but the public docs (and the sample in docs/subresource_integrity.md) use cross_origin_loading.
Unless both keys are declared in configuration.rb, this will resolve to undefined, causing Webpack to emit crossOriginLoading: false and breaking SRI in production.
- crossOriginLoading: config.integrity.enabled
- ? config.integrity.cross_origin
+ crossOriginLoading: config.integrity.enabled
+ ? config.integrity.cross_origin_loading
: falsePlease align the key name here (or in the docs/config) to avoid silent mis-configuration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // This is required for SRI to work. | |
| crossOriginLoading: config.integrity.enabled | |
| ? config.integrity.cross_origin | |
| : false | |
| // This is required for SRI to work. | |
| crossOriginLoading: config.integrity.enabled | |
| ? config.integrity.cross_origin_loading | |
| : false |
There was a problem hiding this comment.
This was named intentionally like that to cover both crossOriginLoading of the plugin and crossorigin html script tag attribute. @G-Rath any suggestions here? Shall i ignore it?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
panagiotisplytas
force-pushed
the
add-support-for-subresource-integrity
branch
from
1a9bd8d to
c72a0fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
spec/shakapacker/helper_spec.rb (1)
366-375: 🛠️ Refactor suggestionTest vulnerability: Hardcoded integrity hash.
The test uses a hardcoded "sha384-hash" value which makes the test fragile. If the actual hash generation changes, the test will fail even though the functionality works correctly.
Consider using a pattern match instead of an exact string match:
- expect(javascript_pack_tag("application_with_integrity", "bootstrap_with_integrity")).to eq expected + expect(javascript_pack_tag("application_with_integrity", "bootstrap_with_integrity")).to match(/integrity="sha384-[A-Za-z0-9+\/=]{64}"/)Alternatively, you could just verify the presence of the integrity attribute without checking its exact value.
🧹 Nitpick comments (3)
spec/shakapacker/test_app/config/shakapacker_integrity.yml (1)
20-20: Add newline at end of file.The file is missing a newline at the end, which is a common best practice.
cross_origin: "use-credentials" +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
docs/subresource_integrity.md (2)
6-9: Important notes section is informative.These notes highlight key considerations when using SRI:
- The implications of modifying files after hash generation
- The changes to manifest.json structure
The indentation should be consistent with Markdown best practices.
-## Important notes - - If you somehow modify the file after the hash was generated, it will automatically be considered as tampered, and the browser will not allow it to be executed. - - Enabling subresource integrity generation, will change the structure of `manifest.json`. Keep that in mind if you utilize this file in any other custom implementation. +## Important notes +- If you somehow modify the file after the hash was generated, it will automatically be considered as tampered, and the browser will not allow it to be executed. +- Enabling subresource integrity generation, will change the structure of `manifest.json`. Keep that in mind if you utilize this file in any other custom implementation.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
38-42: Add a comma after "To enable it".Minor grammar improvement for readability.
-To enable it just add this in `shakapacker.yml` +To enable it, just add this in `shakapacker.yml`🧰 Tools
🪛 LanguageTool
[uncategorized] ~38-~38: A comma might be missing here.
Context: ...hould be enabled by default. To enable it just add this inshakapacker.yml```y...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
CHANGELOG.md(1 hunks)docs/subresource_integrity.md(1 hunks)lib/install/config/shakapacker.yml(1 hunks)lib/shakapacker/configuration.rb(1 hunks)lib/shakapacker/helper.rb(3 hunks)lib/shakapacker/manifest.rb(1 hunks)package.json(3 hunks)package/config.js(1 hunks)package/environments/base.js(3 hunks)spec/dummy/package.json(1 hunks)spec/shakapacker/configuration_spec.rb(1 hunks)spec/shakapacker/helper_spec.rb(1 hunks)spec/shakapacker/manifest_spec.rb(1 hunks)spec/shakapacker/test_app/config/shakapacker_integrity.yml(1 hunks)spec/shakapacker/test_app/public/packs/manifest.json(2 hunks)test/package/config.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- package/config.js
- lib/shakapacker/configuration.rb
- package.json
- spec/dummy/package.json
- lib/shakapacker/helper.rb
- spec/shakapacker/test_app/public/packs/manifest.json
- package/environments/base.js
- lib/install/config/shakapacker.yml
- lib/shakapacker/manifest.rb
- spec/shakapacker/configuration_spec.rb
- test/package/config.test.js
- spec/shakapacker/manifest_spec.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
spec/shakapacker/helper_spec.rb (1)
lib/shakapacker/helper.rb (12)
asset_pack_path(16-18)asset_pack_url(27-29)image_pack_path(34-36)image_pack_url(42-44)image_pack_tag(55-63)favicon_pack_tag(71-73)preload_pack_asset(128-134)javascript_pack_tag(98-118)append_javascript_pack_tag(187-191)prepend_javascript_pack_tag(193-197)stylesheet_pack_tag(161-172)append_stylesheet_pack_tag(174-185)
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
12-12: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
docs/subresource_integrity.md
7-7: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🪛 LanguageTool
docs/subresource_integrity.md
[uncategorized] ~38-~38: A comma might be missing here.
Context: ...hould be enabled by default. To enable it just add this in shakapacker.yml ```y...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
🪛 YAMLlint (1.35.1)
spec/shakapacker/test_app/config/shakapacker_integrity.yml
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (10)
CHANGELOG.md (1)
11-12: Changelog entry looks good.The entry follows the existing format in the changelog, clearly indicating the addition of subresource integrity support and correctly citing the PR and contributor.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
12-12: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
spec/shakapacker/test_app/config/shakapacker_integrity.yml (2)
3-7: Good configuration structure with appropriate defaults.The default configuration provides sensible starting values:
- Integrity disabled by default (backwards compatibility)
- SHA-384 hash function (good security/performance balance)
- "anonymous" cross-origin setting (most common use case)
17-20: Production configuration looks good.Enabling integrity in production with multiple hash functions (sha256, sha384, sha512) provides strong security. Using "use-credentials" for cross-origin is appropriate when credentials need to be sent with requests.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
docs/subresource_integrity.md (3)
1-4: Good introduction to SRI concept.The documentation provides a clear, concise explanation of what Subresource Integrity is and links to MDN for more detailed information.
17-25: Good example of manifest.json structure change.The before/after examples clearly illustrate how enabling SRI changes the manifest.json format from simple key-value pairs to nested objects with integrity hashes.
54-55: Future-proofing documentation suggestion.The documentation mentions that SRI is disabled by default for backward compatibility but might change in the future. This is good forward-thinking to prepare users for potential changes.
spec/shakapacker/helper_spec.rb (4)
7-290: Well-structured tests for standard behavior (without integrity).The test suite provides comprehensive coverage of all helper methods without integrity hashes enabled, establishing a good baseline for comparison. The tests cover asset paths, URLs, image tags, favicon tags, JavaScript tags with different options (defer, async), and stylesheet tags.
293-313: Good test setup for integrity feature.The test setup correctly mocks the integrity configuration to enable testing of the feature. The approach of having a separate context for integrity tests makes the test structure clear and maintainable.
515-531: Comprehensive stylesheet integrity testing.The tests for stylesheet_pack_tag with integrity hashes cover various usage patterns including string arguments, symbol arguments, and mixed arguments. This ensures the feature works correctly across all use cases.
541-553: Good multiple invocation testing.The tests verify that multiple invocations of stylesheet_pack_tag work correctly with integrity hashes, which is important for ensuring compatibility with existing usage patterns.
panagiotisplytas
force-pushed
the
add-support-for-subresource-integrity
branch
from
c72a0fa to
9bfc3a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
spec/shakapacker/manifest_spec.rb (1)
135-136: Test becomes fragile by hard-coding a dummy hash.Using a static "sha384-hash" value may cause the test to break if the actual hash generation logic changes. Consider asserting on the presence and format of the attribute rather than an exact value.
- expected = [{ "src" => "/packs/bootstrap-300631c4f0e0f9c865bc.js", "integrity" => "sha384-hash" }] + expected = [{ "src" => "/packs/bootstrap-300631c4f0e0f9c865bc.js", "integrity" => /^sha384-/ }]And then use a matching expectation:
expect(actual.first["src"]).to eq expected.first["src"] expect(actual.first["integrity"]).to match expected.first["integrity"]spec/shakapacker/helper_spec.rb (1)
367-372: Test becomes fragile by hard-coding a dummy hash.Using a static "sha384-hash" value in HTML expectations may cause the test to break if the hash generation logic changes. Consider asserting on the presence and format rather than an exact value.
One approach would be to split the HTML checking:
rendered_tags = javascript_pack_tag("application_with_integrity", "bootstrap_with_integrity") expect(rendered_tags).to include('defer="defer"') expect(rendered_tags).to include('crossorigin="anonymous"') expect(rendered_tags).to match(/integrity="sha384-[A-Za-z0-9+\/=]+"/i)This would make the tests more resilient to changes in the actual hash values.
🧹 Nitpick comments (1)
CHANGELOG.md (1)
11-12: LGTM, but consider using asterisks for list items for consistency.The added changelog entry correctly documents the new Subresource Integrity (SRI) support. For consistency with the rest of the document though, consider using asterisks instead of dashes for list items.
-### Added -- Support for subresource integrity. [PR 570](https://github.com/shakacode/shakapacker/pull/570) by [panagiotisplytas](https://github.com/panagiotisplytas) +### Added +* Support for subresource integrity. [PR 570](https://github.com/shakacode/shakapacker/pull/570) by [panagiotisplytas](https://github.com/panagiotisplytas)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
12-12: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (2)
spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
CHANGELOG.md(1 hunks)docs/subresource_integrity.md(1 hunks)lib/install/config/shakapacker.yml(1 hunks)lib/shakapacker/configuration.rb(1 hunks)lib/shakapacker/helper.rb(3 hunks)lib/shakapacker/manifest.rb(1 hunks)package.json(3 hunks)package/config.js(1 hunks)package/environments/base.js(3 hunks)spec/dummy/package.json(1 hunks)spec/shakapacker/configuration_spec.rb(1 hunks)spec/shakapacker/helper_spec.rb(1 hunks)spec/shakapacker/manifest_spec.rb(1 hunks)spec/shakapacker/test_app/config/shakapacker_integrity.yml(1 hunks)spec/shakapacker/test_app/public/packs/manifest.json(2 hunks)test/package/config.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- spec/dummy/package.json
- package/config.js
- package.json
- lib/shakapacker/configuration.rb
- spec/shakapacker/test_app/public/packs/manifest.json
- lib/shakapacker/manifest.rb
- spec/shakapacker/configuration_spec.rb
- lib/shakapacker/helper.rb
- package/environments/base.js
- spec/shakapacker/test_app/config/shakapacker_integrity.yml
- lib/install/config/shakapacker.yml
- docs/subresource_integrity.md
- test/package/config.test.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
12-12: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🔇 Additional comments (5)
spec/shakapacker/manifest_spec.rb (2)
6-101: Well-structured context separation for testing with and without integrity.Nice job organizing the test suite to maintain all existing functionality while adding tests for the new integrity hash support. This approach ensures backward compatibility and proper testing of the new feature.
103-207: Tests appropriately verify manifest support for integrity hashes.The tests thoroughly verify the new functionality added to the Manifest class, ensuring it correctly returns both the source path and integrity hash when available. The parallel test structure is excellent for maintaining feature parity.
spec/shakapacker/helper_spec.rb (3)
7-291: Well-structured tests for the original behavior without integrity.The reorganization of tests into separate contexts maintains full coverage of the original functionality while preparing for the new integrity-related features. This approach ensures that the helper methods continue to work as expected in both scenarios.
312-313: Good configuration mock for integrity testing.This effectively sets up the test environment to simulate SRI being enabled with anonymous cross-origin setting, which is essential for properly testing the integrity functionality.
293-579: Comprehensive tests for integrity-enabled helpers.The tests thoroughly verify that all helper methods correctly include integrity and crossorigin attributes when integrity is enabled. The parallel structure with the non-integrity tests ensures feature parity and complete coverage.
panagiotisplytas
force-pushed
the
add-support-for-subresource-integrity
branch
from
9bfc3a7 to
72455f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
spec/shakapacker/helper_spec.rb (1)
312-313: Consistent with previous discussion on hardcoded test values.The use of hardcoded integrity hash values (
"sha384-hash") aligns with the previous discussion about testing security-critical features. While this makes tests potentially brittle to hash format changes, it ensures explicit verification of the exact integrity implementation.
🧹 Nitpick comments (2)
CHANGELOG.md (1)
14-14: Fix markdown list formatting for consistency.The static analysis tool correctly identified an inconsistent list style. The changelog uses asterisks for list items elsewhere, but this line uses a dash.
- - Support for subresource integrity. [PR 570](https://github.com/shakacode/shakapacker/pull/570) by [panagiotisplytas](https://github.com/panagiotisplytas) + * Support for subresource integrity. [PR 570](https://github.com/shakacode/shakapacker/pull/570) by [panagiotisplytas](https://github.com/panagiotisplytas)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
14-14: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
spec/shakapacker/helper_spec.rb (1)
7-291: Consider reducing code duplication between test contexts.The comprehensive test coverage for both integrity scenarios is excellent, but the near-complete duplication between the two contexts creates maintenance burden. Consider these approaches to reduce duplication:
Option 1: Shared examples
shared_examples "asset helper behavior" do |integrity_enabled| let(:suffix) { integrity_enabled ? "_with_integrity" : "" } let(:expected_attrs) { integrity_enabled ? { crossorigin: "anonymous", integrity: "sha384-hash" } : {} } it "#asset_pack_path generates the correct path" do expect(asset_pack_path("bootstrap#{suffix}.js")).to eq "/packs/bootstrap#{suffix}-300631c4f0e0f9c865bc.js" end # ... other shared tests end context "without integrity hashes" do include_examples "asset helper behavior", false end context "with integrity hashes" do include_examples "asset helper behavior", true endOption 2: Parameterized tests
[false, true].each do |integrity_enabled| context integrity_enabled ? "with integrity hashes" : "without integrity hashes" do # Single set of tests with conditional expectations end endThis would maintain the same test coverage while reducing maintenance overhead.
Also applies to: 293-579
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
CHANGELOG.md(1 hunks)docs/subresource_integrity.md(1 hunks)lib/install/config/shakapacker.yml(1 hunks)lib/shakapacker/configuration.rb(1 hunks)lib/shakapacker/helper.rb(3 hunks)lib/shakapacker/manifest.rb(1 hunks)package.json(3 hunks)package/config.js(1 hunks)package/environments/base.js(3 hunks)spec/dummy/package.json(1 hunks)spec/shakapacker/configuration_spec.rb(1 hunks)spec/shakapacker/helper_spec.rb(1 hunks)spec/shakapacker/manifest_spec.rb(1 hunks)spec/shakapacker/test_app/config/shakapacker_integrity.yml(1 hunks)spec/shakapacker/test_app/public/packs/manifest.json(2 hunks)spec/spec_helper.rb(2 hunks)spec/support/html_test_helpers.rb(1 hunks)test/package/config.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- spec/dummy/package.json
- lib/shakapacker/configuration.rb
- package.json
- package/config.js
- lib/shakapacker/manifest.rb
- spec/shakapacker/test_app/config/shakapacker_integrity.yml
- package/environments/base.js
- test/package/config.test.js
- spec/shakapacker/configuration_spec.rb
- lib/install/config/shakapacker.yml
- lib/shakapacker/helper.rb
- spec/shakapacker/test_app/public/packs/manifest.json
- docs/subresource_integrity.md
- spec/shakapacker/manifest_spec.rb
🧰 Additional context used
🧬 Code Graph Analysis (1)
spec/shakapacker/helper_spec.rb (2)
lib/shakapacker/helper.rb (12)
asset_pack_path(16-18)asset_pack_url(27-29)image_pack_path(34-36)image_pack_url(42-44)image_pack_tag(55-63)favicon_pack_tag(71-73)preload_pack_asset(128-134)javascript_pack_tag(98-118)append_javascript_pack_tag(187-191)prepend_javascript_pack_tag(193-197)stylesheet_pack_tag(161-172)append_stylesheet_pack_tag(174-185)spec/support/html_test_helpers.rb (2)
expect_script_tags_match(10-37)expect_stylesheet_link_tags_match(45-72)
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
14-14: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🔇 Additional comments (4)
spec/spec_helper.rb (1)
2-2: LGTM! Proper integration of test helpers.The addition of the HtmlTestHelpers module follows standard RSpec conventions for including shared test utilities.
Also applies to: 50-50
spec/support/html_test_helpers.rb (1)
1-73: Excellent test utility implementation.This helper module provides robust semantic comparison of HTML tags, which is essential for testing SRI attributes where attribute order shouldn't matter. Key strengths:
- Uses Nokogiri for proper HTML parsing
- Ignores attribute order by sorting before comparison
- Provides detailed failure messages with actual vs expected output
- Handles exceptions gracefully with debugging context
- Clear documentation and method signatures
This will significantly improve the reliability and maintainability of HTML tag tests.
spec/shakapacker/helper_spec.rb (2)
374-375: Good use of semantic HTML comparison helpers.The tests properly utilize the new
expect_script_tags_matchandexpect_stylesheet_link_tags_matchhelpers, which is essential for testing SRI attributes where attribute order shouldn't matter. This approach makes the tests more robust and provides better failure diagnostics.Also applies to: 385-386, 398-399, 413-414, 445-446, 455-456, 475-476, 486-487, 499-500, 512-513, 521-522, 530-531, 538-539, 545-548, 558-560, 566-568, 575-577
367-372:✅ Verification successful
Verify test asset names match manifest fixtures.
Ensure that the test asset names like
"application_with_integrity"and"bootstrap_with_integrity"have corresponding entries in the test manifest fixtures with the expected integrity hashes.Also applies to: 378-383, 391-396, 405-411, 439-443, 449-453, 469-473, 480-484, 492-497, 505-510
🏁 Script executed:
#!/bin/bash # Description: Verify that test assets used in specs exist in manifest fixtures # Search for test assets with integrity suffixes in manifest files echo "Checking for test assets in manifest files..." fd "manifest.json" spec/ --exec cat {} \; | jq -r 'keys[]' | grep -E "(application_with_integrity|bootstrap_with_integrity|hello_stimulus_with_integrity|main_with_integrity)" | sort -u echo -e "\nChecking specific asset entries in manifests..." fd "manifest.json" spec/ --exec sh -c 'echo "=== $1 ==="; cat "$1" | jq .' _ {} \;Length of output: 6307
All test assets have matching manifest entries with integrity hashes
I verified that
spec/shakapacker/test_app/public/packs/manifest.jsonincludes root-level entries for:
- application_with_integrity.{js,css,png}
- bootstrap_with_integrity.{js,css}
- main_with_integrity.js
…and that the
entrypointssection defineshello_stimulus_with_integrity(and the others) with the expectedsrcandintegrityfields.No updates to the fixtures or spec are needed.
Summary
javascript_pack_tagandstylesheet_pack_taghelpers work, so they can support integrity hashesManifest.findmethod, so it can handle the newmanifest.jsonformatPull Request checklist
Summary by CodeRabbit
New Features
Documentation
Chores
webpack-subresource-integrityplugin.Tests