The Auth adapter over the Unsafe surface with the conformance suites - #1221
Conversation
…he Unsafe surface The auth plugin no longer hands better-auth `prismaAdapter`. It builds its own adapter with `createAdapterFactory`, running on the Unsafe surface a Prisma 8 context carries: eight methods on the ORM lane, and `incrementOne` plus an unconditional `deleteMany` as single typed-SQL statements through the surface's own executors. `consumeOne` is `where(…).delete()`. `authPlugin` pins `db.idField: 'uuid7'` on every list it injects and the adapter declares `disableIdGeneration`, so the database mints auth ids. `advanced.database.generateId` and `advanced.database.joins` join the refused passthrough keys; `createAuth(config, rawOpensaasContext)` keeps its signature. core gains the engine-owned LIKE-pattern escaping the adapter lowers `contains` / `starts_with` / `ends_with` and insensitive `eq` through — one escaper, for ADR-0055's Where vocabulary to adopt rather than re-write. better-auth's normal, uuid and caseInsensitive suites run over the Test context; numberId and joins are skipped with the reason stated. Implements #1161. Part of #1126. See ADR-0060. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: fc27dd8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Coverage Report for Core Package Coverage (./packages/core)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for UI Package Coverage (./packages/ui)
File CoverageNo changed files found. |
Coverage Report for CLI Package Coverage (./packages/cli)
File CoverageNo changed files found. |
Coverage Report for Auth Package Coverage (./packages/auth)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for Storage Package Coverage (./packages/storage)
File CoverageNo changed files found. |
Coverage Report for RAG Package Coverage (./packages/rag)
File CoverageNo changed files found. |
Coverage Report for Storage S3 Package Coverage (./packages/storage-s3)
File CoverageNo changed files found. |
Coverage Report for Storage Vercel Package Coverage (./packages/storage-vercel)
File CoverageNo changed files found. |
| // Prisma resolves the first matching identity and deletes by it with | ||
| // `RETURNING`, so of two racing consumers exactly one gets the row — | ||
| // the at-most-one guarantee better-auth asks of this method. | ||
| const consumed = await narrow(model, where).delete() |
There was a problem hiding this comment.
consumeOne is not atomic — a one-time token can be consumed twice.
The tripwire test for this method asserts recorder.plans.length is greater than one, i.e. Prisma lowers where(…).delete() to a SELECT that resolves the identity followed by a DELETE. Those two statements run on the surface's executors with no transaction around them (transaction: false), and the result is returned without checking that the DELETE actually affected a row.
Scenario: two requests replay the same magic-link / email-OTP / reset token concurrently. Both SELECTs resolve the same verification row before either DELETE runs. If Prisma's delete() answers with the row it resolved rather than the DELETE's own RETURNING, both callers get a non-null row back and the token is accepted twice — exactly the guarantee consumeOne exists to provide.
better-auth's own reference adapter guards this explicitly: @better-auth/prisma-adapter wraps the pair in db.$transaction(...) and returns target only when deleteMany(...).count > 0. This adapter does neither, and no test exercises the concurrent case (the second consumeOne in adapter-tripwire.test.ts is sequential, which the SELECT alone already answers).
Either wrap the pair in context.transaction / the surface's transaction scope, or make this method a single typed-SQL DELETE … WHERE … RETURNING the way incrementOne already is.
There was a problem hiding this comment.
Fixed in 300b17d. consumeOne now resolves the row, then deletes by its identity, both inside one transaction on the Unsafe surface's transaction-bound lanes, and answers the row only when the delete claimed it (deleteAndCount() > 0) — the shape @better-auth/prisma-adapter uses. The transaction arrives as an adapter option; getDatabaseConfig reads the context's own transaction structurally, beside unsafe, so no public signature widened and none of #1162's factory-transaction work is pulled in.
Three tests in the new packages/auth/tests/adapter-behaviour.test.ts. Two drive concurrency: two (and five) concurrent consumeOne calls against one row — exactly one winner, zero rows left. Those pin the outcome but not the mechanism: the in-process database holds a single connection, so the transactions serialise and the loser's own SELECT finds nothing, which would pass without a gate. So the third drives the lane through a double whose SELECT resolves a row and whose DELETE claims none — the interleaving two overlapping transactions produce on a real pool — and asserts null. That one fails against the previous where(…).delete().
ADR-0060 carries a dated amendment at the top: the decision is unchanged, the mechanism sentence is corrected to "two statements under a transaction, gated on the delete's own count".
| isBigInt: boolean, | ||
| ): Expression<ScopeField> { | ||
| const like = (build: (value: string) => string): Expression<ScopeField> => | ||
| fns.raw`${column} LIKE ${pattern(clause, build)}`.returns(BOOLEAN_CODEC) |
There was a problem hiding this comment.
The raw LIKE fragment interpolates a user-controlled string without param(), and nothing exercises it.
Every other value this PR puts inside fns.raw is wrapped first — incrementOne builds its set assignments as fns.raw`${param(widened, { codecId: codec.codecId })}` . Here the pattern (an attacker-supplied contains / starts_with / ends_with value, escaped for LIKE metacharacters but otherwise raw) goes straight into the template. If RawSqlBuilder inlines non-Expression template values as SQL text rather than binding them, this is either a syntax error on every call or an injection point; escapeLikeLiteral escapes %/_, not quotes, so it is no defence.
This is undetectable from the test suite: sqlWhere is reachable only from incrementOne, and better-auth only ever issues incrementOne with eq/comparison guards on the rate-limit key — so the whole like branch (and the hardcoded BOOLEAN_CODEC = 'pg/bool@1' it returns) never runs in conformance or tripwire. Bind the pattern the same way incrementOne binds its set values.
There was a problem hiding this comment.
Parameterised in 300b17d: the pattern is now param(pattern, { codecId: 'pg/text@1' }), matching what the set branch of incrementOne does — and incrementOne's increment fragment is bound the same way (your summary finding 2).
One correction to the diagnosis, with evidence, since it changes how urgent this was: it was not an injection point at rc.8. Prisma's raw tag binds bare interpolations rather than inlining them — expression-PL-Tra28.d.mts: "Bare RawSqlLiteral interpolations are wrapped as ParamRef nodes with the codec resolved via adapter.inferCodec(value). Use param when the codec cannot be inferred from the value alone (e.g. Date)." So the string travelled as $n, and param() here removes the reliance on that inference rather than closing a hole.
The branch is now exercised: adapter-behaviour.test.ts drives incrementOne with a contains guard whose value is 100% ' OR 1=1 -- and asserts it matches the row holding that literal and not the neighbouring one, and the ORM lane's contains gets the same treatment with a value carrying quotes, %, _ and a backslash.
| ? fns.ilike(column, pattern(clause, likeEqualsPattern)) | ||
| : fns.eq(column, scalar(clause, isBigInt)) | ||
| case 'ne': | ||
| return fns.ne(column, scalar(clause, isBigInt)) |
There was a problem hiding this comment.
The typed-SQL lane loses the null handling the ORM lane has.
ormClause maps eq/ne with value === null to isNull() / isNotNull(). sqlClause does not: fns.eq(column, null) and fns.ne(column, null) lower to col = NULL / col <> NULL, which are NULL in SQL and therefore never true.
Scenario: a better-auth plugin issues incrementOne with a guard like { field: 'consumedAt', operator: 'eq', value: null } (the natural "only bump a row that hasn't been claimed" shape). The UPDATE silently matches zero rows and the adapter answers null — the caller reads it as "no such row" rather than "guard is broken". mode: 'insensitive' is likewise honoured for eq here but dropped for ne, which the ORM lane handles.
There was a problem hiding this comment.
Half of this is a false positive, half is real, and both now have tests.
Null handling — false positive at rc.8. fns.eq/fns.ne already lower a null operand to IS NULL/IS NOT NULL themselves. From @prisma/orm-family-sql/dist/builder__runtime.mjs (the builtin fns set, createBuiltinFunctions → eq/ne):
function eq(a, b) {
if (b === null) return boolExpr(NullCheckExpr.isNull(resolve(a)));
...
function ne(a, b) {
if (b === null) return boolExpr(NullCheckExpr.isNotNull(resolve(a)));So the consumedAt guard you describe works. adapter-behaviour.test.ts now pins it end-to-end through incrementOne: an ne: null guard bumps the row, an eq: null guard answers null. I left the code as it is rather than adding branches that would restate what the builtins do.
Insensitive ne — real, fixed in 300b17d. The SQL lane dropped mode for ne where the ORM lane wraps its ilike in not(). There is no not in the builtins and the Postgres target contributes only ilike, so it is now fns.raw with a NOT ILIKE and the pattern bound through param().
| function insensitiveList(clause: CleanedWhere): readonly string[] | undefined { | ||
| if (clause.mode !== 'insensitive' || !Array.isArray(clause.value)) return undefined | ||
| const strings = clause.value.filter((entry): entry is string => typeof entry === 'string') | ||
| return strings.length === clause.value.length ? strings : undefined |
There was a problem hiding this comment.
An empty insensitive in list reaches or() with zero arguments.
For { operator: 'in', value: [], mode: 'insensitive' } this returns [] — strings.length === clause.value.length is 0 === 0 — rather than undefined. ormClause then calls or(...[]), i.e. or() with no disjuncts (and not(or()) for not_in), which either throws or emits a degenerate predicate instead of the correct FALSE / TRUE.
Guard the empty case: if (strings.length === 0) return undefined, or handle a zero-length disjunct list in ormClause alongside the field.in([]) path.
There was a problem hiding this comment.
Fixed in 300b17d: insensitiveList returns undefined for a zero-length list, so in/not_in fall through to the lane's own field.in([]) / field.notIn([]), which are the FALSE/TRUE the operators mean. Two tests in adapter-behaviour.test.ts: an empty insensitive in answers [], an empty insensitive not_in answers every row.
| // ADR-0048's per-list pin, named for the Auth lists: every id the | ||
| // adapter hands better-auth is minted by the database, and it is the | ||
| // same strategy every other list gets. | ||
| const listConfig = { ...derived, db: { ...derived.db, idField: 'uuid7' as const } } |
There was a problem hiding this comment.
The uuid7 pin overrides an app-level db.idField with no escape hatch, and it does not reach the extendList branch.
uuid7 is already the global default (listConfig.db?.idField ?? config.db.idField ?? 'uuid7' in contract/derive.ts), so this line only changes behaviour for an app that deliberately set db.idField to something else — and for that app it silently wins.
The case that breaks is adoption. adopt-better-auth-tables.test.ts was updated to expect idField: 'uuid7' on AuthUser/AuthSession/AuthAccount/AuthVerification, but those lists exist to diff clean against a live better-auth install whose id columns hold nanoids in text. An app that set db.idField to a text strategy to match that live schema can no longer keep the auth lists on it; the generator emits a pg/uuid column and the diff (and any migration over existing rows) fails.
Second half of the same asymmetry: the extendList branch below forwards only fields/hooks/mcp, so a list the app declared itself keeps whatever idField it had — while the adapter unconditionally declares supportsUUIDs: true and supportsNumericIds: false for every model. An app-declared User on int autoincrement gets ids stringified by better-auth's id output transform and then handed back to a numeric column on the next where.
Consider honouring an explicit per-list or app-level idField and only defaulting when none was declared.
| typeof advancedDatabase === 'object' && | ||
| !Array.isArray(advancedDatabase) | ||
| ) { | ||
| if ('generateId' in advancedDatabase) { |
There was a problem hiding this comment.
'generateId' in advancedDatabase also refuses the two settings that mean what this adapter wants.
better-auth's generateId takes false ("let the database mint it") and 'uuid' alongside a custom function. Both of the first two are exactly the behaviour the adapter asks for — initGetIdField returns undefined for the id in either case — yet the presence check rejects them with a message about "an app-supplied generator ... would write a non-UUID into a uuid column", which is false for false/'uuid'. Same for a config that spells out { generateId: undefined }.
Narrow the check to the values that are actually incompatible (typeof v === 'function' and v === 'serial') rather than key presence.
| ...options, | ||
| advanced: { | ||
| ...options.advanced, | ||
| database: { ...options.advanced?.database, generateId: 'uuid' }, |
There was a problem hiding this comment.
The conformance suite runs a configuration production refuses, and the difference is not cosmetic.
This override sets advanced.database.generateId: 'uuid' — the exact key assertNoUnsupportedPassthroughKeys now throws on. So all 159 passing assertions exercise a config no shipped app can have.
The two configs take different branches of better-auth's initGetIdField. Under the tested one, useUUIDs is true, so transform.input for id returns undefined whenever supportsUUIDs is set — a caller-supplied id is always dropped and the database mints. Under the shipped one (disableIdGeneration: true, no generateId), useUUIDs is false and transform.input falls through to return value, so a caller-supplied id is written to the column. That path — a plugin or databaseHooks create carrying its own id into a uuid7 column — is the one production takes and the one nothing here covers.
Either drop the override and give the suite's fixtures UUIDs some other way, or add a case that runs the shipped id configuration.
There was a problem hiding this comment.
Fixed in 300b17d. The override is gone: customIdGenerator: () => randomUUID() alone gives the fixtures their UUIDs, so normal and caseInsensitive now run disableIdGeneration: true with no generateId — the shipped branch of initGetIdField, where a caller-supplied id is written rather than dropped. The uuid suite still declares that key itself, upstream, as the mode it exists to exercise; the harness no longer declares it for everyone.
That exposed three upstream probes that cannot run this way: findOne/findMany/delete "not found" each hardcode the id "100000" unless the options say generateId: 'uuid', and Postgres rejects a malformed uuid outright instead of answering not-found. They are disabled in the normal suite only, with that reason in the file, and re-covered under the shipped configuration in adapter-behaviour.test.ts with a well-formed id — plus a case asserting a caller-supplied id survives (forceAllowId, which is where the factory lets one through at all; without it the factory itself drops the id before the transform, in both configurations).
156 passed, 76 skipped, 0 failed across normal, uuid and caseInsensitive (was 159/73 under the impossible config; the delta is those three probes).
borisno2
left a comment
borisno2
left a comment
There was a problem hiding this comment.
Code review — /code-review high (verdict: REQUEST CHANGES)
Seven findings are already posted as inline comments on the diff. This summary adds a second pass over the risk areas that weren't covered there, and states the verdict. GitHub blocks a formal Request changes from the PR author's own account, so this is submitted as a Comment review — treat it as request changes.
The adapter itself is good work: the addressing layer, the AND/OR grouping, the incrementOne single-statement lowering, and the structural surface guards are all sound. What blocks is a small set of correctness and test-integrity issues.
Verified clean (the specific claims this PR makes)
@prisma/orm-postgresdependency. Exact pin8.0.0-rc.8as peer + dev, which is exactly howpackages/coredepends on it (required peer, not optional — correct: the adapter importscodecOf/paramat module scope). No version skew.- The published-subpath claim holds. I unpacked the published tarball rather than taking it on trust:
./orm-clientand./relational-coreare both inexports;./orm-clientre-exports@prisma/orm-family-sql/orm-client(a realdependenciesentry, itself exporting./orm-client), whose.d.mtsdeclaresand,or,notin its export list. Nothing here reaches an@internal/…alias. - The LIKE escaper is correct.
escapeLikeLiteralescapes the backslash first, then%and_— the escaped-escape ordering is right, andLIKE_ESCAPE_CHARACTERcorrectly documents Postgres' implicit default. No second escaper exists inpackages/auth(grepped). Only gap: no test for the empty string. - The structural
unsaferead is type-safe.isUnsafeSurface/isCollection/isSqlTableuseReflect.getbehind type predicates — noany, no cast,unknownstays internal. NoteisUnsafeSurfaceonly probesquery/executewhile the adapter also needsorm/sql; that's survivable becauseAuthModelUnreachableErrorcatches it downstream, but the guard is weaker than its error message claims. - The tripwire is not trivially passing.
origins()vsplans.map(() => 'unsafe')would pass vacuously on an empty recorder, butupdate,deleteandconsumeOneeach assertplans.length > 1first, so the multi-statement claim is genuinely asserted. - Skips are all justified in the file, and the count is dominated by
enableJoinTests. Changeset present and accurate.
Additional findings
-
outward()narrows everybiginttoNumber()unconditionally —packages/auth/src/adapter/index.ts(const outward = …). The inbound direction is careful (isBigIntfromgetFieldAttributes), but the outbound direction is a blankettypeof value === 'bigint' ? Number(value) : valueover every column of every model. AboveNumber.MAX_SAFE_INTEGERthat silently mangles the value.lastRequestwon't hit it, but an app column added throughextendUserListwill. Narrow the same way you widen — by the declaredbigintattribute — and at minimum don't lose precision silently. -
incrementOne's increment fragment is not parameterised —fns.raw\${target} + ${delta}`. Thesetbranch three lines below wraps its value inparam(widened, { codecId }); the increment branch interpolates the raw JSdelta. The author clearly knewparam()was the right call here, which makes the omission look accidental — and it's the same class of issue as the unparameterisedLIKEpattern flagged inline inwhere.ts:197`. -
The two lanes disagree on how a column is addressed.
applyOrmWhereresolves throughgetDefaultFieldName(field keys), whilesqlWhereindexesfields[clause.field]by better-auth's column name andcodecFor/columnsOfdo the same. Both assumptions can't hold once an app supplies afieldsmapping on a model — the ORM path resolves and the typed-SQL path throwsAuthWhereError. Reachable viaincrementOneon a remappedrateLimit. Nothing tests a mapped field on the SQL lane. -
widen()throws the wrong error for a non-integer.BigInt(1.5)raises a bareRangeError, escaping theAuthWhereErrorvocabulary the rest of the module is careful to use. -
Two type casts, against the repo's no-casting rule —
asAdapterResult'srow as T(documented, arguably the one unavoidable seam against better-auth's typing) andinward(model, { ...(update as Record<string, unknown>) })inupdate(), which is undocumented and is a second place, contradictingasAdapterResult's own "this is the one place ours does". -
Two skips hide production gaps, not adapter gaps.
should enforce the issuer-scoped account identity keyandshould return null for nullable foreign keysare both deferred to #986. The first means duplicate accounts are possible in a shipped app until that lands. That deserves a stated known limit in the adapter docblock alongside the joins/createSchema/transaction ones, not only a test comment.
Must fix before merge
consumeOneatomicity (inliner3944014739) — a token can be consumed twice.- The conformance harness runs a config production refuses (inline
r3944017230) — the shipped id path is the untested one. - Unparameterised raw values:
where.ts:197(inline) and finding 2 above. sqlClausedropping null handling (inliner3944015510) andor()with zero arguments (inliner3944015825).
…config Addresses the review on #1221. - `consumeOne` runs its resolve-then-delete pair in one transaction on the Unsafe surface's transaction-bound lanes and answers the row only when the delete claimed it. The adapter takes the transaction as an option; the server reads the context's own `transaction` structurally, beside `unsafe`. - The conformance harness drops the `advanced.database.generateId: 'uuid'` override, so normal and caseInsensitive now run the id path production takes. The three upstream probes that hardcode a non-UUID id are disabled in the normal suite with their reason and re-covered under the shipped config. - The typed-SQL LIKE pattern and `incrementOne`'s increment fragment are bound through `param()`; insensitive `ne` no longer loses its mode on that lane. - An empty insensitive `in`/`not_in` lowers through the lane's own `in([])` rather than a zero-argument `or()`. - `outward()` narrows a bigint to a number only for a column better-auth declares as `bigint`, matching the inbound gate. - `update`'s payload is narrowed by a predicate rather than cast; the issuer-scoped account gap is a stated known limit; ADR-0060 carries a dated amendment correcting the `consumeOne` mechanism. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review addressed — 300b17dReplies are on the five inline threads this changes; here are the summary findings and the two I did not act on. Summary findings
LIKE escaper empty-string test — added in Not addressed hereThe two remaining inline threads — the Verification
The existing changeset is updated in place; no second one. |
…e.ts The base moved under this branch while it was in review, so GitHub could compute no merge ref and CI never ran for 300b17d. The one content conflict was `packages/core/src/internal.ts`, where this branch's `where/like.ts` re-exports and the base's `findDatabaseConnection` (#1218) were added at the same point; both are kept. #1220 landed the Where vocabulary from its own branch rather than adopting this branch's engine-owned escaper, leaving a second implementation: `containsPattern()` in `secured/vocabulary.ts`. It performed the same three replacements in the same order with the same wrapping as `likeContainsPattern`, so it is deleted and the `contains` case now calls the shared builder. One escaper, as ADR-0060 requires. #1220's Where-vocabulary and filter tests pass unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Merged One content conflict, in Duplicate escaper consolidated. #1220 landed the Where vocabulary from its own branch rather than adopting this branch's engine-owned escaper, so the base carried a second implementation — #1220's own Where-vocabulary and filter tests pass unchanged ( Full build green. The existing changeset already describes the escaper as shared with the Where vocabulary, which the consolidation now makes true, so it is left as-is. |
borisno2
left a comment
borisno2
left a comment
There was a problem hiding this comment.
Re-review of 300b17d — verdict: APPROVE (submitted as a Comment review; GitHub blocks a formal Approve from the PR author's own account)
I audited the two rebuttals against the installed 8.0.0-rc.8 source rather than either party's characterisation, then went over the new code. The author is right on all three disputed findings, and the four fixes that did land are sound. Nothing blocks.
Rebuttal 1 — "unparameterised SQL was not an injection point": author is right
Not just the docblock the author cited — the implementation and the renderer both.
@prisma/orm-family-sql/dist/expression-DmQbErXU.mjs, resolveInterpolation:
if (typeof value === "number" || typeof value === "bigint" || typeof value === "string"
|| typeof value === "boolean" || value instanceof Uint8Array)
return ParamRef.of(value, { codec: { codecId: adapter.inferCodec(value) } });
throw runtimeError("RUNTIME.RAW_SQL_UNSUPPORTED_INTERPOLATION", …)templateParts only ever pushes the tag's own static strings as SQL text; every interpolation becomes an AST node. And @prisma/orm-target-postgres/dist/sql-renderer-*.mjs closes it: renderRawExpr → renderParts → typeof part === "string" ? part : renderExpr(part, …), and renderExpr's case "param-ref" → renderParamRef → renderTypedParam(index, codecId, …), i.e. a $n placeholder. A bare string interpolation is bound, never concatenated. The original finding's severity was overstated; there was no injection point on the LIKE or incrementOne paths.
The param() change is still worth having — inferCodec throws for anything outside {number, bigint, string, boolean, Uint8Array} — but as a robustness/consistency fix, not a security one.
Rebuttal 2 — "fns.eq(x, null) already lowers to IS NULL": author is right
@prisma/orm-family-sql/dist/builder__runtime.mjs L219–228, reached from createBuiltinFunctions L301–302:
function eq(a, b) { if (b === null) return boolExpr(NullCheckExpr.isNull(resolve(a))); …
function ne(a, b) { if (b === null) return boolExpr(NullCheckExpr.isNotNull(resolve(a))); …So the consumedAt: null guard scenario in the original comment does not occur. The other half of that comment — insensitive ne dropping its mode on the SQL lane — was real and is fixed.
While I was in there: the empty-or() finding was also a false positive. OrExpr.of([]) is exactly OrExpr.false(), and the Postgres renderer has case "or": if (node.exprs.length === 0) return "FALSE". The insensitiveList guard is behaviour-neutral. Harmless and slightly clearer, so keep it.
Rebuttal 3 — "lane addressing was a false positive": author is right, and the retained test is not vacuous
The reverted addressing is internally consistent for a model carrying a fields mapping. Traced end to end for rateLimit.fields.key = 'rate_key' (stack field key key, physical column rate_key, better-auth field name key):
sqlWheregetsclause.field === 'rate_key'(better-auth's factory maps it before the custom adapter sees it) and indexes the SQL proxy by that physical name — correct.resolveField→getDefaultFieldName('rate_key')→'key', which is both the ORM contract's field key and the namegetFieldAttributeswants — soisBigIntandapplyOrmWhereare correct.inwardkeys by column → field key;outwardnarrows on the field key and renames viagetFieldNameback to the column;columnsOfis column-named, matching what the SQL lane returns.
The test discriminates: findOne on the mapped user.name → full_name exercises the ORM lane, incrementOne on the mapped rateLimit.key → rate_key exercises sqlWhere (it would throw AuthWhereError if the addressing were wrong), and consumeOne on mapped verification.identifier → ident crosses both.
Minor, non-blocking: the SQL lane's mapped coverage runs only through sqlWhere. Nothing exercises codecFor with a mapped increment/set column — only count/lastRequest, which are unmapped. A mapped counter column would be the remaining untested corner.
consumeOne — the fix holds
- Gate is correct.
deleteAndCount() > 0under Read Committed: the loser's DELETE blocks on the row lock, re-evaluates after the winner commits, claims zero, answersnull. - The transaction is genuinely the surface's.
packages/core/src/context/index.tsbuilds the child context'sunsafefromcreateUnsafeTransactionSurface(client, _unsafeTransaction)insideclient.transaction, sotx.unsafereally is transaction-bound — not the outer client. Thenarrow(…, lane)/collectionFor(model, lane)threading is complete; no path inconsumeOnefalls back to the ambientunsafe. - The structural read is type-safe.
isTransactionCapableis aReflect.getpredicate over a declaredTransactionCapableContext, and the tx context'sunsafeis re-validated byisUnsafeSurfaceinside the callback. Noany, no cast. TighteningisUnsafeSurfaceto also probeorm/sqlcloses the gap the prior review noted. - The third test is the one that pins it, and it is not tautological. The double answers
first() → rowanddeleteAndCount() → 0. Against the previouswhere(…).delete()shape the double'sdelete()returnsrow, the adapter answers non-null, and the test fails. It genuinely discriminates old from new.
The author's own caveat is the honest one and I'd leave it as stated: the two concurrency tests assert the outcome, not the mechanism, and no test drives the pair through a real transaction on a real pool. That is a residual, not a defect.
Conformance harness — the skips are legitimate
Verified against @better-auth/test-utils@1.7.1 dist/adapter/suites/basic.mjs. Five sites hardcode "100000", each gated on getBetterAuthOptions().advanced?.database?.generateId === "uuid" — exactly the gate described. Three are the disabled tests, named identically to HARDCODED_NON_UUID_ID:
- L276
findOne - should not throw on record not found - L1214
findMany - should return an empty array when no models are found - L1894
delete - should not throw on record not found
The other two (L697, L1236) are inside join tests already disabled via enableJoinTests. So 159→156 / 73→76 accounts for the delta exactly, with no other test silently flipping to skipped.
The replacement coverage is real, not a stub: the three probes are re-run in adapter-behaviour.test.ts under the shipped config with well-formed UUIDs, plus a forceAllowId case the old override could not have run at all (under generateId: 'uuid' the id transform drops a caller-supplied id). Net: the suite lost three probes and gained the id branch production actually takes. That is a stronger suite at a lower pass count.
ADR-0060 amendment
Accurate, dated, PR-linked, stacked above the 2026-09-05 amendment in the same blockquote style, and the decision bullet is cross-referenced rather than rewritten. Correct handling.
One nit, non-blocking: the decision bullet "config.transaction is implemented" is still contradicted by the shipped transaction: false (a stated known limit deferred to #1162). The amendment corrects the consumeOne mechanism but leaves that one standing. Worth folding into #1162's amendment rather than this PR.
Remaining items
asAdapterResult'srow as T— the one cast left, and it is the right one to keep.Tis a free type parameter on every row-returningCustomAdaptermethod, so no database value can satisfy it. The docblock now names that external constraint precisely, which is what the comments rule asks of a kept comment.update's cast is genuinely gone, replaced byisColumnValueswith a real error.- Bigint outbound narrowing — now the exact mirror of
inward's gate, and thelegacyIdround-trip of9007199254740993nalongsiderateLimit.lastRequeststill answering a JS number covers both sides. - Empty insensitive
in/not_in— fine (see above; behaviour-neutral). - House rules:
.jsextensions throughout, noanyin new code, no new undocumented comments.
What I did not re-review
The two threads the author deliberately left open — the uuid7 pin overriding an app-level db.idField, and narrowing the generateId refusal to the values that are actually incompatible. I agree with the author's read that both are real, both change what an app can configure rather than what the adapter does, and both belong in their own issue alongside the unconditional supportsUUIDs/supportsNumericIds declarations. They should not hold this PR.
Approving.
1 participant
Implements #1161. Part of #1126. Decision record: ADR-0060.
@opensaas/stack-authstops handing better-authprismaAdapter— which is written against Prisma Client delegates a Prisma 8 client does not have — and builds its own adapter with better-auth'screateAdapterFactory, driving the Unsafe surface.What landed
packages/auth/src/adapter/— the Auth adapter.context.unsafe.orm):create,findOne,findMany,update,updateMany,delete,deleteMany,count.incrementOneis one typed-SQLUPDATE … SET n = n + δ … RETURNINGthrough the surface's executors. Prisma'sfnsnamespace has no arithmetic builtin, so only then + δfragment isfns.raw, carrying the column's own codec viacodecOf; the guard predicate and theRETURNINGlist stay typed.wheredeleteManyis one typed-SQL unconditionalDELETE— a Collection'sdeleteAndCountis checked against a prior.where(), and better-auth's own cleanup issues this on every model.consumeOneiswhere(…).delete(): Prisma resolves the first matching identity and deletes by it withRETURNING, so of two racing consumers exactly one gets the row.disableIdGeneration: true,supportsUUIDs: true,supportsNumericIds: false, andauthPluginpinsdb.idField: 'uuid7'on every list it injects (ADR-0048).deriveAuthListsnow surfaces the model → list-key map it already built), fields through better-auth's default field names, and the contract applies the column map. A mapped table name never reaches a Collection —getDefaultModelNamemaps it back first.advanced.database.generateIdandadvanced.database.joinsthrow at config time inassertNoUnsupportedPassthroughKeys.betterAuthOptions.databasestays refused.createAuth(config, rawOpensaasContext)keeps its signature.getDatabaseConfigbuilds the adapter from the resolved context's Unsafe surface where it calledprismaAdapterbefore.AccessContextdeliberately does not nameunsafe(ADR-0038), so the surface is read off the running request context through a structural check and refused loudly (AuthUnsafeSurfaceMissingError) when absent, rather than widening the public signature.The
or()decisionNo internal import was needed. ADR-0060 anticipated importing the combinators from an internal package and accepted that, with typed SQL throughout as the fallback. At
8.0.0-rc.8or,andandnotare exported from@prisma/orm-postgres/orm-client, a published subpath of the package the stack already pins — the "internal" caveat in Prisma's own skill docs is about the@internal/…workspace alias, not the published surface.AnyExpression,OrderByItem,Expression,CodecRef,codecOfandparamare likewise public on@prisma/orm-postgres/relational-core, so the adapter's lane types are Prisma's own rather than hand-declared stand-ins.@prisma/orm-postgresis added as a peer + dev dependency ofpackages/auth.The LIKE escaper
ADR-0060 says the pattern operators "reuse the engine-owned LIKE escaping ADR-0055 introduced … no second escaper". That helper did not exist on
prisma-8— ADR-0055's Where vocabulary is #1147, still open. So it is added here, inpackages/core/src/where/like.ts, exported from@opensaas/stack-core/internal:LIKE_ESCAPE_CHARACTER,escapeLikeLiteral, and the four pattern builders (likeEqualsPattern/likeContainsPattern/likeStartsWithPattern/likeEndsWithPattern). Nothing private lives inpackages/auth.#1147 must adopt this helper rather than write a second one.
Tripwire coverage
packages/auth/tests/adapter-tripwire.test.tsdrives all ten methods over the Test context with the tripwire installed (throw mode, no warn mode) plus a plan recorder. Every recorded plan's origin is'unsafe'.update,deleteandconsumeOneeach assert more than one recorded plan — a single-row ORMupdate()/delete()is two statements, not one… RETURNING— and every one of them is marked.incrementOneand empty-wheredeleteManyeach assert exactly one plan, of the expected AST kind.Conformance
packages/auth/tests/adapter-conformance.test.tsruns@better-auth/test-utils@1.7.1'stestAdapterovercreateTestContext's in-process Postgres, rebuilding the database whenever the resolved better-auth tables change. 159 passed, 73 skipped, 0 failed.Suites run: normal, uuid, caseInsensitive.
Stated skips, all in the test file with their reasons:
supportsNumericIds: false.joinsTestSuite, plus every join-named test inside normal/uuid) — the adapter implements none, andadvanced.database.joinsis refused at config time rather than left to better-auth's silent per-model fallback.create - should use generateId if provided— the database mints every auth id; an app-supplied generator is ignored here and refused at config time.create - should enforce the issuer-scoped account identity key— better-auth declares that@@uniquetable-level, whichderiveAuthListsdoes not yet emit (auth: upgrade better-auth to 1.7.1 — new requiredaccount.issuercolumn, new@@unique([issuer, accountId]), and a stale^1.3.29peer floor #986). A schema gap, not an adapter one.create - should return null for nullable foreign keys— a reference field whose name does not end inIdderives a relation field and its own FK column under the same name, and the relation shadows the column on the returned row. AlsoderiveAuthLists; better-auth's own tables all name theirs<target>Id.Two behaviours the suites forced into the open, both handled in the adapter rather than papered over in the harness:
timestamptzcodec decodes to a string at rc.8, so the adapter declaressupportsDates: falseand lets better-auth's own string↔Dateconversion keep its contract true.pg/int8codec refuses a JSnumber, which is how better-auth carries abigint: truefield. The adapter widens on the way in and narrows on the way out, keyed off the field attribute's own flag.Out of scope (left to #1162)
The factory's transaction option, the transactions and authFlow suites, the plain-Node anchor, and the package-manifest / guidance sweep (dropping the
@prisma/clientpeer, de-namingprismaAdapterin the CLAUDE.md files).transaction: falseis declared and stated as a known limit.Verification
pnpm lint— 0 errors (2 pre-existing warnings, untouched files)pnpm manypkg fix,pnpm format— cleanpnpm build— 11/11 taskspackages/auth— 472 passed, 77 skippedpackages/core— 1555 passed, 1 skippedpackages/cli— 335 passed🤖 Generated with Claude Code