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

Pass in content type to resource-timing #1481

Merged
merged 2 commits into from
May 8, 2023
Merged

Conversation

abinpaul1
Copy link
Contributor

@abinpaul1 abinpaul1 commented Aug 22, 2022

These changes are to support addition of content type attribute to Perfomance resource timing. Further details are available at w3c/resource-timing#341

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


Preview | Diff

fetch.bs Outdated Show resolved Hide resolved
@abinpaul1 abinpaul1 marked this pull request as ready for review September 22, 2022 08:01
@abinpaul1 abinpaul1 changed the title [draft] Pass in content type to resource-timing Pass in content type to resource-timing Sep 22, 2022
fetch.bs Outdated Show resolved Hide resolved
@annevk annevk added security/privacy There are security or privacy implications topic: timing Issues and PR that touch on the infrastructure that is used by ResourceTiming, NavigationTiming, etc labels Sep 26, 2022
@annevk
Copy link
Member

annevk commented Sep 26, 2022

It also seems like we need to have w3c/server-timing#89 resolved before continuing here.

@yoavweiss
Copy link
Collaborator

It also seems like we need to have w3c/server-timing#89 resolved before continuing here.

@annevk the equivalent concerns related to this are currently handled in the ResourceTiming PR, essentially enabling user agents to trim entropy off of mime types to align them with a well-known list.

We tried to go off a well-known list of types in MIMESNIFF, but there seem to be types that have inherent entropy in their essence ("+zip" and "+xml" types specifically) which made that unreasonably hard.

So, I guess the open questions are:
a) Is implementation-defined trimming good enough from your perspective? Given that the concern doesn't have consensus around it, I thought this would be good enough.
b) Do you prefer the trimming to happen in Fetch rather than in RT?

/cc @achristensen07

@annevk
Copy link
Member

annevk commented Sep 27, 2022

I think it should happen in Fetch, if that is something that we plan on doing. I also think it should be standardized as it seems like it could be an interop hazard.

Could we perhaps normalize +zip to application/zip and +xml to application/xml and call it a day? And perhaps special case a few types such as image/svg+xml so they are preserved.

@yoavweiss
Copy link
Collaborator

I'm wary of pushing for a complex mechanism here, given that there's no consensus that w3c/server-timing#89 is indeed an issue. Does the fact that this would require CORS (rather than TAO) change the calculus on Apple's threat model?

@annevk
Copy link
Member

annevk commented Oct 21, 2022

@yoavweiss I think so, but then we also don't need stripping to just the essence, right? Not entirely clear to me how that relates to the "navigate"-related aspects of this PR, either.

@yoavweiss
Copy link
Collaborator

@yoavweiss I think so, but then we also don't need stripping to just the essence, right?

From my perspective, we don't need that (for neither this nor ServerTiming, which is TAO protected). I know that @achristensen07 disagrees with me on the ServerTiming/TAO aspects, but I'm not sure if he does on exposing such info to CORS enabled resources.

Not entirely clear to me how that relates to the "navigate"-related aspects of this PR, either.

For the "navigate" parts, they require same-origin, so I don't think @achristensen07's threat model applies there.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 24, 2022
This CL introduces a contentType field to Performance Resource
Timing object. This field is behind a Runtime Enabled Flag.

Resource Timing PR : w3c/resource-timing#341
Fetch PR : whatwg/fetch#1481

Bug:1366706
Change-Id: If4c92ef96d74e3ddbb71420ac61dbea4bfa1163b
aarongable pushed a commit to chromium/chromium that referenced this pull request Oct 25, 2022
This CL introduces a contentType field to Performance Resource
Timing object. This field is behind a Runtime Enabled Flag.

Resource Timing PR : w3c/resource-timing#341
Fetch PR : whatwg/fetch#1481

Bug: 1366706
Change-Id: If4c92ef96d74e3ddbb71420ac61dbea4bfa1163b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3916841
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Abin Paul <abin.paul1@gmail.com>
Cr-Commit-Position: refs/heads/main@{#1063289}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 25, 2022
This CL introduces a contentType field to Performance Resource
Timing object. This field is behind a Runtime Enabled Flag.

Resource Timing PR : w3c/resource-timing#341
Fetch PR : whatwg/fetch#1481

Bug: 1366706
Change-Id: If4c92ef96d74e3ddbb71420ac61dbea4bfa1163b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3916841
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Abin Paul <abin.paul1@gmail.com>
Cr-Commit-Position: refs/heads/main@{#1063289}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 25, 2022
This CL introduces a contentType field to Performance Resource
Timing object. This field is behind a Runtime Enabled Flag.

Resource Timing PR : w3c/resource-timing#341
Fetch PR : whatwg/fetch#1481

Bug: 1366706
Change-Id: If4c92ef96d74e3ddbb71420ac61dbea4bfa1163b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3916841
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Abin Paul <abin.paul1@gmail.com>
Cr-Commit-Position: refs/heads/main@{#1063289}
@abinpaul1 abinpaul1 force-pushed the content-type branch 2 times, most recently from 9cb87bf to 9dab8eb Compare November 7, 2022 17:42
@yoavweiss
Copy link
Collaborator

@annevk - friendly ping! Do we need the essence stripping from your perspective?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Right, I agree we can expose this as-is. Content-Type is a safelisted response header when it comes to CORS.

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 12, 2022
…iming, a=testonly

Automatic update from web-platform-tests
Add content-type to PerformanceResourceTiming

This CL introduces a contentType field to Performance Resource
Timing object. This field is behind a Runtime Enabled Flag.

Resource Timing PR : w3c/resource-timing#341
Fetch PR : whatwg/fetch#1481

Bug: 1366706
Change-Id: If4c92ef96d74e3ddbb71420ac61dbea4bfa1163b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3916841
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Abin Paul <abin.paul1@gmail.com>
Cr-Commit-Position: refs/heads/main@{#1063289}

--

wpt-commits: 4c8e480994182b5cdc60f5c01ebf208df70ec73e
wpt-pr: 36059
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
@abinpaul1 abinpaul1 force-pushed the content-type branch 2 times, most recently from c231b11 to 948dc1b Compare November 14, 2022 11:39
@annevk
Copy link
Member

annevk commented Nov 14, 2022

@abinpaul1 looking at the Chromium patch it strikes me that it might not do MIME type parsing currently. I think it would be good if we had test coverage for that. https://github.com/web-platform-tests/wpt/tree/master/fetch/content-type has some existing tests you might be able to reuse.

I'm also gonna push a small fixup commit for an existing formatting issue in the <dl> list.

jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Nov 14, 2022
…iming, a=testonly

Automatic update from web-platform-tests
Add content-type to PerformanceResourceTiming

This CL introduces a contentType field to Performance Resource
Timing object. This field is behind a Runtime Enabled Flag.

Resource Timing PR : w3c/resource-timing#341
Fetch PR : whatwg/fetch#1481

Bug: 1366706
Change-Id: If4c92ef96d74e3ddbb71420ac61dbea4bfa1163b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3916841
Reviewed-by: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Abin Paul <abin.paul1@gmail.com>
Cr-Commit-Position: refs/heads/main@{#1063289}

--

wpt-commits: 4c8e480994182b5cdc60f5c01ebf208df70ec73e
wpt-pr: 36059
@abinpaul1
Copy link
Contributor Author

@annevk Added tests for checking the parsing over at https://chromium-review.googlesource.com/c/chromium/src/+/4058549.
Please have a look

@annevk
Copy link
Member

annevk commented Dec 1, 2022

@abinpaul1 I didn't realize that directory doesn't test a bunch of the error cases around MIME type handling. It's essentially only handling the Content-Type charset parameter defaulting. It still seems good though, but maybe add https://github.com/web-platform-tests/wpt/blob/master/mimesniff/mime-types/resources/mime-types.json to the mix?

However, it also seems like you're expecting the wrong thing. It should be the mimeType field, not documentContentType, right?

@abinpaul1
Copy link
Contributor Author

abinpaul1 commented Dec 1, 2022

It still seems good though, but maybe add https://github.com/web-platform-tests/wpt/blob/master/mimesniff/mime-types/resources/mime-types.json to the mix?

I've tried to include those as well now. Please have another look

However, it also seems like you're expecting the wrong thing. It should be the mimeType field, not documentContentType, right?

Yeah. Changed to mimeType field.

@annevk
Copy link
Member

annevk commented Dec 2, 2022

Thanks, that looks good. For the problem you're running into with mime-types.json, I recommend taking a look at the README https://github.com/web-platform-tests/wpt/tree/master/mimesniff/mime-types (or one of the tests using it).

@abinpaul1
Copy link
Contributor Author

Thanks again for the pointers. Updated the tests now (extra checks similar to charset-parameter.window.js#L41 before running the test case from json.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This looks good. At this point we need either Gecko or WebKit to support this change.

@yoavweiss
Copy link
Collaborator

Seems like Gecko are now supportive.

@RByers
Copy link

RByers commented Apr 25, 2023

Is there anything still blocking this PR from landing?

@annevk
Copy link
Member

annevk commented Apr 27, 2023

Apologies for the delay here. I was out the last couple weeks and only just circled back with Alex about his objection to this feature. I laid out a path forward in WebKit/standards-positions#88 (comment) that I'm prepared to help with. Feedback from you all as well as @zcorpan @bdekoz @sefeng211 appreciated.

@annevk
Copy link
Member

annevk commented Apr 27, 2023

I put up whatwg/mimesniff#171 which we could call here so the least amount of information leaves the network layer. Seems cleanest. That would require a slight restructuring, but nothing too bad and it would make the Resource Timing side very straightforward.

@yoavweiss indicated support in WebKit/standards-positions#88 (comment). Once I've had review of the MIME Sniffing PR I would be happy to update this PR as well and provide instructions for the Resource Timing PR.

annevk added a commit to whatwg/mimesniff that referenced this pull request May 4, 2023
This defines the content type field as a minimized version of the response's parsed Content-Type header, to give web developers enough information to distinguish responses, but not enough to make it an additional communication channel.

This feature will be used by Resource Timing as per w3c/resource-timing#341.

Tests: ...
@annevk
Copy link
Member

annevk commented May 4, 2023

@abinpaul1 I updated this to account for the MIME Sniffing change (doesn't pass CI yet due to indexing, probably tomorrow). Will you update the tests?

@abinpaul1
Copy link
Contributor Author

@annevk Yeah..I can do that. Couple of things I wanted to clarify before updating the same.

  • Since we now mininimize before returning, I guess the tests we added for checking the content-type parsing can't be kept, right?
  • As part of the new tests, I believe it would be easy to test for javascript, XML, JSON types because we define consistent behaviour , however how do we test for other types say image/png that fall under - If mimeType is supported by the user agent, then return mimeType's essence?

fetch.bs Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented May 5, 2023

@abinpaul1 I think we could add a minimizedMIMEType member. In particular for https://github.com/web-platform-tests/wpt/blob/master/mimesniff/mime-types/resources/mime-types.json that seems good. But there need to be new tests as well, yes. It might well make sense to create a JSON resource specifically for inputs and outputs of the minimized algorithm.

For types that fall into the "is supported by" conditional, I suggest we stick with types we know all browsers implement. And keep types that are in the process of being standardized/implemented in a tentative file or some such.

Also, did we have cross-origin tests already? If not we probably should have some as well, even if only for images and scripts or some such.

@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label May 5, 2023
@abinpaul1
Copy link
Contributor Author

@annevk

I've updated the tests here : https://chromium-review.googlesource.com/c/chromium/src/+/4509283

And keep types that are in the process of being standardized/implemented in a tentative file or some such.

I'm not sure if I know which types are being standardized and have to be included as tentative. Would it make sense for us to just add them to the JSON for the minimized algorithm tests later when they do become standardized ?

Also, did we have cross-origin tests already? If not we probably should have some as well, even if only for images and scripts or some such.

I think these were covered in content-type.html - 1 and 2

@annevk
Copy link
Member

annevk commented May 8, 2023

That seems fine, but e.g., you could assume "image/png" is always supported.

@annevk annevk removed the needs tests Moving the issue forward requires someone to write tests label May 8, 2023
@annevk annevk merged commit 931cd06 into whatwg:main May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security/privacy There are security or privacy implications topic: timing Issues and PR that touch on the infrastructure that is used by ResourceTiming, NavigationTiming, etc
Development

Successfully merging this pull request may close these issues.

None yet

5 participants