-
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 all commits
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,37 @@ 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 | ||
| _MAX_TRACKED_SETUP_STATUS_IPS = 10000 | ||
|
|
||
|
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) | ||
| 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)}, | ||
| ) | ||
| # Evict stale entries when dict grows too large to bound memory usage. | ||
| if len(_SETUP_STATUS_COOLDOWN) >= _MAX_TRACKED_SETUP_STATUS_IPS: | ||
| cutoff = now - _SETUP_STATUS_COOLDOWN_SECONDS | ||
| stale = [k for k, t in _SETUP_STATUS_COOLDOWN.items() if t < cutoff] | ||
| for k in stale: | ||
| del _SETUP_STATUS_COOLDOWN[k] | ||
| # If still too large after evicting expired entries, remove oldest half. | ||
| if len(_SETUP_STATUS_COOLDOWN) >= _MAX_TRACKED_SETUP_STATUS_IPS: | ||
| by_time = sorted(_SETUP_STATUS_COOLDOWN.items(), key=lambda kv: kv[1]) | ||
| for k, _ in by_time[: len(by_time) // 2]: | ||
| del _SETUP_STATUS_COOLDOWN[k] | ||
| _SETUP_STATUS_COOLDOWN[client_ip] = now | ||
|
Comment on lines
+393
to
+415
|
||
| admin_count = await get_local_provider().count_admin_users() | ||
| return {"needs_setup": admin_count == 0} | ||
|
|
||
|
|
||
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.