Extract GitHub webhook route#367
Closed
TUPM96 wants to merge 1 commit into
Closed
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR refactors GitHub webhook handling by extracting routing logic into a dedicated module with header normalization and status mapping, integrates it into the main application, hardens redirect path validation for URL-decoded inputs, adds endpoint tests, and removes unused SQLAlchemy typing support. ChangesGitHub Webhook Routing and Main Application Integration
Webhook Tests and Type Cleanup
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR factors the GitHub webhook endpoint into a dedicated routes module, adds tests around webhook behavior, and hardens redirect “next” path validation.
Changes:
- Added
app/webhooks/routes.pywith header normalization + status code mapping and registered it increate_app. - Added webhook route tests for ignored events and bad signatures.
- Tightened
_safe_next_pathby validating both raw and percent-decoded forms; simplified SQLAlchemy typing aroundrowcount.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/test_webhook_routes.py | Adds coverage for GitHub webhook route behavior and persistence of webhook events. |
| app/webhooks/routes.py | Introduces a dedicated module to register the GitHub webhook route and normalize headers. |
| app/main.py | Switches to the new route registrar and strengthens _safe_next_path validation. |
| app/ledger/service.py | Removes explicit CursorResult casts for update calls that rely on rowcount. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+26
to
+28
| "X-GitHub-Delivery": "delivery-route-ignored", | ||
| "X-GitHub-Event": "issues", | ||
| "X-Hub-Signature-256": _signature("secret", body), |
| webhook_secret, | ||
| accepted_labelers, | ||
| ) | ||
| return JSONResponse(result, status_code=github_webhook_status_code(result)) |
Comment on lines
280
to
294
| def _safe_next_path(next_path: str | None) -> str: | ||
| decoded_next_path = unquote(next_path) if next_path else "" | ||
| if ( | ||
| not next_path | ||
| or not next_path.startswith("/") | ||
| or next_path.startswith("//") | ||
| or len(next_path) > 2048 | ||
| or "\\" in next_path | ||
| or decoded_next_path.startswith("//") | ||
| or "\\" in decoded_next_path | ||
| or any(ord(char) < 32 or 127 <= ord(char) < 160 for char in next_path) | ||
| or any(ord(char) < 32 or 127 <= ord(char) < 160 for char in decoded_next_path) | ||
| ): | ||
| return "/me" | ||
| return next_path |
Comment on lines
+659
to
670
| claimed = session.execute( | ||
| update(Bounty) | ||
| .where(Bounty.id == bounty.id, Bounty.awards_paid < Bounty.max_awards) | ||
| .values( | ||
| awards_paid=Bounty.awards_paid + 1, | ||
| status=case( | ||
| (Bounty.awards_paid + 1 >= Bounty.max_awards, "paid"), | ||
| else_="open", | ||
| ), | ||
| ) | ||
| ) | ||
| if claimed.rowcount != 1: |
Contributor
Author
|
Superseded by the cleaned, scoped PR #368. |
This was referenced May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #320
Summary
/webhooks/githubFastAPI route wiring intoapp/webhooks/routes.py.handle_github_webhookbehavior.mypy appand by sanitizing percent-decoded OAuthnextpaths covered by the existing OAuth regression test.Complexity reduced
app/main.pyno longer owns the GitHub webhook HTTP adapter details. The webhook domain handler stays inapp/webhooks/github.py, while request header shaping, FastAPI route registration, and HTTP status response mapping live in a small webhook routes module with direct tests.Tests
python -m pytest -q-> 337 passedpython -m pytest tests/test_webhook_routes.py tests/test_webhooks.py::test_github_signature_verification tests/test_webhooks.py::test_accepted_label_pays_bounty_once tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 5 passedpython -m ruff format --check app/main.py app/ledger/service.py app/webhooks/routes.py tests/test_webhook_routes.pypython -m ruff check app/main.py app/ledger/service.py app/webhooks/routes.py tests/test_webhook_routes.pypython -m mypy appSummary by CodeRabbit
Bug Fixes
Refactor
Tests