From 6ce7f8c6b3d0788e04369482e6c0103a2ebbcf3d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 18:13:25 +0000 Subject: [PATCH] fix(plugin-auth): honour better-auth Where.mode and normalise the SCIM identifier (#5814) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01BM1tNf5U3nEbHKR4fo5qVQ --- .../scim-case-insensitive-identifier.md | 47 ++ .../plugin-auth/src/objectql-adapter.ts | 247 +++++++-- .../scim-case-insensitive-identifier.test.ts | 502 ++++++++++++++++++ 3 files changed, 760 insertions(+), 36 deletions(-) create mode 100644 .changeset/scim-case-insensitive-identifier.md create mode 100644 packages/plugins/plugin-auth/src/scim-case-insensitive-identifier.test.ts diff --git a/.changeset/scim-case-insensitive-identifier.md b/.changeset/scim-case-insensitive-identifier.md new file mode 100644 index 0000000000..bc0a1a11f1 --- /dev/null +++ b/.changeset/scim-case-insensitive-identifier.md @@ -0,0 +1,47 @@ +--- +"@objectstack/plugin-auth": patch +--- + +fix(plugin-auth): honour better-auth's `Where.mode`, and normalise the identifier SCIM matches on (#5814) + +better-auth's `Where` carries a fourth field — `mode?: "sensitive" | "insensitive"`, +`@default "sensitive"` — and `convertWhere()` in the ObjectQL adapter read `field` / +`operator` / `value` and 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/scim` is that caller. SCIM's `userName` is case-insensitive by RFC 7643 +(`caseExact: false`), so a `filter=userName eq "Alice@example.com"` reaches this adapter +as `{ field: 'email', operator: 'eq', mode: 'insensitive' }`. With `mode` unread, whether +it matched a user stored as `alice@example.com` came down to how the driver under the +auth path happens to compare strings — and because SCIM provisioning is "look up, create +if absent", a missed match did not raise an error, it provisioned a **second user**. +Only deployments that turned SCIM on (`OS_SCIM_ENABLED`, off by default) were exposed. + +Both halves of the fix, per the maintainer's ruling on #5814: + +- **Normalisation, not new vocabulary.** `sys_user.email` — the field SCIM's `userName` + maps onto — is now stored lower-cased and compared lower-cased by this adapter. An + insensitive lookup lower-cases its comparand, which is an *exact* match against the + stored form, so nothing in the query vocabulary changes. The set is a declared table + (`NORMALISED_IDENTIFIER_FIELDS`), not a name heuristic, and it drives the read and + write halves from one place so a field cannot be added to one of them only. +- **The silent drop ends.** `convertWhere()` handles `mode` explicitly. On a normalised + identifier the request is satisfied by construction. On **any other** field, a + `mode: 'insensitive'` clause now emits a loud warning naming the model, the field and + the operator, and stating that the query is being answered case-sensitively — instead + of answering a different question and looking fine doing it. It deliberately does not + throw: refusing here would turn an occasional duplicate user into "`userName` queries + entirely unavailable", which is the worse trade on an authentication path. + +No migration ships and none is needed. Every existing write path already lower-cased +`user.email` before reaching the adapter (better-auth's own `internalAdapter` does it on +`createUser` / `createOAuthUser` / `updateUser` / `updateUserByEmail`, and SCIM's create +path does it again), so the write half changes no existing behaviour — it moves the +invariant the read half depends on into the layer that depends on it, instead of +inheriting it from an internal of a prerelease dependency. Queries that do not set +`mode`, or set it to `"sensitive"`, keep their comparand byte-for-byte: folding case +unasked would be the same failure in the opposite direction. + +Adding a case-insensitive equality operator (`$ieq`) was deferred until there is +demonstrated pull for it, and downgrading `eq + insensitive` to `$icontains` was +rejected — containment is not equality. diff --git a/packages/plugins/plugin-auth/src/objectql-adapter.ts b/packages/plugins/plugin-auth/src/objectql-adapter.ts index d3546dd549..0f89b4f278 100644 --- a/packages/plugins/plugin-auth/src/objectql-adapter.ts +++ b/packages/plugins/plugin-auth/src/objectql-adapter.ts @@ -134,6 +134,107 @@ export const SUPPORTED_WHERE_OPERATORS = [ 'ends_with', ] as const satisfies readonly WhereOperator[]; +// --------------------------------------------------------------------------- +// Case-insensitive identifiers — the normalised set (#5814) +// --------------------------------------------------------------------------- + +/** + * The identifier fields this adapter stores and compares **normalised** + * (lower-cased), per the maintainer's 2026-08-09 ruling on #5814. + * + * ## Why a declared table and not a name heuristic + * + * Membership is a contract with two obligations that must hold **together**: + * + * 1. every write through this adapter normalises the field + * ({@link normaliseIdentifierWrite}), and + * 2. a `mode: 'insensitive'` comparand on the field is normalised the same + * way ({@link convertWhere}), which is then an EXACT match against (1). + * + * Honour (2) without (1) and you get the mirror-image defect of the one #5814 + * was filed on: the query stops matching rows it used to match. So the set is + * spelled out, keyed by better-auth model name, and both directions are driven + * from this one declaration — a field cannot be added to one half only. A + * heuristic (`/email$/`, `endsWith('_email')`) would let a field into the + * compare half without anyone deciding that its writes are normalised, which is + * exactly the widening this table exists to prevent. + * + * `sys_user.email` is the whole set today because it is the whole demand: + * `@better-auth/scim` maps SCIM's `userName` onto better-auth's **`email`** + * field (`SCIMUserFilterAttributeFields = { userName: "email" }`, + * `@better-auth/scim@1.7.0-rc.1/dist/index.mjs:531`) and marks it + * `caseExact: false` (`:409-417`, RFC 7643), so the where clause that reaches + * this adapter is `{ field: 'email', operator: 'eq', mode: 'insensitive' }` on + * model `user`. Adding a member is a deliberate act: it must ALSO be provable + * that every producer of that field writes it normalised, and + * `scim-case-insensitive-identifier.test.ts` pins the set so the addition + * cannot be quiet. + * + * Keyed by better-auth **model** name (`user`), not by the protocol object name + * (`sys_user`), because that is the name every call site in this file already + * holds and the name `Where.field` has been transformed against. `user` is not + * one of the bridged models (see `AUTH_MODEL_TO_PROTOCOL`), so its field names + * reach `convertWhere` untouched by `remapWhere`, and `email` is spelled + * identically on both sides (`auth-schema-config.ts` maps only the names that + * actually differ). + */ +export const NORMALISED_IDENTIFIER_FIELDS: Readonly> = { + user: ['email'], +}; + +/** Is `field` on `model` stored and compared lower-cased? */ +function isNormalisedIdentifier(model: string, field: string): boolean { + return NORMALISED_IDENTIFIER_FIELDS[model]?.includes(field) ?? false; +} + +/** + * Lower-case a comparand, element-wise for the array-valued operators + * (`in` / `not_in`). + * + * Non-strings are returned untouched: better-auth's own `Where.mode` doc says + * the flag "Only applies to string values", and an identifier column holding a + * number/boolean/Date has no case to fold. + */ +function normaliseComparand(value: unknown): unknown { + if (typeof value === 'string') return value.toLowerCase(); + if (Array.isArray(value)) return value.map((v) => (typeof v === 'string' ? v.toLowerCase() : v)); + return value; +} + +/** + * Normalise the identifier fields of an outgoing write payload (#5814). + * + * This is obligation (1) of {@link NORMALISED_IDENTIFIER_FIELDS} — the half + * that makes the comparand normalisation in {@link convertWhere} an *exact* + * match rather than a hopeful one. + * + * It is deliberately NOT a redundant belt over better-auth's own lower-casing. + * better-auth's `internalAdapter` does lower-case `user.email` on + * `createUser` / `createOAuthUser` / `updateUser` / `updateUserByEmail` + * (`better-auth@1.7.0-rc.2/dist/db/internal-adapter.mjs:120,139,594,607`), but + * that is an *internal* of a **prerelease** dependency, invisible to any + * published type, and it does not cover the raw {@link createObjectQLAdapter} + * path (hand-built calls that never pass through better-auth at all). The + * invariant the read half depends on has to be owned where it is relied upon. + * + * Idempotent by construction, so a payload better-auth already normalised is + * unchanged — which is why this adds no behaviour to any existing write. + */ +function normaliseIdentifierWrite>(model: string, data: T): T { + const fields = NORMALISED_IDENTIFIER_FIELDS[model]; + if (!fields) return data; + let out: Record | undefined; + for (const field of fields) { + const value = data[field]; + if (typeof value !== 'string') continue; + const lowered = value.toLowerCase(); + if (lowered === value) continue; + out ??= { ...data }; + out[field] = lowered; + } + return (out ?? data) as T; +} + /** * Convert better-auth where clause to ObjectQL query format. * @@ -171,10 +272,44 @@ export const SUPPORTED_WHERE_OPERATORS = [ * of this translation — better-auth's `Where.mode` defaults to `"sensitive"`, * and `$startsWith` / `$endsWith` are case-sensitive at the contract layer per * the #5701 Q2=A ruling — so the direct translation opens no contract seam. - * (`mode: 'insensitive'` is a separate, still-unread field; see #5814. It is - * NOT an operator and is deliberately not refused here.) + * + * ## `Where.mode` is read, never dropped (#5814) + * + * `mode` is better-auth's fourth `Where` field + * (`mode?: "sensitive" | "insensitive"`, `@default "sensitive"`); it is NOT an + * operator, and until #5814 this function did not read it at all. A dropped + * `mode: 'insensitive'` is the fail-OPEN direction of #5813's fail-closed + * defect: the query is answered, just case-sensitively, so whether it matches + * degrades to "however the driver under the auth path happens to compare + * strings". `@better-auth/scim` is the live producer — SCIM's `userName` is + * `caseExact: false` per RFC 7643 and maps onto `user.email` — and because + * SCIM's provisioning path is "look up, create if absent", the symptom of a + * missed match is a DUPLICATE USER rather than an error. + * + * Two arms, per the maintainer's 2026-08-09 ruling: + * + * - **A normalised identifier** ({@link NORMALISED_IDENTIFIER_FIELDS}): the + * comparand is lower-cased. The column is stored lower-cased + * ({@link normaliseIdentifierWrite}), so this is an EXACT case-insensitive + * match with no new query vocabulary — the request is honoured, not + * approximated. + * - **Any other field**: a loud `console.warn` naming the model, the field + * and the operator, and stating that the query is being answered + * case-sensitively. It does NOT throw: the ruling's own reasoning is that + * fail-closed here would upgrade an occasional duplicate into "userName + * queries entirely unavailable", and by AGENTS.md's degradation-level + * question this is a functional degradation — the caller gets a narrower + * answer than asked for and nothing claims to have persisted — so `warn`, + * not `error`. The one thing it may not be is silent. + * + * Why no `$ieq`: adding a case-insensitive equality operator was **deferred** + * for demonstrated pull (#5814 option 1), and it would need an in-memory + * execution face before it could join `FILTER_OPERATORS` at all — the + * fail-closed reasoning `packages/spec/src/data/filter.zod.ts` records for + * `$icontains`. Downgrading `eq + insensitive` to `$icontains` was **rejected** + * (option 2): containment is not equality. */ -function convertWhere(where: CleanedWhere[]): Record { +function convertWhere(model: string, where: CleanedWhere[]): Record { const filter: Record = {}; for (const condition of where) { @@ -189,36 +324,69 @@ function convertWhere(where: CleanedWhere[]): Record { // that IS spelled out must be in the vocabulary. const operator = condition.operator ?? 'eq'; + // `mode` gets the same treatment as `operator` above and for the same + // reason: better-auth's factory materialises the documented default + // (`mode = "sensitive"` in `transformWhereClause`) before an adapter sees + // the clause, but the raw `createObjectQLAdapter` below is handed clauses + // that never passed through it — so the producer's own default is applied + // here explicitly rather than assumed present. + const mode = condition.mode ?? 'sensitive'; + const insensitive = mode === 'insensitive'; + const normalisedIdentifier = insensitive && isNormalisedIdentifier(model, fieldName); + + if (insensitive && !normalisedIdentifier) { + // Loud, and it names all three things an operator needs to act: which + // query, which field, and what was actually done instead. + console.warn( + `[plugin-auth] Case-insensitive match requested on '${model}.${fieldName}' ` + + `(operator '${operator}', better-auth \`Where.mode: 'insensitive'\`), but ` + + `'${model}.${fieldName}' is not a normalised identifier field — the query is ` + + `being answered CASE-SENSITIVELY, so a differently-cased value will not match. ` + + `ObjectQL has no case-insensitive equality operator (#5814 deferred \`$ieq\` ` + + `until there is demonstrated pull for it). Normalised identifier fields: ` + + `${Object.entries(NORMALISED_IDENTIFIER_FIELDS) + .map(([m, fs]) => fs.map((f) => `${m}.${f}`).join(', ')) + .join(', ')} ` + + `(packages/plugins/plugin-auth/src/objectql-adapter.ts).`, + ); + } + + // Honour the caller's DECLARED mode: a `sensitive` (or absent) clause keeps + // its comparand byte-for-byte, even on a normalised column. Folding case + // unasked would answer a different question than the one put — the same + // failure in the opposite direction. + const value = normalisedIdentifier ? normaliseComparand(condition.value) : condition.value; + switch (operator) { case 'eq': - filter[fieldName] = condition.value; + filter[fieldName] = value; break; case 'ne': - filter[fieldName] = { $ne: condition.value }; + filter[fieldName] = { $ne: value }; break; case 'in': - filter[fieldName] = { $in: condition.value }; + filter[fieldName] = { $in: value }; break; case 'not_in': - filter[fieldName] = { $nin: condition.value }; + filter[fieldName] = { $nin: value }; break; case 'gt': - filter[fieldName] = { $gt: condition.value }; + filter[fieldName] = { $gt: value }; break; case 'gte': - filter[fieldName] = { $gte: condition.value }; + filter[fieldName] = { $gte: value }; break; case 'lt': - filter[fieldName] = { $lt: condition.value }; + filter[fieldName] = { $lt: value }; break; case 'lte': - filter[fieldName] = { $lte: condition.value }; + filter[fieldName] = { $lte: value }; break; case 'starts_with': - filter[fieldName] = { $startsWith: condition.value }; + filter[fieldName] = { $startsWith: value }; break; case 'ends_with': - filter[fieldName] = { $endsWith: condition.value }; + filter[fieldName] = { $endsWith: value }; break; case 'contains': // [#5710] `$contains`, NOT `$regex`. better-auth's `contains` is a @@ -248,7 +416,7 @@ function convertWhere(where: CleanedWhere[]): Record { // `driver-memory/src/filter-refusal.ts` means by "refusing it here would // break a live producer", and the reason #5702's loud refusal is ordered // after this flip. - filter[fieldName] = { $contains: condition.value }; + filter[fieldName] = { $contains: value }; break; default: { // Unreachable for the vocabulary this adapter is compiled against — @@ -491,7 +659,8 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { ): Promise => { const objectName = resolveProtocolName(model); const bridged = objectName !== model; - const result = await dataEngine.insert(objectName, bridged ? remapKeys(data, camelToSnake) : data); + const payload = normaliseIdentifierWrite(model, data); + const result = await dataEngine.insert(objectName, bridged ? remapKeys(payload, camelToSnake) : payload); const norm = normaliseLegacyDates(model, result); return (bridged ? remapKeys(norm, snakeToCamel) : norm) as T; }, @@ -501,7 +670,7 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { ): Promise => { const objectName = resolveProtocolName(model); const bridged = objectName !== model; - const filter = convertWhere(bridged ? remapWhere(where) : where); + const filter = convertWhere(model, bridged ? remapWhere(where) : where); const fields = bridged && select ? select.map(camelToSnake) : select; const result = await dataEngine.findOne(objectName, { where: filter, fields }); @@ -518,7 +687,7 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { ): Promise => { const objectName = resolveProtocolName(model); const bridged = objectName !== model; - const filter = where ? convertWhere(bridged ? remapWhere(where) : where) : {}; + const filter = where ? convertWhere(model, bridged ? remapWhere(where) : where) : {}; const orderBy = sortBy ? [{ field: bridged ? camelToSnake(sortBy.field) : sortBy.field, order: sortBy.direction as 'asc' | 'desc' }] @@ -542,7 +711,7 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { ): Promise => { const objectName = resolveProtocolName(model); const bridged = objectName !== model; - const filter = where ? convertWhere(bridged ? remapWhere(where) : where) : {}; + const filter = where ? convertWhere(model, bridged ? remapWhere(where) : where) : {}; return await dataEngine.count(objectName, { where: filter }); }, @@ -551,13 +720,14 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { ): Promise => { const objectName = resolveProtocolName(model); const bridged = objectName !== model; - const filter = convertWhere(bridged ? remapWhere(where) : where); + const filter = convertWhere(model, bridged ? remapWhere(where) : where); // ObjectQL requires an ID for updates – find the record first const record = await dataEngine.findOne(objectName, { where: filter }); if (!record) return null; - const patch = bridged ? remapKeys(update as any, camelToSnake) : (update as any); + const normalised = normaliseIdentifierWrite(model, update as any); + const patch = bridged ? remapKeys(normalised, camelToSnake) : normalised; const result = await dataEngine.update(objectName, { ...patch, id: record.id }); if (!result) return null; const norm = normaliseLegacyDates(model, result); @@ -569,11 +739,12 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { ): Promise => { const objectName = resolveProtocolName(model); const bridged = objectName !== model; - const filter = convertWhere(bridged ? remapWhere(where) : where); + const filter = convertWhere(model, bridged ? remapWhere(where) : where); // Sequential updates: ObjectQL requires an ID per update const records = await dataEngine.find(objectName, { where: filter }); - const patch = bridged ? remapKeys(update, camelToSnake) : update; + const normalised = normaliseIdentifierWrite(model, update); + const patch = bridged ? remapKeys(normalised, camelToSnake) : normalised; for (const record of records) { await dataEngine.update(objectName, { ...patch, id: record.id }); } @@ -585,7 +756,7 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { ): Promise => { const objectName = resolveProtocolName(model); const bridged = objectName !== model; - const filter = convertWhere(bridged ? remapWhere(where) : where); + const filter = convertWhere(model, bridged ? remapWhere(where) : where); const record = await dataEngine.findOne(objectName, { where: filter }); if (!record) return; @@ -598,7 +769,7 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { ): Promise => { const objectName = resolveProtocolName(model); const bridged = objectName !== model; - const filter = convertWhere(bridged ? remapWhere(where) : where); + const filter = convertWhere(model, bridged ? remapWhere(where) : where); const records = await dataEngine.find(objectName, { where: filter }); for (const record of records) { @@ -615,7 +786,7 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { ): Promise => { const objectName = resolveProtocolName(model); const bridged = objectName !== model; - const filter = convertWhere(bridged ? remapWhere(where) : where); + const filter = convertWhere(model, bridged ? remapWhere(where) : where); const record = await dataEngine.findOne(objectName, { where: filter }); if (!record) return null; @@ -637,7 +808,7 @@ export function createObjectQLAdapterFactory(rawDataEngine: IDataEngine) { ): Promise => { const objectName = resolveProtocolName(model); const bridged = objectName !== model; - const filter = convertWhere(bridged ? remapWhere(where) : where); + const filter = convertWhere(model, bridged ? remapWhere(where) : where); const record = await dataEngine.findOne(objectName, { where: filter }); if (!record) return null; @@ -682,20 +853,20 @@ export function createObjectQLAdapter(rawDataEngine: IDataEngine) { return { create: async >({ model, data, select: _select }: { model: string; data: T; select?: string[] }): Promise => { const objectName = resolveProtocolName(model); - const result = await dataEngine.insert(objectName, data); + const result = await dataEngine.insert(objectName, normaliseIdentifierWrite(model, data)); return result as T; }, findOne: async ({ model, where, select, join: _join }: { model: string; where: CleanedWhere[]; select?: string[]; join?: any }): Promise => { const objectName = resolveProtocolName(model); - const filter = convertWhere(where); + const filter = convertWhere(model, where); const result = await dataEngine.findOne(objectName, { where: filter, fields: select }); return result ? result as T : null; }, findMany: async ({ model, where, limit, offset, sortBy, join: _join }: { model: string; where?: CleanedWhere[]; limit: number; offset?: number; sortBy?: { field: string; direction: 'asc' | 'desc' }; join?: any }): Promise => { const objectName = resolveProtocolName(model); - const filter = where ? convertWhere(where) : {}; + const filter = where ? convertWhere(model, where) : {}; const orderBy = sortBy ? [{ field: sortBy.field, order: sortBy.direction as 'asc' | 'desc' }] : undefined; const results = await dataEngine.find(objectName, { where: filter, limit: limit || 100, offset, orderBy }); return results as T[]; @@ -703,32 +874,36 @@ export function createObjectQLAdapter(rawDataEngine: IDataEngine) { count: async ({ model, where }: { model: string; where?: CleanedWhere[] }): Promise => { const objectName = resolveProtocolName(model); - const filter = where ? convertWhere(where) : {}; + const filter = where ? convertWhere(model, where) : {}; return await dataEngine.count(objectName, { where: filter }); }, update: async ({ model, where, update }: { model: string; where: CleanedWhere[]; update: Record }): Promise => { const objectName = resolveProtocolName(model); - const filter = convertWhere(where); + const filter = convertWhere(model, where); const record = await dataEngine.findOne(objectName, { where: filter }); if (!record) return null; - const result = await dataEngine.update(objectName, { ...update, id: record.id }); + const result = await dataEngine.update(objectName, { + ...normaliseIdentifierWrite(model, update), + id: record.id, + }); return result ? result as T : null; }, updateMany: async ({ model, where, update }: { model: string; where: CleanedWhere[]; update: Record }): Promise => { const objectName = resolveProtocolName(model); - const filter = convertWhere(where); + const filter = convertWhere(model, where); const records = await dataEngine.find(objectName, { where: filter }); + const patch = normaliseIdentifierWrite(model, update); for (const record of records) { - await dataEngine.update(objectName, { ...update, id: record.id }); + await dataEngine.update(objectName, { ...patch, id: record.id }); } return records.length; }, delete: async ({ model, where }: { model: string; where: CleanedWhere[] }): Promise => { const objectName = resolveProtocolName(model); - const filter = convertWhere(where); + const filter = convertWhere(model, where); const record = await dataEngine.findOne(objectName, { where: filter }); if (!record) return; await dataEngine.delete(objectName, { where: { id: record.id } }); @@ -736,7 +911,7 @@ export function createObjectQLAdapter(rawDataEngine: IDataEngine) { deleteMany: async ({ model, where }: { model: string; where: CleanedWhere[] }): Promise => { const objectName = resolveProtocolName(model); - const filter = convertWhere(where); + const filter = convertWhere(model, where); const records = await dataEngine.find(objectName, { where: filter }); for (const record of records) { await dataEngine.delete(objectName, { where: { id: record.id } }); diff --git a/packages/plugins/plugin-auth/src/scim-case-insensitive-identifier.test.ts b/packages/plugins/plugin-auth/src/scim-case-insensitive-identifier.test.ts new file mode 100644 index 0000000000..b78c48d3a4 --- /dev/null +++ b/packages/plugins/plugin-auth/src/scim-case-insensitive-identifier.test.ts @@ -0,0 +1,502 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5814] `convertWhere()` reads better-auth's `Where.mode`, and the identifier + * it is asked to match case-insensitively is stored normalised. + * + * ## The defect + * + * better-auth's `Where` has a fourth field — + * `mode?: "sensitive" | "insensitive"`, `@default "sensitive"` — and + * `convertWhere()` read `field` / `operator` / `value` and nothing else. The + * default covers almost every caller, so the drop was invisible; the callers it + * is NOT invisible for are the ones that explicitly asked. + * + * `@better-auth/scim` is the live producer. SCIM's `userName` is + * `caseExact: false` per RFC 7643, so `parseSCIMResourceFilter` attaches + * `mode: "insensitive"` to the parsed clause + * (`@better-auth/scim@1.7.0-rc.1/dist/index.mjs:576`), and SCIM maps only `eq` + * (`SCIMFilterOperatorMap = { eq: "eq" }`, `:530`) onto better-auth's **email** + * field (`SCIMUserFilterAttributeFields = { userName: "email" }`, `:531`). + * + * With `mode` unread, `userName eq "Alice@example.com"` for a user stored as + * `alice@example.com` matched **or not depending on the driver**. And because + * SCIM provisioning is "look up, create if absent", a missed match does not + * raise — it provisions 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. + * + * ## The ruling this pins (maintainer, 2026-08-09, option 3 — both halves) + * + * 1. **Normalisation.** `userName` / email comparisons become + * case-insensitive by NORMALISING — stored lower-cased, compared + * lower-cased. No new query vocabulary: `$ieq` was deferred for + * demonstrated pull, and downgrading `eq + insensitive` to `$icontains` + * was rejected outright (containment is not equality). + * 2. **The silent drop ends.** `convertWhere()` handles `mode` explicitly. + * On a normalised identifier the request is satisfied by construction; on + * **any other field** the adapter warns loudly, naming the field and the + * unsupported request, instead of quietly answering case-sensitively. + * + * ## Faces, and what each one alone cannot see + * + * 1. CONTRACT — which comparand the adapter emits, read off a spy engine. A + * behavioural pass alone can be right for the wrong reason (e.g. a backend + * that folds case on its own). + * 2. BEHAVIOUR — what a REAL backend answers, plus the DISCRIMINATION pin + * that proves the backend is case-sensitive at all. Without face 2's + * discrimination arm, face 2 would be an always-green pin on any backend + * whose `=` folds case. + * 3. THE WARNING — that it fires for a non-identifier field and stays SILENT + * for `mode: 'sensitive'` / absent. The silence half is what makes the + * firing half mean something: a warning on every query would satisfy "it + * warns" while telling an operator nothing. + * 4. THE WRITE HALF — that the same declared set is normalised on the way + * IN. Face 1 + 2 without this would be a lower-cased comparand hunting + * rows nobody lower-cased: the mirror-image defect. + * 5. THE DECLARED SET — pinned by value, so widening it is a deliberate act + * that has to come here and say so. + * + * ## Backend note + * + * Face 2 runs on `@objectstack/driver-sql` + better-sqlite3 `:memory:`, the + * harness `auth-where-operator-coverage.test.ts` and `auth-contains-filter.test.ts` + * already use (#5704's programme, PR #5880). It is the right backend for this + * file specifically: SQLite's default `BINARY` collation makes `=` case-exact, + * which is precisely the "case-sensitive driver" the issue describes — the + * discrimination pin below measures that rather than assuming it. + * + * ## What is deliberately NOT pinned here + * + * A row written *outside* this adapter with a mixed-case email is not reachable + * by an insensitive lookup, and no migration ships with this change. That is + * not a gap this file hides: `@better-auth/scim`'s own provisioning path + * already compares a lower-cased comparand against `user.email` with no `mode` + * at all (`dist/index.mjs:2227-2234`), so such a row was already invisible to + * SCIM before this change and in the same direction. Normalising the comparand + * takes nothing away from it. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { SqlDriver } from '@objectstack/driver-sql'; +import type { QueryAST } from '@objectstack/spec/data'; +import type { IDataEngine } from '@objectstack/core'; +import { + createObjectQLAdapter, + createObjectQLAdapterFactory, + NORMALISED_IDENTIFIER_FIELDS, +} from './objectql-adapter'; + +/** + * The `sys_user` columns face 2 touches, declared — a real table has to be told. + * + * `email_verified` / `created_at` / `updated_at` are here because the last case + * in face 2 writes a row through the REAL translation, and better-auth's + * `transformInput` materialises its core user shape on every create. The + * sibling files (`auth-contains-filter.test.ts`) declare their fixture down to + * the two columns they assert on precisely because they never write one; the + * rule is the same rule — declare what the test actually exercises (#5806). + */ +const SYS_USER = { + name: 'sys_user', + fields: { + name: { type: 'text', name: 'name' }, + email: { type: 'text', name: 'email' }, + email_verified: { type: 'boolean', name: 'email_verified' }, + created_at: { type: 'datetime', name: 'created_at' }, + updated_at: { type: 'datetime', name: 'updated_at' }, + }, +}; + +/** + * The row the identity provider is looking for: stored the way every write + * path already stores it — lower-cased. + */ +const SEED = [ + { id: 'u_alice', name: 'Alice', email: 'alice@example.com' }, + { id: 'u_bob', name: 'Bob', email: 'bob@example.com' }, +]; + +/** + * A read-only engine facade over a REAL `SqlDriver`. + * + * Only the read verbs face 2 exercises are declared; seeding goes through the + * driver directly, so a hand-written `delete`/`update` here would be a dispatch + * contract this test neither needs nor is able to honour + * (`check:engine-double-contract`, #4550). + */ +function sqlReadEngine(driver: SqlDriver): IDataEngine { + // The query bag keeps its declared driver-side type with no `any` erasure — + // `query-options/no-any-erasure` (#4674/#4918) counts test-side calls too. + return { + find: (object: string, query: QueryAST) => driver.find(object, query), + findOne: (object: string, query: QueryAST) => driver.findOne(object, query), + count: (object: string, query?: QueryAST) => driver.count(object, query), + } as unknown as IDataEngine; +} + +/** + * Live `:memory:` databases, closed after each test — the database dies with + * its connection, so nothing touches the host filesystem. + */ +const openDrivers: SqlDriver[] = []; + +afterEach(async () => { + while (openDrivers.length) { + const driver = openDrivers.pop(); + try { await driver?.disconnect(); } catch { /* noop */ } + } +}); + +async function seededDriver() { + const driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + openDrivers.push(driver); + // Real DDL through the driver's own path — the table the rows land in is + // created by the backend, not conjured by a store on first write. + await driver.initObjects([SYS_USER]); + for (const row of SEED) await driver.create('sys_user', row); + return driver; +} + +async function seededAdapter() { + const driver = await seededDriver(); + const adapter: any = (createObjectQLAdapterFactory(sqlReadEngine(driver)) as any)({} as any); + return { driver, adapter }; +} + +/** + * The clause `@better-auth/scim` produces for `filter=userName eq ""`: + * its `userName` attribute maps onto the `email` field and carries + * `mode: 'insensitive'` because the SCIM schema marks it `caseExact: false`. + */ +function scimUserNameQuery(value: string, mode?: 'sensitive' | 'insensitive') { + return { + model: 'user', + where: [{ field: 'email', value, operator: 'eq', connector: 'AND', ...(mode ? { mode } : {}) }], + limit: 100, + } as any; +} + +function spyEngine(): IDataEngine { + return { + insert: vi.fn().mockResolvedValue({ id: '1' }), + findOne: vi.fn().mockResolvedValue(null), + find: vi.fn().mockResolvedValue([]), + count: vi.fn().mockResolvedValue(0), + update: vi.fn().mockResolvedValue({ id: '1' }), + delete: vi.fn().mockResolvedValue(undefined), + } as unknown as IDataEngine; +} + +// --------------------------------------------------------------------------- +// Face 1 — the contract: which comparand the translation emits +// --------------------------------------------------------------------------- + +describe('[#5814] convertWhere honours `mode: "insensitive"` on a normalised identifier', () => { + let engine: IDataEngine; + + beforeEach(() => { + engine = spyEngine(); + }); + + it('normalises the comparand of a SCIM `userName eq` lookup', async () => { + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + await adapter.findMany(scimUserNameQuery('Alice@Example.com', 'insensitive')); + + const [object, query] = (engine.find as any).mock.calls[0]; + expect(object).toBe('sys_user'); + expect(query.where).toEqual({ email: 'alice@example.com' }); + // Spelled out separately: the ruling forbids reaching for new vocabulary, + // so the emitted filter must stay a plain equality — no `$ieq`, and no + // containment downgrade. + expect(JSON.stringify(query.where)).not.toContain('$ieq'); + expect(JSON.stringify(query.where)).not.toContain('$icontains'); + expect(JSON.stringify(query.where)).not.toContain('$contains'); + }); + + it('leaves the comparand byte-identical when the caller asked for `sensitive`', async () => { + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + await adapter.findMany(scimUserNameQuery('Alice@Example.com', 'sensitive')); + + const [, query] = (engine.find as any).mock.calls[0]; + // Folding case unasked would answer a different question than the one put — + // the same failure in the opposite direction. + expect(query.where).toEqual({ email: 'Alice@Example.com' }); + }); + + it('leaves the comparand byte-identical when `mode` is absent (the default)', async () => { + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + await adapter.findMany(scimUserNameQuery('Alice@Example.com')); + + const [, query] = (engine.find as any).mock.calls[0]; + expect(query.where).toEqual({ email: 'Alice@Example.com' }); + }); + + it('normalises element-wise for the array-valued operators', async () => { + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + await adapter.findMany({ + model: 'user', + where: [{ + field: 'email', + value: ['Alice@Example.com', 'BOB@EXAMPLE.COM'], + operator: 'in', + connector: 'AND', + mode: 'insensitive', + }], + limit: 100, + } as any); + + const [, query] = (engine.find as any).mock.calls[0]; + expect(query.where).toEqual({ email: { $in: ['alice@example.com', 'bob@example.com'] } }); + }); + + it('applies the producer default on the raw adapter, whose clauses never pass the factory', async () => { + // `createObjectQLAdapter` is handed hand-built clauses, so `mode` really can + // be absent at runtime even though `CleanedWhere` types it as required — + // exactly the reason `operator ?? 'eq'` is spelled out next to it. + const adapter = createObjectQLAdapter(engine); + await adapter.findMany({ + model: 'user', + where: [{ field: 'email', value: 'Alice@Example.com', operator: 'eq' } as any], + limit: 100, + } as any); + + const [, query] = (engine.find as any).mock.calls[0]; + expect(query.where).toEqual({ email: 'Alice@Example.com' }); + }); +}); + +// --------------------------------------------------------------------------- +// Face 2 — the behaviour: the duplicate-user symptom, on a real backend +// --------------------------------------------------------------------------- + +describe('[#5814] the differently-cased SCIM lookup finds the existing user', () => { + it('DISCRIMINATION: the backend really is case-sensitive, so the pin below can fail', async () => { + const { adapter } = await seededAdapter(); + // Without this measurement the behavioural pin would be always-green on any + // backend whose `=` folds case, and would say nothing about the adapter. + const rows: any[] = await adapter.findMany(scimUserNameQuery('Alice@Example.com', 'sensitive')); + expect(rows).toEqual([]); + }); + + it('finds the user stored as `alice@example.com` — no second user gets provisioned', async () => { + const { adapter } = await seededAdapter(); + const rows: any[] = await adapter.findMany(scimUserNameQuery('Alice@Example.com', 'insensitive')); + expect(rows.map((r) => r.id)).toEqual(['u_alice']); + }); + + it('matches however the identity provider happens to shout it', async () => { + const { adapter } = await seededAdapter(); + const rows: any[] = await adapter.findMany(scimUserNameQuery('ALICE@EXAMPLE.COM', 'insensitive')); + expect(rows.map((r) => r.id)).toEqual(['u_alice']); + }); + + it('still matches nothing when there is nothing to match', async () => { + const { adapter } = await seededAdapter(); + // Case-insensitivity must not become a wildcard: #5813's lesson is that the + // expensive direction of a translation bug is the WIDENING one. + const rows: any[] = await adapter.findMany(scimUserNameQuery('Carol@Example.com', 'insensitive')); + expect(rows).toEqual([]); + }); + + it('BOTH HALVES: a mixed-case write is found by a differently-cased lookup', async () => { + // The end-to-end shape the ruling describes — normalised in, normalised on + // compare. It goes red if EITHER half is missing, which is why it is here + // in addition to the two halves' own pins. + const driver = await seededDriver(); + const engine = { + ...sqlReadEngine(driver), + insert: (object: string, data: Record) => driver.create(object, data), + } as unknown as IDataEngine; + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + + // No `id`: better-auth mints it, and passing one is refused by the factory. + await adapter.create({ model: 'user', data: { name: 'Carol', email: 'Carol@Example.COM' } }); + + const rows: any[] = await adapter.findMany(scimUserNameQuery('cArOl@eXaMpLe.com', 'insensitive')); + expect(rows.map((r) => r.email)).toEqual(['carol@example.com']); + }); +}); + +// --------------------------------------------------------------------------- +// Face 3 — the warning: loud for what cannot be honoured, silent otherwise +// --------------------------------------------------------------------------- + +describe('[#5814] an unhonourable `mode: "insensitive"` is loud, never silent', () => { + let engine: IDataEngine; + let warn: ReturnType; + + beforeEach(() => { + engine = spyEngine(); + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + warn.mockRestore(); + }); + + it('warns, naming the field and what was done instead, for a non-identifier field', async () => { + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + await adapter.findMany({ + model: 'user', + where: [{ field: 'name', value: 'ALICE', operator: 'eq', connector: 'AND', mode: 'insensitive' }], + limit: 100, + } as any); + + expect(warn).toHaveBeenCalledTimes(1); + const message = String(warn.mock.calls[0]?.[0]); + // The three things an operator needs to act on it. + expect(message).toContain('user.name'); + expect(message).toContain('insensitive'); + expect(message).toContain('CASE-SENSITIVELY'); + expect(message).toContain('#5814'); + }); + + it('answers the query anyway — loud is not fail-closed', async () => { + // Deliberate, and it is the ruling's own reasoning: refusing here would + // upgrade "an occasional duplicate user" into "userName queries entirely + // unavailable", which is the worse trade on an authentication path. + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + await adapter.findMany({ + model: 'user', + where: [{ field: 'name', value: 'ALICE', operator: 'eq', connector: 'AND', mode: 'insensitive' }], + limit: 100, + } as any); + + const [, query] = (engine.find as any).mock.calls[0]; + expect(query.where).toEqual({ name: 'ALICE' }); + }); + + it('stays silent for a normalised identifier — there is nothing unhonoured to report', async () => { + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + await adapter.findMany(scimUserNameQuery('Alice@Example.com', 'insensitive')); + expect(warn).not.toHaveBeenCalled(); + }); + + it('stays silent for `mode: "sensitive"` and for an absent `mode`', async () => { + // The half that makes the firing half mean something: a warning on every + // query satisfies "it warns" and tells an operator nothing. + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + await adapter.findMany({ + model: 'user', + where: [{ field: 'name', value: 'ALICE', operator: 'eq', connector: 'AND', mode: 'sensitive' }], + limit: 100, + } as any); + await adapter.findMany({ + model: 'user', + where: [{ field: 'name', value: 'ALICE', operator: 'eq', connector: 'AND' }], + limit: 100, + } as any); + + expect(warn).not.toHaveBeenCalled(); + }); + + it('names the model too — the same field name on another model is a different fact', async () => { + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + await adapter.findMany({ + model: 'verification', + where: [{ field: 'identifier', value: 'Alice@Example.com', operator: 'eq', connector: 'AND', mode: 'insensitive' }], + limit: 100, + } as any); + + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0]?.[0])).toContain('verification.identifier'); + }); +}); + +// --------------------------------------------------------------------------- +// Face 4 — the write half: the set is normalised on the way IN +// --------------------------------------------------------------------------- + +describe('[#5814] normalised identifiers are stored lower-cased', () => { + let engine: IDataEngine; + + beforeEach(() => { + engine = spyEngine(); + }); + + it('lower-cases `email` on create', async () => { + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + await adapter.create({ model: 'user', data: { email: 'Alice@Example.COM', name: 'Alice' } }); + + const [object, data] = (engine.insert as any).mock.calls[0]; + expect(object).toBe('sys_user'); + expect(data.email).toBe('alice@example.com'); + // The display name is NOT an identifier — it keeps the case its owner chose. + expect(data.name).toBe('Alice'); + }); + + it('lower-cases `email` on update', async () => { + (engine.findOne as any).mockResolvedValue({ id: 'u_alice' }); + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + await adapter.update({ + model: 'user', + where: [{ field: 'id', value: 'u_alice', operator: 'eq', connector: 'AND' }], + update: { email: 'Alice@Example.COM' }, + } as any); + + const [object, patch] = (engine.update as any).mock.calls[0]; + expect(object).toBe('sys_user'); + expect(patch.email).toBe('alice@example.com'); + }); + + it('lower-cases `email` on updateMany', async () => { + (engine.find as any).mockResolvedValue([{ id: 'u_alice' }]); + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + await adapter.updateMany({ + model: 'user', + where: [{ field: 'id', value: 'u_alice', operator: 'eq', connector: 'AND' }], + update: { email: 'Alice@Example.COM' }, + } as any); + + const [, patch] = (engine.update as any).mock.calls[0]; + expect(patch.email).toBe('alice@example.com'); + }); + + it('lower-cases on the raw adapter too — it bypasses better-auth entirely', async () => { + const adapter = createObjectQLAdapter(engine); + await adapter.create({ model: 'user', data: { email: 'Alice@Example.COM' } }); + + const [, data] = (engine.insert as any).mock.calls[0]; + expect(data.email).toBe('alice@example.com'); + }); + + it('touches nothing on a model outside the declared set', async () => { + const adapter: any = (createObjectQLAdapterFactory(engine) as any)({} as any); + await adapter.create({ + model: 'verification', + data: { identifier: 'Alice@Example.COM', value: 'TOKEN', expiresAt: new Date() }, + }); + + const [object, data] = (engine.insert as any).mock.calls[0]; + expect(object).toBe('sys_verification'); + expect(data.identifier).toBe('Alice@Example.COM'); + }); +}); + +// --------------------------------------------------------------------------- +// Face 5 — the declared set, pinned by value +// --------------------------------------------------------------------------- + +describe('[#5814] the normalised identifier set cannot widen quietly', () => { + it('is exactly `user.email` today', () => { + // Widening this is a contract change with a precondition, not a config + // tweak: a field may join only once EVERY producer of it writes it + // normalised. Adding a member without coming here is the failure this pin + // exists to make impossible — the compare half would start lower-casing a + // comparand against rows nobody lower-cased. + expect(NORMALISED_IDENTIFIER_FIELDS).toEqual({ user: ['email'] }); + }); + + it('is the field `@better-auth/scim` actually queries', () => { + // SCIM's `userName` is mapped onto better-auth's `email` field, not onto a + // `userName` column — `SCIMUserFilterAttributeFields = { userName: "email" }`. + // A set that named `userName` would be spelled plausibly and match nothing. + expect(NORMALISED_IDENTIFIER_FIELDS.user).toContain('email'); + expect(NORMALISED_IDENTIFIER_FIELDS.user).not.toContain('userName'); + }); +});