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

Revisit destination for <iframe>, <frame>, <embed>, and <object>. #948

Merged
merged 2 commits into from
Jun 5, 2020

Conversation

mikewest
Copy link
Member

@mikewest mikewest commented Oct 7, 2019

As discussed in w3c/webappsec-fetch-metadata#45, splitting 'document' into a set of
destination values that developers can use to determine whether the request is for
a top-level document or a nested document will allow us to simplify Fetch Metadata
checks performed server side.


Preview | Diff

@mikewest
Copy link
Member Author

mikewest commented Oct 7, 2019

I'll follow this up with an HTML patch in a moment.

mikewest added a commit to whatwg/html that referenced this pull request Oct 7, 2019
Currently, top-level and nested navigations set a request destination of
"document" when performing the "process a navigate fetch" algorithm.
This patch follows whatwg/fetch#948, splitting "document" into "frame"
and "iframe" for nested navigation requests.
fetch.bs Outdated
or "<code>worker</code>".
whose <a for=request>destination</a> is "<code>document</code>", "<code>frame</code>",
"<code>iframe</code>", "<code>report</code>", "<code>serviceworker</code>",
"<code>sharedworker</code>", or "<code>worker</code>".
Copy link
Member

Choose a reason for hiding this comment

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

This isn't accurate anymore. Currently for embed/object the destination would be document if they have a browsing context that is navigated and they would end up classified here as a non-subresource request.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, embed and object are "potential-navigation-or-subresource" requests, and don't get categorized into either "subresource" or "non-subresource". I don't think this patch changes that.

Looking at "display a plugin", it looks like <embed> and <object> will always create a nested browsing context. Is that how we're categorizing "subresource" vs "non-subresource"? If so, then I agree that it seems like we should lump them into the "non-subresource" bucket.

It's not clear to me what the impact is there, though. Where is "non-subresource request" used?

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that embed (and object) are initially no-cors / embed, but change to navigate / document the moment they display a non-XXX resource (maybe XXX is only images?) and instantiate a nested browsing context.

https://w3c.github.io/ServiceWorker/ uses non-subresource.

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as Fetch is concerned, I think <embed> and <object> are both always no-cors / {embed,object}.

Tracing through from https://html.spec.whatwg.org/#the-embed-element-setup-steps:

  1. Step 3.4 fetches with no-cors / embed.
  2. Step 3.4.3 switches on the type of the response (in a way that I'm not sure matches implementations; both Firefox and Chrome render text/html, for instance) to either display a plugin or navigate to the response.
  3. Displaying a plugin drops any nested browsing context in step 1, and hand-waves at finding a plugin for the response type.
  4. Navigating to the response does not re-fetch with navigation / document; it navigates to the response that was given for the no-cors/embed fetch above.

<object> looks pretty similar, though more capacious with regard to the types it handles specially.

(It's probably worth reevaluating all of this complexity in the light of Flash's impending death. Perhaps we can make <object> and <embed> act like strangely-spelled <iframe>s at some point?)

https://w3c.github.io/ServiceWorker/ uses non-subresource.

https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm uses "non-subresource request" only after punting entirely on "potential-navigation-or-subresource" requests, which means that the categorization would have no practical effect there.

Copy link
Member

Choose a reason for hiding this comment

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

If you navigate a browsing context whose owner is embed, how does it end up with no-cors / embed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a little worried it's gonna linger and never get fixed, despite being somewhat important from an interoperability perspective.

And note that before it'd go from no-cors/embed to navigate/document and that is being changed too.

I agree that we should fix it in the spec and lock it in with tests. I'm not sure this patch is the right place to do it, given how simple it is today, and how not-simple I think the <embed> change is going to be more generally. :)

If you'd prefer one big patch that addresses <embed>/<object>/<frame>/<iframe> all together, I'll do that! But I think I'd prefer to just land this and the associated HTML PR, and worry about <embed> and <object> in a subsequent set of PRs.

And given that it also changes for iframe et al, there needs to be an associated HTML PR already for this change.

whatwg/html#4976 is that change.

Copy link
Member

Choose a reason for hiding this comment

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

I might be missing something, but apart from the tests I don't think it's as big of a change on this side? The HTML side would require a lot of work, but not sure all of that is blocking given the many issues embed/object have already.

Copy link
Member Author

Choose a reason for hiding this comment

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

Step 5 of main fetch (https://fetch.spec.whatwg.org/#main-fetch) has pretty different behavior between "navigate" and "no-cors". I think it'll turn out to be incorrect for the initial request from <embed>, but I haven't through through what needs to change (or whether I'm even correct that it does need to change).

Copy link
Member Author

Choose a reason for hiding this comment

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

(There's also a minor MIME type change in step 1.3.2 of Fetching (https://fetch.spec.whatwg.org/#fetching), but that's likely not a big deal to change.)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, what Accept should say.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 7, 2019
@annevk noted in whatwg/fetch#948 (comment)
that it was possible to navigate an `<embed>` after it loads. I think
we should probably figure out how to make that navigation show up with a
destination of `embed` and a mode of `no-cors`.

Surprisingly, that seems to be what we're sending via Fetch Metadata.
I suspect it's not what the underlying `fetch()` API would show in a
service worker.

Bug: 1011763
Change-Id: I0156b2b1b467c914b15f6af50dfa37ce74db6c62
@mikewest
Copy link
Member Author

mikewest commented Oct 7, 2019

at that point it is navigation and cannot be used as a subresource anymore

I hesitate to accept this claim. :) Per spec, you're probably right. But short of auditing Chromium's plugin loading code, I'm not sure that it's practically the case in that engine. The code is a big mess, and it's not clear to me that it couldn't backslide into having an effect on the document in which it's embedded. I really can't wait until we can lock most of the weirdness behind a PPAPI flag for enterprise.

I'm willing to be convinced that navigate / embed is reasonable, but I think I'd like to err on the side of advertising the worst-case potential with no-cors.

@annevk
Copy link
Member

annevk commented Oct 7, 2019

Wouldn't that end up affecting other parts of the system in weird ways? E.g., how a service worker is selected to handle the request? And is no-cors always worse than navigate?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 7, 2019
@annevk noted in whatwg/fetch#948 (comment)
that it was possible to navigate an `<embed>` after it loads. In this
case, it seems safest to ensure that this navigation shows up with a
destination of `embed` and a mode of `no-cors`.

That seems to be what we're sending today via Fetch Metadata, so let's
lock it in via this patch's tests. I suspect it's not what the
underlying `fetch()` API would show in a service worker, but we can
deal with that in a separate patch.

Bug: 1011763
Change-Id: I0156b2b1b467c914b15f6af50dfa37ce74db6c62
@mikewest
Copy link
Member Author

mikewest commented Oct 7, 2019

Wouldn't that end up affecting other parts of the system in weird ways? E.g., how a service worker is selected to handle the request?

The way things are specified today, I'm not sure any of our current behaviors around this space are correct. :)

What would you like the behavior to be for Service Workers? Could we treat <embed> navigations the same way we treat the original request from the <embed> element itself?

cc @jakearchibald who possibly has opinions.

And is no-cors always worse than navigate?

My assumption is that no-cors can/will end up in an attacker's memory, whereas a navigate probably won't (assuming out-of-process frames). @arturjanc probably has an opinion from the server side of things...

@annevk
Copy link
Member

annevk commented Oct 7, 2019

If A frames B using embed, we cannot use A's service worker for navigations that happen in B. Navigations have to be in scope of a service worker, which only a service worker from B can be. We could say they always bypass the service worker, maybe, but I doubt that's what's going on. (I'm slowly remembering now though that we might always skip service workers here. Not sure what else would be affected.)

aarongable pushed a commit to chromium/chromium that referenced this pull request Oct 8, 2019
The conversation in whatwg/fetch#948 makes me
curious about how locked-in we are to the existing behavior around both
`<embed>` and `<object>` when used for non-plugin content. They're
basically strange `<iframe>`s, and it might be reasonable to push them
closer towards _being_ `<iframe>` elements, or to push them further
away, depending on how usage numbers look.

Change-Id: I843ae37ae74033fdb3ea9b920ce3932004a067b0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1847291
Commit-Queue: Mike West <[email protected]>
Reviewed-by: Daniel Vogelheim <[email protected]>
Cr-Commit-Position: refs/heads/master@{#703722}
aarongable pushed a commit to chromium/chromium that referenced this pull request Oct 14, 2019
@annevk noted in whatwg/fetch#948 (comment)
that it was possible to navigate an `<embed>` after it loads. In this
case, it seems safest to ensure that this navigation shows up with a
destination of `embed` and a mode of `no-cors`.

That seems to be what we're sending today via Fetch Metadata, so let's
lock it in via this patch's tests. I suspect it's not what the
underlying `fetch()` API would show in a service worker, but we can
deal with that in a separate patch.

Bug: 1011763
Change-Id: I0156b2b1b467c914b15f6af50dfa37ce74db6c62
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1844996
Commit-Queue: Mike West <[email protected]>
Reviewed-by: Daniel Vogelheim <[email protected]>
Cr-Commit-Position: refs/heads/master@{#705518}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 15, 2019
@annevk noted in whatwg/fetch#948 (comment)
that it was possible to navigate an `<embed>` after it loads. In this
case, it seems safest to ensure that this navigation shows up with a
destination of `embed` and a mode of `no-cors`.

That seems to be what we're sending today via Fetch Metadata, so let's
lock it in via this patch's tests. I suspect it's not what the
underlying `fetch()` API would show in a service worker, but we can
deal with that in a separate patch.

Bug: 1011763
Change-Id: I0156b2b1b467c914b15f6af50dfa37ce74db6c62
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1844996
Commit-Queue: Mike West <[email protected]>
Reviewed-by: Daniel Vogelheim <[email protected]>
Cr-Commit-Position: refs/heads/master@{#705518}
@mikewest
Copy link
Member Author

My reading of the service worker spec is that we skip the service worker for these requests. But I don't know if that's what vendors actually implement. Pinging @jakearchibald again who might have opinions (and data?).

@arturjanc apparently has the plague today, but when he recovers, he might be able to weigh in on the threat model around no-cors and navigate, and whether my assertions above match the mental model his team is working with.

(In the hopes of avoiding surprise, I currently think that this is the last open question we need to answer before shipping a first pass at sec-fetch-dest, which @iVanlIsh has started to work through in Chromium. Once we resolve this, I'd like to send an intent to ship for the feature so Artur, et al, can start rolling it out server-side.)

Hexcles pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 16, 2019
@annevk noted in whatwg/fetch#948 (comment)
that it was possible to navigate an `<embed>` after it loads. In this
case, it seems safest to ensure that this navigation shows up with a
destination of `embed` and a mode of `no-cors`.

That seems to be what we're sending today via Fetch Metadata, so let's
lock it in via this patch's tests. I suspect it's not what the
underlying `fetch()` API would show in a service worker, but we can
deal with that in a separate patch.

Bug: 1011763
Change-Id: I0156b2b1b467c914b15f6af50dfa37ce74db6c62
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1844996
Commit-Queue: Mike West <[email protected]>
Reviewed-by: Daniel Vogelheim <[email protected]>
Cr-Commit-Position: refs/heads/master@{#705518}
@annevk
Copy link
Member

annevk commented Oct 17, 2019

I'm really curious whether we skip service workers once a browsing context is established there. I'd like to see that tested at least.

From Firefox's perspective solving embed/object is the main blocker too.

@arturjanc
Copy link

weigh in on the threat model around no-cors and navigate, and whether my assertions above match the mental model his team is working with.

They do match our mental model: we expect developers to focus on blocking cross-site no-cors loads while allowing navigations so they protect from XS-Leaks but still allow incoming links to work.

One counterexample could be sites that don't care about XS-Leaks but want to more tightly control navigations to defend from XSS and e.g. prevent cross-site navigations to anything but a tightly controlled set of application endpoints. But for this -- less common -- scenario developers already have X-Frame-Options to fall back on so the resource shouldn't render when it's embedded; we can also publish reference logic that will consider a cross-site mode=no-cors + dest=embed/object request as a potential navigation (in addition to mode=navigate). This complication in the less likely use case for Fetch Metadata seems like a good trade-off compared to requiring everyone who cares about XS-Leaks to implement special handling for loads via embed/object.

Overall, I like the model of having initial loads via embed/object be no-cors, but turn into navigate when the browsing context gets navigated.

@mikewest
Copy link
Member Author

Overall, I like the model of having initial loads via embed/object be no-cors, but turn into navigate when the browsing context gets navigated.

@annevk I think you would be happy with this too, given your comments in #948 (comment). Is that still your preference?

I don't think this distinction will be easy to implement in Chromium, given the way <embed>/<object> are intertwined with navigation code, but I'm happy to assume that it's possible, spec and test it accordingly, and move on to more interesting debates. :)


For clarity, I think we're saying that the initial request from <embed> would send:

Sec-Fetch-Dest: embed
Sec-Fetch-Mode: no-cors
Sec-Fetch-Site: whatever

and subsequent requests from <embed> caused either by internal interaction or by poking at window.frames would send:

Sec-Fetch-Dest: embed
Sec-Fetch-Mode: navigate
Sec-Fetch-Site: whatever

@annevk
Copy link
Member

annevk commented Oct 18, 2019

The way the standard describes <embed src>/<object data> is roughly:

  1. Let response be the result of fetching something.
  2. If response is something that necessitates a browsing context, then:
    1. Create one.
    2. Navigate that browsing context to response.
  3. Otherwise, display response without browsing context.

And then once there is a browsing context, you'd navigate that normally. It's surprising that Chrome has apparently a very different model from this. I haven't checked with folks familiar with Gecko's loading code recently, but I'm pretty sure the above is somewhat accurate for Gecko (now whether it's easy to get the right values might still be up for debate there of course).

@mikewest
Copy link
Member Author

It's surprising that Chrome has apparently a very different model from this.

Chromium is nothing if not surprising. The challenge I'm thinking of is that we're currently keying the Sec-Fetch-Dest and Sec-Fetch-Mode header values off the framing element type (see https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_request.cc?rcl=5514046a3db301445a8e1c8aa23f638523e4f462&l=323 for example), and we run through the same codepath for the initial navigation from <embed> as we do for subsequent navigations. That is, Chromium treats the request in your step 1 as a navigation regardless of whether we're going to render it as a plugin or not. My recollection here is a bit fuzzy, but I think we end up synthesizing a PluginDocument to hold the plugin, so when we extracted navigation from the renderer process, <embed> and <object> came right along with it.

@jakearchibald
Copy link
Collaborator

I'm a little lost about what's trying to be achieved here, so I'm sorry if this is useless:

With service worker, we're happy for a service worker to intercept a request for <script src="https://example.com"> instead deliver a resource from evil.com. The script runs in the context of the page, and the service worker owns the page, so it's only lying to itself.

A service worker on evil.com cannot intercept the request for <iframe href="http://example.com">, because, as far as I know, this would allow evil.com to run content from a different origin within example.com.

This means requests which form a client (which have their own CSP, and run against a particular origin based on their final request), will go through a service worker that matches the request scope, rather than the controller of the client that initiated the request.

This all works nicely because we can tell if a response is going to be used to create a client based on the destination. An <img> doesn't, an <iframe> does, <script> doesn't, new Worker does etc etc.

We punted on <object>/<embed> because they're an exception. Here, the type of the response judges whether it creates a client or not. image/jpeg doesn't, but text/html does. This leads to the kind of mess you see in step 4.9 of the object element where it flip-flops around when <object> meets appcache.

If we can guarantee in advance whether a <object>/<embed> resource is going to create a client, we can feed it through service worker, otherwise it feels safest to avoid.

@annevk
Copy link
Member

annevk commented Oct 22, 2019

@mikewest so embed/object navigate and if the result of the navigation is an image, the browsing context being navigated is tossed away and the image is rendered? And some care is taken to not exposing the browsing context being navigated to the embedding document?

@mikewest
Copy link
Member Author

mikewest commented Oct 22, 2019

@annevk:

so embed/object navigate and if the result of the navigation is an image, the browsing context being navigated is tossed away and the image is rendered?

No. We do the same thing as we do for more general navigation to an image resource: synthesize an ImageDocument which wraps the image we just downloaded in a minimal HTML document for rendering. :)

And some care is taken to not exposing the browsing context being navigated to the embedding document?

I'm not sure what you mean here? We use the origin of the resource as the browsing context's origin, and access controls flow naturally from that.

@annevk
Copy link
Member

annevk commented Oct 22, 2019

Very interesting, but there's no initial about:blank it seems, so there's no exposed browsing context (e.g., via self[0]) until the load event fires, as far as I can tell. So it's still a bit different from how iframe works, but much less so than in Firefox.

I'm not sure what we want to do in that case.

cc @bzbarsky

@mikewest
Copy link
Member Author

mikewest commented Oct 22, 2019

Very interesting, but there's no initial about:blank it seems, so there's no exposed browsing context (e.g., via self[0]) until the load event fires, as far as I can tell. So it's still a bit different from how iframe works, but much less so than in Firefox.

I think that's correct. If/when we actually get rid of plugins, it might be reasonable to make embed and object work more like strangely-spelled iframe elements? Given their usage in the wild, I think it's going to be hard to make substantial changes to their behavior. I added some metrics to Chrome, and based on a week of Canary, it looks like ~0.38% of page views contain an embed or object tag that loaded an HTML document (higher than I expected, even with all the caveats around Canary not being terribly representative).

Implementation details to the side, does the behavior in #948 (comment) seem reasonable to you? If so, I'll figure out how to make it work in HTML and Chromium.

@annevk
Copy link
Member

annevk commented Oct 22, 2019

If Firefox is okay with moving slowly towards making embed/object always create browsing contexts as well (even if it's only visible once the load event fires), I think the better solution is your initial take of always using mode navigate. I've pinged some folks internally and also bz above to see what they think.

@mikewest
Copy link
Member Author

I think my original proposal was no-cors for everything. navigate for everything seems reasonable once we're in a world where plugins aren't a problem. no-cors seems reasonable for requests whose response might end up in a plugin depending on type.

I don't think it makes a huge difference either way, as @arturjanc and team would just need to look at the destination in addition to the mode for navigation requests, which they already need to do for iframe and frame. That said, less well-informed developers might be inclined to treat navigate as totally fine.

Better documentation and example code might help mitigate that risk? I can live with either, I think.

@bzbarsky
Copy link

bzbarsky commented Oct 22, 2019

it might be reasonable to make embed and object work more like strangely-spelled iframe elements?

They have quite different layout behavior in the image case, no? Including in Chrome.

If Firefox is okay with moving slowly towards making embed/object always create browsing contexts as well (even if it's only visible once the load event fires)

What web-observables would this actually affect?

Ignoring for the moment plug-ins (let's pretend we killed them already), there are basically two cases for embed/object in the spec and in Gecko's implementation right now: document and image. Which case we're in is decided based on the MIME type of the last load triggered via the "src"/"data" attribute respectively.

If you set the relevant attr to a URL that returns a non-image MIME type that the browser is able to render, you get a browsing context (once we get the headers from the server) and general iframe-like layout behavior. The resulting browsing context can be navigated via its Location, including to image URLs (which will render like they would in an iframe). But setting the relevant attr again will (always? I'd have to check the code) get rid of the browsing context and then run the selection logic again once the response is received.

If you set the relevant attr to a URL that returns an image MIME type, we don't create a browsing context at all. We just have the image data (like <img> does), and create a layout object identical to the one for <img>, which knows how to do the right intrinsic sizing, etc. This could perhaps be done on top of a browsing context but would require some work to make the layout behavior correct. This is work that Chrome is apparently already doing, or it's doing something at least. If I do <object data="100x100px-image" style="height: 200px"> in Chrome, I get a 200x2000 image shown, which is totally not how iframes work. Also, note that the renderings of an iframe pointing to an image are NOT interoperable across Chrome and Firefox, nor are they standardized, unlike the case of <object> pointing to an image.

@mikewest
Copy link
Member Author

They have quite different layout behavior in the image case, no? Including in Chrome.

That's true, my claim earlier in the thread was incorrect: on testing again, I realize that Chrome has exactly the difference in behavior you're describing for <embed src="[image URL]"> on the one hand, and direct navigating to an image from within a browsing context associated with <embed src="[HTML URL]">. I was testing the latter case, but not the former.

Still, both cases run through the same navigation architecture in Chrome, and we make a decision about whether to commit into a new browsing context vs. render the result.

I suspect that the cases in which the image-like display nature you're pointing to is something we could change if we wanted to simplify the model. Based on the same (sketchy because Canary) data source as above, <0.0001% of page views contain either <embed> or <object> used to render an image.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 1, 2020
Step 13 of https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm
should exclude `embed` and `object` requests from Service Workers. Our
implementation handles this correctly for the initial request, but failed
to bypass the Service Worker for subsequent navigations. This patch adds
a destination check to
`ServiceWorkerMainResourceLoaderInterceptor::ShouldCreateForNavigation`,
and ensures that the `destination` for a given request is set early enough
in the lifecycle to ensure that the check succeeds.

See also whatwg/fetch#948.

Change-Id: I21a1d37da438e1d0f185696f2b3b4058bc3911fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209456
Reviewed-by: Matt Falkenhagen <[email protected]>
Reviewed-by: Tsuyoshi Horo <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Ben Kelly <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773781}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 1, 2020
Step 13 of https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm
should exclude `embed` and `object` requests from Service Workers. Our
implementation handles this correctly for the initial request, but failed
to bypass the Service Worker for subsequent navigations. This patch adds
a destination check to
`ServiceWorkerMainResourceLoaderInterceptor::ShouldCreateForNavigation`,
and ensures that the `destination` for a given request is set early enough
in the lifecycle to ensure that the check succeeds.

See also whatwg/fetch#948.

Change-Id: I21a1d37da438e1d0f185696f2b3b4058bc3911fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209456
Reviewed-by: Matt Falkenhagen <[email protected]>
Reviewed-by: Tsuyoshi Horo <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Ben Kelly <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773781}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this pull request Jun 1, 2020
Step 13 of https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm
should exclude `embed` and `object` requests from Service Workers. Our
implementation handles this correctly for the initial request, but failed
to bypass the Service Worker for subsequent navigations. This patch adds
a destination check to
`ServiceWorkerMainResourceLoaderInterceptor::ShouldCreateForNavigation`,
and ensures that the `destination` for a given request is set early enough
in the lifecycle to ensure that the check succeeds.

See also whatwg/fetch#948.

Change-Id: I21a1d37da438e1d0f185696f2b3b4058bc3911fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209456
Reviewed-by: Matt Falkenhagen <[email protected]>
Reviewed-by: Tsuyoshi Horo <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Ben Kelly <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773781}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 3, 2020
…bypasses Service Workers., a=testonly

Automatic update from web-platform-tests
Ensure that <object>/<embed> navigation bypasses Service Workers.

Step 13 of https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm
should exclude `embed` and `object` requests from Service Workers. Our
implementation handles this correctly for the initial request, but failed
to bypass the Service Worker for subsequent navigations. This patch adds
a destination check to
`ServiceWorkerMainResourceLoaderInterceptor::ShouldCreateForNavigation`,
and ensures that the `destination` for a given request is set early enough
in the lifecycle to ensure that the check succeeds.

See also whatwg/fetch#948.

Change-Id: I21a1d37da438e1d0f185696f2b3b4058bc3911fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209456
Reviewed-by: Matt Falkenhagen <[email protected]>
Reviewed-by: Tsuyoshi Horo <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Ben Kelly <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773781}

--

wpt-commits: 51e3a46a45c3a3ff3c934246d6de70fa3e63c7e9
wpt-pr: 23706
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jun 3, 2020
…bypasses Service Workers., a=testonly

Automatic update from web-platform-tests
Ensure that <object>/<embed> navigation bypasses Service Workers.

Step 13 of https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm
should exclude `embed` and `object` requests from Service Workers. Our
implementation handles this correctly for the initial request, but failed
to bypass the Service Worker for subsequent navigations. This patch adds
a destination check to
`ServiceWorkerMainResourceLoaderInterceptor::ShouldCreateForNavigation`,
and ensures that the `destination` for a given request is set early enough
in the lifecycle to ensure that the check succeeds.

See also whatwg/fetch#948.

Change-Id: I21a1d37da438e1d0f185696f2b3b4058bc3911fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209456
Reviewed-by: Matt Falkenhagen <[email protected]>
Reviewed-by: Tsuyoshi Horo <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Ben Kelly <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773781}

--

wpt-commits: 51e3a46a45c3a3ff3c934246d6de70fa3e63c7e9
wpt-pr: 23706
@wanderview
Copy link
Member

I believe the tests have landed. Can this be merged now?

mikewest and others added 2 commits June 4, 2020 17:09
As discussed in w3c/webappsec-fetch-metadata#45, this patch splits the
`document` destination into `document`, `frame`, and `iframe`. These
destinations distinguish top-level navigation from nested navigation,
and exposing this data via `Sec-Fetch-Dest` will allow developers to
better understand the nature of a request.

This patch also redefines "navigation request" and "non-subresource
request" to include `embed` and `object` destinations as a consequence
of the conversation in [1], which will also change the `mode` of those
requests from `no-cors` to `navigate` [2].

These changes are covered by WPT in //fetch/metadata, specifically
[3], [4], [5], and [6].

[1]: #948 (comment)
[2]: https://github.com/whatwg/html/pull/4976/files
[3]: https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/embed.tentative.https.sub.html
[4]: https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/object.tentative.https.sub.html
[5]: https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/iframe.tentative.https.sub.html
[6]: https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/navigation.tentative.https.sub.html
@annevk
Copy link
Member

annevk commented Jun 4, 2020

Can someone review my nit?

https://web-platform.test:8443/service-workers/service-worker/embed-and-object-are-not-intercepted.https.html fails in Firefox (things do get intercepted) so this needs a bug as well. Do any other bugs need to be filed?

@annevk annevk closed this Jun 4, 2020
@annevk annevk reopened this Jun 4, 2020
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 4, 2020
…bypasses Service Workers., a=testonly

Automatic update from web-platform-tests
Ensure that <object>/<embed> navigation bypasses Service Workers.

Step 13 of https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm
should exclude `embed` and `object` requests from Service Workers. Our
implementation handles this correctly for the initial request, but failed
to bypass the Service Worker for subsequent navigations. This patch adds
a destination check to
`ServiceWorkerMainResourceLoaderInterceptor::ShouldCreateForNavigation`,
and ensures that the `destination` for a given request is set early enough
in the lifecycle to ensure that the check succeeds.

See also whatwg/fetch#948.

Change-Id: I21a1d37da438e1d0f185696f2b3b4058bc3911fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209456
Reviewed-by: Matt Falkenhagen <[email protected]>
Reviewed-by: Tsuyoshi Horo <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Ben Kelly <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773781}

--

wpt-commits: 51e3a46a45c3a3ff3c934246d6de70fa3e63c7e9
wpt-pr: 23706

Differential Revision: https://phabricator.services.mozilla.com/D78336
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jun 5, 2020
…bypasses Service Workers., a=testonly

Automatic update from web-platform-tests
Ensure that <object>/<embed> navigation bypasses Service Workers.

Step 13 of https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm
should exclude `embed` and `object` requests from Service Workers. Our
implementation handles this correctly for the initial request, but failed
to bypass the Service Worker for subsequent navigations. This patch adds
a destination check to
`ServiceWorkerMainResourceLoaderInterceptor::ShouldCreateForNavigation`,
and ensures that the `destination` for a given request is set early enough
in the lifecycle to ensure that the check succeeds.

See also whatwg/fetch#948.

Change-Id: I21a1d37da438e1d0f185696f2b3b4058bc3911fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209456
Reviewed-by: Matt Falkenhagen <[email protected]>
Reviewed-by: Tsuyoshi Horo <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Ben Kelly <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#773781}

--

wpt-commits: 51e3a46a45c3a3ff3c934246d6de70fa3e63c7e9
wpt-pr: 23706

Differential Revision: https://phabricator.services.mozilla.com/D78336
@mikewest
Copy link
Member Author

mikewest commented Jun 5, 2020

Your nit LGTM, @annevk. Thank you.

@annevk
Copy link
Member

annevk commented Jun 5, 2020

@annevk annevk merged commit 57305b8 into master Jun 5, 2020
@annevk annevk deleted the framed-destinations branch June 5, 2020 08:58
annevk pushed a commit to whatwg/html that referenced this pull request Jun 5, 2020
Currently, top-level and nested navigations use a request destination of "document". This splits that into "document" (top-level) and "embed", "frame", "iframe", and "object" (nested).

It also makes embed/object use "navigate" as their request mode.

This complements whatwg/fetch#948 (which links relevant tests).
@annevk
Copy link
Member

annevk commented Jun 5, 2020

Thanks @mikewest and @wanderview for getting this done, I think all is merged now and all relevant bugs have been filed and linked.

yutakahirano pushed a commit that referenced this pull request Jun 23, 2020
As discussed in w3c/webappsec-fetch-metadata#45, this splits the "document" destination into "document", "frame", and "iframe". These destinations distinguish top-level navigation from nested navigation, and exposing this data via `Sec-Fetch-Dest` will allow developers to better understand the nature of a request.

This patch also redefines "navigation request" and "non-subresource request" to include "embed" and "object" destinations as discussed at #948 (comment). That discussion also resulted in other changes:

* whatwg/html#4976
* w3c/ServiceWorker#1486

Tests:

* https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/embed.tentative.https.sub.html
* https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/object.tentative.https.sub.html
* https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/iframe.tentative.https.sub.html
* https://github.com/web-platform-tests/wpt/blob/master/fetch/metadata/navigation.tentative.https.sub.html
* https://github.com/web-platform-tests/wpt/blob/master/service-workers/service-worker/embed-and-object-are-not-intercepted.https.html
mikewest added a commit to w3c/webappsec-fetch-metadata that referenced this pull request Jul 13, 2021
After whatwg/fetch#948,
whatwg/fetch#993, and
whatwg/html#5203, the integration with Fetch and
HTML is complete. This patch points to those integration points rather
than claiming that there's still work to be done.

Closes #73.
mikewest added a commit to w3c/webappsec-fetch-metadata that referenced this pull request Jul 20, 2021
After whatwg/fetch#948,
whatwg/fetch#993, and
whatwg/html#5203, the integration with Fetch and
HTML is complete. This patch points to those integration points rather
than claiming that there's still work to be done.

Closes #73.
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
Step 13 of https://w3c.github.io/ServiceWorker/#on-fetch-request-algorithm
should exclude `embed` and `object` requests from Service Workers. Our
implementation handles this correctly for the initial request, but failed
to bypass the Service Worker for subsequent navigations. This patch adds
a destination check to
`ServiceWorkerMainResourceLoaderInterceptor::ShouldCreateForNavigation`,
and ensures that the `destination` for a given request is set early enough
in the lifecycle to ensure that the check succeeds.

See also whatwg/fetch#948.

Change-Id: I21a1d37da438e1d0f185696f2b3b4058bc3911fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209456
Reviewed-by: Matt Falkenhagen <[email protected]>
Reviewed-by: Tsuyoshi Horo <[email protected]>
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Ben Kelly <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#773781}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: e59cf87ae0944fd6fe3e7e013e703bd6171fe382
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.

8 participants