Repository ids, branches and commit ids have a very specific set of allowed values. Rather than permit any string to be uploaded, restrict these values to the valid ones.
<rdar://problem/54564837>
Created attachment 377354 [details] Patch
Created attachment 377359 [details] Patch
Created attachment 377388 [details] Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=377354&action=review > Tools/ChangeLog:13 > + repostiroy_id, branch or commit id are rejected. Nit: typo repostiroy_id > Tools/resultsdbpy/resultsdbpy/controller/commit.py:58 > + raise ValueError(f'{self.branch} is an invalid branch') Nit: 'invalid branch' => 'invalid branch name'. We don't know if the branch is invalid, the 'branch name' is invalid. Curious, can branch name contain unicode characters? > Tools/resultsdbpy/resultsdbpy/controller/commit.py:63 > + raise ValueError(f'{self.id} is an invalid id') Nit: 'invalid id' => 'invalid commit id'. > Tools/resultsdbpy/resultsdbpy/controller/commit_unittest.py:110 > + with self.assertRaises(ValueError): Can we test the exact error message as well (e.g.: 'invalid-repo is an invalid repository id').
(In reply to Aakash Jain from comment #5) > View in context: > https://bugs.webkit.org/attachment.cgi?id=377354&action=review > > > Tools/ChangeLog:13 > > + repostiroy_id, branch or commit id are rejected. > > Nit: typo repostiroy_id > > > Tools/resultsdbpy/resultsdbpy/controller/commit.py:58 > > + raise ValueError(f'{self.branch} is an invalid branch') > > Nit: 'invalid branch' => 'invalid branch name'. We don't know if the branch > is invalid, the 'branch name' is invalid. > > Curious, can branch name contain unicode characters? Even if git allows it, I don't think we should. We can cross this bridge when and if we come to it. > > > Tools/resultsdbpy/resultsdbpy/controller/commit.py:63 > > + raise ValueError(f'{self.id} is an invalid id') > > Nit: 'invalid id' => 'invalid commit id'. > > > Tools/resultsdbpy/resultsdbpy/controller/commit_unittest.py:110 > > + with self.assertRaises(ValueError): > > Can we test the exact error message as well (e.g.: 'invalid-repo is an > invalid repository id').
Created attachment 377395 [details] Patch
Created attachment 377397 [details] Patch
Comment on attachment 377397 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377397&action=review > Tools/resultsdbpy/resultsdbpy/controller/commit_unittest.py:102 > + def assertRaisesWithMessage(self, msg, func, *args, **kwargs): is this used anywhere? > Tools/resultsdbpy/resultsdbpy/controller/commit_unittest.py:140 > + self.assertEqual(str(error.exception), "'<html>1234</html>' is an invalid commit id") Please add a test-case for checking the max length as well.
Created attachment 377403 [details] Patch
Comment on attachment 377403 [details] Patch Rejecting attachment 377403 [details] from commit-queue. New failing tests: fullscreen/full-screen-request-removed-with-raf.html Full output: https://webkit-queues.webkit.org/results/12975101
Created attachment 377411 [details] Archive of layout-test-results from webkit-cq-03 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: webkit-cq-03 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 377403 [details] Patch Rejecting attachment 377403 [details] from commit-queue. New failing tests: fullscreen/full-screen-request-removed-with-raf.html Full output: https://webkit-queues.webkit.org/results/12976521
Created attachment 377443 [details] Archive of layout-test-results from webkit-cq-03 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: webkit-cq-03 Port: mac-highsierra Platform: Mac OS X 10.13.6
Committed r249200: <https://trac.webkit.org/changeset/249200>