-
Notifications
You must be signed in to change notification settings - Fork 112
Improvement: Github webhook, CORS, global exception exception handling in FastAPI returns JSON response. #216
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,15 @@ | ||
| import asyncio | ||
| import uuid | ||
| import hmac | ||
| import hashlib | ||
| import logging | ||
| from fastapi import APIRouter, Request, HTTPException | ||
| import uuid | ||
| import json | ||
| from fastapi import APIRouter, Request, HTTPException, Header | ||
| from app.core.events.event_bus import EventBus | ||
| from app.core.events.enums import EventType, PlatformType | ||
| from app.core.events.base import BaseEvent | ||
| from app.core.handler.handler_registry import HandlerRegistry | ||
| from pydantic import BaseModel | ||
| from app.core.config.settings import settings | ||
|
|
||
| router = APIRouter() | ||
|
|
||
|
|
@@ -35,50 +38,74 @@ def register_event_handlers(): | |
| event_bus.register_handler(EventType.PR_COMMENTED, sample_handler, PlatformType.GITHUB) | ||
| event_bus.register_handler(EventType.PR_MERGED, sample_handler, PlatformType.GITHUB) | ||
|
|
||
| async def verify_signature(body: bytes, x_hub_signature_256: str): | ||
| """Verify that the payload was sent from GitHub by validating the SHA256 signature.""" | ||
| if not settings.github_webhook_secret: | ||
| # If secret is not configured, we skip verification (not recommended for production) | ||
| return | ||
|
|
||
| if not x_hub_signature_256: | ||
| raise HTTPException(status_code=401, detail="X-Hub-Signature-256 header is missing") | ||
|
|
||
| signature = hmac.new( | ||
| settings.github_webhook_secret.encode(), | ||
| body, | ||
| hashlib.sha256 | ||
| ).hexdigest() | ||
|
|
||
| if not hmac.compare_digest(f"sha256={signature}", x_hub_signature_256): | ||
| raise HTTPException(status_code=401, detail="Invalid webhook signature") | ||
|
|
||
| @router.post("/github/webhook") | ||
| async def github_webhook(request: Request): | ||
| payload = await request.json() | ||
| event_header = request.headers.get("X-GitHub-Event") | ||
| logging.info(f"Received GitHub event: {event_header}") | ||
| async def github_webhook( | ||
| request: Request, | ||
| x_github_event: str = Header(None), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate required webhook headers explicitly. The 🔎 Proposed fix async def github_webhook(
request: Request,
- x_github_event: str = Header(None),
+ x_github_event: str = Header(...),
x_hub_signature_256: str = Header(None)
):Using 🤖 Prompt for AI Agents |
||
| x_hub_signature_256: str = Header(None) | ||
| ): | ||
| # Read body once to avoid stream exhaustion | ||
| body = await request.body() | ||
|
|
||
| # Verify GitHub signature | ||
| await verify_signature(body, x_hub_signature_256) | ||
|
|
||
| try: | ||
| payload = json.loads(body.decode()) | ||
| except json.JSONDecodeError: | ||
| raise HTTPException(status_code=400, detail="Invalid JSON payload") from None | ||
| logging.info(f"Received GitHub event: {x_github_event}") | ||
|
|
||
| event_type = None | ||
|
|
||
| # Handle issue events | ||
| if event_header == "issues": | ||
| action = payload.get("action") | ||
| if action == "opened": | ||
| event_type = EventType.ISSUE_CREATED | ||
| elif action == "closed": | ||
| event_type = EventType.ISSUE_CLOSED | ||
| elif action == "edited": | ||
| event_type = EventType.ISSUE_UPDATED | ||
|
|
||
| # Handle issue comment events | ||
| elif event_header == "issue_comment": | ||
| action = payload.get("action") | ||
| if action == "created": | ||
| event_type = EventType.ISSUE_COMMENTED | ||
| # Mapping GitHub events and actions to EventType | ||
| event_mapping = { | ||
| "issues": { | ||
| "opened": EventType.ISSUE_CREATED, | ||
| "closed": EventType.ISSUE_CLOSED, | ||
| "edited": EventType.ISSUE_UPDATED, | ||
| }, | ||
| "issue_comment": { | ||
| "created": EventType.ISSUE_COMMENTED, | ||
| }, | ||
| "pull_request": { | ||
| "opened": EventType.PR_CREATED, | ||
| "edited": EventType.PR_UPDATED, | ||
| }, | ||
| "pull_request_review_comment": { | ||
| "created": EventType.PR_COMMENTED, | ||
| }, | ||
| } | ||
|
|
||
| # Handle pull request events | ||
| elif event_header == "pull_request": | ||
| if x_github_event in event_mapping: | ||
| action = payload.get("action") | ||
| if action == "opened": | ||
| event_type = EventType.PR_CREATED | ||
| elif action == "edited": | ||
| event_type = EventType.PR_UPDATED | ||
| elif action == "closed": | ||
| # Determine if the PR was merged or simply closed | ||
| event_type = event_mapping[x_github_event].get(action) | ||
|
|
||
| # Special casing for PR merged | ||
| if x_github_event == "pull_request" and action == "closed": | ||
| if payload.get("pull_request", {}).get("merged"): | ||
| event_type = EventType.PR_MERGED | ||
| else: | ||
| logging.info("Pull request closed without merge; no event dispatched.") | ||
|
|
||
| # Handle pull request comment events | ||
| elif event_header in ["pull_request_review_comment", "pull_request_comment"]: | ||
| action = payload.get("action") | ||
| if action == "created": | ||
| event_type = EventType.PR_COMMENTED | ||
|
|
||
| # Dispatch the event if we have a matching type | ||
| if event_type: | ||
| event = BaseEvent( | ||
|
|
@@ -90,6 +117,6 @@ async def github_webhook(request: Request): | |
| ) | ||
| await event_bus.dispatch(event) | ||
| else: | ||
| logging.info(f"No matching event type for header: {event_header} with action: {payload.get('action')}") | ||
| logging.info(f"No matching event type for header: {x_github_event} with action: {payload.get('action')}") | ||
|
|
||
| return {"status": "ok"} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,16 @@ | ||
| <!doctype html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <link rel="icon" type="image/svg+xml" href="/vite.svg" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Devr.AI</title> | ||
| </head> | ||
| <body> | ||
| <div id="root"></div> | ||
| <script type="module" src="/src/main.tsx"></script> | ||
| </body> | ||
| </html> | ||
|
|
||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <link rel="icon" type="image/svg+xml" href="/DevrAI.svg" /> | ||
RishiGoswami-code marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Devr.AI</title> | ||
| </head> | ||
|
|
||
| <body> | ||
| <div id="root"></div> | ||
| <script type="module" src="/src/main.tsx"></script> | ||
| </body> | ||
|
|
||
| </html> | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add warning log when signature verification is skipped.
Silently skipping signature verification creates a security risk. If accidentally deployed to production without a configured secret, the endpoint will accept unauthenticated webhook requests from any source.
🔎 Proposed fix
if not settings.github_webhook_secret: # If secret is not configured, we skip verification (not recommended for production) + logging.warning("GitHub webhook secret not configured - signature verification skipped. This is insecure for production!") return🤖 Prompt for AI Agents