Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2640903006:
Show FormNotSecure warnings on sensitive inputs in non-secure contexts (Closed)
|
Created:
3 years, 11 months ago by elawrence Modified:
3 years, 11 months ago CC:
estark, chromium-reviews, mlamouri+watch-content_chromium.org, jam, vabr+watchlistpasswordmanager_chromium.org, droger+watchlist_chromium.org, rouslan+autofill_chromium.org, blundell+watchlist_chromium.org, sebsg+autofillwatch_chromium.org, sdefresne+watchlist_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, darin-cc_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow FormNotSecure warnings on sensitive inputs in non-secure contexts
Ensure that we show the on-field warnings for sensitive input fields,
even if the user does not have any stored autofill data for the fields.
BUG=672663
Review-Url: https://codereview.chromium.org/2640903006
Cr-Commit-Position: refs/heads/master@{#446104}
Committed: https://chromium.googlesource.com/chromium/src/+/68da9592ad01a360f95d866a6b124be8bf766337
Patch Set 1 #Patch Set 2 : Update DEPS in build.gn #Patch Set 3 : Fixup tests #Patch Set 4 : Fix misplaced brace. #Patch Set 5 : Convert ui_interactive_test to use HTTPS to avoid breakage #
Total comments: 1
Messages
Total messages: 54 (45 generated)
The CQ bit was checked by elawrence@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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by elawrence@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by elawrence@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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by elawrence@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by elawrence@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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by elawrence@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: 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 elawrence@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by elawrence@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.
elawrence@chromium.org changed reviewers: + mathp@chromium.org
Please take a look? This change ensures that the Form-Not-Secure warnings are shown in the popup even if user does not have any credentials/creditcards cached for the target.
great work and thanks for the cleanup in autofill_manager! lgtm https://codereview.chromium.org/2640903006/diff/160001/components/autofill/co... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/2640903006/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_manager_unittest.cc:698: ASSERT_LE(expected_num_suggestions, suggestions_.size()); what happened between 698 and 707 warranting the different checks?
On 2017/01/25 16:28:50, Mathieu Perreault wrote: > great work and thanks for the cleanup in autofill_manager! lgtm > > https://codereview.chromium.org/2640903006/diff/160001/components/autofill/co... > File components/autofill/core/browser/autofill_manager_unittest.cc (right): > > https://codereview.chromium.org/2640903006/diff/160001/components/autofill/co... > components/autofill/core/browser/autofill_manager_unittest.cc:698: > ASSERT_LE(expected_num_suggestions, suggestions_.size()); > what happened between 698 and 707 warranting the different checks? The old assert on 698 was correct, but meant the fact that it's fatal meant that if the size differed from the expectation, the human diagnosing the test couldn't see whether the suggestions in the list were correct (and there was just an extra one at the end) or if they were entirely wrong. The relaxed assertion on 698 validates that the suggestions list contains AT LEAST the number of expected items, then the EXPECTs from 701 to 704 validate that the contents are as expected, then the assert on 707 finally validates that there are no extra items. I could change this back if you prefer.
On 2017/01/25 16:36:34, elawrence wrote: > On 2017/01/25 16:28:50, Mathieu Perreault wrote: > > great work and thanks for the cleanup in autofill_manager! lgtm > > > > > https://codereview.chromium.org/2640903006/diff/160001/components/autofill/co... > > File components/autofill/core/browser/autofill_manager_unittest.cc (right): > > > > > https://codereview.chromium.org/2640903006/diff/160001/components/autofill/co... > > components/autofill/core/browser/autofill_manager_unittest.cc:698: > > ASSERT_LE(expected_num_suggestions, suggestions_.size()); > > what happened between 698 and 707 warranting the different checks? > > The old assert on 698 was correct, but meant the fact that it's fatal meant that > if the size differed from the expectation, the human diagnosing the test > couldn't see whether the suggestions in the list were correct (and there was > just an extra one at the end) or if they were entirely wrong. > > The relaxed assertion on 698 validates that the suggestions list contains AT > LEAST the number of expected items, then the EXPECTs from 701 to 704 validate > that the contents are as expected, then the assert on 707 finally validates that > there are no extra items. > > I could change this back if you prefer. It's good. Interesting trick :)
The CQ bit was checked by elawrence@chromium.org
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Show FormNotSecure warnings on sensitive inputs in non-secure contexts Ensure that we show the on-field warnings for sensitive input fields, even if the user does not have any stored autofill data for the fields. BUG=672663 ========== to ========== Show FormNotSecure warnings on sensitive inputs in non-secure contexts Ensure that we show the on-field warnings for sensitive input fields, even if the user does not have any stored autofill data for the fields. BUG=672663 ==========
elawrence@chromium.org changed reviewers: + estark@chromium.org
elawrence@chromium.org changed required reviewers: + estark@chromium.org
lgtm, thanks!
The CQ bit was checked by elawrence@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": 160001, "attempt_start_ts": 1485375310182520,
"parent_rev": "93392878eba7dd29a5cde0b510ce6ab514e50f3c", "commit_rev":
"68da9592ad01a360f95d866a6b124be8bf766337"}
Message was sent while issue was closed.
Description was changed from ========== Show FormNotSecure warnings on sensitive inputs in non-secure contexts Ensure that we show the on-field warnings for sensitive input fields, even if the user does not have any stored autofill data for the fields. BUG=672663 ========== to ========== Show FormNotSecure warnings on sensitive inputs in non-secure contexts Ensure that we show the on-field warnings for sensitive input fields, even if the user does not have any stored autofill data for the fields. BUG=672663 Review-Url: https://codereview.chromium.org/2640903006 Cr-Commit-Position: refs/heads/master@{#446104} Committed: https://chromium.googlesource.com/chromium/src/+/68da9592ad01a360f95d866a6b12... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001) as https://chromium.googlesource.com/chromium/src/+/68da9592ad01a360f95d866a6b12... |
