Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[AssetMapper] Add integrity metadata to importmaps#60378
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.4
Are you sure you want to change the base?
Conversation
37d5d36 to4648474CompareIf you want to check, started some work was done here#58722 (and was waiting for people to step in and help, so really glad you did here 👍 ) |
| @@ -25,6 +26,7 @@ class MappedAssetFactory implements MappedAssetFactoryInterface | |||
| { | |||
| private const PREDIGESTED_REGEX = '/-([0-9a-zA-Z]{7,128}\.digested)/'; | |||
| private const PUBLIC_DIGEST_LENGTH = 7; | |||
| private const INTEGRITY_HASH_ALGORITHMS = ['sha256', 'sha384', 'sha512']; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we really need multiple algorithms ?
MatTheCatMay 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think so: it’s required to supporthttps://w3c.github.io/webappsec-subresource-integrity/#agility, and people apparently don’t agree on what the default algorithm should be.
| ->info('Array of algorithms used to compute importmap resources integrity.') | ||
| ->beforeNormalization()->castToArray()->end() | ||
| ->prototype('scalar')->end() | ||
| ->defaultValue(['sha384']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| ->defaultValue(['sha384']) | |
| ->defaultNull() |
This would be a BC break imho.
But it will also slow down the build time / memory usage.. so I do not think this is something that should be enabled per default (like cors)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Makes sense; guess the recipe would be a good place to put this default then?
365d76d tof401b42Compare This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -25,6 +26,7 @@ class MappedAssetFactory implements MappedAssetFactoryInterface | |||
| { | |||
| private const PREDIGESTED_REGEX = '/-([0-9a-zA-Z]{7,128}\.digested)/'; | |||
| private const PUBLIC_DIGEST_LENGTH = 7; | |||
| private const INTEGRITY_HASH_ALGORITHMS = ['sha256', 'sha384', 'sha512']; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These names are actually both the integrity prefix and the algorithm. For now they all match so as the list is private I guess we'll update it to a map if needed.
7f203cd to9fd9f8aCompare…ult)This PR was merged into the 7.4 branch.Discussion----------Replace `get_class()` calls by `::class`| Q | A| ------------- | ---| Branch? | 7.4| Bug fix? | no| New feature? | no| Deprecations? | no| Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->| License | MITReplace `get_class()` by `::class`It was already done in past insymfony#47401Commits-------e0a602b Replace get_class() calls by ::class
Uh oh!
There was an error while loading.Please reload this page.
Takes over#58722 because I missed it existed..!