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.

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

Integrity-metadata should not be a preload key #1418

Closed
wants to merge 1 commit into from

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Mar 22, 2022

In conjunction with whatwg/html#7738

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

@annevk
Copy link
Member

annevk commented Mar 23, 2022

I'm not sure I understand the tests. Maybe @domfarolino can help explain how to read them. For instance, "Same-origin script with non-matching digest does not re-use preload with matching digest." suggests to me that integrity metadata is looked at when looking at preloads to consume. Or am I missing something?

@noamr
Copy link
Contributor Author

noamr commented Mar 23, 2022

Same-origin script with non-matching digest

That test has a good digest for preload and a bad one for consume, and it shows that the preload is ignored.
"not CORS request, with correct hash" tests the main use-case here; that even when the consumer doesn't specify integrity metadata, the preload (which does have integrity metadata) can be used.

There is a test missing to show that if both preload and consumer have matching integrity but with a different SHA the preload should be consumed. It's an edge case that IIRC Chromium currently doesn't implement due to it being esoteric and the cost of not implementing it is low (a missed preload). I can add a test for it and open a bug if that's necessary.

@annevk
Copy link
Member

annevk commented Mar 23, 2022

That test has a good digest for preload and a bad one for consume, and it shows that the preload is ignored.

Why would the preload be ignored though? How does that follow from the specification text?

@noamr
Copy link
Contributor Author

noamr commented Mar 23, 2022

That test has a good digest for preload and a bad one for consume, and it shows that the preload is ignored.

Why would the preload be ignored though? How does that follow from the specification text?

Actually you're right. The test shows that there would be two requests, while in the spec text there would be one request and the image would fail.

I think a spec text that would correspond with the tests (and current implementation) is to keep fetch as is, but to make integrity meta data an "optional" part of the preload key, meaning that if it's not present then the lookup ignores integrity metadata.

This is also reflected by "Cross-origin script, not CORS request, with hash mismatch" - there is only one (failing) request that also fails the consumer even though the consumer doesn't specify a hash.

@noamr
Copy link
Contributor Author

noamr commented Mar 23, 2022

Added web-platform-tests/wpt#33326

There were 3 cases that were not handled in the original SRI test:

  • Resource has SRI but preload doesn't. Both Chromium & Gecko ignore the preload in that case
  • Resource and preload have matching SRIs but different algos. Chromium ignores the preload and Firefox accepts.
  • Same as previous, but algo of consumer is stronger than that of preload

I'm ignoring WebKit results here as it doesn't check for resource integrity when consuming resources (which can lead to consuming resources with a mismatch!)

So:

  • In both Gecko & Chromium, if consumer doesn't have SRI the preload is considered valid.
  • In Chromium, the preload is consumed only if the SRI matches 1:1 (or consumer doesn't have SRI)
  • In Gecko, the preload is consumed only if the SRI of the preload is same or stronger algo than the consumer (or consumer doesn't have SRI)

As I said before, the latter is quite an edge case - perhaps this difference can be a MAY?

@noamr
Copy link
Contributor Author

noamr commented Mar 23, 2022

Amended whatwg/html#7738 to match current implementations, accounting for the edge-case mismatch

@hiroshige-g
Copy link

As the parameters of https://html.spec.whatwg.org/multipage/links.html#consume-a-preloaded-resource are not modified by whatwg/html#7738, I suspect this change to the caller-side of #consume-a-preloaded-resource is not needed? (The discussion above is still valid/useful for whatwg/html#7738)

@noamr
Copy link
Contributor Author

noamr commented Mar 24, 2022

As the parameters of https://html.spec.whatwg.org/multipage/links.html#consume-a-preloaded-resource are not modified by whatwg/html#7738, I suspect this change to the caller-side of #consume-a-preloaded-resource is not needed? (The discussion above is still valid/useful for whatwg/html#7738)

Yes, that's accurate. I am leaving this open for the discussion

@noamr
Copy link
Contributor Author

noamr commented Mar 24, 2022

Closing as there is no action item for the fetch spec, though some of the discussion is useful for reference.

@noamr noamr closed this Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants