From 5dafc07c297d88ae9aeecb3c24e3bbf00f25d679 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C4=93teris=20Caune?= Date: Fri, 10 Mar 2017 10:35:21 +0200 Subject: [PATCH] Return 403 when API key is wrong. Return 404 when resource not found. Return 405 when request method is wrong. Return 400 when request syntax is wrong. --- hc/api/decorators.py | 5 ++-- hc/api/tests/test_badge.py | 2 +- hc/api/tests/test_bounce.py | 8 ++++- hc/api/tests/test_create_check.py | 4 +-- hc/api/tests/test_pause.py | 4 +-- hc/api/tests/test_ping.py | 4 +++ hc/api/tests/test_update_check.py | 9 +++++- hc/api/views.py | 42 ++++++++++----------------- hc/front/tests/test_add_check.py | 2 +- hc/front/tests/test_cron_preview.py | 2 +- hc/front/tests/test_pause.py | 2 +- hc/front/tests/test_remove_channel.py | 2 +- hc/front/tests/test_remove_check.py | 2 +- hc/front/tests/test_update_name.py | 2 +- hc/front/tests/test_update_timeout.py | 2 +- hc/front/views.py | 29 +++++------------- 16 files changed, 58 insertions(+), 63 deletions(-) diff --git a/hc/api/decorators.py b/hc/api/decorators.py index cc0777e6..7b87863b 100644 --- a/hc/api/decorators.py +++ b/hc/api/decorators.py @@ -3,7 +3,8 @@ import uuid from functools import wraps from django.contrib.auth.models import User -from django.http import HttpResponseBadRequest, JsonResponse +from django.http import (HttpResponseBadRequest, HttpResponseForbidden, + JsonResponse) from hc.lib.jsonschema import ValidationError, validate @@ -44,7 +45,7 @@ def check_api_key(f): try: request.user = User.objects.get(profile__api_key=api_key) except User.DoesNotExist: - return make_error("wrong api_key") + return HttpResponseForbidden() return f(request, *args, **kwds) diff --git a/hc/api/tests/test_badge.py b/hc/api/tests/test_badge.py index 31042e52..e51232b3 100644 --- a/hc/api/tests/test_badge.py +++ b/hc/api/tests/test_badge.py @@ -13,7 +13,7 @@ class BadgeTestCase(BaseTestCase): def test_it_rejects_bad_signature(self): r = self.client.get("/badge/%s/12345678/foo.svg" % self.alice.username) - assert r.status_code == 400 + assert r.status_code == 404 def test_it_returns_svg(self): sig = base64_hmac(str(self.alice.username), "foo", settings.SECRET_KEY) diff --git a/hc/api/tests/test_bounce.py b/hc/api/tests/test_bounce.py index da858de2..9b51019a 100644 --- a/hc/api/tests/test_bounce.py +++ b/hc/api/tests/test_bounce.py @@ -33,10 +33,16 @@ class BounceTestCase(BaseTestCase): url = "/api/v1/notifications/%s/bounce" % self.n.code r = self.client.post(url, "foo", content_type="text/plain") - self.assertEqual(r.status_code, 400) + self.assertEqual(r.status_code, 403) def test_it_handles_long_payload(self): url = "/api/v1/notifications/%s/bounce" % self.n.code payload = "A" * 500 r = self.client.post(url, payload, content_type="text/plain") self.assertEqual(r.status_code, 200) + + def test_it_handles_missing_notification(self): + fake_code = "07c2f548-9850-4b27-af5d-6c9dc157ec02" + url = "/api/v1/notifications/%s/bounce" % fake_code + r = self.client.post(url, "", content_type="text/plain") + self.assertEqual(r.status_code, 404) diff --git a/hc/api/tests/test_create_check.py b/hc/api/tests/test_create_check.py index e8122eb8..3a91534a 100644 --- a/hc/api/tests/test_create_check.py +++ b/hc/api/tests/test_create_check.py @@ -95,8 +95,8 @@ class CreateCheckTestCase(BaseTestCase): self.assertEqual(r.json()["error"], "could not parse request body") def test_it_rejects_wrong_api_key(self): - self.post({"api_key": "wrong"}, - expected_error="wrong api_key") + r = self.post({"api_key": "wrong"}) + self.assertEqual(r.status_code, 403) def test_it_rejects_small_timeout(self): self.post({"api_key": "abc", "timeout": 0}, diff --git a/hc/api/tests/test_pause.py b/hc/api/tests/test_pause.py index cee8c62a..3e134be6 100644 --- a/hc/api/tests/test_pause.py +++ b/hc/api/tests/test_pause.py @@ -31,7 +31,7 @@ class PauseTestCase(BaseTestCase): r = self.client.post(url, "", content_type="application/json", HTTP_X_API_KEY="abc") - self.assertEqual(r.status_code, 400) + self.assertEqual(r.status_code, 403) def test_it_validates_uuid(self): url = "/api/v1/checks/not-uuid/pause" @@ -45,4 +45,4 @@ class PauseTestCase(BaseTestCase): r = self.client.post(url, "", content_type="application/json", HTTP_X_API_KEY="abc") - self.assertEqual(r.status_code, 400) + self.assertEqual(r.status_code, 404) diff --git a/hc/api/tests/test_ping.py b/hc/api/tests/test_ping.py index 43b0be67..3285e157 100644 --- a/hc/api/tests/test_ping.py +++ b/hc/api/tests/test_ping.py @@ -45,6 +45,10 @@ class PingTestCase(TestCase): r = self.client.get("/ping/not-uuid/") assert r.status_code == 400 + def test_it_handles_missing_check(self): + r = self.client.get("/ping/07c2f548-9850-4b27-af5d-6c9dc157ec02/") + assert r.status_code == 404 + def test_it_handles_120_char_ua(self): ua = ("Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) " "AppleWebKit/537.36 (KHTML, like Gecko) " diff --git a/hc/api/tests/test_update_check.py b/hc/api/tests/test_update_check.py index c350f0ed..fd49739c 100644 --- a/hc/api/tests/test_update_check.py +++ b/hc/api/tests/test_update_check.py @@ -74,7 +74,14 @@ class UpdateCheckTestCase(BaseTestCase): def test_it_handles_missing_check(self): made_up_code = "07c2f548-9850-4b27-af5d-6c9dc157ec02" r = self.post(made_up_code, {"api_key": "abc"}) - self.assertEqual(r.status_code, 400) + self.assertEqual(r.status_code, 404) + + def test_it_validates_ownership(self): + check = Check(user=self.bob, status="up") + check.save() + + r = self.post(check.code, {"api_key": "abc"}) + self.assertEqual(r.status_code, 403) def test_it_updates_cron_to_simple(self): self.check.kind = "cron" diff --git a/hc/api/views.py b/hc/api/views.py index 2d10a479..63ad58d1 100644 --- a/hc/api/views.py +++ b/hc/api/views.py @@ -1,10 +1,13 @@ from datetime import timedelta as td from django.db.models import F -from django.http import HttpResponse, HttpResponseBadRequest, JsonResponse +from django.http import (HttpResponse, HttpResponseForbidden, + HttpResponseNotFound, JsonResponse) +from django.shortcuts import get_object_or_404 from django.utils import timezone from django.views.decorators.cache import never_cache from django.views.decorators.csrf import csrf_exempt +from django.views.decorators.http import require_POST from hc.api import schemas from hc.api.decorators import check_api_key, uuid_or_400, validate_json @@ -16,10 +19,7 @@ from hc.lib.badges import check_signature, get_badge_svg @uuid_or_400 @never_cache def ping(request, code): - try: - check = Check.objects.get(code=code) - except Check.DoesNotExist: - return HttpResponseBadRequest() + check = get_object_or_404(Check, code=code) check.n_pings = F("n_pings") + 1 check.last_ping = timezone.now() @@ -122,34 +122,27 @@ def checks(request): @csrf_exempt +@require_POST @uuid_or_400 @check_api_key @validate_json(schemas.check) def update(request, code): - if request.method != "POST": - return HttpResponse(status=405) # method not allowed - - try: - check = Check.objects.get(code=code, user=request.user) - except Check.DoesNotExist: - return HttpResponseBadRequest() + check = get_object_or_404(Check, code=code) + if check.user != request.user: + return HttpResponseForbidden() _update(check, request.json) return JsonResponse(check.to_dict(), status=200) @csrf_exempt +@require_POST @uuid_or_400 @check_api_key def pause(request, code): - if request.method != "POST": - # Method not allowed - return HttpResponse(status=405) - - try: - check = Check.objects.get(code=code, user=request.user) - except Check.DoesNotExist: - return HttpResponseBadRequest() + check = get_object_or_404(Check, code=code) + if check.user != request.user: + return HttpResponseForbidden() check.status = "paused" check.save() @@ -159,7 +152,7 @@ def pause(request, code): @never_cache def badge(request, username, signature, tag): if not check_signature(username, tag, signature): - return HttpResponseBadRequest() + return HttpResponseNotFound() status = "up" q = Check.objects.filter(user__username=username, tags__contains=tag) @@ -181,15 +174,12 @@ def badge(request, username, signature, tag): @csrf_exempt @uuid_or_400 def bounce(request, code): - try: - notification = Notification.objects.get(code=code) - except Notification.DoesNotExist: - return HttpResponseBadRequest() + notification = get_object_or_404(Notification, code=code) # If webhook is more than 10 minutes late, don't accept it: td = timezone.now() - notification.created if td.total_seconds() > 600: - return HttpResponseBadRequest() + return HttpResponseForbidden() notification.error = request.body[:200] notification.save() diff --git a/hc/front/tests/test_add_check.py b/hc/front/tests/test_add_check.py index a0dc89ff..553d71f5 100644 --- a/hc/front/tests/test_add_check.py +++ b/hc/front/tests/test_add_check.py @@ -24,4 +24,4 @@ class AddCheckTestCase(BaseTestCase): url = "/checks/add/" self.client.login(username="alice@example.org", password="password") r = self.client.get(url) - self.assertEqual(r.status_code, 400) + self.assertEqual(r.status_code, 405) diff --git a/hc/front/tests/test_cron_preview.py b/hc/front/tests/test_cron_preview.py index 975bfe2a..cc06c2ab 100644 --- a/hc/front/tests/test_cron_preview.py +++ b/hc/front/tests/test_cron_preview.py @@ -29,4 +29,4 @@ class CronPreviewTestCase(BaseTestCase): def test_it_rejects_get(self): r = self.client.get("/checks/cron_preview/", {}) - self.assertEqual(r.status_code, 400) + self.assertEqual(r.status_code, 405) diff --git a/hc/front/tests/test_pause.py b/hc/front/tests/test_pause.py index 26d709f6..ba173506 100644 --- a/hc/front/tests/test_pause.py +++ b/hc/front/tests/test_pause.py @@ -23,4 +23,4 @@ class PauseTestCase(BaseTestCase): url = "/checks/%s/pause/" % self.check.code self.client.login(username="alice@example.org", password="password") r = self.client.get(url) - self.assertEqual(r.status_code, 400) + self.assertEqual(r.status_code, 405) diff --git a/hc/front/tests/test_remove_channel.py b/hc/front/tests/test_remove_channel.py index 8cd8e311..9ba167d3 100644 --- a/hc/front/tests/test_remove_channel.py +++ b/hc/front/tests/test_remove_channel.py @@ -52,4 +52,4 @@ class RemoveChannelTestCase(BaseTestCase): url = "/integrations/%s/remove/" % self.channel.code self.client.login(username="alice@example.org", password="password") r = self.client.get(url) - self.assertEqual(r.status_code, 400) + self.assertEqual(r.status_code, 405) diff --git a/hc/front/tests/test_remove_check.py b/hc/front/tests/test_remove_check.py index d2c09d32..a661782d 100644 --- a/hc/front/tests/test_remove_check.py +++ b/hc/front/tests/test_remove_check.py @@ -53,4 +53,4 @@ class RemoveCheckTestCase(BaseTestCase): url = "/checks/%s/remove/" % self.check.code self.client.login(username="alice@example.org", password="password") r = self.client.get(url) - self.assertEqual(r.status_code, 400) + self.assertEqual(r.status_code, 405) diff --git a/hc/front/tests/test_update_name.py b/hc/front/tests/test_update_name.py index 047db4b9..f88b0b25 100644 --- a/hc/front/tests/test_update_name.py +++ b/hc/front/tests/test_update_name.py @@ -71,4 +71,4 @@ class UpdateNameTestCase(BaseTestCase): url = "/checks/%s/name/" % self.check.code self.client.login(username="alice@example.org", password="password") r = self.client.get(url) - self.assertEqual(r.status_code, 400) + self.assertEqual(r.status_code, 405) diff --git a/hc/front/tests/test_update_timeout.py b/hc/front/tests/test_update_timeout.py index fc0a4a31..dc25bfeb 100644 --- a/hc/front/tests/test_update_timeout.py +++ b/hc/front/tests/test_update_timeout.py @@ -125,4 +125,4 @@ class UpdateTimeoutTestCase(BaseTestCase): url = "/checks/%s/timeout/" % self.check.code self.client.login(username="alice@example.org", password="password") r = self.client.get(url) - self.assertEqual(r.status_code, 400) + self.assertEqual(r.status_code, 405) diff --git a/hc/front/views.py b/hc/front/views.py index 8c6e6238..c409dd23 100644 --- a/hc/front/views.py +++ b/hc/front/views.py @@ -14,6 +14,7 @@ from django.urls import reverse from django.utils import timezone from django.utils.crypto import get_random_string from django.views.decorators.csrf import csrf_exempt +from django.views.decorators.http import require_POST from django.utils.six.moves.urllib.parse import urlencode from hc.api.decorators import uuid_or_400 from hc.api.models import (DEFAULT_GRACE, DEFAULT_TIMEOUT, Channel, Check, @@ -131,11 +132,9 @@ def about(request): return render(request, "front/about.html", {"page": "about"}) +@require_POST @login_required def add_check(request): - if request.method != "POST": - return HttpResponseBadRequest() - check = Check(user=request.team.user) check.save() @@ -144,12 +143,10 @@ def add_check(request): return redirect("hc-checks") +@require_POST @login_required @uuid_or_400 def update_name(request, code): - if request.method != "POST": - return HttpResponseBadRequest() - check = get_object_or_404(Check, code=code) if check.user_id != request.team.user.id: return HttpResponseForbidden() @@ -163,12 +160,10 @@ def update_name(request, code): return redirect("hc-checks") +@require_POST @login_required @uuid_or_400 def update_timeout(request, code): - if request.method != "POST": - return HttpResponseBadRequest() - check = get_object_or_404(Check, code=code) if check.user != request.team.user: return HttpResponseForbidden() @@ -200,10 +195,8 @@ def update_timeout(request, code): @csrf_exempt +@require_POST def cron_preview(request): - if request.method != "POST": - return HttpResponseBadRequest() - schedule = request.POST.get("schedule") tz = request.POST.get("tz") ctx = {"tz": tz, "dates": []} @@ -223,12 +216,10 @@ def cron_preview(request): return render(request, "front/cron_preview.html", ctx) +@require_POST @login_required @uuid_or_400 def pause(request, code): - if request.method != "POST": - return HttpResponseBadRequest() - check = get_object_or_404(Check, code=code) if check.user_id != request.team.user.id: return HttpResponseForbidden() @@ -239,12 +230,10 @@ def pause(request, code): return redirect("hc-checks") +@require_POST @login_required @uuid_or_400 def remove_check(request, code): - if request.method != "POST": - return HttpResponseBadRequest() - check = get_object_or_404(Check, code=code) if check.user != request.team.user: return HttpResponseForbidden() @@ -375,12 +364,10 @@ def unsubscribe_email(request, code, token): return render(request, "front/unsubscribe_success.html") +@require_POST @login_required @uuid_or_400 def remove_channel(request, code): - if request.method != "POST": - return HttpResponseBadRequest() - # user may refresh the page during POST and cause two deletion attempts channel = Channel.objects.filter(code=code).first() if channel: