Check ancestors when setting an <iframe> navigation's "site for cookies". [chromium/src : master]
Mike West (Gerrit)
Mike West would like Alex Moshchuk and Arthur Sonzogni to review this change.
Check ancestors when setting an <iframe> navigation's "site for cookies".
Currently, we're setting the "site for cookies" only by looking at the
top-level document. We ought to be verifying that the ancestor frames
are same-site before doing so. We do this correctly in Blink (see
`Document::SiteForCookies`), but didn't do so when navigating in the
browser.
This patch addresses the majority of the problem by walking the ancestor
chain when processing a NavigationRequest. If all the ancestors are
same-site, we set the "site for cookies" to the top-level document's URL.
If they aren't all same-site, we set it to an empty URL to ensure that
we don't send SameSite cookies.
Bug: 833847
Change-Id: Icd77f31fa618fa9f8b59fc3b15e1bed6ee05aabd
---
M content/browser/frame_host/navigation_request.cc
A third_party/WebKit/LayoutTests/http/tests/cookies/resources/frame.php
A third_party/WebKit/LayoutTests/http/tests/cookies/resources/post-cookies-to-top.php
A third_party/WebKit/LayoutTests/http/tests/cookies/same-site/framed.html
4 files changed, 193 insertions(+), 4 deletions(-)
To view, visit change 1025772. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
Hey Alex, Arthur: this patch feels a little hacky (and I don't really understand why we don't have a "site for cookies" from Blink, since the frame request is renderer-initiated). WDYT?
Arthur Sonzogni (Gerrit)
Patch Set 2:
Hey Alex, Arthur: this patch feels a little hacky (and I don't really understand why we don't have a "site for cookies" from Blink, since the frame request is renderer-initiated). WDYT?
It looks good to me. Maybe we want to wrap it in a function in the future, expecially if we add more edge cases.
This function is called for every cross-document navigation with no exceptions. It can be for an iframe and be browser-initiated, I am thinking about a back/forward navigation in an iframe mainly. I don't think blink can always provide |site_for_cookie|.
+CC clamy@ FYI.
Alex Moshchuk (Gerrit)
Thanks, looks good overall, though two quick questions below.
Patch Set 2:
Patch Set 2:
Hey Alex, Arthur: this patch feels a little hacky (and I don't really understand why we don't have a "site for cookies" from Blink, since the frame request is renderer-initiated). WDYT?
It looks good to me. Maybe we want to wrap it in a function in the future, expecially if we add more edge cases.
This function is called for every cross-document navigation with no exceptions. It can be for an iframe and be browser-initiated, I am thinking about a back/forward navigation in an iframe mainly. I don't think blink can always provide |site_for_cookie|.
+CC clamy@ FYI.
Right, there might be cases such as the history one Arthur mentions where we won't be able to rely on a value from Blink, since this codepath is common for both browser and renderer-initiated navigations. Doing the walk here is also nice because we can actually get the top frame's URL consistently, whereas in Blink we have to fall back to the replicated origin when the top frame is remote, which doesn't work correctly with sandboxed documents. (though I guess sandboxed main frames are rare enough that nobody's noticed?)
2 comments:
File content/browser/frame_host/navigation_request.cc:
Patch Set #2, Line 1209:
frame_tree_node_->parent()Looks like in this walk the site comparison starts from the current frame's parent, whereas Document::SiteForCookies() seems to start on the current frame itself. OTOH your tests are passing, so I don't know if it's a bug or not?
Patch Set #2, Line 1213:
current->current_url()Do we care whether this uses the URL vs the last committed origin (i.e., current->current_origin())? The latter corresponds a bit closer to what Document::SiteForCookies uses (current_frame->GetSecurityContext()->GetSecurityOrigin()), but there are the usual gotchas (the URL will work for sandboxed docs whereas the origin won't, the origin will work for about:blank whereas the URL won't, etc.), and I don't know if we care about them here.
To view, visit change 1025772. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
Patch Set 2:
(2 comments)
Thanks, looks good overall, though two quick questions below.
Patch Set 2:
Patch Set 2:
Hey Alex, Arthur: this patch feels a little hacky (and I don't really understand why we don't have a "site for cookies" from Blink, since the frame request is renderer-initiated). WDYT?
It looks good to me. Maybe we want to wrap it in a function in the future, expecially if we add more edge cases.
This function is called for every cross-document navigation with no exceptions. It can be for an iframe and be browser-initiated, I am thinking about a back/forward navigation in an iframe mainly. I don't think blink can always provide |site_for_cookie|.
+CC clamy@ FYI.Right, there might be cases such as the history one Arthur mentions where we won't be able to rely on a value from Blink, since this codepath is common for both browser and renderer-initiated navigations. Doing the walk here is also nice because we can actually get the top frame's URL consistently, whereas in Blink we have to fall back to the replicated origin when the top frame is remote, which doesn't work correctly with sandboxed documents. (though I guess sandboxed main frames are rare enough that nobody's noticed?)
History navigations make sense, but renderer-initiated navigations really should be able to pass information up to the browser about the request's initiator. As noted in one of the comments below, knowing who's responsible for a given navigation makes a difference in edge cases.
If y'all are happy with the general shape of this patch, I'd like to land it, as it's an improvement over the status quo, but I think it would be helpful to reevaluate the way that we're triggering this navigation so that we're not throwing away data in cases where it would distinguish behavior.
2 comments:
Patch Set #2, Line 1209:
frame_tree_node_->parent()
Looks like in this walk the site comparison starts from the current frame's parent, whereas Document […]
The current `FrameTreeNode`'s `current_url()` is empty at this point, so far as I can tell. Is there a way to gather the URL of the document that actually initiated the request at this point? There are some sharp edges here relating to the distinctions between an embedder setting `iframe.src = 'https://whatever/'` vs the embedee setting `window.location = 'https://whatever/'`": I don't think we actually have the data here to make that distinction.
Patch Set #2, Line 1213:
current->current_url()
Do we care whether this uses the URL vs the last committed origin (i.e. […]
We ought to be using the URL because we want to allow sandboxed frames to send requests with cookies (see https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-02#section-5.2.1, which processes "the origin of X's URI" rather than "X's origin"; that's intentional).
Ideally, this will be more correct than what we can do in Blink (and is actually a good argument for setting Blink's "site for cookies" at commit-time rather than calculating it from the Document).
To view, visit change 1025772. To unsubscribe, or for help writing mail filters, visit settings.
Alex Moshchuk (Gerrit)
LGTM
Patch set 2:Code-Review +1
2 comments:
Patch Set #2, Line 1209:
frame_tree_node_->parent()
The current `FrameTreeNode`'s `current_url()` is empty at this point, so far as I can tell. […]
Ah, right, it'll be empty for a new child frame if this is its first navigation. Yeah, we don't have a way to get the initiator URL here. :( Perhaps that's something we can pass in common_params_ for renderer-initiated navigations and use here down the road (along the lines of how andypaicu@ is passing the initiator's CSP in https://crrev.com/c/957726).
Patch Set #2, Line 1213:
current->current_url()
We ought to be using the URL because we want to allow sandboxed frames to send requests with cookies […]
Ack
To view, visit change 1025772. To unsubscribe, or for help writing mail filters, visit settings.
Mike West (Gerrit)
Patch set 2:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
Check ancestors when setting an <iframe> navigation's "site for cookies".
Currently, we're setting the "site for cookies" only by looking at the
top-level document. We ought to be verifying that the ancestor frames
are same-site before doing so. We do this correctly in Blink (see
`Document::SiteForCookies`), but didn't do so when navigating in the
browser.
This patch addresses the majority of the problem by walking the ancestor
chain when processing a NavigationRequest. If all the ancestors are
same-site, we set the "site for cookies" to the top-level document's URL.
If they aren't all same-site, we set it to an empty URL to ensure that
we don't send SameSite cookies.
Bug: 833847
Change-Id: Icd77f31fa618fa9f8b59fc3b15e1bed6ee05aabd
Reviewed-on: https://chromium-review.googlesource.com/1025772
Reviewed-by: Alex Moshchuk <ale...@chromium.org>
Commit-Queue: Mike West <mk...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553942}
---
M content/browser/frame_host/navigation_request.cc
A third_party/WebKit/LayoutTests/http/tests/cookies/resources/frame.php
A third_party/WebKit/LayoutTests/http/tests/cookies/resources/post-cookies-to-top.php
A third_party/WebKit/LayoutTests/http/tests/cookies/same-site/framed.html
4 files changed, 193 insertions(+), 4 deletions(-)