fix: enforce token type separation and harden CORS defaults #804
Conversation
📝 WalkthroughWalkthroughAdds explicit JWT token typing ("typ") for access vs refresh tokens and enforces it at runtime; disables CORS credentials by default and emits a startup warning when Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as FastAPI
participant AuthDep as get_jwt_user
participant JWT as AuthManager
participant DB as UserStore
Client->>API: Request with Authorization: Bearer <token>
API->>AuthDep: invoke auth dependency
AuthDep->>JWT: decode & verify JWT
JWT-->>AuthDep: return payload (may include typ)
alt payload.typ == "access"
AuthDep->>DB: fetch user by sub/claims
DB-->>AuthDep: user record
AuthDep-->>API: return authenticated user
API-->>Client: 200 / route response
else payload.typ missing or != "access"
AuthDep->>AuthDep: log warning "INVALID_TOKEN"
AuthDep-->>API: raise 401 INVALID_TOKEN
API-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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: 0
🧹 Nitpick comments (1)
app/main.py (1)
65-72: Refactor the dependency on module-levelcors_list.The
lifespanfunction referencescors_list(line 65), butcors_listis defined later at line 173. While this works becauselifespanexecutes after module initialization, it creates a non-obvious dependency on evaluation order and reduces code clarity.🔎 Suggested refactoring approaches
Option 1: Define cors_list before lifespan
Move the cors_list definition to before the lifespan function:
+# set up CORS +cors_list = [ + origin.strip() + for origin in get_settings().cors_origins.split(",") + if origin.strip() +] + @asynccontextmanager async def lifespan(app: FastAPI) -> AsyncGenerator[Any, None]:Option 2: Build cors_list inside lifespan
@asynccontextmanager async def lifespan(app: FastAPI) -> AsyncGenerator[Any, None]: """Lifespan function Replaces the previous startup/shutdown functions.""" # Initialize loguru logging within the server process. get_log_config() + cors_list = [ + origin.strip() + for origin in get_settings().cors_origins.split(",") + if origin.strip() + ] + if "*" in cors_list:Option 1 is preferred as it keeps cors_list at module level for the middleware configuration while making the dependency explicit.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.env.exampleSECURITY-REVIEW.mdapp/main.pyapp/managers/auth.pydocs/deployment/deployment.mddocs/usage/configuration/environment.mdtests/integration/test_protected_user_routes.pytests/unit/test_auth_manager.pytests/unit/test_cors_config.pytests/unit/test_jwt_auth.pytests/unit/test_lifespan.py
🧰 Additional context used
🧬 Code graph analysis (4)
app/main.py (2)
app/logs.py (1)
warning(36-39)app/config/settings.py (1)
get_settings(247-249)
tests/unit/test_lifespan.py (1)
app/main.py (1)
lifespan(55-138)
tests/integration/test_protected_user_routes.py (3)
app/config/settings.py (1)
get_settings(247-249)app/managers/auth.py (1)
encode_refresh_token(80-106)app/models/user.py (1)
User(10-36)
tests/unit/test_jwt_auth.py (2)
app/config/settings.py (1)
get_settings(247-249)app/managers/auth.py (3)
refresh(170-236)get_jwt_user(479-555)ResponseMessages(29-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.14)
- GitHub Check: test (3.11)
🔇 Additional comments (17)
app/managers/auth.py (2)
55-55: LGTM! Critical security improvement.Adding
typ="access"to access tokens enables type-based validation and prevents refresh tokens from being used as access tokens. This addresses critical security issue #1 from SECURITY-REVIEW.md.
496-505: LGTM! Proper token type enforcement.The validation correctly rejects tokens with
typ != "access", preventing authentication bypass via refresh tokens or other token types. The error handling is appropriate: logs a warning, increments auth failure metrics, and returns 401 with INVALID_TOKEN..env.example (1)
48-49: LGTM! Clear CORS guidance.The updated comment accurately describes the new behavior:
*is acceptable for public APIs using Bearer tokens (with a warning), while explicit origins should be used for browser clients. This aligns with the code changes inapp/main.py.docs/deployment/deployment.md (1)
19-20: LGTM! Clear deployment guidance.The updated CORS documentation properly distinguishes between browser clients (requiring explicit origins) and public APIs using Bearer tokens (where
*is acceptable but logged). This aligns with the security hardening objectives.docs/usage/configuration/environment.md (1)
181-185: LGTM! Comprehensive CORS documentation.The documentation clearly explains the two use cases: public APIs with Bearer tokens (where
*is acceptable) vs. browser clients (requiring explicit origins). The note about the warning being logged helps developers understand the intended behavior.app/main.py (2)
173-177: LGTM! Improved CORS origin parsing.The list comprehension properly strips whitespace and filters empty entries, making the origin parsing more robust against malformed configuration.
182-182: LGTM! Critical security hardening.Setting
allow_credentials=Falseis correct for Bearer token authentication and prevents the security issue of wildcard CORS with credentials. This addresses critical security issue #2 from SECURITY-REVIEW.md.Note: This is a breaking change for clients that relied on credentialed requests, but it's intentional and necessary for security.
tests/unit/test_cors_config.py (1)
11-24: LGTM! Test correctly validates CORS security configuration.The test properly verifies that:
- CORS middleware is present in the application
- Credentials are disabled (
allow_credentials=False)- Wildcard origin is configured
The use of
next()without a default is appropriate here—if the middleware isn't found, the test should fail withStopIteration.tests/unit/test_auth_manager.py (1)
45-45: LGTM! Assertions correctly validate token type claims.The new assertions verify that:
- Access tokens include
typ="access"(line 45)- Refresh tokens include
typ="refresh"(line 72)This test coverage aligns with the security fix for token type enforcement described in the PR objectives.
Also applies to: 72-72
tests/unit/test_lifespan.py (1)
215-239: LGTM! Test correctly validates CORS wildcard warning.The test properly verifies that:
- A warning is logged when
CORS_ORIGINS="*"is configured- Both the uvicorn logger (via
caplog) and loguru logger emit the warning- The warning message contains the expected text
The test setup appropriately disables caching to isolate the CORS warning behavior.
SECURITY-REVIEW.md (1)
7-9: LGTM! Documentation accurately reflects the security fixes.The notes correctly document:
- Issue #1: Token type enforcement now implemented with
typ="access"validation- Issue #2: CORS credentials disabled and startup warning for wildcard origins
These updates align with the PR objectives and the code changes across the other files.
Also applies to: 26-28
tests/unit/test_jwt_auth.py (3)
5-5: LGTM! New imports appropriately support JWT test scenarios.The imports enable:
jwt: Direct token encoding for testing edge casesget_settings: Access to the secret key for token creationThese are necessary for the new test cases that validate token type enforcement.
Also applies to: 10-10
64-81: LGTM! Test correctly validates refresh token rejection.The test ensures that:
- A valid refresh token is obtained from registration
- When used as an access token (in the Authorization header), it's rejected
- The response is HTTP 401 with
INVALID_TOKENmessageThis directly validates the token type separation security fix described in the PR objectives.
83-107: LGTM! Test correctly validates missing typ claim rejection.The test ensures that:
- A JWT without the
typclaim is manually crafted- When presented as an access token, it's rejected by
get_jwt_user- The response is HTTP 401 with
INVALID_TOKENmessageThis validates that tokens lacking the
typclaim (legacy tokens or tokens from external sources) are properly rejected, completing the token type enforcement security fix.tests/integration/test_protected_user_routes.py (3)
3-12: LGTM! Clean imports for token-type testing.All new imports support the token-type enforcement tests and are used appropriately.
74-95: LGTM! Excellent coverage for refresh token rejection.This test properly verifies that refresh tokens (with
typ="refresh") are rejected on protected routes, addressing the critical security issue mentioned in the PR objectives.
97-126: LGTM! Thorough validation of typ claim requirement.This test effectively verifies that tokens without the
typclaim are rejected. The manual token construction usingjwt.encodeis the appropriate approach to create a valid JWT that intentionally omits the requiredtypfield.
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
What
typ="access"on access tokens and reject refresh/typ-less tokens in auth dependency.CORS_ORIGINS="*".SECURITY-REVIEW.md.Why
SECURITY-REVIEW.md:*).Testing
poe testpoe mypypoe ruff