From dbd6bb6cd0a82fd81ec44c4d79b7f53bc9a8bb2a Mon Sep 17 00:00:00 2001 From: claude Date: Sun, 23 Aug 2026 12:21:43 -0400 Subject: [PATCH] fix(link): revoke endpoint was disabled by its own rate-limit rule Live QA: every call to /v1/devices/revoke returned 429 retry_after=3600. The rule was written (0, 3600) with a comment saying 'never throttle someone out of signing a lost laptop out' -- but in this limiter a limit of 0 DISABLES the endpoint outright. The comment said unlimited; the code said never. Set to 600/hour instead. Every unit test passed while the endpoint was 100% dead over HTTP, because they called svc.revoke_device() directly and never went through the handler. Added 4 tests that speak HTTP, including one that fails if ANY route in DEFAULT_RATE_RULES is configured to 0. 176 tests (was 172). --- server/granthi_link.py | 11 ++++++--- tests/test_server.py | 56 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/server/granthi_link.py b/server/granthi_link.py index a2b4624..35342fa 100644 --- a/server/granthi_link.py +++ b/server/granthi_link.py @@ -90,9 +90,14 @@ DEFAULT_RATE_RULES = {"/v1/link": (5, 3600), "/v1/repos": (60, 3600), # Reads are cheap but still authenticated work. "/v1/devices": (120, 3600), "/v1/audit": (120, 3600), - # Revocation is a safety action: never throttle - # someone out of signing a lost laptop out. - "/v1/devices/revoke": (0, 3600)} + # Revocation is a safety action, so its limit is set + # high rather than tight. It is NOT 0: in this limiter + # a limit of 0 DISABLES the endpoint outright (see + # RateLimiter.check), which would mean nobody could + # ever sign a lost laptop out. Caught in live QA -- + # unit tests called the service method directly and so + # never went through the limiter at all. + "/v1/devices/revoke": (600, 3600)} MAX_RATE_KEYS = 10000 diff --git a/tests/test_server.py b/tests/test_server.py index bb902fb..3d27434 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -728,6 +728,62 @@ class TestTokenNameUniqueness(ServiceTestBase): self.assertIsNone(self.svc.whoami(a["token"])) self.assertEqual(self.svc.whoami(b["token"]), "alice") + +class TestDeviceEndpointsOverHttp(HandlerTestBase): + """Through the real handler, not the service method. + + The first live run of the revoke endpoint returned 429 on every call + while every unit test passed: the tests called svc.revoke_device() + directly, so nothing ever went through the rate limiter. Any route whose + limit is a policy decision needs at least one test that speaks HTTP. + """ + + def setUp(self): + super().setUp() + self.enable_test_mode() + self.a = self.stub_link("s1", "alice", device="laptop", + device_id="dev-a")[1] + self.b = self.stub_link("s1", "alice", device="desktop", + device_id="dev-b")[1] + + def post(self, path, obj): + body = json.dumps(obj).encode() + return self.raw_post(path, body, + {"Content-Type": "application/json", + "Content-Length": str(len(body))}) + + def test_revoke_is_reachable_and_not_rate_limited_away(self): + status, resp = self.post("/v1/devices/revoke", + {"token": self.b["token"], + "device_id": "dev-a"}) + self.assertEqual(status, 200, resp) + self.assertIsNone(self.svc.whoami(self.a["token"])) + + def test_repeated_revokes_keep_working(self): + """A person signing several lost machines out in one sitting must not + be locked out partway through.""" + for i in range(12): + self.stub_link("s1", "alice", device=f"d{i}", device_id=f"gone-{i}") + for i in range(12): + status, resp = self.post("/v1/devices/revoke", + {"token": self.b["token"], + "device_id": f"gone-{i}"}) + self.assertEqual(status, 200, f"revoke {i}: {resp}") + + def test_devices_and_audit_are_reachable_over_http(self): + status, resp = self.post("/v1/devices", {"token": self.a["token"]}) + self.assertEqual(status, 200) + self.assertEqual(len(resp["devices"]), 2) + status, resp = self.post("/v1/audit", {"token": self.a["token"]}) + self.assertEqual(status, 200) + self.assertTrue(resp["events"]) + + def test_no_route_a_client_uses_is_configured_to_zero(self): + """0 means DISABLED in this limiter, so a zero on a live route is a + dead endpoint, not an unlimited one.""" + for route, (limit, _window) in granthi_link.DEFAULT_RATE_RULES.items(): + self.assertGreater(limit, 0, f"{route} is disabled by its rule") + if __name__ == "__main__": unittest.main()