Newly added mojom files bypass security review enforcement |
||
Issue descriptionThis is because of https://crrev.com/d0573ec067f852c3a967b8e6aa38ad07615a6dc9, which does OWNERS checks with OldContents(). If a CL adds OWNERS rules for a mojom file and a new mojom file in the same CL, owners.py will use the version of the OWNERS files that doesn't enforce security review. Doh! One workaround might be to extend the _CheckIpcOwners() check to verify that at least one security reviewer is added...
,
Jan 11 2018
One idea: When doing OWNERS checks, only use override_files for checking changes to OWNERS files themselves. Unfortunately, this means that if you remove a directory, the removed files would use NewContents() as the check and you'd need parent OWNER approval...
,
Jan 12 2018
ouch :/
,
Aug 25
https://chromium-review.googlesource.com/c/chromium/src/+/1137103 is an example of a CL that got missed due to this bug.
,
Aug 25
,
Aug 28
OK, I've been thinking about this more. Would it be reasonable to tweak the OWNERS check so that we use OldContents() when determining owners for changes to OWNERS files... and NewContents() otherwise? To me, the main situation we want to avoid is someone being able to add themselves to an OWNERS file *and* be able to +1 it. It's not a big issue for non-OWNERS files: either there are no changes to OWNERS, and an existing reviewer will look at it--or the CL also includes a change to OWNERS, at which point, a pre-existing OWNER will have to look at the CL anyway. Of course, this gets tricky due to file: includes... so I'd also propose restricting file: includes for OWNERS to require that the included file end in a suffix of "OWNERS". dpranke, WDYT of this proposal since jochen is OOO? I explored several other ways to approach this, including having a seperate presubmit to try to ensure IPC security reviewers are included as needed--but it ends up being kind of hard to support all the edge cases properly.
,
Aug 29
@dcheng - the proposal in #c6 seems fine.
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/74fda7147c1b547d343e3323e5babdd304ad4f98 commit 74fda7147c1b547d343e3323e5babdd304ad4f98 Author: Daniel Cheng <dcheng@chromium.org> Date: Wed Sep 05 03:56:39 2018 [owners.py] require stricter naming conventions for file: includes Files that control ownership have special ownership rules, to disallow someone from self-approving a CL that adds themselves to a file that controls ownership. owners.py now requires that they be named OWNERS or end with a suffix of _OWNERS to ensure the special ownership rules are correctly applied. Bug: 801315 Change-Id: I083a2d15bdc9c749e3838fb1c983286d5a7c4af9 Reviewed-on: https://chromium-review.googlesource.com/1204917 Reviewed-by: Dirk Pranke <dpranke@chromium.org> Commit-Queue: Daniel Cheng <dcheng@chromium.org> [modify] https://crrev.com/74fda7147c1b547d343e3323e5babdd304ad4f98/owners.py [modify] https://crrev.com/74fda7147c1b547d343e3323e5babdd304ad4f98/tests/owners_unittest.py
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/02958afbab8ca4e117639434e311f3c744ed3e4c commit 02958afbab8ca4e117639434e311f3c744ed3e4c Author: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Date: Wed Sep 05 09:13:21 2018 Roll src/third_party/depot_tools f215ae6ed0ce..74fda7147c1b (1 commits) https://chromium.googlesource.com/chromium/tools/depot_tools.git/+log/f215ae6ed0ce..74fda7147c1b git log f215ae6ed0ce..74fda7147c1b --date=short --no-merges --format='%ad %ae %s' 2018-09-05 dcheng@chromium.org [owners.py] require stricter naming conventions for file: includes Created with: gclient setdep -r src/third_party/depot_tools@74fda7147c1b The AutoRoll server is located here: https://autoroll.skia.org/r/depot-tools-chromium-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC'd on the roll, and stop the roller if necessary. BUG=chromium:801315 TBR=agable@chromium.org Change-Id: I91dee5ab9d4c5b0d83f4ae6669a475689f265e01 Reviewed-on: https://chromium-review.googlesource.com/1205617 Reviewed-by: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Commit-Queue: depot-tools-chromium-autoroll <depot-tools-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com> Cr-Commit-Position: refs/heads/master@{#588802} [modify] https://crrev.com/02958afbab8ca4e117639434e311f3c744ed3e4c/DEPS |
||
►
Sign in to add a comment |
||
Comment 1 by dcheng@chromium.org
, Jan 11 2018