Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2917873004:
Implement 'Not secure' warning for non-secure pages in Incognito mode (Closed)
|
Created:
3 years, 6 months ago by elawrence Modified:
3 years, 6 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), jam, noyau+watch_chromium.org, marq+watch_chromium.org, darin-cc_chromium.org, Eugene But (OOO till 7-30) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement 'Not secure' warning for non-secure pages in Incognito mode
Non-secure HTTP cannot meet users’ security and privacy expectations
when browsing the web. This change marks HTTP pages as 'Not secure' in
the Security Chip UI when the user is browsing in Incognito mode.
BUG=718556
TESTS=
browser_tests.exe --gtest_filter=SecurityStateTabHelperIncognitoTest.*
browser_tests.exe --gtest_filter=SecurityStateTabHelperTest.PasswordSecurityLevelDowngraded
browser_tests.exe --gtest_filter=SecurityStateTabHelperTest.SecurityLevelNotDowngradedForHTTPInGuestMode
components_unittests.exe --gtest_filter=SecurityStateTest.IncognitoFlagPropagates
components_unittests.exe --gtest_filter=SecurityStateTest.MarkHttpAsStatusHistogram
components_unittests.exe --gtest_filter=SecurityStateContentUtilsTest.HTTPWarning
Review-Url: https://codereview.chromium.org/2917873004
Cr-Commit-Position: refs/heads/master@{#479494}
Committed: https://chromium.googlesource.com/chromium/src/+/bb56996bdeb8a961f734140764e6c8e2218b7b21
Patch Set 1 #Patch Set 2 : Add tests and lint #Patch Set 3 : Add histogram test #Patch Set 4 : Ensure sensitive fields trigger warnings outside of Incognito #
Total comments: 4
Patch Set 5 : Move console log to Navigation completion #
Total comments: 20
Patch Set 6 : Address feedback #Patch Set 7 : Fix Guest mode test #
Total comments: 18
Patch Set 8 : Address feedback #Patch Set 9 : Rebase #Patch Set 10 : Remove obsolete includes #Messages
Total messages: 50 (38 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) 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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Description was changed from ========== Implement 'Not secure' warning for non-secure pages in Incognito mode Non-secure HTTP cannot meet users’ security and privacy expectations when browsing the web. This change marks HTTP pages as 'Not secure' in the Security Chip UI when the user is browsing in Incognito mode. BUG=718556 ========== to ========== Implement 'Not secure' warning for non-secure pages in Incognito mode Non-secure HTTP cannot meet users’ security and privacy expectations when browsing the web. This change marks HTTP pages as 'Not secure' in the Security Chip UI when the user is browsing in Incognito mode. BUG=718556 TESTS= browser_tests.exe --gtest_filter=SecurityStateTabHelperIncognitoTest.* components_unittests.exe --gtest_filter=SecurityStateTest.IncognitoFlagPropagates components_unittests.exe --gtest_filter=SecurityStateTest.MarkHttpAsStatusHistogram components_unittests.exe --gtest_filter=SecurityStateContentUtilsTest.HTTPWarning ==========
elawrence@chromium.org changed reviewers: + estark@chromium.org
PTAL? Much of the code change relates to handling the various states of the MarkNonSecureAs flag, and thus will be changing again soon when the MarkHttpAsNonSecureAfterEditing portion of HTTPBad Phase2 is added. https://codereview.chromium.org/2917873004/diff/100001/chrome/browser/ssl/sec... File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right): https://codereview.chromium.org/2917873004/diff/100001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:952: private: git cl lint demands this. https://codereview.chromium.org/2917873004/diff/100001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1383: helper->GetSecurityInfo(&security_info); Opportunistic fix. https://codereview.chromium.org/2917873004/diff/100001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1460: helper->GetSecurityInfo(&security_info); Opportunistic fix. https://codereview.chromium.org/2917873004/diff/100001/components/security_st... File components/security_state/core/security_state.h (right): https://codereview.chromium.org/2917873004/diff/100001/components/security_st... components/security_state/core/security_state.h:5: #ifndef COMPONENTS_SECURITY_STATE_CORE_SECURITY_STATE_H_ git cl lint insisted.
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2917873004/diff/150001/chrome/browser/ssl/sec... File chrome/browser/ssl/security_state_tab_helper.cc (right): https://codereview.chromium.org/2917873004/diff/150001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper.cc:118: !navigation_handle->IsSameDocument()) { I think this needs a `&& navigation_handle->HasCommitted()` + test as well. (sorry, existing bug, but might as well fix it here since it'll apply to the incognito case) https://codereview.chromium.org/2917873004/diff/150001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper.cc:214: if (!state->certificate && Curious why check the certificate here? You could check content::IsOriginSecure(web_contents()->GetController().GetVisibleEntry()->GetURL()) to be consistent with Phase 1 (https://cs.chromium.org/chromium/src/components/password_manager/content/brow...) https://codereview.chromium.org/2917873004/diff/150001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper.cc:215: security_state::IsHttpWarningForIncognitoEnabled()) { IIRC, the reason for checking the field trial here was to avoid checking the field trial on every call, but it doesn't look like you're caching the result of the field trial check anywhere. I suggest either: - Check security_state::IsHttpWarningForIncognitoEnabled() once in the SecurityStateTabHelper constructor and remember the result as a data member, and rename VisibleSecurityState::is_incognito to |is_incognito_warning_enabled| or something to indicate that it might be false even when we are in incognito. - OR decide that we don't care about the cost of checking the field trial (I think that would probably be reasonable given that it hasn't been an issue so far). In that case I'd make VisibileSecurityState::is_incognito do what it says on the tin (set it to true if and only if we are in incognito mode) and do the field trial check inside security_state.cc where we do the other field trial checks. https://codereview.chromium.org/2917873004/diff/150001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper.cc:218: !Profile::FromBrowserContext(context)->IsGuestSession()) { If it's not too hard, could you add a test that the warning isn't shown in guest mode? https://codereview.chromium.org/2917873004/diff/150001/components/security_st... File components/security_state/core/security_state.h (right): https://codereview.chromium.org/2917873004/diff/150001/components/security_st... components/security_state/core/security_state.h:144: // True if |IsHttpWarningForIncognitoEnabled| and the page was displayed in a nit: IsHttpWarningForIncognitoEnabled() https://codereview.chromium.org/2917873004/diff/150001/components/security_st... components/security_state/core/security_state.h:146: bool is_incognito; This also could use a more descriptive name; how about |did_warn_for_incognito| or something like that? I'm thinking it would be nice if the name makes it clear that it can be false even when we are in incognito. https://codereview.chromium.org/2917873004/diff/150001/components/security_st... components/security_state/core/security_state.h:187: // True if |IsHttpWarningForIncognitoEnabled| and the page was displayed in a nitto https://codereview.chromium.org/2917873004/diff/150001/components/security_st... components/security_state/core/security_state.h:189: bool is_incognito; I mentioned this a few files ago, but I think this should either be named more descriptively, or should really just mean "is incognito". https://codereview.chromium.org/2917873004/diff/150001/components/security_st... File components/security_state/core/security_state_unittest.cc (right): https://codereview.chromium.org/2917873004/diff/150001/components/security_st... components/security_state/core/security_state_unittest.cc:420: helper.set_displayed_password_field_on_http(false); set_is_incognito? https://codereview.chromium.org/2917873004/diff/150001/components/security_st... components/security_state/core/security_state_unittest.cc:446: helper.set_displayed_password_field_on_http(false); ditto
Description was changed from ========== Implement 'Not secure' warning for non-secure pages in Incognito mode Non-secure HTTP cannot meet users’ security and privacy expectations when browsing the web. This change marks HTTP pages as 'Not secure' in the Security Chip UI when the user is browsing in Incognito mode. BUG=718556 TESTS= browser_tests.exe --gtest_filter=SecurityStateTabHelperIncognitoTest.* components_unittests.exe --gtest_filter=SecurityStateTest.IncognitoFlagPropagates components_unittests.exe --gtest_filter=SecurityStateTest.MarkHttpAsStatusHistogram components_unittests.exe --gtest_filter=SecurityStateContentUtilsTest.HTTPWarning ========== to ========== Implement 'Not secure' warning for non-secure pages in Incognito mode Non-secure HTTP cannot meet users’ security and privacy expectations when browsing the web. This change marks HTTP pages as 'Not secure' in the Security Chip UI when the user is browsing in Incognito mode. BUG=718556 TESTS= browser_tests.exe --gtest_filter=SecurityStateTabHelperIncognitoTest.* browser_tests.exe --gtest_filter=SecurityStateTabHelperTest.PasswordSecurityLevelDowngraded browser_tests.exe --gtest_filter=SecurityStateTabHelperTest.SecurityLevelNotDowngradedForHTTPInGuestMode components_unittests.exe --gtest_filter=SecurityStateTest.IncognitoFlagPropagates components_unittests.exe --gtest_filter=SecurityStateTest.MarkHttpAsStatusHistogram components_unittests.exe --gtest_filter=SecurityStateContentUtilsTest.HTTPWarning ==========
Please take another look? https://codereview.chromium.org/2917873004/diff/150001/chrome/browser/ssl/sec... File chrome/browser/ssl/security_state_tab_helper.cc (right): https://codereview.chromium.org/2917873004/diff/150001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper.cc:118: !navigation_handle->IsSameDocument()) { On 2017/06/09 05:09:38, estark wrote: > I think this needs a `&& navigation_handle->HasCommitted()` + test as well. > (sorry, existing bug, but might as well fix it here since it'll apply to the > incognito case) Done and test added. The console clears when a navigation begins; if the navigation doesn't commit (e.g. goes to a download or a 204) then the console message will be lost and won't reflect the present state of the page. That's in contrast to the Security panel, which refreshes itself with the current page's data in this scenario. But I suppose that is in line with how the console works otherwise. https://codereview.chromium.org/2917873004/diff/150001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper.cc:214: if (!state->certificate && On 2017/06/09 05:09:37, estark wrote: > Curious why check the certificate here? You could check > content::IsOriginSecure(web_contents()->GetController().GetVisibleEntry()->GetURL()) > to be consistent with Phase 1 > (https://cs.chromium.org/chromium/src/components/password_manager/content/brow...) It was intended as a simple optimization to avoid performing more expensive work in the common case where the page is HTTPS. I'll revise this section to make it simpler. https://codereview.chromium.org/2917873004/diff/150001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper.cc:215: security_state::IsHttpWarningForIncognitoEnabled()) { On 2017/06/09 05:09:38, estark wrote: > IIRC, the reason for checking the field trial here was to avoid checking the > field trial on every call, but it doesn't look like you're caching the result of > the field trial check anywhere. I suggest either: > > - Check security_state::IsHttpWarningForIncognitoEnabled() once in the > SecurityStateTabHelper constructor and remember the result as a data member, and > rename VisibleSecurityState::is_incognito to |is_incognito_warning_enabled| or > something to indicate that it might be false even when we are in incognito. > > - OR decide that we don't care about the cost of checking the field trial (I > think that would probably be reasonable given that it hasn't been an issue so > far). In that case I'd make VisibileSecurityState::is_incognito do what it says > on the tin (set it to true if and only if we are in incognito mode) and do the > field trial check inside security_state.cc where we do the other field trial > checks. The is_incognito flag now does what it says on the tin. https://codereview.chromium.org/2917873004/diff/150001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper.cc:218: !Profile::FromBrowserContext(context)->IsGuestSession()) { On 2017/06/09 05:09:38, estark wrote: > If it's not too hard, could you add a test that the warning isn't shown in guest > mode? Added SecurityStateTabHelperTest.SecurityLevelNotDowngradedForHTTPInGuestMode. https://codereview.chromium.org/2917873004/diff/150001/components/security_st... File components/security_state/core/security_state.h (right): https://codereview.chromium.org/2917873004/diff/150001/components/security_st... components/security_state/core/security_state.h:144: // True if |IsHttpWarningForIncognitoEnabled| and the page was displayed in a On 2017/06/09 05:09:38, estark wrote: > nit: IsHttpWarningForIncognitoEnabled() Removed. https://codereview.chromium.org/2917873004/diff/150001/components/security_st... components/security_state/core/security_state.h:146: bool is_incognito; On 2017/06/09 05:09:38, estark wrote: > This also could use a more descriptive name; how about |did_warn_for_incognito| > or something like that? I'm thinking it would be nice if the name makes it clear > that it can be false even when we are in incognito. Simplified. It now always reflects the incognito state. https://codereview.chromium.org/2917873004/diff/150001/components/security_st... components/security_state/core/security_state.h:187: // True if |IsHttpWarningForIncognitoEnabled| and the page was displayed in a On 2017/06/09 05:09:38, estark wrote: > nitto Removed. https://codereview.chromium.org/2917873004/diff/150001/components/security_st... components/security_state/core/security_state.h:189: bool is_incognito; On 2017/06/09 05:09:38, estark wrote: > I mentioned this a few files ago, but I think this should either be named more > descriptively, or should really just mean "is incognito". Done. It now always reflects the incognito state. https://codereview.chromium.org/2917873004/diff/150001/components/security_st... File components/security_state/core/security_state_unittest.cc (right): https://codereview.chromium.org/2917873004/diff/150001/components/security_st... components/security_state/core/security_state_unittest.cc:420: helper.set_displayed_password_field_on_http(false); On 2017/06/09 05:09:38, estark wrote: > set_is_incognito? Gah, fixed. https://codereview.chromium.org/2917873004/diff/150001/components/security_st... components/security_state/core/security_state_unittest.cc:446: helper.set_displayed_password_field_on_http(false); On 2017/06/09 05:09:38, estark wrote: > ditto Gah, fixed.
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_...)
> > If it's not too hard, could you add a test that the warning isn't shown in > guest > > mode? > > Added SecurityStateTabHelperTest.SecurityLevelNotDowngradedForHTTPInGuestMode. Unfortunately, it appears that automated testing of Guest mode doesn't work very well; other tests attempting this (e.g. https://crbug.com/471953) have been disabled.
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.
lgtm with some nits https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right): https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1708: // Check that the expected console message is present. nit: "Check that no additional console message is present." https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1780: // Guest mode cannot be readily browser-tested on ChromeOS. Optional nit: Is there a bug or some other kind of reference you can link to for this? https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1800: base::RunLoop().RunUntilIdle(); Is this necessary? ProfileWindowBrowserTest::OpenGuestBrowser doesn't seem to do it. https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1806: EXPECT_TRUE(guest_browser); nit: ASSERT_TRUE to abort the test if false (otherwise the test will crash on the next line) https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1829: // Navigate to an HTTP page. Use a non-local hostname so that is it nit: is it => it is https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1843: EXPECT_TRUE(content::ExecuteScript( I think you could probably cut out this part of the test (lines 1842-1850) https://codereview.chromium.org/2917873004/diff/190001/components/security_st... File components/security_state/core/security_state.h (right): https://codereview.chromium.org/2917873004/diff/190001/components/security_st... components/security_state/core/security_state.h:145: bool is_incognito; optional nit: it might be more useful to have this field be |did_warn_for_incognito|, and only set it to true when we actually set HTTP_SHOW_WARNING because of incognito. That way, the consumers of this field don't have to know to also check for IsHttpWarningForIncognitoEnabled() and it doesn't have to be part of the public interface https://codereview.chromium.org/2917873004/diff/190001/ios/chrome/browser/ssl... File ios/chrome/browser/ssl/ios_security_state_tab_helper.mm (right): https://codereview.chromium.org/2917873004/diff/190001/ios/chrome/browser/ssl... ios/chrome/browser/ssl/ios_security_state_tab_helper.mm:66: if (!state->certificate && This conditional can go away now, right? Also, we really ought to have an iOS test for this, but I can't remember how to write such tests. I think it's okay to land as-is without adding one, but maybe we can add a test as a follow-up. eugenebut@ can probably help with that.
estark@chromium.org changed reviewers: + eugenebut@chromium.org
eugenebut, could you advise how to write a test for the change in ios/chrome/browser/ssl/ios_security_state_tab_helper.mm?
estark@chromium.org changed reviewers: - eugenebut@chromium.org
eugenebut@chromium.org changed reviewers: + eugenebut@chromium.org
https://codereview.chromium.org/2917873004/diff/190001/ios/chrome/browser/ssl... File ios/chrome/browser/ssl/ios_security_state_tab_helper.mm (right): https://codereview.chromium.org/2917873004/diff/190001/ios/chrome/browser/ssl... ios/chrome/browser/ssl/ios_security_state_tab_helper.mm:66: if (!state->certificate && On 2017/06/13 22:47:11, estark wrote: > This conditional can go away now, right? > > Also, we really ought to have an iOS test for this, but I can't remember how to > write such tests. I think it's okay to land as-is without adding one, but maybe > we can add a test as a follow-up. eugenebut@ can probably help with that. iOS team would really appreciate the test, which should not be hard to write: typedef web::WebTestWithWebController IOSSecurityStateTabHelperTest; TEST_F(IOSSecurityStateTabHelperTest, SecurityInfo) { AddPendingItem(GURL("http://chromium.test"), ui::PAGE_TRANSITION_TYPED); web::NavigationItem* item = web_state()->GetNavigationManager()->GetVisibleItem(); // configure |item| // add |TestBrowserState* test_browser_state()| to web::WebTest, because // |test_browser_state()| does not exist. test_browser_state()->SetOffTheRecord(...) IOSSecurityStateTabHelper::CreateForWebState(web_state()); security_state::SecurityInfo info; IOSSecurityStateTabHelper::FromWebState(web_state())->GetSecurityInfo(&info); // verify |info|. }
Thanks for the thorough review! https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right): https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1708: // Check that the expected console message is present. On 2017/06/13 22:47:11, estark wrote: > nit: "Check that no additional console message is present." Done. https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1780: // Guest mode cannot be readily browser-tested on ChromeOS. On 2017/06/13 22:47:11, estark wrote: > Optional nit: Is there a bug or some other kind of reference you can link to for > this? I'll try to find something better than https://www.chromium.org/chromium-os/chromiumos-design-docs/user-accounts-and.... Notably, the existing Guest test I found also ifdefs out running on ChromeOS. Naively trying to run the test on ChromeOS fails on a CHECK: https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile_impl.cc?... [ERROR:logging_chrome.cc(179)] Unable to create symlink /b/s/w/itigPkkL/.org.chromium.Chromium.XVWZC0/d2SbUfe/test-user/chrome_debug.log pointing at /b/s/w/itigPkkL/.org.chromium.Chromium.XVWZC0/d2SbUfe/test-user/chrome_debug_20170613-085134.log: No such file or directory [FATAL:profile_impl.cc(441)] Check failed: user. #0 base::debug::StackTrace::StackTrace() #1 logging::LogMessage::~LogMessage() #2 ProfileImpl::ProfileImpl() #3 Profile::CreateProfile() #4 ProfileManager::CreateProfileAsync() #5 profiles::SwitchToGuestProfile() https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1800: base::RunLoop().RunUntilIdle(); On 2017/06/13 22:47:11, estark wrote: > Is this necessary? ProfileWindowBrowserTest::OpenGuestBrowser doesn't seem to do > it. Removed. It doesn't appear to be necessary, although it was present in https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_command_contro.... https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1806: EXPECT_TRUE(guest_browser); On 2017/06/13 22:47:11, estark wrote: > nit: ASSERT_TRUE to abort the test if false (otherwise the test will crash on > the next line) Gah, fixed. That'll teach me to copy/paste out of random Chrome tests. :) https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1829: // Navigate to an HTTP page. Use a non-local hostname so that is it On 2017/06/13 22:47:11, estark wrote: > nit: is it => it is Fxied. https://codereview.chromium.org/2917873004/diff/190001/chrome/browser/ssl/sec... chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:1843: EXPECT_TRUE(content::ExecuteScript( On 2017/06/13 22:47:11, estark wrote: > I think you could probably cut out this part of the test (lines 1842-1850) Done. https://codereview.chromium.org/2917873004/diff/190001/components/security_st... File components/security_state/core/security_state.h (right): https://codereview.chromium.org/2917873004/diff/190001/components/security_st... components/security_state/core/security_state.h:145: bool is_incognito; On 2017/06/13 22:47:11, estark wrote: > optional nit: it might be more useful to have this field be > |did_warn_for_incognito|, and only set it to true when we actually set > HTTP_SHOW_WARNING because of incognito. That way, the consumers of this field > don't have to know to also check for IsHttpWarningForIncognitoEnabled() and it > doesn't have to be part of the public interface This is now |incognito_downgraded_security_level| and it does indeed make various things simpler. https://codereview.chromium.org/2917873004/diff/190001/ios/chrome/browser/ssl... File ios/chrome/browser/ssl/ios_security_state_tab_helper.mm (right): https://codereview.chromium.org/2917873004/diff/190001/ios/chrome/browser/ssl... ios/chrome/browser/ssl/ios_security_state_tab_helper.mm:66: if (!state->certificate && On 2017/06/13 22:47:11, estark wrote: > This conditional can go away now, right? #oops. Fixed. https://codereview.chromium.org/2917873004/diff/190001/ios/chrome/browser/ssl... ios/chrome/browser/ssl/ios_security_state_tab_helper.mm:66: if (!state->certificate && On 2017/06/14 01:18:10, Eugene But wrote: > On 2017/06/13 22:47:11, estark wrote: > > This conditional can go away now, right? > > > > Also, we really ought to have an iOS test for this, but I can't remember how > to > > write such tests. I think it's okay to land as-is without adding one, but > maybe > > we can add a test as a follow-up. eugenebut@ can probably help with that. > iOS team would really appreciate the test, which should not be hard to write: > > typedef web::WebTestWithWebController IOSSecurityStateTabHelperTest; > TEST_F(IOSSecurityStateTabHelperTest, SecurityInfo) { > AddPendingItem(GURL("http://chromium.test"), ui::PAGE_TRANSITION_TYPED); > web::NavigationItem* item = > web_state()->GetNavigationManager()->GetVisibleItem(); > // configure |item| > > // add |TestBrowserState* test_browser_state()| to web::WebTest, because > // |test_browser_state()| does not exist. > test_browser_state()->SetOffTheRecord(...) > > IOSSecurityStateTabHelper::CreateForWebState(web_state()); > security_state::SecurityInfo info; > IOSSecurityStateTabHelper::FromWebState(web_state())->GetSecurityInfo(&info); > // verify |info|. > } > Will do this in a followup. crbug.com/733311
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 checked by elawrence@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/2917873004/#ps250001 (title: "Remove obsolete includes")
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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
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": 250001, "attempt_start_ts": 1497472603537050,
"parent_rev": "d8fd21baec879f33a97f88b929a9307b531ceeb4", "commit_rev":
"bb56996bdeb8a961f734140764e6c8e2218b7b21"}
Message was sent while issue was closed.
Description was changed from ========== Implement 'Not secure' warning for non-secure pages in Incognito mode Non-secure HTTP cannot meet users’ security and privacy expectations when browsing the web. This change marks HTTP pages as 'Not secure' in the Security Chip UI when the user is browsing in Incognito mode. BUG=718556 TESTS= browser_tests.exe --gtest_filter=SecurityStateTabHelperIncognitoTest.* browser_tests.exe --gtest_filter=SecurityStateTabHelperTest.PasswordSecurityLevelDowngraded browser_tests.exe --gtest_filter=SecurityStateTabHelperTest.SecurityLevelNotDowngradedForHTTPInGuestMode components_unittests.exe --gtest_filter=SecurityStateTest.IncognitoFlagPropagates components_unittests.exe --gtest_filter=SecurityStateTest.MarkHttpAsStatusHistogram components_unittests.exe --gtest_filter=SecurityStateContentUtilsTest.HTTPWarning ========== to ========== Implement 'Not secure' warning for non-secure pages in Incognito mode Non-secure HTTP cannot meet users’ security and privacy expectations when browsing the web. This change marks HTTP pages as 'Not secure' in the Security Chip UI when the user is browsing in Incognito mode. BUG=718556 TESTS= browser_tests.exe --gtest_filter=SecurityStateTabHelperIncognitoTest.* browser_tests.exe --gtest_filter=SecurityStateTabHelperTest.PasswordSecurityLevelDowngraded browser_tests.exe --gtest_filter=SecurityStateTabHelperTest.SecurityLevelNotDowngradedForHTTPInGuestMode components_unittests.exe --gtest_filter=SecurityStateTest.IncognitoFlagPropagates components_unittests.exe --gtest_filter=SecurityStateTest.MarkHttpAsStatusHistogram components_unittests.exe --gtest_filter=SecurityStateContentUtilsTest.HTTPWarning Review-Url: https://codereview.chromium.org/2917873004 Cr-Commit-Position: refs/heads/master@{#479494} Committed: https://chromium.googlesource.com/chromium/src/+/bb56996bdeb8a961f734140764e6... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:250001) as https://chromium.googlesource.com/chromium/src/+/bb56996bdeb8a961f734140764e6... |
