Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1556613002:
Adds SB V4 response handler to Protocol Manager. (Closed)
|
Created:
4 years, 11 months ago by kcarattini Modified:
4 years, 11 months ago CC:
chromium-reviews, grt+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@osb-pm-1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds SB V4 response handler to Protocol Manager.
BUG=561867
Committed: https://crrev.com/71d77cc8bfef35760a57c6644105ffe444c162fd
Cr-Commit-Position: refs/heads/master@{#369067}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Review Comments #
Total comments: 3
Patch Set 3 : Rebase #Patch Set 4 : Rebase #
Total comments: 18
Patch Set 5 : Review Comments #Patch Set 6 : Fix test #
Total comments: 2
Patch Set 7 : Rebase #
Dependent Patchsets: Messages
Total messages: 17 (4 generated)
kcarattini@chromium.org changed reviewers: + awoz@google.com, nparker@chromium.org, vakh@google.com
https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/protocol_manager.cc (right): https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/protocol_manager.cc:286: match.platform_type() == CHROME_PLATFORM && If you're making this generic, you won't want to filter CHROME_PLATFORM. That enum is used specifically for the API abuse blacklist items. Chrome will likely end up querying for hashes on lists with different platforms, ideally ALL_PLATFORMS lists (on desktop, at least). Perhaps allow the caller to also define the platform types they're interested in when calling GetV4FullHashes? https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/protocol_manager.cc:308: result.metadata += m.value() + ","; Do you care about having a trailing comma? https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/protocol_manager.cc:430: std::string data_base64; The protocol returns a serialized protobuf in binary format, i.e. not base64 encoded. https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/protocol_manager.h (right): https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/protocol_manager.h:260: // |full_hashes| and |cache_lifetime|. |data_base64| is the base 64 encoded Can you explain what |cache_lifetime| is? It's unclear how |cache_lifetime| and SBFullHashResult's |cache_duration| differ. https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/protocol_manager.h:261: // string of a serialized FindFullHashes procol buffer. Returns true if s/procol/protocol https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/protocol_manager.h:262: // parsing is successfull, false otherwise. s/successfull/successful https://codereview.chromium.org/1556613002/diff/1/components/safe_browsing_db... File components/safe_browsing_db/util.h (right): https://codereview.chromium.org/1556613002/diff/1/components/safe_browsing_db... components/safe_browsing_db/util.h:67: // Used only for V4 results. Can you briefly explain how this duration is used?
Thanks for the review, PTAL! Kendra https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/protocol_manager.cc (right): https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/protocol_manager.cc:286: match.platform_type() == CHROME_PLATFORM && On 2015/12/30 18:23:13, awoz wrote: > If you're making this generic, you won't want to filter CHROME_PLATFORM. That > enum is used specifically for the API abuse blacklist items. Chrome will likely > end up querying for hashes on lists with different platforms, ideally > ALL_PLATFORMS lists (on desktop, at least). > > Perhaps allow the caller to also define the platform types they're interested in > when calling GetV4FullHashes? Done. https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/protocol_manager.cc:308: result.metadata += m.value() + ","; On 2015/12/30 18:23:13, awoz wrote: > Do you care about having a trailing comma? No, I imagine I'll parse this to a set and check for inclusion later. https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/protocol_manager.cc:430: std::string data_base64; On 2015/12/30 18:23:13, awoz wrote: > The protocol returns a serialized protobuf in binary format, i.e. not base64 > encoded. Done. https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/protocol_manager.h (right): https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/protocol_manager.h:260: // |full_hashes| and |cache_lifetime|. |data_base64| is the base 64 encoded On 2015/12/30 18:23:13, awoz wrote: > Can you explain what |cache_lifetime| is? It's unclear how |cache_lifetime| and > SBFullHashResult's |cache_duration| differ. Done. https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/protocol_manager.h:261: // string of a serialized FindFullHashes procol buffer. Returns true if On 2015/12/30 18:23:13, awoz wrote: > s/procol/protocol Done. https://codereview.chromium.org/1556613002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/protocol_manager.h:262: // parsing is successfull, false otherwise. On 2015/12/30 18:23:13, awoz wrote: > s/successfull/successful Done. https://codereview.chromium.org/1556613002/diff/1/components/safe_browsing_db... File components/safe_browsing_db/util.h (right): https://codereview.chromium.org/1556613002/diff/1/components/safe_browsing_db... components/safe_browsing_db/util.h:67: // Used only for V4 results. On 2015/12/30 18:23:13, awoz wrote: > Can you briefly explain how this duration is used? Done.
https://codereview.chromium.org/1556613002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/protocol_manager.cc (right): https://codereview.chromium.org/1556613002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:292: result.hash = StringToSBFullHash(match.threat().hash()); Should the threat type also be passed back? https://codereview.chromium.org/1556613002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/protocol_manager.h (left): https://codereview.chromium.org/1556613002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.h:91: ThreatType threat_type, How about a vector of threat types as well? That way we can bundle up multiple lists into a single request.
Nathan, Varun, do you think it is necessary to include multiple threat types per request? As far as I can tell this isn't done at the moment. With multiple threat types, we would also need to add the threat type to the SBFullHashResult and I don't see an easy 1:1 correspondence between the ThreatType in the proto and the SBThreatType that protocol manager clients know about. Kendra
lgtm Mostly nits, except the erase(v4_it). LGTM once that's all done. As for multiple threat types: If (in the future) we were checking a URL against phishing/malware/uws lists, and it were on multiple lists or it happened to have a hash collision on multiple lists, we'd need to get-full-hashes for multiple threat types. But we could make multiple calls for that, since it would be rare. We'd likely still collapse multiple threat responses down to the most sever, like is done in GetUrlSeverestThreatType(). awoz -- Do we support get-full-hash calls for _multiple_ threat types in both pver3 and v4? https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/protocol_manager.cc (right): https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:274: if (!response.ParseFromString(data)) TODO: UMA https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:314: } else, increment an UMA error counter. We should aim to be able to see in UMA any time we get an unexpected result. For that matter, any time we don't end up with a valid/useful response, we should return false. https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:347: std::vector<PlatformType> platform; platform = {CHROME_PLATFORM} https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:428: fetcher.reset(v4_it->first); Do we need to hang on to the fetcher ptr? This scoped_ptr doesn't extend the lifetime of the ptr, so there doesn't seem to be a value. Could remove it above as well. https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:447: source->GetURL() << " failed with error: " << status.error(); nit: Could put these into one DVLOG by logging both status.error and response_code. https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:459: v4_hash_requests_.erase(it); erase(v4_it) https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:555: IssueChunkRequest(); It looks like this should be moved up one curly bracket -- we don't want this to run for v3 or v4 hash responses, just for chunk responses. Leave it here for now, and I'll move it in a separate CL. https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right): https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager_unittest.cc:452: } We should add some tests that validate the network-error-handling. In another CL is fine. https://codereview.chromium.org/1556613002/diff/60001/components/safe_browsin... File components/safe_browsing_db/util.h (right): https://codereview.chromium.org/1556613002/diff/60001/components/safe_browsin... components/safe_browsing_db/util.h:70: base::TimeDelta cache_duration; How does this relate to the SBCachedFullHashResult::expire_after? Is expire_after not used for v4 responses?
Thanks for your review! I will leave the multiple threats per request stuff out of this cl until we decide whether we want to support it. Alex, Varun, it would be great if you could get to this cl soon because the others are dependent on it. Thanks! Kendra https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/protocol_manager.cc (right): https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:274: if (!response.ParseFromString(data)) On 2016/01/11 23:50:32, nparker wrote: > TODO: UMA Done. https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:314: } On 2016/01/11 23:50:32, nparker wrote: > else, increment an UMA error counter. We should aim to be able to see in UMA > any time we get an unexpected result. > > For that matter, any time we don't end up with a valid/useful response, we > should return false. Done, although this is definitely assuming that we will not have multiple threat types per request, and any new requests for a different threat type will need to add an 'else if' clause here, even if they don't use the metadata. https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:347: std::vector<PlatformType> platform; On 2016/01/11 23:50:32, nparker wrote: > > platform = {CHROME_PLATFORM} Done. https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:428: fetcher.reset(v4_it->first); On 2016/01/11 23:50:32, nparker wrote: > Do we need to hang on to the fetcher ptr? This scoped_ptr doesn't extend the > lifetime of the ptr, so there doesn't seem to be a value. Could remove it above > as well. Removed here and above. https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:447: source->GetURL() << " failed with error: " << status.error(); On 2016/01/11 23:50:32, nparker wrote: > nit: Could put these into one DVLOG by logging both status.error and > response_code. Done. https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:459: v4_hash_requests_.erase(it); On 2016/01/11 23:50:32, nparker wrote: > erase(v4_it) Opps! Thanks for catching that. Done. https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:555: IssueChunkRequest(); On 2016/01/11 23:50:32, nparker wrote: > It looks like this should be moved up one curly bracket -- we don't want this to > run for v3 or v4 hash responses, just for chunk responses. Leave it here for > now, and I'll move it in a separate CL. I was wondering about that. will leave for you or I can do in a follow-up cl. https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right): https://codereview.chromium.org/1556613002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager_unittest.cc:452: } On 2016/01/11 23:50:32, nparker wrote: > We should add some tests that validate the network-error-handling. In another > CL is fine. Acknowledged. https://codereview.chromium.org/1556613002/diff/60001/components/safe_browsin... File components/safe_browsing_db/util.h (right): https://codereview.chromium.org/1556613002/diff/60001/components/safe_browsin... components/safe_browsing_db/util.h:70: base::TimeDelta cache_duration; On 2016/01/11 23:50:32, nparker wrote: > How does this relate to the SBCachedFullHashResult::expire_after? Is > expire_after not used for v4 responses? SBCachedFullHashResult is used by the database, and it looks like it takes the cache_lifetime passed to the callback and sets that value for all hashes in the cache that were part of the same request. In V4, there can be a different cache_duration per hash (as opposed to per request), so we need a way to capture that in the results that are passed to the callback. If we end up using the SBCachedFullHashResult in the database manager (and I suspect we will), then yes, this cache_duration will be used to the the expire_after field.
lgtm https://codereview.chromium.org/1556613002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/protocol_manager.cc (right): https://codereview.chromium.org/1556613002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/protocol_manager.cc:292: result.hash = StringToSBFullHash(match.threat().hash()); On 2016/01/05 17:20:10, awoz wrote: > Should the threat type also be passed back? This is irrelevant since only one threat type is being queried at a time.
lgtm https://codereview.chromium.org/1556613002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/protocol_manager.cc (right): https://codereview.chromium.org/1556613002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/protocol_manager.cc:352: std::vector<PlatformType> platform = {CHROME_PLATFORM}; platforms?
Thank you both! https://codereview.chromium.org/1556613002/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/protocol_manager.cc (right): https://codereview.chromium.org/1556613002/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/protocol_manager.cc:352: std::vector<PlatformType> platform = {CHROME_PLATFORM}; On 2016/01/12 23:09:49, vakh (old account. dont use) wrote: > platforms? I think I'm going to leave this singular to indicate that we only have one value in the vector.
The CQ bit was checked by kcarattini@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, awoz@google.com, vakh@google.com Link to the patchset: https://codereview.chromium.org/1556613002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1556613002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1556613002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Adds SB V4 response handler to Protocol Manager. BUG=561867 ========== to ========== Adds SB V4 response handler to Protocol Manager. BUG=561867 Committed: https://crrev.com/71d77cc8bfef35760a57c6644105ffe444c162fd Cr-Commit-Position: refs/heads/master@{#369067} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/71d77cc8bfef35760a57c6644105ffe444c162fd Cr-Commit-Position: refs/heads/master@{#369067} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
