Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2607413003:
Add security feature to ProvisionalSavePassword (Closed)
|
Created:
3 years, 11 months ago by jdoerrie Modified:
3 years, 11 months ago CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd security feature to ProvisionalSavePassword
Implement check against overriding provisional saved forms when the origin is the same but the scheme is insecure. Adds logging to understand how often this occurs.
BUG=571580
Review-Url: https://codereview.chromium.org/2607413003
Cr-Commit-Position: refs/heads/master@{#443545}
Committed: https://chromium.googlesource.com/chromium/src/+/4ed25061887ba8183010f4cdd800dff04bcdc794
Patch Set 1 #Patch Set 2 : Almost Working Browser Test #
Total comments: 12
Patch Set 3 : Addressed comments. #
Total comments: 4
Patch Set 4 : Fix Test Failures #
Total comments: 7
Patch Set 5 : Address comments: Log to UMA, call WaitForPasswordStore() #
Total comments: 2
Patch Set 6 : Addressed next round of comments. #
Total comments: 1
Patch Set 7 : More explicit unittest, log origins, and update histogram description. #
Total comments: 20
Patch Set 8 : Addressed more comments. #Patch Set 9 : XML Update #
Total comments: 1
Patch Set 10 : Remove now redundant lines. #
Total comments: 4
Patch Set 11 : Addressed nit. #Messages
Total messages: 52 (28 generated)
jdoerrie@chromium.org changed reviewers: + vasilii@chromium.org
Hi all, this is a first draft of the proposed change to ProvisionalSavePassword. I would like to hear your advice with a couple of points, not a thorough review yet. https://codereview.chromium.org/2607413003/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2607413003/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1496: CheckThatCredentialsStored(password_store.get(), base::ASCIIToUTF16("user"), This test currently fails, because |ShouldBlockPasswordForSameOriginButDifferentScheme| returns false due to different ports. During an example run of the test requests were served from the following origins: https://127.0.0.1:45407/ http://127.0.0.1:45779/ I would like to avoid the port mismatch here, but I'm not quite sure how I would do this. Dropping the port would work, or setting them to their default values of 80 and 443. Unfortunately, I do not think that is possible. Is there any other possible approach? https://codereview.chromium.org/2607413003/diff/20001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/2607413003/diff/20001/components/password_man... components/password_manager/core/browser/password_manager.cc:575: old_origin != new_origin && !new_origin.SchemeIsCryptographic(); Using |provisional_save_manager_| does not work here, which is why I am comparing against main_frame_url_. Ideally we would keep a vector of visited origins in the past, so that we can do the analysis that is mentioned in the doc. I would like to hear your feedback here, though.
https://codereview.chromium.org/2607413003/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2607413003/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1457: first_done_observer.SetPathToWaitFor("/password/done.html"); Looks like an unnecessary step. You can land on simple_password.html directly. https://codereview.chromium.org/2607413003/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1493: run_loop.RunUntilIdle(); It's dangerous in the browser test. Use WaitForPasswordStore(). https://codereview.chromium.org/2607413003/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1496: CheckThatCredentialsStored(password_store.get(), base::ASCIIToUTF16("user"), On 2017/01/04 16:55:18, jdoerrie wrote: > This test currently fails, because > |ShouldBlockPasswordForSameOriginButDifferentScheme| returns false due to > different ports. During an example run of the test requests were served from the > following origins: > > https://127.0.0.1:45407/ > http://127.0.0.1:45779/ > > I would like to avoid the port mismatch here, but I'm not quite sure how I would > do this. Dropping the port would work, or setting them to their default values > of 80 and 443. > > Unfortunately, I do not think that is possible. Is there any other possible > approach? Sounds like a bug in production. In the real world the ports will be different (80 vs. 443). The attacker can redirect the user to any port. https://codereview.chromium.org/2607413003/diff/20001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/2607413003/diff/20001/components/password_man... components/password_manager/core/browser/password_manager.cc:575: old_origin != new_origin && !new_origin.SchemeIsCryptographic(); On 2017/01/04 16:55:18, jdoerrie wrote: > Using |provisional_save_manager_| does not work here, which is why I am > comparing against main_frame_url_. Ideally we would keep a vector of visited > origins in the past, so that we can do the analysis that is mentioned in the > doc. I would like to hear your feedback here, though. The problem with implementing it in PasswordManager is that you don't know the state of the UI. The following is possible: - I submit an HTTPS form and save the password. - Then I browse the HTTP pages of the same site. - I submit an HTTP form on that site. The bubble doesn't appear. Maybe that behavior is even justifiable. However, if I open the same form in another tab then the bubble will appear. That sounds broken indeed. Not sure how the vector of origins helps you. You can though add a code to the UI layer to notify PasswordManager when the bubble is gone. So PasswordManager will use ShouldBlockPasswordForSameOriginButDifferentScheme logic only while the UI is active. Usage of |main_frame_url_| seems reasonable to me. What's the concern with it?
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jdoerrie@chromium.org changed reviewers: + dvadym@chromium.org
Dear both, PTAL :) dvadym@chromium.org: Please review changes in components/autofill/core/common/save_password_progress_logger.* https://codereview.chromium.org/2607413003/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2607413003/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1457: first_done_observer.SetPathToWaitFor("/password/done.html"); On 2017/01/05 11:26:11, vasilii wrote: > Looks like an unnecessary step. You can land on simple_password.html directly. You are right. My intention was to simulate an attack, where the user is normally redirected to done.html before the attacker intervenes, who then also redirects to done.html after submitting the malicious credential. This might be overkill for this test though. https://codereview.chromium.org/2607413003/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1493: run_loop.RunUntilIdle(); On 2017/01/05 11:26:11, vasilii wrote: > It's dangerous in the browser test. Use WaitForPasswordStore(). WaitForPasswordStore() is only implemented for CredentialManagerBrowserTest, not PasswordManagerBrowserTestBase. Should I move it into that class instead? Other tests in this file use |base::RunLoop::RunUntilIdle()|, are they all unsafe? https://codereview.chromium.org/2607413003/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1496: CheckThatCredentialsStored(password_store.get(), base::ASCIIToUTF16("user"), On 2017/01/05 11:26:11, vasilii wrote: > On 2017/01/04 16:55:18, jdoerrie wrote: > > This test currently fails, because > > |ShouldBlockPasswordForSameOriginButDifferentScheme| returns false due to > > different ports. During an example run of the test requests were served from > the > > following origins: > > > > https://127.0.0.1:45407/ > > http://127.0.0.1:45779/ > > > > I would like to avoid the port mismatch here, but I'm not quite sure how I > would > > do this. Dropping the port would work, or setting them to their default values > > of 80 and 443. > > > > Unfortunately, I do not think that is possible. Is there any other possible > > approach? > > Sounds like a bug in production. In the real world the ports will be different > (80 vs. 443). The attacker can redirect the user to any port. Acknowledged, dropped the check for port equality. https://codereview.chromium.org/2607413003/diff/20001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/2607413003/diff/20001/components/password_man... components/password_manager/core/browser/password_manager.cc:575: old_origin != new_origin && !new_origin.SchemeIsCryptographic(); On 2017/01/05 11:26:11, vasilii wrote: > On 2017/01/04 16:55:18, jdoerrie wrote: > > Using |provisional_save_manager_| does not work here, which is why I am > > comparing against main_frame_url_. Ideally we would keep a vector of visited > > origins in the past, so that we can do the analysis that is mentioned in the > > doc. I would like to hear your feedback here, though. > > The problem with implementing it in PasswordManager is that you don't know the > state of the UI. The following is possible: > - I submit an HTTPS form and save the password. > - Then I browse the HTTP pages of the same site. > - I submit an HTTP form on that site. The bubble doesn't appear. > Maybe that behavior is even justifiable. However, if I open the same form in > another tab then the bubble will appear. That sounds broken indeed. > Not sure how the vector of origins helps you. You can though add a code to the > UI layer to notify PasswordManager when the bubble is gone. So PasswordManager > will use ShouldBlockPasswordForSameOriginButDifferentScheme logic only while the > UI is active. > Usage of |main_frame_url_| seems reasonable to me. What's the concern with it? Discussed offline, the current solution is not ideal, but very simple and does address the issue raised in the doc. https://codereview.chromium.org/2607413003/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2607413003/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1427: ASSERT_TRUE(https_test_server.Start()); This part probably should be moved into its own method to avoid code duplication with NoPromptForLoginFailedAndServerPushSeperateLoginForm_HttpsToHttp and CredentialManagerBrowserTest::StoreSavesPSLMatchedCredential. Will do a follow up here. https://codereview.chromium.org/2607413003/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/2607413003/diff/40001/components/password_man... components/password_manager/core/browser/password_manager.cc:577: } Does it make sense to avoid the declaration in the header file and just move this logic in the anonymous namespace on the top of this file (taking main_frame_url_ as an extra argument)?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
What do you think about adding URLs in logging of this event? My feeling that it would be easier to debug.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please change the description to a non-draft version. The tracking bug is 571580. https://codereview.chromium.org/2607413003/diff/20001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2607413003/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1457: first_done_observer.SetPathToWaitFor("/password/done.html"); On 2017/01/05 18:11:30, jdoerrie wrote: > On 2017/01/05 11:26:11, vasilii wrote: > > Looks like an unnecessary step. You can land on simple_password.html directly. > > You are right. My intention was to simulate an attack, where the user is > normally redirected to done.html before the attacker intervenes, who then also > redirects to done.html after submitting the malicious credential. This might be > overkill for this test though. The attacker don't need to redirect to simple_password.html. He can just inject the content to done.html. Thus, it's indeed an overkill. https://codereview.chromium.org/2607413003/diff/20001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1493: run_loop.RunUntilIdle(); On 2017/01/05 18:11:30, jdoerrie wrote: > On 2017/01/05 11:26:11, vasilii wrote: > > It's dangerous in the browser test. Use WaitForPasswordStore(). > > WaitForPasswordStore() is only implemented for CredentialManagerBrowserTest, not > PasswordManagerBrowserTestBase. Should I move it into that class instead? Other > tests in this file use |base::RunLoop::RunUntilIdle()|, are they all unsafe? They are all unsafe. The documentation says clearly that RunUntilIdle() may never return. You can move WaitForPasswordStore() here. https://codereview.chromium.org/2607413003/diff/40001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2607413003/diff/40001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:1427: ASSERT_TRUE(https_test_server.Start()); On 2017/01/05 18:11:30, jdoerrie wrote: > This part probably should be moved into its own method to avoid code duplication > with NoPromptForLoginFailedAndServerPushSeperateLoginForm_HttpsToHttp and > CredentialManagerBrowserTest::StoreSavesPSLMatchedCredential. Will do a follow > up here. Acknowledged. https://codereview.chromium.org/2607413003/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/2607413003/diff/40001/components/password_man... components/password_manager/core/browser/password_manager.cc:577: } On 2017/01/05 18:11:30, jdoerrie wrote: > Does it make sense to avoid the declaration in the header file and just move > this logic in the anonymous namespace on the top of this file (taking > main_frame_url_ as an extra argument)? Only if you have a strong reason not to unittest this method ;-)
https://codereview.chromium.org/2607413003/diff/60001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2607413003/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:142: .WillByDefault(ReturnRef(GURL::EmptyGURL())); Why wasn't it required before? https://codereview.chromium.org/2607413003/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:784: .WillOnce(WithArg<0>(SaveToScopedPtr(&form_manager_to_save))); Move it before the line that triggers the call.
Description was changed from ========== First Version Implement check against overriding provisional saved forms BUG= ========== to ========== Add security feature to ProvisionalSavePassword Implement check against overriding provisional saved forms when the origin is the same but the scheme is insecure. Adds logging to understand how often this occurs. BUG=571580 ==========
Please take another look :) https://codereview.chromium.org/2607413003/diff/60001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2607413003/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:142: .WillByDefault(ReturnRef(GURL::EmptyGURL())); On 2017/01/09 16:42:16, vasilii wrote: > Why wasn't it required before? This wasn't required before because only in this CL I introduced the mock of GetMainFrameURL. If these two lines are omitted all tests that don't define the behavior of the mock crash (see e.g. the trybot results after Patch Set 3). This crash happens because we return a GURL by const-ref and not by value. Otherwise the default behavior of gmock would work. Ideally I would only define the mock inside my test, but I think the mock needs to be defined in the class already. https://codereview.chromium.org/2607413003/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:784: .WillOnce(WithArg<0>(SaveToScopedPtr(&form_manager_to_save))); On 2017/01/09 16:42:16, vasilii wrote: > Move it before the line that triggers the call. Actually this is almost as late as possible. The call to PromtUserToSaveOrUpdatePassword is triggered in OnLoginSuccessful which is called by OnPasswordFormsRendered, which is only three lines later. I would prefer to not split the following block because it's logically connected.
Please take another look :) https://codereview.chromium.org/2607413003/diff/60001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2607413003/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:142: .WillByDefault(ReturnRef(GURL::EmptyGURL())); On 2017/01/09 16:42:16, vasilii wrote: > Why wasn't it required before? This wasn't required before because only in this CL I introduced the mock of GetMainFrameURL. If these two lines are omitted all tests that don't define the behavior of the mock crash (see e.g. the trybot results after Patch Set 3). This crash happens because we return a GURL by const-ref and not by value. Otherwise the default behavior of gmock would work. Ideally I would only define the mock inside my test, but I think the mock needs to be defined in the class already. https://codereview.chromium.org/2607413003/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:784: .WillOnce(WithArg<0>(SaveToScopedPtr(&form_manager_to_save))); On 2017/01/09 16:42:16, vasilii wrote: > Move it before the line that triggers the call. Actually this is almost as late as possible. The call to PromtUserToSaveOrUpdatePassword is triggered in OnLoginSuccessful which is called by OnPasswordFormsRendered, which is only three lines later. I would prefer to not split the following block because it's logically connected.
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2607413003/diff/60001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2607413003/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:784: .WillOnce(WithArg<0>(SaveToScopedPtr(&form_manager_to_save))); On 2017/01/10 16:56:05, jdoerrie wrote: > On 2017/01/09 16:42:16, vasilii wrote: > > Move it before the line that triggers the call. > > Actually this is almost as late as possible. The call to > PromtUserToSaveOrUpdatePassword is triggered in OnLoginSuccessful which is > called by OnPasswordFormsRendered, which is only three lines later. I would > prefer to not split the following block because it's logically connected. Sure? I'm not ;-) It's triggered from OnPasswordFormsRendered() in line 800. The problem with the current implementation is that you capture |form_manager_to_save| only once. Therefore, there is no way it can capture |insecure_form|. Moving the expectation below line 795 will harden the test.
https://codereview.chromium.org/2607413003/diff/80001/components/password_man... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/2607413003/diff/80001/components/password_man... components/password_manager/core/browser/password_manager.cc:282: Logger::STRING_BLOCK_PASSWORD_SAME_ORIGIN_INSECURE_SCHEME); What do you think about logging of urls here? I suspect it would be much more useful for debugging. You can introduce a new method similar to LogFormStructure https://cs.chromium.org/chromium/src/components/password_manager/core/browser... i.e. with creating custom log message. https://codereview.chromium.org/2607413003/diff/80001/components/password_man... File components/password_manager/core/browser/password_manager_metrics_util.cc (right): https://codereview.chromium.org/2607413003/diff/80001/components/password_man... components/password_manager/core/browser/password_manager_metrics_util.cc:109: "PasswordManager.ShouldBlockPasswordForSameOriginButDifferentScheme", You should add description of this metric in histograms.xml, check please some boolean histogram (for instance PasswordManager.FilledCredentialWasFromAndroidApp) how to do this.
jdoerrie@chromium.org changed reviewers: + isherman@chromium.org
isherman, please review changes in histograms.xml https://codereview.chromium.org/2607413003/diff/60001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2607413003/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:784: .WillOnce(WithArg<0>(SaveToScopedPtr(&form_manager_to_save))); On 2017/01/10 18:25:23, vasilii wrote: > On 2017/01/10 16:56:05, jdoerrie wrote: > > On 2017/01/09 16:42:16, vasilii wrote: > > > Move it before the line that triggers the call. > > > > Actually this is almost as late as possible. The call to > > PromtUserToSaveOrUpdatePassword is triggered in OnLoginSuccessful which is > > called by OnPasswordFormsRendered, which is only three lines later. I would > > prefer to not split the following block because it's logically connected. > > Sure? I'm not ;-) > It's triggered from OnPasswordFormsRendered() in line 800. > > The problem with the current implementation is that you capture > |form_manager_to_save| only once. Therefore, there is no way it can capture > |insecure_form|. Moving the expectation below line 795 will harden the test. Acknowledged. In the current form it does get called in line 789 and will result in a test failure if I move it down. The reason is that I also pass |did_stop_loading| as |true|. When changing it in line 789 and 794 to false, the test passes when I move it down. This is done in the most recent version.
https://codereview.chromium.org/2607413003/diff/60001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2607413003/diff/60001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:784: .WillOnce(WithArg<0>(SaveToScopedPtr(&form_manager_to_save))); On 2017/01/11 17:26:37, jdoerrie wrote: > On 2017/01/10 18:25:23, vasilii wrote: > > On 2017/01/10 16:56:05, jdoerrie wrote: > > > On 2017/01/09 16:42:16, vasilii wrote: > > > > Move it before the line that triggers the call. > > > > > > Actually this is almost as late as possible. The call to > > > PromtUserToSaveOrUpdatePassword is triggered in OnLoginSuccessful which is > > > called by OnPasswordFormsRendered, which is only three lines later. I would > > > prefer to not split the following block because it's logically connected. > > > > Sure? I'm not ;-) > > It's triggered from OnPasswordFormsRendered() in line 800. > > > > The problem with the current implementation is that you capture > > |form_manager_to_save| only once. Therefore, there is no way it can capture > > |insecure_form|. Moving the expectation below line 795 will harden the test. > > Acknowledged. In the current form it does get called in line 789 and will result > in a test failure if I move it down. The reason is that I also pass > |did_stop_loading| as |true|. When changing it in line 789 and 794 to false, the > test passes when I move it down. This is done in the most recent version. Something is completely wrong. Wait with changing |did_stop_loading|. How can it be that OnPasswordFormsRendered() triggers the UI? You pass nonempty |observed| to it. As the same form is still on the page then it can't be a successful login. Maybe it's triggered from the line 794?
Metrics LGTM though I'd suggest clarifying the metric's semantics by rewording the description. https://codereview.chromium.org/2607413003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2607413003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44484: + http://crbug.com/571580. Reading this description, I'm still somewhat confused as to what exactly is being measured here. I understand that there might be a need to be a bit circumspect so as to not spell out a potential attack vector, but I think this metric could still be clarified within those bounds. In particular, I'd recommend (1) documenting exactly when this metric is recorded, and (2) using multiple, shorter sentences to explain what is being measured. For example, something along the lines of: "This metric is recorded every time Chrome detects a new credential submitted via a form hosted on a secure scheme. The credential might be saved, or it might be blocked. It is blocked if Chrome has previously detected [or maybe currently has stored?] any credentials from a URL with the same origin, but a different and insecure scheme. That is, if there is already a credential saved for http://example.com, a new credential for https://example.com would be blocked." (I'm not sure whether the description I proposed is fully accurate, but hopefully it conveys the idea.)
Dear all, please take another look.
logging LGTM
https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... File chrome/browser/password_manager/credential_manager_browsertest.cc (right): https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/credential_manager_browsertest.cc:36: void WaitForPasswordStore() { It's now unused. https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_browsertest.cc:1472: We need WaitForPasswordStore() to be safe here. https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_browsertest.cc:1484: EXPECT_FALSE(password_store->IsEmpty()); Is it needed? https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_test_base.h (right): https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_test_base.h:92: class PasswordStoreResultsObserver Is it needed in the header? https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... File components/password_manager/core/browser/password_manager.h (right): https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager.h:220: // This prevents users from unintentionally storing credentials, see storing which credentials? https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_unittest.cc:762: form.origin = GURL("https://example.com/login"); Optionally: introduce an array with input data and expected result. Iterate over the array. See password_bubble_experiment_unittest.cc It's just less code in total. https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_unittest.cc:780: form.origin = GURL("http://example.com/login"); Looks suspiciously similar to the one above ;-) https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_unittest.cc:825: .WillRepeatedly(ReturnRef(secure_form.origin)); I want to be more careful about this one. It's used in ProvisionallySavePassword and may influence the test result. Please write the expectation before the places they are needed. Obviously, before OnPasswordFormSubmitted(insecure_form) you'll need another (unsecure) URL. https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_unittest.cc:860: form_manager_to_save->Save(); Optionally: EXPECT_THAT(form_manager_to_save->pending_credentials(), FormMatches(secure_form)); https://codereview.chromium.org/2607413003/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2607413003/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44482: + submission. The credential might be saved, or it might be blocked. It is Here and below: it's not saved, it's just proposed to be saved.
https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... File chrome/browser/password_manager/credential_manager_browsertest.cc (right): https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/credential_manager_browsertest.cc:36: void WaitForPasswordStore() { On 2017/01/12 14:59:20, vasilii wrote: > It's now unused. Done. I thought I had removed it, thanks for catching that. https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_browsertest.cc:1472: On 2017/01/12 14:59:21, vasilii wrote: > We need WaitForPasswordStore() to be safe here. Done. https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_browsertest.cc:1484: EXPECT_FALSE(password_store->IsEmpty()); On 2017/01/12 14:59:21, vasilii wrote: > Is it needed? No, it's implied by a successful execution of |CheckThatCredentialsStored|. I added it for consistency with other tests in this file, eg. |PromptForInputElementWithoutIdAndName| and |ChangePwdNoAccountStored|. https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_test_base.h (right): https://codereview.chromium.org/2607413003/diff/120001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_test_base.h:92: class PasswordStoreResultsObserver On 2017/01/12 14:59:21, vasilii wrote: > Is it needed in the header? Nope. Moved it into an anonymous namespace in the file where it's needed. https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... File components/password_manager/core/browser/password_manager.h (right): https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager.h:220: // This prevents users from unintentionally storing credentials, see On 2017/01/12 14:59:21, vasilii wrote: > storing which credentials? Changed the description to be hopefully more clear. https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_unittest.cc:762: form.origin = GURL("https://example.com/login"); On 2017/01/12 14:59:21, vasilii wrote: > Optionally: introduce an array with input data and expected result. Iterate over > the array. See password_bubble_experiment_unittest.cc > > It's just less code in total. Done. https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_unittest.cc:780: form.origin = GURL("http://example.com/login"); On 2017/01/12 14:59:21, vasilii wrote: > Looks suspiciously similar to the one above ;-) Done. https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_unittest.cc:825: .WillRepeatedly(ReturnRef(secure_form.origin)); On 2017/01/12 14:59:21, vasilii wrote: > I want to be more careful about this one. It's used in ProvisionallySavePassword > and may influence the test result. > Please write the expectation before the places they are needed. Obviously, > before OnPasswordFormSubmitted(insecure_form) you'll need another (unsecure) > URL. Done. https://codereview.chromium.org/2607413003/diff/120001/components/password_ma... components/password_manager/core/browser/password_manager_unittest.cc:860: form_manager_to_save->Save(); On 2017/01/12 14:59:21, vasilii wrote: > Optionally: EXPECT_THAT(form_manager_to_save->pending_credentials(), > FormMatches(secure_form)); Done. https://codereview.chromium.org/2607413003/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2607413003/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44482: + submission. The credential might be saved, or it might be blocked. It is On 2017/01/12 14:59:21, vasilii wrote: > Here and below: it's not saved, it's just proposed to be saved. Done.
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2607413003/diff/160001/components/password_ma... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2607413003/diff/160001/components/password_ma... components/password_manager/core/browser/password_manager_unittest.cc:851: form_manager_to_save->Save(); I think lines 849 and 851 aren't needed anymore.
The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still LGTM, thanks. https://codereview.chromium.org/2607413003/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2607413003/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44541: + submission. The credential might proposed to be saved, or it might be nit: s/proposed/be proposed https://codereview.chromium.org/2607413003/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44542: + blocked. It is blocked if the previously detected password form submission nit: s/the/a
https://codereview.chromium.org/2607413003/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2607413003/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44541: + submission. The credential might proposed to be saved, or it might be On 2017/01/12 20:35:10, Ilya Sherman wrote: > nit: s/proposed/be proposed Done. https://codereview.chromium.org/2607413003/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:44542: + blocked. It is blocked if the previously detected password form submission On 2017/01/12 20:35:10, Ilya Sherman wrote: > nit: s/the/a Acknowledged. Choice of language is a bit subtle here, but I think 'the' is more appropriate. This is because the credential is only blocked, if the immediately preceding credential was from the same domain and different secure scheme. In other words, https://example.com -> http://example.com is blocked, but https://example.com -> http://any.other.domain.com -> http://example.com is not. Subjectively I feel that using 'the' makes this relationship more clear.
The CQ bit was checked by jdoerrie@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dvadym@chromium.org, vasilii@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2607413003/#ps200001 (title: "Addressed nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by jdoerrie@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1484310728890080,
"parent_rev": "ab7a0ee9d3485c19da8b50af2d1cfc95fd6e4a98", "commit_rev":
"4ed25061887ba8183010f4cdd800dff04bcdc794"}
Message was sent while issue was closed.
Description was changed from ========== Add security feature to ProvisionalSavePassword Implement check against overriding provisional saved forms when the origin is the same but the scheme is insecure. Adds logging to understand how often this occurs. BUG=571580 ========== to ========== Add security feature to ProvisionalSavePassword Implement check against overriding provisional saved forms when the origin is the same but the scheme is insecure. Adds logging to understand how often this occurs. BUG=571580 Review-Url: https://codereview.chromium.org/2607413003 Cr-Commit-Position: refs/heads/master@{#443545} Committed: https://chromium.googlesource.com/chromium/src/+/4ed25061887ba8183010f4cdd800... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/4ed25061887ba8183010f4cdd800... |
