Extract auth helpers from app.main#353
Conversation
|
Warning Review limit reached
More reviews will be available in 2 minutes and 45 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 (2)
📝 WalkthroughWalkthroughExtracts auth logic into app/auth.py: HMAC-signed values, CSRF tokens, safe redirect sanitizer, GitHub OAuth flow, session cookie handling, and route registration. app/main.py delegates auth helpers and route registration; tests exercise signing and redirect safety; minor ledger import cleanup. ChangesAuth Module Extraction and Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 |
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 extracts GitHub OAuth + session/cookie helpers into a dedicated app/auth.py module and adds tests to harden redirect handling (next parameter) against encoded open-redirect and header-injection patterns.
Changes:
- Added
app/auth.pywith OAuth routes registration, cookie/session helpers, CSRF helpers, and a strictersafe_next_path. - Refactored
app/main.pyto delegate auth routes and auth request parsing toapp.auth. - Added unit/integration-style tests for signing/verification and
safe_next_pathbehavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/test_auth.py | Adds tests validating cookie signing and hardened redirect sanitization. |
| app/main.py | Removes inline auth helpers/routes and wires the app to app.auth helpers instead. |
| app/auth.py | Introduces centralized auth logic including redirect sanitization and OAuth route registration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async with httpx.AsyncClient(timeout=10) as client: | ||
| token_response = await client.post( | ||
| "https://github.com/login/oauth/access_token", | ||
| headers={"Accept": "application/json"}, | ||
| data={ | ||
| "client_id": settings.github_oauth_client_id, | ||
| "client_secret": settings.github_oauth_client_secret, | ||
| "code": code, | ||
| "redirect_uri": f"{settings.public_base_url}/auth/github/callback", | ||
| }, | ||
| ) | ||
| token_response.raise_for_status() | ||
| access_token = token_response.json().get("access_token") | ||
| if not access_token: | ||
| raise HTTPException(status_code=401, detail="GitHub OAuth token exchange failed") | ||
| user_response = await client.get( | ||
| "https://api.github.com/user", | ||
| headers={ | ||
| "Accept": "application/vnd.github+json", | ||
| "Authorization": f"Bearer {access_token}", | ||
| }, | ||
| ) | ||
| user_response.raise_for_status() | ||
| login = str(user_response.json().get("login", "")).lower() | ||
| if not login: | ||
| raise HTTPException(status_code=401, detail="GitHub OAuth user lookup failed") |
| return RedirectResponse("/auth/github/login?next=/admin", status_code=302) | ||
|
|
||
| @app.get("/admin/callback") | ||
| async def admin_callback(request: Request) -> RedirectResponse: |
| ("/me", "/me"), | ||
| ("/bounties?status=open", "/bounties?status=open"), | ||
| ("/%2f%2fevil.example/me", "/me"), | ||
| ("/%5cevil.example/me", "/me"), |
| 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 |
TateLyman
left a comment
TateLyman
left a comment
There was a problem hiding this comment.
Blocking issue: current head fails the required type check because the branch removes the CursorResult[Any] casts/import from app/ledger/service.py while pay_bounty() and close_bounty() still read claimed.rowcount after session.execute(update(...)). SQLAlchemy typing infers Result[Any], so mypy rejects the attribute.\n\nEvidence:\n- Hosted Quality, readiness, docs, and image checks is failing in type check.\n- Local repro: uv run --extra dev python -m mypy app/auth.py app/ledger/service.py app/main.py -> app/ledger/service.py:670: error: "Result[Any]" has no attribute "rowcount" and app/ledger/service.py:741: error: "Result[Any]" has no attribute "rowcount".\n- Focused auth/security/wallet validation otherwise passes: uv run --extra dev python -m pytest tests/test_auth.py tests/test_security.py tests/test_wallet_api.py -q -> 90 passed.\n\nThe auth extraction may still be reviewable after that is fixed, but the ledger typing cleanup is unrelated to the auth boundary and should either be reverted or typed explicitly as a cursor result before merge.
![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
🤖 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.
Inline comments:
In `@app/auth.py`:
- Around line 130-156: Wrap the GitHub token exchange and user lookup HTTP calls
in try/except to catch httpx.RequestError and httpx.HTTPStatusError so
network/timeouts and non-2xx responses are handled and translated into
appropriate HTTPException responses; specifically, surround the
httpx.AsyncClient POST to "https://github.com/login/oauth/access_token" (where
token_response.raise_for_status() and access_token are used) and the GET to
"https://api.github.com/user" (where user_response.raise_for_status() and login
are used) with exception handling that returns a clear HTTPException (e.g., 401
for auth failures or 502/503 for upstream/network errors) and include brief
error context in the detail message.
In `@tests/test_auth.py`:
- Around line 12-16: The current test covers secret mismatch but not age-based
expiry; add a new test (e.g., test_verified_value_rejects_expired_tokens) that
uses signed_value("github:alice", "test-cookie-secret"), asserts
verified_value(token, "test-cookie-secret", 60) returns "github:alice", then
asserts verified_value(token, "test-cookie-secret", max_age=0) is None to
validate timestamp/age rejection; alternatively, if you prefer real-time expiry,
use time.sleep to exceed a very short max_age, but prefer the max_age=0 approach
for a deterministic unit test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e10c6ebe-9c0a-493f-8dc0-4a062f3d9525
📒 Files selected for processing (3)
app/auth.pyapp/main.pytests/test_auth.py
|
Follow-up pushed in What changed:
Validation:
|
TateLyman
left a comment
TateLyman
left a comment
There was a problem hiding this comment.
Rechecked current head 4427ccd after the auth follow-up fixes. The earlier app/ledger/service.py rowcount type-check blocker is gone, and the branch now also wraps GitHub OAuth upstream failures with explicit HTTPException responses plus expiry coverage.
Validation:
uv run --extra dev python -m pytest tests/test_auth.py tests/test_security.py tests/test_wallet_api.py -q-> 91 passed.uv run --extra dev python -m mypy app/auth.py app/ledger/service.py app/main.py-> success.uv run --extra dev ruff check app/auth.py app/ledger/service.py app/main.py tests/test_auth.py-> passed.uv run --extra dev ruff format --check app/auth.py app/ledger/service.py app/main.py tests/test_auth.py-> passed.git diff --check origin/main...HEAD-> clean.- Hosted quality check is green and GitHub reports the branch mergeable.
No remaining blocker found from my earlier review.
|
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
Complexity reduced
Tests
Summary by CodeRabbit
New Features
Tests
Refactor