Only dispatch auth requests to extensions once per request

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: If9eb71b38dd1738a471c3dadab4093367ab0d03f
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-Commit-Position: refs/heads/master@{#727041}

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

@@ -14,6 +14,7 @@
 #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"
@@ -43,6 +44,7 @@
 #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 +58,24 @@ using content::Referrer;
 
 namespace {
 
+// This request handler returns a WWW-Authenticate header along with a long
+// 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>
+BasicAuthLongResponseRequestHandler(
+    const net::test_server::HttpRequest& request) {
+  net::test_server::BasicHttpResponse* response =
+      new net::test_server::BasicHttpResponse();
+  response->set_code(net::HTTP_UNAUTHORIZED);
+  response->AddCustomHeader("WWW-Authenticate", "Basic realm=\"test\"");
+  // There is no magic number for how long the response body should be; it's
+  // chosen to tickle the race described in the aforementioned bug, which
+  // triggers on long responses.
+  std::string body(500000000, '1');
+  response->set_content(body);
+  return std::unique_ptr<net::test_server::HttpResponse>(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 +2158,98 @@ 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(&BasicAuthLongResponseRequestHandler));
+  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.
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  NavigationController* controller = &contents->GetController();
+  WindowedAuthNeededObserver auth_needed_waiter(controller);
+  GURL test_page = embedded_test_server()->GetURL("/");
+  ui_test_utils::NavigateToURL(browser(), test_page);
+
+  // 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 long response body (as provided by
+  // BasicAuthLongResponseRequestHandler), 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();
+  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("/second-page");
+  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 " + test_page.spec()),
+            console_observer.messages()[0].message);
+  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"
+}