Extract auth session helpers from app main#338
Conversation
📝 WalkthroughWalkthroughA new ChangesAuthentication module extraction and integration
Sequence DiagramsequenceDiagram
participant Client
participant AuthGithubLogin as /auth/github/login
participant AuthGithubCallback as /auth/github/callback
participant AuthContext
participant Response
Client->>AuthGithubLogin: POST with next path
AuthGithubLogin->>AuthContext: oauth_configured()
AuthGithubLogin->>AuthContext: signed_value(random state)
AuthGithubLogin->>AuthContext: set_oauth_state_cookie(state)
AuthGithubLogin->>Response: Redirect to GitHub OAuth
Client->>AuthGithubCallback: GET code + state from GitHub
AuthGithubCallback->>AuthContext: verify_oauth_state(cookie, state)
AuthContext->>AuthContext: Verify signature and expiry
AuthContext->>Response: Return sanitized next_path
AuthGithubCallback->>AuthContext: set_login_cookies(login)
AuthGithubCallback->>Response: Redirect to next_path
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
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 167-184: In set_login_cookies, normalize the provided login to
lowercase before checking against settings.admin_logins so case differences
don't prevent setting the ADMIN_COOKIE_NAME; use the lowercased value for the
comparison (but continue to sign/store the original or decide consistently) and
ensure the check against settings.admin_logins uses the same normalization as
wherever admin logins are stored.
In `@tests/test_auth.py`:
- Around line 82-87: Add a negative assertion to
test_csrf_uses_same_signed_cookie_boundary to ensure action mismatch is
rejected: after creating auth = AuthContext(_settings()) and token =
auth.csrf_token("admin-bounty", "alice"), call auth.verify_csrf_token(token,
action="other-action", login="alice") and assert that it returns False; this
mirrors the existing login-mismatch check and verifies the action boundary of
csrf_token/verify_csrf_token.
🪄 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: d4d299b2-7b67-4ad4-b7bf-308b6314c80a
📒 Files selected for processing (6)
app/auth.pyapp/main.pytests/test_auth.pytests/test_bounty_attempts.pytests/test_security.pytests/test_wallet_api.py
| def set_login_cookies(self, response: Response, login: str) -> None: | ||
| response.set_cookie( | ||
| USER_COOKIE_NAME, | ||
| self.signed_value(login), | ||
| httponly=True, | ||
| secure=True, | ||
| samesite="lax", | ||
| max_age=USER_COOKIE_MAX_AGE_SECONDS, | ||
| ) | ||
| if login in self.settings.admin_logins: | ||
| response.set_cookie( | ||
| ADMIN_COOKIE_NAME, | ||
| self.signed_value(login), | ||
| httponly=True, | ||
| secure=True, | ||
| samesite="lax", | ||
| max_age=ADMIN_COOKIE_MAX_AGE_SECONDS, | ||
| ) |
There was a problem hiding this comment.
Case mismatch may prevent admin cookie from being set.
Line 176 compares login directly against admin_logins, but GitHub usernames may arrive in original case (e.g., "Alice") while admin_logins is expected to contain lowercase values. If the OAuth callback passes the login without lowercasing, admin users won't receive the admin cookie.
Normalize login to lowercase before the comparison:
🐛 Proposed fix
def set_login_cookies(self, response: Response, login: str) -> None:
response.set_cookie(
USER_COOKIE_NAME,
self.signed_value(login),
httponly=True,
secure=True,
samesite="lax",
max_age=USER_COOKIE_MAX_AGE_SECONDS,
)
- if login in self.settings.admin_logins:
+ if login.lower() in self.settings.admin_logins:
response.set_cookie(
ADMIN_COOKIE_NAME,
self.signed_value(login),
httponly=True,
secure=True,
samesite="lax",
max_age=ADMIN_COOKIE_MAX_AGE_SECONDS,
)🤖 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 `@app/auth.py` around lines 167 - 184, In set_login_cookies, normalize the
provided login to lowercase before checking against settings.admin_logins so
case differences don't prevent setting the ADMIN_COOKIE_NAME; use the lowercased
value for the comparison (but continue to sign/store the original or decide
consistently) and ensure the check against settings.admin_logins uses the same
normalization as wherever admin logins are stored.
| def test_csrf_uses_same_signed_cookie_boundary() -> None: | ||
| auth = AuthContext(_settings()) | ||
| token = auth.csrf_token("admin-bounty", "alice") | ||
|
|
||
| assert auth.verify_csrf_token(token, action="admin-bounty", login="alice") | ||
| assert not auth.verify_csrf_token(token, action="admin-bounty", login="bob") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding negative test case for action mismatch.
The test verifies login mismatch rejection but doesn't test action mismatch. Adding a test like assert not auth.verify_csrf_token(token, action="other-action", login="alice") would strengthen CSRF boundary validation coverage.
💡 Suggested addition
assert auth.verify_csrf_token(token, action="admin-bounty", login="alice")
assert not auth.verify_csrf_token(token, action="admin-bounty", login="bob")
+ assert not auth.verify_csrf_token(token, action="other-action", login="alice")🤖 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_auth.py` around lines 82 - 87, Add a negative assertion to
test_csrf_uses_same_signed_cookie_boundary to ensure action mismatch is
rejected: after creating auth = AuthContext(_settings()) and token =
auth.csrf_token("admin-bounty", "alice"), call auth.verify_csrf_token(token,
action="other-action", login="alice") and assert that it returns False; this
mirrors the existing login-mismatch check and verifies the action boundary of
csrf_token/verify_csrf_token.
|
Maintainer update: holding this with mrwk:needs-info. Please address the auth review items before merge: normalize the admin-login comparison consistently in set_login_cookies, and add a CSRF action-mismatch regression. No PR bounty payout is recorded while it is held. |
TateLyman
left a comment
TateLyman
left a comment
There was a problem hiding this comment.
Reviewed the auth/session extraction for #320.
What I checked:
app/auth.py: signed cookie helpers, OAuth state verification, safe next-path handling, GitHub/admin session lookup, admin-token-only dependency, login cookie setting, and session clearing.app/main.py: route dependencies now callAuthContextmethods without changing the existing boundaries for bounty attempts, wallet linking/claims, admin webhook/payout APIs, OAuth callbacks, and admin form CSRF.tests/test_auth.py,tests/test_security.py,tests/test_wallet_api.py, andtests/test_bounty_attempts.py: regression coverage moved from privateapp.mainhelpers to public auth helpers and keeps cookie auth separate from admin-token API auth.
Validation run locally:
uv run --extra dev python -m pytest tests/test_auth.py tests/test_security.py::test_admin_bounty_form_requires_csrf_for_cookie_auth tests/test_security.py::test_admin_bounty_api_requires_admin_token_not_cookie_auth tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 8 passed.uv run --extra dev python -m pytest tests/test_security.py tests/test_wallet_api.py tests/test_bounty_attempts.py -q-> 86 passed.uv run --extra dev ruff check app tests-> passed.uv run --extra dev ruff format --check app tests-> passed.uv run --extra dev mypy app-> passed.uv run --extra dev python scripts/docs_smoke.py-> docs smoke ok.git diff --check origin/main...HEAD-> clean.
I did not find a behavioral blocker. GitHub currently reports the PR merge state as CLEAN, and the hosted quality check is green.
artylobos
left a comment
artylobos
left a comment
There was a problem hiding this comment.
Reviewed the auth extraction on head 079c38b. I would keep this held until two small auth-boundary items are addressed:
app/auth.py:176:set_login_cookies()checksif login in self.settings.admin_logins, while the read paths normalize logins (github_login_from_request()lowercases the user cookie, andadmin_login_from_request()lowercases before admin membership). Withadmin_logins=("alice",)and an OAuth login value of"ALICE", the user cookie is set but themr�
|
Reviewed on head Two auth-boundary items remain:
Validation:
|
|
Follow-up on my earlier review: I agree the admin-cookie case-normalization note is valid. My validation covered the lower-case login path, but The CSRF action-mismatch suggestion is useful regression coverage too, but lower severity than the admin-cookie write/read normalization mismatch. |
|
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. |
|
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
Refs #320
Summary
app/main.pyinto focusedapp/auth.py.Why this is distinct
app/main.pyand is one of the route/session boundaries named in MRWK bounty: code health and app.main expansion readiness #320.Validation
.\.venv\Scripts\python.exe -m pytest -q-> 323 passed..\.venv\Scripts\python.exe -m ruff check app tests-> passed..\.venv\Scripts\python.exe -m ruff format --check app tests-> passed..\.venv\Scripts\python.exe -m mypy app-> success..\.venv\Scripts\python.exe scripts\docs_smoke.py-> docs smoke ok.git diff --check origin/main...HEAD-> clean.Review gate
go; the only overlapping live PR is Extract bounty attempt routes from app main #337, which covers bounty-attempt routing rather than auth/session behavior.No secrets, wallet private keys, payout credentials, deployment values, private vulnerability details, or MRWK price claims are included.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests