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

@@ -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
 

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:

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)

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):

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)

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)

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)

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)

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(["*"])

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="[email protected]")
         self.alice.set_password("password")