Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2643083003:
Show form not secure warnings for blob and filesystem URLs. (Closed)
DescriptionShow form not secure warnings for blob and filesystem URLs.
These URLs do not define a secure context by themselves. They can be
created by HTTP URLs in which case they should be treated same as HTTP
URLs. Additionally, there is a spoofing risk associated with these URLs,
so mark them as "not secure".
Note that this CL excludes data: URLs. data URLs are being marked as
not secure in https://crrev.com/2648353005, regardless of whether they
display a password/credit card field or not.
BUG=680810
Review-Url: https://codereview.chromium.org/2643083003
Cr-Commit-Position: refs/heads/master@{#449072}
Committed: https://chromium.googlesource.com/chromium/src/+/39f049e94f28f0c26dba4aae5fe7d2818d4c570d
Patch Set 1 #
Total comments: 8
Patch Set 2 : More tests #Patch Set 3 : Rebase #Patch Set 4 : Remove data URLs #Patch Set 5 : Rebase #Patch Set 6 : Rebase #Patch Set 7 : Fix tests #
Messages
Total messages: 37 (26 generated)
meacer@chromium.org changed reviewers: + estark@chromium.org
PTAL?
lgtm w/ nits. Thanks for taking this! https://codereview.chromium.org/2643083003/diff/1/chrome/browser/ssl/security... File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right): https://codereview.chromium.org/2643083003/diff/1/chrome/browser/ssl/security... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:958: // Tests that when a visible password field is detected on a data URL, and when Could you add one more test case that data URLs *without* password fields are security level NONE? If you want to be really thorough you could do that for blob and filesystem as well, but I think one would be sufficient. https://codereview.chromium.org/2643083003/diff/1/chrome/browser/ssl/security... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1024: // Same as above, but instead of a blob URL, this creates a filesystem URL. nit: instead of "above", use the name of the test "PasswordSecurityLevelDowngradedOnBlobUrl" in case the tests get reordered in future https://codereview.chromium.org/2643083003/diff/1/components/security_state/c... File components/security_state/core/security_state_unittest.cc (right): https://codereview.chromium.org/2643083003/diff/1/components/security_state/c... components/security_state/core/security_state_unittest.cc:101: void GetSecurityInfo(SecurityInfo* security_info) const { thanks :) https://codereview.chromium.org/2643083003/diff/1/components/security_state/c... components/security_state/core/security_state_unittest.cc:333: // command-line switch is NOT set. This will probably conflict with https://codereview.chromium.org/2635423002/ which is in the CQ and enables HTTP-bad by default for all platforms but iOS. In that CL I removed the tests of this nature (tests that test what happens when the flag is not set) so you could probably just remove this one too.
https://codereview.chromium.org/2643083003/diff/1/chrome/browser/ssl/security... File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right): https://codereview.chromium.org/2643083003/diff/1/chrome/browser/ssl/security... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:958: // Tests that when a visible password field is detected on a data URL, and when On 2017/01/19 22:42:51, estark wrote: > Could you add one more test case that data URLs *without* password fields are > security level NONE? > > If you want to be really thorough you could do that for blob and filesystem as > well, but I think one would be sufficient. Done for all urls. Refactored a bit as well. https://codereview.chromium.org/2643083003/diff/1/chrome/browser/ssl/security... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1024: // Same as above, but instead of a blob URL, this creates a filesystem URL. On 2017/01/19 22:42:51, estark wrote: > nit: instead of "above", use the name of the test > "PasswordSecurityLevelDowngradedOnBlobUrl" in case the tests get reordered in > future Done. https://codereview.chromium.org/2643083003/diff/1/components/security_state/c... File components/security_state/core/security_state_unittest.cc (right): https://codereview.chromium.org/2643083003/diff/1/components/security_state/c... components/security_state/core/security_state_unittest.cc:101: void GetSecurityInfo(SecurityInfo* security_info) const { On 2017/01/19 22:42:51, estark wrote: > thanks :) You're welcome :) https://codereview.chromium.org/2643083003/diff/1/components/security_state/c... components/security_state/core/security_state_unittest.cc:333: // command-line switch is NOT set. On 2017/01/19 22:42:51, estark wrote: > This will probably conflict with https://codereview.chromium.org/2635423002/ > which is in the CQ and enables HTTP-bad by default for all platforms but iOS. In > that CL I removed the tests of this nature (tests that test what happens when > the flag is not set) so you could probably just remove this one too. Thanks for the heads up. I'll wait for that CL to land and then rebase this.
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org Link to the patchset: https://codereview.chromium.org/2643083003/#ps40001 (title: "Rebase")
Rebased and landing.
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/01/20 20:54:50, commit-bot: I haz the power wrote: > 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_...) > mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) The tests are failing because data is marked as a secure scheme (sigh): https://cs.chromium.org/chromium/src/url/url_util.cc?rcl=0&l=55 I'll try removing it from there (I feel like this will end up in a blink-dev intent to deprecate). If that doesn't work, I'll add a hack to security_state.
On 2017/01/24 01:04:39, Mustafa Emre Acer wrote: > On 2017/01/20 20:54:50, commit-bot: I haz the power wrote: > > 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_...) > > mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > > The tests are failing because data is marked as a secure scheme (sigh): > https://cs.chromium.org/chromium/src/url/url_util.cc?rcl=0&l=55 > > I'll try removing it from there (I feel like this will end up in a blink-dev > intent to deprecate). If that doesn't work, I'll add a hack to security_state. Good news, removing data from that list only broke two tests, and both are SSLUITests: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... This seems doable.
The CQ bit was checked by meacer@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Show form not secure warnings for data, blob and filesystem URLs. BUG=680810 ========== to ========== Show form not secure warnings for blob and filesystem URLs. These URLs do not define a secure context by themselves. They can be created by HTTP URLs in which case they should be treated same as HTTP URLs. Additionally, there is a spoofing risk associated with these URLs, so mark them as "not secure". BUG=680810 ==========
Description was changed from ========== Show form not secure warnings for blob and filesystem URLs. These URLs do not define a secure context by themselves. They can be created by HTTP URLs in which case they should be treated same as HTTP URLs. Additionally, there is a spoofing risk associated with these URLs, so mark them as "not secure". BUG=680810 ========== to ========== Show form not secure warnings for blob and filesystem URLs. These URLs do not define a secure context by themselves. They can be created by HTTP URLs in which case they should be treated same as HTTP URLs. Additionally, there is a spoofing risk associated with these URLs, so mark them as "not secure". Note that this CL excludes data: URLs. data URLs are being marked as not secure in https://crrev.com/2648353005, regardless of whether they display a password/credit card field or not. BUG=680810 ==========
The CQ bit was checked by meacer@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_...) 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 meacer@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 meacer@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.
Okay, tests fixed, so landing this.
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org Link to the patchset: https://codereview.chromium.org/2643083003/#ps120001 (title: "Fix tests")
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": 120001, "attempt_start_ts": 1486584207006630,
"parent_rev": "c2b2fca2531d524ce5020683c81a7e9b57fbe52e", "commit_rev":
"39f049e94f28f0c26dba4aae5fe7d2818d4c570d"}
Message was sent while issue was closed.
Description was changed from ========== Show form not secure warnings for blob and filesystem URLs. These URLs do not define a secure context by themselves. They can be created by HTTP URLs in which case they should be treated same as HTTP URLs. Additionally, there is a spoofing risk associated with these URLs, so mark them as "not secure". Note that this CL excludes data: URLs. data URLs are being marked as not secure in https://crrev.com/2648353005, regardless of whether they display a password/credit card field or not. BUG=680810 ========== to ========== Show form not secure warnings for blob and filesystem URLs. These URLs do not define a secure context by themselves. They can be created by HTTP URLs in which case they should be treated same as HTTP URLs. Additionally, there is a spoofing risk associated with these URLs, so mark them as "not secure". Note that this CL excludes data: URLs. data URLs are being marked as not secure in https://crrev.com/2648353005, regardless of whether they display a password/credit card field or not. BUG=680810 Review-Url: https://codereview.chromium.org/2643083003 Cr-Commit-Position: refs/heads/master@{#449072} Committed: https://chromium.googlesource.com/chromium/src/+/39f049e94f28f0c26dba4aae5fe7... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/39f049e94f28f0c26dba4aae5fe7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
