feat(server): move browser sign-in onto Better Auth sessions - #201
Conversation
Google sign-in now issues a Better Auth session, and every authenticated route resolves one. Credentials issued before this change keep working until they expire, so nobody is signed out by the deployment. **The published surface is an allowlist, not a catch-all.** Better Auth serves its whole API from one handler, including `/update-user` — a second Account profile writer that bypasses `UserDisplayNameSchema`, the suspension guard, and the authenticated, origin-checked `/api/v1/me`. Only the OAuth callback is published, because the provider redirects the browser straight to it; sign-in and sign-out run through OpenTag routes that call Better Auth server-side, so the request contract and the origin check stay ours. A regression asserts the unpublished paths 404 and fails naming the path that escaped. **Account invariants are enforced at issuance.** `onSessionCreating` runs before any session row exists and must throw to prevent one, so there is no window in which a session exists for an Account that is suspended or lacks the compatibility Workspace grant every authenticated route derives authority from. `ensureAccountReady` derives "was this Account just created?" from the absence of a grant, which makes it idempotent — Better Auth owns account creation on its own sign-in paths and cannot tell us. **Session resolution stays live.** Better Auth returns an identity; suspension and grants are still read from the database on every request, so revoking either takes effect immediately rather than at session expiry. Sign-out now revokes the session row instead of only dropping a cookie, which the stateless JWTs could never do, and clears the legacy cookies too because a browser mid-rollout can hold either credential. The origin check moved back behind the credential check: a request with no credential reads as unauthenticated again rather than forbidden. `publicOrigin` is threaded as the shared preHandler options object rather than a bare string, so the seven route modules that only ever used it to build that preHandler did not each grow a parameter.
baixiaohang
left a comment
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The Better Auth cutover currently leaves newly signed-in browsers unable to mutate or sign out, can restore revoked Workspace authority on the next session issuance, and does not migrate the Slack OAuth flow.
Risk level: B-high
- Path baseline:
packages/server/**-> B-low - Semantic lift: the change replaces browser session issuance and resolution across authenticated server routes -> B-high
PR summary
- Author / repo: bestony / first-tree-ai/opentag
- Problem: Browser sign-in needs revocable server-side sessions without signing out users who still hold pre-deploy JWT credentials.
- Approach: Google sign-in and authenticated-route resolution move to Better Auth while legacy credentials remain accepted; only the provider callback is publicly mounted, and authorization is intended to remain live through Account and Workspace-grant reads.
- Impacted modules: server auth integration, browser auth routes, authenticated route preHandlers, post-authentication provisioning, and auth integration tests.
Review findings
❌ 1. A fresh Better Auth sign-in never issues the opentag_csrf cookie required by every browser mutation. The new callback only forwards Better Auth's cookies, while requireBrowserMutationSecurity and the Web API still use the separate opentag_csrf / X-OpenTag-CSRF pair. A user who signs in after this deploy can read data, but profile/Agent/IM mutations and even /auth/browser/logout fail with 403. Please issue the OpenTag double-submit token as part of the Better Auth callback/session transition and cover a fresh sign-in followed by an authenticated mutation and logout. [R4, R5 / packages/server/src/auth/fastify-handler.ts:31]
❌ 2. ensureAccountReady equates "no active grant" with "new Account" and calls establishDefaultWorkspaceForNewAccount(..., true). A previously revoked Account therefore gets a brand-new Workspace and active Admin grant on its next Better Auth session, undoing the live revocation semantics already asserted by auth-migrations.test.ts. The code needs to distinguish first-time provisioning from an existing Account whose grants were revoked; absence of an active grant is not sufficient. [R3, R4 / packages/server/src/services/auth/post-authentication.ts:39]
❌ 3. Slack OAuth remains wired exclusively to the legacy access JWT. Its start route constructs a preHandler without the Better Auth resolver, and its callback passes only opentag_access into SlackOAuthService; a browser holding only the new session cannot start or complete Slack authorization. Please include both halves of this flow in the migration and add Better Auth-session coverage. [R5 / packages/server/src/api/slack-oauth.ts:75]
/sign-out catches session-deletion failures and still returns success, while this route always returns 204. That means the claimed server-side revocation can silently degrade to cookie-only logout during a database failure. Please either use a revocation path whose failure is observable to this route or explicitly narrow the guarantee and add diagnostics/coverage for the failure case. [R4 / packages/server/src/api/browser-auth.ts:210]
✅ The explicit callback allowlist is the right boundary for keeping Better Auth's profile-writing endpoints unpublished.
Action taken
- Submitted request changes.
… revoked authority Four gaps in the cutover, all reachable by a browser that signs in after it deploys. **A fresh sign-in could read but never write.** Better Auth's callback issues its own session cookie and knows nothing about OpenTag's double-submit token, which `requireBrowserMutationSecurity` demands of every browser mutation — profile edits, Agent and IM changes, and sign-out itself. The callback now issues that token whenever the response carries a session cookie, and only then, so a failed sign-in hands out nothing. **Revoked authority came back.** `ensureAccountReady` read "no active grant" as "new Account", so an Account whose grants were revoked was re-provisioned with a fresh Workspace and Admin grant on its next sign-in. Whether the Account has *ever* held a grant is the discriminator that separates first-time provisioning from revocation, and it keeps the call idempotent. **Slack authorization only understood the legacy token.** Its start route built a preHandler without the Better Auth resolver and its callback read `opentag_access` directly, so a browser holding only the new session could neither start nor complete it. Authentication moves out of `SlackOAuthService` — the route resolves the identity through the same resolver every other route uses and passes it in. The service compares identities, which is all it ever needed to do. **Sign-out promised more than it could observe.** Better Auth's `/sign-out` swallows a failed session delete and still reports success, so returning `204` unconditionally would let revocation degrade silently to a cookie-only logout. The route reads the session row back and fails loudly if it survived.
|
All four correct. Fixed in 1 — a fresh sign-in could read but never write. This one follows directly from a call I made badly. I kept the double-submit CSRF specifically to avoid touching the web app's security posture in the same PR that swaps the session mechanism, and then did not follow that decision through: Better Auth's callback issues its own session cookie and knows nothing about The callback now issues the token, and only when the response actually carries a session cookie — matched against 2 — revoked authority came back. Correct, and the sharper framing is the useful part: "has no active grant" and "is new" are not the same predicate. Whether the Account has ever held a grant is what separates first-time provisioning from revocation, and it keeps the call idempotent. A regression provisions an Account, revokes its grant, signs in again, and asserts the grant count stays at one and stays revoked. 3 — Slack authorization only understood the legacy token. Correct. Rather than teach the callback about a second cookie, authentication moved out of That is also the better seam. The old shape meant every new credential type had to be taught to a Slack service. 4 — sign-out promised more than it could observe. Correct, and I had written the stronger claim into the commit message. The route now reads the session row back after sign-out and fails loudly if it survived, so the guarantee is checkable rather than inherited from a call that swallows its own failure. Validation on One note in case it shows up in your run: |
baixiaohang
left a comment
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The follow-up fixes address the original findings, but the logout failure path still strands a live session after removing the browser's token, and the Slack callback's shared resolver omits the live Account check performed by the normal preHandler.
Risk level: B-high
- Path baseline:
packages/server/**-> B-low - Semantic lift: browser session resolution and revocation are core authentication/session behavior -> B-high
PR summary
- Author / repo: bestony / first-tree-ai/opentag
- Problem: Browser sign-in needs revocable Better Auth sessions while pre-deploy JWTs remain usable through rollout.
- Approach: Google sign-in issues Better Auth cookies, authenticated routes resolve either credential type, the provider callback remains the only published Better Auth endpoint, and live Account/Workspace state remains the authorization source.
- Impacted modules: server auth integration, browser and Slack OAuth routes, authenticated-route preHandlers, post-authentication provisioning, and auth integration tests.
Review findings
❌ 1. The revocation failure path copies Better Auth's clearing Set-Cookie headers before checking whether the session survived. Better Auth clears that cookie even when its caught database delete failed, and the Fastify error handler does not remove headers already placed on the reply. The resulting 500 therefore leaves the server-side session live while deleting the browser's only copy of its token; a retry cannot identify and revoke that survivor, so a stolen copy remains usable until expiry. Delay propagating the sign-out cookies until deletion is verified (or otherwise preserve a retryable credential on failure), and cover the survivor response headers as well as the status. [R4 / packages/server/src/api/browser-auth.ts:217]
❌ 2. resolveAuthenticatedUserId does not actually preserve the preHandler's live Account invariant for Better Auth credentials: it returns session.user.id directly instead of calling getActiveUserById. An Account suspended after session issuance can therefore pass the Slack callback identity check; SlackOAuthService consumes the nonce and exchanges the provider code before configure reaches the later resource-authority check. Restore the active-Account check before those side effects and add a suspended Better Auth-session callback regression. [R1, R4, R5 / packages/server/src/plugins/user-auth.ts:37]
BetterAuthConfig.onSessionCreating and the integration-test comment still state that no such session can exist. Please align those core-invariant comments with the identity-versus-authority behavior now enforced by the code. [R1 / packages/server/src/auth/better-auth.ts:21]
✅ The new callback CSRF issuance, historical-grant discriminator, and credential-neutral Slack route wiring resolve the three original functional blockers.
Action taken
- Submitted request changes on the new head.
…t revoke The revocation check added last round ran after Better Auth's clearing cookies were already on the reply, and Fastify keeps headers placed before an error. A failed delete therefore returned `500` having destroyed the browser's only copy of a token whose session was still live: nothing left to retry the revocation with, and a stolen copy usable until expiry. Nothing reaches the reply now until the session is confirmed gone. `resolveAuthenticatedUserId` also claimed to give the same answer as the preHandler and did not: it returned the id on the session without the live Account read. An Account suspended after issuance therefore passed the Slack callback's identity check, and `SlackOAuthService` consumed the nonce and exchanged the provider code before any authority check ran. It resolves through `getActiveUserById` now, so suspension is enforced before those side effects. Both are covered by regressions that fail on the previous behaviour. Also corrects two comments that described an invariant the code deliberately does not hold. Session issuance is a question about identity, not authority: an Account whose grants were all revoked can still sign in and see that it has no Workspace, and re-provisioning it at issuance would hand the revoked authority back.
|
All three correct. Fixed in 1 — a failed sign-out stranded a live session. This is worse than the problem I was fixing, and I introduced it in the fix. Copying the clearing cookies before the survivor check meant a failed delete returned Nothing reaches the reply now until the session is confirmed gone. The regression drives the route with a surviving session and asserts the response carries no 2 — Worth noting the shape of this one: I extracted that helper in the previous round precisely so routes authenticating outside the preHandler would get the same answer, and then wrote a version that did not. The regression asserts the same request resolves before suspension and rejects with 3 — comments overstated the invariant. Correct, and the distinction is worth stating explicitly rather than just softening the wording: session issuance is a question about identity, not authority. An Account whose grants were all revoked can still sign in and see it has no Workspace; re-provisioning at issuance would hand the revoked authority straight back — which is the bug you caught last round. Validation on |
baixiaohang
left a comment
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: approve
- Rationale: The latest head resolves every blocking authentication, authorization, Slack OAuth, and logout-atomicity issue raised in the prior review rounds, with focused regression coverage and green CI.
Risk level: B-high
- Path baseline:
packages/server/**-> B-low - Semantic lift: browser session issuance, resolution, and revocation are core authentication/session behavior -> B-high
PR summary
- Author / repo: bestony / first-tree-ai/opentag
- Problem: Browser users need revocable server-side sessions without invalidating credentials issued before the migration.
- Approach: Google sign-in now issues a Better Auth session while legacy JWTs remain accepted; only the provider callback is public, and live Account and Workspace state remains authoritative on each request.
- Impacted modules: server auth integration, browser and Slack OAuth routes, authenticated-route preHandlers, post-authentication provisioning, and auth integration tests.
Review findings
✅ 1. Fresh Better Auth sign-in now issues the OpenTag double-submit CSRF token only when a real session cookie is present, preserving the mutation and logout boundary.
✅ 2. First-time provisioning now uses grant history rather than active-grant absence, so a later sign-in cannot restore revoked Workspace authority.
✅ 3. Slack OAuth accepts both credential generations and performs the live Account suspension check before nonce consumption or provider exchange side effects.
✅ 4. Logout withholds clearing cookies until session deletion is verified, preserving a retryable credential when revocation fails.
resolveAuthenticatedUserId and the regular preHandler retain some duplicated credential-resolution logic. Their behavior is aligned and covered here, so this is non-blocking; consolidating them before adding another credential type would reduce future drift risk.
Action taken
- Approved current head
b331a34.
…er Auth sessions (#203) Stage 4 of the Better Auth migration: the CLI's credential becomes a Better Auth session, and development sign-in stops being the one path that produces a session nothing can revoke. ## The CLI The connect-code exchange hands out a session row instead of a signed pair, so a CLI credential can finally be withdrawn rather than only waited out. Nothing else changes shape. Every caller already speaks `AuthTokenProvider`, so Better Auth implements that interface and the exchange, refresh, and request paths are untouched — as is the four-field response the CLI stores and the exactly-four-keys validator that reads it back. **A CLI built before this keeps working without an upgrade**, which matters because the production `open-tag` package only ships on a `vX.Y.Z` tag. `accessToken` and `refreshToken` carry the same token, because a session is not a pair: it is one credential the server can revoke, and re-presenting it is what extends it. The pair existed to limit the damage of a leaked access token that could not be withdrawn, which no longer describes the system. Verification tries the session store first and falls back to the legacy signature, so a CLI that has not reached the server since the cutover still authenticates. Issuance only ever produces a session, which makes refresh the step that quietly moves such a credential across — no re-login, and the old access token keeps working until it expires. ## Development sign-in Development sign-in went through `AuthService.issueTokensForUser`, so the change above repointed it at Better Auth too — and it was writing the result into OpenTag's own `opentag_access` cookie. That combination is worse than either half. The token still authenticates, because the legacy cookie path falls through to the same session store, so the sign-in looks correct. But `getSession` reads Better Auth's cookie, not this one, so sign-out finds nothing to revoke and clears cookies while the session row lives on — the failure #201 removed for Google, now with a session lifetime rather than fifteen minutes behind it. Every browser refresh minted another orphan row on top. Better Auth mints it instead, through a plugin endpoint that holds the request context `setSessionCookie` needs. The endpoint takes no input: the Account comes from a resolver fixed at construction, so it cannot be aimed at another Account even by a caller that reaches it — and reaching it takes doing, since it is absent from the published path allowlist and the plugin is only registered when development sign-in is configured, which `parseServerConfig` already restricts to a loopback `OPENTAG_ENV=dev` server. The route keeps its own loopback fences and rate limit ahead of all of this. The two commits are not independently revertable: reverting only the second puts a session token back in the legacy cookie. ## On the tests The existing connect-code contract tests construct `AuthService` with the legacy provider directly, so they would have passed against a completely unwired change. The new coverage exercises the composition the server actually runs: exchange yields a session row that authenticates as a bearer token and stops working the moment it is deleted, and a legacy refresh token comes back as a session. For development sign-in the assertion that matters is the negative one — that no `opentag_access` cookie is set — because that is precisely the mistake that hides itself. Integration coverage drives the real endpoint against Postgres and asserts the session row appears, resolves through `getSession`, and is gone after sign-out. ## Deployment No schema change, no environment variable, no manual step. Server and `open-tag-staging` go out together on merge; the production CLI waits for the next tag and is covered by the bridge until then. Stage 5 removes `AuthTokenService`, the hand-rolled OAuth modules, and `OPENTAG_JWT_SECRET` — and must not go out until the production CLI has shipped and been adopted, since it is the one step no rollback recovers. ## Validation `pnpm check`, `pnpm build`, `pnpm typecheck`, `pnpm test` (1274), and `pnpm --filter @opentag/server test:integration` (209) all pass.
2 participants
Summary
Third step of the Better Auth migration, and the first that changes what a request does. Google sign-in now issues a Better Auth session and every authenticated route resolves one. Credentials issued before this deploys keep working until they expire, so the rollout signs nobody out.
Until now the browser held a stateless HS256 JWT with no server-side session row, which meant logout could not revoke anything and the only kill switch for an Account was
users.suspended_atre-read on each request.The published surface is an allowlist
Better Auth serves its entire API from one handler. Reviewers of #196 showed what that costs:
/update-userwrites Better Auth'snamestraight intousers.display_name, bypassingUserDisplayNameSchema, the suspension guard, and the authenticated, origin-checked/api/v1/me.So only the OAuth callback is published — the provider redirects the browser straight to it and nothing else can serve it. Sign-in and sign-out run through OpenTag routes that call Better Auth server-side, keeping the request contract, the origin check, and the response shape ours.
better-auth-surface.test.tsasserts the unpublished paths 404 and names the one that escaped if that regresses. Restoring a catch-all fails it onget-session.Account invariants are enforced at issuance, not after it
onSessionCreatingruns before any session row exists and must throw to prevent one. There is therefore no window in which a session exists for an Account that is suspended, or that lacks the compatibility Workspace grant every authenticated route derives authority from.ensureAccountReadyderives was this Account just created? from the absence of a grant. Better Auth owns account creation on its own sign-in paths and cannot tell us, and deriving it makes the call idempotent: a returning Account is a no-op, one that never received a grant gets one.Resolution stays live
Better Auth returns an identity. Suspension and Workspace grants are still read from the database on every request, exactly as the legacy path does, so revoking either takes effect immediately rather than at session expiry.
Other behaviour changes
403instead of401; the existing contract test caught it./api/v1/*business routes, which are not Better Auth endpoints, so Better Auth's own protection does not cover them.Not in this step
The dev sign-in bypass and
POST /api/v1/auth/browser/refreshstill issue and accept legacy JWTs. They keep working through the same preHandler and move in the CLI step, which is where the legacy token service is retired.Testing
pnpm check,pnpm build,pnpm typecheck,pnpm testpnpm --filter @opentag/server test:integration— 191 passingbetter-auth-surface.test.ts: the allowlist holds, and the forwarded URL is built from the configured origin rather than the requestHost.AUTH_USER_SUSPENDED.Not covered by automated tests: a live Google sign-in, which needs OAuth credentials.
Breaking changes
None for users. Operationally, the Google OAuth client needs the new redirect URI before this deploys — see below.
Deployment prerequisite
Add
https://<app>/api/v1/auth/callback/googleto the Google OAuth client's authorized redirect URIs, and keep the existing/api/v1/auth/google/callbackuntil the legacy path is retired. Better Auth's callback path is<basePath>/callback/<provider>, so sign-in fails at the provider without it.Checklist
pnpm check,pnpm build,pnpm typecheck, andpnpm testpass.