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/
