Skip to content

GitLab

T
tangled
Commit 93781c70 authored by Emily Stark's avatar Emily Stark Committed by Commit Bot
Browse files
Options

Fix HTTP auth cancellation tracking when there's no visible entry 

When an HTTP auth prompt is cancelled, we close the prompt and refresh
the page to retrieve the 401 error body from the server. There is some
logic in LoginNavigationThrottle/LoginTabHelper that tracks/detects
these refreshes to handle them specially, i.e. not re-show an auth
prompt when the refresh commits. This logic was looking at
NavigationController:GetVisibleEntry() to determine when an observed
navigation was one of these refresh navigations -- but a visible entry
is not always guaranteed to exist, leading to crashes. One correct fix
would be to check for a null visible entry, but instead we now look at
the pending entry. This is more specific because the pending entry is
the one that would be for the refresh.

Note: this fix relies on the assumption that GetPendingEntry() will
always be non-null during a main-frame WillProcessResponse(). I think
this condition is guaranteed, but if I'm wrong, it will show up as
crashes when dereferencing GetPendingEntry(). In that case we'll need
to add a check for a null GetPendingEntry().

Bug: 1005096
Change-Id: If4a72ca903e14da588c0ee515648e8d2694f8612
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1819331
Commit-Queue: Emily Stark <estark@chromium.org>
Reviewed-by: default avatarCarlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699074}
parent bc30a3aa
Hide whitespace changes
Inline Side-by-side
Showing with 57 additions and 2 deletions
chrome/browser/ui/login/login_handler_browsertest.cc
View file @ 93781c70
......@@ -1694,6 +1694,52 @@ IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest,
EXPECT_EQ(expected_title, auth_supplied_title_watcher.WaitAndGetTitle());
}
// Tests that when HTTP Auth committed interstitials are enabled, showing a
// login prompt in a new window opened from window.open() does not
// crash. Regression test for https://crbug.com/1005096.
IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, PromptWithNoVisibleEntry) {
ASSERT_TRUE(embedded_test_server()->Start());
content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();
ui_test_utils::NavigateToURL(browser(), GURL("about:blank"));
// Open a new window via JavaScript and navigate it to a page that delivers an
// auth prompt.
GURL test_page = embedded_test_server()->GetURL(kAuthBasicPage);
ASSERT_NE(false, content::EvalJs(contents, "w = window.open();"));
content::WebContents* opened_contents =
browser()->tab_strip_model()->GetWebContentsAt(1);
NavigationController* opened_controller = &opened_contents->GetController();
ASSERT_FALSE(opened_controller->GetVisibleEntry());
LoginPromptBrowserTestObserver observer;
observer.Register(content::Source<NavigationController>(opened_controller));
WindowedAuthNeededObserver auth_needed_waiter(opened_controller);
ASSERT_NE(false, content::EvalJs(contents, "w.location.href = '" +
test_page.spec() + "';"));
// Test that the login prompt displays above an empty page.
EXPECT_EQ(
"<head></head><body></body>",
content::EvalJs(opened_contents, "document.documentElement.innerHTML"));
auth_needed_waiter.Wait();
ASSERT_EQ(1u, observer.handlers().size());
// Test that credentials are handled correctly.
WindowedAuthSuppliedObserver auth_supplied_waiter(opened_controller);
LoginHandler* handler = *observer.handlers().begin();
SetAuthFor(handler);
auth_supplied_waiter.Wait();
base::string16 expected_title = ExpectedTitleFromAuth(
base::ASCIIToUTF16("basicuser"), base::ASCIIToUTF16("secret"));
content::TitleWatcher auth_supplied_title_watcher(opened_contents,
expected_title);
EXPECT_EQ(expected_title, auth_supplied_title_watcher.WaitAndGetTitle());
}
// Tests that FTP auth challenges appear over a blank committed interstitial.
IN_PROC_BROWSER_TEST_F(LoginPromptBrowserTest, FtpAuth) {
net::SpawnedTestServer ftp_server(
......
chrome/browser/ui/login/login_tab_helper.cc
View file @ 93781c70
......@@ -159,8 +159,17 @@ LoginTabHelper::WillProcessMainFrameUnauthorizedResponse(
content::NavigationHandle* navigation_handle) {
// If the user has just cancelled the auth prompt for this navigation, then
// the page is being refreshed to retrieve the 401 body from the server, so
// allow the refresh to proceed.
if (web_contents()->GetController().GetVisibleEntry()->GetUniqueID() ==
// allow the refresh to proceed. The entry to compare against is the pending
// entry, because while refreshing after cancelling the prompt, the page that
// showed the prompt will be the pending entry until the refresh
// commits. Comparing against GetVisibleEntry() would also work, but it's less
// specific and not guaranteed to exist in all cases (e.g., in the case of
// navigating a window just opened via window.open()).
//
// TODO(https://crbug.com/1006955): if this line is crashing, the assumption
// that GetPendingEntry() must be non-null is incorrect, in which case a null
// check should be added here.
if (web_contents()->GetController().GetPendingEntry()->GetUniqueID() ==
navigation_entry_id_with_cancelled_prompt_) {
// Note the navigation handle ID so that when this refresh navigation
// finishes, DidFinishNavigation declines to show another login prompt. We
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment