Only dispatch auth requests to extensions once per request

(This is a reland of
https://chromium-review.googlesource.com/c/chromium/src/+/1978898,
with a different test that relies on a slow HTTP response instead of a
long one, to avoid OOMing some trybots.)

With HTTP auth committed interstitials enabled, extensions were
getting notified of each auth request twice: once when LoginHandler is
first called to cancel the auth request and show a blank error page,
and again when LoginHandler is called after committing the error page
to show a login prompt on top of it.

In common operation, the second extension dispatch was a no-op,
because the request had been destroyed already by the second entry
into LoginHandler. But, there was a race ticked by long response
bodies. That is, it was possible that the second onAuthRequired event
would be dispatched to an extension before the request was
destroyed. The request was destroyed between the second dispatch and
the extension's reply. When the extension replied,the WebRequestAPI
would simply drop the reply for the now non-existent request, never
returning control to LoginHandler so that it can proceed to show the
login prompt.

The fix is to ensure that auth requests are dispatched to extensions
only once, on the first time LoginHandler is called, before the
request is cancelled.

Bug: 1034468
Change-Id: I735f9c76bce5788c5362d7d04e6f81fdcd9535d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1978898Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Commit-Queue: Emily Stark <estark@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#727041}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1980766
Cr-Commit-Position: refs/heads/master@{#727664}

chrome/browser/ui/login/login_handler.cc

@@ -143,17 +143,27 @@ void LoginHandler::Start(
   // If the WebRequest API wants to take a shot at intercepting this, we can
   // return immediately. |continuation| will eventually be invoked if the
   // request isn't cancelled.
-  auto* api =
-      extensions::BrowserContextKeyedAPIFactory<extensions::WebRequestAPI>::Get(
-          web_contents()->GetBrowserContext());
-  auto continuation = base::BindOnce(&LoginHandler::MaybeSetUpLoginPrompt,
-                                     weak_factory_.GetWeakPtr(), request_url,
-                                     is_main_frame, mode);
-  if (api->MaybeProxyAuthRequest(web_contents()->GetBrowserContext(),
-                                 auth_info_, std::move(response_headers),
-                                 request_id, is_main_frame,
-                                 std::move(continuation))) {
-    return;
+  //
+  // When committed interstitials are enabled, LoginHandler::Start is called
+  // twice: once when Chrome initially receives the auth challenge, and again
+  // after a blank error page is committed to show a login prompt on top of
+  // it. Extensions should only be notified of each auth request once, and only
+  // before the request is cancelled. Therefore, the WebRequest API is only
+  // notified in PRE_COMMIT mode.
+  if (!base::FeatureList::IsEnabled(
+          features::kHTTPAuthCommittedInterstitials) ||
+      mode == PRE_COMMIT) {
+    auto* api = extensions::BrowserContextKeyedAPIFactory<
+        extensions::WebRequestAPI>::Get(web_contents()->GetBrowserContext());
+    auto continuation = base::BindOnce(&LoginHandler::MaybeSetUpLoginPrompt,
+                                       weak_factory_.GetWeakPtr(), request_url,
+                                       is_main_frame, mode);
+    if (api->MaybeProxyAuthRequest(web_contents()->GetBrowserContext(),
+                                   auth_info_, std::move(response_headers),
+                                   request_id, is_main_frame,
+                                   std::move(continuation))) {
+      return;
+    }
   }
 #endif
 

chrome/browser/ui/login/login_handler.h

@@ -43,6 +43,10 @@ class LoginHandler : public content::LoginDelegate,
   // optionally handled by extensions and then cancelled so that an error page
   // can commit. Post-commit, LoginHandler shows a login prompt on top of the
   // committed error page.
+  //
+  // TODO(https://crbug.com/1036468): once committed interstitials has fully
+  // launched, we can remove the concept of modes and just have two separate
+  // entrypoints to LoginHandler.
   enum HandlerMode {
     PRE_COMMIT,
     POST_COMMIT,

chrome/browser/ui/login/login_handler_browsertest.cc

@@ -9,11 +9,13 @@
 #include "base/bind.h"
 #include "base/feature_list.h"
 #include "base/metrics/field_trial.h"
+#include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/net/proxy_test_utils.h"
 #include "chrome/browser/prerender/prerender_manager.h"
 #include "chrome/browser/prerender/prerender_origin.h"
@@ -39,10 +41,12 @@
 #include "content/public/common/content_features.h"
 #include "content/public/common/network_service_util.h"
 #include "content/public/test/browser_test_utils.h"
+#include "content/public/test/slow_http_response.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "net/base/auth.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/http_response.h"
 #include "services/network/public/cpp/features.h"
 #include "url/gurl.h"
 
@@ -56,6 +60,50 @@ using content::Referrer;
 
 namespace {
 
+// A slow HTTP response that serves a WWW-Authenticate header and 401 status
+// code.
+class SlowAuthResponse : public content::SlowHttpResponse {
+ public:
+  explicit SlowAuthResponse(const std::string& relative_url)
+      : content::SlowHttpResponse(relative_url) {}
+  ~SlowAuthResponse() override = default;
+
+  SlowAuthResponse(const SlowAuthResponse& other) = delete;
+  SlowAuthResponse operator=(const SlowAuthResponse& other) = delete;
+
+  // content::SlowHttpResponse:
+  void AddResponseHeaders(std::string* response) override {
+    response->append("WWW-Authenticate: Basic realm=\"test\"\r\n");
+    response->append("Cache-Control: max-age=0\r\n");
+    // Content-length and Content-type are both necessary to trigger the bug
+    // that this class is used to test. Specifically, there must be a delay
+    // between the OnAuthRequired notification from the net stack and when the
+    // response body is ready, and the OnAuthRequired notification requires
+    // headers to be complete (which requires a known content type and length).
+    response->append("Content-type: text/html");
+    response->append(
+        base::StringPrintf("Content-Length: %d\r\n",
+                           kFirstResponsePartSize + kSecondResponsePartSize));
+  }
+
+  void SetStatusLine(std::string* response) override {
+    response->append("HTTP/1.1 401 Unauthorized\r\n");
+  }
+};
+
+// This request handler returns a WWW-Authenticate header along with a slow
+// response body. It is used to exercise a race in how auth requests are
+// dispatched to extensions (https://crbug.com/1034468).
+std::unique_ptr<net::test_server::HttpResponse> HandleBasicAuthSlowResponse(
+    const net::test_server::HttpRequest& request) {
+  std::unique_ptr<SlowAuthResponse> response =
+      std::make_unique<SlowAuthResponse>(request.relative_url);
+  if (!response->IsHandledUrl()) {
+    return nullptr;
+  }
+  return response;
+}
+
 // This helper function sets |notification_fired| to true if called. It's used
 // as an observer callback for notifications that are not expected to fire.
 bool FailIfNotificationFires(bool* notification_fired) {
@@ -2138,4 +2186,112 @@ IN_PROC_BROWSER_TEST_F(ProxyBrowserTestWithHttpAuthCommittedInterstitials,
       TestProxyAuth(browser(), embedded_test_server()->GetURL("/simple.html")));
 }
 
+class LoginPromptExtensionBrowserTest
+    : public extensions::ExtensionBrowserTest,
+      public testing::WithParamInterface<SplitAuthCacheByNetworkIsolationKey> {
+ public:
+  LoginPromptExtensionBrowserTest() {
+    if (GetParam() == SplitAuthCacheByNetworkIsolationKey::kFalse) {
+      scoped_feature_list_.InitWithFeatures(
+          // enabled_features
+          {features::kHTTPAuthCommittedInterstitials},
+          // disabled_features
+          {network::features::kSplitAuthCacheByNetworkIsolationKey});
+    } else {
+      scoped_feature_list_.InitWithFeatures(
+          // enabled_features
+          {features::kHTTPAuthCommittedInterstitials,
+           network::features::kSplitAuthCacheByNetworkIsolationKey},
+          // disabled_features
+          {});
+    }
+  }
+
+  ~LoginPromptExtensionBrowserTest() override = default;
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+INSTANTIATE_TEST_SUITE_P(
+    All,
+    LoginPromptExtensionBrowserTest,
+    ::testing::Values(SplitAuthCacheByNetworkIsolationKey::kFalse,
+                      SplitAuthCacheByNetworkIsolationKey::kTrue));
+
+// Tests that with committed interstitials, extensions are notified once per
+// request when auth is required. Regression test for https://crbug.com/1034468.
+IN_PROC_BROWSER_TEST_P(LoginPromptExtensionBrowserTest,
+                       OnAuthRequiredNotifiedOnce) {
+  embedded_test_server()->RegisterRequestHandler(
+      base::BindRepeating(&HandleBasicAuthSlowResponse));
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Load an extension that logs to the console each time onAuthRequired is
+  // called. We attach a console observer so that we can verify that the
+  // extension only logs once per request.
+  const extensions::Extension* extension =
+      LoadExtension(test_data_dir_.AppendASCII("log_auth_required"));
+  ASSERT_TRUE(extension);
+  content::WebContentsConsoleObserver console_observer(
+      extensions::ProcessManager::Get(profile())
+          ->GetBackgroundHostForExtension(extension->id())
+          ->host_contents());
+
+  // Navigate to a page that prompts for basic auth and then hangs.
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  NavigationController* controller = &contents->GetController();
+  WindowedAuthNeededObserver auth_needed_waiter(controller);
+  GURL test_page =
+      embedded_test_server()->GetURL(SlowAuthResponse::kSlowResponseUrl);
+  ui_test_utils::NavigateToURL(browser(), test_page);
+
+  console_observer.Wait();
+  ASSERT_EQ(1u, console_observer.messages().size());
+  EXPECT_EQ(base::ASCIIToUTF16("onAuthRequired " + test_page.spec()),
+            console_observer.messages()[0].message);
+
+  // Trigger a background request to end the response that prompted for basic
+  // auth.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(),
+      embedded_test_server()->GetURL(SlowAuthResponse::kSlowResponseHostName,
+                                     SlowAuthResponse::kFinishSlowResponseUrl),
+      WindowOpenDisposition::NEW_BACKGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+
+  // If https://crbug.com/1034468 regresses, the test may hang here. In that
+  // bug, extensions were getting notified of each auth request twice, and the
+  // extension must handle the auth request both times before LoginHandler
+  // proceeds to show the login prompt. Usually, the request is fully destroyed
+  // before the second extension dispatch, so the second extension dispatch is a
+  // no-op. But when there is a delay between the OnAuthRequired notification
+  // and the response body being read (as provided by SlowAuthResponse), the
+  // WebRequestAPI is notified that the request is destroyed between the second
+  // dispatch to an extension and when the extension replies. When this happens,
+  // the LoginHandler is never notified that it can continue to show the login
+  // prompt, so the auth needed notification that we are waiting for <will never
+  // come. The fix to this bug is to ensure that extensions are notified of each
+  // auth request only once; this test verifies that condition by checking that
+  // the auth needed notification comes as expected and that the test extension
+  // only logs once for onAuthRequired.
+  auth_needed_waiter.Wait();
+  // No second console message should have been logged, because extensions
+  // should only be notified of the auth request once.
+  EXPECT_EQ(1u, console_observer.messages().size());
+
+  // It's possible that a second message was in fact logged, but the observer
+  // hasn't heard about it yet. Navigate to a different URL and wait for the
+  // corresponding console message, to "flush" any possible second message from
+  // the current page load.
+  WindowedAuthNeededObserver second_auth_needed_waiter(controller);
+  GURL second_test_page = embedded_test_server()->GetURL("/auth-basic");
+  ui_test_utils::NavigateToURL(browser(), second_test_page);
+  second_auth_needed_waiter.Wait();
+  ASSERT_EQ(2u, console_observer.messages().size());
+  EXPECT_EQ(base::ASCIIToUTF16("onAuthRequired " + second_test_page.spec()),
+            console_observer.messages()[1].message);
+}
+
 }  // namespace

chrome/test/data/extensions/log_auth_required/log_auth_required.js

+// Copyright 2019 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.
+
+chrome.webRequest.onAuthRequired.addListener((_details, callback) => {
+  console.log("onAuthRequired", _details.url);
+  callback({});
+}, { urls: ["<all_urls>"] }, ["asyncBlocking"]);

chrome/test/data/extensions/log_auth_required/manifest.json

+{
+  "background": {
+    "scripts": ["log_auth_required.js"]
+  },
+  "permissions": [
+    "tabs",
+    "webRequest",
+    "webRequestBlocking",
+    "<all_urls>"
+  ],
+  "manifest_version": 2,
+  "name": "Log onAuthRequired",
+  "version": "1"
+}