Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2270283002:
Downgrade security state after user clicks through SB interstitial (Closed)
DescriptionDowngrade security state after user clicks through SB interstitial
BUG=615123
TEST=
1. Go to the following URLs in different tabs:
http://ianfette.org
http://parkerly.com/sb-tests/bad-subresources.html
https://parkerly.com/sb-tests/bad-subresources.html
2. Click through the warnings that appear
3. Verify that the omnibox security indicator shows a red triangle
Committed: https://crrev.com/6b00a3a260d63e917cb1534cf17876e7d8297688
Cr-Commit-Position: refs/heads/master@{#414638}
Patch Set 1 #Patch Set 2 : Remove unnecessary assert in tests #
Total comments: 9
Patch Set 3 : Updated test, added UIManager method wrapper #
Total comments: 8
Patch Set 4 : Addressed comments for estark #Patch Set 5 : For debugging (ignore this) #Patch Set 6 : Remove logging statements #
Messages
Total messages: 33 (19 generated)
Description was changed from ========== Downgrade security state after user clicks through SB interstitial BUG=615123 TEST= 1. Go to the following URLs in different tabs: http://ianfette.org http://parkerly.com/sb-tests/bad-subresources.html https://parkerly.com/sb-tests/bad-subresources.html 2. Click through the warnings that appear 3. Verify that the omnibox security indicator shows a red triangle ========== to ========== Downgrade security state after user clicks through SB interstitial BUG=615123 TEST= 1. Go to the following URLs in different tabs: http://ianfette.org http://parkerly.com/sb-tests/bad-subresources.html https://parkerly.com/sb-tests/bad-subresources.html 2. Click through the warnings that appear 3. Verify that the omnibox security indicator shows a red triangle ==========
felt@chromium.org changed reviewers: + estark@chromium.org, jialiul@chromium.org
estark, jialiul, PTAL?
The CQ bit was checked by felt@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/2270283002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1032: HTTPSOverridePostInterstitial) { Either I'm missing something or this test is not testing what it's supposed to be testing. Looks like it sets up an HTTPS server but then never makes any requests to it. https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:270: scoped_refptr<SafeBrowsingUIManager> sb_ui_manager = sb_service->ui_manager(); optional: it might be simpler to add a new SBUIManager method that takes a WebContents and a URL as arguments, and factor that method out of IsWhitelisted(). Something like: SBUIManager::IsWhitelisted(const UnsafeResource& resource) { ... return IsURLWhitelistedForWebContents(resource.web_contents_getter.Run(), maybe_whitelisted_url); } SBUIManager::IsURLWhitelistedForWebContents(WebContents* wc, const GURL& url) { site_list = wc->GetUserData(...); return site_list->Contains(url); } and then you can just call IsURLWhitelistedForWebContents(web_contents_, entry->GetURL()) here. (Besides simpler code, the other reason I'm thinking about this is that originally GetVisibleSecurityState() was supposed to be super fast, and its purpose was to zippily provide an object that SecurityStateModel memoizes on, to avoid computing a whole SecurityInfo every time the omnibox wants to redraw. To that end, I'm not sure if this WebContentsGetter stuff is fast or slow, so it would be nice to cut it out if possible. OTOH, I'm increasingly thinking that this whole memoize-on-VisibleSecurityState thing is more complicated than it's worth and quite possibly doesn't even actually buy us any performance.) https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:280: state->initial_security_level = SecurityStateModel::SECURITY_ERROR; Strictly speaking, you don't need this, right? The actual security level in the page's SecurityInfo will get set to SECURITY_ERROR in GetSecurityLevelForRequest() by virtue of having fails_malware_check == true.
https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1032: HTTPSOverridePostInterstitial) { This test seems very similar to the previous one, except the following line "SetUpMockCertVerifierForHttpsServer(0, net::OK);" Does this line mean there's be no cert error in this test case but only safe browsing error? If so, may be add another test case where cert error and safe browsing error are both detected on the same url. and some comments maybe helpful? https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:270: scoped_refptr<SafeBrowsingUIManager> sb_ui_manager = sb_service->ui_manager(); On 2016/08/24 at 05:15:39, estark wrote: > optional: it might be simpler to add a new SBUIManager method that takes a WebContents and a URL as arguments, and factor that method out of IsWhitelisted(). Something like: > > SBUIManager::IsWhitelisted(const UnsafeResource& resource) { > ... > return IsURLWhitelistedForWebContents(resource.web_contents_getter.Run(), maybe_whitelisted_url); > } > > SBUIManager::IsURLWhitelistedForWebContents(WebContents* wc, const GURL& url) { > site_list = wc->GetUserData(...); > return site_list->Contains(url); > } > > and then you can just call IsURLWhitelistedForWebContents(web_contents_, entry->GetURL()) here. > > (Besides simpler code, the other reason I'm thinking about this is that originally GetVisibleSecurityState() was supposed to be super fast, and its purpose was to zippily provide an object that SecurityStateModel memoizes on, to avoid computing a whole SecurityInfo every time the omnibox wants to redraw. To that end, I'm not sure if this WebContentsGetter stuff is fast or slow, so it would be nice to cut it out if possible. OTOH, I'm increasingly thinking that this whole memoize-on-VisibleSecurityState thing is more complicated than it's worth and quite possibly doesn't even actually buy us any performance.) I agree with estark@.
The CQ bit was checked by felt@chromium.org to run a CQ dry run
https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1032: HTTPSOverridePostInterstitial) { On 2016/08/24 05:15:39, estark wrote: > Either I'm missing something or this test is not testing what it's supposed to > be testing. Looks like it sets up an HTTPS server but then never makes any > requests to it. Whoops you're right, I was missing an if-statement. Fixed. https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1032: HTTPSOverridePostInterstitial) { On 2016/08/24 06:27:20, Jialiu Lin wrote: > This test seems very similar to the previous one, except the following line > "SetUpMockCertVerifierForHttpsServer(0, net::OK);" > Does this line mean there's be no cert error in this test case but only safe > browsing error? All of the existing SB tests, including SecurityStatePostInterstitial, are done over HTTP. This tests adds a test over valid HTTPS. > If so, may be add another test case where cert error and safe > browsing error are both detected on the same url. Done. > and some comments maybe helpful? Added comments. https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:270: scoped_refptr<SafeBrowsingUIManager> sb_ui_manager = sb_service->ui_manager(); On 2016/08/24 05:15:39, estark wrote: > optional: it might be simpler to add a new SBUIManager method that takes a > WebContents and a URL as arguments, and factor that method out of > IsWhitelisted(). Something like: > > SBUIManager::IsWhitelisted(const UnsafeResource& resource) { > ... > return IsURLWhitelistedForWebContents(resource.web_contents_getter.Run(), > maybe_whitelisted_url); > } > > SBUIManager::IsURLWhitelistedForWebContents(WebContents* wc, const GURL& url) { > site_list = wc->GetUserData(...); > return site_list->Contains(url); > } > > and then you can just call IsURLWhitelistedForWebContents(web_contents_, > entry->GetURL()) here. > > (Besides simpler code, the other reason I'm thinking about this is that > originally GetVisibleSecurityState() was supposed to be super fast, and its > purpose was to zippily provide an object that SecurityStateModel memoizes on, to > avoid computing a whole SecurityInfo every time the omnibox wants to redraw. To > that end, I'm not sure if this WebContentsGetter stuff is fast or slow, so it > would be nice to cut it out if possible. OTOH, I'm increasingly thinking that > this whole memoize-on-VisibleSecurityState thing is more complicated than it's > worth and quite possibly doesn't even actually buy us any performance.) I was trying to avoid introducing a new method to the UIManager just to wrap this, but done. https://codereview.chromium.org/2270283002/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:280: state->initial_security_level = SecurityStateModel::SECURITY_ERROR; On 2016/08/24 05:15:39, estark wrote: > Strictly speaking, you don't need this, right? The actual security level in the > page's SecurityInfo will get set to SECURITY_ERROR in > GetSecurityLevelForRequest() by virtue of having fails_malware_check == true. Strictly speaking, no. But it seems potentially confusing to leave the initial security level at a higher value, to me.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a few nits https://codereview.chromium.org/2270283002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2270283002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:375: GURL SetupWarningAndNavigateToURL(GURL url) { nit: looks like this could be private https://codereview.chromium.org/2270283002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1059: EXPECT_EQ(security_state::SecurityStateModel::SECURITY_ERROR, nit: could also check that model_client->GetSecurityInfo().fails_malware_check is true for all of these, if you feel so inclined https://codereview.chromium.org/2270283002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1077: model_client->GetSecurityInfo().security_level); nit: as a sanity check, you could do EXPECT_EQ(0, model_client->GetSecurityInfo().cert_status) here, and the same thing but EXPECT_NE in the test below. https://codereview.chromium.org/2270283002/diff/40001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2270283002/diff/40001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:21: #include "content/public/browser/render_frame_host.h" lines 21-22 not needed anymore
https://codereview.chromium.org/2270283002/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2270283002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:375: GURL SetupWarningAndNavigateToURL(GURL url) { On 2016/08/24 22:30:39, estark wrote: > nit: looks like this could be private Done. https://codereview.chromium.org/2270283002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1059: EXPECT_EQ(security_state::SecurityStateModel::SECURITY_ERROR, On 2016/08/24 22:30:39, estark wrote: > nit: could also check that model_client->GetSecurityInfo().fails_malware_check > is true for all of these, if you feel so inclined Done. https://codereview.chromium.org/2270283002/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1077: model_client->GetSecurityInfo().security_level); On 2016/08/24 22:30:39, estark wrote: > nit: as a sanity check, you could do EXPECT_EQ(0, > model_client->GetSecurityInfo().cert_status) here, and the same thing but > EXPECT_NE in the test below. Done. https://codereview.chromium.org/2270283002/diff/40001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2270283002/diff/40001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:21: #include "content/public/browser/render_frame_host.h" On 2016/08/24 22:30:39, estark wrote: > lines 21-22 not needed anymore Done.
The CQ bit was checked by felt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jialiul@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2270283002/#ps60001 (title: "Addressed comments for estark")
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by felt@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jialiul@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2270283002/#ps100001 (title: "Remove logging statements")
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 felt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Downgrade security state after user clicks through SB interstitial BUG=615123 TEST= 1. Go to the following URLs in different tabs: http://ianfette.org http://parkerly.com/sb-tests/bad-subresources.html https://parkerly.com/sb-tests/bad-subresources.html 2. Click through the warnings that appear 3. Verify that the omnibox security indicator shows a red triangle ========== to ========== Downgrade security state after user clicks through SB interstitial BUG=615123 TEST= 1. Go to the following URLs in different tabs: http://ianfette.org http://parkerly.com/sb-tests/bad-subresources.html https://parkerly.com/sb-tests/bad-subresources.html 2. Click through the warnings that appear 3. Verify that the omnibox security indicator shows a red triangle ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Downgrade security state after user clicks through SB interstitial BUG=615123 TEST= 1. Go to the following URLs in different tabs: http://ianfette.org http://parkerly.com/sb-tests/bad-subresources.html https://parkerly.com/sb-tests/bad-subresources.html 2. Click through the warnings that appear 3. Verify that the omnibox security indicator shows a red triangle ========== to ========== Downgrade security state after user clicks through SB interstitial BUG=615123 TEST= 1. Go to the following URLs in different tabs: http://ianfette.org http://parkerly.com/sb-tests/bad-subresources.html https://parkerly.com/sb-tests/bad-subresources.html 2. Click through the warnings that appear 3. Verify that the omnibox security indicator shows a red triangle Committed: https://crrev.com/6b00a3a260d63e917cb1534cf17876e7d8297688 Cr-Commit-Position: refs/heads/master@{#414638} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6b00a3a260d63e917cb1534cf17876e7d8297688 Cr-Commit-Position: refs/heads/master@{#414638} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
