webhook: fix non-matching webhook secrets not being rejected (#601)

Fixes an issue which allowed webhooks to be processed with an incorrect
webhook secret.

This adds a case to return a `401` when the `verify` function returns
false. The `verify` function also always returns false as it is being
passed in JSON from a `WorkflowJob` webhook object instead of the
request body (which is how GitHub generates it's signitures) meaning the
signatures will never match (see
https://github.com/yanyongyu/githubkit?tab=readme-ov-file#webhook-verification).

Author: harryfinbow

runner_manager/routers/webhook.py

@@ -1,6 +1,6 @@
 from typing import Annotated, Set
 
-from fastapi import APIRouter, Depends, Header, HTTPException, Security
+from fastapi import APIRouter, Depends, Header, HTTPException, Request, Security
 from githubkit.versions.latest.models import WebhookPing
 from githubkit.webhooks import verify
 from rq import Queue
@@ -18,21 +18,30 @@
 }
 
 
-def validate_webhook(
-    webhook: AcceptedWebhookEvents,
+async def validate_webhook(
+    request: Request,
     settings: Settings = Depends(get_settings),
     x_hub_signature_256: Annotated[str | None, Header()] = None,
 ) -> bool:
 
+    body = await request.body()
+
     if settings.github_webhook_secret is None:
         return True
     elif x_hub_signature_256 is None:
         raise HTTPException(status_code=401, detail="Missing signature")
     valid: bool = verify(
         settings.github_webhook_secret.get_secret_value(),
-        webhook.json(exclude_unset=True),
+        body,
         x_hub_signature_256,
     )
+
+    if not valid:
+        raise HTTPException(
+            status_code=401,
+            detail="Signature values do not match - check webhook secret value",
+        )
+
     return valid
 
 
@@ -43,6 +52,7 @@ def post(
     valid: bool = Security(validate_webhook),
     queue: Queue = Depends(get_queue),
 ) -> WebhookResponse:
+
     if not isinstance(webhook, WebhookPing):
         action = webhook.action
     else:

tests/api/test_webhook_router.py

@@ -41,9 +41,11 @@ def test_workflow_job_hypothesis(workflow_job: WorkflowJobCompleted):
 @given(workflow_job=WorkflowJobCompletedStrategy)
 def test_webhook_authentication(workflow_job, client, authentified_app):
     data = workflow_job.json(exclude_unset=True)
+
     # First request without authentication
     response = client.post("/webhook/", content=data)
     assert response.status_code == 401
+
     # Second request with authentication
     signature = sign("secret", data, method="sha256")
     response = client.post(
@@ -53,6 +55,15 @@ def test_webhook_authentication(workflow_job, client, authentified_app):
     )
     assert response.status_code == 200
 
+    # Third request with incorrect authentication
+    signature = sign("wrong_secret", data, method="sha256")
+    response = client.post(
+        "/webhook/",
+        content=data,
+        headers={"X-Hub-Signature-256": signature, "X-GitHub-Event": "workflow_job"},
+    )
+    assert response.status_code == 401
+
 
 @given(ping=PingStrategy)
 def test_ping_event(ping, client):