|
| 1 | +# Project Review Notes |
| 2 | + |
| 3 | +## ⚠️ High Priority Security Issues |
| 4 | + |
| 5 | +**CRITICAL - Refresh Token Authentication Bypass** |
| 6 | + |
| 7 | +- **Issue**: Refresh tokens can be used as access tokens because access tokens |
| 8 | + don't carry a `typ` claim and `get_jwt_user` doesn't enforce token type. This |
| 9 | + completely undermines the token separation model and allows 30-day tokens to be |
| 10 | + used where 120-minute tokens should be required. |
| 11 | +- **Impact**: Attackers with a stolen refresh token get extended unauthorized |
| 12 | + access to protected endpoints. The entire point of short-lived access tokens is |
| 13 | + defeated. |
| 14 | +- **Fix**: Add `typ="access"` in `AuthManager.encode_token` and validate |
| 15 | + `typ="access"` in `get_jwt_user`. Reject any token without the correct type. |
| 16 | +- **Priority**: Fix immediately before addressing other issues. |
| 17 | + |
| 18 | +## Code Issues / Risks |
| 19 | + |
| 20 | +- `payload["typ"]` / `payload["sub"]` are accessed directly in |
| 21 | + refresh/verify/reset flows; a token missing those keys will throw `KeyError` |
| 22 | + and return a 500 instead of a clean 401. Use `payload.get(...)` and treat |
| 23 | + missing keys as invalid token. |
| 24 | +- CORS default allows `cors_origins="*"` with `allow_credentials=True`, which |
| 25 | + browsers reject. Either disallow `*` when credentials are on or auto-flip |
| 26 | + `allow_credentials=False` for the wildcard case. |
| 27 | +- `get_jwt_user` doesn’t apply the JWT format/length guard you already built; |
| 28 | + very large or malformed tokens go straight to `jwt.decode`. Add the |
| 29 | + `MAX_JWT_TOKEN_LENGTH` and `is_valid_jwt_format` checks here. |
| 30 | +- Verification success uses `HTTPException` for control flow and returns 200 |
| 31 | + with a message; docs already note this should be 204. Consider returning a |
| 32 | + normal response (or 204) to reduce exception noise. |
| 33 | + |
| 34 | +## Existing TODOs Worth Prioritizing |
| 35 | + |
| 36 | +- Logout + token invalidation (security win) |
| 37 | +- Fix docs status codes/examples (quick win) |
| 38 | +- Test TODOs for better assertions |
| 39 | +- Refactor `ApiKeyAuth.__call__()` complexity |
| 40 | + |
| 41 | +## Enhancements From Code Review |
| 42 | + |
| 43 | +- API key scopes are stored but not enforced anywhere. Add scope checks in a |
| 44 | + dependency that resources can opt into. |
| 45 | +- Trim/normalize `cors_origins` entries (split+strip) to avoid accidental |
| 46 | + leading spaces causing mismatches. |
| 47 | +- Consider adding `created_at`/`updated_at` on `User` for auditability and to |
| 48 | + support features like “recently active users.” |
| 49 | + |
| 50 | +## Feature Ideas (new) |
| 51 | + |
| 52 | +- Session management: list/revoke active refresh tokens per user (ties into |
| 53 | + logout/invalidations). |
| 54 | +- Account security notifications: email user when password/email changes or |
| 55 | + when banned/unbanned. |
| 56 | +- API key scope enforcement + per-route required scopes (pairs well with |
| 57 | + existing API key model). |
| 58 | +- Optional 2FA (TOTP) for admin and/or all users. |
| 59 | +- Webhook events for user lifecycle changes (register/verify/ban/unban/delete) |
| 60 | + to integrate with external systems. |
0 commit comments