OOPIFs: Fixing submitting forms targeting a remote frame.

third_party/WebKit/Source/core/frame/RemoteFrame.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/frame/RemoteFrame.h"
 
 #include "bindings/core/v8/WindowProxy.h"
 #include "bindings/core/v8/WindowProxyManager.h"
 #include "core/dom/RemoteSecurityContext.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/RemoteDOMWindow.h"
 #include "core/frame/RemoteFrameClient.h"
 #include "core/frame/RemoteFrameView.h"
 #include "core/html/HTMLFrameOwnerElement.h"
 #include "core/loader/FrameLoadRequest.h"
+#include "core/loader/FrameLoader.h"
 #include "core/paint/PaintLayer.h"
 #include "platform/PluginScriptForbiddenScope.h"
 #include "platform/UserGestureIndicator.h"
 #include "platform/graphics/GraphicsLayer.h"
+#include "platform/network/ResourceRequest.h"
 #include "platform/weborigin/SecurityPolicy.h"
 #include "public/platform/WebLayer.h"
 
 namespace blink {
 
 inline RemoteFrame::RemoteFrame(RemoteFrameClient* client, FrameHost* host, FrameOwner* owner)
     : Frame(client, host, owner)
     , m_view(nullptr)
     , m_securityContext(RemoteSecurityContext::create())
     , m_domWindow(RemoteDOMWindow::create(*this))
@@ skipping 29 matching lines @@
 WindowProxy* RemoteFrame::windowProxy(DOMWrapperWorld& world)
 {
     WindowProxy* windowProxy = m_windowProxyManager->windowProxy(world);
     ASSERT(windowProxy);
     windowProxy->initializeIfNeeded();
     return windowProxy;
 }
 
 void RemoteFrame::navigate(Document& originDocument, const KURL& url, bool replaceCurrentItem, UserGestureStatus userGestureStatus)
 {
-    // The process where this frame actually lives won't have sufficient information to determine
-    // correct referrer, since it won't have access to the originDocument. Set it now.
-    ResourceRequest request(url);
-    request.setHTTPReferrer(SecurityPolicy::generateReferrer(originDocument.getReferrerPolicy(), url, originDocument.outgoingReferrer()));

[Łukasz Anforowicz, 2016/06/08 20:32:43]

In theory, I could have just moved this setHTTPReferrer call to the other
::navigate overload, but instead I decided to replace it with a call to
FrameLoader::setReferrerForFrameRequest.  This is because
FrameLoader::setReferrerForFrameRequest seems more complete + calling it seems
good from code deduplication / DRY perspective.  For example
FrameLoader::setReferrerForFrameRequest also calls
ResourceRequest::addHTTPOriginIfNeeded + consults
FrameLoadRequest::getShouldSendReferrer (which defaults to MaybeSendReferrer,
but still...).

-    request.setHasUserGesture(userGestureStatus == UserGestureStatus::Active);
-    client()->navigate(request, replaceCurrentItem);
+    FrameLoadRequest frameRequest(&originDocument);

[Nate Chapin, 2016/06/09 23:40:43]

frameRequest(&originDocument, url) is fine here instead of calling setURL()
later, KURL->ResourceRequest is a permitted implicit conversion.

[Łukasz Anforowicz, 2016/06/10 16:17:25]

Done.

+    frameRequest.setReplacesCurrentItem(replaceCurrentItem);
+    frameRequest.resourceRequest().setURL(url);
+    frameRequest.resourceRequest().setHasUserGesture(userGestureStatus == UserGestureStatus::Active);

[Łukasz Anforowicz, 2016/06/08 20:32:43]

Calling setHasUserGesture twice (in both overloads of ::navigate) seems ok -
once it this method flips |m_hasUserGesture| to true, it will stay set to true
going forward:

void ResourceRequest::setHasUserGesture(bool hasUserGesture)
{
    m_hasUserGesture |= hasUserGesture;
}


Calling setHasUserGesture twice (in both overloads of ::navigate) is needed to:

1) preserve behavior of ::navigate(Document&, KURL&, ...) overload, which needs
to use the |userGestureStatus| argument.

2) make sure we also set user gesture indicator when going only through
::navigate(FrameLoadRequest&) overload (something that wasn't possible before
this CL).


OTOH, I am not sure how important this is - after all, this ResourceRequest is
only ever passed to RemoteFrameProxy::navigate which doesn't look at the
contents of the user gesture indicator *inside* ResourceRequest / WebURLRequest,
but instead directly inspects WebUserGestureIndicator::isProcessingUserGesture()
and unconditionally calls WebUserGestureIndicator::consumeUserGesture().

+    navigate(frameRequest);
 }
 
 void RemoteFrame::navigate(const FrameLoadRequest& passedRequest)
 {
-    UserGestureStatus gesture = UserGestureIndicator::processingUserGesture() ? UserGestureStatus::Active : UserGestureStatus::None;
-    navigate(*passedRequest.originDocument(), passedRequest.resourceRequest().url(), passedRequest.replacesCurrentItem(), gesture);
+    FrameLoadRequest frameRequest(passedRequest);
+
+    // The process where this frame actually lives won't have sufficient information to determine
+    // correct referrer, since it won't have access to the originDocument. Set it now.
+    FrameLoader::setReferrerForFrameRequest(frameRequest);
+
+    frameRequest.resourceRequest().setHasUserGesture(UserGestureIndicator::processingUserGesture());
+    client()->navigate(frameRequest.resourceRequest(), frameRequest.replacesCurrentItem());
 }

[Łukasz Anforowicz, 2016/06/08 20:32:43]

The changes here make navigate(const FrameLoadRequest&) the main overload that
everything goes through (i.e. the overload that actually calls out into
RemoteFrameClient::navigate(ResourceRequest&, bool).

I've talked to Nasko and he says that it is accidental that the old code of
navigate(const FrameLoadRequest&) used to copy only a few pieces from the old
request (i.e. there were no security reasons for doing this / this was mostly
done for code reuse).  This is great - in this CL we actually need to forward
the whole request (i.e. we care about keeping the HTTP body).

 
 void RemoteFrame::reload(FrameLoadType frameLoadType, ClientRedirectPolicy clientRedirectPolicy)
 {
     client()->reload(frameLoadType, clientRedirectPolicy);
 }
 
 void RemoteFrame::detach(FrameDetachType type)
 {
     PluginScriptForbiddenScope forbidPluginDestructorScripting;
     detachChildren();
@@ skipping 92 matching lines @@
     ASSERT(owner());
     toHTMLFrameOwnerElement(owner())->setNeedsCompositingUpdate();
 }
 
 void RemoteFrame::advanceFocus(WebFocusType type, LocalFrame* source)
 {
     client()->advanceFocus(type, source);
 }
 
 } // namespace blink