-
Notifications
You must be signed in to change notification settings - Fork 8.4k
fix(security): harden auth system and fix run journal logic bug #2593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/2.0-rc
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -146,7 +146,13 @@ def _set_session_cookie(response: Response, token: str, request: Request) -> Non | |||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # ── Rate Limiting ──────────────────────────────────────────────────────── | ||||||||||||||||||||||
| # In-process dict — not shared across workers. Sufficient for single-worker deployments. | ||||||||||||||||||||||
| # In-process dict — not shared across workers. | ||||||||||||||||||||||
| # | ||||||||||||||||||||||
| # **Limitation**: with multi-worker deployments (e.g., gunicorn -w N), each | ||||||||||||||||||||||
| # worker maintains its own lockout table, so an attacker effectively gets | ||||||||||||||||||||||
| # N × _MAX_LOGIN_ATTEMPTS guesses before being locked out everywhere. For | ||||||||||||||||||||||
| # production multi-worker setups, replace this with a shared store (Redis, | ||||||||||||||||||||||
| # database-backed counter) to enforce a true per-IP limit. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| _MAX_LOGIN_ATTEMPTS = 5 | ||||||||||||||||||||||
| _LOCKOUT_SECONDS = 300 # 5 minutes | ||||||||||||||||||||||
|
|
@@ -376,9 +382,19 @@ async def get_me(request: Request): | |||||||||||||||||||||
| return UserResponse(id=str(user.id), email=user.email, system_role=user.system_role, needs_setup=user.needs_setup) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| _SETUP_STATUS_COOLDOWN: dict[str, float] = {} | ||||||||||||||||||||||
| _SETUP_STATUS_COOLDOWN_SECONDS = 60 | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
Comment on lines
+385
to
+388
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @router.get("/setup-status") | ||||||||||||||||||||||
| async def setup_status(): | ||||||||||||||||||||||
| async def setup_status(request: Request): | ||||||||||||||||||||||
| """Check if an admin account exists. Returns needs_setup=True when no admin exists.""" | ||||||||||||||||||||||
| client_ip = _get_client_ip(request) | ||||||||||||||||||||||
| now = time.time() | ||||||||||||||||||||||
| last_check = _SETUP_STATUS_COOLDOWN.get(client_ip, 0) | ||||||||||||||||||||||
| if now - last_check < _SETUP_STATUS_COOLDOWN_SECONDS: | ||||||||||||||||||||||
| return {"needs_setup": False} | ||||||||||||||||||||||
|
||||||||||||||||||||||
| if now - last_check < _SETUP_STATUS_COOLDOWN_SECONDS: | |
| return {"needs_setup": False} | |
| elapsed = now - last_check | |
| if elapsed < _SETUP_STATUS_COOLDOWN_SECONDS: | |
| retry_after = max(1, int(_SETUP_STATUS_COOLDOWN_SECONDS - elapsed)) | |
| raise HTTPException( | |
| status_code=status.HTTP_429_TOO_MANY_REQUESTS, | |
| detail="Setup status check is rate limited", | |
| headers={"Retry-After": str(retry_after)}, | |
| ) |
Copilot
AI
Apr 27, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The /setup-status cooldown currently returns {"needs_setup": False} during the cooldown window, which can mislead legitimate clients (e.g., UI polling) into thinking setup is complete when it may not be. Instead of returning a falsified state, consider returning 429 with Retry-After, or returning a cached last-computed needs_setup value per IP while enforcing the refresh cooldown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 9d83642. The /setup-status endpoint now returns HTTP 429 with a Retry-After header (value = remaining cooldown seconds) instead of the misleading {"needs_setup": False} response. The _SETUP_STATUS_COOLDOWN dict also gets bounded eviction (capped at 10,000 IPs) using the same TTL-first then oldest-half strategy as _login_attempts. A new test test_setup_status_rate_limited_on_second_call covers the 429 + Retry-After behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing verify_password() to always SHA-256 pre-hash before bcrypt will break authentication for any existing users whose password_hash values were generated with the old (raw-password) bcrypt scheme. Consider supporting both formats during a transition (e.g., try legacy check first, then new check; or version/prefix hashes and migrate on successful login), otherwise this is a backwards-incompatible auth change that can lock out current deployments after upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@greatmengqi I'd been hit by this issue. It should work, as we don't have a public release yet. But we also need to provide a password reset method for users to use.