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.
2 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