Add domain request detection to incident reporting service.

Add domain request detection to incident reporting service.

BUG=none
R=grt@chromium.org

Committed: https://crrev.com/045b8f6f2a83aa436d4c2d9cf024510599da3eec
Cr-Commit-Position: refs/heads/master@{#324144}

[romanl, 2015-03-20 17:15:58]

Hi Greg,

Could you please review my domain request detection changes? (mostly copy-pasted
from script request detection)

ScriptRequestDetector is now responsible for both Script and Domain requests
detection, maybe I should rename that class or refactor otherwise?

Also, ScriptRequestIncident and DomainRequestIncident in csd.proto have very
similar data but fields named differently, I am not sure if there is a better
way.

Thanks,
Roman.

[grt (UTC plus 2), 2015-03-20 20:50:04]

A good start. I think we may need to tune when the new incident is created a
bit. Generalizing the incident is good, too.

https://codereview.chromium.org/1024943002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc
(right):

https://codereview.chromium.org/1024943002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:264:
crypto::SHA256HashString(domain.c_str(), &domain_digest[0],
use request->url().host() directly here

https://codereview.chromium.org/1024943002/diff/20001/chrome/common/safe_brow...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/1024943002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/csd.proto:374: message DomainRequestIncident {
This new type is so similar to the existing one, that I think it makes sense to
generalize the existing one a bit. With the wonder of protobufs, we can change
ScriptRequestIncident to:

  message ResourceIncident {
    enum Type {
      SCRIPT = 0;
      DOMAIN = 1;
    }
    optional string digest = 1;
    optional string origin = 2;
    optional Type type = 3;
  }

and update the various class and method names accordingly.

[grt (UTC plus 2), 2015-03-21 01:30:08]

https://codereview.chromium.org/1024943002/diff/20001/chrome/common/safe_brow...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/1024943002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/csd.proto:374: message DomainRequestIncident {
On 2015/03/20 20:50:04, grt wrote:
>     enum Type {
>       SCRIPT = 0;
>       DOMAIN = 1;
>     }

I just remembered that enums in protobufs must start with an "unknown" value for
the sake of extensibility, so make that:

enum Type {
  UNKNOWN = 0;
  SCRIPT = 1;
  DOMAIN = 2;
}

[grt (UTC plus 2), 2015-03-21 01:44:36]

On 2015/03/21 01:30:08, grt wrote:
>
https://codereview.chromium.org/1024943002/diff/20001/chrome/common/safe_brow...
> File chrome/common/safe_browsing/csd.proto (right):
> 
>
https://codereview.chromium.org/1024943002/diff/20001/chrome/common/safe_brow...
> chrome/common/safe_browsing/csd.proto:374: message DomainRequestIncident {
> On 2015/03/20 20:50:04, grt wrote:
> >     enum Type {
> >       SCRIPT = 0;
> >       DOMAIN = 1;
> >     }
> 
> I just remembered that enums in protobufs must start with an "unknown" value
for
> the sake of extensibility, so make that:
> 
> enum Type {
>   UNKNOWN = 0;
>   SCRIPT = 1;
>   DOMAIN = 2;
> }

Hmm. More stylish to use TYPE_SCRIPT and TYPE_DOMAIN for the latter two,
methinks.

[romanl, 2015-03-23 21:31:29]

Hi Greg,

As you suggested, I have merged domain and script incidents into a single entity
("Resource") with a Type. I renamed a few classes -> files. It showed up
correctly in git, but then `git cl upload` seems to have lost the track on file
renames. I will probably need your help to fix this...

Thanks,
Roman.

[romanl, 2015-03-26 17:29:00]

Patchset #3 (id:40001) has been deleted

[romanl, 2015-03-26 17:51:55]

Patchset #3 (id:60001) has been deleted

[romanl, 2015-03-26 22:01:47]

Hi Greg,

I made the change that we discussed today to ignore top-level requests
(RESOURCE_TYPE_MAIN_FRAME) and added a corresponding unit test. Could you review
the updated version please?

Thanks,
Roman.

https://codereview.chromium.org/1024943002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc
(right):

https://codereview.chromium.org/1024943002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/script_request_detector.cc:264:
crypto::SHA256HashString(domain.c_str(), &domain_digest[0],
On 2015/03/20 20:50:04, grt wrote:
> use request->url().host() directly here

Done.

https://codereview.chromium.org/1024943002/diff/20001/chrome/common/safe_brow...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/1024943002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/csd.proto:374: message DomainRequestIncident {
On 2015/03/20 20:50:04, grt wrote:
> This new type is so similar to the existing one, that I think it makes sense
to
> generalize the existing one a bit. With the wonder of protobufs, we can change
> ScriptRequestIncident to:
> 
>   message ResourceIncident {
>     enum Type {
>       SCRIPT = 0;
>       DOMAIN = 1;
>     }
>     optional string digest = 1;
>     optional string origin = 2;
>     optional Type type = 3;
>   }
> 
> and update the various class and method names accordingly.

Done.

https://codereview.chromium.org/1024943002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/csd.proto:374: message DomainRequestIncident {
On 2015/03/21 01:30:07, grt wrote:
> On 2015/03/20 20:50:04, grt wrote:
> >     enum Type {
> >       SCRIPT = 0;
> >       DOMAIN = 1;
> >     }
> 
> I just remembered that enums in protobufs must start with an "unknown" value
for
> the sake of extensibility, so make that:
> 
> enum Type {
>   UNKNOWN = 0;
>   SCRIPT = 1;
>   DOMAIN = 2;
> }

Done.

[grt (UTC plus 2), 2015-03-27 14:17:44]

this looks great. minor tweaks below.

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/resource_request_detector.cc
(right):

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/resource_request_detector.cc:265:
// Only detect non top-level requests
nit: add a period at the end of the sentence.

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/resource_request_detector.cc:270:
crypto::SHA256HashString(request->url().host().c_str(), &domain_digest[0],
StringPiece is more efficient if you give it a std::string rather than a char*,
so use request->url().host() here without the .c_str()

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/resource_request_detector.cc:310:
crypto::SHA256HashString(url.c_str(), &script_digest[0],
oops, same thing here: url should work without the .c_str()

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/resource_request_incident.cc
(right):

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/resource_request_incident.cc:29:
// Use a static key in addition to a fixed digest below to ensure that only
please update the comment:
  // Use a static key per resource request type in addition to a fixed digest
  // below to ensure that only one incident per user (and incident type) is
  // reported.

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/resource_request_incident.cc:39:
// Return a constant in addition to a fixed key above to ensure that only one
  // Return a constant in addition to a fixed key per resource request type
  // above to ensure that only one incident per user is reported.

https://codereview.chromium.org/1024943002/diff/80001/chrome/common/safe_brow...
File chrome/common/safe_browsing/csd.proto (left):

https://codereview.chromium.org/1024943002/diff/80001/chrome/common/safe_brow...
chrome/common/safe_browsing/csd.proto:371: optional string script_digest = 1;
I changed this from "string" to "bytes" yesterday. Please sync to ToT
(tip-of-tree) to pick this up.

[romanl, 2015-03-30 14:46:50]

Hi Greg,

Fixed!

Thanks,
Roman.

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/resource_request_detector.cc
(right):

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/resource_request_detector.cc:265:
// Only detect non top-level requests
On 2015/03/27 14:17:43, grt wrote:
> nit: add a period at the end of the sentence.

Done.

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/resource_request_detector.cc:270:
crypto::SHA256HashString(request->url().host().c_str(), &domain_digest[0],
On 2015/03/27 14:17:43, grt wrote:
> StringPiece is more efficient if you give it a std::string rather than a
char*,
> so use request->url().host() here without the .c_str()

Done.

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/resource_request_detector.cc:310:
crypto::SHA256HashString(url.c_str(), &script_digest[0],
On 2015/03/27 14:17:43, grt wrote:
> oops, same thing here: url should work without the .c_str()

Done.

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
File
chrome/browser/safe_browsing/incident_reporting/resource_request_incident.cc
(right):

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/resource_request_incident.cc:29:
// Use a static key in addition to a fixed digest below to ensure that only
On 2015/03/27 14:17:43, grt wrote:
> please update the comment:
>   // Use a static key per resource request type in addition to a fixed digest
>   // below to ensure that only one incident per user (and incident type) is
>   // reported.

Done.

https://codereview.chromium.org/1024943002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/incident_reporting/resource_request_incident.cc:39:
// Return a constant in addition to a fixed key above to ensure that only one
On 2015/03/27 14:17:43, grt wrote:
>   // Return a constant in addition to a fixed key per resource request type
>   // above to ensure that only one incident per user is reported.

Done.

https://codereview.chromium.org/1024943002/diff/80001/chrome/common/safe_brow...
File chrome/common/safe_browsing/csd.proto (left):

https://codereview.chromium.org/1024943002/diff/80001/chrome/common/safe_brow...
chrome/common/safe_browsing/csd.proto:371: optional string script_digest = 1;
On 2015/03/27 14:17:44, grt wrote:
> I changed this from "string" to "bytes" yesterday. Please sync to ToT
> (tip-of-tree) to pick this up.

Done.

[grt (UTC plus 2), 2015-03-30 15:50:44]

LGTM, thanks.

https://codereview.chromium.org/1024943002/diff/100001/chrome/common/safe_bro...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/1024943002/diff/100001/chrome/common/safe_bro...
chrome/common/safe_browsing/csd.proto:385: 
nit: remove blank line for consistency with all of the above

[romanl, 2015-03-30 17:14:42]

Removed the blank line. Thanks!

[grt (UTC plus 2), 2015-04-01 20:17:07]

still lgtm with a final change. please update and hit the CQ box.

https://codereview.chromium.org/1024943002/diff/120001/chrome/common/safe_bro...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/1024943002/diff/120001/chrome/common/safe_bro...
chrome/common/safe_browsing/csd.proto:393: optional Type type = 3;
please change this line to:
      optional Type type = 3 [default = UNKNOWN];

[grt (UTC plus 2), 2015-04-01 20:17:35]

On 2015/04/01 20:17:07, grt wrote:
> still lgtm with a final change. please update and hit the CQ box.

oops, still need some OWNERS reviews. hang tight.

[grt (UTC plus 2), 2015-04-01 20:19:03]

grt@chromium.org changed reviewers:
+ asvitkine@chromium.org, mattm@chromium.org

[grt (UTC plus 2), 2015-04-01 20:20:24]

adding OWNERS for their approval:
asvitkine: tools/metrics/histograms/histograms.xml
mattm: chrome/browser/safe_browsing/safe_browsing_service.{cc,h} and
chrome/common/safe_browsing/csd.proto

thanks.

[Alexei Svitkine (slow), 2015-04-01 21:02:59]

https://codereview.chromium.org/1024943002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1024943002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:51991: +  <int value="6"
label="ResourceRequest"/>
Don't re-use the same enum value for a different purpose - else if you aggregate
data between versions on the dashboards, the results will be nonsensical.

[grt (UTC plus 2), 2015-04-01 22:04:02]

https://codereview.chromium.org/1024943002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1024943002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:51991: +  <int value="6"
label="ResourceRequest"/>
On 2015/04/01 21:02:59, Alexei Svitkine wrote:
> Don't re-use the same enum value for a different purpose - else if you
aggregate
> data between versions on the dashboards, the results will be nonsensical.

This is a rename rather than a reuse. Does that pose any problems?

[mattm, 2015-04-01 23:11:10]

lgtm

[Alexei Svitkine (slow), 2015-04-02 15:30:31]

lgtm

https://codereview.chromium.org/1024943002/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1024943002/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:51991: +  <int value="6"
label="ResourceRequest"/>
On 2015/04/01 22:04:02, grt wrote:
> On 2015/04/01 21:02:59, Alexei Svitkine wrote:
> > Don't re-use the same enum value for a different purpose - else if you
> aggregate
> > data between versions on the dashboards, the results will be nonsensical.
> 
> This is a rename rather than a reuse. Does that pose any problems?

Ah ok - in that case it's ok - assuming the meaning is the same between
different versions.

[Alexei Svitkine (slow), 2015-04-02 15:31:19]

However, this CL appears large enough that it should have an associated crbug
with it. Could you file one and set BUG= on the CL?

[grt (UTC plus 2), 2015-04-02 15:31:22]

On 2015/04/02 15:30:31, Alexei Svitkine wrote:
> lgtm
> 
>
https://codereview.chromium.org/1024943002/diff/120001/tools/metrics/histogra...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1024943002/diff/120001/tools/metrics/histogra...
> tools/metrics/histograms/histograms.xml:51991: +  <int value="6"
> label="ResourceRequest"/>
> On 2015/04/01 22:04:02, grt wrote:
> > On 2015/04/01 21:02:59, Alexei Svitkine wrote:
> > > Don't re-use the same enum value for a different purpose - else if you
> > aggregate
> > > data between versions on the dashboards, the results will be nonsensical.
> > 
> > This is a rename rather than a reuse. Does that pose any problems?
> 
> Ah ok - in that case it's ok - assuming the meaning is the same between
> different versions.

For our purposes, yes. Thanks for scrutinizing.

[grt (UTC plus 2), 2015-04-02 15:50:36]

On 2015/04/01 20:17:07, grt wrote:
> still lgtm with a final change. please update and hit the CQ box.
> 
>
https://codereview.chromium.org/1024943002/diff/120001/chrome/common/safe_bro...
> File chrome/common/safe_browsing/csd.proto (right):
> 
>
https://codereview.chromium.org/1024943002/diff/120001/chrome/common/safe_bro...
> chrome/common/safe_browsing/csd.proto:393: optional Type type = 3;
> please change this line to:
>       optional Type type = 3 [default = UNKNOWN];

You're clear to check the CQ box after uploading this change. Cheers.

[romanl, 2015-04-07 21:31:25]

The CQ bit was checked by romanl@google.com

[romanl, 2015-04-07 21:31:26]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org,
asvitkine@chromium.org, mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/1024943002/#ps140001
(title: "Set default type to unknown")

[commit-bot: I haz the power, 2015-04-07 21:31:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1024943002/140001

[commit-bot: I haz the power, 2015-04-07 22:55:50]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-04-07 22:57:00]

Patchset 6 (id:??) landed as
https://crrev.com/045b8f6f2a83aa436d4c2d9cf024510599da3eec
Cr-Commit-Position: refs/heads/master@{#324144}