Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
410373003:
Fix cross origin check when deciding to show the HTTP auth interstitial. (Closed)
|
Created:
6 years, 5 months ago by meacer Modified:
6 years, 4 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFix cross origin check when deciding to show the HTTP auth interstitial.
HTTP auth prompts show a blank interstitial which causes the omnibox to show
the correct URL and page contents to clear in order to prevent phishing. The
logic for this interstitial currently checks only for the visible URL. This misses
the case where the user navigates directly the URL: The URL of the resource
with HTTP Auth will be the same as the visible entry so no interstitial will be
shown. This patch adds a check to see if the last committed entry is also
different then the URL of the resource with HTTP Auth.
BUG=396961
TEST=LoginPromptBrowserTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290090
Patch Set 1 #Patch Set 2 : Add test #Patch Set 3 : Fix test #
Total comments: 4
Patch Set 4 : Check only LastCommittedURL #
Total comments: 13
Patch Set 5 : pkasting and nasko comments #Patch Set 6 : Fix WebSocket test #Patch Set 7 : Fix mac test #Patch Set 8 : Change test name #
Messages
Total messages: 20 (0 generated)
Nasko, here is another http auth bug. Can you please see if the cross origin check is reasonable?
LGTM
Nasko: I added a test case. Peter: Can you please take a look as an owner? This is one of the few HTTP auth bugs. Thanks.
https://codereview.chromium.org/410373003/diff/40001/chrome/browser/ui/login/... File chrome/browser/ui/login/login_prompt.cc (right): https://codereview.chromium.org/410373003/diff/40001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt.cc:516: request_url.GetOrigin())) { It seems a bit weird that we check both of these. If the two are the same, we should only need to check one. If the two aren't the same, then it seems like one must be the one we want to check, and the other not. In either case, I'd think we'd only ever want to check one. Clearly always checking just the visible URL isn't correct or you wouldn't be making a change. So does always checking the last committed URL work? If not, why not, and how can we write the code so that we know when we should check one and when the other?
https://codereview.chromium.org/410373003/diff/40001/chrome/browser/ui/login/... File chrome/browser/ui/login/login_prompt.cc (right): https://codereview.chromium.org/410373003/diff/40001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt.cc:516: request_url.GetOrigin())) { On 2014/08/05 20:18:10, Peter Kasting wrote: > It seems a bit weird that we check both of these. > > If the two are the same, we should only need to check one. If the two aren't > the same, then it seems like one must be the one we want to check, and the other > not. In either case, I'd think we'd only ever want to check one. > > Clearly always checking just the visible URL isn't correct or you wouldn't be > making a change. So does always checking the last committed URL work? If not, > why not, and how can we write the code so that we know when we should check one > and when the other? The two different cases are: 1- The user navigates directly by entering a url in the omnibox. In this case: resource_url!=GetLastCommittedURL and resource_url==GetVisibleURL because the navigation hasn't committed yet (which is the oddity of requests with HTTP Auth headers). 2- The page redirects to a cross origin resource. In this case: resource_url!=GetLastCommitedURL and resource_url!=GetVisibleURL So you are right, it looks like we don't need to check for GetVisibleURL. I'm trying to think of a situation where resource_url isn't equal to GetLastCommittedURL but is is equal to GetVisibleURL, but I can't find any.
https://codereview.chromium.org/410373003/diff/40001/chrome/browser/ui/login/... File chrome/browser/ui/login/login_prompt.cc (right): https://codereview.chromium.org/410373003/diff/40001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt.cc:516: request_url.GetOrigin())) { On 2014/08/05 22:29:19, Mustafa Emre Acer wrote: > On 2014/08/05 20:18:10, Peter Kasting wrote: > > It seems a bit weird that we check both of these. > > > > If the two are the same, we should only need to check one. If the two aren't > > the same, then it seems like one must be the one we want to check, and the > other > > not. In either case, I'd think we'd only ever want to check one. > > > > Clearly always checking just the visible URL isn't correct or you wouldn't be > > making a change. So does always checking the last committed URL work? If > not, > > why not, and how can we write the code so that we know when we should check > one > > and when the other? > > > The two different cases are: > > 1- The user navigates directly by entering a url in the omnibox. In this case: > > resource_url!=GetLastCommittedURL and resource_url==GetVisibleURL > > because the navigation hasn't committed yet (which is the oddity of requests > with HTTP Auth headers). > > 2- The page redirects to a cross origin resource. In this case: > > resource_url!=GetLastCommitedURL and resource_url!=GetVisibleURL > > So you are right, it looks like we don't need to check for GetVisibleURL. I'm > trying to think of a situation where resource_url isn't equal to > GetLastCommittedURL but is is equal to GetVisibleURL, but I can't find any. I would probably change to just checking against GetLastCommittedURL() then. You might want to take the comment that's inside the conditional here and hoist it above, and expand it to note how it catches both scenarios you describe.
https://codereview.chromium.org/410373003/diff/40001/chrome/browser/ui/login/... File chrome/browser/ui/login/login_prompt.cc (right): https://codereview.chromium.org/410373003/diff/40001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt.cc:516: request_url.GetOrigin())) { On 2014/08/05 23:15:34, Peter Kasting wrote: > On 2014/08/05 22:29:19, Mustafa Emre Acer wrote: > > On 2014/08/05 20:18:10, Peter Kasting wrote: > > > It seems a bit weird that we check both of these. > > > > > > If the two are the same, we should only need to check one. If the two > aren't > > > the same, then it seems like one must be the one we want to check, and the > > other > > > not. In either case, I'd think we'd only ever want to check one. > > > > > > Clearly always checking just the visible URL isn't correct or you wouldn't > be > > > making a change. So does always checking the last committed URL work? If > > not, > > > why not, and how can we write the code so that we know when we should check > > one > > > and when the other? > > > > > > The two different cases are: > > > > 1- The user navigates directly by entering a url in the omnibox. In this case: > > > > resource_url!=GetLastCommittedURL and resource_url==GetVisibleURL > > > > because the navigation hasn't committed yet (which is the oddity of requests > > with HTTP Auth headers). > > > > 2- The page redirects to a cross origin resource. In this case: > > > > resource_url!=GetLastCommitedURL and resource_url!=GetVisibleURL > > > > So you are right, it looks like we don't need to check for GetVisibleURL. I'm > > trying to think of a situation where resource_url isn't equal to > > GetLastCommittedURL but is is equal to GetVisibleURL, but I can't find any. > > I would probably change to just checking against GetLastCommittedURL() then. > You might want to take the comment that's inside the conditional here and hoist > it above, and expand it to note how it catches both scenarios you describe. Done.
LGTM https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... File chrome/browser/ui/login/login_prompt.cc (right): https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt.cc:512: // Check if the request is cross origin. There are two different ways it can Nit: "it can be cross origin" -> "the navigation can occur" https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... File chrome/browser/ui/login/login_prompt_browsertest.cc (right): https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt_browsertest.cc:65: std::string landing_host) const; Nit: Pass by const ref
Still LGTM with a few nits. https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... File chrome/browser/ui/login/login_prompt.cc (right): https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt.cc:514: // 1- The user enters the resource URL in the omnibox. nit: This will not be true once site isolation is enabled, as any navigation could cause a cross-process swap, including link clicks. It will only happen for sites that are opted into isolation though. Not sure if it is worth changing the comment, just pointing it out. https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... File chrome/browser/ui/login/login_prompt_browsertest.cc (right): https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt_browsertest.cc:789: EXPECT_EQ(new_host, contents->GetVisibleURL().host()); Awesome!!! https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt_browsertest.cc:1187: std::string auth_host) const { nit: const std::string& https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt_browsertest.cc:1234: std::string auth_host = "127.0.0.1"; nit: std::string auth_host("127.0.0.1"); https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt_browsertest.cc:1245: const char* kTestPage = "files/login/cross_origin.html"; Why not use std::string?
Thanks! https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... File chrome/browser/ui/login/login_prompt.cc (right): https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt.cc:512: // Check if the request is cross origin. There are two different ways it can On 2014/08/06 01:07:54, Peter Kasting wrote: > Nit: "it can be cross origin" -> "the navigation can occur" Done. https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt.cc:514: // 1- The user enters the resource URL in the omnibox. On 2014/08/06 10:17:36, nasko (in Munich) wrote: > nit: This will not be true once site isolation is enabled, as any navigation > could cause a cross-process swap, including link clicks. It will only happen for > sites that are opted into isolation though. Not sure if it is worth changing the > comment, just pointing it out. I added your description as a note to the end. https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... File chrome/browser/ui/login/login_prompt_browsertest.cc (right): https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt_browsertest.cc:65: std::string landing_host) const; On 2014/08/06 01:07:54, Peter Kasting wrote: > Nit: Pass by const ref Done. https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt_browsertest.cc:1187: std::string auth_host) const { On 2014/08/06 10:17:37, nasko (in Munich) wrote: > nit: const std::string& Done. https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt_browsertest.cc:1234: std::string auth_host = "127.0.0.1"; On 2014/08/06 10:17:37, nasko (in Munich) wrote: > nit: std::string auth_host("127.0.0.1"); Done. https://codereview.chromium.org/410373003/diff/60001/chrome/browser/ui/login/... chrome/browser/ui/login/login_prompt_browsertest.cc:1245: const char* kTestPage = "files/login/cross_origin.html"; On 2014/08/06 10:17:37, nasko (in Munich) wrote: > Why not use std::string? Mainly for consistency with other test cases. Happy to change though.
The CQ bit was checked by meacer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/meacer@chromium.org/410373003/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
rsleevi: Can you please take a look at chrome/browser/net/websocket_browsertest.cc? Thanks.
Nice catch. LGTM
The CQ bit was checked by meacer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/meacer@chromium.org/410373003/140001
Message was sent while issue was closed.
Committed patchset #8 (140001) as 290090 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
