From 74fda7147c1b547d343e3323e5babdd304ad4f98 Mon Sep 17 00:00:00 2001 From: Daniel Cheng Date: Wed, 5 Sep 2018 03:56:39 +0000 Subject: [PATCH] [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 Commit-Queue: Daniel Cheng --- owners.py | 37 ++++++++++++++++++++++++------------- tests/owners_unittest.py | 17 +++++++++++++---- 2 files changed, 37 insertions(+), 17 deletions(-) diff --git a/owners.py b/owners.py index 1b572bf1dd..0c5dbd6b10 100644 --- a/owners.py +++ b/owners.py @@ -11,19 +11,22 @@ so you may need approval from both an OWNER and a reviewer in many cases. The syntax of the OWNERS file is, roughly: -lines := (\s* line? \s* comment? \s* "\n")* +lines := (\s* line? \s* comment? \s* "\n")* -line := directive - | "per-file" \s+ glob \s* "=" \s* directive +line := directive + | "per-file" \s+ glob \s* "=" \s* directive -directive := "set noparent" - | "file:" glob - | email_address - | "*" +directive := "set noparent" + | "file:" owner_file + | email_address + | "*" -glob := [a-zA-Z0-9_-*?]+ +glob := [a-zA-Z0-9_-*?]+ -comment := "#" [^"\n"]* +comment := "#" [^"\n"]* + +owner_file := "OWNERS" + | [^"\n"]* "_OWNERS" Email addresses must follow the foo@bar.com short form (exact syntax given in BASIC_EMAIL_REGEXP, below). Filename globs follow the simple unix @@ -48,7 +51,8 @@ glob. If the "file:" directive is used, the referred to OWNERS file will be parsed and considered when determining the valid set of OWNERS. If the filename starts with "//" it is relative to the root of the repository, otherwise it is relative to -the current file +the current file. The referred to file *must* be named OWNERS or end in a suffix +of _OWNERS. Examples for all of these combinations can be found in tests/owners_unittest.py. """ @@ -356,7 +360,7 @@ class Database(object): if directive == 'set noparent': self._stop_looking.add(owned_paths) elif directive.startswith('file:'): - include_file = self._resolve_include(directive[5:], owners_path) + include_file = self._resolve_include(directive[5:], owners_path, lineno) if not include_file: raise SyntaxErrorInOwnersFile(owners_path, lineno, ('%s does not refer to an existing file.' % directive[5:])) @@ -376,7 +380,7 @@ class Database(object): ('"%s" is not a "set noparent", file include, "*", ' 'or an email address.' % (directive,))) - def _resolve_include(self, path, start): + def _resolve_include(self, path, start, lineno): if path.startswith('//'): include_path = path[2:] else: @@ -388,6 +392,13 @@ class Database(object): return include_path owners_path = self.os_path.join(self.root, include_path) + # Paths included via "file:" must end in OWNERS or _OWNERS. Files that can + # affect ownership have a different set of ownership rules, so that users + # cannot self-approve changes adding themselves to an OWNERS file. + if not (owners_path.endswith('/OWNERS') or owners_path.endswith('_OWNERS')): + raise SyntaxErrorInOwnersFile(start, lineno, 'file: include must specify ' + 'a file named OWNERS or ending in _OWNERS') + if not self.os_path.exists(owners_path): return None @@ -416,7 +427,7 @@ class Database(object): owners.add(line) continue if line.startswith('file:'): - sub_include_file = self._resolve_include(line[5:], include_file) + sub_include_file = self._resolve_include(line[5:], include_file, lineno) sub_owners = self._read_just_the_owners(sub_include_file) owners.update(sub_owners) continue diff --git a/tests/owners_unittest.py b/tests/owners_unittest.py index bac4955a85..b416396e38 100755 --- a/tests/owners_unittest.py +++ b/tests/owners_unittest.py @@ -306,12 +306,21 @@ class OwnersDatabaseTest(_BaseTestCase): self.test_file_include_absolute_path() def test_file_include_different_filename(self): - self.files['/owners/garply'] = owners_file(peter) + self.files['/owners/GARPLY_OWNERS'] = owners_file(peter) self.files['/content/garply/OWNERS'] = owners_file(john, - lines=['per-file foo.*=file://owners/garply']) + lines=['per-file foo.*=file://owners/GARPLY_OWNERS']) self.assert_files_not_covered_by(['content/garply/foo.cc'], [peter], []) + def test_file_include_invalid_filename(self): + self.files['/base/SECURITY_REVIEWERS'] = owners_file(peter) + self.files['/ipc/OWNERS'] = owners_file(file='//base/SECURITY_REVIEWERS') + try: + self.db().reviewers_for(['ipc/ipc_message_utils.h'], None) + self.fail() # pragma: no cover + except owners.SyntaxErrorInOwnersFile, e: + self.assertTrue(str(e).startswith('/ipc/OWNERS:1')) + def assert_syntax_error(self, owners_file_contents): db = self.db() self.files['/foo/OWNERS'] = owners_file_contents @@ -332,10 +341,10 @@ class OwnersDatabaseTest(_BaseTestCase): self.assert_syntax_error('ben\n') def test_syntax_error__invalid_absolute_file(self): - self.assert_syntax_error('file://foo/bar/baz\n') + self.assert_syntax_error('file://foo/bar/OWNERS\n') def test_syntax_error__invalid_relative_file(self): - self.assert_syntax_error('file:foo/bar/baz\n') + self.assert_syntax_error('file:foo/bar/OWNERS\n') def test_non_existant_status_file(self): db = self.db()