Maciej Szulik added the comment: I'm missing following elements in this submission: - tests: * negative cases (you only have unsupported media type and request method) but more is needed, iow. test every path: missing Github event, secret, etc. * issue created by anonymous, user with github username set * multiple fixes both in title like you have in pullrequestevent3.txt but also in title - code comments in roundup/pull_request.py
And more comments: roundup/pull_request.py: > raise Reject("Key {} is not present".format(str(e))) This information is important for administrator, the requester should not see this information. Log the error and throw generic 'Invalid request' exception. > raise Reject('missing X-GitHub-Event header') Generally, you don't want to 'help' the attacker know what went wrong, but rather just throw generic 'Invalid request' exception in such cases, as well. > def _get_issue_id(self): With the new approach (multiple ids) it would be nice to rename the variables and methods to represent the plurality, iow. use ids in all those places. > ids = self.issue_id_re.findall(match.group()) > return ids Just return self_issue_id_re.findall(match.group()), you don't need that variable. > self._extract() Is vulnerable to other exceptions, I'm a lazy person and I test your code with following command: curl -X POST -H "content-type: application/json" -H "X-GitHub-Event: pull_request" -d @test/data/pullrequestevent3.txt http://localhost:9999/python-dev/pull_request doing so I frequently forgot you have additional data in the testfiles and I'm getting "ValueError: No JSON object could be decoded" at this point. Verify other exceptions and throw generic 'Invalid request' in those places (and don't forget to add tests). > url_exists = len(self.db.pull_request.filter(None, {'url': url})) == 1 Not being used, please remove this line. Unfortunately, I'm able to add multiple times the same URL, you should be checking the URLs but in the context of a single issue. _______________________________________________________ PSF Meta Tracker <metatrac...@psf.upfronthosting.co.za> <http://psf.upfronthosting.co.za/roundup/meta/issue589> _______________________________________________________ _______________________________________________ Tracker-discuss mailing list Tracker-discuss@python.org https://mail.python.org/mailman/listinfo/tracker-discuss Code of Conduct: https://www.python.org/psf/codeofconduct/