Forwarding POST body into renderer after a cross-site transfer.

content/browser/frame_host/navigation_handle_impl.cc

Index: content/browser/frame_host/navigation_handle_impl.cc
diff --git a/content/browser/frame_host/navigation_handle_impl.cc b/content/browser/frame_host/navigation_handle_impl.cc
index 240eba7416107135f096249c2f9b94954e080c84..c1c9fa188d2ec12d9c0a015500c089f5f2f60ea9 100644
--- a/content/browser/frame_host/navigation_handle_impl.cc
+++ b/content/browser/frame_host/navigation_handle_impl.cc
@@ -12,6 +12,7 @@
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_navigation_handle.h"
 #include "content/common/frame_messages.h"
+#include "content/common/resource_request_body.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_client.h"
@@ -144,10 +145,10 @@ const base::TimeTicks& NavigationHandleImpl::NavigationStart() {
   return navigation_start_;
 }
 
-bool NavigationHandleImpl::IsPost() {
+const std::string& NavigationHandleImpl::GetMethod() {
   CHECK_NE(INITIAL, state_)
       << "This accessor should not be called before the request is started.";
-  return method_ == "POST";
+  return method_;
 }
 
 const Referrer& NavigationHandleImpl::GetReferrer() {
@@ -250,7 +251,21 @@ NavigationHandleImpl::CallWillStartRequestForTesting(
     ui::PageTransition transition,
     bool is_external_protocol) {
   NavigationThrottle::ThrottleCheckResult result = NavigationThrottle::DEFER;
-  WillStartRequest(is_post ? "POST" : "GET", sanitized_referrer,
+
+  scoped_refptr<ResourceRequestBody> resource_request_body;
+  std::string method;
+  if (is_post) {
+    method = "POST";
+
+    std::string body = "test=body";
+    resource_request_body = new ResourceRequestBody();
+    resource_request_body->AppendBytes(body.data(), body.size());
+  } else {
+    method = "GET";

[Charlie Reis, 2016/05/31 21:08:36]

Maybe we can just initialize |method| to "GET" and leave out the else block? 
Leaving it uninitialized above always makes me nervous when people might add
another branch to the if/else (or make it an else if) and forget to initialize
it.

[Łukasz Anforowicz, 2016/05/31 22:40:55]

Done.

+    resource_request_body = nullptr;
+  }
+
+  WillStartRequest(method, resource_request_body, sanitized_referrer,
                    has_user_gesture, transition, is_external_protocol,
                    base::Bind(&UpdateThrottleCheckResult, &result));
 
@@ -289,13 +304,18 @@ void NavigationHandleImpl::InitServiceWorkerHandle(
 
 void NavigationHandleImpl::WillStartRequest(
     const std::string& method,
+    const scoped_refptr<content::ResourceRequestBody>& resource_request_body,
     const Referrer& sanitized_referrer,
     bool has_user_gesture,
     ui::PageTransition transition,
     bool is_external_protocol,
     const ThrottleChecksFinishedCallback& callback) {
+  // |method != "POST"| should imply absence of |resource_request_body|.
+  DCHECK(method == "POST" || !resource_request_body);

[Charlie Reis, 2016/05/31 21:08:36]

Maybe we should upgrade this to a CHECK?  Seems like we should be careful not to
accidentally send a POST body to the renderer due to unrelated bugs.

[Łukasz Anforowicz, 2016/05/31 22:40:55]

Maybe we should try this in a follow-up, smaller (= more easily revertable) CL?


I also have a few questions about adding the CHECK:

1. Should we promote the CommonNavigationParams DCHECK instead (this is where we
construct the IPC payload for renderer;  above it just network stack processing
[which also covers subresources (?) or other things where http body won't get
sent to the renderer process]).

2. I agree with crashing the browser if we detect an obviously security problem
(i.e. browser is confused about origin of frames).  Is sending of POST body the
same level of a security issue (i.e. something that we want to crash upon)?


Also - when thinking about this, I've found out that only GET and POST are
supported for page navigations (so - i.e. we don't have to worry about not
CHECKing/crashing for PUT [which would contain a HTTP body]).  So - I wonder if
I should maybe prefer |bool uses_post| over |std::string method| in my other CLs
(i.e. in https://codereview.chromium.org/2022483003/).

[Charlie Reis, 2016/06/01 23:46:32]

Sure.  Any CHECKs we add can happen in a separate, revertable CL.
Yes, that seems like the right place.
Assuming that a renderer can't lie to make this happen (and that it requires a
browser process bug to trigger), then yes, I think we should crash the browser
if it's about to send a POST to a renderer that shouldn't receive it.  That
risks information disclosure (violating our Site Isolation goals) in some cases,
and in all cases indicates a logic bug that might otherwise be concerning.  I
think that's worthwhile to verify.
See my other response.  :)

[Łukasz Anforowicz, 2016/06/02 22:07:04]

Ack.
Hmmm... maybe the renderer can manage to send a body for a non-POST method.  I
am not sure if this is possible (feels like something that we should already vet
before starting the request), but I've added some checks against this in the
latest patchset in NavigationHandleImpl::WillStartRequest (symmetrical / similar
to the check already present in NavigationHandleImpl::WillRedirectRequest).

+
   // Update the navigation parameters.
   method_ = method;
+  resource_request_body_ = resource_request_body;
   sanitized_referrer_ = sanitized_referrer;
   has_user_gesture_ = has_user_gesture;
   transition_ = transition;
@@ -339,6 +359,8 @@ void NavigationHandleImpl::WillRedirectRequest(
   is_external_protocol_ = new_is_external_protocol;
   response_headers_ = response_headers;
   was_redirected_ = true;
+  if (new_method != "POST")
+    resource_request_body_ = nullptr;
 
   state_ = WILL_REDIRECT_REQUEST;
   complete_callback_ = callback;