Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2275123004:
Downgrade security state while displaying an SB interstitial (Closed)
DescriptionDowngrade security state while displaying an SB interstitial
BUG=615123
TBR=jialiul@chromium.org
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. Verify that the omnibox security indicator shows a red triangle for
all three warnings
Committed: https://crrev.com/49e363a7209ccde311170742af9844b619099c93
Cr-Commit-Position: refs/heads/master@{#414700}
Patch Set 1 #Patch Set 2 : Added tests #
Total comments: 8
Patch Set 3 : Style nits #Patch Set 4 : Rebased #Patch Set 5 : bugfix #Patch Set 6 : Moar bugfix (thanks trybots!!) #
Messages
Total messages: 29 (16 generated)
Description was changed from ========== Downgrade security state while displaying an 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. Verify that the omnibox security indicator shows a red triangle for all three warnings ========== to ========== Downgrade security state while displaying an 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. Verify that the omnibox security indicator shows a red triangle for all three warnings ==========
felt@chromium.org changed reviewers: + estark@chromium.org, jialiul@chromium.org
estark, jialiul, PTAL? https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right): https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1108: EXPECT_NE(0u, model_client->GetSecurityInfo().cert_status); Note: this line (which depends on having a model_client) needs to move up into ExpectSecurityIndicatorDowngrade(), but I'm leaving it here for now while I figure out why that check is failing in the dependent CL (https://codereview.chromium.org/2270283002/).
I like the way you did it as part of the whitelist check instead of a separate interstitial check. Mostly LG, just a question inline about cleaning up pending URLs. https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ui_manager.cc (right): https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ui_manager.cc:63: auto iter = pending_.find(url.GetWithEmptyPath()); optional nit: I'd have a slight preference for `return pending_.find(...) != pending_.end()` https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ui_manager.cc:166: AddToWhitelistUrlSet(resource, false /* Pending -> permanent */); I don't see pending URLs getting cleaned up anywhere in the event the user doesn't proceed. Maybe an else block here that removes a URL from the pending whitelist set? https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:254: if (entry->GetSSL().security_style == content::SECURITY_STYLE_UNKNOWN) { nit: could you add a comment around here along the lines of "Connection security information is still being initialized, but malware status might already be known"?
https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ui_manager.cc (right): https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ui_manager.cc:63: auto iter = pending_.find(url.GetWithEmptyPath()); On 2016/08/25 06:54:06, estark wrote: > optional nit: I'd have a slight preference for `return pending_.find(...) != > pending_.end()` Sure. Also updated Contains() for consistency. https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ui_manager.cc:166: AddToWhitelistUrlSet(resource, false /* Pending -> permanent */); On 2016/08/25 06:54:06, estark wrote: > I don't see pending URLs getting cleaned up anywhere in the event the user > doesn't proceed. Maybe an else block here that removes a URL from the pending > whitelist set? There isn't any need to explicitly remove them. Costs of leaving the pending entries?: * No memory cost -- the number of pending entries in any WC is going to be very small. * No behavioral difference from UI perspective. Costs of removing the pending entries?: * There is a small runtime cost to looking up the entry to remove them (run the WC getter, get the user data, and redo the lookup logic). Weighing the pros/cons I figured I'd just leave them. If you strongly think they should be cleaned up tho, I can add that; it isn't really that expensive to remove them. https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:254: if (entry->GetSSL().security_style == content::SECURITY_STYLE_UNKNOWN) { On 2016/08/25 06:54:06, estark wrote: > nit: could you add a comment around here along the lines of "Connection security > information is still being initialized, but malware status might already be > known"? Done.
LGTM On Thu, Aug 25, 2016 at 8:17 AM <felt@chromium.org> wrote: > > > https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/safe_bro... > File chrome/browser/safe_browsing/ui_manager.cc (right): > > > https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/ui_manager.cc:63: auto iter = > pending_.find(url.GetWithEmptyPath()); > On 2016/08/25 06:54:06, estark wrote: > > optional nit: I'd have a slight preference for `return > pending_.find(...) != > > pending_.end()` > > Sure. Also updated Contains() for consistency. > > > > https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/ui_manager.cc:166: > AddToWhitelistUrlSet(resource, false /* Pending -> permanent */); > On 2016/08/25 06:54:06, estark wrote: > > I don't see pending URLs getting cleaned up anywhere in the event the > user > > doesn't proceed. Maybe an else block here that removes a URL from the > pending > > whitelist set? > > There isn't any need to explicitly remove them. > > Costs of leaving the pending entries?: > * No memory cost -- the number of pending entries in any WC is going to > be very small. > * No behavioral difference from UI perspective. > > Costs of removing the pending entries?: > * There is a small runtime cost to looking up the entry to remove them > (run the WC getter, get the user data, and redo the lookup logic). > > Weighing the pros/cons I figured I'd just leave them. If you strongly > think they should be cleaned up tho, I can add that; it isn't really > that expensive to remove them. > > > > https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/ssl/chro... > File chrome/browser/ssl/chrome_security_state_model_client.cc (right): > > > https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/ssl/chro... > chrome/browser/ssl/chrome_security_state_model_client.cc:254: if > (entry->GetSSL().security_style == content::SECURITY_STYLE_UNKNOWN) { > On 2016/08/25 06:54:06, estark wrote: > > nit: could you add a comment around here along the lines of > "Connection security > > information is still being initialized, but malware status might > already be > > known"? > > Done. > > https://codereview.chromium.org/2275123004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ui_manager.cc (right): https://codereview.chromium.org/2275123004/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ui_manager.cc:166: AddToWhitelistUrlSet(resource, false /* Pending -> permanent */); On 2016/08/25 15:17:27, felt wrote: > On 2016/08/25 06:54:06, estark wrote: > > I don't see pending URLs getting cleaned up anywhere in the event the user > > doesn't proceed. Maybe an else block here that removes a URL from the pending > > whitelist set? > > There isn't any need to explicitly remove them. > > Costs of leaving the pending entries?: > * No memory cost -- the number of pending entries in any WC is going to be very > small. > * No behavioral difference from UI perspective. > > Costs of removing the pending entries?: > * There is a small runtime cost to looking up the entry to remove them (run the > WC getter, get the user data, and redo the lookup logic). > > Weighing the pros/cons I figured I'd just leave them. If you strongly think they > should be cleaned up tho, I can add that; it isn't really that expensive to > remove them. Ah, ok, I see. That sounds fine to me. Thanks for the explanation.
The CQ bit was checked by felt@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/2275123004/#ps80001 (title: "bugfix")
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...)
jialiu, l-g-t-m's don't work from email responses. can you reply on thread please? thank you!
Description was changed from ========== Downgrade security state while displaying an 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. Verify that the omnibox security indicator shows a red triangle for all three warnings ========== to ========== Downgrade security state while displaying an SB interstitial BUG=615123 TBR=jialiul@chromium.org 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. Verify that the omnibox security indicator shows a red triangle for all three warnings ==========
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) 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 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_...)
The CQ bit was checked by felt@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/2275123004/#ps100001 (title: "Moar bugfix (thanks trybots!!)")
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 while displaying an SB interstitial BUG=615123 TBR=jialiul@chromium.org 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. Verify that the omnibox security indicator shows a red triangle for all three warnings ========== to ========== Downgrade security state while displaying an SB interstitial BUG=615123 TBR=jialiul@chromium.org 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. Verify that the omnibox security indicator shows a red triangle for all three warnings ==========
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 while displaying an SB interstitial BUG=615123 TBR=jialiul@chromium.org 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. Verify that the omnibox security indicator shows a red triangle for all three warnings ========== to ========== Downgrade security state while displaying an SB interstitial BUG=615123 TBR=jialiul@chromium.org 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. Verify that the omnibox security indicator shows a red triangle for all three warnings Committed: https://crrev.com/49e363a7209ccde311170742af9844b619099c93 Cr-Commit-Position: refs/heads/master@{#414700} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/49e363a7209ccde311170742af9844b619099c93 Cr-Commit-Position: refs/heads/master@{#414700} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
