Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
488083002:
[Password Manager] Fix to recognise failed login attempt for sites where content server pushes new … (Closed)
|
Created:
6 years, 4 months ago by Pritam Nikam Modified:
6 years, 3 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, mkwst+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@branch_autofill_todo_20140813 Project:
chromium Visibility:
Public. |
Description[Password Manager] Fix to recognise failed login attempt for sites where content server pushes new login form.
With current implementation chromium browser does not recognize the failed login attempt for sites where content server pushes different login form (e.g. http://www.xda-developers.com), and apparently it assumes a login success and offers to save an incorrect password.
With this patch in addition to submitted form's action URL to that of visible form's action URL it ignores the schemes for HTTP or HTTPS URLs as well.
BUG=400769
Committed: https://crrev.com/0dbb996819103f467678db593e57f2d1fc7467ca
Cr-Commit-Position: refs/heads/master@{#292877}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Incorporated review comments and added browser_tests. #
Total comments: 15
Patch Set 3 : Incorporated review comments. #Patch Set 4 : Incorporatd review comments and added unit-tests #
Total comments: 46
Patch Set 5 : Incorporated review comments. #
Total comments: 10
Patch Set 6 : Incorporated review comments. #
Total comments: 4
Patch Set 7 : Updated as per review comments. #
Messages
Total messages: 24 (1 generated)
Dear vabr & Garrett, Please take a look.
Hi Pritam Nikam, I'm a bit concerned that this might break some legitimate use-cases. Also, whatever the final fix will be, please do not forget to add tests. Cheers, Vaclav https://codereview.chromium.org/488083002/diff/1/components/password_manager/... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/488083002/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.cc:440: // TODO(vabr): The similarity check is just action equality for now. If it Please keep the comment up to date. https://codereview.chromium.org/488083002/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.cc:447: all_visible_forms_[i].origin))) { I'm a bit afraid that this might actually break pages, which do contain forms on successful landing pages. Consider: (1) Login page at example.org, posting the credentials to example.org/auth. (2) After a successful login, example.org/auth shows a login form for a third-party site, or a password change form for example.org. Then, the origin of the form in step (2) should equal the origin of the login form in step (1).
On 2014/08/20 14:23:02, vabr (Chromium) wrote: > Hi Pritam Nikam, > > I'm a bit concerned that this might break some legitimate use-cases. Also, > whatever the final fix will be, please do not forget to add tests. > > Cheers, > Vaclav > > https://codereview.chromium.org/488083002/diff/1/components/password_manager/... > File components/password_manager/core/browser/password_manager.cc (right): > > https://codereview.chromium.org/488083002/diff/1/components/password_manager/... > components/password_manager/core/browser/password_manager.cc:440: // TODO(vabr): > The similarity check is just action equality for now. If it > Please keep the comment up to date. > > https://codereview.chromium.org/488083002/diff/1/components/password_manager/... > components/password_manager/core/browser/password_manager.cc:447: > all_visible_forms_[i].origin))) { > I'm a bit afraid that this might actually break pages, which do contain forms on > successful landing pages. Consider: > (1) Login page at http://example.org, posting the credentials to example.org/auth. > (2) After a successful login, example.org/auth shows a login form for a > third-party site, or a password change form for http://example.org. > > Then, the origin of the form in step (2) should equal the origin of the login > form in step (1). Yes, you are right! Even when one login form redirects to another login form (unrelated - PasswordManagerBrowserTest.LoginSuccessWithUnrelatedForm is one such example), this patch consider it as login failure case. But unlike FTP or other serveices where for login failure case content-server responds with status code 530, HTTP does not have any such provision. Is there any other way to detect whether it's login failure or normal redirection? May be for this particular web-page we can ignore it's schema while compairing action URL's i.e. *http* or *https*.
Thanks Vaclav for review. May be for this particular web-page we can ignore it's schema while compairing action URL's i.e. *http* or *https*. If it sounds okay to you then i'll add a patch with test-cases. Is there a way to get login failure instead of redirection in all caese?
(+jww for a security question below) Hi Pritam Nikam, I agree that for the "failed login?" check in PasswordManager::OnPasswordFormsRendered, treating HTTP action URLs as equivalent to their HTTPS versions sounds like a reasonable fix. Joel, as a security person, do you see any issues with that? > Is there a way to get login failure instead of redirection in all caese? I don't know about any. I believe the absence of it is the reason, why we have these fragile heuristics. :) Cheers, Vaclav
Thanks Vaclav for your inputs. I've added a new patch for review. Please have a look. It still needs Joel confirmation. Thanks! https://codereview.chromium.org/488083002/diff/1/components/password_manager/... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/488083002/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.cc:440: // TODO(vabr): The similarity check is just action equality for now. If it On 2014/08/20 14:23:02, vabr (Chromium) wrote: > Please keep the comment up to date. Done. https://codereview.chromium.org/488083002/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.cc:447: all_visible_forms_[i].origin))) { On 2014/08/20 14:23:02, vabr (Chromium) wrote: > I'm a bit afraid that this might actually break pages, which do contain forms on > successful landing pages. Consider: > (1) Login page at http://example.org, posting the credentials to example.org/auth. > (2) After a successful login, example.org/auth shows a login form for a > third-party site, or a password change form for http://example.org. > > Then, the origin of the form in step (2) should equal the origin of the login > form in step (1). Done. Similarity check is added on case and scheme ignored action URL equality instead.
Hi Pritam Nikam, Thanks for the update. I'm not convinced that the check should be case insensitive -- which page is broken by the case sensitivity? Further comments are below. Thanks! Vaclav https://codereview.chromium.org/488083002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/488083002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:1123: // with action URL having different scheme and/or case. Heuristic shall be able The test below does not test the different scheme, only the different case. Please introduce a separate test to test the different scheme. https://codereview.chromium.org/488083002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:1123: // with action URL having different scheme and/or case. Heuristic shall be able I'm actually not convinced that the action URL comparison should be case insensitive. URL paths are case insensitive. Are there real examples, when Chrome's password manager misunderstands a failed login because of a different letter case? https://codereview.chromium.org/488083002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:1127: NavigateToFile("/password/seperate_login_form.html"); typo: seperate -> separate Please also correct in the file names, id names etc. https://codereview.chromium.org/488083002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/488083002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager.cc:72: bool CompareURLContentsEqualIgnoreCase(const GURL& src, const GURL& dst) { Please spell out concretely that scheme is omitted, the notion of a URLContents is not known widely enough to indicate that. https://codereview.chromium.org/488083002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager.cc:72: bool CompareURLContentsEqualIgnoreCase(const GURL& src, const GURL& dst) { nit: "Compare" seems superfluous in the function name. https://codereview.chromium.org/488083002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager.cc:445: // TODO(vabr): The similarity check is just on case and scheme ignored I'm not a native English speaker, but the first sentence seems difficult to parse to me. I'd suggest rephrasing, but before we do that, we should probably clarify whether the case sensitivity is included or not. https://codereview.chromium.org/488083002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager.cc:450: CompareURLContentsEqualIgnoreCase( Please do not allow mixing of arbitrary schemes, just HTTP and HTTPS at this point. Let's be conservative, because it is security relevant.
Thanks Vaclav for your inputs. Currently if we don't ignore cases, GURL comparisons failing and it will fail any use-case having such provision. And in my opinion this check is needed. Also, I'm finding it difficult to add a test-case by ignoring schemes as DNS resolution is off for test servers hosted. I've incorporated rest of the comments in new patch (#3). Please have a look. Thanks! https://codereview.chromium.org/488083002/diff/20001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/488083002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:1123: // with action URL having different scheme and/or case. Heuristic shall be able On 2014/08/21 14:54:12, vabr (Chromium) wrote: > I'm actually not convinced that the action URL comparison should be case > insensitive. URL paths are case insensitive. Are there real examples, when > Chrome's password manager misunderstands a failed login because of a different > letter case? I didn't find any real example though, this particular test-case is failing keeping password_manager.cc untouched. Thats the reason I've added this case insensitivity check as well. https://codereview.chromium.org/488083002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:1123: // with action URL having different scheme and/or case. Heuristic shall be able On 2014/08/21 14:54:12, vabr (Chromium) wrote: > The test below does not test the different scheme, only the different case. > Please introduce a separate test to test the different scheme. Seems it's quite difficult as we need http and https servers running. Current test class uses only HTTP test_server i,e, embedded_test_server(). Moreover, it does not have support for DNS resolution, it publishes a random port and with 127.0.0.1 (localhost) every time we run browser_tests. So even we have a HTTPS server running, port number will be different than that of and test cases will fail even if we ignore schemes. e.g. say separate_login_form.html action >> http://127.0.0.1:1234:/password/done_and_separate_login_form.html and done_and_separate_login_form.html action >> https://127.0.0.1:7890:/password/done_and_separate_login_form.html comparing urls even ignoring schemes will not match. https://codereview.chromium.org/488083002/diff/20001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:1127: NavigateToFile("/password/seperate_login_form.html"); On 2014/08/21 14:54:12, vabr (Chromium) wrote: > typo: seperate -> separate > Please also correct in the file names, id names etc. Done. https://codereview.chromium.org/488083002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/488083002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager.cc:72: bool CompareURLContentsEqualIgnoreCase(const GURL& src, const GURL& dst) { On 2014/08/21 14:54:12, vabr (Chromium) wrote: > nit: "Compare" seems superfluous in the function name. Done. https://codereview.chromium.org/488083002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager.cc:72: bool CompareURLContentsEqualIgnoreCase(const GURL& src, const GURL& dst) { On 2014/08/21 14:54:12, vabr (Chromium) wrote: > Please spell out concretely that scheme is omitted, the notion of a URLContents > is not known widely enough to indicate that. Done. https://codereview.chromium.org/488083002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager.cc:445: // TODO(vabr): The similarity check is just on case and scheme ignored On 2014/08/21 14:54:12, vabr (Chromium) wrote: > I'm not a native English speaker, but the first sentence seems difficult to > parse to me. I'd suggest rephrasing, but before we do that, we should probably > clarify whether the case sensitivity is included or not. Done. https://codereview.chromium.org/488083002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager.cc:450: CompareURLContentsEqualIgnoreCase( On 2014/08/21 14:54:12, vabr (Chromium) wrote: > Please do not allow mixing of arbitrary schemes, just HTTP and HTTPS at this > point. Let's be conservative, because it is security relevant. Done.
https://codereview.chromium.org/488083002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/488083002/diff/20001/components/password_mana... components/password_manager/core/browser/password_manager.cc:450: CompareURLContentsEqualIgnoreCase( On 2014/08/21 16:49:21, Pritam Nikam wrote: > On 2014/08/21 14:54:12, vabr (Chromium) wrote: > > Please do not allow mixing of arbitrary schemes, just HTTP and HTTPS at this > > point. Let's be conservative, because it is security relevant. > > Done. This lgtm from a security perspective but is certainly treading on very dangerous territory. This seems fine because it leads to an *inaction*... but we we need to be really, really careful. So if I am misunderstanding the fact that this is an inaction, please correct me.
Hi Pritam Nikam, On 2014/08/21 16:49:21, Pritam Nikam wrote: > Thanks Vaclav for your inputs. > > Currently if we don't ignore cases, GURL comparisons failing and it will fail > any use-case having such provision. And in my opinion this check is needed. Sorry, but unless there is an actual website failing because of this, we need to compare actions in a case sensitive way. As I said, the path of an URL is case sensitive (e.g., see what you get if you navigate to http://www.google.com/robotS.txt and http://www.google.com/robots.txt), so two URLs can point to two different pages even if they are case-insensitive equivalent. > > Also, I'm finding it difficult to add a test-case by ignoring schemes as DNS > resolution is off for test servers hosted. Yes, adding tests is most of the time more difficult than the fix itself. :) Please have a look at net::SpawnedTestServer, available in the browser test as content::BrowserTestBase::test_server(). That server can be created to serve HTTPS. Once codesearch is working again, try to find callsites of that accessor which deal with SSL. The drawback is that the spawned server is more flaky (requires running Python scripts during the test execution). Alternatively maybe you can find a way to test this in a unit test, but in that case please make sure you only use public API of the PasswordManager, not relying on implementation details. I will review the patch once you address the two comments above, because they are likely to cause significant changes. Thanks! Vaclav > > I've incorporated rest of the comments in new patch (#3). Please have a look. > > Thanks! > > https://codereview.chromium.org/488083002/diff/20001/chrome/browser/password_... > File chrome/browser/password_manager/password_manager_browsertest.cc (right): > > https://codereview.chromium.org/488083002/diff/20001/chrome/browser/password_... > chrome/browser/password_manager/password_manager_browsertest.cc:1123: // with > action URL having different scheme and/or case. Heuristic shall be able > On 2014/08/21 14:54:12, vabr (Chromium) wrote: > > I'm actually not convinced that the action URL comparison should be case > > insensitive. URL paths are case insensitive. Are there real examples, when > > Chrome's password manager misunderstands a failed login because of a different > > letter case? > > I didn't find any real example though, this particular test-case is failing > keeping password_manager.cc untouched. Thats the reason I've added this case > insensitivity check as well. > > https://codereview.chromium.org/488083002/diff/20001/chrome/browser/password_... > chrome/browser/password_manager/password_manager_browsertest.cc:1123: // with > action URL having different scheme and/or case. Heuristic shall be able > On 2014/08/21 14:54:12, vabr (Chromium) wrote: > > The test below does not test the different scheme, only the different case. > > Please introduce a separate test to test the different scheme. > > Seems it's quite difficult as we need http and https servers running. > > Current test class uses only HTTP test_server i,e, embedded_test_server(). > Moreover, it does not have support for DNS resolution, it publishes a random > port and with 127.0.0.1 (localhost) every time we run browser_tests. > > So even we have a HTTPS server running, port number will be different than that > of and test cases will fail even if we ignore schemes. > > e.g. > say separate_login_form.html action >> > http://127.0.0.1:1234:/password/done_and_separate_login_form.html > > and done_and_separate_login_form.html action >> > https://127.0.0.1:7890:/password/done_and_separate_login_form.html > > comparing urls even ignoring schemes will not match. > > https://codereview.chromium.org/488083002/diff/20001/chrome/browser/password_... > chrome/browser/password_manager/password_manager_browsertest.cc:1127: > NavigateToFile("/password/seperate_login_form.html"); > On 2014/08/21 14:54:12, vabr (Chromium) wrote: > > typo: seperate -> separate > > Please also correct in the file names, id names etc. > > Done. > > https://codereview.chromium.org/488083002/diff/20001/components/password_mana... > File components/password_manager/core/browser/password_manager.cc (right): > > https://codereview.chromium.org/488083002/diff/20001/components/password_mana... > components/password_manager/core/browser/password_manager.cc:72: bool > CompareURLContentsEqualIgnoreCase(const GURL& src, const GURL& dst) { > On 2014/08/21 14:54:12, vabr (Chromium) wrote: > > nit: "Compare" seems superfluous in the function name. > > Done. > > https://codereview.chromium.org/488083002/diff/20001/components/password_mana... > components/password_manager/core/browser/password_manager.cc:72: bool > CompareURLContentsEqualIgnoreCase(const GURL& src, const GURL& dst) { > On 2014/08/21 14:54:12, vabr (Chromium) wrote: > > Please spell out concretely that scheme is omitted, the notion of a > URLContents > > is not known widely enough to indicate that. > > Done. > > https://codereview.chromium.org/488083002/diff/20001/components/password_mana... > components/password_manager/core/browser/password_manager.cc:445: // TODO(vabr): > The similarity check is just on case and scheme ignored > On 2014/08/21 14:54:12, vabr (Chromium) wrote: > > I'm not a native English speaker, but the first sentence seems difficult to > > parse to me. I'd suggest rephrasing, but before we do that, we should probably > > clarify whether the case sensitivity is included or not. > > Done. > > https://codereview.chromium.org/488083002/diff/20001/components/password_mana... > components/password_manager/core/browser/password_manager.cc:450: > CompareURLContentsEqualIgnoreCase( > On 2014/08/21 14:54:12, vabr (Chromium) wrote: > > Please do not allow mixing of arbitrary schemes, just HTTP and HTTPS at this > > point. Let's be conservative, because it is security relevant. > > Done.
Thanks Vaclav for review comments. I've incorporated review comments in patch set #4. PTAL! Thanks!
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Hi Pritam Nikam, Thanks for adding the tests! I like the structure now, but please do a one more pass on the comments below. Thank you for your work on this. Vaclav https://codereview.chromium.org/488083002/diff/100001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/488083002/diff/100001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:30: #include "components/content_settings/core/common/content_settings_types.h" What do you need this header for? (If you don't, please remove.) https://codereview.chromium.org/488083002/diff/100001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1167: "password/separate_login_form_with_onload_submit_script.html"; Why do you need the onload submit script here? Could you use the same file password/separate_login_form.html as in the test above? (If you need to wait for loading, could you use observer.Wait() instead?) https://codereview.chromium.org/488083002/diff/100001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1180: observer.Wait(); I'm afraid this will be a source of flakiness. Suppose the browser manages to both load the first page, and submit the form before the Wait() call on line 1177 finishes. In that case, the call here, on 1180, will get stuck until timeoff, and the test will fail for no good reason. Please use the same pattern as in the test you added above. https://codereview.chromium.org/488083002/diff/100001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1182: // Verify that browser doest not prompt save password popup. nit: You can drop this comment, the code below is self-documenting. https://codereview.chromium.org/488083002/diff/100001/chrome/test/data/passwo... File chrome/test/data/password/done_and_separate_login_form.html (right): https://codereview.chromium.org/488083002/diff/100001/chrome/test/data/passwo... chrome/test/data/password/done_and_separate_login_form.html:4: window.onload = function() { nit: In short, use C++ indentation rules here: window.onload = function() { ...; } function foo() { var ...; ; } For more, have a look at http://dev.chromium.org/developers/web-development-style-guide#TOC-JavaScript, linked from http://dev.chromium.org/developers/coding-style. https://codereview.chromium.org/488083002/diff/100001/chrome/test/data/passwo... chrome/test/data/password/done_and_separate_login_form.html:7: function getAction() { nit: The name is uninformative, it should express that the returned URL is created from the current location by swapping the scheme between HTTP and HTTPS. For example: function getLocationWithHttpSchemeSecuritySwapped() {...} https://codereview.chromium.org/488083002/diff/100001/chrome/test/data/passwo... chrome/test/data/password/done_and_separate_login_form.html:9: var actionUrl = protocol + "//" + window.location.host + "/password/done_and_separate_login_form.html"; Why not use window.location.pathname, instead of retyping the path again? https://codereview.chromium.org/488083002/diff/100001/chrome/test/data/passwo... chrome/test/data/password/done_and_separate_login_form.html:9: var actionUrl = protocol + "//" + window.location.host + "/password/done_and_separate_login_form.html"; nit: This function is oblivious to the fact, that the resulting URL is assigned to the action attribute. So don't include "action" in the variable name, it's a bit confusing. Names like |result|, |url|, etc. would do fine. https://codereview.chromium.org/488083002/diff/100001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.cc:72: bool EqualsIgnoreScheme(const GURL& src, const GURL& dst) { The function name is unclear, and the roles of the arguments are the same, nothing asymmetric like source and destination (that would indicate that src is being overwritten by dst). So I suggest renaming to: bool URLsEqualUpToScheme(const GURL& a, const GURL& b) or similar. https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.cc:76: bool EqualsLoginFormAction(const GURL& src, const GURL& dst) { nit: The function is actually oblivious to the fact that the URLs are actions of some forms. Also the roles of the arguments are the same, nothing asymmetric like source and destination (that would indicate that src is being overwritten by dst). So I suggest renaming to bool URLsEqualUpToHttpHttpsSubstitution(const GURL& a, const GURL& b) or similar. https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.cc:77: bool is_same_form = (src == dst); I suggest an explicit early exit instead: if (src == dst) return true; The advantage is that you don't have to test |is_same_form| below, which reduces the clutter in the if-conditions. Also, it's clear on first sight, that what goes below is only for unequal URLs. https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.cc:78: // If action URLs are not matching, there is a possiblility that the nit: This comment id long enough to harm its own usefulness, and on top of it incorrect (it suggests, that schemes are ignored even outside of the HTTP-HTTPS substitution). Please consider replacing with a short mention of the bug number which contains a use-case for this exception, e.g.: // The first-time and retry login forms action URLs sometimes differ in switching from HTTP to HTTPS, see http://crbug.com/400769. https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.cc:82: if (!is_same_form && src.SchemeIsHTTPOrHTTPS() && dst.SchemeIsHTTPOrHTTPS()) If you replace |is_same_form| with the early return above, I suggest to rewrite the rest as follows: if (src.SchemeIsHTTPOrHTTPS() && dst.SchemeIsHTTPOrHTTPS()) return EqualsIgnoreScheme(src, dst); return false; That would result in three return statements overall, corresponding to the three major cases in the code's logic: equal URLs, URLs equal up to HTTP<->HTTPS substitution, and everything else, improving the code's readability. https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager.cc:457: // Moreover, for cases where action URLs are not matching, extends The comment is out of date again. My suggestion is to actually keep the very original comment (before this CL), and just insert "up to HTTP<->HTTPS substitution" after "action equality". https://codereview.chromium.org/488083002/diff/100001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:761: // Verify that the form submission resulting into concent-server pushing nit: Although generally correct, the sentence is a bit long and confusing (e.g., it uses "prompt" as a verb, while in most of the text in the code it's used as a noun). Suggested reformulation, split into two sentences: "On failed login attempts, the retry-form can have action scheme changed from HTTP to HTTPS (see http://crbug.com/400769). Check that such retry-form is considered equal to the original login form, and the attempt recognised as a failure." https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:765: FormSubmitWithNavigateToNewFormDifferingActionsOnlyBySchemes) { nit: The test name does not state what is tested, and describes the situation in a little bit confusing way. What about: SeeingFormActionWithOnlyHttpHttpsChangeIsLoginFailure https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:774: PasswordForm second_form(MakeSimpleForm()); nit: Create this form from the previous, to emphasise they are almost the same (and shorten the initialisation below): PasswordForm second_form(first_form); https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:775: second_form.origin = GURL("http://forum.xda-developers.com/login.php"); The origin is not related to the tested change, so although in the real example the origin differed in this way, here you can keep it the same. As a bonus, it will be clearer, what the change is. https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:776: second_form.action = GURL("https://forum.xda-developers.com/login.php"); nit: Please add a comment emphasising that the change is in the scheme (it's hard to spot). https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:777: second_form.signon_realm = "http://www.xda-developers.com/"; You can drop this if you create second_form based on first_form. https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:783: manager()->OnPasswordFormsRendered(observed, true); Why do you clear |observed| before calling OnPasswordFormsRendered? If we pretend the |first_form| to be the one submitted, it should be among the ones which were visible (i.e., reported to OnPasswordFormsRendered). https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:788: // This page contains a form with the same action, but on a different scheme, nit: No need to comment on the difference, it should be clear from the definition of second_form above. Instead, perhaps mention what putting it to |observed| means: // Simulate loading a page, which contains |second_form| instead of |first_form|. https://codereview.chromium.org/488083002/diff/100001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:792: // Verify that not to prompt with password save popup dialog. grammar nit: "Verify that no prompt to save the password is shown."
Thanks Vaclav for your review inputs.
For https pages, this fails in ExecuteScript() as it's returning
document.getElementById('id').value as NULL/undefined.
[INFO:CONSOLE(1)] "Uncaught TypeError: Cannot set property 'value' of null",
source: (1)]
I didn't find exact matching sample for this over HTTPS test server, I've tried
adding multiple command-line flags as well.
I even tried executing this under ssl_browser_test as SSLUITest, but even their
it's giving same problem. Even I didn't find
window.domAutomationController.send() with script working.
Rest of the inputs I've incorporated in patch set #5. PTAL.
Thanks!
https://codereview.chromium.org/488083002/diff/100001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):
https://codereview.chromium.org/488083002/diff/100001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:30: #include
"components/content_settings/core/common/content_settings_types.h"
On 2014/08/26 09:42:58, vabr (Chromium) wrote:
> What do you need this header for?
> (If you don't, please remove.)
Done.
https://codereview.chromium.org/488083002/diff/100001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:1167:
"password/separate_login_form_with_onload_submit_script.html";
On 2014/08/26 09:42:58, vabr (Chromium) wrote:
> Why do you need the onload submit script here?
> Could you use the same file password/separate_login_form.html as in the test
> above?
> (If you need to wait for loading, could you use observer.Wait() instead?)
std::string submit =
"document.getElementById('username_separate').value = 'user';"
"document.getElementById('password_separate').value = 'password';"
"document.getElementById('submit_separate').click();";
ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), submit));
For https pages, this fails in ExecuteScript() as it's returning
document.getElementById('id').value as NULL/undefined. I didn't find exact
matching sample for this over HTTPS test server, I've tried adding multiple
command-line flags as well. I even tried executing this under ssl_browser_test
as SSLUITest, but even their it's giving same problem.
Even I didn't find window.domAutomationController.send() with script working.
https://codereview.chromium.org/488083002/diff/100001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:1180:
observer.Wait();
As explained in previous comments, content::ExecuteScript() fail to execute
document.getElementById('username_separate').value is undefined.
https://codereview.chromium.org/488083002/diff/100001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:1182: // Verify
that browser doest not prompt save password popup.
On 2014/08/26 09:42:58, vabr (Chromium) wrote:
> nit: You can drop this comment, the code below is self-documenting.
Done.
https://codereview.chromium.org/488083002/diff/100001/chrome/test/data/passwo...
File chrome/test/data/password/done_and_separate_login_form.html (right):
https://codereview.chromium.org/488083002/diff/100001/chrome/test/data/passwo...
chrome/test/data/password/done_and_separate_login_form.html:4: window.onload =
function() {
On 2014/08/26 09:42:58, vabr (Chromium) wrote:
> nit: In short, use C++ indentation rules here:
>
> window.onload = function() {
> ...;
> }
>
> function foo() {
> var ...;
> ;
> }
>
> For more, have a look at
> http://dev.chromium.org/developers/web-development-style-guide#TOC-JavaScript,
> linked from http://dev.chromium.org/developers/coding-style.
Done.
https://codereview.chromium.org/488083002/diff/100001/chrome/test/data/passwo...
chrome/test/data/password/done_and_separate_login_form.html:7: function
getAction() {
On 2014/08/26 09:42:58, vabr (Chromium) wrote:
> nit: The name is uninformative, it should express that the returned URL is
> created from the current location by swapping the scheme between HTTP and
HTTPS.
> For example:
>
> function getLocationWithHttpSchemeSecuritySwapped() {...}
Done.
https://codereview.chromium.org/488083002/diff/100001/chrome/test/data/passwo...
chrome/test/data/password/done_and_separate_login_form.html:9: var actionUrl =
protocol + "//" + window.location.host +
"/password/done_and_separate_login_form.html";
On 2014/08/26 09:42:58, vabr (Chromium) wrote:
> Why not use window.location.pathname, instead of retyping the path again?
Done.
https://codereview.chromium.org/488083002/diff/100001/chrome/test/data/passwo...
chrome/test/data/password/done_and_separate_login_form.html:9: var actionUrl =
protocol + "//" + window.location.host +
"/password/done_and_separate_login_form.html";
On 2014/08/26 09:42:58, vabr (Chromium) wrote:
> nit: This function is oblivious to the fact, that the resulting URL is
assigned
> to the action attribute. So don't include "action" in the variable name, it's
a
> bit confusing. Names like |result|, |url|, etc. would do fine.
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
File components/password_manager/core/browser/password_manager.cc (right):
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager.cc:72: bool
EqualsIgnoreScheme(const GURL& src, const GURL& dst) {
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> The function name is unclear, and the roles of the arguments are the same,
> nothing asymmetric like source and destination (that would indicate that src
is
> being overwritten by dst).
>
> So I suggest renaming to:
> bool URLsEqualUpToScheme(const GURL& a, const GURL& b)
> or similar.
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager.cc:76: bool
EqualsLoginFormAction(const GURL& src, const GURL& dst) {
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> nit: The function is actually oblivious to the fact that the URLs are actions
of
> some forms. Also the roles of the arguments are the same, nothing asymmetric
> like source and destination (that would indicate that src is being overwritten
> by dst).
>
> So I suggest renaming to
> bool URLsEqualUpToHttpHttpsSubstitution(const GURL& a, const GURL& b)
> or similar.
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager.cc:77: bool
is_same_form = (src == dst);
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> I suggest an explicit early exit instead:
>
> if (src == dst)
> return true;
>
> The advantage is that you don't have to test |is_same_form| below, which
reduces
> the clutter in the if-conditions. Also, it's clear on first sight, that what
> goes below is only for unequal URLs.
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager.cc:78: // If action
URLs are not matching, there is a possiblility that the
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> nit: This comment id long enough to harm its own usefulness, and on top of it
> incorrect (it suggests, that schemes are ignored even outside of the
HTTP-HTTPS
> substitution).
>
> Please consider replacing with a short mention of the bug number which
contains
> a use-case for this exception, e.g.:
>
> // The first-time and retry login forms action URLs sometimes differ in
> switching from HTTP to HTTPS, see http://crbug.com/400769.
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager.cc:82: if
(!is_same_form && src.SchemeIsHTTPOrHTTPS() && dst.SchemeIsHTTPOrHTTPS())
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> If you replace |is_same_form| with the early return above, I suggest to
rewrite
> the rest as follows:
>
> if (src.SchemeIsHTTPOrHTTPS() && dst.SchemeIsHTTPOrHTTPS())
> return EqualsIgnoreScheme(src, dst);
>
> return false;
>
> That would result in three return statements overall, corresponding to the
three
> major cases in the code's logic: equal URLs, URLs equal up to HTTP<->HTTPS
> substitution, and everything else, improving the code's readability.
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager.cc:457: // Moreover,
for cases where action URLs are not matching, extends
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> The comment is out of date again.
> My suggestion is to actually keep the very original comment (before this CL),
> and just insert "up to HTTP<->HTTPS substitution" after "action equality".
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:761: //
Verify that the form submission resulting into concent-server pushing
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> nit: Although generally correct, the sentence is a bit long and confusing
(e.g.,
> it uses "prompt" as a verb, while in most of the text in the code it's used as
a
> noun). Suggested reformulation, split into two sentences:
>
> "On failed login attempts, the retry-form can have action scheme changed from
> HTTP to HTTPS (see http://crbug.com/400769). Check that such retry-form is
> considered equal to the original login form, and the attempt recognised as a
> failure."
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:765:
FormSubmitWithNavigateToNewFormDifferingActionsOnlyBySchemes) {
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> nit: The test name does not state what is tested, and describes the situation
in
> a little bit confusing way. What about:
>
> SeeingFormActionWithOnlyHttpHttpsChangeIsLoginFailure
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:774:
PasswordForm second_form(MakeSimpleForm());
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> nit: Create this form from the previous, to emphasise they are almost the same
> (and shorten the initialisation below):
>
> PasswordForm second_form(first_form);
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:775:
second_form.origin = GURL("http://forum.xda-developers.com/login.php");
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> The origin is not related to the tested change, so although in the real
example
> the origin differed in this way, here you can keep it the same. As a bonus, it
> will be clearer, what the change is.
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:776:
second_form.action = GURL("https://forum.xda-developers.com/login.php");
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> nit: Please add a comment emphasising that the change is in the scheme (it's
> hard to spot).
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:777:
second_form.signon_realm = "http://www.xda-developers.com/";
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> You can drop this if you create second_form based on first_form.
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:783:
manager()->OnPasswordFormsRendered(observed, true);
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> Why do you clear |observed| before calling OnPasswordFormsRendered? If we
> pretend the |first_form| to be the one submitted, it should be among the ones
> which were visible (i.e., reported to OnPasswordFormsRendered).
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:788: //
This page contains a form with the same action, but on a different scheme,
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> nit: No need to comment on the difference, it should be clear from the
> definition of second_form above. Instead, perhaps mention what putting it to
> |observed| means:
>
> // Simulate loading a page, which contains |second_form| instead of
> |first_form|.
Done.
https://codereview.chromium.org/488083002/diff/100001/components/password_man...
components/password_manager/core/browser/password_manager_unittest.cc:792: //
Verify that not to prompt with password save popup dialog.
On 2014/08/26 09:42:59, vabr (Chromium) wrote:
> grammar nit: "Verify that no prompt to save the password is shown."
Done.
Thanks, Pritam Nikam. LGTM once you address the comments below. I'm not sure why ExecuteScript won't work with HTTPS, probably something to do with the content security policy. Your work-around looks reasonable, though, so let's go with that. Cheers, Vaclav https://codereview.chromium.org/488083002/diff/120001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/488083002/diff/120001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1131: std::string path = "/password/separate_login_form.html"; Although for the http-first version of the test using ExecuteScript is not technically an issue, to keep things simple, could you please use the same structure here, as you do below? That is, load "password/separate_login_form_with_onload_submit_script.html", don't ExecutScript, and instead wait for the final path. After doing that, please remove the file separate_login_form.html completely. https://codereview.chromium.org/488083002/diff/120001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1179: observer.Wait(); To avoid flakiness, instead of doing Wait() twice, please do: observer.SetPathToWaitFor("/password/done_and_separate_login_form.html"); observer.Wait(); (Also, no need for code comments in that.) https://codereview.chromium.org/488083002/diff/120001/chrome/test/data/passwo... File chrome/test/data/password/done_and_separate_login_form.html (right): https://codereview.chromium.org/488083002/diff/120001/chrome/test/data/passwo... chrome/test/data/password/done_and_separate_login_form.html:10: var url = protocol + window.location.pathname + "done_and_separate_login_form.html"; Did you forget to add window.location.host? https://codereview.chromium.org/488083002/diff/120001/chrome/test/data/passwo... chrome/test/data/password/done_and_separate_login_form.html:10: var url = protocol + window.location.pathname + "done_and_separate_login_form.html"; You should not need to add the filename, it should be part of the pathname. https://codereview.chromium.org/488083002/diff/120001/chrome/test/data/passwo... File chrome/test/data/password/separate_login_form_with_onload_submit_script.html (right): https://codereview.chromium.org/488083002/diff/120001/chrome/test/data/passwo... chrome/test/data/password/separate_login_form_with_onload_submit_script.html:1: <html> Please include a comment explaining why this test has its own JavaScript to submit the form instead of using content::ExecuteScript.
Thanks Vaclav for inputs! Added new patch (#6) for review. PTAL! Thanks!
Thanks Vaclav for inputs! In my opinion https to http test need to revisit in near future. Added new patch (#6) incorporating your review comments. PTAL! Thanks! https://codereview.chromium.org/488083002/diff/120001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/488083002/diff/120001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1131: std::string path = "/password/separate_login_form.html"; On 2014/08/27 11:51:47, vabr (Chromium) wrote: > Although for the http-first version of the test using ExecuteScript is not > technically an issue, to keep things simple, could you please use the same > structure here, as you do below? That is, load > "password/separate_login_form_with_onload_submit_script.html", don't > ExecutScript, and instead wait for the final path. After doing that, please > remove the file separate_login_form.html completely. Done. https://codereview.chromium.org/488083002/diff/120001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1179: observer.Wait(); On 2014/08/27 11:51:47, vabr (Chromium) wrote: > To avoid flakiness, instead of doing Wait() twice, please do: > > observer.SetPathToWaitFor("/password/done_and_separate_login_form.html"); > observer.Wait(); > > (Also, no need for code comments in that.) Done. https://codereview.chromium.org/488083002/diff/120001/chrome/test/data/passwo... File chrome/test/data/password/done_and_separate_login_form.html (right): https://codereview.chromium.org/488083002/diff/120001/chrome/test/data/passwo... chrome/test/data/password/done_and_separate_login_form.html:10: var url = protocol + window.location.pathname + "done_and_separate_login_form.html"; On 2014/08/27 11:51:47, vabr (Chromium) wrote: > You should not need to add the filename, it should be part of the pathname. Done. https://codereview.chromium.org/488083002/diff/120001/chrome/test/data/passwo... chrome/test/data/password/done_and_separate_login_form.html:10: var url = protocol + window.location.pathname + "done_and_separate_login_form.html"; On 2014/08/27 11:51:47, vabr (Chromium) wrote: > Did you forget to add window.location.host? Done. https://codereview.chromium.org/488083002/diff/120001/chrome/test/data/passwo... File chrome/test/data/password/separate_login_form_with_onload_submit_script.html (right): https://codereview.chromium.org/488083002/diff/120001/chrome/test/data/passwo... chrome/test/data/password/separate_login_form_with_onload_submit_script.html:1: <html> On 2014/08/27 11:51:47, vabr (Chromium) wrote: > Please include a comment explaining why this test has its own JavaScript to > submit the form instead of using content::ExecuteScript. Done.
LGTM with optional comments about comments :). Thanks for fixing this bug! Vaclav https://codereview.chromium.org/488083002/diff/140001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/488083002/diff/140001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1163: // This testcase is having provision to submit form on page load completion. optional nit: This comment sounds a little weird to me. I would suggest to simplify it a little, e.g., like this: "This test case cannot inject the scripts via content::ExecuteScript() in files served through HTTPS. Therefore the scripts are made part of the HTML site and executed on load." https://codereview.chromium.org/488083002/diff/140001/chrome/test/data/passwo... File chrome/test/data/password/separate_login_form_with_onload_submit_script.html (right): https://codereview.chromium.org/488083002/diff/140001/chrome/test/data/passwo... chrome/test/data/password/separate_login_form_with_onload_submit_script.html:4: // This testcase is having provision to submit form on completion of page load itself. optional nit: I think you can drop this comment, because the reason for using the onload script is described in the C++ callsite. If you do want to keep the comment here, then please consider rephrasing it in a simpler way, and also keep the lines length to 80 lines (to be consistent with the surrounding code).
Thanks Vaclav for inputs. Updated as per your inputs PTAL. Thanks! https://codereview.chromium.org/488083002/diff/140001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/488083002/diff/140001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1163: // This testcase is having provision to submit form on page load completion. On 2014/09/01 08:39:39, vabr (Chromium) wrote: > optional nit: This comment sounds a little weird to me. I would suggest to > simplify it a little, e.g., like this: > "This test case cannot inject the scripts via content::ExecuteScript() in files > served through HTTPS. Therefore the scripts are made part of the HTML site and > executed on load." Done. https://codereview.chromium.org/488083002/diff/140001/chrome/test/data/passwo... File chrome/test/data/password/separate_login_form_with_onload_submit_script.html (right): https://codereview.chromium.org/488083002/diff/140001/chrome/test/data/passwo... chrome/test/data/password/separate_login_form_with_onload_submit_script.html:4: // This testcase is having provision to submit form on completion of page load itself. On 2014/09/01 08:39:39, vabr (Chromium) wrote: > optional nit: I think you can drop this comment, because the reason for using > the onload script is described in the C++ callsite. If you do want to keep the > comment here, then please consider rephrasing it in a simpler way, and also keep > the lines length to 80 lines (to be consistent with the surrounding code). Done.
The CQ bit was checked by pritam.nikam@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pritam.nikam@samsung.com/488083002/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as 260706a4f45d69521b4072d90ff99ec23d9e355b
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0dbb996819103f467678db593e57f2d1fc7467ca Cr-Commit-Position: refs/heads/master@{#292877} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
