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

Consider splitting document into document and nested-document. #755

Closed
mikewest opened this issue Jun 5, 2018 · 23 comments
Closed

Consider splitting document into document and nested-document. #755

mikewest opened this issue Jun 5, 2018 · 23 comments
Labels
addition/proposal New features or enhancements topic: api

Comments

@mikewest
Copy link
Member

mikewest commented Jun 5, 2018

As discussed in the context of Sec-Metadata, it might be useful to clearly distinguish between top-level and nested navigation requests by splitting the document destination into document and nested-document.

@annevk notes that this might be a breaking change, as folks might be relying on the existing document destination that's exposed on Request objects. @wanderview, @jakearchibald: WDYT?

@annevk annevk added addition/proposal New features or enhancements topic: api labels Jun 5, 2018
@wanderview
Copy link
Member

Since Request.destination was only relatively recently shipped in browsers, now might be the time to make this change. Sites would need to adapt, but they could do evt.request.destination.includes('document') or something.

FWIW, this better matches what firefox uses internally:

https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/dom/base/nsIContentPolicy.idl#87-96

Which we then map to the exposed document destination:

https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/dom/fetch/InternalRequest.cpp#274-277

I was wondering if this change would interact badly with the portals proposal, but I guess the navigation request would have to complete well before any portal is promoted from 3rd party to 1st party.

@mikewest
Copy link
Member Author

mikewest commented Jun 5, 2018

Thanks, @wanderview, that's helpful. Who should we loop in from WebKit/Edge to see if they'd be willing to do the same?

@wanderview
Copy link
Member

I think @youennf can speak to the fetch API in webkit.

I'm not sure for edge. Maybe @aliams or @jungkees can help.

@youennf
Copy link
Collaborator

youennf commented Jun 5, 2018

I am not totally clear about the intent of this change.
Is it to allow a simpler definition/implementation of Sec-Metadata?
Is it to allow service worker to start acting as some sort of security proxy that could decide to stop loads based on a better knowledge of the client triggering the fetch?

FWIW, WebKit has this is-top-level-navigation concept, although at a slightly lower level. The proposed change seems ok from that prospective.
The same information could be made available elsewhere in a backward compatible way: fetch event or service worker client for instance.

@mikewest
Copy link
Member Author

mikewest commented Jun 5, 2018

Hey, @youennf!

Is it to allow a simpler definition/implementation of Sec-Metadata?

Right now, Sec-Metadata defines a target attribute to describe the target browsing context of a navigation request: https://mikewest.github.io/sec-metadata/#target-member. It would be ideal if we could get rid of that member by folding the top-level/nested distinction into the destination.

Is it to allow service worker to start acting as some sort of security proxy that could decide to stop loads based on a better knowledge of the client triggering the fetch?

That's also a distinction that I think might be useful, but it's not my intent. Offering that capability to developers would be a happy side-effect of this change. :)

@youennf
Copy link
Collaborator

youennf commented Jun 5, 2018

That seems reasonable to me.

Given a service worker might expose the same data that servers would provide directly, it makes sense to me that a service worker should get the same level of information as the one provided to the server through Sec-Metadata, at least for navigation loads.
The cause member information does not seem to be exposed to service worker.
The same-site value might be somehow exposed although computing the public suffix might be more easily done by the browser.

@jakearchibald
Copy link
Collaborator

Isn't this information (and more) already available by getting the client associated with fetchEvent.targetClientId, then looking at its client.ancestorOrigins?

@wanderview
Copy link
Member

Do you mean resultingClientId? You won't be able to look at any information about that client until after the fetch completes, right?

Also, I don't think client.ancestorOrigins has been implemented widely yet due to privacy concerns being resolved with the main thread ancestorOrigins API. I think we still have client.frameType, though, until that is resolved.

@jungkees
Copy link
Contributor

The idea of exposing the header to service workers sounds interesting.

My initial thought is what'd be a downside of maintaining the target member? The values of the destination look more like what the request's resulting resource kind would be. If we split them, there seems to be quite a few places that need some clarification. E.g. "A navigation request is a request whose destination is "document" or "nested-document", etc.

Do you mean resultingClientId? You won't be able to look at any information about that client until after the fetch completes, right?

Yes.

@jakearchibald
Copy link
Collaborator

@wanderview wouldn't targetClientId be the right thing to look at here?

@mikewest
Copy link
Member Author

There seems to be some level of agreement that splitting document might be a reasonable thing to do. If I send out a PR against Fetch, HTML, and added some tests, would other vendors implement the change? :)

@wanderview
Copy link
Member

@wanderview wouldn't targetClientId be the right thing to look at here?

I think we answered this on IRC. Since targetClientId will be null for cross-origin targets or new top level windows it can't really be used reliably to determine the type of request.

@mikewest
Copy link
Member Author

@arturjanc reminded me that I never came back to this.

Do y'all think it's worth trying to poke at destination in this way? Or would y'all prefer introducing a new concept on request objects instead?

@annevk
Copy link
Member

annevk commented Oct 26, 2018

I'd prefer changing destination over adding a new concept.

@mikewest
Copy link
Member Author

I put up #819. I'll find time to put up an HTML counterpart.

@mikewest
Copy link
Member Author

In w3c/webappsec-fetch-metadata#16, @annevk suggested that we move this to the request's mode instead, which seems fine.

@yoavweiss
Copy link
Collaborator

When discussing the definition of Prefetch's processing model it was raised that we probably want to limit it in some cases to top-level navigations. In that context, a request.destination that indicates a top-level navigation would be helpful to us, and would cleanly map to the as attribute already exposed on HTMLLinkElement.

/cc @kinu @jkarlin

@mikewest
Copy link
Member Author

Would a request.mode of navigate vs nested-navigate work for you as well, @yoavweiss? That's what @annevk suggested in the linked bug.

@yoavweiss
Copy link
Collaborator

It will require us to rewire some as values to request mode, or to come up with some other opt-in that maps into request mode directly, which is feasibale, but not as clean. Admittedly, I haven't yet read w3c/webappsec-fetch-metadata#16 regarding the reasoning for request.mode. I'll do that now! :)

@mikewest
Copy link
Member Author

I don't have strong opinions either way. As long as I can distinguish nested vs. non-nested navigations in some way, I'm happy. We could even do both. Whatever. :)

@yoavweiss
Copy link
Collaborator

OK, I read the thread on the issue, and IIUC there's still an open question there on whether we want to expose request.destination to servers, right?

If it is exposed, I think it'd be cleaner to have nestedness be part of destination.

@yoavweiss
Copy link
Collaborator

If it ends up not being exposed, and "nested-navigate" request.mode is a thing, I guess we can somehow live with that spec-complexity and map as values to that.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 4, 2019
In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 4, 2019
In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
aarongable pushed a commit to chromium/chromium that referenced this issue Sep 5, 2019
In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Łukasz Anforowicz <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#693517}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 5, 2019
In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Łukasz Anforowicz <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#693517}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Sep 5, 2019
In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Łukasz Anforowicz <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#693517}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 12, 2019
…rvice., a=testonly

Automatic update from web-platform-tests
Set `Sec-Fetch-Mode` from the network service.

In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Łukasz Anforowicz <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#693517}

--
Mark appcache.tentative.https.sub.html as slow
--

wpt-commits: 4f6052ac7224c6bd0c859bea09d73f4a846920f8, d6b04f2d1e8a3268ab31b373ec0d14fa55829dc9
wpt-pr: 18838
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Sep 12, 2019
…rvice., a=testonly

Automatic update from web-platform-tests
Set `Sec-Fetch-Mode` from the network service.

In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <[email protected]>
Reviewed-by: Łukasz Anforowicz <[email protected]>
Commit-Queue: Mike West <[email protected]>
Cr-Commit-Position: refs/heads/master@{#693517}

--
Mark appcache.tentative.https.sub.html as slow
--

wpt-commits: 4f6052ac7224c6bd0c859bea09d73f4a846920f8, d6b04f2d1e8a3268ab31b373ec0d14fa55829dc9
wpt-pr: 18838
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
…rvice., a=testonly

Automatic update from web-platform-tests
Set `Sec-Fetch-Mode` from the network service.

In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <kinukochromium.org>
Reviewed-by: Łukasz Anforowicz <lukaszachromium.org>
Commit-Queue: Mike West <mkwstchromium.org>
Cr-Commit-Position: refs/heads/master{#693517}

--
Mark appcache.tentative.https.sub.html as slow
--

wpt-commits: 4f6052ac7224c6bd0c859bea09d73f4a846920f8, d6b04f2d1e8a3268ab31b373ec0d14fa55829dc9
wpt-pr: 18838

UltraBlame original commit: 7d3b0c03f936cc0fb5a532f740664b0517ec0cc9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
…rvice., a=testonly

Automatic update from web-platform-tests
Set `Sec-Fetch-Mode` from the network service.

In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <kinukochromium.org>
Reviewed-by: Łukasz Anforowicz <lukaszachromium.org>
Commit-Queue: Mike West <mkwstchromium.org>
Cr-Commit-Position: refs/heads/master{#693517}

--
Mark appcache.tentative.https.sub.html as slow
--

wpt-commits: 4f6052ac7224c6bd0c859bea09d73f4a846920f8, d6b04f2d1e8a3268ab31b373ec0d14fa55829dc9
wpt-pr: 18838

UltraBlame original commit: 7d3b0c03f936cc0fb5a532f740664b0517ec0cc9
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 4, 2019
…rvice., a=testonly

Automatic update from web-platform-tests
Set `Sec-Fetch-Mode` from the network service.

In order to move `Sec-Fetch-Mode` into the network service to ensure
that it's consistently set every time `Sec-Fetch-Site` is set, this
patch does a few things:

1.  Based on the conversation in [1] and [2], this patch extends
    `network::mojom::RequestMode` to include two new values to
    represent nested navigations and object/embed navigations as
    distinct from main-frame navigations.

2.  It introduces a new `network::IsNavigationRequestMode()` function
    to replace direct comparisons to `kNavigate` in various parts of
    the codebase.

3.  It refactors `network::SetSecFetchSiteHeader()` into
    `network::SetFetchMetadataHeaders()`. This creates a single entry
    point from `URLLoader` to set all Fetch Metadata headers, and
    fleshes out the `Sec-Fetch-Mode` header.

[1]: w3c/webappsec-fetch-metadata#37
[2]: whatwg/fetch#755

Bug: 972263, 990864
Change-Id: Icd20c7640d3d08ecb34a739f0203140fdcc195d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1780725
Reviewed-by: Kinuko Yasuda <kinukochromium.org>
Reviewed-by: Łukasz Anforowicz <lukaszachromium.org>
Commit-Queue: Mike West <mkwstchromium.org>
Cr-Commit-Position: refs/heads/master{#693517}

--
Mark appcache.tentative.https.sub.html as slow
--

wpt-commits: 4f6052ac7224c6bd0c859bea09d73f4a846920f8, d6b04f2d1e8a3268ab31b373ec0d14fa55829dc9
wpt-pr: 18838

UltraBlame original commit: 7d3b0c03f936cc0fb5a532f740664b0517ec0cc9
@annevk
Copy link
Member

annevk commented Jan 29, 2021

This was fixed by #948.

@annevk annevk closed this as completed Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: api
Development

No branches or pull requests

7 participants