| Message ID | 20210820153258.2191277-1-dja@axtens.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | REST: don't 500 if we get a non-int ID in /comment/ views | expand |
On Sat, 2021-08-21 at 01:32 +1000, Daniel Axtens wrote: > The Patch and Cover comment views passed unsanitised patch_id/pk > down to the query set filter, which attempts to convert them to > integers, fails, and propagates a ValueError up to us. > > Rather than propagating it to the user as a 500, catch it explicitly > and return a 404. Ideally we'd like to return a 400 and some explanation, > but I can't see an easy way to bend Django or DRF to my will here. > > Signed-off-by: Daniel Axtens <dja@axtens.net> > > --- > > Will need some tweaking for 2.2, but conceptually simple. > > Stephen, would be interested if you can come up with a better way > to propagate a meaningful error out to the user! I did indeed find another way to do this. Two ways, in fact. I've submitted a patch to do both, but tl;dr: A. We can use path converters (of type 'int') to ensure these requests never even hit our handlers B. We can use 'rest_framework.generic.get_object_or_404' instead of Django's own implementation, since the former also handles TypeErrors Let me know if this works for you. I can apply it and cut a new release early next week if you can't. Cheers, Stephen
diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py index 0c578b44f1cb..4fa48ff73e49 100644 --- a/patchwork/api/comment.py +++ b/patchwork/api/comment.py @@ -86,6 +86,13 @@ class CoverCommentList(ListAPIView): lookup_url_kwarg = 'pk' def get_queryset(self): + # ideally we'd want to raise a 400 here with some explanation, but + # it's not clear how to. DRF throws away any 404 message we provide + try: + int(self.kwargs['pk']) + except ValueError: + raise Http404 + if not Cover.objects.filter(pk=self.kwargs['pk']).exists(): raise Http404 @@ -105,6 +112,13 @@ class PatchCommentList(ListAPIView): lookup_url_kwarg = 'patch_id' def get_queryset(self): + # ideally we'd want to raise a 400 here with some explanation, but + # it's not clear how to. DRF throws away any 404 message we provide + try: + int(self.kwargs['patch_id']) + except ValueError: + raise Http404 + if not Patch.objects.filter(id=self.kwargs['patch_id']).exists(): raise Http404
The Patch and Cover comment views passed unsanitised patch_id/pk down to the query set filter, which attempts to convert them to integers, fails, and propagates a ValueError up to us. Rather than propagating it to the user as a 500, catch it explicitly and return a 404. Ideally we'd like to return a 400 and some explanation, but I can't see an easy way to bend Django or DRF to my will here. Signed-off-by: Daniel Axtens <dja@axtens.net> --- Will need some tweaking for 2.2, but conceptually simple. Stephen, would be interested if you can come up with a better way to propagate a meaningful error out to the user! --- patchwork/api/comment.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+)