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

Define opaque-response blocking #1442

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Define opaque-response blocking #1442

wants to merge 1 commit into from

Conversation

annevk
Copy link
Member

@annevk annevk commented May 17, 2022

This is good enough for early review.

TODO:


  • At least two implementers are interested (and none opposed):
    • Chrome
    • Firefox
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chrome: …
    • Firefox: …
    • Safari: …
    • Deno (not for CORS changes): …

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


Preview | Diff

@annevk
Copy link
Member Author

annevk commented May 30, 2022

@domenic thoughts on "ParseText" vs "create a classic script" + mock data?

<li><p>If <var>request</var>'s <a for=request>response tainting</a> is "<code>opaque</code>",
<var>response</var>'s <a for=response>status</a> is not a <a>redirect status</a>, and the
<a>opaque-response-safelist check</a> given <var>request</var> and <var>response</var> returns
false, then return a <a>network error</a>.
Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps we should make the redirect status check part of the algorithm?

annevk added a commit to whatwg/html that referenced this pull request May 31, 2022
@domenic
Copy link
Member

domenic commented May 31, 2022

So I think using ParseText here is pretty clean, especially since ParseScript seems to really prefer you giving it a Realm (even though it's used just for pass through).

However there may be a slight issue here, which is that ParseText does early-error detection. I wonder if people really plan to do early-error detection (which IIUC involves pretty detailed JavaScript engine semantics) inside the network code where ORB is implemented.

@sefeng211
Copy link

Does the spec cover the case where service worker synthesizing a response, so the synthesized response shouldn't be blocked by ORB if service worker's request passes?

This is good enough for early review, but there are a number of issues that still need resolving: https://github.com/annevk/orb/labels/mvp.

There are also some inline TODO comments.

A PR against HTML is needed to ensure it passes the appropriate metadata for media element and classic script requests. We might also want to depend on HTML for parsing JavaScript.
@annevk
Copy link
Member Author

annevk commented Oct 27, 2022

Yes, if you look at https://whatpr.org/fetch/1442.html#http-fetch it currently handles service workers in step 5. Then in step 6, if there's still no response, it'll do a network fetch in substep 3. Then in substep 4 it'll do the opaque-response-safelist check. This is also clarified in a note at the end of step 6.

@domfarolino domfarolino mentioned this pull request Feb 9, 2023
4 tasks
aarongable pushed a commit to chromium/chromium that referenced this pull request Jun 7, 2023
This moves the check for empty MIME types before sniffing for JS + JSON.
This brings it into alignment with the spec. (Which solves JS + JSON
with proper parsing instead of sniffing, but also handles the
empty MIME type check before doing so.)

This is tested in WPT fetch/orb/tentative/unknown-mime-type.sub.any.js.

The bug currently doesn't show, because our decision to inject en empty
response hides the rejection from the test. Once ORB "v0.2" is enabled, the test breaks (without this fix).

Spec proposal: whatwg/fetch#1442

Bug: 1178928
Change-Id: I0ed64ef0b66cccd6fdda0d3771b45e7dfc19a81d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4594337
Commit-Queue: Daniel Vogelheim <vogelheim@chromium.org>
Reviewed-by: Titouan Rigoudy <titouan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1154463}
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 security-tracker Group bringing to attention of security, or tracked by the security Group but not needing response. topic: orb topic: same-origin policy
Development

Successfully merging this pull request may close these issues.

3 participants