Skip to content

feat: extract sri from fetch, upgrade to latest spec #4307

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Jun 28, 2025

No description provided.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jun 28, 2025

I am kind of dissappointed of the latest spec.

Copy link
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

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

Very good

*/
const applyAlgorithmToBytes = crypto.hash
? /** @type {ApplyAlgorithmToBytes} */ (algorithm, bytes) => {
return crypto.hash(algorithm, bytes, 'base64')
Copy link
Member

Choose a reason for hiding this comment

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

Will this cause an ExperimentalWarning on node v20? If not, crypto.hash is available in all versions of node undici supports. If it does, we should only use createHash for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didnt see any ExperimentalWarning on node 20.19.2

Copy link
Member

Choose a reason for hiding this comment

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

crypto.hash will be available on all supported versions of node then, we don't need the fallback

Comment on lines +101 to +102
// See https://github.com/web-platform-tests/wpt/commit/e4c5cc7a5e48093220528dfdd1c4012dc3837a0e
// "be liberal with padding". This is annoying, and it's not even in the spec.

Choose a reason for hiding this comment

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

Can you file an issue? I agree we want that tackled. It's very annoying, but Firefox was perceived as "incompatible" because the implementation in Chrome was more permissive and websites didn't work outside of Chrome. They couldn't easily go through an deprecation, so we had to budge.

The way we do this (iirc), is to decode all of the values liberally (base64, base64url with/without padding) and compare raw bytes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoa! That is of course smarter?!

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jul 9, 2025

I think we can review and merge it. The problems with base64url are well known.

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.

3 participants