RESOLVED FIXED 201189
results.webkit.org: Sanitize all commit arguments on upload 
https://bugs.webkit.org/show_bug.cgi?id=201189
Summary results.webkit.org: Sanitize all commit arguments on upload
Jonathan Bedard
Reported 2019-08-27 11:06:54 PDT
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.
Attachments
Patch (3.26 KB, patch)
2019-08-27 11:10 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (3.58 KB, patch)
2019-08-27 11:39 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (4.34 KB, patch)
2019-08-27 14:47 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (5.27 KB, patch)
2019-08-27 15:56 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (5.25 KB, patch)
2019-08-27 16:09 PDT, Jonathan Bedard 		no flags
DetailsFormatted DiffDiff
Patch (5.95 KB, patch)
2019-08-27 17:08 PDT, Jonathan Bedard 		commit-queue: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from webkit-cq-03 for mac-highsierra (3.32 MB, application/zip)
2019-08-27 17:54 PDT, WebKit Commit Bot 		no flags
Details
Archive of layout-test-results from webkit-cq-03 for mac-highsierra (3.20 MB, application/zip)
2019-08-28 08:34 PDT, WebKit Commit Bot 		no flags
Details
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Jonathan Bedard
Comment 1 2019-08-27 11:07:26 PDT
<rdar://problem/54564837>
Jonathan Bedard
Comment 2 2019-08-27 11:10:59 PDT
Created attachment 377354 [details] Patch
Jonathan Bedard
Comment 3 2019-08-27 11:39:20 PDT
Created attachment 377359 [details] Patch
Jonathan Bedard
Comment 4 2019-08-27 14:47:12 PDT
Created attachment 377388 [details] Patch
Aakash Jain
Comment 5 2019-08-27 15:01:06 PDT
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').
Jonathan Bedard
Comment 6 2019-08-27 15:17:37 PDT
(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').
Jonathan Bedard
Comment 7 2019-08-27 15:56:53 PDT
Created attachment 377395 [details] Patch
Jonathan Bedard
Comment 8 2019-08-27 16:09:41 PDT
Created attachment 377397 [details] Patch
Aakash Jain
Comment 9 2019-08-27 16:40:40 PDT
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.
Jonathan Bedard
Comment 10 2019-08-27 17:08:44 PDT
Created attachment 377403 [details] Patch
WebKit Commit Bot
Comment 11 2019-08-27 17:54:02 PDT
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
WebKit Commit Bot
Comment 12 2019-08-27 17:54:04 PDT
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
WebKit Commit Bot
Comment 13 2019-08-28 08:34:17 PDT
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
WebKit Commit Bot
Comment 14 2019-08-28 08:34:19 PDT
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
Jonathan Bedard
Comment 15 2019-08-28 08:45:22 PDT
Committed r249200: <https://trac.webkit.org/changeset/249200>
Note You need to log in before you can comment on or make changes to this bug.