Extract GitHub webhook route from app.main#368
Conversation
|
Warning Review limit reached
More reviews will be available in 3 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ 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 refactors the GitHub webhook endpoint into a dedicated routes module, adds tests for webhook behavior, and hardens OAuth redirect “next” path validation against encoded bypasses.
Changes:
- Extracted GitHub webhook route registration + header normalization into
app/webhooks/routes.py. - Updated
create_appto use the new route registration helper. - Added tests covering success/ignored behavior and bad-signature unauthorized responses.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_webhook_routes.py | Adds integration tests for the GitHub webhook endpoint responses and persistence. |
| app/webhooks/routes.py | Introduces a reusable GitHub webhook route registrar + header normalization utilities. |
| app/main.py | Switches to the new webhook route registrar and tightens _safe_next_path validation using URL-decoding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "X-GitHub-Delivery": "delivery-route-ignored", | ||
| "X-GitHub-Event": "issues", | ||
| "X-Hub-Signature-256": _signature("secret", body), |
| 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 |
prettyboyvic
left a comment
prettyboyvic
left a comment
There was a problem hiding this comment.
No blockers from my review.
Evidence checked:
- Inspected the diff from
origin/mainto1073cc4:app/main.py,app/webhooks/routes.py, andtests/test_webhook_routes.py. - Verified
/webhooks/githubis still registered fromcreate_app, reads the raw request body, preserves the existinghandle_github_webhook()domain handler, and keeps the sameunauthorized-> HTTP 401 mapping. - Checked
github_webhook_headers()with mixed/lowercase header input; it returns the canonical keys expected byhandle_github_webhook(). - Checked
_safe_next_path()rejects protocol-relative paths plus encoded slash/backslash ambiguity while preserving normal in-app paths like/bounties?status=open. - Confirmed the cleaned successor PR no longer includes the
app/ledger/service.pyCursorResulttyping removal that made superseded PR #367 failmypy app.
Validation run on PR head 1073cc4:
uv run --extra dev python -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 passed.uv run --extra dev ruff check app/main.py app/webhooks/routes.py tests/test_webhook_routes.py-> passed.uv run --extra dev ruff format --check app/main.py app/webhooks/routes.py tests/test_webhook_routes.py-> 3 files already formatted.uv run --extra dev python -m mypy app-> success.uv run --extra dev python -m pytest -q-> 337 passed.
No secrets, wallet private keys, payout credentials, private vulnerability details, deployment values, or MRWK price claims were reviewed or disclosed.
CharlieLZ
left a comment
CharlieLZ
left a comment
There was a problem hiding this comment.
No blockers found on current head 1073cc4.
Evidence summary:
- Inspected
app/webhooks/routes.py,app/main.py, andtests/test_webhook_routes.py. - Verified the extracted
/webhooks/githubroute still reads the raw body before dispatch, maps request headers back to the canonical names expected byhandle_github_webhook, preserves the 401 response for failed signatures, and passessettings.github_accepted_labelersthrough to the webhook handler. - Verified the route extraction keeps signature verification in
handle_github_webhookbefore payload parsing or event recording. - Checked the adjacent
_safe_next_pathdecoded-path hardening against existing OAuth redirect ambiguity coverage.
Validation:
/Users/charliesimmon/tmp/bounty-work/mergework/.venv/bin/python -m pytest tests/test_webhook_routes.py tests/test_webhooks.py tests/test_security.py tests/test_wallet_api.py -q-> 103 passed./Users/charliesimmon/tmp/bounty-work/mergework/.venv/bin/python -m ruff check app/main.py app/webhooks/routes.py tests/test_webhook_routes.py-> passed./Users/charliesimmon/tmp/bounty-work/mergework/.venv/bin/python -m ruff format --check app/main.py app/webhooks/routes.py tests/test_webhook_routes.py-> 3 files already formatted./Users/charliesimmon/tmp/bounty-work/mergework/.venv/bin/python -m mypy app/main.py app/webhooks/routes.py app/webhooks/github.py-> success.git diff --check origin/main...HEAD-> clean.- Hosted checks are green: CodeRabbit pass and Quality/readiness/docs/image checks pass.
newmattock
left a comment
newmattock
left a comment
There was a problem hiding this comment.
No blockers found on current head 1073cc4.
Evidence checked:
- Inspected
app/webhooks/routes.py, the route registration call inapp/main.py,tests/test_webhook_routes.py, and the existing domain handler inapp/webhooks/github.py. - Verified
/webhooks/githubstill reads the raw request body before dispatch, normalizes GitHub delivery/event/signature headers back to the canonical names expected byhandle_github_webhook(), and passes through the configured database URL, webhook secret, and accepted labeler tuple. - Verified route status mapping preserves
unauthorizedas HTTP 401 while other webhook handler results return HTTP 200. - Rechecked the branch's decoded OAuth
nexthardening via the existing wallet regression.
Validation run locally:
uv run --extra dev python -m pytest tests/test_webhook_routes.py tests/test_webhooks.py::test_accepted_issue_event_for_pull_request_does_not_pay_matching_bounty tests/test_webhooks.py::test_webhook_rejects_bad_signature tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 5 passed.uv run --extra dev python -m mypy app/webhooks/routes.py app/main.py-> success.uv run --extra dev python -m ruff check app/webhooks/routes.py app/main.py tests/test_webhook_routes.py-> passed.uv run --extra dev python -m ruff format --check app/webhooks/routes.py app/main.py tests/test_webhook_routes.py-> 3 files already formatted.git diff --check origin/main...HEAD-> clean.
|
Maintainer note after #320 filled: this PR still references closed bounty #320. If you want it considered for the current code-health round, rebase against main, update the bounty reference to #377, and keep the scope distinct from already accepted #328, #337, #364, #375, and #376. This is not accepted or paid as-is. |
Refs #320
Summary
/webhooks/githubroute registration, GitHub header normalization, and webhook status-code mapping intoapp.webhooks.routes.app.mainresponsible only for app wiring by registering the webhook route with database URL, webhook secret, and accepted labeler settings.Complexity reduced
app.mainno longer owns GitHub webhook request-body/header handling or HTTP response mapping. Webhook transport wiring now lives beside the webhook domain handler, making webhook behavior easier to review independently from page, MCP, ledger, and admin routes.Tests
uv run --extra dev python -m pytest tests/test_webhook_routes.py tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 3 passed.uv run --extra dev python -m pytest -q-> 337 passed.uv run --extra dev python -m mypy app-> passed.uv run --extra dev ruff check app/main.py app/webhooks/routes.py tests/test_webhook_routes.py-> passed.uv run --extra dev ruff format --check app/main.py app/webhooks/routes.py tests/test_webhook_routes.py-> passed.uv run --extra dev python scripts/docs_smoke.py-> docs smoke ok.git diff --check HEAD~1..HEAD-> clean.No private keys, seed material, secrets, deployment credentials, private vulnerability details, payout credentials, or MRWK price claims are included.