fix: add token expiry and timing-safe password check#23
fix: add token expiry and timing-safe password check#23ankushchk wants to merge 1 commit intoalphaonelabs:mainfrom
Conversation
Made-with: Cursor
WalkthroughAuthentication and authorization security enhancements added to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/worker.py`:
- Around line 195-196: The token validation currently allows tokens lacking an
"exp" claim to pass; update the check so tokens without exp are rejected and
expired tokens are rejected too by replacing the condition with a strict
requirement (e.g., use data.get("exp") presence check and compare
int(data["exp"]) against int(time.time())); specifically change the conditional
around data, exp and time.time() such that if not data.get("exp") or
int(data["exp"]) < int(time.time()): return None, ensuring the validator returns
None for missing or expired exp values (optionally log a warning before
rejecting for migration scenarios).
- Around line 178-179: The function verify_token lacks a return type annotation;
update its signature to include a precise return type such as Optional[Dict[str,
Any]] (or dict | None for Python 3.10+) to reflect "decoded payload dict or
None", and add the required typing imports (Optional, Dict, Any) at the top if
not already present so static checkers and readers can understand the return
value; keep the existing docstring but ensure the signature reads e.g. def
verify_token(raw: str, secret: str) -> Optional[Dict[str, Any]]:.
🪄 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: alphaonelabs/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3b84fce-c3d1-4a5c-a4fc-579c87bae777
📒 Files selected for processing (1)
src/worker.py
Summary
expclaim) to auth tokens, previously tokens never expiredverify_password()from==tohmac.compare_digest()to prevent timing attacksexptoexpectedinverify_token()for clarityWhat was wrong
create_token()had noexpfield andverify_token()never checked expiration. A leaked token would grant permanent access.hash_password(password, username) == storedleaks information about how many characters matched via response time differences.Test plan
expfield (base64-decode payload to verify)/api/dashboardexp) is rejected with"Authentication required"Purpose
This PR implements two critical security improvements to the authentication system: token expiration to limit the window of exposure for leaked credentials, and timing-safe password verification to prevent timing attacks that could leak password information.
Key Modifications
Token Expiration: Added 1-hour (
_TOKEN_TTL = 3600) expiry to all authentication tokens via a newexpclaim in the token payload. Theverify_token()function now validates this claim and rejects expired tokens.Timing-Safe Password Comparison: Replaced direct string equality (
==) withhmac.compare_digest()inverify_password()to provide constant-time comparison and prevent attackers from leveraging response timing to infer password information.Code Clarity: Renamed the ambiguous
expvariable toexpectedinverify_token()to avoid shadowing theexpclaim from the token payload.Impact
Verification
The changes have been verified with existing login/register flows, token expiration validation (fresh tokens authorize access, expired tokens are rejected), and normal operation of the authentication system.