Follow-up hardening beyond the explicit review findings:
- Propagate session revocation across uvicorn workers: token
validation now syncs issuance AND revocation from sessions.json
(mtime-gated), _save_sessions merges on-disk state under an
inter-process flock so concurrent workers can't lose each other's
sessions, and revocation tombstones prevent a just-revoked token
from being re-merged.
- Restrict sessions.json and auth.json to 0600 (bearer tokens and
password hashes; same policy as data/app.db, #4420), applied
atomically at write time and retroactively at load.
- Password-login session cookie: SECURE_COOKIES=false can no longer
downgrade the cookie when the request arrived over HTTPS (spoofable
X-Forwarded-Proto still requires TRUST_PROXY_HEADERS opt-in).
- Document why OIDC state tokens are deliberately not single-use and
which mechanisms bound the replay window.
- Warn once per process (not twice per login) when
OIDC_ALLOW_INSECURE_COOKIES is enabled; pass the variable through
the Compose files so the documented dev override actually reaches
containers.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GRiLb12nnLnBnYsg14oSWd
Addresses RaresKeY's review (5 findings) and follow-up Basic RP
validation comment on PR #3508:
- Add PKCE (RFC 7636, S256): code_challenge in the authorization
request, verifier carried in the Fernet-encrypted state, and
code_verifier sent to the token endpoint.
- Require the iat claim in id_tokens (OIDC Core §2); tokens without
iat are now rejected.
- Prefer client_secret_basic at the token endpoint per discovery
(OIDC default), falling back to client_secret_post only when the
provider excludes basic.
- Require HTTPS for the issuer and authorization endpoint, not just
the back-channel endpoints.
- Preserve OIDC subs exactly (no strip) so distinct whitespace-bearing
subjects can never collapse into one local account; same for the
UserInfo sub-binding comparison.
- Sync admin state only on a well-formed groups claim; UserInfo
availability alone (or a malformed groups value) no longer demotes
an existing admin.
- OIDC session/CSRF cookies are Secure by default regardless of
SECURE_COOKIES; explicit OIDC_ALLOW_INSECURE_COOKIES=true is the
only (documented, dev-only) opt-out.
- Make sessions issued by one uvicorn worker validate on others via
an mtime-gated read-through reload of sessions.json.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GRiLb12nnLnBnYsg14oSWd
All auth.json mutation methods now acquire _interprocess_auth_lock
(flock-based) + _config_lock with reload-before-save, matching the
pattern already used by create_user / create_user_oidc /
set_oidc_user_admin. This prevents stale-write clobbers when an OIDC
callback races a concurrent set_privileges, delete_user, rename_user,
set_admin, change_password, or TOTP mutation from another uvicorn
worker.
Also:
- Normalize setup() username (strip + lower) to match every other
user-creation path, preventing "Alice" vs "alice" lockout.
- Normalize in _create_user_locked() as defense-in-depth.
- Remove dead _setup_lock — its serialisation was subsumed by
_interprocess_auth_lock + _config_lock.
- Move pre-lock validation inside the critical section for
change_password, totp_generate_secret, and totp_confirm_enable.
- Re-read backup codes from disk inside the lock in totp_verify()
to prevent dual-consumption across workers.
Co-Authored-By: antigravity <agy@antigravity>
The reserved-username guard was only in create_user(), but setup()
now calls _create_user_locked() directly (to avoid nested flock
deadlock). Move the check into the shared helper so it applies
regardless of which path creates the user.
Fixes CI failure in test_setup_rejects_reserved_admin_username.
create_user() (password-user creation) now takes _interprocess_auth_lock
with reload-before-save, preventing a concurrent set_oidc_user_admin()
from losing a newly-created password user.
Introduces _create_user_locked() internal helper so setup() (which
already holds the inter-process lock) can create users without a
nested fcntl.flock deadlock.
Also changed _auth_intraprocess_lock to threading.RLock to support
safe nesting patterns across mutation methods.
Regression: test_create_user_survives_concurrent_set_oidc_user_admin
(109 total, 0 failures).
1. Serialize set_oidc_user_admin() across workers via
_interprocess_auth_lock() with reload-recheck-save, preventing
stale in-memory snapshots from overwriting users concurrently
created by another worker.
2. Accept single-element aud arrays without azp (OIDC Core §2 only
requires azp for multi-audience tokens). Normalize aud to a list
and only enforce azp when len > 1.
3. Add threading.Lock around _get_fernet() key creation to prevent
two threads in the same process from racing on the shared temp-file
path and destroying each other's work.
4. Guard import fcntl with try/except so AuthManager imports on native
Windows. _interprocess_auth_lock degrades to intra-process-only
when fcntl is unavailable.
Tests: +4 regression tests (108 total, 0 failures).
fcntl.flock serialises across processes (uvicorn workers) but does NOT
block threads within the same process — two threads calling
flock(LOCK_EX) on the same file both succeed immediately. This caused a
rare race in concurrent first-admin bootstrap where both
create_user_oidc() and setup() could enter the critical section
simultaneously, resulting in a lost write and zero admins.
Add a module-level threading.Lock acquired before the file lock so the
critical section is serialised across both threads and workers.
Fixes the flaky test:
TestInterprocessFirstAdminSerialisation::test_two_managers_single_first_admin
("Expected exactly 1 admin after concurrent bootstrap; found 0")
- Add fcntl.flock inter-process file lock shared by setup() and
create_user_oidc() so multi-worker first-admin bootstrap is
serialised across processes, not just threads within one worker.
Both methods reload auth.json inside the lock so the loser sees
the winner's write.
- _fetch_userinfo() now returns None (not {}) when discovery has
no userinfo_endpoint, and exchange_code() only sets
_userinfo_available=True when a live endpoint was reached.
Prevents the callback from treating 'no endpoint' as
authoritative group non-membership evidence.
- Rewrite _load_or_create_key() to write the Fernet key to a temp
file, fsync, then atomically os.link() into place. No reader
ever sees the final path before the complete key bytes are
available — a racing worker either sees no file or a complete
one, never an empty/partial file.
105 tests pass (97 existing + 8 new regressions covering the
three fixes).
Co-Authored-By: Kevin <holden093@users.noreply.github.com>
Three fixes from second review pass:
1. (security) Serialize the first-OIDC-user admin bootstrap inside
_config_lock. Previously the check and username
collision resolution happened outside the lock, so two concurrent
first-login callbacks could both observe an empty user map and
both persist as admin. Now idempotent lookup, bootstrap decision,
and collision resolution are all inside one critical section.
2. (auth) Make data/.app_key creation atomic via O_EXCL open so two
racing workers on a fresh deployment cannot generate different
keys. The loser reads the winner's key, guaranteeing every worker
shares the same Fernet key for OIDC state encryption.
3. (auth) Track whether UserInfo was successfully fetched
(_userinfo_available flag in claims). The callback now skips
admin group sync for existing users when UserInfo is unavailable
AND the id_token lacks a groups claim — a transient provider
failure no longer silently demotes existing OIDC admins. When
UserInfo succeeds or the id_token carries groups, admin status
syncs as before.
Regression tests: concurrent admin bootstrap, key-creation race,
UserInfo-unavailable preserves admin, UserInfo-available demotes,
id_token groups authoritative without UserInfo.
1. Suppress first-user bootstrap admin when OIDC_ADMIN_GROUPS is
configured — a non-admin IdP user must not get admin just by being
the first to log in. Group membership is the only path when groups
are set.
2. Reject mismatched UserInfo sub: the UserInfo endpoint MUST NOT
overwrite the verified id_token subject. Also guard all verified
identity claims (sub, iss, aud, exp, iat, nonce, azp) from being
overwritten by UserInfo.
1. aud validation: handle JSON array audiences per OIDC spec (check
membership, validate azp for multi-audience tokens).
2. JWKS caching: cache keys after first fetch; refresh only on unknown
kid — avoids live IdP round-trip on every login.
3. Algorithm pinning: restrict to RS256/ES256/etc from discovery doc,
never allow HS256 or 'none'.
4. OIDC_REDIRECT_URI: support a fixed redirect URI (proxy-safe,
doesn't trust Host header).
5. Stateless state: replace in-memory dict with Fernet-encrypted state
tokens — works across multiple uvicorn workers/processes.
6. OIDC_FIRST_USER_IS_ADMIN: bootstrap first OIDC user as admin when
no users exist and OIDC_ADMIN_GROUPS is unset — prevents zero-admin
lockout.
Adds OIDC authentication as an alternative to password login, enabling
sign-in via any standard provider (Authentik, Keycloak, Authelia, etc.).
New features:
- Generic OIDC provider support via .well-known discovery (authlib)
- Coexists with existing password auth — users choose at login
- Auto-creates local users on first OIDC login
- Admin group mapping: OIDC_ADMIN_GROUPS grants admin based on IdP groups
- Admin status syncs on every login (follows IdP membership)
- OIDC users cannot use password login or set up 2FA
- UI hides change-password and 2FA cards for OIDC users
New env vars:
- OIDC_ENABLED, OIDC_ISSUER, OIDC_CLIENT_ID, OIDC_CLIENT_SECRET
- OIDC_SCOPES, OIDC_ADMIN_GROUPS
New files:
- core/oidc.py — OidcManager (discovery, auth URL, code exchange,
id_token verification with JWT/JWKS)
- routes/oidc_routes.py — /api/auth/oidc/{login,callback,config}
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per code audit #4388: Wrap _migrate_single_user and
_drop_reserved_loaded_users with _config_lock to ensure atomic
config reads/writes and prevent potential race conditions during
concurrent access.
This is a defense-in-depth fix - these methods run at startup
before concurrent requests are accepted, but adding the lock
makes the code consistent with other config mutations.
The in-process tool loopback stamps current_user = "internal-tool" and
require_admin grants admin to that sentinel; it is also a reserved username.
That security-sensitive string was hand-typed in ~7 places (stamp, admin gate,
RESERVED_USERNAMES, and standalone admin-equivalent checks in note/research/
shell/task routes), where a typo silently breaks an auth gate.
Add INTERNAL_TOOL_USER in core/middleware.py next to INTERNAL_TOOL_TOKEN/
INTERNAL_TOOL_HEADER and use it at every such site. A typo is now an
ImportError, not a silent mismatch. auth.py importing middleware is acyclic
(middleware imports no app modules). Behaviour is unchanged.
The multi-sentinel sets bundling internal-tool with api/demo/system
(assistant_routes, task_scheduler, research_routes) are a separate reserved-set
dedup, left for a follow-up.
Closes#4332
Added PASSWORD_MIN_LENGTH and RESERVED_USERNAMES to src/constants.py as the
single source of truth. Previously PASSWORD_MIN_LENGTH was hardcoded as 8 in
four route handlers and all three JS validation paths; RESERVED_USERNAMES was
an inline frozenset duplicated in core/auth.py, routes/assistant_routes.py,
routes/research_routes.py, and src/task_scheduler.py.
Added GET /api/auth/policy (unauthenticated) so the frontend reads the real
values from the server instead of hardcoding them in JS.
Added missing empty-username guard to /setup and admin POST /users. Both
returned a misleading 500/409 on whitespace-only input. /signup already had the
check; this makes all three consistent.
* feat(auth): add per-user admin promote/demote toggle
Admin-only API and Users-tab control to grant/revoke admin rights; refuses to demote the last admin.
* fix(auth): restore pre-admin privilege restrictions on demotion
Promoting now stashes the user's privilege map (privileges_before_admin)
and demoting restores it instead of resetting to defaults, so a
promote/demote round trip can no longer broaden a restricted user's
access. Users without a stash (created as admin, or promoted before this
fix) still demote to DEFAULT_PRIVILEGES so a born-admin's stored all-True
map — including can_use_bash — can't survive demotion.
---------
Co-authored-by: K M Merajul Arefin <merajul.arefin@therapservices.net>
Three issues combined to make the per-user 'Allowed models' checklist
unreliable (#3032):
1. admin.js _loadModelsForUser fetched /api/models, which is backed by
cached_models — endpoints that haven't been probed yet (e.g. a
freshly-added DeepSeek API endpoint) simply didn't show up in the
checklist. Switched to /api/model-endpoints, which always reflects
every configured endpoint regardless of cache state.
2. _saveModels sent allowed_models: [] both when the admin clicked
[All] (no restriction) and [None] (block everything) — the backend
had no way to distinguish the two.
3. _enforce_chat_privileges treated an empty allowed_models list as
'no restriction' (falsy -> skip the check), so [None] had no effect.
Added an explicit block_all_models privilege flag (defaulting to False,
and forced to False for admins) that admin.js now sets when zero models
are checked. _enforce_chat_privileges checks it first and 403s
regardless of allowed_models contents.
* refactor(constants): single source of truth for data dir + merge core/src constants
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs(contributing): use named src.constants for data paths, drop core/constants references
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: revoke API bearer tokens when their owner is deleted
* Re-run CI
* Invalidate bearer-token cache on user delete so warmed cached tokens stop working
Use func.lower() when updating SQL owner columns, match prefs keys
case-insensitively, and normalize session usernames before comparing
during rename. Prevents silently skipping legacy mixed-case owner data.
Fixes#1165
After a successful password change, revoke all browser sessions for the
same user except the one that submitted the request. This prevents stale
sessions on other devices from remaining valid after credentials are
updated.
Keep API-token behavior unchanged. The current browser session is
preserved so the user can continue from the tab that changed the
password.
Add focused regression tests for preserving the current session, revoking
other sessions, persisting revocation, and avoiding revocation when the
current password is incorrect.
`core.middleware.require_admin` grants admin to any request whose
`request.state.current_user == "internal-tool"` — the sentinel meant only
for the in-process tool-loopback path. But the normal cookie auth path
(app.py) sets `current_user` to the raw username, and neither `create_user`
nor the signup route reserved that name. As a result an account literally
named "internal-tool" was silently treated as admin by every
`require_admin`-gated route. With self-service signup enabled this is an
anonymous -> admin privilege escalation.
Reserve the full synthetic-owner set the codebase already special-cases —
"internal-tool", "api", "demo", "system" (see `_SYNTHETIC_OWNERS` in
routes/assistant_routes.py and the matching guards in src/task_scheduler.py
and routes/research_routes.py). "api" collides with the bearer-token owner
sentinel; "demo"/"system" would leave a real account denied an assistant
and inconsistently owner-scoped.
Refuse to create or rename into any reserved name (case/space-normalized),
and reject empty usernames while we're here. Adds a regression test.
Co-authored-by: Claude <noreply@anthropic.com>
verify_password() and create_session() both call .strip().lower() on
the incoming username, but _load() stored keys verbatim from auth.json.
Any mixed-case key (e.g. written by manual edit or a future migration)
would never match, producing a permanent 'Invalid credentials' error.
Fix: lowercase all keys at load time so the in-memory dict always
matches what the login path expects.
Fixes#423
