From f8131741ef18e957aa65c1ee054526650533476a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C4=93teris=20Caune?= Date: Fri, 10 Sep 2021 22:49:12 +0300 Subject: [PATCH] Fix minor API inconsistencies 1. Drop API support for GET, DELETE requests with a request body. Healthchecks had an undocumented quirk where you could authenticate a GET or DELETE request by putting a '{"api_key":"..."}' in request body. This commit removes this feature. Note: POST requests can still authenticate either by sending a X-Api-Key header, or by putting a "api_key" key in request body. GET and DELETE requests can now only authenticate with the request header. 2. Add missing @csrf_exempt annotations in API views When client sends a HTTP POST request to a GET-only endpoint, the server is supposed to respond with "405 Method Not Allowed". Due to CSRF checking, a couple endpoints were responding with "403 Forbidden" instead. Adding @csrf_exempt annotations fixes the problem. --- CHANGELOG.md | 2 ++ hc/api/decorators.py | 2 +- hc/api/tests/test_delete_check.py | 11 +++++++---- hc/api/tests/test_get_badges.py | 2 +- hc/api/tests/test_get_flips.py | 6 +++++- hc/api/tests/test_get_pings.py | 6 +++++- hc/api/tests/test_list_channels.py | 19 ++++++++++--------- hc/api/tests/test_list_checks.py | 9 --------- hc/api/views.py | 10 +++------- hc/test.py | 4 +++- 10 files changed, 37 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6af79b3..83454db9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ All notable changes to this project will be documented in this file. - Add handling for non-latin-1 characters in webhook headers - Fix dark mode bug in selectpicker widgets - Fix a crash during login when user's profile does not exist (#77) +- Drop API support for GET, DELETE requests with a request body +- Add missing @csrf_exempt annotations in API views ## v1.22.0 - 2020-08-06 diff --git a/hc/api/decorators.py b/hc/api/decorators.py index 22df3105..09885067 100644 --- a/hc/api/decorators.py +++ b/hc/api/decorators.py @@ -73,7 +73,7 @@ def validate_json(schema=None): def decorator(f): @wraps(f) def wrapper(request, *args, **kwds): - if request.body: + if request.method == "POST" and request.body: try: request.json = json.loads(request.body.decode()) except ValueError: diff --git a/hc/api/tests/test_delete_check.py b/hc/api/tests/test_delete_check.py index d06dbc0d..4e359aa3 100644 --- a/hc/api/tests/test_delete_check.py +++ b/hc/api/tests/test_delete_check.py @@ -6,11 +6,10 @@ class DeleteCheckTestCase(BaseTestCase): def setUp(self): super().setUp() self.check = Check.objects.create(project=self.project) + self.url = f"/api/v1/checks/{self.check.code}" def test_it_works(self): - r = self.client.delete( - "/api/v1/checks/%s" % self.check.code, HTTP_X_API_KEY="X" * 32 - ) + r = self.client.delete(self.url, HTTP_X_API_KEY="X" * 32) self.assertEqual(r.status_code, 200) self.assertEqual(r["Access-Control-Allow-Origin"], "*") @@ -23,6 +22,10 @@ class DeleteCheckTestCase(BaseTestCase): self.assertEqual(r.status_code, 404) def test_it_handles_options(self): - r = self.client.options("/api/v1/checks/%s" % self.check.code) + r = self.client.options(self.url) self.assertEqual(r.status_code, 204) self.assertIn("DELETE", r["Access-Control-Allow-Methods"]) + + def test_it_handles_missing_api_key(self): + r = self.client.delete(self.url) + self.assertContains(r, "missing api key", status_code=401) diff --git a/hc/api/tests/test_get_badges.py b/hc/api/tests/test_get_badges.py index ef235d8e..6865ce46 100644 --- a/hc/api/tests/test_get_badges.py +++ b/hc/api/tests/test_get_badges.py @@ -39,7 +39,7 @@ class GetBadgesTestCase(BaseTestCase): self.assertEqual(r.status_code, 200) def test_it_rejects_post(self): - r = self.client.post(self.url, HTTP_X_API_KEY="X" * 32) + r = self.csrf_client.post(self.url, HTTP_X_API_KEY="X" * 32) self.assertEqual(r.status_code, 405) def test_it_handles_missing_api_key(self): diff --git a/hc/api/tests/test_get_flips.py b/hc/api/tests/test_get_flips.py index 05393b0d..da585470 100644 --- a/hc/api/tests/test_get_flips.py +++ b/hc/api/tests/test_get_flips.py @@ -53,7 +53,7 @@ class GetFlipsTestCase(BaseTestCase): self.assertEqual(r.status_code, 200) def test_it_rejects_post(self): - r = self.client.post(self.url, HTTP_X_API_KEY="X" * 32) + r = self.csrf_client.post(self.url, HTTP_X_API_KEY="X" * 32) self.assertEqual(r.status_code, 405) def test_it_rejects_non_integer_start(self): @@ -82,3 +82,7 @@ class GetFlipsTestCase(BaseTestCase): def test_it_rejects_huge_seconds(self): r = self.get(qs="?seconds=12345678901234567890") self.assertEqual(r.status_code, 400) + + def test_it_handles_missing_api_key(self): + r = self.client.get(self.url) + self.assertContains(r, "missing api key", status_code=401) diff --git a/hc/api/tests/test_get_pings.py b/hc/api/tests/test_get_pings.py index bcd7546e..8b156a6d 100644 --- a/hc/api/tests/test_get_pings.py +++ b/hc/api/tests/test_get_pings.py @@ -54,5 +54,9 @@ class GetPingsTestCase(BaseTestCase): self.assertEqual(r.status_code, 401) def test_it_rejects_post(self): - r = self.client.post(self.url, HTTP_X_API_KEY="X" * 32) + r = self.csrf_client.post(self.url, HTTP_X_API_KEY="X" * 32) self.assertEqual(r.status_code, 405) + + def test_it_handles_missing_api_key(self): + r = self.client.get(self.url) + self.assertContains(r, "missing api key", status_code=401) diff --git a/hc/api/tests/test_list_channels.py b/hc/api/tests/test_list_channels.py index 7a9a8289..70c5093d 100644 --- a/hc/api/tests/test_list_channels.py +++ b/hc/api/tests/test_list_channels.py @@ -13,8 +13,10 @@ class ListChannelsTestCase(BaseTestCase): self.c1.name = "Email to Alice" self.c1.save() + self.url = "/api/v1/channels/" + def get(self): - return self.client.get("/api/v1/channels/", HTTP_X_API_KEY="X" * 32) + return self.client.get(self.url, HTTP_X_API_KEY="X" * 32) def test_it_works(self): r = self.get() @@ -30,7 +32,7 @@ class ListChannelsTestCase(BaseTestCase): self.assertEqual(c["name"], "Email to Alice") def test_it_handles_options(self): - r = self.client.options("/api/v1/channels/") + r = self.client.options(self.url) self.assertEqual(r.status_code, 204) self.assertIn("GET", r["Access-Control-Allow-Methods"]) @@ -43,11 +45,10 @@ class ListChannelsTestCase(BaseTestCase): for c in data["channels"]: self.assertNotEqual(c["name"], "Bob") - def test_it_accepts_api_key_from_request_body(self): - payload = json.dumps({"api_key": "X" * 32}) - r = self.client.generic( - "GET", "/api/v1/channels/", payload, content_type="application/json" - ) + def test_it_handles_missing_api_key(self): + r = self.client.get(self.url) + self.assertContains(r, "missing api key", status_code=401) - self.assertEqual(r.status_code, 200) - self.assertContains(r, "Email to Alice") + def test_it_rejects_post(self): + r = self.csrf_client.post(self.url, HTTP_X_API_KEY="X" * 32) + self.assertEqual(r.status_code, 405) diff --git a/hc/api/tests/test_list_checks.py b/hc/api/tests/test_list_checks.py index cbfb5492..0101366e 100644 --- a/hc/api/tests/test_list_checks.py +++ b/hc/api/tests/test_list_checks.py @@ -94,15 +94,6 @@ class ListChecksTestCase(BaseTestCase): for check in data["checks"]: self.assertNotEqual(check["name"], "Bob 1") - def test_it_accepts_api_key_from_request_body(self): - payload = json.dumps({"api_key": "X" * 32}) - r = self.client.generic( - "GET", "/api/v1/checks/", payload, content_type="application/json" - ) - - self.assertEqual(r.status_code, 200) - self.assertContains(r, "Alice") - def test_it_works_with_tags_param(self): r = self.client.get("/api/v1/checks/?tag=a2-tag", HTTP_X_API_KEY="X" * 32) self.assertEqual(r.status_code, 200) diff --git a/hc/api/views.py b/hc/api/views.py index 35fc3ff8..d726e55e 100644 --- a/hc/api/views.py +++ b/hc/api/views.py @@ -171,7 +171,6 @@ def _update(check, spec): check.channel_set.set(new_channels) -@validate_json() @authorize_read def get_checks(request): q = Check.objects.filter(project=request.project) @@ -222,7 +221,7 @@ def checks(request): @cors("GET") -@validate_json() +@csrf_exempt @authorize def channels(request): q = Channel.objects.filter(project=request.project) @@ -230,7 +229,6 @@ def channels(request): return JsonResponse({"channels": channels}) -@validate_json() @authorize_read def get_check(request, code): check = get_object_or_404(Check, code=code) @@ -241,7 +239,6 @@ def get_check(request, code): @cors("GET") @csrf_exempt -@validate_json() @authorize_read def get_check_by_unique_key(request, unique_key): checks = Check.objects.filter(project=request.project.id) @@ -266,7 +263,6 @@ def update_check(request, code): return JsonResponse(check.to_dict()) -@validate_json() @authorize def delete_check(request, code): check = get_object_or_404(Check, code=code) @@ -312,6 +308,7 @@ def pause(request, code): @cors("GET") +@csrf_exempt @validate_json() @authorize def pings(request, code): @@ -369,7 +366,6 @@ def flips(request, check): @cors("GET") @csrf_exempt -@validate_json() @authorize_read def flips_by_uuid(request, code): check = get_object_or_404(Check, code=code) @@ -378,7 +374,6 @@ def flips_by_uuid(request, code): @cors("GET") @csrf_exempt -@validate_json() @authorize_read def flips_by_unique_key(request, unique_key): checks = Check.objects.filter(project=request.project.id) @@ -389,6 +384,7 @@ def flips_by_unique_key(request, unique_key): @cors("GET") +@csrf_exempt @authorize_read def badges(request): tags = set(["*"]) diff --git a/hc/test.py b/hc/test.py index 2413ad99..b8221650 100644 --- a/hc/test.py +++ b/hc/test.py @@ -1,6 +1,6 @@ from django.contrib.auth.models import User from django.core.signing import TimestampSigner -from django.test import TestCase +from django.test import Client, TestCase from hc.accounts.models import Member, Profile, Project @@ -9,6 +9,8 @@ class BaseTestCase(TestCase): def setUp(self): super().setUp() + self.csrf_client = Client(enforce_csrf_checks=True) + # Alice is a normal user for tests. Alice has team access enabled. self.alice = User(username="alice", email="alice@example.org") self.alice.set_password("password")