url_loader: Execute inbound Trust Tokens op before opening response pipe

This CL moves the inbound ("Finalize") half of Trust Tokens operation
execution before URLLoader opens its response pipe.

Requests configured for Trust Tokens operations execute outbound
("Begin") and inbound ("Finalize") halves of their Trust Tokens
operation against their request and response headers. The intended
behavior when either half fails is to fail the request altogether.
Currently, due to a bug, requests whose inbound ("Finalize") Trust
Tokens operation halves fail can still report a success to the renderer.
This is because URLLoader::OnResponseStarted opens a request's response
data pipe before executing the request's inbound Trust Tokens operation
half. Later, in URLLoader::NotifyCompleted, the loader sees that the
pipe is open and reports a completed response to the loader's client,
before seeing that there is a failure status and reporting the error.
This means that the client will always see a successful response before
sending the error; the effect of this is that fetch(..., trustToken)
will resolve with a success instead of rejecting with a DOMException in
cases where the request fails on the inbound half of its Trust Tokens
operation.

This CL fixes the issue by moving the inbound half of the Trust Tokens
operation before the data pipe is opened, so that the data pipe will not
open if the inbound half of the Trust Tokens operation fails.

Fixed: 1105696
Change-Id: Icf80e0f2d8d8231ec1c2854087c7afa023422c71
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2303677Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Commit-Queue: David Van Cleve <davidvc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791396}

content/test/trust_token_browsertest.cc

@@ -598,4 +598,26 @@ IN_PROC_BROWSER_TEST_F(TrustTokenBrowsertest,
                                         server_.GetURL("a.test", "/issue"))));
 }
 
+// If a server issues with a key not present in the client's collection of key
+// commitments, the issuance operation should fail.
+IN_PROC_BROWSER_TEST_F(TrustTokenBrowsertest, IssuanceWithAbsentKeyFails) {
+  ProvideRequestHandlerKeyCommitmentsToNetworkService({"a.test"});
+
+  // Reset the handler, so that the client's valid keys disagree with the
+  // server's keys. (This is theoretically flaky, but the chance of the client's
+  // random keys colliding with the server's random keys is negligible.)
+  request_handler_.UpdateOptions(TrustTokenRequestHandler::Options());
+
+  GURL start_url = server_.GetURL("a.test", "/title1.html");
+  ASSERT_TRUE(NavigateToURL(shell(), start_url));
+
+  std::string command = R"(fetch($1, {trustToken: {type: 'token-request'}})
+                             .then(() => "Success")
+                             .catch(error => error.name);)";
+  EXPECT_THAT(
+      EvalJs(shell(), JsReplace(command, server_.GetURL("a.test", "/issue")))
+          .ExtractString(),
+      HasSubstr("OperationError"));
+}
+
 }  // namespace content

services/network/url_loader.cc

@@ -1155,6 +1155,44 @@ void URLLoader::OnResponseStarted(net::URLRequest* url_request, int net_error) {
     return;
   }
 
+  response_ = network::mojom::URLResponseHead::New();
+  PopulateResourceResponse(
+      url_request_.get(), is_load_timing_enabled_,
+      options_ & mojom::kURLLoadOptionSendSSLInfoWithResponse, response_.get());
+  if (report_raw_headers_) {
+    response_->raw_request_response_info = BuildRawRequestResponseInfo(
+        *url_request_, raw_request_headers_, raw_response_headers_.get());
+    raw_request_headers_ = net::HttpRawRequestHeaders();
+    raw_response_headers_ = nullptr;
+  }
+
+  // Parse and remove the Trust Tokens response headers, if any are expected,
+  // potentially failing the request if an error occurs.
+  if (trust_token_helper_) {
+    trust_token_helper_->Finalize(
+        response_.get(),
+        base::BindOnce(&URLLoader::OnDoneFinalizingTrustTokenOperation,
+                       weak_ptr_factory_.GetWeakPtr()));
+    // |this| may have been deleted.
+    return;
+  }
+
+  ContinueOnResponseStarted();
+}
+
+void URLLoader::OnDoneFinalizingTrustTokenOperation(
+    mojom::TrustTokenOperationStatus status) {
+  trust_token_status_ = status;
+
+  if (status != mojom::TrustTokenOperationStatus::kOk) {
+    NotifyCompleted(net::ERR_TRUST_TOKEN_OPERATION_FAILED);
+    // |this| may have been deleted.
+    return;
+  }
+  ContinueOnResponseStarted();
+}
+
+void URLLoader::ContinueOnResponseStarted() {
   MojoCreateDataPipeOptions options;
   options.struct_size = sizeof(MojoCreateDataPipeOptions);
   options.flags = MOJO_CREATE_DATA_PIPE_FLAG_NONE;
@@ -1180,17 +1218,6 @@ void URLLoader::OnResponseStarted(net::URLRequest* url_request, int net_error) {
     upload_progress_tracker_ = nullptr;
   }
 
-  response_ = network::mojom::URLResponseHead::New();
-  PopulateResourceResponse(
-      url_request_.get(), is_load_timing_enabled_,
-      options_ & mojom::kURLLoadOptionSendSSLInfoWithResponse, response_.get());
-  if (report_raw_headers_) {
-    response_->raw_request_response_info = BuildRawRequestResponseInfo(
-        *url_request_, raw_request_headers_, raw_response_headers_.get());
-    raw_request_headers_ = net::HttpRawRequestHeaders();
-    raw_response_headers_ = nullptr;
-  }
-
   peer_closed_handle_watcher_.Watch(
       response_body_stream_.get(), MOJO_HANDLE_SIGNAL_PEER_CLOSED,
       base::BindRepeating(&URLLoader::OnResponseBodyStreamConsumerClosed,
@@ -1220,34 +1247,6 @@ void URLLoader::OnResponseStarted(net::URLRequest* url_request, int net_error) {
     return;
   }
 
-  // Parse and remove the Trust Tokens response headers, if any are expected,
-  // potentially failing the request if an error occurs.
-  if (trust_token_helper_) {
-    trust_token_helper_->Finalize(
-        response_.get(),
-        base::BindOnce(
-            [](base::WeakPtr<URLLoader> loader, net::URLRequest* request,
-               int net_error, mojom::TrustTokenOperationStatus status) {
-              if (!loader)
-                return;
-              if (status != mojom::TrustTokenOperationStatus::kOk) {
-                loader->trust_token_status_ = status;
-                loader->NotifyCompleted(net::ERR_TRUST_TOKEN_OPERATION_FAILED);
-                // |loader| may have been deleted.
-                return;
-              }
-              loader->ContinueOnResponseStarted(request, net_error);
-            },
-            weak_ptr_factory_.GetWeakPtr(), url_request, net_error));
-    // |this| may have been deleted.
-    return;
-  }
-
-  ContinueOnResponseStarted(url_request, net_error);
-}
-
-void URLLoader::ContinueOnResponseStarted(net::URLRequest* url_request,
-                                          int net_error) {
   // Figure out if we need to sniff (for MIME type detection or for Cross-Origin
   // Read Blocking / CORB).
   if (factory_params_->is_corb_enabled && !is_nocors_corb_excluded_request_) {
@@ -1304,8 +1303,8 @@ void URLLoader::ContinueOnResponseStarted(net::URLRequest* url_request,
     // request of the original URL.
     net::IsolationInfo isolation_info = net::IsolationInfo::Create(
         net::IsolationInfo::RedirectMode::kUpdateNothing,
-        url_request->isolation_info().top_frame_origin().value(),
-        url_request->isolation_info().frame_origin().value(),
+        url_request_->isolation_info().top_frame_origin().value(),
+        url_request_->isolation_info().frame_origin().value(),
         net::SiteForCookies());
     origin_policy_manager_->RetrieveOriginPolicy(
         url::Origin::Create(url_request_->url()), isolation_info,

services/network/url_loader.h

@@ -266,7 +266,15 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
   //   - receive the result in OnDoneBeginningTrustTokenOperation;
   //   - if successful, ScheduleStart; if there was an error, fail.
   //
-  // (Inbound, response handling just takes a synchronous Finalize call.)
+  // Inbound control flow:
+  //
+  // Start in OnResponseStarted
+  // - If there are no Trust Tokens parameters, proceed to
+  // ContinueOnResponseStarted.
+  // - Otherwise:
+  //   - execute TrustTokenRequestHelper::Finalize against the helper;
+  //   - receive the result in OnDoneFinalizingTrusttokenOperation;
+  //   - if successful, ContinueOnResponseStarted; if there was an error, fail.
   void BeginTrustTokenOperationIfNecessaryAndThenScheduleStart(
       const ResourceRequest& request);
   void OnDoneConstructingTrustTokenHelper(
@@ -274,9 +282,11 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) URLLoader
       TrustTokenStatusOrRequestHelper status_or_helper);
   void OnDoneBeginningTrustTokenOperation(
       mojom::TrustTokenOperationStatus status);
+  void OnDoneFinalizingTrustTokenOperation(
+      mojom::TrustTokenOperationStatus status);
   // Continuation of |OnResponseStarted| after possibly asynchronously
   // concluding the request's Trust Tokens operation.
-  void ContinueOnResponseStarted(net::URLRequest* url_request, int net_error);
+  void ContinueOnResponseStarted();
 
   void ScheduleStart();
   void ReadMore();