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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Emit build error when external resources are detected without integrity hashes #10

Merged
merged 21 commits into from
Feb 19, 2024
Merged

Emit build error when external resources are detected without integrity hashes #10

merged 21 commits into from
Feb 19, 2024

Conversation

evoactivity
Copy link
Contributor

@evoactivity evoactivity commented Feb 17, 2024 •
edited
Loading

Results in the following error being thown

image 
馃毃馃毃 The following external resources do not have an integrity hash:

<script src="https://ajax.googleapis.com/ajax/libs/jquery/3.7.1/jquery.min.js"></script>
<link rel="stylesheet" href="https://rsms.me/inter/inter.css" media="print" onload="this.media='all'">

You should generate an integrity hash for these files and apply it to the <script> or <link> tag in your index.html file.

Learn more about generating an integrity hash here:
https://developer.mozilla.org/en-US/docs/Web/Security/Subresource_Integrity#tools_for_generating_sri_hashes

I also have a version which does do the hashing for you, just to see what that looks like
evoactivity#1

@evoactivity
- error on external resources missing an integrity hash
59e8204 
- build test apps as before the 'Embroider build' test group
- add test apps for testing build error, excluding that from default scenarios
@evoactivity evoactivity changed the title Build Error when external resources are detected without integrity hashes WIP: Build Error when external resources are detected without integrity hashes Feb 17, 2024
@evoactivity
Copy link
Contributor Author

This includes changes from my other PR. So lets land that first. I'm working on the test now.

evoactivity
evoactivity commented Feb 17, 2024
View reviewed changes
tests/test.js Outdated Show resolved Hide resolved
@evoactivity
Copy link
Contributor Author

We'll need to filter these test apps from the root package.json build step

"build": "pnpm --filter '@test-apps/*' build",

otherwise the workflow will fail

@evoactivity
filter external-resource test apps from root build script
75831f3
@evoactivity
lint:fix decided these need updating 馃し
54dcfa7
@evoactivity
remove app build in normal test scenarios
14518e7
@evoactivity
fix app names
89bcaf2
@evoactivity
linting changed this again :/
6753ef8
@evoactivity
Merge branch 'main' into error-on-external-resources
abecee5 
* main:
  Release 0.1.3
  lint wanted to fix this again
  modified incorrect environment file
  fix app name in test app
  - ignore all node_modules - update test app name
  fix lockfile
  add test scenario for rootURL
  run lint fix
  rename buildPath to outputPath to support destructuring
  remove external resource skip
  - use destructuring to get buildPath and publicPath - fix typo
  - strip publicPath from asset locations - skip external resources - use base64 encoding for hash instead of hex
@evoactivity
prettier wasn't ignoring the changelog when set as /CHANGELOG.md
975587b
@evoactivity evoactivity changed the title WIP: Build Error when external resources are detected without integrity hashes Build Error when external resources are detected without integrity hashes Feb 17, 2024
@evoactivity evoactivity changed the title Build Error when external resources are detected without integrity hashes Emit build error when external resources are detected without integrity hashes Feb 17, 2024
jelhan
jelhan requested changes Feb 18, 2024
View reviewed changes
Copy link
Owner

@jelhan jelhan left a comment

Choose a reason for hiding this comment

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

Thanks a lot for fixing all my typos. 馃檲

I think we can improve test coverage without much work. Please have a look at my tests.

package.json Outdated Show resolved Hide resolved
test-apps/external-resource-missing-integrity-hash/app/index.html Show resolved Hide resolved
test-apps/external-resource-with-integrity-hash/app/index.html Show resolved Hide resolved
tests/test.js Outdated Show resolved Hide resolved
tests/test.js Outdated Show resolved Hide resolved
tests/test.js Outdated Show resolved Hide resolved
tests/test.js Outdated Show resolved Hide resolved
tests/test.js Outdated Show resolved Hide resolved
tests/test.js Outdated Show resolved Hide resolved
webpack-subresource-integrity-embroider/index.js Outdated Show resolved Hide resolved
evoactivity and others added 9 commits February 18, 2024 12:11
@evoactivity @jelhan
Update test-apps/external-resource-missing-integrity-hash/app/index.html
22f94af 
Add comment to make sure we don't remove jquery script tag being used for external resrouce test

Co-authored-by: Jeldrik Hanschke <jelhan@users.noreply.github.com>
@evoactivity @jelhan
Update test-apps/external-resource-with-integrity-hash/app/index.html
831bf8e 
comment to ensure we do not remove the jquery script being used to test we pass the build when the integrity hash is present

Co-authored-by: Jeldrik Hanschke <jelhan@users.noreply.github.com>
@evoactivity @jelhan
Update webpack-subresource-integrity-embroider/index.js
d483789 
test for protocol-less src as well as http(s)

Co-authored-by: Jeldrik Hanschke <jelhan@users.noreply.github.com>
@evoactivity
- improve tests
4d2d373 
- check integrity attribute isn't an empty string
@evoactivity
ensure we detect https, http and // external resources
72dc390
@evoactivity
lint fix again. My default prettier settings must do something to html.
ddb1f88
@evoactivity
move test setup to it's own file
48abfc1
@evoactivity
rename readFile function to getIndexHtml
0eb94b4
@evoactivity
don't use loop for existing integrity checks
231cb74
jelhan
jelhan requested changes Feb 18, 2024
View reviewed changes
Copy link
Owner

@jelhan jelhan left a comment

Choose a reason for hiding this comment

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

Just minor code formatting. Hope it's okay to change before merging. I struggled reading it due to missing blank lines when reviewing. It had problems noticing immediately where one code block ends and the other starts. Maybe I'm just too used to blank lines as deliminators. But as I will likely read that code from time to time again, would be great if you could accept the suggestions.

tests/setup.js Show resolved Hide resolved
tests/setup.js Show resolved Hide resolved
tests/setup.js Show resolved Hide resolved
@evoactivity
Copy link
Contributor Author

Yeah no problem. I assumed prettier would take care of that 馃槄

jelhan
jelhan approved these changes Feb 18, 2024
View reviewed changes
Copy link
Owner

@jelhan jelhan left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@jelhan jelhan merged commit b9ff095 into jelhan:main Feb 19, 2024
2 checks passed
@jelhan jelhan added the enhancement New feature or request label Feb 19, 2024
@jelhan
Copy link
Owner

jelhan commented Feb 19, 2024

Released as v0.2.0.

@evoactivity evoactivity deleted the error-on-external-resources branch February 19, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jelhan jelhan jelhan approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@evoactivity @jelhan