Skip to content

Commit a5f0fb2

Browse files
committed
feat: add project review notes and security improvement plans
Add comprehensive PLANS.md documenting security issues, code risks, and enhancement opportunities discovered during code review. Signed-off-by: Grant Ramsay <seapagan@gmail.com>
1 parent 49ff3dc commit a5f0fb2

File tree

1 file changed

+49
-0
lines changed

1 file changed

+49
-0
lines changed

PLANS.md

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

0 commit comments

Comments
 (0)