feat(server): issue CLI and development credentials as revocable Better Auth sessions - #203
Conversation
The connect-code exchange now hands out a session row instead of a signed pair, so a CLI credential can finally be revoked 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. `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. 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.
Development sign-in went through `AuthService.issueTokensForUser`, which the previous commit repointed at Better Auth. It therefore started handing out a session token — and writing it 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 exact failure the browser work removed for Google, and now with a session lifetime rather than fifteen minutes behind it. Every browser refresh minted another orphan row on top. Better Auth mints the session instead, through a plugin endpoint that holds the request context `setSessionCookie` needs. Development sign-in is now the same credential a Google sign-in is: visible to `getSession`, ended by sign-out, renewed rather than duplicated, and running the same `session.create` hook. 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 requires a loopback `OPENTAG_ENV=dev` server. The route keeps its own loopback fences and its rate limit ahead of all of this. `DevBrowserAuthService` now answers only which Account, which is all it ever knew; minting was never its business. A misconfigured email is reported as the answerable failure it is rather than thrown, so it is not logged as an internal server error among real ones.
baixiaohang
left a comment
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The CLI session direction is sound, but the current migration can still strand Better Auth sessions in legacy browser cookies and refreshes stateful credentials by minting additional live sessions without retiring the old ones.
Risk level: B-high
- Path baseline:
packages/server/**without a listed high-risk service path -> B-low - Semantic lift: changes core authentication/session issuance, refresh, compatibility fallback, and browser logout behavior -> B-high
PR summary
- Author / repo: bestony / first-tree-ai/opentag
- Problem: CLI users and local-development browsers need credentials that can be revoked immediately, while clients and browser sessions created before the Better Auth cutover must continue working through the rollout.
- Approach: New CLI issuance moves behind the existing token-provider interface onto Better Auth sessions, legacy JWT verification remains as a fallback, and development sign-in asks a private Better Auth endpoint to issue the canonical session cookie.
- Impacted modules: server auth composition, CLI credential exchange/refresh, development and legacy browser auth routes, Better Auth integration, and auth integration tests.
Review findings
❌ 1. The shared issuer cutover also changes two legacy-cookie writers that were not migrated. POST /auth/browser/refresh still calls authService.refresh() and writes the result with setBrowserSessionCookies; after this PR that result is a Better Auth session token, so a legacy browser refresh stores it only in opentag_access / opentag_refresh. The normal preHandler accepts it through the fallback, but getSession and sign-out cannot see or revoke its session row—the exact orphan-session failure fixed for development sign-in here. The retained legacy Google callback has the same outcome for an in-flight pre-deploy OAuth flow. Please either keep legacy issuance for every legacy-cookie path or move each writer onto Better Auth's cookie, and cover legacy browser refresh → authenticated session → logout with the row gone (plus the retained callback if it remains). [R4, R5 / packages/server/src/api/browser-auth.ts:223, packages/server/src/api/browser-auth.ts:230]
❌ 2. Refreshing an already-migrated CLI credential creates a second session and leaves the presented one live. AuthService.refresh() verifies the token and then calls issueTokensForUser(), while BetterAuthSessionTokens.issuePairForUser() always calls createSession(); nothing updates or deletes the existing row. Because access and refresh fields contain the same credential, the CLI replaces its file with the new token but every copied or leaked old token remains usable until its original (and potentially rolling) expiry, and repeated refreshes accumulate rows. The stateful provider needs an explicit refresh/rotation operation that either extends the same session or atomically revokes the old token when issuing a replacement; please add a migrated-session refresh regression that asserts the intended old-token behavior and active-row count. [R3, R4 / packages/server/src/services/auth/auth-service.ts:108, packages/server/src/auth/session-tokens.ts:23]
❌ 3. BridgedSessionTokens.#either() falls back to JWT verification for every exception from the session provider, not only an explicit session miss. A database/adapter failure during findSession() is therefore swallowed and can become a misleading AUTH_INVALID_TOKEN (or briefly accept a legacy credential if the second check succeeds) instead of preserving the infrastructure failure. Restrict the fallback to the known invalid/not-found credential case and let transient/internal session-store failures propagate with their original classification. [R4 / packages/server/src/auth/session-tokens.ts:84]
✅ 5. The development route now delegates cookie issuance to Better Auth behind the existing loopback and surface allowlist, and the negative opentag_access assertion protects the specific logout invisibility regression.
Action taken
- Submitted request changes on
aa5be9dbbc907c2fed07102227d364e09f2c530d.
…ces it The browser refresh bridge had the same defect the development route did, for the same reason: repointing `AuthService` at Better Auth made it mint a session and then write it into `opentag_access` and `opentag_refresh`. The result still authenticated through the legacy fallback, so it looked right, while `getSession` could not see it and sign-out had nothing to revoke. Only a browser that has not signed in since the cutover still holds that cookie, so refreshing it is that browser's one chance to move across. It is now spent on a Better Auth session: the same revocable credential a Google sign-in produces, and one that keeps working when the legacy secret is retired. The retired pair is cleared on the way out, and only after the replacement is on the reply, so a refused upgrade leaves the browser able to try again. Establishing that session needs Better Auth's own request context, so it goes through a second endpoint alongside the development one — both unpublished, both reachable only from the OpenTag route that already decided the request may have a session. This one is not a way to sign in without a credential: the caller must present a refresh token that still verifies, checked against the legacy provider alone and followed by a live Account read, so a suspended Account cannot refresh its way back in. The two share `internal-sign-in.ts`, which also reports an `AuthServiceError` as the decision it is rather than letting it escape as a 500. The route restates what comes back in OpenTag's envelope, keeping the status and code: forwarding Better Auth's shape verbatim would reach the client as an unrecognized failure and flatten to `AUTH_INVALID_TOKEN`, telling a suspended Account to sign in again, and again.
|
Pushed Repointing It now spends that refresh on a Better Auth session instead. Only a browser that has not signed in since the cutover still holds the cookie, so this is that browser's one chance to move across; reissuing another legacy pair would leave it on a credential stage 5 removes. The retired pair is cleared only after the replacement is on the reply, so a refused upgrade leaves the browser able to retry. The upgrade endpoint is not a way to sign in without a credential: the caller must present a refresh token that still verifies, checked against the legacy provider alone and followed by a live Account read. Both it and the development endpoint stay off the published allowlist, asserted in Validation on |
baixiaohang
left a comment
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The new browser bridge now places the replacement in Better Auth's cookie, but the exchange is replayable and can mint orphan sessions under normal concurrent refreshes; the migrated CLI refresh and compatibility-error blockers also remain, and the retained legacy Google callback still uses the wrong cookie writer.
Risk level: B-high
- Path baseline:
packages/server/**without a listed high-risk service path -> B-low - Semantic lift: changes core authentication/session issuance, refresh, compatibility fallback, and browser logout behavior -> B-high
PR summary
- Author / repo: bestony / first-tree-ai/opentag
- Problem: CLI users and pre-cutover browsers need to move onto revocable Better Auth sessions without reauthentication, while legacy credentials remain usable during rollout.
- Approach: New CLI credentials are Better Auth sessions; legacy JWT verification remains bridged; development sign-in and legacy browser refresh now invoke private Better Auth endpoints so the replacement is written into Better Auth's own cookie.
- Impacted modules: server auth composition, CLI credential exchange/refresh, browser sign-in/refresh/logout, Better Auth plugins, compatibility cookies, and auth integration tests.
Review findings
❌ 1. The new legacy upgrade is described as “spent,” but it only verifies a stateless refresh JWT and clears the browser's cookie; nothing makes the exchange one-time or idempotent. BrowserApi.fetchWithRefresh() also has no shared refresh-in-flight guard, so two requests that receive 401 concurrently can both send the same legacy cookie, and each call to establishSession() creates a distinct row. The last response wins the browser cookie, while the other row becomes invisible to sign-out and survives it—the same orphan-session class this fix is meant to remove. Please make one legacy credential converge atomically on one replacement (or otherwise ensure every row minted by the exchange is revocable together), and cover concurrent/replayed upgrades followed by logout with no survivor. [R4 / packages/server/src/auth/internal-sign-in.ts:47, apps/web/src/api.ts:316]
❌ 2. Refreshing an already-migrated CLI credential still creates a second session and leaves the presented one live. AuthService.refresh() verifies the token and then calls issueTokensForUser(), while BetterAuthSessionTokens.issuePairForUser() always calls createSession(); nothing updates or deletes the existing row. Because access and refresh fields contain the same credential, the CLI replaces its file with the new token but copied or leaked old tokens remain usable until expiry, and repeated refreshes accumulate rows. The stateful provider needs an explicit refresh/rotation operation that extends the same session or atomically retires the old token; please cover the old-token behavior and active-row count after a migrated-session refresh. [R3, R4 / packages/server/src/services/auth/auth-service.ts:108, packages/server/src/auth/session-tokens.ts:23]
❌ 3. BridgedSessionTokens.#either() still falls back to JWT verification for every exception from the session provider, not only an explicit session miss. A database/adapter failure during findSession() is therefore swallowed and can become a misleading AUTH_INVALID_TOKEN (or accept a legacy credential if the second check succeeds) rather than preserving the infrastructure failure. Restrict fallback to the known invalid/not-found credential case and let transient/internal session-store failures propagate. [R4 / packages/server/src/auth/session-tokens.ts:84]
❌ 4. The retained legacy Google callback remains on the shared authService issuer and still writes result.tokens with setBrowserSessionCookies(). An OAuth flow started just before deployment therefore returns after this cutover with a Better Auth session token in opentag_access / opentag_refresh; requests authenticate through the fallback, but getSession and logout cannot see or revoke the row. The new browser-refresh endpoint resolves the main sibling from the prior finding, but this retained deployment bridge needs either legacy issuance or Better Auth cookie establishment too, with an in-flight callback → logout regression. [R5 / packages/server/src/api/browser-auth.ts:198, packages/server/src/index.ts:296]
Action taken
- Submitted request changes on
b97b97cfe58674cbd61c2e04bf22797db087f746.
yuezengwu
left a comment
yuezengwu
left a comment
There was a problem hiding this comment.
Recommendation: request changes
The Better Auth direction is viable, but this exact head still permits orphaned and replayable sessions, does not rotate stateful CLI credentials, masks session-store failures, and silently changes the CLI lifetime contract.
Risk level: B-high. This changes core authentication, refresh, compatibility fallback, browser migration, and logout behavior.
Blocking findings
-
The legacy browser upgrade is replayable and does not converge on one replacement session.
legacyUpgradePluginverifies the stateless refresh token and always callscreateSession(); it never consumes, records, or deduplicates the presented credential.BrowserApi.fetchWithRefresh()also has no shared refresh-in-flight guard, so ordinary concurrent401responses can send the same cookie twice. Both calls mint live rows, the last cookie wins, and sign-out can revoke only that row. Make one legacy credential atomically converge on one replacement, or otherwise ensure every minted row is revoked together, and cover concurrent/replayed upgrade followed by logout. [packages/server/src/auth/internal-sign-in.ts:47,apps/web/src/api.ts:316] -
Refreshing a migrated CLI session creates a new session and leaves the presented one live.
AuthService.refresh()verifies the token and callsissueTokensForUser(), whileBetterAuthSessionTokens.issuePairForUser()unconditionally creates another row. Because access and refresh fields contain the same credential, the CLI discards its reference to the old token while copied or leaked copies remain valid, and repeated refreshes accumulate active rows. Extend the same session or rotate it atomically, then assert old-token behavior and the active-row count. [packages/server/src/services/auth/auth-service.ts:108,packages/server/src/auth/session-tokens.ts:23] -
BridgedSessionTokens.#either()falls back for every exception from the session provider. A database/adapter failure infindSession()is therefore swallowed and reclassified as an invalid credential, or a legacy credential is accepted after the primary store failed. Fall back only on the explicit session-miss/invalid-credential result and preserve infrastructure failures. [packages/server/src/auth/session-tokens.ts:84] -
The retained legacy Google callback still uses the shared issuer and
setBrowserSessionCookies(). An OAuth flow started before deployment can therefore finish after this cutover, receive a Better Auth session token inopentag_access/opentag_refresh, authenticate through the fallback, and remain invisible to Better AuthgetSessionand sign-out. Keep legacy issuance for this bridge or establish the replacement through Better Auth, with an in-flight callback to logout regression. [packages/server/src/api/browser-auth.ts:198,packages/server/src/index.ts:304] -
The new CLI credential lifetime silently ignores both documented token TTL settings.
createBetterAuth()supplies no session lifetime, so the installed Better Auth configuration resolves toexpiresIn=604800andupdateAge=86400, whileDEVELOPMENT.mdstill promisesOPENTAG_ACCESS_TOKEN_TTL_SECONDS=900andOPENTAG_REFRESH_TOKEN_TTL_SECONDS=2592000. New CLI credentials consequently change from a 15-minute access / 30-day refresh pair to one 7-day token: an access-only disclosure lasts much longer, and an inactive CLI cannot refresh after that same token expires. Define the intended lifetime and renewal contract, wire the relevant configuration, update both language docs, and add boundary coverage. [packages/server/src/auth/better-auth.ts:61,packages/server/src/auth/session-tokens.ts:23,DEVELOPMENT.md:383] -
The source proposal conflicts with the current durable runtime contract. At Context Tree commit
73b6f0b8b2a2ebc76e4944afc2e5555aff3cdf52,opentag/system/server-client-runtime-boundary.mdstill states that CLI/daemon sessions use access and refresh tokens and that access credentials are JWTs. Resolve that contract explicitly with the source-backed Tree update before this migration lands.
The loopback development route, unpublished Better Auth surface, live Account check, and negative legacy-cookie assertion are good changes, but they do not offset the blockers above.
Validation on b97b97cfe58674cbd61c2e04bf22797db087f746
git diff --check e82d00a...HEADpnpm checkpnpm buildpnpm --filter @opentag/server typecheckpnpm --filter @opentag/server test— 281/281pnpm --filter @opentag/server test:integration— 211/211- Live CI: all six reported checks successful
Action taken: requested changes on the exact head above.
Four consequences of moving Account credentials from signatures to rows, each of which the previous commits left half-made. **Refresh withdraws what it replaces.** Access and refresh carry the same token, so a refresh replaces the only credential the CLI has — but issuance alone left the presented one valid until its own expiry. Revoking what a client currently holds therefore did not lock out a copy taken before its last refresh, and every refresh left another live row behind. Rotation is now an operation on the provider rather than an accident of reissuing: the replacement exists before the old token is withdrawn, so a failure between them costs a stale row instead of a signed-out client. A signature cannot be withdrawn, so the legacy provider still just reissues, and says so. **One legacy credential converges on one session.** A stateless refresh token has nothing to consume, so nothing stopped it being exchanged twice — a replay, or two requests from one browser that met a `401` together. Each exchange minted its own row, and every row but the last was invisible to the browser that made it, surviving the sign-out meant to end it: the orphan-session failure this endpoint exists to remove, reintroduced by concurrency. The exchange is now recorded against the credential and serialized on it, so a later caller is handed the session that already exists. The browser also collapses concurrent refreshes into one, which is worth doing even though the server no longer depends on it. Better Auth's `reserveVerificationValue` is the natural gate and does not work here: its first-writer-wins comes from writing a derived primary key, and `auth_verifications.id` is a `uuid` column with a default, so the derived id does not survive the insert and every caller reserves successfully. The lock is what actually serializes it. **The bridge falls back only on a rejected credential.** Any exception from the session store was being read as "not mine", so a failing store could be reported as an invalid token — or could let a legacy credential in behind its back, an outage silently widening what the server accepts. Only an explicit `AUTH_INVALID_TOKEN` now means the credential might be legacy. **The retained Google callback keeps the legacy issuer.** It only ever completes a flow that started before this revision deployed, and it writes its result into the legacy cookies; the shared issuer would put a session token there, which authenticates through the fallback but is invisible to `getSession` and so beyond what sign-out can revoke. A pre-cutover flow now finishes exactly as it would have, and that browser moves across on its next refresh. Each fix is pinned by a regression that fails without it: the rotation test asserts the old token stops working and one row remains, the convergence test fails with two rows when the lock is removed, and the wiring assertion reports `BridgedSessionTokens` where it wants the legacy provider.
|
Both reviews are right, and the two of you converged on the same four independently. All of them are fixed in 1 — the legacy upgrade was replayable. A stateless refresh token has nothing to consume, so nothing stopped it being exchanged twice: a replay, or two requests from one browser that met a The exchange is now recorded against the credential and serialized on it, so a later caller is handed the session that already exists rather than a second one. The browser also collapses concurrent refreshes into one, which is worth having even though the server no longer depends on it. Worth recording what did not work: 2 — refresh created a session and left the presented one live. Access and refresh carry the same token, so a refresh replaces the only credential the CLI has. Revoking what a client currently holds therefore did not lock out a copy taken before its last refresh — most of the point of a credential the server can withdraw — and every refresh left another live row behind. Rotation is now an operation on the provider: the replacement is issued, then the presented token is withdrawn, in that order, so a failure between them costs a stale row rather than a signed-out client. A signature cannot be withdrawn, so the legacy provider still just reissues and says so. 3 — the bridge fell back on any exception. A failing session store could be reported as an invalid credential, or could get a legacy credential admitted behind its back, which means an outage silently widening what the server accepts. Only an explicit 4 — the retained Google callback used the shared issuer. It only ever completes a flow that started before this revision deployed, and writes into the legacy cookies; the bridged issuer put a session token there, authenticated through the fallback and invisible to 5 — the CLI lifetime changed silently, and this one neither I nor the first review caught. No The docs now also state the half of the trade that got worse: a disclosed credential is usable for the session lifetime rather than the old fifteen-minute window. What made that window necessary was that its thirty-day refresh partner could not be revoked at all, and a session can be — immediately. 6 — Context Tree. Correct, and I cannot make that change from this repository: On the tests. Each fix is pinned by a regression I checked fails without it, rather than assuming: removing the lock gives two rows, restoring the shared issuer reports One of those tests also had a defect of its own worth mentioning, because it is the kind that hides: it built both rejecting promises up front and awaited them in turn, so the second rejected while nothing was awaiting it. The suite reported 214/214 passing and exited non-zero. Fixed by starting them one at a time. Validation on |
baixiaohang
left a comment
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The prior fallback and legacy-callback blockers are fixed, but the new serialization can deadlock the database pool and does not atomically record the replacement, CLI rotation is still replayable, and the advertised browser session TTL is not wired through the browser's CSRF lifetime.
Risk level: B-high
- Path baseline:
packages/server/**without a listed high-risk service path -> B-low - Semantic lift: changes core authentication/session issuance, rotation, compatibility migration, and browser lifetime behavior -> B-high
PR summary
- Author / repo: bestony / first-tree-ai/opentag
- Problem: CLI users and pre-cutover browsers need to move onto revocable Better Auth sessions without reauthentication or orphaning credentials during refresh, replay, and logout.
- Approach: Session rotation becomes a provider operation, legacy browser upgrades are keyed and serialized, compatibility fallback is narrowed to explicit invalid credentials, the retained OAuth callback keeps legacy issuance, and a configurable 30-day session lifetime replaces the old refresh-token lifetime.
- Impacted modules: server auth composition, CLI credential refresh, legacy browser upgrade, Better Auth sessions, web refresh coordination, configuration, and authentication integration tests.
Review findings
❌ 1. The legacy-upgrade lock is held by one transaction connection, but every protected operation uses Better Auth's internalAdapter, which is backed by the base database, not the transaction passed to serialize. With the default pool size of 10, ten same-token requests can occupy one connection in the lock holder plus nine connections waiting for that lock; the holder then needs an eleventh connection to run findVerificationValue/createSession and the pool stalls. The split also means the session insert and verification marker are not atomic: a marker-write failure leaves an unrecorded live session, and retry mints another. Run the protected reads/writes on the lock-owning transaction (or use a primitive that does not reserve waiting pool connections), and cover pool-sized concurrency plus failure between session and marker persistence. [R4 / packages/server/src/index.ts:300, packages/server/src/auth/internal-sign-in.ts:74, packages/server/src/db/client.ts:5]
❌ 2. Stateful CLI rotation is still check-then-act rather than atomic or convergent. Two refreshes can both verify the same old session before either deletes it; each then creates a replacement, both deletes succeed/no-op against the old token, and two independent live sessions survive. A revoke/delete racing after verification can likewise be followed by rotate() recreating access. The sequential regression only proves the uncontended case. Serialize/consume the presented session in the same atomic operation as replacement (or make replay converge on one replacement), and assert concurrent refresh and revoke-vs-refresh outcomes. [R4 / packages/server/src/services/auth/auth-service.ts:108, packages/server/src/auth/session-tokens.ts:34, packages/server/src/__tests__/integration/better-auth.test.ts:321]
❌ 3. OPENTAG_SESSION_TTL_SECONDS is documented as the browser-and-CLI session lifetime, but the composition passes only refreshTokenTtlSeconds into browser auth. Consequently the Google callback, development sign-in, and legacy upgrade still give the CSRF cookie the legacy refresh-JWT TTL, and registerBetterAuthRoutes does the same. Any deployment that sets the new session TTL differently can keep a Better Auth session while losing the CSRF cookie, leaving an authenticated browser unable to mutate or sign out; rolling session renewal can produce the same mismatch even when the initial defaults match. Thread the session TTL through every canonical-session cookie path and renew the CSRF lifetime with the session, with an unequal-TTL/renewal regression. [R1, R5 / packages/server/src/index.ts:338, packages/server/src/app.ts:197, packages/server/src/api/browser-auth.ts:157]
❌ 4. The durable runtime contract still says CLI/daemon credentials are an access/refresh pair and access credentials are JWTs, while this source change makes them one Better Auth session and defines a stage-5 compatibility boundary. The author has acknowledged this remains unresolved outside this repository. Pair the source change with the source-backed Context Tree update before merge so the canonical contract and runtime do not land in conflict. [R1, R5 / OpenTag Server and Client Runtime Boundary]
✅ 5. Fallback now occurs only for explicit AUTH_INVALID_TOKEN, and the retained pre-deploy Google callback receives the legacy issuer, resolving those two prior blockers.
Action taken
- Submitted request changes on
eb708535d197c42bfce8b338e1795a4721b3bcde.
Three consequences of the last round, two of which it introduced. **The exchange lock could stall the pool.** It was held by one connection while every waiter held another, and the holder still needed a connection of its own to do the work — Better Auth's adapter runs on the base pool, not the transaction the lock lived in. Ten concurrent exchanges of one credential exhaust a pool of ten and the server stops answering. The split also meant the session and the record of it were written separately, so a failure between them left a live session nothing had recorded. The record is the gate now: one insert whose conflict branch returns the row already there, so the first writer wins and a later caller withdraws the session it had just created and hands back the winner's. No lock, nothing waiting on one, and the session is created before the statement that decides whether it counts — a failure before that leaves a session handed to nobody, which expires on its own. First writer rather than last, deliberately. The browser keeps whichever `Set-Cookie` arrives last, which is not necessarily the exchange that won, so converging on one token is what stops a browser being left holding a cookie for a row that was deleted. **Rotation was check-then-act.** Verifying and then replacing decides on stale information: two refreshes could verify the same token and each go on to mint a session, and a revocation landing between the two would be undone by the replacement. The withdrawal is the gate instead — a conditional delete, which exactly one caller can win — and the replacement is issued only to whoever won it. This reverses the earlier ordering argument: withdrawing first means a failure signs the client out, and that is the direction to fail in, because the alternative keeps alive a credential something already decided to end. The bridge still upgrades a legacy credential on refresh, but only once the legacy provider vouches for it. Issuing on any rejection would resurrect a session revoked mid-flight; the legacy check separates "never was a session" from "this session is gone". **The double-submit token kept the legacy lifetime.** `OPENTAG_SESSION_TTL_SECONDS` is documented as the session lifetime but only the sessions were given it, so every canonical sign-in still bounded the CSRF cookie by the legacy refresh TTL. A deployment setting them differently could hold a valid session with no way to mutate or sign out. The session lifetime now reaches every path that issues the cookie, and the cookie is re-sent — same value, full lifetime — on browser requests that carry a session, so rolling renewal cannot pull the two apart. Each fix is pinned by a regression that fails without it: restoring check-then-act leaves two fulfilled refreshes where the test wants one.
|
All three are right, and two of them are damage from my last round rather than from the original change. Fixed in 1 — the lock could stall the pool. This is the serious one and you traced it exactly: the lock lived in a transaction connection, every waiter held a second, and the holder still needed a third to do its work, because Better Auth's adapter runs on the base pool and not on the transaction I passed in. Ten concurrent exchanges of one credential exhaust a pool of ten and the server stops answering — I introduced an availability failure while fixing a correctness one. The record is the gate now, not a lock: one insert whose conflict branch rewrites the key with its own value, purely so That also closes the atomicity half. The session is created before the statement that decides whether it counts, so a failure between them leaves a session that was handed to nobody and expires on its own — rather than a live session no record points at. One design note, since it is not obvious: first writer rather than last. Last-writer-wins is the tidier statement, but the browser keeps whichever 2 — rotation was check-then-act, and you are right that my test only proved the uncontended case. Verifying and then replacing decides both outcomes on stale information: two refreshes could verify the same token and each mint a session, and a revocation landing in between would be undone by the replacement. The conditional delete is the gate now — exactly one caller can remove a given row — and only that caller goes on to issue the replacement. This reverses the ordering I argued for last round, and the reversal is the point. Withdraw-then-issue means a failure between them signs the client out; issue-then-withdraw means a credential something already decided to end stays alive. For an authentication boundary the first is the direction to fail in. The bridge still upgrades a legacy credential on refresh, but only after the legacy provider vouches for it — issuing on any rejection would be exactly the resurrection you describe, and the legacy check is what separates "never was a session" from "this session is gone". 3 — the CSRF cookie kept the legacy TTL. Correct, and worse than a mismatched number: a deployment that set the new session TTL differently could hold a valid session with no way to mutate or sign out, which reads as a broken account rather than an expired one. The session lifetime now reaches every path that issues the cookie, and the rolling-renewal half is handled by re-sending the cookie — same value, full lifetime — on browser requests that carry a session, so a session that keeps renewing cannot outlive the token it needs. 4 — Context Tree. Still outside this repository and still unresolved; I have not touched it and am not claiming otherwise. It needs the access/refresh-pair and JWT statements replaced with the session credential, the bridge, and the stage-5 boundary. On the regressions. The concurrency ones are checked against the code they replace rather than assumed: restoring check-then-act leaves two fulfilled refreshes where the test wants one. The revoke-versus-refresh case is asserted in the same test — deleting the session and then refreshing it must not hand back access. Validation on |
baixiaohang
left a comment
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The upsert gate, conditional session withdrawal, and configured CSRF lifetime resolve the prior implementation blockers, but the current authentication pre-handler can still treat a cookie-authenticated request as bearer-authenticated and discard Better Auth's rolling session cookie.
Risk level: B-high
- Path baseline: adds
packages/server/drizzle/0023_motionless_gideon.sqland a new durable upgrade table -> B-high database-structure change - Semantic lift: also changes core authentication transport selection, session rotation, browser renewal, and compatibility migration -> remains B-high
PR summary
- Author / repo: bestony / first-tree-ai/opentag
- Problem: CLI users and pre-cutover browsers need to move onto revocable Better Auth sessions without reauthentication, replay-created survivors, or lifetime regressions.
- Approach: A durable first-writer upgrade record converges legacy browser exchanges, conditional session deletion gates CLI rotation, Better Auth session TTL replaces the old refresh-token lifetime, and browser responses renew the accompanying CSRF cookie.
- Impacted modules: database schema/migrations, server auth composition, CLI refresh, legacy browser upgrade, Better Auth session resolution, browser CSRF, web refresh coordination, and auth tests.
Review findings
❌ 1. The pre-handler decides whether to enforce browser origin/CSRF from the mere presence of a non-empty Authorization: Bearer ... header, not from the credential Better Auth actually authenticated. In Better Auth 1.7.2, an invalid bearer leaves the existing cookie intact and getSession deliberately succeeds from that cookie; OpenTag then sees bearer as truthy, skips requireBrowserOrigin(), and authorizes the cookie session. A caller holding only the HttpOnly session cookie can therefore add any dummy Bearer header and perform mutations without the double-submit token. Select one transport before authentication (a presented bearer must authenticate as bearer only; otherwise use the cookie path and its browser guards), and add a valid-session-cookie + invalid-bearer mutation regression. [R4 / packages/server/src/plugins/user-auth.ts:58, Better Auth 1.7.2 bearer fallback behavior]
❌ 2. The rolling-lifetime fix re-sends only opentag_csrf. auth.api.getSession({ headers }) may extend the database session and prepare a refreshed Better Auth session-token Set-Cookie once updateAge is reached, but this result-only direct API call discards those response headers; the pre-handler never copies them to Fastify. The CSRF cookie can now keep extending while the browser's actual session cookie still expires on its original schedule, signing an active user out and leaving the renewed row behind. Resolve the session through a response/header-preserving call and forward Better Auth's cookie together with the CSRF renewal; cover a request past updateAge and authentication beyond the original cookie expiry. [R4, R5 / packages/server/src/plugins/user-auth.ts:78, packages/server/src/plugins/user-auth.ts:99, Better Auth 1.7.2 session renewal]
❌ 3. The durable runtime contract still says CLI/daemon credentials are an access/refresh pair and access credentials are JWTs, while this source change makes them one Better Auth session and defines a stage-5 compatibility boundary. The author confirms this remains unresolved outside the repository. Pair the source change with the source-backed Context Tree update before merge so the canonical contract and runtime do not land in conflict. [R1, R5 / OpenTag Server and Client Runtime Boundary]
✅ 4. The one-statement first-writer upgrade record removes the pool-deadlock pattern, conditional deletion prevents concurrent stateful refreshes from both minting replacements, and the configured session TTL now reaches every initial CSRF issuance path.
Action taken
- Submitted request changes on
4717aec53ff5fbd1202b29a7867da0204a803fb8.
A cookie-only caller could get its request treated as the CLI's, and skip the browser guards entirely, by attaching a junk `Authorization` header. The pre-handler decided which rules applied from the header merely being present, then asked Better Auth to authenticate. Better Auth reads the header and the cookie: its bearer plugin drops a token it reads as signed-but-invalid and leaves the request otherwise untouched, so `getSession` answers from the cookie. The header was still there, so the origin check and the double-submit token were skipped and the mutation went through on a cookie alone. The transport is now chosen before anything authenticates. A presented bearer authenticates as a bearer or not at all; only a request without one takes the cookie path and its guards. The token has to carry a `.` and a bad signature to reach this. A dotless one is signed by the plugin and installed as the session cookie, which overwrites the real one and fails on its own — which is why an earlier attempt at this regression passed against the vulnerable code and proved nothing. The test now sends what an attacker would, and fails without the fix. Session renewal was also being discarded. `getSession` extends a session as it is used and reports the refreshed cookie in its response headers, which a result-only call throws away: the row kept moving while the browser's cookie expired on its original schedule, signing out an active user and leaving the renewed row behind. The call asks for headers now, and anything Better Auth sets is forwarded alongside the double-submit renewal.
|
Both correct. Fixed in 1 — the CSRF bypass is real, and I reproduced it before fixing it. The pre-handler decided which rules applied from the header merely being present, then asked Better Auth to authenticate; Better Auth answered from the cookie, and the guards were skipped on the strength of a header that had authenticated nothing. The transport is now chosen before anything authenticates — a presented bearer authenticates as a bearer or not at all. Worth recording the detail that nearly cost me the regression, because it decides whether the test proves anything: the token has to contain a The test also asserts the same cookie is genuinely good on its own: without the header it reaches the browser path and is refused only for the missing double-submit token. Otherwise a regression that rejects everything would look like a pass. 2 — session renewal was being discarded. 3 — Context Tree. Unchanged and still outside this repository. I have not touched it. Validation on |
baixiaohang
left a comment
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The transport-selection and response-header fixes are sound, but the rolling-expiry behavior remains unpinned and the durable runtime contract still describes the superseded JWT pair.
Risk level: B-high
- Path baseline: adds
packages/server/drizzle/0023_motionless_gideon.sqland a durable legacy-upgrade table -> B-high database-structure change - Semantic lift: also changes core authentication transport selection, session rotation, browser renewal, and compatibility migration -> remains B-high
PR summary
- Author / repo: bestony / first-tree-ai/opentag
- Problem: CLI users and pre-cutover browsers need to move onto revocable Better Auth sessions without reauthentication, replay-created survivors, CSRF bypasses, or rolling-lifetime regressions.
- Approach: Better Auth sessions replace newly issued CLI token pairs, compatibility paths migrate legacy credentials, session replacement is gated against races, and browser authentication now selects one credential transport before resolving identity while forwarding renewed cookies.
- Impacted modules: database schema/migrations, server auth composition, CLI refresh, legacy browser upgrade, Better Auth session resolution, browser CSRF, web refresh coordination, and auth tests.
Review findings
❌ 1. The response-header implementation now preserves Better Auth's renewed session cookie, but the requested rolling-lifetime regression is still absent. This head adds only the invalid-bearer integration case; no test advances a live browser session past updateAge, captures the replacement Better Auth cookie from the Fastify response, and proves authentication continues beyond the original cookie expiry. That boundary is the part that previously failed silently while the database row kept renewing, and it depends on the exact returnHeaders / multi-Set-Cookie bridge rather than on the initial TTL tests. Please pin the full renewal path so a later result-only getSession call or cookie-forwarding regression cannot reintroduce the orphan/sign-out behavior. [R5 / packages/server/src/plugins/user-auth.ts:86, packages/server/src/__tests__/integration/better-auth.test.ts:458]
❌ 2. The durable runtime contract remains unresolved. At Context Tree commit abab9b6d87ee3b924356b96d8c0de1c293c9fe49, opentag/system/server-client-runtime-boundary.md still says CLI/daemon credentials are an access/refresh pair and access credentials are JWTs, while this PR makes them one Better Auth session and defines a stage-5 compatibility boundary. The author confirms the Tree is unchanged. Pair this source change with the source-backed Tree update before merge so the canonical contract and runtime do not land in conflict. [R1, R5 / OpenTag Server and Client Runtime Boundary]
✅ 3. A presented bearer now authenticates exclusively through the bearer path, and the signed-invalid-bearer regression demonstrates that a valid browser cookie can no longer bypass origin and double-submit checks.
✅ 4. getSession({ returnHeaders: true }) now forwards Better Auth's Set-Cookie values before renewing opentag_csrf, resolving the implementation half of the rolling-cookie finding.
Action taken
- Submitted request changes on
1c74e75e5ee15cd8cb57c179ec9d39df2edd9346.
The implementation was fixed last round but nothing held it there, and this is the failure mode that hides: a result-only `getSession` still authenticates every request, so the suite stays green while the database row silently renews past a browser cookie that expires on its original schedule. The user is signed out while active and the renewed row is left behind. The regression ages a real signed-in session past `updateAge` — by moving its expiry back, which is what Better Auth actually reads — then drives the pre-handler and asserts three things: the row moved forward, the replacement cookie reached the reply, and that cookie is a working credential rather than merely present. Reverting to a result-only call fails it on the middle assertion, which is the one the initial-TTL tests could never have covered.
|
Fair — I fixed the implementation and left it unpinned, which is exactly the gap you name: a result-only The regression ages a real signed-in session past
Checked against the code it replaces: reverting to a result-only call fails it on the middle assertion. None of the TTL tests could have caught that, which was your point. On the Context Tree. Still unresolved and still outside this repository — Validation on |
baixiaohang
left a comment
baixiaohang
left a comment
There was a problem hiding this comment.
Recommendation: request changes
- Rationale: The source implementation and regression coverage now resolve the authentication/session blockers, but the durable runtime contract still describes the superseded JWT access/refresh pair.
Risk level: B-high
- Path baseline: adds
packages/server/drizzle/0023_motionless_gideon.sqland a durable legacy-upgrade table -> B-high database-structure change - Semantic lift: also changes core authentication transport selection, session rotation, browser renewal, and compatibility migration -> remains B-high
PR summary
- Author / repo: bestony / first-tree-ai/opentag
- Problem: CLI users and pre-cutover browsers need to move onto revocable Better Auth sessions without reauthentication, replay-created survivors, CSRF bypasses, or rolling-lifetime regressions.
- Approach: Better Auth sessions replace newly issued CLI token pairs, compatibility paths migrate legacy credentials, session replacement is gated against races, and browser authentication now selects one transport while preserving rolling session and CSRF cookies.
- Impacted modules: database schema/migrations, server auth composition, CLI refresh, legacy browser upgrade, Better Auth session resolution, browser CSRF, web refresh coordination, and auth tests.
Review findings
❌ 1. The durable runtime contract remains unresolved. The bound Context Tree is still at abab9b6d87ee3b924356b96d8c0de1c293c9fe49, where opentag/system/server-client-runtime-boundary.md says CLI/daemon credentials are an access/refresh pair and access credentials are JWTs. This PR instead makes them one revocable Better Auth session, keeps the four-field wire shape by repeating the token, rotates it on refresh, and defines the stage-5 bridge boundary. The author confirms the Tree is unchanged and has now supplied the exact replacement semantics; please pair this source change with the source-backed Tree update before merge so the canonical contract and runtime do not land in conflict. [R1, R5 / OpenTag Server and Client Runtime Boundary]
✅ 2. The new renewal regression uses a real Better Auth session, moves it past the library's updateAge condition, verifies the database expiry advances, captures the session cookie written through the Fastify reply bridge, and authenticates that replacement credential. This closes the final source-side blocker from the prior review.
✅ 3. The transport-selection, replay convergence, conditional CLI rotation, narrowed legacy fallback, retained Google callback, configured session lifetime, and synchronized session/CSRF renewal findings remain resolved on this head.
Action taken
- Submitted request changes on
e58aba288c7bc90bb4195a10ae46bce2f3ed6e8c.
|
Context Tree update opened: first-tree-ai/first-tree-context#984. It replaces the access/refresh-pair bullet in The sentence separating identity from authority is kept verbatim — it is still true, and it is the load-bearing half of that bullet. What changed is the credential's shape and revocability, not that proving who you are proves what you may do. Scope is one bullet in one file. |
|
The paired Tree PR is scoped correctly and supplies the missing contract update, but one sentence currently reverses the final source behavior.
Please change that clause to record the actual durable contract—for example, “refreshing withdraws the presented session and then issues its replacement”—while keeping the rotation, compatibility bridge, and identity-vs-authority statements. Once the paired Tree PR reflects that ordering, the final blocker on this source head is cleared. |
|
The Context Tree blocker is resolved: first-tree-context#984 merged as It was reworded during review into implementation-neutral language — no vendor name, no field names — which is the better shape for a canonical contract. I checked the merged text against this PR rather than assuming, and each claim holds:
That was the last open finding on this head. CI is green on |
|
@baixiaohang @yuezengwu — re-review requested. Everything raised across the four rounds is addressed on
CI is 6/6 green. Local: Not requesting re-review on #205 yet — that was your call and I agree with it: it is still based on this PR's older commit, and rebasing onto a branch that is still changing would only have to be redone. It gets rebased and sent for a full review once this merges. |
Rebuilding this stage on the merged base took `app.ts` from the pre-rebase branch, which predates the options #203 added — so the pre-handler was handed no session lifetime and quietly declined to renew the token. Better Auth would keep rolling the session while `opentag_csrf` expired on its original schedule, leaving an active browser able to read but not to mutate or sign out. I checked the documentation diff for reverts of that kind and did not check the composition, which is where it landed. The regression goes through `createApp` rather than constructing the pre-handler directly. That distinction is the whole point here: the logic was already correct and already covered, and what broke was the wiring that decides whether it runs at all. Removing the options again fails it.
3 participants
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 productionopen-tagpackage only ships on avX.Y.Ztag.accessTokenandrefreshTokencarry 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 ownopentag_accesscookie.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
getSessionreads 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
setSessionCookieneeds. 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, whichparseServerConfigalready restricts to a loopbackOPENTAG_ENV=devserver. 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
AuthServicewith 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_accesscookie 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 throughgetSession, and is gone after sign-out.Deployment
No schema change, no environment variable, no manual step. Server and
open-tag-staginggo 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, andOPENTAG_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), andpnpm --filter @opentag/server test:integration(209) all pass.