Skip to content

fix(auth): reject repeated OAuth query params (#820) #1184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yanyishuai wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(auth): reject repeated OAuth query params (#820) #1184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion app/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

from app.config import Settings
from app.control_chars import contains_control_character
from app.query_validation import reject_control_char_query_param, reject_repeated_query_param


def oauth_configured(settings: Settings) -> bool:
Expand Down Expand Up @@ -109,7 +110,11 @@ def register_auth_routes(app: FastAPI, *, settings: Settings) -> AuthService:
auth = AuthService(settings)

@app.get("/auth/github/login")
def auth_github_login(next_path: str | None = Query(None, alias="next")) -> RedirectResponse:
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")
Comment on lines +116 to 119

@coderabbitai coderabbitai Bot Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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

safe_next = safe_next_path(next_path)
Expand All @@ -133,6 +138,9 @@ def auth_github_login(next_path: str | None = Query(None, alias="next")) -> Redi

@app.get("/auth/github/callback")
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")
cookie_state = request.cookies.get("mrwk_oauth_state")
Expand Down
41 changes: 41 additions & 0 deletions tests/test_wallet_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,47 @@ def test_github_login_stores_safe_default_for_backslash_next(sqlite_url: str, mo
assert next_path == "/me"


def test_github_login_rejects_repeated_next(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/login?next=/me&next=/admin", follow_redirects=False)

assert response.status_code == 400
assert response.json()["detail"] == "next 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&code=def&state=xyz",
follow_redirects=False,
)

assert response.status_code == 400
assert response.json()["detail"] == "code must be provided at most once"
Comment on lines +672 to +685

@coderabbitai coderabbitai Bot Jun 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
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



def test_github_oauth_callback_route_is_registered(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", follow_redirects=False)

assert response.status_code == 422


def test_wallet_pages_expose_transfer_and_github_claim_flows(sqlite_url: str) -> None:
create_schema(sqlite_url)
_, public_hex, address = _keypair()
Expand Down
Loading