chore(release): promote develop to main - #147
Merged
Merged
Conversation
Phase 1 scopes the next-auth v5 to Better Auth migration with per-company 2FA policy and TOTP. Records the seam-preserving approach, Prisma schema and data migration, session strategy, and phase boundaries.
requireAuth and requireAdmin now reject un-enrolled users when their company sets require2fa, matching the dashboard layout redirect so the policy is not bypassable through direct API calls. The company lookup only runs for users without 2FA, keeping the enrolled path a single round-trip. Enrollment endpoints under /api/auth stay ungated so a forced user can still enroll. Also reject an empty allowedAuthMethods list in the auth-policy PATCH (an empty array previously passed the truthy check and persisted an invalid policy the UI already forbids).
Full NextAuth to Better Auth migration (10-task plan) plus post-review fixes: API-level 2FA policy enforcement and empty-method-list rejection.
Resolves both Dependabot high alerts for brace-expansion (ReDoS via exponential expansion of consecutive non-expanding braces). Transitive via minimatch under eslint tooling; patch-level bumps to 1.1.16 and 5.0.8, no API impact.
Better Auth 1.6.25's two-factor plugin writes verified, failedVerificationCount and lockedUntil on the TwoFactor row. The model only had secret/backupCodes/userId, so every enrollment failed with a PrismaClientValidationError (Unknown argument verified) and 2FA was completely broken. Unit tests mock Prisma and the e2e admin never enrolls, so nothing caught it. Add the three columns (matching the plugin's declared defaults) plus a migration, and a seed:2fa dev helper that enrolls a dedicated mfa@datashield.local through the real Better Auth flow and prints the TOTP secret and backup codes for local testing.
Adds mfa@datashield.local with a credential account so the two-factor spec can enroll it without touching the password-only admin the smoke test relies on.
Drives the real Better Auth enable/verify flow over HTTP and generates matching TOTP codes (base32-decoding the URI secret to the raw string Better Auth signs over).
Enrolls a user, then asserts password alone shows the TOTP form (not the dashboard) and only a valid code completes sign-in. Guards the missing TwoFactor column regression that the mocked unit suite could not catch.
Pin sharp, find-my-way and valibot to patched versions through npm overrides instead of npm audit fix --force, which would downgrade Next 15 to 14 and Prisma 7.9 to 7.8 (breaking, and older majors). Clears the sharp (DoS), find-my-way and valibot advisories with no framework change. The remaining audit 'high' entries are brace-expansion false positives: the installed 1.1.16 and 5.0.8 are the patched releases of each branch, but npm audit merges the two advisories into a single <=5.0.7 range and misflags the fixed 1.x line. All are dev-only eslint tooling.
Replace the checkmark glyph with plain text so the pre-push ascii compliance gate passes.
A Better Auth before-hook maps each enrollment endpoint to the method it enables and rejects with 403 when that method is not in the caller's Company.allowedAuthMethods, so the policy is real server-side instead of decorative. Currently covers TOTP; the map grows as methods are added.
Mirrors the server enforcement in the UI so users are not offered a method their company policy rejects.
Adds a serial test asserting a company that excludes TOTP gets a 403 on enable, alongside the existing enrollment/challenge test. Restores the policy afterwards so the parallel smoke run is unaffected.
Wire the twoFactor plugin's OTP sub-mode so a 2FA user can receive a login code by email as an alternative to TOTP. Codes go through the shared email sender, with a dev console fallback when email is disabled. Extend the policy gate to /two-factor/send-otp so a company that does not allow EMAIL_OTP is refused server-side. That endpoint runs mid-login, in the 2FA-pending state where there is no full session yet, so resolve the caller's company from the signed two_factor cookie the same way Better Auth's verifyTwoFactor does. Without this the gate would silently skip and EMAIL_OTP would never be enforced. Also record the phase-B approach plus the later OAuth (D) and SSO (E) phases in the plan. Claude-Session: https://claude.ai/code/session_01XU4Pf6uxHPJhEBwkFvwtuh
After the password step, a 2FA user can pick "email me a code" instead of the authenticator app, then send and verify it. The server policy hook stays the real guard: a disallowed EMAIL_OTP is reported as such. Claude-Session: https://claude.ai/code/session_01XU4Pf6uxHPJhEBwkFvwtuh
Two Playwright cases: a company that disallows EMAIL_OTP gets a 403 at send-otp, and an allowed company completes the email-code sign-in. A helper reads the stored code from the verification table since the dev fallback only logs it. The serial 2FA suite makes several sign-ins inside Better Auth's 3-per-10s window, so set E2E=1 on the CI e2e job to disable that rate limit there (armed only under a loopback base URL, inert in prod). Claude-Session: https://claude.ai/code/session_01XU4Pf6uxHPJhEBwkFvwtuh
Add @better-auth/passkey with a Passkey model and migration. The server plugin derives rpID and origin from AUTH_URL so dev (localhost) and prod are correct without extra config. Passkey enrollment is gated the same way TOTP is: /passkey/generate-register-options is refused when a company does not allow PASSKEY. Registration and authentication ceremonies (UI) come next. Claude-Session: https://claude.ai/code/session_01XU4Pf6uxHPJhEBwkFvwtuh
Spec for SSO login via company IdP (no self-signup), a granular permission catalog with customizable per-company roles and presets, in-app role management with anti-escalation invariants, a sealed break-glass local account, and a threat model covering the RBAC, SSO, session, and break-glass attack surfaces. Claude-Session: https://claude.ai/code/session_01XU4Pf6uxHPJhEBwkFvwtuh
Five suites created companies and users per run and never removed them. On this machine that had grown to 450 companies and 730 users, which makes the local database unusable for manual testing: the seeded admin is buried under hundreds of rows nobody can tell apart from real data. Deleting the company is the whole cleanup. Users, credential accounts, sessions, roles and audit rows all reach it through onDelete: Cascade, so tracking the company id is enough and no suite needs a bespoke teardown list that would rot as it grows new fixtures. Verified by counting rows around a full integration run: 451 companies before, 451 after, where the same run previously added one per suite.
The 2FA, passkey, auth policy and SSO controls lived inside the onboarding checklist, which redirects to the dashboard as soon as a workspace has its first employee or API key. Past that point an administrator could not reach the SSO configuration at all: no menu entry pointed there, and the page bounced. The only way in was the enroll=2fa query param the layout used for forced enrollment, which e2e had already resorted to. The blocks move into a shared component rendered by both the new page and the checklist, so onboarding keeps them without a second copy. The route is /security rather than /settings because next.config.ts holds a permanent redirect from /settings to /data-sources. A 308 is cached hard by browsers, so anyone who had ever hit that path would keep being sent away even after removing the rule.
A company with require2fa plus a pre-provisioned SSO account locked that user out of the whole application. The layout redirected anyone without two-factor to the enrollment screen, and that screen calls twoFactor.enable, which demands the current password before it will issue a TOTP secret. An SSO-only account has never had one, so the form could not be satisfied and every other page bounced the user back to it. Those accounts get their second factor from the identity provider, which is the same reasoning the ssoExempt flag already encodes for the opposite direction, so the redirect now skips users with no credential account. The account lookup only runs on the path that can redirect. The rule moved into a pure function because it is a three-state combination and the state that matters is the one nobody reaches by accident. The layout is the wrong place to discover that.
Creating an account left no way to hand someone their first access. The obvious fix, generating a temporary password and passing it along, has three problems that no amount of care removes: the secret has to travel over a channel nobody controls, it keeps working until the user gets around to changing it, and the administrator knows it, so every action that account takes afterwards is deniable. An invitation link avoids all three. The invitee chooses their own password and nobody else ever learns it. Security properties, each covered by a test that fails without it: - 32 random bytes, stored only as a SHA-256 hash. Reading the table yields nothing that can be replayed. SHA-256 rather than bcrypt because the input already carries 256 bits of entropy: there is no dictionary to slow down, and the accept path stays cheap enough to rate-limit. - Single use, claimed with a conditional update inside a transaction, so two requests arriving together cannot both succeed. - 72-hour expiry, and issuing a new link retires the outstanding one, so a resend after a suspected leak actually closes the leak. - Redeeming sets the password, marks the address verified (clicking the link is the proof), and deletes every session that user already had. - Unknown, spent and expired tokens return one identical message, so the endpoint cannot be used to learn which tokens ever existed. - Issuing requires users:manage plus a fresh step-up, refuses a target in another company, and refuses outright when the company mandates SSO, where a password path would be dead on arrival but still worth stealing. - If the mail cannot be delivered the invitation is voided rather than left live with its only copy in a log. With no mail provider configured the link comes back to the caller, but never in production. The middleware needed /invite in its public list: the invitee has no session, so the link would otherwise bounce to a login form they cannot pass.
An administrator can now require somebody to replace their password, for a suspected compromise or a rotation. The requirement holds in the API guard, not only in a page redirect: a rule that lives in the layout is obeyed by browsers and ignored by everything else that can hold a session cookie. requireAuth and requirePermission both refuse with a distinct PASSWORD_CHANGE_REQUIRED code, which the client uses to route to the form rather than show a dead end. The flag is read from the session instead of querying per request, which is sound because setting it also deletes that user's sessions: a session saying false was necessarily issued after the flag was last cleared. That deletion is also what makes the requirement land immediately rather than whenever the session happened to expire. The password endpoint deliberately sits outside requireAuth, since that guard refuses the very user who needs it. It authenticates on its own terms and re-checks the current password, so a stolen cookie alone cannot lock the owner out, keeps the calling session alive and drops the others, and is rate-limited because it sits outside Better Auth's own limiter. In the dashboard layout the rotation gate runs before the two-factor gate. Enrolling an authenticator asks for the current password, so the other order would have users binding a second factor while still on the password an administrator handed them. Verified end to end against a running server: invite, accept, sign in, forced rotation, old session refused, API refused with the code, page redirected, password changed, access restored.
Two changes to how a forced password rotation is presented, and one to who may start one. The rotation and the mandatory two-factor enrollment now happen on /secure, a screen outside the dashboard layout: no sidebar, no other page to wander into while the requirement is outstanding. Both steps run in order on that one screen, password first, and it re-evaluates after each, so somebody who owes both sees them one at a time and lands on the dashboard when nothing is left. Previously the layout pushed such users at /security, a normal page of the application they were not otherwise allowed to use. Changing a password is no longer self-service: a user who wants a new one asks an administrator, who requires a rotation, which is what brings them to /secure. The endpoint enforces that itself rather than relying on the form being absent from the interface, since the request can be made with any HTTP client. The trade-off is real and deliberate: somebody who suspects their password is compromised now waits for an administrator instead of acting immediately. Verified against a running server: with a rotation pending, /dashboard and /security both redirect to /secure and the API refuses with its code; after the change the dashboard opens and /secure sends the user back to it; a change with no rotation pending is refused.
Only /access and /setup checked a permission before rendering. Every other dashboard page resolved one, then used it to decide what to draw: an address typed into the bar reached the page anyway, ran its queries and returned the company's data to a role that was never granted it. The rail hiding the entry was the only thing in the way, and an address bar goes around that. Pages now declare their permission in one registry, next to the one the API side has always had, and a coverage test fails the build when a page is missing from it or does not call the guard. An undeclared page resolves to no permission at all, which the guard treats as a refusal, so the failure mode of forgetting is a locked page rather than an open one. The guard sits in the page, before its first query, not only in the layout. A layout cannot stop a page from running: Next renders both, so a page that fetches first and is hidden afterwards has already put the data in the response. Measured before fixing, with the layout check alone: a role without employees:read still received every employee address in the HTML of /employees. The layout keeps its own check for the shell, and builds the rail from the same registry, so the menu advertises exactly what the server would allow. The e2e fixture for this is a role holding two permissions, because every preset holds all the ":read" ones and none of them could show the refusal. It asserts both halves: the page refuses, and the address that used to leak is absent from the HTML.
Security stops being a section of the product and becomes the account: its own entry at the foot of the rail, next to signing out, rather than a row among Employees and Alerts. What lives there is about the caller, not about the company's data, and more will follow. Reaching it now asks for a password or an authenticator code, whichever the account has. This is where a session turns into lasting access: a new authenticator, a passkey, an identity provider. A screen left unlocked should not be enough to add one, and a session cookie is a poor proof of who is holding the keyboard right now. The step-up endpoint accepts either factor for the same reason. Password only would have locked out SSO-only accounts, which have no password to re-enter and would then be the one kind of user unable to open their own account. The code is verified by the two-factor plugin against its own encrypted secret rather than reimplemented here, and both branches are rate-limited per user: five attempts a minute, since six digits are guessable in a way a password is not. The e2e specs clear the grant up front. It lives five minutes and is shared by everything that user does, so one spec proving its identity would carry the gate away for the next one.
Two guards asked for together, both about what an interface leaves available rather than what it displays. A ceiling on how fast one account can call the API, applied in the guard every authenticated route already passes through. Route-by-route limits were in place on the handful somebody thought about (sign-in, SSO resolution, scanning, redeeming an invitation, changing a password, proving identity again); the other thirty had none, and the next route added would have been the one nobody remembered. Sized for a dashboard, so a person never approaches it while a script hits it in seconds. Three endpoints keep a tighter ceiling of their own because each call is expensive elsewhere: testing and syncing a directory reach out to somebody else's LDAP or Azure, and a PDF export walks the whole company. The cost is one indexed upsert per API call, which is what buys the ceiling existing at all rather than living where it gets forgotten. Password fields now refuse copy, cut and drag. Masking hides characters from someone looking at the screen and does nothing about selecting the field and copying, which is how a password actually leaves a borrowed machine. Paste stays allowed: password managers fill fields that way, and blocking it pushes people towards passwords they can type from memory. Applied to every secret-bearing field, including the API keys and the SSO client secret, not only to the login form. The limit is not a claim about attackers with the machine: anyone at the keyboard can read the value from the devtools. It closes the casual path.
Verifying a password used to answer at two very different speeds. A wrong password ran bcrypt and took roughly a quarter of a second; an account with no password at all was refused in about a millisecond, because there was nothing to compare and the function returned early. That gap is readable over the network, and it answers a question nobody asked: which accounts have a password worth attacking. Both paths now do the same work. When there is no stored hash the supplied password is compared against a fixed one instead, which fails and costs what a real comparison costs, and the whole check is held to a floor so the surrounding work cannot leak the difference either. The floor is not what stops guessing. bcrypt at cost 12 already makes each attempt expensive, and the per-account ceiling in the API guard is what bounds how many attempts are possible at all. What this closes is the enumeration channel on top of them. Sign-in is deliberately left alone. Better Auth already hashes the supplied password when the address is unknown, when it has no credential account, and when that account has no password, so it is equalised already. Adding a second burn made it worse, and the measurement is the reason this commit says so: 590ms for an unknown address against 350ms for a known one with a wrong password, an inverted gap where there had been none. Removed, and re-measured at 347ms against 347ms.
Creating an account was the second way to hand out a role, and it checked neither of the two rules the reassignment route applies. It accepted any role of the company marked assignable, Administrator included. The consequence was full tenant takeover from a semi-trusted position. A holder of users:manage (the Security Manager preset, which deliberately lacks roles:manage) could read the Administrator role id, create an account carrying it at an address they own, mint a step-up grant with their own password since that endpoint asks only for a session, send themselves the invitation, choose a password, and sign in with every permission in the tenant. The reassignment route would have refused the same grant with a 403. Both routes now ask the same function: no-escalation first, refusing any role holding a permission the actor lacks, then a step-up when the role carries a crown jewel. One decision in one place, because the two copies already drifted once and the drift is the whole bug. The role pickers filter on a grantable flag the server computes for the caller instead of on isAssignable, so the interface stops offering a role the write path would refuse. That is presentation: the rule is enforced on the write paths, which is where it was missing. Found by the security review on this branch. The tests fail without the guard: removing the call makes both new cases go red, and the step-up case proves a gate rather than a blanket refusal by passing once the grant exists.
The pre-sign-in lookup answered from the User row, so "sso: true" meant "this address has an account here". Anyone could type addresses at a company running a verified provider and read back which people exist. Rate limiting slowed that walk down; it did not stop it, and enumeration is what precedes a targeted attack on the accounts it finds. The answer now comes from the address's domain alone. A domain reveals nothing personal, and reading it is safe because a provider only counts once domainVerified is set, which requires proving control of that domain through DNS: a company still cannot capture another company's users by claiming a domain it does not own, which is what the User lookup was there to prevent in the first place. Two verified providers on one domain now answer like none at all. Sending somebody to the wrong company's identity provider is worse than asking them for a password, and improvising that routing is not this endpoint's call to make. One consequence worth stating: routing keys off the address the person types, so a user whose address is not at a verified domain lands on the password path. A company whose people use several domains registers a provider per domain, which the schema already allows. Tests assert the property directly: an address nobody has ever created gets the same reply as a real one, and a real account at a domain with no provider is refused. Both go red against the previous implementation.
Brings the branch up to date before the pull request: develop had moved to next 16.3.0 and added a js-yaml override, and this branch was two commits behind. Two conflicts, both mechanical. package.json: the overrides block. develop added js-yaml, this branch added nanoid, and both floors are needed, so both are kept. The lockfile was regenerated from the merged manifest rather than hand-resolved, and the audit comes back clean. The login page: develop pinned the single form to method="post" while this branch had already split the page into an email step and a password step. Kept this branch's structure, which carries that same fix on all four of its forms, so the protection develop added is not lost in the merge. Verified on the merged tree, against next 16.3.0: 233 unit, 86 integration, 14 e2e, tsc, eslint, production build, npm audit clean.
The integration job carried only DATABASE_URL. That was enough until this branch, which writes SsoProvider rows: the Prisma extension that seals oidcConfig refuses to run without an encryption key, so every suite touching a provider failed at the first write with "DIRECTORY_ENCRYPTION_KEY missing or too short". Same placeholders the build and e2e jobs already use, and for the same reason they are safe there: the value only ever encrypts throwaway rows in a database this job creates and discards, and CI must never depend on a real secret to run its tests.
feat(sso): enterprise SSO over OIDC with invitations
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
WhiteMuush
changed the title
The page guards added this week closed the pages and left the endpoints behind them open. Eight read routes sat on requireAuth, meaning any valid session of the company, while the POST in the same file demanded a permission. A member with no role at all, the "no access pending" state the reassignment route sets deliberately, saw NoAccess everywhere and could still fetch the data: GET /api/reports/export?section=employees returned every employee with name, email, department, exposures, risk score and MFA status. Same shape for the exposure register and its evidence, the directory connections, the API credentials, the report schedules, the webhooks, and the per-employee breach detail. The reason it survived is in the coverage test: it only ever looked at mutating handlers, so a GET could skip the check and nothing noticed. It now also fails the build when a route that declares a permission falls back to a bare session check, with the two endpoints that authenticate on their own terms named explicitly rather than left to drift. Found by the security review run against the develop-to-main promotion, which is exactly what that review is for.
Signing in with a passkey is a complete primary authentication: the plugin creates the session and sets the cookie itself. It went through no policy check at all, because the before-hook only applied ssoMandatory to /sign-in/email and allowedAuthMethods to the three enrollment paths. So a company that turned on mandatory SSO to cut somebody's access, which is how offboarding and conditional access are meant to reach this app, still let that person in through the passkey button. Removing PASSKEY from the allowed methods only hid the button; the endpoint kept working. The assertion names the credential rather than the user, so the owner is resolved from the credential id before deciding, and both rules apply: mandatory SSO refuses a non-exempt user, and a method the company no longer allows refuses everyone.
fix(rbac): close the read APIs the page guards left open
The compliance job blocks AI attribution markers in added lines, and this plan states the rule by quoting the exact trailer it forbids. On a feature branch the line is old and never re-added, so nothing noticed; the develop-to-main promotion diffs against main, every line of develop counts as added, and the doc trips the check it is describing. Says the same thing without carrying the literal marker.
docs(plan): reword the trailer rule
CodeQL flags the unbounded form (js/polynomial-redos): each part uses + on the same negated class, so a long adversarial local part makes the engine backtrack polynomially before rejecting. It only surfaced now because the promotion diffs against main, where this route does not exist yet. The bounds are the addressing limits, so nothing legitimate is refused: 64 for the local part, 255 for the domain, 63 for the last label.
fix(rbac): bound the email pattern
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
2 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.
Deliberate release promotion, not feature work.
mainstill runs the old NextAuth stack; this brings it to the statedevelophas been integrating since 2026-07-25.107 commits, 212 files.
What lands
Verification
CI is green on the head of
develop: quality, unit, build, integration, e2e, plus CodeQL and the Security workflow withnpm audit --audit-level=high.A security review ran against the SSO branch before it merged and produced one finding, a privilege escalation through account creation, which is fixed in this promotion (
4b5090b).Known and open, carried into production
style-src 'unsafe-inline'in the CSP.main, so they should close on their own once this lands.