feat(auth): store more than two local accounts (#784) - #1122
Merged
Merged
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic Password | 47ab8ba | tests/api/auth/totp-reauth.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
When storage is sqlite or postgres, local email/password accounts live in an accounts table. Env credentials seed that table once. Passwords are scrypt, and Admin → Accounts can create, disable, delete, and enrol TOTP. Local mode and OIDC stay on their existing paths.
nawazish2
force-pushed
the
feat/784-local-accounts
branch
from
September 25, 2026 17:23
d99282b to
d195507
Compare
cevheri
added
Needs Triage
security
Member
|
@nktnet1 @nawazish2 I'm taking this PR over and starting the review now. Please don't push new commits to this branch from here on. I'll review it, update it where needed, and let you both know here once that's done. Thanks for the work so far. |
cevheri
self-requested a review
September 27, 2026 22:51
…ed, deleted or reset A session is a 24-hour JWT, so the registry decided only who could log in next. A disabled account kept reading and writing its storage, a demoted admin kept the account registry and could promote itself back, and a deleted account's live session wrote rows that a later account with the same email inherited. Accounts now carry a session version that is signed into the token at login and compared on every getSession(). Disabling, a role change and a password change increment it; a deleted account has no row. A registry that cannot be read refuses the session. OIDC and STORAGE_PROVIDER=local sessions are not looked up.
Two separate statements could remove the account and leave its user_storage rows, which the next account created with the same email then inherited.
Delete removes the account's saved connections, history and saved queries with it, and a single click did it with no way back.
Once the registry is seeded the environment no longer sets the admin's password, so an operator who rotated ADMIN_PASSWORD saw no effect and no explanation, and an admin locked out of every enabled account had no way back short of editing the database. Each start now compares the env admin with the store once and logs a warning when the password differs or ADMIN_EMAIL has no row. ADMIN_PASSWORD_RESET=true makes ADMIN_EMAIL an enabled admin with ADMIN_PASSWORD and ADMIN_TOTP_SECRET (or no second factor), ends its old sessions, audits the change with actor "environment", and warns until the variable is removed. New rows now start at a random session version, so a session of a deleted account never matches a later account with the same email, and a token without a version matches nothing.
A session cookie alone could replace an active TOTP secret (begin, then confirm with the thief's authenticator) or remove it, so a stolen cookie defeated the factor that exists to limit what a stolen credential can do. begin now needs the current password and refuses while a factor is active; disable needs the password and, when a factor is active, a current code that has not been used. A wrong answer is audited and charged to the same login_client and login_account budgets as a failed login.
Enrolment was reachable only from the admin Accounts tab, so an account without admin rights had no way to turn a second factor on. The authenticator section is now one component, shown in the Accounts tab and on /settings/authenticator, linked from the user menu. It asks for the current password, and for a current code when turning the factor off. GET /api/auth/totp says whether the account has a factor, or why setup is not offered (OIDC, or no server store).
…deleted or reset An MCP token carries the role it was minted with and lives 30 days by default, so with the account registry it outlived every change to its account: a disabled or deleted account kept run_read_query, and a demoted admin kept the admin role on the MCP channel. A token minted for a stored account now carries its session version, and the route checks it against the registry on every call, answering the same 401 an invalid token gets. The proxy stays a signature check, because it must not load the storage drivers.
…action The last-admin check read the accounts, awaited, then wrote, so two concurrent requests could each pass it and remove one of the last two admins. The red-team run left zero admins in 6 of 12 rounds on Postgres, and the same race reproduces in one process on SQLite. A write that removes an enabled admin now runs with keepEnabledAdmin: Postgres locks the enabled admin rows FOR UPDATE, SQLite takes the write lock with BEGIN IMMEDIATE, and the write rolls back with LastAdminError when no enabled admin would remain. The application-level pre-check is gone; its 409 now comes from the store.
The accounts table held each second factor's shared secret in the clear while connection passwords in the same store are sealed, so a copy of the database was enough to mint codes. totp_secret and totp_pending now go through the same AES-GCM envelope and key as connection secrets. A sealed factor the current key cannot open reads as an unusable secret rather than as no factor, so rotating the key locks the account's second factor instead of switching it off, and the log names the account.
…th a dot The matcher skips every path containing a dot, which is meant for static assets, so PATCH and DELETE on /api/admin/accounts/<email> bypassed the Origin check and returned no CSP or HSTS whenever the email had a dot, which is nearly always. The route's session check still held. A second matcher entry covers API paths with a dot; static assets and /api/storage/config stay excluded.
… name Only successful account changes reached the audit log, so an attempt to remove the last enabled admin, or to act on an account that does not exist, left no trace. Refused creates, changes and deletes now emit an account event with outcome failure and reason account_refused. Also files H14: an unclassified 500 returns the raw error message, which with the account registry means an unreachable Postgres store's address reaches an unauthenticated caller.
STORAGE.md gains the session_version column, the DELETE grant on user_storage that removing an account needs, and how sessions, the last-admin guard, the drift warning and ADMIN_PASSWORD_RESET behave. MFA.md describes self-service setup with re-authentication and the sealed secrets. API_DOCS.md documents the account and TOTP routes. SECURITY.md adds control 1.7 and corrects the MCP revocation note, which no longer holds for a stored account.
The Accounts tab becomes a data table with role and status badges, a two-factor mark and one row menu per account, over a toolbar with search by email, a role, status and two-factor filter and a count. Adding an account and setting a password open their own dialogs, and a refusal is shown inside the dialog that caused it. Every change that ends someone's sessions or weakens sign-in (make admin, make user, disable, clear two-factor, delete) asks the admin to type the account's email before the button enables. The authenticator section and /settings/authenticator are restyled to match.
…oken editor Driving it in a browser: after an admin disabled an account, its open editor tab stayed on a screen of "Authentication required" errors, and because the proxy only verifies the cookie's signature, /login redirected straight back to the editor, so the person could not sign in again without clearing cookies by hand. getSession() now clears the cookie when the registry refuses the session, and useAuth() sends the tab to /login when /api/auth/me answers 401.
cevheri
approved these changes
Sep 28, 2026
Member
|
@nktnet1 @nawazish2 The review and update are done, and the result is pushed to this branch. The shape matches what #784 asked for, and the zero-config path is unchanged: with The security pass found gaps that had to close before this could merge, now fixed with tests:
Every account can also set up its own authenticator from the user menu, and the Accounts screen is redesigned, with typed confirmation for destructive changes. Thanks for the solid base, @nawazish2. |
cevheri
added
enhancement
A realistic literal in a fixture is what secret scanners flag; these tests now read their passwords from named placeholder constants.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged js/user-controlled-bypass on the begin, confirm and disable branches: a value from the request body chose which credential-checking call ran. Every entry already sits behind the session guard and makes its own password and code checks, so the table changes nothing a caller can reach; inherited keys such as toString are refused.
This was referenced Sep 28, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
With
STORAGE_PROVIDER=sqliteorpostgres, local email/password accounts live in anaccountstable next touser_storage, so a team is no longer capped at one admin and one user.ADMIN_*(andUSER_*when a password is set) seed the table once; further accounts are managed under Admin → Accounts.STORAGE_PROVIDER=local(the zero-config default) and OIDC mode are unchanged and never touch the table.Original implementation by @nawazish2; review, security fixes and the Accounts screen redesign by the maintainers.
What it does
FOR UPDATE, SQLiteBEGIN IMMEDIATE), so concurrent requests cannot remove the last two.session_version, checked on every request. Disabling, deleting, a role change or a password reset ends them at the next request, and the revoked tab is sent to the login screen. Upgrading a sqlite/postgres deployment signs local users out once.ADMIN_PASSWORDno longer matches the stored admin;ADMIN_PASSWORD_RESET=truerestoresADMIN_EMAILas an enabled admin with the environment's password and factor./api/admin/accounts/<email>) now get the Origin check and security headers, and refused admin changes are audited with the acting admin.Docs:
docs/STORAGE.md,docs/MFA.md,docs/API_DOCS.md,docs/SECURITY.md(control 1.7),.env.example. H14 filed indocs/BACKLOG.md(an unclassified 500 returns the raw error message).Closes #784
Testing
bun run test,build,test:coverage+coverage:check(100%),build:lib,attw.