Harden OAuth redirects and MCP work-proof guidance#355
Conversation
…nce' into codex/bounty-315-repo-scoped-guidance
|
Warning Review limit reached
More reviews will be available in 1 minute and 24 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 (1)
📝 WalkthroughWalkthroughThe PR hardens OAuth redirect validation by checking both raw and URL-decoded next_path, blocking traversal and header-injection patterns. It also adds optional ChangesOAuth Redirect Validation Hardening
MCP submit_work_proof Repo Scoping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_security.py (1)
851-874: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd one route-level test for doubly-encoded
nextpayloads.These cases only hit
_safe_next_path()directly. Inauth_github_login, FastAPI has already decoded one layer of the query string, so the newunquote()logic is really exercised by inputs like/%255cevil.example/meor/me%250d%250aLocation:%20https://evil.example. Please add oneTestClientassertion that/auth/github/loginstill stores/mefor those payloads.As per coding guidelines, "
tests/**/*.py: Add or update tests for changed behavior".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_security.py` around lines 851 - 874, Add a route-level TestClient test that exercises auth_github_login (in addition to the unit tests for _safe_next_path) by issuing a GET to /auth/github/login with a doubly-encoded next value (e.g. "/%255cevil.example/me" and "/me%250d%250aLocation:%20https://evil.example") and assert the app stores/returns "/me" as the next path; place the new test alongside test_oauth_next_path_rejects_external_or_headerlike_paths and use TestClient to call auth_github_login and inspect the session/cookie or response where the next value is recorded to confirm it is normalized to "/me".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/test_security.py`:
- Around line 851-874: Add a route-level TestClient test that exercises
auth_github_login (in addition to the unit tests for _safe_next_path) by issuing
a GET to /auth/github/login with a doubly-encoded next value (e.g.
"/%255cevil.example/me" and "/me%250d%250aLocation:%20https://evil.example") and
assert the app stores/returns "/me" as the next path; place the new test
alongside test_oauth_next_path_rejects_external_or_headerlike_paths and use
TestClient to call auth_github_login and inspect the session/cookie or response
where the next value is recorded to confirm it is normalized to "/me".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 32ca0a37-97ca-4edf-add3-1327ce190100
📒 Files selected for processing (4)
app/main.pyapp/mcp.pytests/test_api_mcp.pytests/test_security.py
s0584273828-ctrl
left a comment
s0584273828-ctrl
left a comment
There was a problem hiding this comment.
Reviewed PR #355 for the MRWK review bounty.
No blockers found on current head dc08c47078ec8b18cf7497b652b32562d21b0a52.
Evidence checked:
app/main.py:_safe_next_path()now rejects percent-decoded protocol-relative redirects, decoded backslashes, and decoded control/header injection payloads before returning the original local path.app/main.py:submit_work_proofnow accepts optionalrepoonly withissue_number, normalizes it to lowercase, and keeps the existing ambiguity guard when multiple bounties match an unscoped issue number.app/mcp.py:submit_work_prooftool description now advertises the repo-scoped issue lookup behavior.tests/test_api_mcp.py: coverage proves repo scoping selects the intended bounty when issue numbers overlap across repos, rejects repo withoutissue_number, rejects repo withbounty_id, rejects non-string repo, and caps repo length.tests/test_security.py: coverage includes encoded//, encoded backslash, and encoded CRLF OAuthnextpayloads.
Validation run locally:
uv run --extra dev python -m pytest tests/test_api_mcp.py tests/test_security.py -q-> 137 passeduv run --extra dev python -m pytest -q-> 343 passeduv run --extra dev python -m mypy app-> no issues in 14 source filesuv run --extra dev ruff format --check .-> 48 files already formatteduv run --extra dev ruff check .-> all checks passedgit diff --check origin/main...HEAD-> clean
I do not see a release blocker.
newmattock
left a comment
newmattock
left a comment
There was a problem hiding this comment.
Focused follow-up review on current head 40a7904: no blockers found.
Evidence checked:
- Inspected
app/main.py,app/mcp.py,tests/test_api_mcp.py, andtests/test_security.py. - Verified the repo-scoped
submit_work_proofpath only permitsrepowithissue_number, rejects invalid selector combinations, handles legacy mixed-case repository rows, and keeps the ambiguous-selector guard for unscoped issue numbers. - Verified the encoded OAuth
nexthardening is covered at the route level, not just by direct_safe_next_path()calls:/auth/github/login?next=/%255cevil.example/meand the double-encoded CRLFLocation:payload both store/mein the signed OAuth state. - Confirmed the ledger rowcount typing paths still type-check.
Validation:
uv run --extra dev python -m pytest tests/test_security.py::test_github_login_stores_safe_default_for_encoded_next_route tests/test_api_mcp.py::test_mcp_submit_work_proof_scopes_issue_number_by_repo tests/test_api_mcp.py::test_mcp_submit_work_proof_scopes_legacy_mixed_case_repo tests/test_api_mcp.py::test_mcp_submit_work_proof_rejects_invalid_bounty_selectors -q-> 15 passed.uv run --extra dev python -m mypy app/main.py app/mcp.py app/ledger/service.py-> success.uv run --extra dev python -m ruff check app/main.py app/mcp.py app/ledger/service.py tests/test_api_mcp.py tests/test_security.py-> passed.uv run --extra dev python -m ruff format --check app/main.py app/mcp.py app/ledger/service.py tests/test_api_mcp.py tests/test_security.py-> 5 files already formatted.
|
Closing this stale bounty submission. It still references a closed/filled bounty round and is dirty or unstable against current main after the route/test extraction batches. A future attempt needs a fresh PR against current main, a currently open bounty reference, and scope distinct from work already accepted. |
4 participants
Summary
nextpath validation against percent-encoded redirect/header injection payloadsRefs #315
Tests
python -m pytest -qpython -m ruff format --check .python -m ruff check .uv run --python 3.12 mypy appSummary by CodeRabbit
Security
New Features
Tests