Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2481743009:
Expose malware/phishing/etc. distinction from SafeBrowsingUIManager (Closed)
DescriptionExpose malware/phishing/etc. distinction from SafeBrowsingUIManager
SafeBrowsingUIManager reports whether a URL is whitelisted (or pending
to be whitelisted), but does not report the type of threat for which the
URL is whitelisted. This CL returns a SBThreatType from the whitelist
lookup methods, which is used to set a
SecurityInfo::malicious_content_status field, replacing the old boolean
fails_malware_check field that didn't capture the type of threat.
A follow-up CL will use the new |malicious_content_status| field to set
WebsiteSettings (page info bubble) strings appropriately.
BUG=646629
Committed: https://crrev.com/7ffa8c6bfc4f532cef64baa3944568c3f5939c89
Cr-Commit-Position: refs/heads/master@{#431684}
Patch Set 1 #Patch Set 2 : fix tests #
Total comments: 15
Patch Set 3 : nparker comments #Patch Set 4 : rebase #Messages
Total messages: 38 (25 generated)
The CQ bit was checked by estark@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_compile_dbg_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 estark@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.
estark@chromium.org changed reviewers: + lgarron@chromium.org, nparker@chromium.org
nparker, can you please review chrome/browser/safe_browsing? lgarron, can you please review chrome/browser/ssl and components/security_state? Thanks!
https://codereview.chromium.org/2481743009/diff/20001/components/security_sta... File components/security_state/security_state_model.h (right): https://codereview.chromium.org/2481743009/diff/20001/components/security_sta... components/security_state/security_state_model.h:101: MALICIOUS_CONTENT_STATUS_MALWARE, These different statuses will be used to distinguish which page info strings to use (strings are in the bug, 646629)
lgtm https://codereview.chromium.org/2481743009/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ui_manager.cc (right): https://codereview.chromium.org/2481743009/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ui_manager.cc:69: if (Contains(url, &existing_threat_type)) I'm not sure this should ever happen. Can you think of a case? It seems like the safe route though, so the threat_type doesn't change on a page. https://codereview.chromium.org/2481743009/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ui_manager.cc:500: if (site_list->ContainsPending(whitelist_url, &threat_type)) nit: How about passing a nullptr, and having the function check for that? That'll avoid an unused var here. Otherwise, rename it to |unused_threat_type|. Same nullptr below. https://codereview.chromium.org/2481743009/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2481743009/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:174: case safe_browsing::SB_THREAT_TYPE_EXTENSION: nit: I think _EXTENSION and _BINARY_MALWARE_URL won't ever be associated with an interstitial. So therefore those could move down to the NOTREACHED section. https://codereview.chromium.org/2481743009/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:183: case safe_browsing::SB_THREAT_TYPE_BLACKLISTED_RESOURCE: Maybe add a comment that these are not associated with interstitials. I'm sure we'll add some new types in the future, and we'll need to add them here as well. https://codereview.chromium.org/2481743009/diff/20001/components/security_sta... File components/security_state/security_state_model.cc (right): https://codereview.chromium.org/2481743009/diff/20001/components/security_sta... components/security_state/security_state_model.cc:217: SecurityStateModel::MALICIOUS_CONTENT_STATUS_NONE) { I don't understand what this logic does, but I trust you do. It appears to mirror the old code. https://codereview.chromium.org/2481743009/diff/20001/components/security_sta... components/security_state/security_state_model.cc:323: !!certificate == !!other.certificate && ooo I like all the the !!'s. That mirrors my mood today. https://codereview.chromium.org/2481743009/diff/20001/components/security_sta... File components/security_state/security_state_model.h (right): https://codereview.chromium.org/2481743009/diff/20001/components/security_sta... components/security_state/security_state_model.h:101: MALICIOUS_CONTENT_STATUS_MALWARE, On 2016/11/09 16:07:44, estark (slow thru Nov 18) wrote: > These different statuses will be used to distinguish which page info strings to > use (strings are in the bug, 646629) Acknowledged. https://codereview.chromium.org/2481743009/diff/20001/components/security_sta... File components/security_state/security_state_model_unittest.cc (right): https://codereview.chromium.org/2481743009/diff/20001/components/security_sta... components/security_state/security_state_model_unittest.cc:227: EXPECT_EQ(SecurityStateModel::MALICIOUS_CONTENT_STATUS_MALWARE, nit: Do you want a check to verify it starts in the _NONE state?
estark@chromium.org changed reviewers: + msw@chromium.org
+msw for chrome/browser/ui Friendly ping to lgarron (Thanks for the review nparker; haven't addressed your comments yet but will before landing)
c/b/ui rubber stamp lgtm
lgarron@chromium.org changed reviewers: + nparker@chromium.orgs - nparker@chromium.org
security_state changes LGTM; those should be straightforward to read from website_settings/Page Info.
https://codereview.chromium.org/2481743009/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/ui_manager.cc (right): https://codereview.chromium.org/2481743009/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ui_manager.cc:69: if (Contains(url, &existing_threat_type)) On 2016/11/10 00:13:03, Nathan Parker wrote: > I'm not sure this should ever happen. Can you think of a case? It seems like > the safe route though, so the threat_type doesn't change on a page. Hmm. I can't think of a case where this does happen, but I think this code probably shouldn't assume that it doesn't happen. https://codereview.chromium.org/2481743009/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/ui_manager.cc:500: if (site_list->ContainsPending(whitelist_url, &threat_type)) On 2016/11/10 00:13:03, Nathan Parker wrote: > nit: How about passing a nullptr, and having the function check for that? > That'll avoid an unused var here. Otherwise, rename it to |unused_threat_type|. > > Same nullptr below. Done. https://codereview.chromium.org/2481743009/diff/20001/chrome/browser/ssl/chro... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/2481743009/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:174: case safe_browsing::SB_THREAT_TYPE_EXTENSION: On 2016/11/10 00:13:03, Nathan Parker wrote: > nit: I think _EXTENSION and _BINARY_MALWARE_URL won't ever be associated with an > interstitial. So therefore those could move down to the NOTREACHED section. Done. https://codereview.chromium.org/2481743009/diff/20001/chrome/browser/ssl/chro... chrome/browser/ssl/chrome_security_state_model_client.cc:183: case safe_browsing::SB_THREAT_TYPE_BLACKLISTED_RESOURCE: On 2016/11/10 00:13:03, Nathan Parker wrote: > Maybe add a comment that these are not associated with interstitials. I'm sure > we'll add some new types in the future, and we'll need to add them here as well. Done. https://codereview.chromium.org/2481743009/diff/20001/components/security_sta... File components/security_state/security_state_model.cc (right): https://codereview.chromium.org/2481743009/diff/20001/components/security_sta... components/security_state/security_state_model.cc:217: SecurityStateModel::MALICIOUS_CONTENT_STATUS_NONE) { On 2016/11/10 00:13:03, Nathan Parker wrote: > I don't understand what this logic does, but I trust you do. It appears to > mirror the old code. Yeah the whole |connection_info_initialized| thing is kinda weird and probably unnecessary (a byproduct of https://crbug.com/655220, I think). IIRC this is basically handling the case where if the navigation hasn't committed yet, we won't have connection security info yet, but we still want to reflect the Dangerous status in the omnibox if the URL is marked bad because of SB. https://codereview.chromium.org/2481743009/diff/20001/components/security_sta... File components/security_state/security_state_model_unittest.cc (right): https://codereview.chromium.org/2481743009/diff/20001/components/security_sta... components/security_state/security_state_model_unittest.cc:227: EXPECT_EQ(SecurityStateModel::MALICIOUS_CONTENT_STATUS_MALWARE, On 2016/11/10 00:13:04, Nathan Parker wrote: > nit: Do you want a check to verify it starts in the _NONE state? Done.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org, msw@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2481743009/#ps40001 (title: "nparker comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Expose malware/phishing/etc. distinction from SafeBrowsingUIManager SafeBrowsingUIManager reports whether a URL is whitelisted (or pending to be whitelisted), but does not report the type of threat for which the URL is whitelisted. This CL returns a SBThreatType from the whitelist lookup methods, which is used to set a SecurityInfo::malicious_content_status field, replacing the old boolean fails_malware_check field that didn't capture the type of threat. A follow-up CL will use the new |malicious_content_status| field to set WebsiteSettings (page info bubble) strings appropriately. BUG=646629 ========== to ========== Expose malware/phishing/etc. distinction from SafeBrowsingUIManager SafeBrowsingUIManager reports whether a URL is whitelisted (or pending to be whitelisted), but does not report the type of threat for which the URL is whitelisted. This CL returns a SBThreatType from the whitelist lookup methods, which is used to set a SecurityInfo::malicious_content_status field, replacing the old boolean fails_malware_check field that didn't capture the type of threat. A follow-up CL will use the new |malicious_content_status| field to set WebsiteSettings (page info bubble) strings appropriately. BUG=646629 ==========
estark@chromium.org changed reviewers: + nparker@chromium.org - nparker@chromium.orgs
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for components/security_state/security_state_model.cc:
While running git apply --index -p1;
error: patch failed: components/security_state/security_state_model.cc:239
error: components/security_state/security_state_model.cc: patch does not apply
Patch: components/security_state/security_state_model.cc
Index: components/security_state/security_state_model.cc
diff --git a/components/security_state/security_state_model.cc
b/components/security_state/security_state_model.cc
index
7ed1a53347d4e314bed110ed5b8ee1f46bd8e92a..4b8330e1449da26400b8f5ed0bec5d2fedf55772
100644
--- a/components/security_state/security_state_model.cc
+++ b/components/security_state/security_state_model.cc
@@ -125,12 +125,15 @@ SecurityStateModel::SecurityLevel
GetSecurityLevelForRequest(
SecurityStateModel::ContentStatus mixed_content_status,
SecurityStateModel::ContentStatus content_with_cert_errors_status) {
DCHECK(visible_security_state.connection_info_initialized ||
- visible_security_state.fails_malware_check);
+ visible_security_state.malicious_content_status !=
+ SecurityStateModel::MALICIOUS_CONTENT_STATUS_NONE);
// Override the connection security information if the website failed the
// browser's malware checks.
- if (visible_security_state.fails_malware_check)
+ if (visible_security_state.malicious_content_status !=
+ SecurityStateModel::MALICIOUS_CONTENT_STATUS_NONE) {
return SecurityStateModel::DANGEROUS;
+ }
GURL url = visible_security_state.url;
@@ -208,9 +211,10 @@ void SecurityInfoForRequest(
SecurityStateModel::SecurityInfo* security_info) {
if (!visible_security_state.connection_info_initialized) {
*security_info = SecurityStateModel::SecurityInfo();
- security_info->fails_malware_check =
- visible_security_state.fails_malware_check;
- if (security_info->fails_malware_check) {
+ security_info->malicious_content_status =
+ visible_security_state.malicious_content_status;
+ if (security_info->malicious_content_status !=
+ SecurityStateModel::MALICIOUS_CONTENT_STATUS_NONE) {
security_info->security_level = GetSecurityLevelForRequest(
visible_security_state, client, SecurityStateModel::UNKNOWN_SHA1,
SecurityStateModel::CONTENT_STATUS_UNKNOWN,
@@ -239,8 +243,8 @@ void SecurityInfoForRequest(
security_info->sct_verify_statuses =
visible_security_state.sct_verify_statuses;
- security_info->fails_malware_check =
- visible_security_state.fails_malware_check;
+ security_info->malicious_content_status =
+ visible_security_state.malicious_content_status;
security_info->displayed_private_user_data_input_on_http =
visible_security_state.displayed_password_field_on_http ||
@@ -263,7 +267,8 @@ const SecurityStateModel::SecurityLevel
SecurityStateModel::SecurityInfo::SecurityInfo()
: security_level(SecurityStateModel::NONE),
- fails_malware_check(false),
+ malicious_content_status(
+ SecurityStateModel::MALICIOUS_CONTENT_STATUS_NONE),
sha1_deprecation_status(SecurityStateModel::NO_DEPRECATED_SHA1),
mixed_content_status(SecurityStateModel::CONTENT_STATUS_NONE),
content_with_cert_errors_status(SecurityStateModel::CONTENT_STATUS_NONE),
@@ -294,7 +299,8 @@ void SecurityStateModel::SetClient(SecurityStateModelClient*
client) {
}
SecurityStateModel::VisibleSecurityState::VisibleSecurityState()
- : fails_malware_check(false),
+ : malicious_content_status(
+ SecurityStateModel::MALICIOUS_CONTENT_STATUS_NONE),
connection_info_initialized(false),
cert_status(0),
connection_status(0),
@@ -313,7 +319,7 @@
SecurityStateModel::VisibleSecurityState::~VisibleSecurityState() {}
bool SecurityStateModel::VisibleSecurityState::operator==(
const SecurityStateModel::VisibleSecurityState& other) const {
return (url == other.url &&
- fails_malware_check == other.fails_malware_check &&
+ malicious_content_status == other.malicious_content_status &&
!!certificate == !!other.certificate &&
(certificate ? certificate->Equals(other.certificate.get()) : true)
&&
connection_status == other.connection_status &&
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org, msw@chromium.org, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2481743009/#ps60001 (title: "rebase")
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 estark@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.
The CQ bit was checked by estark@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 ========== Expose malware/phishing/etc. distinction from SafeBrowsingUIManager SafeBrowsingUIManager reports whether a URL is whitelisted (or pending to be whitelisted), but does not report the type of threat for which the URL is whitelisted. This CL returns a SBThreatType from the whitelist lookup methods, which is used to set a SecurityInfo::malicious_content_status field, replacing the old boolean fails_malware_check field that didn't capture the type of threat. A follow-up CL will use the new |malicious_content_status| field to set WebsiteSettings (page info bubble) strings appropriately. BUG=646629 ========== to ========== Expose malware/phishing/etc. distinction from SafeBrowsingUIManager SafeBrowsingUIManager reports whether a URL is whitelisted (or pending to be whitelisted), but does not report the type of threat for which the URL is whitelisted. This CL returns a SBThreatType from the whitelist lookup methods, which is used to set a SecurityInfo::malicious_content_status field, replacing the old boolean fails_malware_check field that didn't capture the type of threat. A follow-up CL will use the new |malicious_content_status| field to set WebsiteSettings (page info bubble) strings appropriately. BUG=646629 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Expose malware/phishing/etc. distinction from SafeBrowsingUIManager SafeBrowsingUIManager reports whether a URL is whitelisted (or pending to be whitelisted), but does not report the type of threat for which the URL is whitelisted. This CL returns a SBThreatType from the whitelist lookup methods, which is used to set a SecurityInfo::malicious_content_status field, replacing the old boolean fails_malware_check field that didn't capture the type of threat. A follow-up CL will use the new |malicious_content_status| field to set WebsiteSettings (page info bubble) strings appropriately. BUG=646629 ========== to ========== Expose malware/phishing/etc. distinction from SafeBrowsingUIManager SafeBrowsingUIManager reports whether a URL is whitelisted (or pending to be whitelisted), but does not report the type of threat for which the URL is whitelisted. This CL returns a SBThreatType from the whitelist lookup methods, which is used to set a SecurityInfo::malicious_content_status field, replacing the old boolean fails_malware_check field that didn't capture the type of threat. A follow-up CL will use the new |malicious_content_status| field to set WebsiteSettings (page info bubble) strings appropriately. BUG=646629 Committed: https://crrev.com/7ffa8c6bfc4f532cef64baa3944568c3f5939c89 Cr-Commit-Position: refs/heads/master@{#431684} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7ffa8c6bfc4f532cef64baa3944568c3f5939c89 Cr-Commit-Position: refs/heads/master@{#431684} |
