Add PRESUBMIT rule to make sure IPC changes are security reviewed.

Add PRESUBMIT rule to make sure IPC changes are security reviewed.

The PRESUBMIT rule makes sure that files involving IPC are covered
by the appropriate per-file rules. See https://goo.gl/NRiIPv for
other alternatives that were considered (e.g. why not recursive
per-file rules?).

BUG=611905
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e07de81b11b1c5b62eaaa258e896878eb5364ff2
Cr-Commit-Position: refs/heads/master@{#400748}

[dcheng, 2016-06-15 10:32:16]

Description was changed from

==========
Add PRESUBMIT rule to make sure IPC changes are security reviewed.

BUG=611905
==========

to

==========
Add PRESUBMIT rule to make sure IPC changes are security reviewed.

BUG=611905
==========

[dcheng, 2016-06-15 10:32:16]

dcheng@chromium.org changed reviewers:
+ dpranke@chromium.org

[dcheng, 2016-06-15 10:41:55]

Description was changed from

==========
Add PRESUBMIT rule to make sure IPC changes are security reviewed.

BUG=611905
==========

to

==========
Add PRESUBMIT rule to make sure IPC changes are security reviewed.

The PRESUBMIT rule makes sure that files involving IPC are covered
by the appropriate per-file rules. See https://goo.gl/NRiIPv for
other alternatives that were considered (e.g. why not recursive
per-file rules?).

BUG=611905
==========

[dcheng, 2016-06-15 10:42:27]

Assuming this patch is acceptable, I'll update the CL to fix up other OWNERS
files as well.

https://codereview.chromium.org/2070483002/diff/20001/content/common/OWNERS
File content/common/OWNERS (right):

https://codereview.chromium.org/2070483002/diff/20001/content/common/OWNERS#n...
content/common/OWNERS:20: per-file *_messages*.h=set noparent
The loss in coverage from this seems relatively minor: I have a hard time seeing
how something meaningful will get missed with *_messages*.h but not
*_message*.h. The reason for doing this is the PRESUBMIT check is fairly dumb
and straightforward: it's just a straight string comparison.

I can make it more clever in a followup CL if this is really that important
though.

https://codereview.chromium.org/2070483002/diff/20001/ipc/SECURITY_OWNERS
File ipc/SECURITY_OWNERS (right):

https://codereview.chromium.org/2070483002/diff/20001/ipc/SECURITY_OWNERS#new...
ipc/SECURITY_OWNERS:2: # new sandbox escapes.
I've checked that git cl owners picks up this comment correctly when pulled in
via file:

[Robert Sesek, 2016-06-15 14:55:38]

rsesek@chromium.org changed reviewers:
+ rsesek@chromium.org

[Robert Sesek, 2016-06-15 14:55:39]

https://codereview.chromium.org/2070483002/diff/20001/ipc/SECURITY_OWNERS
File ipc/SECURITY_OWNERS (right):

https://codereview.chromium.org/2070483002/diff/20001/ipc/SECURITY_OWNERS#new...
ipc/SECURITY_OWNERS:14: ricky@chromium.org
rickyz maybe?

[inferno, 2016-06-15 15:04:42]

inferno@chromium.org changed reviewers:
+ inferno@chromium.org

[inferno, 2016-06-15 15:04:43]

https://codereview.chromium.org/2070483002/diff/20001/ipc/SECURITY_OWNERS
File ipc/SECURITY_OWNERS (right):

https://codereview.chromium.org/2070483002/diff/20001/ipc/SECURITY_OWNERS#new...
ipc/SECURITY_OWNERS:4: inferno@chromium.org
Can you please remove me, i haven't done ipc reviews in a long time.

[Will Harris, 2016-06-15 15:19:44]

wfh@chromium.org changed reviewers:
+ wfh@chromium.org

[Will Harris, 2016-06-15 15:19:45]

this only affects messages in content/common?

[dcheng, 2016-06-15 15:46:01]

On 2016/06/15 15:19:45, Will Harris wrote:
> this only affects messages in content/common?

I'll update the remaining OWNERS once it's clear no one wants me to do something
completely different. =)

[dcheng, 2016-06-16 08:29:13]

https://codereview.chromium.org/2070483002/diff/20001/ipc/SECURITY_OWNERS
File ipc/SECURITY_OWNERS (right):

https://codereview.chromium.org/2070483002/diff/20001/ipc/SECURITY_OWNERS#new...
ipc/SECURITY_OWNERS:4: inferno@chromium.org
On 2016/06/15 15:04:43, inferno wrote:
> Can you please remove me, i haven't done ipc reviews in a long time.

Done.

https://codereview.chromium.org/2070483002/diff/20001/ipc/SECURITY_OWNERS#new...
ipc/SECURITY_OWNERS:14: ricky@chromium.org
On 2016/06/15 14:55:39, Robert Sesek wrote:
> rickyz maybe?

Oops, yeah. Done.

[Dirk Pranke, 2016-06-17 05:45:52]

https://codereview.chromium.org/2070483002/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2070483002/diff/40001/PRESUBMIT.py#newcode1403
PRESUBMIT.py:1403: file_patterns = [
Please add a docstring or a comment here describing what this is checking for.

Also, please add some comments describing the algorithm, since I'm not really
following it and it's not clear to me that it's correct. We should probably add
some unit tests for it to PRESUBMIT_test.py, too.

https://codereview.chromium.org/2070483002/diff/40001/PRESUBMIT.py#newcode1434
PRESUBMIT.py:1434: with file(owners_file) as f:
Make sure the file exists (or at least handle a missing file cleanly).

[dcheng, 2016-06-17 22:01:31]

https://codereview.chromium.org/2070483002/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2070483002/diff/40001/PRESUBMIT.py#newcode1403
PRESUBMIT.py:1403: file_patterns = [
On 2016/06/17 05:45:52, Dirk Pranke wrote:
> Please add a docstring or a comment here describing what this is checking for.
> 
> Also, please add some comments describing the algorithm, since I'm not really
> following it and it's not clear to me that it's correct. We should probably
add
> some unit tests for it to PRESUBMIT_test.py, too.
> 

Done (and added comments throughout as well).

Not sure how to write a unit test for this, since we use open(). Is there an
example of how to do this?

https://codereview.chromium.org/2070483002/diff/40001/PRESUBMIT.py#newcode1434
PRESUBMIT.py:1434: with file(owners_file) as f:
On 2016/06/17 05:45:52, Dirk Pranke wrote:
> Make sure the file exists (or at least handle a missing file cleanly).

Done.

[Dirk Pranke, 2016-06-18 20:49:26]

lgtm.

I'm not sure there is a good way to test this since you're reading files, as you
say.

The comments definitely help, though.

https://codereview.chromium.org/2070483002/diff/60001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2070483002/diff/60001/PRESUBMIT.py#newcode1473
PRESUBMIT.py:1473: entry['rules'] = [rule for rule in entry['rules'] if rule !=
line]
Maybe

  lines = f.read().splitlines()
  for entry in patterns.itervalues():
    entry['rules'] = [rule for rule in entry['rules'] if rule not in lines]

is clearer and possibly also more efficient?

[dcheng, 2016-06-19 00:03:28]

https://codereview.chromium.org/2070483002/diff/60001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/2070483002/diff/60001/PRESUBMIT.py#newcode1473
PRESUBMIT.py:1473: entry['rules'] = [rule for rule in entry['rules'] if rule !=
line]
On 2016/06/18 20:49:26, Dirk Pranke wrote:
> Maybe
> 
>   lines = f.read().splitlines()
>   for entry in patterns.itervalues():
>     entry['rules'] = [rule for rule in entry['rules'] if rule not in lines]
> 
> is clearer and possibly also more efficient?

I tweaked this slightly and put the lines in a set. So now it's more efficient
from a strict algorithmic complexity standpoint… though I'm not sure it will
actually be more efficient in practice =P

[dcheng, 2016-06-19 19:56:57]

Description was changed from

==========
Add PRESUBMIT rule to make sure IPC changes are security reviewed.

The PRESUBMIT rule makes sure that files involving IPC are covered
by the appropriate per-file rules. See https://goo.gl/NRiIPv for
other alternatives that were considered (e.g. why not recursive
per-file rules?).

BUG=611905
==========

to

==========
Add PRESUBMIT rule to make sure IPC changes are security reviewed.

The PRESUBMIT rule makes sure that files involving IPC are covered
by the appropriate per-file rules. See https://goo.gl/NRiIPv for
other alternatives that were considered (e.g. why not recursive
per-file rules?).

BUG=611905
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[dcheng, 2016-06-19 19:59:11]

dcheng@chromium.org changed reviewers:
+ jam@chromium.org

[dcheng, 2016-06-19 19:59:13]

+jam, mind taking a look at the OWNERS files changes and confirming this is OK?

https://codereview.chromium.org/2070483002/diff/120001/chrome/browser/importe...
File chrome/browser/importer/OWNERS (left):

https://codereview.chromium.org/2070483002/diff/120001/chrome/browser/importe...
chrome/browser/importer/OWNERS:4: # For security review of IPC message files.
I removed these lines since they don't match anything. I only cleared the
obvious low-hanging fruit, like messages defined in 'browser' or 'child'
directories.

[jam, 2016-06-20 17:50:38]

lgtm

[dcheng, 2016-06-20 18:04:26]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-06-20 18:04:27]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/2070483002/#ps120001
(title: "Update OWNERS and add Blink-specific patterns")

[commit-bot: I haz the power, 2016-06-20 18:04:58]

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

[commit-bot: I haz the power, 2016-06-20 19:27:34]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-20 19:29:17]

Description was changed from

==========
Add PRESUBMIT rule to make sure IPC changes are security reviewed.

The PRESUBMIT rule makes sure that files involving IPC are covered
by the appropriate per-file rules. See https://goo.gl/NRiIPv for
other alternatives that were considered (e.g. why not recursive
per-file rules?).

BUG=611905
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add PRESUBMIT rule to make sure IPC changes are security reviewed.

The PRESUBMIT rule makes sure that files involving IPC are covered
by the appropriate per-file rules. See https://goo.gl/NRiIPv for
other alternatives that were considered (e.g. why not recursive
per-file rules?).

BUG=611905
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e07de81b11b1c5b62eaaa258e896878eb5364ff2
Cr-Commit-Position: refs/heads/master@{#400748}
==========

[commit-bot: I haz the power, 2016-06-20 19:29:19]

Patchset 7 (id:??) landed as
https://crrev.com/e07de81b11b1c5b62eaaa258e896878eb5364ff2
Cr-Commit-Position: refs/heads/master@{#400748}

[xlai (Olivia), 2016-06-20 20:50:17]

xlai@chromium.org changed reviewers:
+ xlai@chromium.org

[xlai (Olivia), 2016-06-20 20:50:19]

This patch is failing the PRESUBMIT bot of my CL
(https://codereview.chromium.org/2036663003/#ps360001) that has already been
lgtm'ed by dcheng@, one of the security owners. I made changes to cc/ipc.
Because of this, I am blocked to land my CL.

I guess that the path pointing to //ipc/SECURITY_OWNERS inside cc/ipc/OWNERS is
probably of wrong syntax. 

The PRESUBMIT error I'm getting is:
** Presubmit ERRORS **
Found changes to IPC files without a security OWNER!

***************
cc/ipc/OWNERS is missing the following lines:

per-file *_struct_traits*.*=set noparent
per-file *_struct_traits*.*=file://ipc/SECURITY_OWNERS

for changed files:
  cc/ipc/surface_id_struct_traits.h
  cc/ipc/surface_sequence_struct_traits.h

[dcheng, 2016-06-20 21:09:51]

On 2016/06/20 20:50:19, xlai (Olivia) wrote:
> This patch is failing the PRESUBMIT bot of my CL
> (https://codereview.chromium.org/2036663003/#ps360001) that has already been
> lgtm'ed by dcheng@, one of the security owners. I made changes to cc/ipc.
> Because of this, I am blocked to land my CL.
> 
> I guess that the path pointing to //ipc/SECURITY_OWNERS inside cc/ipc/OWNERS
is
> probably of wrong syntax. 
> 
> The PRESUBMIT error I'm getting is:
> ** Presubmit ERRORS **
> Found changes to IPC files without a security OWNER!
> 
> ***************
> cc/ipc/OWNERS is missing the following lines:
> 
> per-file *_struct_traits*.*=set noparent
> per-file *_struct_traits*.*=file://ipc/SECURITY_OWNERS
> 
> for changed files:
>   cc/ipc/surface_id_struct_traits.h
>   cc/ipc/surface_sequence_struct_traits.h

On the train ATM, but as discussed offline, the problem is a typo I made when
updating cc/ipc (it's missing the _ before struct and param). I'll fix when I'm
off the train.

[xlai (Olivia), 2016-06-20 21:34:17]

Description was changed from

==========
Add PRESUBMIT rule to make sure IPC changes are security reviewed.

The PRESUBMIT rule makes sure that files involving IPC are covered
by the appropriate per-file rules. See https://goo.gl/NRiIPv for
other alternatives that were considered (e.g. why not recursive
per-file rules?).

BUG=611905
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e07de81b11b1c5b62eaaa258e896878eb5364ff2
Cr-Commit-Position: refs/heads/master@{#400748}
==========

to

==========
Add PRESUBMIT rule to make sure IPC changes are security reviewed.

The PRESUBMIT rule makes sure that files involving IPC are covered
by the appropriate per-file rules. See https://goo.gl/NRiIPv for
other alternatives that were considered (e.g. why not recursive
per-file rules?).

BUG=611905
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e07de81b11b1c5b62eaaa258e896878eb5364ff2
Cr-Commit-Position: refs/heads/master@{#400748}
==========

[xlai (Olivia), 2016-06-20 21:34:17]

xlai@chromium.org changed reviewers:
- xlai@chromium.org

[vinceh, 2016-06-21 06:19:45]

On 2016/06/20 21:09:51, dcheng wrote:
> On 2016/06/20 20:50:19, xlai (Olivia) wrote:
> > This patch is failing the PRESUBMIT bot of my CL
> > (https://codereview.chromium.org/2036663003/#ps360001) that has already been
> > lgtm'ed by dcheng@, one of the security owners. I made changes to cc/ipc.
> > Because of this, I am blocked to land my CL.
> > 
> > I guess that the path pointing to //ipc/SECURITY_OWNERS inside cc/ipc/OWNERS
> is
> > probably of wrong syntax. 
> > 
> > The PRESUBMIT error I'm getting is:
> > ** Presubmit ERRORS **
> > Found changes to IPC files without a security OWNER!
> > 
> > ***************
> > cc/ipc/OWNERS is missing the following lines:
> > 
> > per-file *_struct_traits*.*=set noparent
> > per-file *_struct_traits*.*=file://ipc/SECURITY_OWNERS
> > 
> > for changed files:
> >   cc/ipc/surface_id_struct_traits.h
> >   cc/ipc/surface_sequence_struct_traits.h
> 
> On the train ATM, but as discussed offline, the problem is a typo I made when
> updating cc/ipc (it's missing the _ before struct and param). I'll fix when
I'm
> off the train.

I hit a similar problem when uploading change for issue 2039813002. Should we
fix the ui/gfx/*/OWNERS like https://codereview.chromium.org/2087643002/ ?

** Presubmit ERRORS **
Found changes to IPC files without a security OWNER!

***************
ui/gfx/ipc/OWNERS is missing the following lines:

per-file *_param_traits*.*=set noparent
per-file *_param_traits*.*=file://ipc/SECURITY_OWNERS

for changed files:
  ui/gfx/ipc/gfx_param_traits_macros.h
***************

[kjellander_chromium, 2016-06-21 08:28:39]

On 2016/06/21 06:19:45, vince.h wrote:
> On 2016/06/20 21:09:51, dcheng wrote:
> > On 2016/06/20 20:50:19, xlai (Olivia) wrote:
> > > This patch is failing the PRESUBMIT bot of my CL
> > > (https://codereview.chromium.org/2036663003/#ps360001) that has already
been
> > > lgtm'ed by dcheng@, one of the security owners. I made changes to cc/ipc.
> > > Because of this, I am blocked to land my CL.
> > > 
> > > I guess that the path pointing to //ipc/SECURITY_OWNERS inside
cc/ipc/OWNERS
> > is
> > > probably of wrong syntax. 
> > > 
> > > The PRESUBMIT error I'm getting is:
> > > ** Presubmit ERRORS **
> > > Found changes to IPC files without a security OWNER!
> > > 
> > > ***************
> > > cc/ipc/OWNERS is missing the following lines:
> > > 
> > > per-file *_struct_traits*.*=set noparent
> > > per-file *_struct_traits*.*=file://ipc/SECURITY_OWNERS
> > > 
> > > for changed files:
> > >   cc/ipc/surface_id_struct_traits.h
> > >   cc/ipc/surface_sequence_struct_traits.h
> > 
> > On the train ATM, but as discussed offline, the problem is a typo I made
when
> > updating cc/ipc (it's missing the _ before struct and param). I'll fix when
> I'm
> > off the train.
> 
> I hit a similar problem when uploading change for issue 2039813002. Should we
> fix the ui/gfx/*/OWNERS like https://codereview.chromium.org/2087643002/ ?
> 
> ** Presubmit ERRORS **
> Found changes to IPC files without a security OWNER!
> 
> ***************
> ui/gfx/ipc/OWNERS is missing the following lines:
> 
> per-file *_param_traits*.*=set noparent
> per-file *_param_traits*.*=file://ipc/SECURITY_OWNERS
> 
> for changed files:
>   ui/gfx/ipc/gfx_param_traits_macros.h
> ***************

FYI to others ending up here: If you get an error like this you have a SVN-based
copy of depot_tools:

Running presubmit upload checks ...
Traceback (most recent call last):
  File "/ssd/depot_tools/git_cl.py", line 4938, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/ssd/depot_tools/git_cl.py", line 4920, in main
    return dispatcher.execute(OptionParser(), argv)
  File "/ssd/depot_tools/subcommand.py", line 252, in execute
    return command(parser, args[1:])
  File "/ssd/depot_tools/git_cl.py", line 3651, in CMDupload
    return cl.CMDUpload(options, args, orig_args)
  File "/ssd/depot_tools/git_cl.py", line 1381, in CMDUpload
    change=change)
  File "/ssd/depot_tools/git_cl.py", line 1325, in RunHook
    gerrit_obj=self._codereview_impl.GetGerritObjForPresubmit())
  File "/ssd/depot_tools/presubmit_support.py", line 1548, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/ssd/depot_tools/presubmit_support.py", line 1460, in
ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 2173, in CheckChangeOnUpload
  File "<string>", line 1925, in _CommonChecks
  File "<string>", line 1992, in _CheckAuthorizedAuthor
  File "<string>", line 1992, in <genexpr>
AttributeError: 'InputApi' object has no attribute 'fnmatch'

See
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/eGYkwi5T-YY/SDdaw...
for instructions on how to replace it with a pure Git based version.

[engedy, 2016-06-21 10:06:35]

I prepared https://codereview.chromium.org/2086803003/ to make all OWNERS file
use the same spelling as required by the PRESUBMIT script for files matching the
globs.