fix(plugin-auth): honour better-auth Where.mode and normalise the SCIM identifier (#5814) - #7124
Conversation
…M identifier (#5814) convertWhere() read field/operator/value and never `mode`, so a SCIM `userName eq "Alice@example.com"` lookup (mode: 'insensitive', because RFC 7643 marks userName caseExact:false) was answered case-sensitively — matching or not depending on the driver, and provisioning a duplicate user rather than raising, because SCIM's path is "look up, create if absent". Both halves of the maintainer's option-3 ruling: - NORMALISED_IDENTIFIER_FIELDS declares the identifier set ({ user: ['email'] }, the field @better-auth/scim actually maps userName onto) and drives the read and write halves from one place, so a field cannot join one of them only. Stored lower-cased, compared lower-cased — no new query vocabulary. - convertWhere() handles `mode` explicitly: satisfied by construction on a normalised identifier, and a loud warning naming model, field and operator on any other field, instead of silently answering case-sensitively. `sensitive` / absent-mode clauses keep their comparand byte-for-byte. No migration: every existing producer already lower-cased user.email. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BM1tNf5U3nEbHKR4fo5qVQ
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 1 package(s): 8 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 2 release-owned page(s) also reference the affected code. These are read-only:
|
github-actions
Bot
added
documentation
|
Downstream sweep: green.
Recording the false red honestly, since it cost a lap and reads exactly like a broken import: the first run of this suite failed a dozen files outright. That was the AGENTS.md §9 stale-artefact trap, not this change — Full local verification for this PR, per gate:
The family gates were run one by one rather than trusting the aggregate, because they run inside the ESLint job and a red one surfaces nowhere in CI's own conclusions are the verdict that counts; this is the local half. Generated by Claude Code |
|
PM review — PASS. Marked ready and enqueued ( Verified independently, not taken on report
The two ruled-out options are PINNED ABSENT, which is better than merely not done
expect(JSON.stringify(query.where)).not.toContain('$ieq');
expect(JSON.stringify(query.where)).not.toContain('$icontains');So a future change cannot quietly reintroduce either shape without a test going red. That is a stronger guarantee than the ruling asked for. The anchor correction is the most valuable thing in this PRThe card's own title and body say The existing-rows question — measured, and correctly did NOT become a decision cardThe envelope flagged this as the likely The residual case (a mixed-case row written outside the normalising paths staying unreachable from a SCIM insensitive lookup) is pre-existing and unchanged in direction — SCIM already compared a lower-cased comparand against Reverse verification — accepted, including the prediction that missedPredicted 13 red / 9 green in writing first; measured exactly that split. One honest delta reported rather than smoothed: the dev expected the missing export to fail the whole file and mask the silence-class faces, requiring a second surgical mutation — measured, vitest resolves a missing named export to One thing this seat fixed rather than bouncedThe PR body was missing the Claude Code attribution footer required of every GitHub artifact here. Appended by this seat rather than sent back for a round trip — the author was twice killed mid-task by token limits (host-environment blocks, recorded as such and never counted against the run), and a round trip for a two-line footer would have risked a third. Generated by Claude Code |
2 participants
Fixes #5814
Implements the maintainer's option 3 ruling of 2026-08-09 — both halves. Re-measured against
origin/main@0bffdae(the card's anchors were taken at1624f4aand had drifted; the drift is recorded below).The defect, re-verified at this branch point
better-auth's
Wherecarries a fourth field —mode?: "sensitive" | "insensitive",@default "sensitive"(@better-auth/core/dist/db/adapter/index.d.mts:293-312) — andconvertWhere()readfield/operator/valueand nothing else. The default covers almost every caller, so the drop was invisible; the caller it is not invisible for is the one that explicitly asked.@better-auth/scimis that caller, and the measurement moved one detail of the card: SCIM'suserNameis mapped onto better-auth'semailfield, not onto auserNamecolumn —SCIMUserFilterAttributeFields = { userName: "email" }(@better-auth/scim@1.7.0-rc.1/dist/index.mjs:531). Because RFC 7643 marksuserNamecaseExact: false(:409-417), the parsed clause carriesmode: "insensitive"(:576), and SCIM maps onlyeq(:530). So the clause reaching this adapter is{ field: 'email', operator: 'eq', mode: 'insensitive' }on modeluser.With
modeunread, that lookup matched a user stored asalice@example.comor not depending on the driver, and since SCIM provisioning is "look up, create if absent", a missed match did not raise — it provisioned a second user. This is the fail-open twin of #5813's fail-closed dropped predicate: there the query widened, here it answers a different question and looks fine doing it. Scope is unchanged: SCIM is off by default (OS_SCIM_ENABLED).Half 1 — normalisation, driven by one declared table
NORMALISED_IDENTIFIER_FIELDS = { user: ['email'] }names the set, keyed by better-auth model name, and both directions read it:normaliseIdentifierWrite()lower-cases the declared fields oncreate/update/updateMany, in the factory adapter and in the raw one;mode: 'insensitive'comparand on a declared field is lower-cased (element-wise forin/not_in), which is then an exact match against the stored form.One declaration for both halves is the point: a field cannot be added to the compare half without also being normalised on write, which would be the mirror-image defect — a lower-cased comparand hunting rows nobody lower-cased. A name heuristic (
/email$/) would have allowed exactly that, so the set is spelled out and pinned by value; widening it is a deliberate act that has to come to the pin and say so.No new query vocabulary is involved: the emitted filter stays a plain equality.
Half 2 — the silent drop ends
convertWhere()now readscondition.mode, applying the producer's documented default explicitly (?? 'sensitive') for the same reasonoperator ?? 'eq'is spelled out beside it: the factory materialises the default (transformWhereClause,mode = "sensitive"), but the rawcreateObjectQLAdapteris handed clauses that never passed through it.console.warnnaming the model, the field and the operator, and stating that the query is being answered case-sensitively so a differently-cased value will not match.It deliberately does not throw. That is the ruling's own reasoning — fail-closed here would upgrade an occasional duplicate user into "
userNamequeries entirely unavailable" — and it is the level AGENTS.md's degradation question asks for: nothing claims to have persisted, the caller simply gets a narrower answer than requested, which is functional degradation ⇒warn, noterror.pnpm check:durability-log-levelpasses.A
sensitiveor absentmodekeeps its comparand byte-for-byte, on a normalised column too. Folding case unasked would answer a different question than the one put — the same failure in the opposite direction.Existing rows: why no migration, and why none is needed
This was the question most likely to force a
needs_decision, so it was measured rather than assumed:user.emailon write. better-auth's owninternalAdapterdoes it oncreateUser/createOAuthUser/updateUser/updateUserByEmail(better-auth@1.7.0-rc.2/dist/db/internal-adapter.mjs:120,139,594,607);@better-auth/scimdoes it again on its create path (dist/index.mjs:2227); ObjectStack's own paths do too (admin-user-endpoints.ts:461,497,admin-import-users.ts:177). The write half is therefore idempotent over every producer that exists — it adds no behaviour to any current write. What it adds is ownership: the invariant the read half depends on now lives in the layer that depends on it, instead of being inherited from an internal of a prerelease dependency that no published type describes, and it also covers the raw adapter path, which bypasses better-auth entirely.mode: 'insensitive', which only SCIM sends. Everysensitive/absent query is unchanged byte-for-byte, so no stored row changes its match behaviour for any query shape in use today.@better-auth/scim's own provisioning path already compares a lower-cased comparand againstuser.emailwith nomodeat all (dist/index.mjs:2227-2234), so it was already invisible to SCIM before this change and in the same direction. Normalising the comparand takes nothing away from it. This is stated in the test file's "what is deliberately NOT pinned" section rather than left for the next reader.Deliberately not done
$ieq(option 1) — deferred for demonstrated pull, and it would need an in-memory execution face before it could joinFILTER_OPERATORSat all (filter.zod.ts's own recorded fail-closed reasoning; the [spec/drivers]AggregationNode.distinctis honoured by the in-memory fallback and ignored by every SQL face — one query, two numbers (ADR-0049) #6815 one-query-two-numbers class).eq + insensitive→$icontainsdowngrade (option 2) — containment is not equality.packages/spec's filter vocabulary is untouched, and SCIM's default-off posture is unchanged.Tests — 22 pins across five faces
packages/plugins/plugin-auth/src/scim-case-insensitive-identifier.test.ts, following the harness its two siblings already use (@objectstack/driver-sql+ better-sqlite3:memory:).=really is case-exact. Without it the behavioural pin would be always-green on any backend that folds case, and would say nothing about the adapter.sensitive/absent mode. The silence half is what makes the firing half mean something.create/update/updateMany, factory and raw, plus a model outside the set left untouched.emailand notuserName, since a set spelleduserNamewould look plausible and match nothing.Reverse verification — direction predicted in writing before running
Prediction recorded first, then the whole fix removed (
git checkout origin/main -- objectql-adapter.ts; nogit stash— that stack is shared across worktrees).Measured: 13 red / 9 green, the exact split predicted. Red: both contract pins, both behavioural pins, the both-halves round trip, both warning pins, all four write pins, both set pins — e.g.
expected { email: 'Alice@Example.com' } to deeply equal { email: 'alice@example.com' }andexpected [] to deeply equal [ 'u_alice' ](the duplicate-user symptom, witnessed).Green, correctly: the sensitive/absent comparand pins, the raw-adapter default pin, the no-match pin, loud-is-not-fail-closed, both silence pins, the non-set model pin — and the discrimination pin, which is the one that proves sqlite's
=is case-exact and therefore that the behavioural pin can fail at all. A guard that correctly stays green under its own mutation is a result, not a gap.Honest delta — one prediction was wrong. I predicted that the deleted
NORMALISED_IDENTIFIER_FIELDSexport would fail the whole file under the mutation and mask the two silence-class faces, requiring a second surgical mutation to read them. Measured: vitest/esbuild resolves a missing named export toundefinedrather than throwing at import time, so only the two set-pins went red (expected undefined to deeply equal { user: ['email'] }) and those faces were readable after all. The second mutation was therefore unnecessary and not run. The prediction about those faces themselves — that they stay green — held.Verification
pnpm --filter @objectstack/plugin-auth test— 996 passed / 996, 40 files.pnpm --filter @objectstack/runtime test— 1837 passed / 1837, 118 files (downstream consumer; see the comment below for the stale-distfalse red that preceded it).pnpm --filter @objectstack/plugin-auth typecheck— clean (tsc --noEmit, no output).pnpm lint— clean.check:engine-double-contract,check:error-code-casing,check:route-envelope,check:durability-log-level,check:nul-bytes,check:changeset-gate-self-tests,check:published-files,check:query-options-erasure..changeset/scim-case-insensitive-identifier.md(@objectstack/plugin-auth: patch).CI on this head has converged: 25 check runs, 23 success + 2 skipped (path-filtered), zero failures — including ESLint (the job the family gates run inside) and TypeScript Type Check.
Generated by Claude Code