fix(auth): reject repeated OAuth query params (#820) - #1184
Conversation
📝 WalkthroughWalkthrough
OAuth Query Parameter Validation
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 92a31c7c-da72-43ff-935d-32b487b77d62
📒 Files selected for processing (2)
app/auth.pytests/test_wallet_api.py
| reject_repeated_query_param(request, "next") | ||
| reject_control_char_query_param(request, "next") | ||
| if not oauth_configured(settings): | ||
| raise HTTPException(status_code=503, detail="GitHub OAuth is not configured") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the existing 503 behavior when OAuth is disabled.
These new guards run before oauth_configured(settings), so repeated/control-character requests now fail with 400 instead of the previous 503 when GitHub OAuth is not configured. That breaks the unconfigured-path contract called out in the PR objectives.
Proposed fix
def auth_github_login(
request: Request, next_path: str | None = Query(None, alias="next")
) -> RedirectResponse:
- reject_repeated_query_param(request, "next")
- reject_control_char_query_param(request, "next")
if not oauth_configured(settings):
raise HTTPException(status_code=503, detail="GitHub OAuth is not configured")
+ reject_repeated_query_param(request, "next")
+ reject_control_char_query_param(request, "next")
safe_next = safe_next_path(next_path) async def auth_github_callback(request: Request, code: str, state: str) -> RedirectResponse:
- for name in ("code", "state"):
- reject_repeated_query_param(request, name)
- reject_control_char_query_param(request, name)
if not oauth_configured(settings):
raise HTTPException(status_code=503, detail="GitHub OAuth is not configured")
+ for name in ("code", "state"):
+ reject_repeated_query_param(request, name)
+ reject_control_char_query_param(request, name)
cookie_state = request.cookies.get("mrwk_oauth_state")Also applies to: 141-145
| def test_github_callback_rejects_repeated_state(sqlite_url: str, monkeypatch) -> None: | ||
| monkeypatch.setenv("MERGEWORK_GITHUB_OAUTH_CLIENT_ID", "client-id") | ||
| monkeypatch.setenv("MERGEWORK_GITHUB_OAUTH_CLIENT_SECRET", "client-secret") | ||
| monkeypatch.setenv("MERGEWORK_COOKIE_SECRET", "test-cookie-secret") | ||
| monkeypatch.setenv("MERGEWORK_PUBLIC_BASE_URL", "https://mrwk.example.test") | ||
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | ||
|
|
||
| response = client.get( | ||
| "/auth/github/callback?code=abc&code=def&state=xyz", | ||
| follow_redirects=False, | ||
| ) | ||
|
|
||
| assert response.status_code == 400 | ||
| assert response.json()["detail"] == "code must be provided at most once" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
This test never exercises repeated state.
The function name says state, but the request repeats code, so the new state rejection path in app/auth.py is still unproven.
Proposed fix
def test_github_callback_rejects_repeated_state(sqlite_url: str, monkeypatch) -> None:
@@
response = client.get(
- "/auth/github/callback?code=abc&code=def&state=xyz",
+ "/auth/github/callback?code=abc&state=xyz&state=uvw",
follow_redirects=False,
)
@@
- assert response.json()["detail"] == "code must be provided at most once"
+ assert response.json()["detail"] == "state must be provided at most once"As per coding guidelines, "Add or update tests for changed behavior"; as per path instructions, "Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_github_callback_rejects_repeated_state(sqlite_url: str, monkeypatch) -> None: | |
| monkeypatch.setenv("MERGEWORK_GITHUB_OAUTH_CLIENT_ID", "client-id") | |
| monkeypatch.setenv("MERGEWORK_GITHUB_OAUTH_CLIENT_SECRET", "client-secret") | |
| monkeypatch.setenv("MERGEWORK_COOKIE_SECRET", "test-cookie-secret") | |
| monkeypatch.setenv("MERGEWORK_PUBLIC_BASE_URL", "https://mrwk.example.test") | |
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | |
| response = client.get( | |
| "/auth/github/callback?code=abc&code=def&state=xyz", | |
| follow_redirects=False, | |
| ) | |
| assert response.status_code == 400 | |
| assert response.json()["detail"] == "code must be provided at most once" | |
| def test_github_callback_rejects_repeated_state(sqlite_url: str, monkeypatch) -> None: | |
| monkeypatch.setenv("MERGEWORK_GITHUB_OAUTH_CLIENT_ID", "client-id") | |
| monkeypatch.setenv("MERGEWORK_GITHUB_OAUTH_CLIENT_SECRET", "client-secret") | |
| monkeypatch.setenv("MERGEWORK_COOKIE_SECRET", "test-cookie-secret") | |
| monkeypatch.setenv("MERGEWORK_PUBLIC_BASE_URL", "https://mrwk.example.test") | |
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | |
| response = client.get( | |
| "/auth/github/callback?code=abc&state=xyz&state=uvw", | |
| follow_redirects=False, | |
| ) | |
| assert response.status_code == 400 | |
| assert response.json()["detail"] == "state must be provided at most once" |
Sources: Coding guidelines, Path instructions
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed current head 4f75bef78527c909fd6ceb0e2c3bfb832cf4633a. The auth route changes apply the existing raw-query validators before OAuth redirect or callback processing, so repeated next, code, and state values are rejected before FastAPI scalar coercion or token exchange can silently choose one value. The single-value safety path is preserved through safe_next_path, the unconfigured-OAuth behavior remains behind the existing 503 guard, and the added wallet/auth tests cover duplicate next, duplicate callback query handling, and callback route registration. Hosted quality checks are passing. I did not find a blocking issue.
|
External bounty ready for maintainer review — acceptance criteria documented in PR body. Happy to address feedback immediately. |
piaigmt
left a comment
piaigmt
left a comment
There was a problem hiding this comment.
APPROVE — third-party PR #1184 (fix(auth): reject repeated OAuth query params (refs #820), author LarryHu0217, not piaigmt — no self-review).
Reviewed current head 4f75bef78527c909fd6ceb0e2c3bfb832cf4633a locally (Python 3.12.3, editable dev install).
Scope: 2 files (app/auth.py, tests/test_wallet_api.py), +50/-1. The change adds reject_repeated_query_param + reject_control_char_query_param guards to the two GitHub OAuth routes (/auth/github/login on next; /auth/github/callback on code and state), reusing the already-vetted app/query_validation.py helpers rather than introducing new validation logic.
Correctness / non-tautology check: reject_repeated_query_param inspects the raw multi-value query list (_query_param_values) before FastAPI collapses duplicates to a single scalar, so the 400 genuinely fires on ambiguous ?next=/me&next=/admin and ?code=abc&code=def inputs — verified via source trace, not just the test. Request is already imported in app/auth.py; helpers exist at app/query_validation.py:27,55.
Local gates (all pass):
pytest tests/test_wallet_api.py -k "repeated_next or repeated_state or callback_route_is_registered"→ 3 passedpytest tests/test_wallet_api.py(full suite, regression) → 48 passedruff check app/auth.py tests/test_wallet_api.py→ cleanruff format --check→ 2 files already formatted- CI on head: CodeRabbit pass, Quality/readiness/docs/image checks pass; PR MERGEABLE.
The three new tests correctly cover: repeated next → 400, repeated code → 400, and callback route registration (missing required params → 422).
Payout boundary: this is a submitted #1009 review-bounty claim only. It is not earned, accepted, pending, or withdrawable unless maintainers accept it and record proof-backed payment.
|
@qingfeng312 Follow-up on #820 — OAuth repeated/control query guards remain open on Ready for re-review/merge when bandwidth allows. Wallet: |
|
@qingfeng312 @piaigmt Follow-up on #820 — still dual-APPROVED + CI success + mergeable=clean on Merge-ready whenever maintainers have bandwidth. Wallet: |
|
Tier-S merge ready on 4f75bef — dual APPROVED + CI green + mergeable=clean. Please merge when convenient. |
|
Re-review request on 4f75bef. CI green / mergeable=clean. Please re-approve when convenient. |
|
Merge ping — CI green / mergeable=clean on 4f75bef (OAuth query params). Please merge when convenient — payout blocked on merge. |
3 participants
Summary
Harden GitHub OAuth login and callback routes against repeated scalar query
parameters (
next,code,state) using the existingreject_repeated_query_paramandreject_control_char_query_paramhelpers.Closes #820.
Changes
app/auth.py/auth/github/login: validatenextbefore redirect./auth/github/callback: validatecodeandstatebefore token exchange.tests/test_wallet_api.pynexton login (400).codeon callback (400)./auth/github/callbackis registered (422 without params, not 404).Why
FastAPI silently accepts the last value when a scalar query param is repeated.
For OAuth flows that is ambiguous and can mask proxy/cache bugs. Matching the
wallet API query validation pattern keeps behavior consistent and fail-fast.
Test plan
Wallet
Do4v7foHJvRJLpRRoGaVPWX6DDEjX3yTK7J91gpwUQpECloses #820
Summary by CodeRabbit
Bug Fixes
Tests