From 0d3dc51b65706c085d7dec20d1c72b6d04892984 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 10:18:23 +0000 Subject: [PATCH 1/2] fix(auth): generalize credential field read-deny to better-auth plugin tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the ADR-0036 field-level read-deny from the four base Auth models to plugin tables derived through the same registry (ADR-0034): the mcp/oauth-provider plugin's oauthClient.clientSecret/oauthAccessToken.token/ oauthRefreshToken.token, and twoFactor()'s twoFactor.secret/backupCodes. Adds authPlugin({ credentialFields }) so an app can mark credential fields on a plugin the stack doesn't seed a set for — additive only, keyed by better-auth's own model/field keys, and validated (throws on a field name that doesn't exist on a derived model; no-op for an undeployed plugin). Closes #1014 --- .changeset/plugin-table-credential-fields.md | 23 ++++ ...e-same-registry-as-the-base-auth-models.md | 2 + ...ived-credential-fields-ship-read-denied.md | 3 +- docs/content/reference/auth.md | 42 ++++++ packages/auth/CLAUDE.md | 62 ++++++--- packages/auth/src/config/derive-auth-lists.ts | 78 ++++++++++- packages/auth/src/config/index.ts | 1 + packages/auth/src/config/plugin.ts | 1 + packages/auth/src/config/types.ts | 30 +++++ packages/auth/src/lists/index.ts | 11 +- packages/auth/tests/derive-auth-lists.test.ts | 127 ++++++++++++++++++ .../auth/tests/plugin-derived-keys.test.ts | 39 ++++++ 12 files changed, 399 insertions(+), 20 deletions(-) create mode 100644 .changeset/plugin-table-credential-fields.md diff --git a/.changeset/plugin-table-credential-fields.md b/.changeset/plugin-table-credential-fields.md new file mode 100644 index 00000000..1ccb6d6d --- /dev/null +++ b/.changeset/plugin-table-credential-fields.md @@ -0,0 +1,23 @@ +--- +'@opensaas/stack-auth': minor +--- + +Extend the ADR-0036 credential field read-deny from the four base Auth models to better-auth plugin tables, and add a `credentialFields` config option for plugins the stack doesn't seed a set for. + +The following fields now ship field-level `read`-denied, on top of the existing `Session.token`/`Verification.value`/`Account.password`/`accessToken`/`refreshToken`/`idToken`: + +- `oauthClient.clientSecret`, `oauthAccessToken.token`, `oauthRefreshToken.token` (the `mcp`/oauth-provider plugin) +- `twoFactor.secret`, `twoFactor.backupCodes` (`twoFactor()`) + +An application opening one of these lists (e.g. declaring `OauthClient` under its own `lists` to grant access) no longer also exposes the credential column — same behavior as the existing base-model deny, `sudo()` still reads it. + +For a plugin the stack has no seeded credential set for, mark a field yourself: + +```typescript +authPlugin({ + betterAuthPlugins: [passkey()], + credentialFields: { passkey: ['publicKey'] }, +}) +``` + +`credentialFields` is additive only — it can add fields to any model (including a seeded one) but can never unmark a seeded field. An entry naming a field missing from a model your app actually derives throws at config time; an entry for a model your app doesn't derive is a no-op. diff --git a/docs/adr/0034-plugin-tables-derive-through-the-same-registry-as-the-base-auth-models.md b/docs/adr/0034-plugin-tables-derive-through-the-same-registry-as-the-base-auth-models.md index b20ed46f..4dcd5cbb 100644 --- a/docs/adr/0034-plugin-tables-derive-through-the-same-registry-as-the-base-auth-models.md +++ b/docs/adr/0034-plugin-tables-derive-through-the-same-registry-as-the-base-auth-models.md @@ -27,3 +27,5 @@ Concretely: - This absorbs #994 (which had specified the same FK fix against list-only refs, before ADR-0033 shipped the base models' bidirectional-ref convention) — the FK column name is controllable on a bidirectional ref already, so #994's premise (list-only refs as the only viable shape) no longer applies, and its fix is superseded by this consolidation rather than landed separately. See issue #992 for the full triage history, including the confirmed orphaning and the correction that bidirectional refs were viable all along. + +Because plugin tables derive through this same registry and scalar-field-derivation pass, they are also in scope for ADR-0036's credential field read-deny — see that ADR (as amended by issue #1014) for the plugin-table credential set and the `credentialFields` seam. diff --git a/docs/adr/0036-plugin-derived-credential-fields-ship-read-denied.md b/docs/adr/0036-plugin-derived-credential-fields-ship-read-denied.md index bf81db4e..1ee4699c 100644 --- a/docs/adr/0036-plugin-derived-credential-fields-ship-read-denied.md +++ b/docs/adr/0036-plugin-derived-credential-fields-ship-read-denied.md @@ -2,7 +2,7 @@ Status: accepted -A field on a plugin-derived list that holds a **live, presentable credential** — one where reading the value is equivalent to holding it — ships with a field-level `read` deny that the plugin sets when it creates the list. Granting operation-level access to such a list does not grant access to those fields. For the auth plugin this covers `Session.token`, `Verification.value`, `Account.password`, and the OAuth token columns (`accessToken`, `refreshToken`, `idToken`). +A field on a plugin-derived list that holds a **live, presentable credential** — one where reading the value is equivalent to holding it — ships with a field-level `read` deny that the plugin sets when it creates the list. Granting operation-level access to such a list does not grant access to those fields. For the auth plugin this covers `Session.token`, `Verification.value`, `Account.password`, and the OAuth token columns (`accessToken`, `refreshToken`, `idToken`) on the four **base** models, and — as of issue #1014 — the same rule over **better-auth plugin tables** derived through the same registry (ADR-0034): `oauthClient.clientSecret`, `oauthAccessToken.token`, `oauthRefreshToken.token` (the `mcp`/oauth-provider plugin) and `twoFactor.secret`/`twoFactor.backupCodes` (`twoFactor()`). An application can mark further plugin fields as credentials via `authPlugin({ credentialFields })`, additive only over the stack's seeded set. ## Context @@ -29,3 +29,4 @@ Two properties make the deny cheap. `sudo()` skips field-level access checks, so - **A denied field is stripped, not an error.** It follows the field-level access path's existing behaviour; a list read still succeeds and returns every other field. The application sees a missing column, not a failure. - **`sudo()` still reads these fields.** That is the supported path for an application with a legitimate need until a narrower seam exists, and it is what the auth package's own helpers already use. - **The rule is about the credential, not the list.** A derived field that merely identifies (`Session.ipAddress`, `Account.providerId`) stays open. The test is whether reading the value confers the ability to act as someone — that is what earns a deny. +- **The rule applies uniformly to plugin tables, keyed by better-auth's own model/field keys — not by whatever store mode the value happens to sit in.** `oauth-provider`'s token/secret columns default to hashed storage (encrypted when `disableJwtPlugin`) and `twoFactor`'s `secret`/`backupCodes` are stored encrypted; the deny does not condition on `storeTokens`/`storeClientSecret` or any other at-rest mode, and it is not derived from better-auth's own `returned: false` flag — that flag is both noisy (it also marks plain identifiers, e.g. `twoFactor.userId`) and incomplete (`oauth-provider` never sets it). The stack seeds the set for the plugins it has first-class support for; `authPlugin({ credentialFields })` lets an application extend it to a plugin the stack has never seen, additive-only over the seeded set (issue #1014). diff --git a/docs/content/reference/auth.md b/docs/content/reference/auth.md index 4b44c3bd..a89f057d 100644 --- a/docs/content/reference/auth.md +++ b/docs/content/reference/auth.md @@ -310,6 +310,19 @@ authPlugin({ The auth plugin automatically converts Better Auth plugin schemas to OpenSaaS lists. +### `credentialFields` + +Mark additional better-auth model fields as credentials, so they ship field-level read-denied +alongside the stack's own seeded set. See [Credential fields are read-denied](#credential-fields-are-read-denied-adr-0036) +below for the full contract and the seeded set. + +```typescript +authPlugin({ + betterAuthPlugins: [passkey()], + credentialFields: { passkey: ['publicKey'] }, +}) +``` + ### `betterAuthOptions` Escape hatch for any better-auth option the stack doesn't model as its own config field. Deep-merged into the options `createAuth()` builds, applied **last** — a plain-object value at a given key merges recursively with what the stack already set there (so a nested addition like `session.cookieCache` adds alongside the stack's own `session.expiresIn`/`updateAge` rather than replacing them), and on a genuine key collision `betterAuthOptions` wins. Arrays and any other value type replace the stack's value outright. @@ -439,6 +452,35 @@ derives the list, so granting operation-level access to a list (e.g. `access: { column — the field is silently stripped from a returned row, the same as any other field-level read denial, and the rest of the row is returned normally. +The same deny covers **better-auth plugin table** credential fields for the plugins the stack has +first-class support for — plugin tables derive through the identical field-derivation pass as the +base models (ADR-0034): + +| Model (better-auth key) | Field(s) | Plugin | +| ----------------------- | ----------------------- | ---------------------- | +| `oauthClient` | `clientSecret` | `mcp` / oauth-provider | +| `oauthAccessToken` | `token` | `mcp` / oauth-provider | +| `oauthRefreshToken` | `token` | `mcp` / oauth-provider | +| `twoFactor` | `secret`, `backupCodes` | `twoFactor()` | + +For any other plugin, mark a field as a credential yourself via `credentialFields` — keyed by +better-auth's own **model key** (not the derived list key) and naming better-auth's own **field +keys** (not mapped column names). It is strictly additive: it can mark further fields, but can never +unmark one of the fields above. + +```typescript +authPlugin({ + betterAuthPlugins: [passkey()], + credentialFields: { + passkey: ['publicKey'], + }, +}) +``` + +An entry naming a field that doesn't exist on a model your app actually derives (the plugin is +registered) throws at config time, naming the model and field; an entry for a model your app doesn't +derive at all (the plugin isn't registered) is a silent no-op. + Naming a denied field in `findMany`'s (or `count`'s) `where`/`orderBy` is different: that's rejected up front with a `ValidationError` rather than silently stripped, the same as any other field-level `read` deny. A `findUnique` lookup is not — its `where` only unique-selects the row, so diff --git a/packages/auth/CLAUDE.md b/packages/auth/CLAUDE.md index 223255ff..c4dce4c9 100644 --- a/packages/auth/CLAUDE.md +++ b/packages/auth/CLAUDE.md @@ -195,13 +195,11 @@ then stands, same as any other `extendList`). ### Credential fields are read-denied independent of operation access (ADR-0036) Operation-level access is all-or-nothing at the list, not the column — so -the deny above (list-level) isn't the whole story for the six -credential-bearing fields (`Session.token`, `Verification.value`, -`Account.password`/`accessToken`/`refreshToken`/`idToken`): `deriveAuthLists` -sets a field-level `access: { read: () => false }` on each of them -unconditionally, independent of whatever `accessConfig` an app supplies. -Opening `query` on `Session` for a "your active sessions" screen no longer -also exposes `token` — the field is stripped from a returned row (the +the deny above (list-level) isn't the whole story for a credential-bearing +field: `deriveAuthLists` sets a field-level `access: { read: () => false }` +on each one unconditionally, independent of whatever `accessConfig` an app +supplies. Opening `query` on `Session` for a "your active sessions" screen no +longer also exposes `token` — the field is stripped from a returned row (the ordinary field-access-denial behavior), the row itself still returns. This holds even for a `findUnique` lookup that selects the row BY the denied field (`context.db.session.findUnique({ where: { token } })` still finds @@ -214,17 +212,51 @@ predicate-time path (`validateQueryFieldReadAccess` in required for that shape too, not only for reading the column back off a row fetched another way. +This is not a closed list of six base-model fields — it also covers **plugin +table** credential fields the stack has first-class support for (ADR-0034), +since a plugin table derives through the same scalar-field derivation pass: + +| Model (better-auth key) | Field(s) | Source | +| ----------------------- | ---------------------------------------------------- | ---------------------- | +| `session` | `token` | base | +| `verification` | `value` | base | +| `account` | `password`, `accessToken`, `refreshToken`, `idToken` | base | +| `oauthClient` | `clientSecret` | `mcp` / oauth-provider | +| `oauthAccessToken` | `token` | `mcp` / oauth-provider | +| `oauthRefreshToken` | `token` | `mcp` / oauth-provider | +| `twoFactor` | `secret`, `backupCodes` | `twoFactor()` | + The deny is applied in the scalar-field derivation loop -(`withCredentialAccess` in `derive-auth-lists.ts`), keyed by better-auth's -own model/field key (`CREDENTIAL_FIELDS`) — not the app's list key or -column `db.map` — so it survives a `modelName` remap or a `fields` column -override. `sudo()` still reads these fields either way, unaffected: this is -the supported path for a genuine need (an admin tool, or an app's own auth +(`withCredentialAccess` in `derive-auth-lists.ts`), against a registry built +by `buildCredentialFieldRegistry` — the stack-seeded `CREDENTIAL_FIELDS` table +above merged with an app's `authPlugin({ credentialFields })` — keyed by +better-auth's own model/field key, not the app's list key or column `db.map`, +so it survives a `modelName` remap or a `fields` column override either way. +`credentialFields` (`Record`) is how an +app marks a credential field on a better-auth plugin the stack doesn't seed a +set for; it is **additive only** — it can add fields to any model, including a +seeded one, but can never unmark a seeded field. An entry naming a field +absent from a model the app actually derives (that plugin is registered) +throws, naming the model and field; an entry for a model the app doesn't +derive at all is a silent no-op: + +```typescript +authPlugin({ + betterAuthPlugins: [passkey()], + credentialFields: { passkey: ['publicKey'] }, +}) +``` + +`sudo()` still reads every one of these fields either way, unaffected: this +is the supported path for a genuine need (an admin tool, or an app's own auth code verifying a password hash via `HashedPassword.compare()`). See `packages/core/CLAUDE.md`'s "Access Control Execution Flow" for how a -field-level `read` denial is enforced, and ADR-0036 for why this list is -exactly these six fields and not, say, `Account.providerId` or -`Session.ipAddress` (identifying, not authenticating — left open). +field-level `read` denial is enforced, and ADR-0036 for why the rule is about +whether reading the value confers a live credential, not, say, +`Account.providerId` or `Session.ipAddress` (identifying, not authenticating +— left open) — and it does not condition on better-auth's own storage mode +(`storeTokens`/`storeClientSecret`) or `returned: false` flag, which is +neither a reliable nor a complete signal (see ADR-0036). ### Schema placement (relocatable Auth lists) diff --git a/packages/auth/src/config/derive-auth-lists.ts b/packages/auth/src/config/derive-auth-lists.ts index f2e7a89a..fed1f976 100644 --- a/packages/auth/src/config/derive-auth-lists.ts +++ b/packages/auth/src/config/derive-auth-lists.ts @@ -107,17 +107,82 @@ const TIMESTAMP_FIELDS = new Set(['createdAt', 'updatedAt']) * OAuth token) — rather than merely identifying a row. Keyed by better-auth's * own model/field keys, not the app's list/column names, so the deny holds * under `modelName` and column `fields` remapping alike (ADR-0036). + * + * Covers the four base models plus every better-auth plugin table the stack + * has first-class support for (ADR-0034's registry of known plugins) — the + * `mcp`/oauth-provider plugin's client secret and token columns, and + * `twoFactor()`'s encrypted secret/backup codes (issue #1014). An app can + * mark further fields via `authPlugin({ credentialFields })`, merged in by + * {@link buildCredentialFieldRegistry} — this constant is never mutated. */ -const CREDENTIAL_FIELDS: Partial> = { +const CREDENTIAL_FIELDS: Record = { session: ['token'], verification: ['value'], account: ['password', 'accessToken', 'refreshToken', 'idToken'], + oauthClient: ['clientSecret'], + oauthAccessToken: ['token'], + oauthRefreshToken: ['token'], + twoFactor: ['secret', 'backupCodes'], } const DENY_READ: FieldAccess = { read: () => false } -function withCredentialAccess(modelKey: string, fieldKey: string, field: FieldConfig): FieldConfig { - if (!CREDENTIAL_FIELDS[modelKey as BaseModelKey]?.includes(fieldKey)) return field +/** Every better-auth model/field key marked as a credential — the stack's own {@link CREDENTIAL_FIELDS} plus an app's `credentialFields`. */ +type CredentialFieldRegistry = Record> + +/** + * Merges the stack-seeded {@link CREDENTIAL_FIELDS} with an app's + * `authPlugin({ credentialFields })`, then validates every entry against the + * models actually being derived (`tables`, keyed by better-auth model key). + * + * Additive only: an app entry can add fields to a model — including a + * stack-seeded one — but nothing can remove a seeded field, so a config that + * omits or empties a seeded model's list leaves that model's seeded deny + * standing. + * + * A field named on a model that isn't in `tables` at all (a plugin the app + * doesn't use) is a silent no-op — the entry simply never matches anything. + * A field named on a model that IS in `tables` but doesn't declare that field + * throws, naming the model and field, since that is a config mistake the app + * would otherwise never learn about (the deny would just never fire). + */ +function buildCredentialFieldRegistry( + tables: Record, + appConfig: Record, +): CredentialFieldRegistry { + const merged = new Map>() + for (const [modelKey, fields] of Object.entries(CREDENTIAL_FIELDS)) { + merged.set(modelKey, new Set(fields)) + } + for (const [modelKey, fields] of Object.entries(appConfig)) { + const set = merged.get(modelKey) ?? new Set() + for (const fieldKey of fields) set.add(fieldKey) + merged.set(modelKey, set) + } + + const registry: CredentialFieldRegistry = {} + for (const [modelKey, fields] of merged) { + const table = tables[modelKey] + if (!table) continue // model not derived (plugin unused) -> silent no-op + for (const fieldKey of fields) { + if (!(fieldKey in table.fields)) { + throw new Error( + `deriveAuthLists: credentialFields names "${modelKey}.${fieldKey}", but "${modelKey}" has no field "${fieldKey}"`, + ) + } + } + registry[modelKey] = fields + } + return registry +} + +function withCredentialAccess( + registry: CredentialFieldRegistry, + modelKey: string, + fieldKey: string, + field: FieldConfig, +): FieldConfig { + if (!registry[modelKey]?.has(fieldKey)) return field return { ...field, access: DENY_READ } } @@ -493,6 +558,9 @@ function buildModelRegistry( * @param accessConfig - App-authored access for each base Auth list, keyed by better-auth model name * @param plugins - The app's better-auth plugins (`authPlugin({ betterAuthPlugins })`), whose own * `schema` (base-model extensions and standalone plugin tables) is derived alongside the base models + * @param credentialFieldsConfig - App-authored additions to the credential-field read-deny + * (`authPlugin({ credentialFields })`), keyed by better-auth model key. Additive only — see + * {@link buildCredentialFieldRegistry}. * @returns The derived base-model list keys and every derived list config (base models and plugin tables) */ export function deriveAuthLists( @@ -500,12 +568,14 @@ export function deriveAuthLists( userConfig: ExtendUserListConfig = {}, accessConfig: AuthAccessConfig = {}, plugins: BetterAuthPlugin[] = [], + credentialFieldsConfig: Record = {}, ): DerivedAuthLists { const tables = getAuthTables(buildBetterAuthTableOptions(models, plugins)) as Record< string, ResolvedTable > const { keys, registry } = buildModelRegistry(tables, models) + const credentialRegistry = buildCredentialFieldRegistry(tables, credentialFieldsConfig) // Only the five base models carry an app-authored `db.indexes` passthrough // (`AuthModelConfig.indexes`) — plugin tables have no per-model config @@ -577,6 +647,7 @@ export function deriveAuthLists( // column. Left as a plain scalar column, same as pre-consolidation // behavior (issue #992). ;(scalarFields[modelKey] ??= {})[fieldKey] = withCredentialAccess( + credentialRegistry, modelKey, fieldKey, buildScalarField( @@ -588,6 +659,7 @@ export function deriveAuthLists( } } else { ;(scalarFields[modelKey] ??= {})[fieldKey] = withCredentialAccess( + credentialRegistry, modelKey, fieldKey, buildScalarField( diff --git a/packages/auth/src/config/index.ts b/packages/auth/src/config/index.ts index b1231547..4214df8e 100644 --- a/packages/auth/src/config/index.ts +++ b/packages/auth/src/config/index.ts @@ -138,6 +138,7 @@ export function normalizeAuthConfig(config: AuthConfig): NormalizedAuthConfig { sessionFields, extendUserList: config.extendUserList || {}, access: config.access || {}, + credentialFields: config.credentialFields || {}, betterAuthPlugins: config.betterAuthPlugins || [], rateLimit: config.rateLimit, betterAuthOptions: config.betterAuthOptions || {}, diff --git a/packages/auth/src/config/plugin.ts b/packages/auth/src/config/plugin.ts index 7019a08f..caac0cb9 100644 --- a/packages/auth/src/config/plugin.ts +++ b/packages/auth/src/config/plugin.ts @@ -51,6 +51,7 @@ export function authPlugin(config: AuthConfig): Plugin { normalized.models, normalized.access, normalized.betterAuthPlugins, + normalized.credentialFields, ) // Base models are always the first entries in `authLists` (see diff --git a/packages/auth/src/config/types.ts b/packages/auth/src/config/types.ts index b153693a..032c3bbb 100644 --- a/packages/auth/src/config/types.ts +++ b/packages/auth/src/config/types.ts @@ -393,6 +393,36 @@ export type AuthConfig = { */ access?: AuthAccessConfig + /** + * Mark additional better-auth model fields as credentials, so they ship + * field-level read-denied like the stack's own seeded set (ADR-0036) — the + * six base-model fields plus, as of issue #1014, the `mcp`/oauth-provider + * plugin's `oauthClient.clientSecret`/`oauthAccessToken.token`/ + * `oauthRefreshToken.token` and `twoFactor()`'s `twoFactor.secret`/ + * `backupCodes`. Use this for a better-auth plugin the stack doesn't seed a + * credential set for. + * + * Keyed by better-auth's own **model key** (e.g. `'twoFactor'`, + * `'oauthClient'` — not the derived list key, so it stays remap-proof), each + * naming better-auth **field keys** (not mapped column names). + * + * Strictly additive: an entry can mark further fields as credentials, but + * can never unmark one of the stack's seeded fields — omitting or emptying + * a seeded model's list here leaves that model's seeded deny standing. An + * entry naming a field that doesn't exist on a model the app actually + * derives (has that plugin registered) throws, naming the model and field; + * an entry for a model the app doesn't derive at all is a silent no-op. + * + * @example + * ```typescript + * authPlugin({ + * betterAuthPlugins: [passkey()], + * credentialFields: { passkey: ['publicKey'] }, + * }) + * ``` + */ + credentialFields?: Record + /** * Additional Better Auth plugins to enable * Allows integrating any Better Auth plugin (MCP, 2FA, etc.) diff --git a/packages/auth/src/lists/index.ts b/packages/auth/src/lists/index.ts index 7207f4a7..638302cd 100644 --- a/packages/auth/src/lists/index.ts +++ b/packages/auth/src/lists/index.ts @@ -90,13 +90,22 @@ export function createVerificationList(): ListConfig { * @param models - Resolved better-auth model config; defaults to the better-auth defaults * @param accessConfig - App-authored access for each base Auth list, keyed by better-auth model name * @param plugins - The app's better-auth plugins (`authPlugin({ betterAuthPlugins })`) + * @param credentialFieldsConfig - App-authored additions to the credential-field read-deny + * (`authPlugin({ credentialFields })`), keyed by better-auth model key */ export function getAuthLists( userConfig?: ExtendUserListConfig, models: NormalizedAuthModels = DEFAULT_MODELS, accessConfig?: AuthAccessConfig, plugins?: BetterAuthPlugin[], + credentialFieldsConfig?: Record, // eslint-disable-next-line @typescript-eslint/no-explicit-any -- ListConfig must accept any TypeInfo ): Record> { - return deriveAuthLists(models, userConfig || {}, accessConfig || {}, plugins || []).lists + return deriveAuthLists( + models, + userConfig || {}, + accessConfig || {}, + plugins || [], + credentialFieldsConfig || {}, + ).lists } diff --git a/packages/auth/tests/derive-auth-lists.test.ts b/packages/auth/tests/derive-auth-lists.test.ts index 83db1f02..a3900157 100644 --- a/packages/auth/tests/derive-auth-lists.test.ts +++ b/packages/auth/tests/derive-auth-lists.test.ts @@ -1,4 +1,6 @@ import { describe, it, expect } from 'vitest' +import { mcp } from '@better-auth/mcp' +import { twoFactor } from 'better-auth/plugins' import { deriveAuthLists } from '../src/config/derive-auth-lists.js' import type { NormalizedAuthModels } from '../src/config/types.js' @@ -534,6 +536,131 @@ describe('deriveAuthLists - credential fields ship read-denied (ADR-0036, issue }) }) +describe('deriveAuthLists - credential fields on plugin tables (issue #1014)', () => { + const mcpPlugin = mcp({ + loginPage: '/sign-in', + consentPage: '/consent', + resource: 'https://example.com/mcp', + }) + + it('denies read on the mcp/oauth-provider credential fields', async () => { + const { lists } = deriveAuthLists(defaultModels, {}, {}, [mcpPlugin]) + + const denied: Array<[string, string]> = [ + ['OauthClient', 'clientSecret'], + ['OauthAccessToken', 'token'], + ['OauthRefreshToken', 'token'], + ] + + for (const [listKey, fieldKey] of denied) { + const field = lists[listKey].fields[fieldKey] + expect(field.access?.read).toBeTypeOf('function') + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- minimal read-access call fixture + expect(await field.access!.read!({} as any)).toBe(false) + } + }) + + it('leaves identifying oauth-provider fields open', () => { + const { lists } = deriveAuthLists(defaultModels, {}, {}, [mcpPlugin]) + + expect(lists.OauthClient.fields.name.access).toBeUndefined() + expect(lists.OauthClient.fields.uri.access).toBeUndefined() + expect(lists.OauthClient.fields.clientId.access).toBeUndefined() + expect(lists.OauthAccessToken.fields.scopes.access).toBeUndefined() + expect(lists.OauthAccessToken.fields.expiresAt.access).toBeUndefined() + }) + + it('denies read on twoFactor.secret and twoFactor.backupCodes', async () => { + const { lists } = deriveAuthLists(defaultModels, {}, {}, [twoFactor()]) + + for (const fieldKey of ['secret', 'backupCodes']) { + const field = lists.TwoFactor.fields[fieldKey] + expect(field.access?.read).toBeTypeOf('function') + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- minimal read-access call fixture + expect(await field.access!.read!({} as any)).toBe(false) + } + }) + + it('leaves twoFactor.verified open, and twoFactor.userId open despite carrying returned: false upstream', () => { + const { lists } = deriveAuthLists(defaultModels, {}, {}, [twoFactor()]) + + expect(lists.TwoFactor.fields.verified.access).toBeUndefined() + // userId is a relationship field (references user.id), not a scalar — + // it's never routed through the credential-deny path at all. + expect(lists.TwoFactor.fields.user.access).toBeUndefined() + }) + + it('survives a plugin-table modelName + column remap', () => { + const plugin = { + id: 'test-remap', + schema: { + widget: { + modelName: 'Gadget', + fields: { + apiKey: { type: 'string' as const, required: true, fieldName: 'api_key' }, + }, + }, + }, + } + + const { lists } = deriveAuthLists(defaultModels, {}, {}, [plugin], { + widget: ['apiKey'], + }) + + expect(lists.Gadget.fields.apiKey.access?.read).toBeTypeOf('function') + expect(lists.Gadget.fields.apiKey.db?.map).toBe('api_key') + }) + + it('authPlugin({ credentialFields }) marks an additional field on a synthetic plugin table', async () => { + const plugin = { + id: 'test-passkey', + schema: { + passkey: { + fields: { + publicKey: { type: 'string' as const, required: true }, + deviceType: { type: 'string' as const, required: true }, + }, + }, + }, + } + + const { lists } = deriveAuthLists(defaultModels, {}, {}, [plugin], { + passkey: ['publicKey'], + }) + + expect(await lists.Passkey.fields.publicKey.access!.read!({} as never)).toBe(false) + expect(lists.Passkey.fields.deviceType.access).toBeUndefined() + }) + + it('cannot unmark a stack-seeded credential field', async () => { + // An empty (or omitted) field list for a seeded model must not remove its + // seeded deny — credentialFields is additive-only. + const { lists } = deriveAuthLists(defaultModels, {}, {}, [], { session: [] }) + + expect(await lists.Session.fields.token.access!.read!({} as never)).toBe(false) + }) + + it('throws, naming the model and field, when credentialFields names a field missing from a derived model', () => { + const plugin = { + id: 'test-typo', + schema: { + widget: { fields: { apiKey: { type: 'string' as const } } }, + }, + } + + expect(() => deriveAuthLists(defaultModels, {}, {}, [plugin], { widget: ['apiKye'] })).toThrow( + /widget\.apiKye.*no field "apiKye"/, + ) + }) + + it('is a silent no-op when credentialFields names a model that is not derived at all', () => { + // No plugin registers `passkey` here, so `tables` never contains it. + expect(() => + deriveAuthLists(defaultModels, {}, {}, [], { passkey: ['publicKey'] }), + ).not.toThrow() + }) +}) + describe('deriveAuthLists - extendUserList', () => { it('adds custom fields to the derived user list', () => { const { lists } = deriveAuthLists( diff --git a/packages/auth/tests/plugin-derived-keys.test.ts b/packages/auth/tests/plugin-derived-keys.test.ts index c06454a5..fe9030f0 100644 --- a/packages/auth/tests/plugin-derived-keys.test.ts +++ b/packages/auth/tests/plugin-derived-keys.test.ts @@ -209,6 +209,45 @@ describe('authPlugin - add-vs-extend with derived keys', () => { // ...or its access config. expect(authUser.access?.operation?.query).toBe(userQuery) }) + + it('keeps a plugin-table credential field read-denied even when the app redeclares that list and field (issue #1014)', async () => { + // An app opening a plugin-derived list the documented way (declaring the + // list itself under the derived key) merges in via extendList — field + // additions win over the app's own field of the same key (core's + // extendList spreads `extension.fields` last), so the derived, + // read-denied clientSecret must survive even though the app also + // declares a `clientSecret` field on its own `OauthClient` list. + const mcpBetterAuthPlugin = { + id: 'test-mcp', + schema: { + oauthClient: { + fields: { + clientSecret: { type: 'string' as const, required: false }, + name: { type: 'string' as const, required: false }, + }, + }, + }, + } + + const result = await config({ + db: { provider: 'sqlite' }, + plugins: [authPlugin({ betterAuthPlugins: [mcpBetterAuthPlugin] })], + lists: { + OauthClient: list({ + fields: { clientSecret: text() }, + access: { operation: { query: () => true } }, + }), + }, + }) + + const oauthClient = result.lists.OauthClient + expect(oauthClient.fields.clientSecret.access?.read).toBeTypeOf('function') + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- minimal read-access call fixture + expect(await oauthClient.fields.clientSecret.access!.read!({} as any)).toBe(false) + // The app's own access survives (extendList never forwards access). + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- access control parameters are runtime values + expect(oauthClient.access?.operation?.query?.({} as any)).toBe(true) + }) }) describe('authPlugin - runtime user-key resolution', () => { From e7098ab4a88258bd547bd0333c2045ea122882b0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 10:23:47 +0000 Subject: [PATCH 2/2] fix(auth): reject credentialFields entries naming a relationship field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An id-referencing field (references.field === 'id') derives to a relationship(), never a scalar column, so withCredentialAccess — only ever applied on the scalar-field path — could never actually deny it. Naming one in credentialFields previously validated (the field exists) and then silently had no effect. Throw instead, naming the model and field. Found in code review of #1019. --- packages/auth/src/config/derive-auth-lists.ts | 14 ++++++++++++- packages/auth/src/config/types.ts | 3 +++ packages/auth/tests/derive-auth-lists.test.ts | 21 +++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/packages/auth/src/config/derive-auth-lists.ts b/packages/auth/src/config/derive-auth-lists.ts index fed1f976..9b53676d 100644 --- a/packages/auth/src/config/derive-auth-lists.ts +++ b/packages/auth/src/config/derive-auth-lists.ts @@ -165,11 +165,23 @@ function buildCredentialFieldRegistry( const table = tables[modelKey] if (!table) continue // model not derived (plugin unused) -> silent no-op for (const fieldKey of fields) { - if (!(fieldKey in table.fields)) { + const upstream = table.fields[fieldKey] + if (!upstream) { throw new Error( `deriveAuthLists: credentialFields names "${modelKey}.${fieldKey}", but "${modelKey}" has no field "${fieldKey}"`, ) } + // An id-referencing field derives to a relationship() (see the + // `references.field === 'id'` branch below), never a scalar field — + // withCredentialAccess is only ever applied on the scalar-field path, + // so a deny registered against one would silently never apply. Fail + // loudly instead of accepting a config that has no effect. + if (upstream.references?.field === 'id') { + throw new Error( + `deriveAuthLists: credentialFields names "${modelKey}.${fieldKey}", but "${fieldKey}" is a ` + + `relationship field (references "${upstream.references.model}.id"), not a scalar credential column`, + ) + } } registry[modelKey] = fields } diff --git a/packages/auth/src/config/types.ts b/packages/auth/src/config/types.ts index 032c3bbb..4e163976 100644 --- a/packages/auth/src/config/types.ts +++ b/packages/auth/src/config/types.ts @@ -412,6 +412,9 @@ export type AuthConfig = { * entry naming a field that doesn't exist on a model the app actually * derives (has that plugin registered) throws, naming the model and field; * an entry for a model the app doesn't derive at all is a silent no-op. + * An entry naming an id-referencing relationship field (e.g. an FK ending + * `Id`) also throws — that field derives to a `relationship()`, never a + * scalar column, so a deny registered against it could never apply. * * @example * ```typescript diff --git a/packages/auth/tests/derive-auth-lists.test.ts b/packages/auth/tests/derive-auth-lists.test.ts index a3900157..5ac88aff 100644 --- a/packages/auth/tests/derive-auth-lists.test.ts +++ b/packages/auth/tests/derive-auth-lists.test.ts @@ -659,6 +659,27 @@ describe('deriveAuthLists - credential fields on plugin tables (issue #1014)', ( deriveAuthLists(defaultModels, {}, {}, [], { passkey: ['publicKey'] }), ).not.toThrow() }) + + it('throws, naming the model and field, when credentialFields names an id-referencing relationship field', () => { + // An id-referencing field (references.field === 'id') derives to a + // relationship(), never a scalar — withCredentialAccess only ever runs on + // the scalar-field path, so a deny registered against one would silently + // never apply. Reject the config instead of accepting a no-op. + const plugin = { + id: 'test-fk-credential', + schema: { + widget: { + fields: { + ownerId: { type: 'string' as const, references: { model: 'user', field: 'id' } }, + }, + }, + }, + } + + expect(() => deriveAuthLists(defaultModels, {}, {}, [plugin], { widget: ['ownerId'] })).toThrow( + /widget\.ownerId.*relationship field.*"user\.id"/, + ) + }) }) describe('deriveAuthLists - extendUserList', () => {