-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Ensure iframe requests include Referer when location.replace or location.assign is called #19093
Conversation
EWS run on previous version of this PR (hash 64a2c7d) |
64a2c7d
to
6062b09
Compare
EWS run on previous version of this PR (hash 6062b09)
|
6062b09
to
6639ed3
Compare
EWS run on previous version of this PR (hash 6639ed3) |
6639ed3
to
a771aeb
Compare
EWS run on previous version of this PR (hash a771aeb) |
a771aeb
to
e535e9a
Compare
e535e9a
to
cb9d9cb
Compare
EWS run on previous version of this PR (hash cb9d9cb) |
EWS run on previous version of this PR (hash e535e9a) |
cb9d9cb
to
4111f29
Compare
EWS run on previous version of this PR (hash 4111f29) |
@@ -2461,6 +2461,13 @@ void LocalDOMWindow::setLocation(LocalDOMWindow& activeWindow, const URL& comple | |||
if (completedURL.protocolIsJavaScript() && frameElement() && !frameElement()->document().contentSecurityPolicy()->allowJavaScriptURLs(aboutBlankURL().string(), { }, completedURL.string(), frameElement())) | |||
return; | |||
|
|||
RefPtr mainFrame = dynamicDowncast<LocalFrame>(frame->mainFrame()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think we want the parent frame rather than the main frame. If a.com has an iframe with b.com that has an iframe with c.com, then a.com didn't "refer" to c.com. b.com did, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anne seems to disagree. He's probably right, but I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your scenario is for the initial navigation which is based on the attribute value. That's different. In this case it's really about who called the method and there's no reason for it to be different from the other Location
methods. (Which in part is why I argued for refactoring it so they all invoke the same primitive.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, further testing indicates that the parent frame is actually what Gecko and Blink set the Referer from.
So, because the purpose of this change has, so far, been to achieve interoperability/compat with Gecko and Blink, I’ve updated the implementation to use the parent frame rather than the “main” frame — and I’ve added more tests.
If a.com has an iframe with b.com that has an iframe with c.com, then a.com didn't "refer" to c.com. b.com did, right?
Right it seems — yes, as far as what happens when script running in b.com calls location.replace
or location.assign
are called on a c.com iframe.
Specifically:
- The
assign-with-nested-iframe.html
andreplace-with-nested-iframe.html
tests I added are the a.com top-level documents that have a b.com iframe (resources/replace-or-assign-call-on-iframe.html
) that in turn also has an iframe (c.com). - Script running in the b.com (
resources/replace-or-assign-call-on-iframe.html
) iframe callslocation.replace
orlocation.assign
on the c.com iframe - The Referer for the c.com request in Gecko and Blink is the parent b.com (
resources/replace-or-assign-call-on-iframe.html
) iframe — not the top-levelassign-with-nested-iframe.html
andreplace-with-nested-iframe.html
documents (“main” frames).
It’s not clear to me which part of the spec states what the required behavior is for that case — and not clear to me if Gecko and Blink behavior is violating the relevant spec requirements.
But if we want to have interop/compat with Gecko and Blink on this, then it seems we need to use the parent frame as the referrer in the above case. And from the spec side, if it seems desirable to have the spec match the existing Gecko and Blink behavior, and the spec needs to change to do that — then I’d be happy to also work on spec patch.
If on the other hand, the existing Gecko and Blink behavior here seems undesirable — if it seems we don’t want WebKit to match that — then I could change the implementation here back to using whatever the current spec actually requires (which is maybe the “main” window, but I’m still actually not clear if that’s what the spec says…).
And in that case, I guess I’d also need to raise Gecko and Blink bugs to try to get them to change (again, with the goal still being to get interop on this).
// to the URL of its main frame's Document — which is the incumbent global object's associated Document. | ||
// https://html.spec.whatwg.org/#location-object-navigate | ||
if (RefPtr activeFrame = activeWindow.frame(); activeFrame && activeFrame->loader().outgoingReferrer().isEmpty() && mainFrame) | ||
activeFrame->loader().setOutgoingReferrer(mainFrame->document()->urlForBindings()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
urlForBindings looks a little strange here. This is not the bindings. We're also missing a call to strippedForUseAsReferrer. Also, I think we may want to restrict this to just the origin, rather than sending the whole path and query as a referrer.
Could you also add tests for the cases I mentioned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
urlForBindings looks a little strange here. This is not the bindings.
Thanks, yeah, changed to just url()
We're also missing a call to strippedForUseAsReferrer
OK, added
Also, I think we may want to restrict this to just the origin, rather than sending the whole path and query as a referrer.
Gecko and Blink send the whole path and query — so in order to have interop/compat with their existing behavior, the change here matches that.
But if we decide it’s desirable to not send the whole path and query, then I could switch the implementation to doing that — and then, I guess, raise Gecko and Blink bugs to try to get them to change (again, with the goal still being to get interop on this).
Could you also add tests for the cases I mentioned?
See my previous comment — I added more tests (and the added tests show that Gecko and Blink are using the parent frame URL to set the Referer, not the “main” frame URL).
4111f29
to
533bf02
Compare
EWS run on previous version of this PR (hash 533bf02) |
// If the loader for activeWindow's frame (browsing context) has no outgoing referrer, set its outgoing referrer | ||
// to the URL of its parent frame's Document. | ||
if (RefPtr activeFrame = activeWindow.frame(); activeFrame && activeFrame->loader().outgoingReferrer().isEmpty() && parent) | ||
activeFrame->loader().setOutgoingReferrer(document()->completeURL(dynamicDowncast<LocalFrame>(parent)->document()->url().strippedForUseAsReferrer())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for diligently looking into this.
If the parent is not a LocalFrame, we don't want to crash. This would happen if the referrer is empty and the iframe is cross-site and site-isolation (currently an experimental feature) is turned on. Please change to something like this before merging:
RefPtr localParent = dynamicDowncast(frame->tree().parent());
if (... && localParent)
...completeURL(localParent->document()...)
533bf02
to
6d41cba
Compare
EWS run on current version of this PR (hash 6d41cba) |
@sideshowbarker - Anything to do or good to go? Happy to add 'merge-queue'. |
Nothing more to do — all good to go — so please do add, thanks! |
…ion.assign is called https://bugs.webkit.org/show_bug.cgi?id=263072 Reviewed by Chris Dumez and Alex Christensen. When the loader for a Frame (e.g, an iframe) has no referrer, then this change causes the loader's referrer to be set to the URL of its parent Frame’s Document. That in turn ensures the associated request is sent with a Referer header — including in the case where a document calls location.replace or location.assign on an iframe element — which makes the WebKit behavior in this case interoperable with existing behavior in Gecko and Blink. Otherwise, without this change, no Referer header is sent in the associated request, which breaks interoperability with Gecko and Blink. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/assign-replace-from-iframe-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/assign-replace-from-iframe.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/assign-replace-from-top-to-nested-iframe-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/assign-replace-from-top-to-nested-iframe.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/assign-with-nested-iframe-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/assign-with-nested-iframe.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/replace-with-nested-iframe-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/replace-with-nested-iframe.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/resources/iframe-contents.sub.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/resources/iframe-postmessage-to-parent-parent.sub.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/resources/iframe-with-iframe.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/resources/replace-or-assign-call-on-iframe.html: Added. * LayoutTests/imported/w3c/web-platform-tests/html/browsers/the-window-object/open-close/no_window_open_when_term_nesting_level_nonzero.window-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections/promise-rejection-events-expected.txt: * Source/WebCore/page/LocalDOMWindow.cpp: (WebCore::LocalDOMWindow::setLocation): Canonical link: https://commits.webkit.org/270741@main
6d41cba
to
1350b59
Compare
Committed 270741@main (1350b59): https://commits.webkit.org/270741@main Reviewed commits have been landed. Closing PR #19093 and removing active labels. |
1350b59
6d41cba