diff --git a/.changeset/settings-declared-value-window-enforced.md b/.changeset/settings-declared-value-window-enforced.md new file mode 100644 index 0000000000..67ba029f3b --- /dev/null +++ b/.changeset/settings-declared-value-window-enforced.md @@ -0,0 +1,43 @@ +--- +"@objectstack/service-settings": patch +--- + +fix(service-settings): 写入路径与 env 路径执行 settings 声明的 min / max / minLength / maxLength (#5932) + +`SpecifierSchema` 从存在起就声明了五类值约束 —— `pattern` / `min` / `max` / +`minLength` / `maxLength` —— 而 `SettingsService.validatePatch` 只读其中一类。 +另外四个在整个写入路径上**没有任何读取点**:已发布的 manifest 里 42 个 +specifier 声明了取值窗口,每一个都只是装饰。 + +落点最重的是 `auth.password_min_length`。它声明 `min: 6`,控制台的数字框也按这个 +下限渲染,而 `PUT /api/settings/auth` 会接受 `1`(以及负数)并存下来,better-auth +的口令策略随后照这个值执行。也就是说,声明是唯一一个宣称「存在下限」的东西,却没有 +任何一层在守它 —— 正是 Prime Directive #10 的正面形状。`ai.manifest.ts` 的六项 +(temperature / max_tokens / timeout 等)同理。 + +**修法与 #5131(options 表)同形,是同一族的第三个成员:** + +- `validatePatch` 补一个取值窗口分支,发既有码表里的 `FieldError`(ADR-0114 D2): + `min_value` / `max_value` / `min_length` / `max_length` —— 与 + `record-validator.ts` 对同一类越界发出的码一致。`constraint` 带**完整窗口** + (`{ min, max }`,长度类再带 `actual`),客户端据此自行组织文案,不必解析我方 + 英文句子。⛔ `packages/spec` 未改动:约束早已声明,码表现有即够用。 +- 沿用 #5131 的 **TOUCH 闸门**:只校验本次 patch 触及的键。取值窗口在产品生命周期里 + 会被**收紧**(口令下限从 6 提到 8),窗口下方的老工作区必须仍能编辑它无关的设置, + 只在重写该键时才被告知。 +- env 侧走 `effectiveEnvOverride` 这**一个**判定点,与 options 表同处,复用同一组 + 比较函数 —— #5204 的成因就是同一比较有两份实现并各自漂移。因此 + `OS_AUTH_PASSWORD_MIN_LENGTH=1` 与写入路径得到同一个裁决:该 override 不生效、 + 不贡献 cascade 条目、不锁定该键,并在注册时打出一条(且仅一条)`error` 日志。 + +**刻意不做的判断:** 取值窗口只裁决**可比较的值** —— `min`/`max` 只看数字(含经 +JSON / 表单往返变成字符串的数字),`minLength`/`maxLength` 只看字符串。布尔、数组、 +对象不做强制转换(`Number(true)` 是 1、`Number([])` 是 0):值的**形状**是 +`invalid_type`,属于另一个约束、另一个负责人,在这里发明裁决会拒掉本检查从未被要求 +过问的写入。空值仍归 `required` 管。 + +约束的读取以**声明**为准,而不是以 specifier 的 `type` 为准 —— 与旁边按类型收口的 +options 检查不同,这个差异是 spec 定的:`SpecifierSchema` 的 superRefine 把 options +表**绑定**到 `select`/`radio`/`multiselect` 三型,却没有把四个窗口键绑定到任何类型。 +在这里自拟一份类型清单,正是 options 注释警告的「第三份会漂移的清单」,并且会把本 +issue 原样复制到下一层:窗口键声明在清单外的类型上,照样解析、照样渲染、照样不执行。 diff --git a/packages/services/service-settings/src/settings-service.test.ts b/packages/services/service-settings/src/settings-service.test.ts index 74f6095d94..9cc3824012 100644 --- a/packages/services/service-settings/src/settings-service.test.ts +++ b/packages/services/service-settings/src/settings-service.test.ts @@ -6,6 +6,7 @@ import { SettingsLockedError, UnknownKeyError, UnknownNamespaceError, envKeyOf } import { NoopCryptoAdapter } from './crypto-adapter.js'; import { mailSettingsManifest, mailTestActionHandler } from './manifests/mail.manifest.js'; import { aiSettingsManifest } from './manifests/ai.manifest.js'; +import { authSettingsManifest } from './manifests/auth.manifest.js'; import { brandingSettingsManifest } from './manifests/branding.manifest.js'; import { featureFlagsSettingsManifest } from './manifests/feature-flags.manifest.js'; import { SettingsManifestSchema } from '@objectstack/spec/system'; @@ -721,12 +722,17 @@ describe('SettingsService — env overrides are checked against declared options }); it('leaves keys with no declared option table completely alone', async () => { - // The check must not widen past `select`/`radio`/`multiselect` with a table: - // a free-text, a boolean and a number env override behave exactly as before. + // The OPTION check must not widen past `select`/`radio`/`multiselect` with + // a table. Note the two values below are legal on every axis, so this stays + // a pin for the option check specifically: `branding.workspace_name` is + // free text to the enumeration but does declare `minLength: 1, + // maxLength: 60`, and since #5932 that window IS enforced on this same path + // — `EnvCorp` (7) simply sits inside it. `feature_flags.ai_enabled` is a + // toggle and declares no window at all. const { errors, logger } = spyLogger(); const svc = new SettingsService({ env: { - OS_BRANDING_WORKSPACE_NAME: 'EnvCorp', // text — any string is legal + OS_BRANDING_WORKSPACE_NAME: 'EnvCorp', // no option table; length 7 of [1, 60] OS_FEATURE_FLAGS_AI_ENABLED: 'true', // boolean — coerced, not enumerated }, logger, @@ -945,6 +951,426 @@ describe('SettingsService — env overrides are checked against declared options }); }); +/** + * #5932 — a manifest's declared value WINDOW is enforced at SAVE time. + * + * The third member of the family `required`/`pattern` (#4224) and `options` + * (#5131) already belong to. `SpecifierSchema` has declared five value + * constraints since it existed — `pattern`, `min`, `max`, `minLength`, + * `maxLength` — and `validatePatch` read exactly one of them. The other four + * had no reader anywhere on the write path: 42 specifiers across the shipped + * manifests declared a window, and every one of those windows was decoration. + * + * The load-bearing case is `auth.password_min_length`. It declares `min: 6`, + * the console renders a number input with that floor, and `PUT + * /api/settings/auth` accepted `1` — or `-3` — and stored it, whereupon + * better-auth's password policy honoured the stored number. The declaration was + * the only thing claiming a floor existed, and nothing at all was holding it. + */ +describe('SettingsService — save-time validation (declared value windows are enforced, #5932)', () => { + /** The issue's own probe manifest, verbatim. */ + const boundsManifest = { + namespace: 'probe', + version: 1, + label: 'Probe', + scope: 'tenant', + readPermission: 'setup.access', + writePermission: 'setup.access', + specifiers: [ + { type: 'number', key: 'quota', label: 'Quota', required: false, default: 10, min: 0, max: 100 }, + { type: 'text', key: 'code', label: 'Code', required: false, minLength: 2, maxLength: 4 }, + { type: 'slider', key: 'ratio', label: 'Ratio', required: false, default: 0.5, min: 0, max: 1 }, + ], + } as any; + + const probeService = () => { + const svc = new SettingsService({ env: {} }); + svc.registerManifest(boundsManifest); + return svc; + }; + + it('refuses a number below the declared min, naming the window', async () => { + const svc = probeService(); + await expect(svc.setMany('probe', { quota: -500 })).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [ + { + field: 'quota', + // The `FieldErrorCode` member that mirrors the breached property + // (ADR-0114 D2) — the same one `record-validator.ts` emits for the + // same breach on a field def. + code: 'min_value', + label: 'Quota', + // BOTH declared bounds travel as a discrete constraint, which is the + // shape `errors.zod.ts` documents by example (`{ min: 0, max: 120 }` + // on a `max_value`): a client rendering the input needs the whole + // window, not only the side that was breached. + constraint: { min: 0, max: 100 }, + value: -500, + }, + ], + }); + // Atomic: the rejected batch persisted nothing. + expect((await svc.get('probe', 'quota')).source).toBe('default'); + }); + + it('refuses a number above the declared max', async () => { + const svc = probeService(); + await expect(svc.setMany('probe', { quota: 999999 })).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [{ field: 'quota', code: 'max_value', constraint: { min: 0, max: 100 }, value: 999999 }], + }); + }); + + it('refuses a string shorter than minLength and longer than maxLength', async () => { + const svc = probeService(); + await expect(svc.setMany('probe', { code: 'X' })).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [ + { + field: 'code', + code: 'min_length', + // `actual` rides along for the length codes, the way the record + // validator already emits it — a client formatting "3 of 4 max" + // should not have to measure the string it just sent back. + constraint: { minLength: 2, maxLength: 4, actual: 1 }, + }, + ], + }); + await expect(svc.setMany('probe', { code: 'ABCDEFGHIJ' })).rejects.toMatchObject({ + fields: [ + { field: 'code', code: 'max_length', constraint: { minLength: 2, maxLength: 4, actual: 10 } }, + ], + }); + }); + + it('refuses a slider outside its declared window', async () => { + const svc = probeService(); + await expect(svc.setMany('probe', { ratio: 42 })).rejects.toMatchObject({ + fields: [{ field: 'ratio', code: 'max_value', constraint: { min: 0, max: 1 } }], + }); + }); + + it('accepts every value inside the window, bounds INCLUSIVE', async () => { + // `min`/`max` name the window's members, not the values just outside it — + // an off-by-one here would reject `password_history_count: 0` (declared + // `min: 0`, and the value that DISABLES the check) on every workspace. + const svc = probeService(); + await expect(svc.setMany('probe', { quota: 0 })).resolves.toBeDefined(); + await expect(svc.setMany('probe', { quota: 100 })).resolves.toBeDefined(); + await expect(svc.setMany('probe', { quota: 50 })).resolves.toBeDefined(); + await expect(svc.setMany('probe', { code: 'ab' })).resolves.toBeDefined(); + await expect(svc.setMany('probe', { code: 'abcd' })).resolves.toBeDefined(); + await expect(svc.setMany('probe', { ratio: 0 })).resolves.toBeDefined(); + await expect(svc.setMany('probe', { ratio: 1 })).resolves.toBeDefined(); + }); + + it('compares a number that arrived as a string, so a form post is not enforced-as-transport', async () => { + // Same rule the option table applies for the same reason: a stored value + // has been through JSON and, over REST, a form post. + const svc = probeService(); + await expect(svc.setMany('probe', { quota: '50' })).resolves.toBeDefined(); + await expect(svc.setMany('probe', { quota: '999999' })).rejects.toMatchObject({ + fields: [{ field: 'quota', code: 'max_value' }], + }); + }); + + it('checks the window only when the patch TOUCHES the key', async () => { + // The #5131 gate, inherited. Bounds get TIGHTENED over a product's life, so + // a workspace can hold a value that was legal when it was written — here, + // written under `max: 1000` and re-registered under `max: 100`. It must not + // be locked out of its own settings page over that. + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + ...boundsManifest, + specifiers: boundsManifest.specifiers.map((s: any) => + s.key === 'quota' ? { ...s, max: 1000 } : s, + ), + } as any); + await svc.setMany('probe', { quota: 900 }); + svc.registerManifest(boundsManifest); // the narrowed window + + // The stale value is still there … + expect((await svc.get('probe', 'quota')).value).toBe(900); + // … and a patch that never mentions `quota` is not rejected on its account. + await expect(svc.setMany('probe', { code: 'abc' })).resolves.toBeDefined(); + expect((await svc.get('probe', 'code')).value).toBe('abc'); + // Only re-writing the key itself is refused. + await expect(svc.setMany('probe', { quota: 900 })).rejects.toMatchObject({ + fields: [{ field: 'quota', code: 'max_value' }], + }); + // And a reset still clears it — an all-null patch is never blocked. + await expect(svc.resetNamespace('probe')).resolves.toBeGreaterThan(0); + expect((await svc.get('probe', 'quota')).value).toBe(10); // back to the default + }); + + it('leaves a specifier that declares no window completely alone', async () => { + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'freeform', version: 1, label: 'Freeform', scope: 'tenant', + readPermission: 'setup.access', writePermission: 'setup.access', + specifiers: [ + { type: 'number', key: 'anything', label: 'Anything' }, + { type: 'text', key: 'note', label: 'Note' }, + ], + } as any); + await expect(svc.setMany('freeform', { anything: -1e9 })).resolves.toBeDefined(); + await expect(svc.setMany('freeform', { note: '' })).resolves.toBeDefined(); + await expect(svc.setMany('freeform', { note: 'x'.repeat(5000) })).resolves.toBeDefined(); + }); + + it('leaves a value it cannot compare alone — that is a different constraint', async () => { + // Policing the value's SHAPE is `invalid_type`/`invalid_number`, a different + // constraint with a different owner. Coercing here would be worse than + // silent: `Number(true)` is 1 and `Number([])` is 0, so a boolean written to + // a `min: 6` key would be REJECTED as "1" — inventing a verdict about a + // shape this check was never asked to judge. + const svc = probeService(); + await expect(svc.setMany('probe', { quota: true })).resolves.toBeDefined(); + await expect(svc.setMany('probe', { quota: 'not-a-number' })).resolves.toBeDefined(); + await expect(svc.setMany('probe', { code: 12345 })).resolves.toBeDefined(); + // Empty is `required`'s business, not the window's — a blank text field + // under `minLength: 2` is "unset", not "too short". + await expect(svc.setMany('probe', { code: '' })).resolves.toBeDefined(); + await expect(svc.setMany('probe', { code: null })).resolves.toBeDefined(); + }); + + it('never echoes the out-of-window value for an encrypted specifier', async () => { + // Same rule as `invalid_option`, same reason: a bound is not a secret, but + // `encrypted` is authorable on any specifier and this message travels back + // through the API and into logs. + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'vault', version: 1, label: 'Vault', scope: 'tenant', + readPermission: 'setup.access', writePermission: 'setup.access', + specifiers: [ + { type: 'text', key: 'token', label: 'Token', encrypted: true, minLength: 32 }, + ], + } as any); + const err = await svc.setMany('vault', { token: 's3cr3t' }).catch((e) => e); + expect(err.code).toBe('SETTINGS_VALIDATION'); + expect(err.fields[0]).toMatchObject({ field: 'token', code: 'min_length' }); + expect(err.fields[0].value).toBeUndefined(); + expect(err.message).not.toContain('s3cr3t'); + // The window still travels, so the caller learns what to do. + expect(err.fields[0].constraint).toMatchObject({ minLength: 32, actual: 6 }); + }); + + it('reports one FieldError per key — the first constraint the value broke', async () => { + // `required` and `invalid_option` already `continue` once they have spoken; + // the window check keeps that contract so a client is handed one verdict to + // render rather than a pile it must rank itself. + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'both', version: 1, label: 'Both', scope: 'tenant', + readPermission: 'setup.access', writePermission: 'setup.access', + specifiers: [ + { type: 'text', key: 'slug', label: 'Slug', pattern: '^[a-z]+$', minLength: 5 }, + ], + } as any); + const err = await svc.setMany('both', { slug: 'AB' }).catch((e) => e); + expect(err.fields).toHaveLength(1); + expect(err.fields[0].code).toBe('invalid_format'); // pattern is checked first + }); + + it('closes the password-policy hole end-to-end on the real auth manifest', async () => { + // THE case the issue was filed for. `auth.password_min_length` declares + // `min: 6, max: 64`; before this gate a `PUT /api/settings/auth` writing `1` + // was accepted, stored, and honoured by better-auth's password policy. + const svc = new SettingsService({ env: {} }); + svc.registerManifest(authSettingsManifest); + + await expect(svc.setMany('auth', { password_min_length: 1 })).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [ + { + field: 'password_min_length', + code: 'min_value', + label: 'Minimum password length', + constraint: { min: 6, max: 64 }, + value: 1, + }, + ], + }); + // Nothing was stored — the floor is still whatever the manifest says. + expect((await svc.get('auth', 'password_min_length')).value).toBe(8); + // Negatives, the other half of the issue's table. + await expect(svc.setMany('auth', { password_history_count: -1 })).rejects.toMatchObject({ + fields: [{ field: 'password_history_count', code: 'min_value', constraint: { min: 0, max: 24 } }], + }); + await expect(svc.setMany('auth', { password_expiry_days: -1 })).rejects.toMatchObject({ + fields: [{ field: 'password_expiry_days', code: 'min_value' }], + }); + // …and a legal tightening still goes through. + await expect(svc.setMany('auth', { password_min_length: 12 })).resolves.toBeDefined(); + expect((await svc.get('auth', 'password_min_length')).value).toBe(12); + }); + + it('covers the rest of the six password-policy keys the issue tabulated', async () => { + const svc = new SettingsService({ env: {} }); + svc.registerManifest(authSettingsManifest); + // `password_min_classes` is visible only with complexity on, so the patch + // carries the toggle it depends on — otherwise the TOUCH gate skips it as + // an invisible specifier, which is the required/visible contract, not a hole. + await expect( + svc.setMany('auth', { password_require_complexity: true, password_min_classes: 9 }), + ).rejects.toMatchObject({ + fields: [{ field: 'password_min_classes', code: 'max_value', constraint: { min: 1, max: 4 } }], + }); + await expect(svc.setMany('auth', { password_max_length: 4 })).rejects.toMatchObject({ + fields: [{ field: 'password_max_length', code: 'min_value', constraint: { min: 16, max: 256 } }], + }); + }); +}); + +/** + * #5932, env half — the declared window is enforced on the ENV side too, at the + * ONE decision point the option table is already judged at. + * + * This is the explicit ruling on the issue: #5204 exists because the same + * comparison lived in two places and the two drifted, so the window check goes + * through `effectiveEnvOverride` rather than opening a second implementation on + * the env path. Consequences follow from that single point, not from a second + * copy of the policy: a rejected override is IGNORED (never repaired), it + * contributes no cascade entry, it pins nothing, and it is reported once. + */ +describe('SettingsService — env overrides are checked against declared windows (#5932)', () => { + const spyLogger = () => { + const errors: string[] = []; + return { errors, logger: { error: (m: string) => void errors.push(m) } }; + }; + + it('ignores OS_AUTH_PASSWORD_MIN_LENGTH=1 and resolves the manifest default instead', async () => { + // The issue's own env repro: the same value the save path now refuses, + // arriving through the one door that had no gate on it. + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_AUTH_PASSWORD_MIN_LENGTH: '1' }, logger }); + svc.registerManifest(authSettingsManifest); + + const r = await svc.get('auth', 'password_min_length'); + expect(r.value).toBe(8); // the manifest default, not 1 + expect(r.source).toBe('default'); + // Not in force, so it pins nothing either — read and write agree. + expect(r.locked).toBe(false); + expect(r.cascadeChain?.some((e) => e.scope === 'env')).toBe(false); + + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('OS_AUTH_PASSWORD_MIN_LENGTH'); + expect(errors[0]).toContain('min 6, max 64'); + expect(errors[0]).toContain('IGNORED'); + expect(errors[0]).toContain('does NOT take effect'); + }); + + it('an in-window override still wins at the top of the cascade and locks the key', async () => { + // The regression pin for the untouched path — the check must not turn into + // "env never applies to a bounded key". + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_AUTH_PASSWORD_MIN_LENGTH: '12' }, logger }); + svc.registerManifest(authSettingsManifest); + + const r = await svc.get('auth', 'password_min_length'); + expect(r.value).toBe(12); + expect(r.source).toBe('env'); + expect(r.locked).toBe(true); + expect(errors).toHaveLength(0); + }); + + it('a REJECTED override pins nothing — the key stays editable', async () => { + // The `locked` coherence rule #5204 established, extended to the second + // family for free BECAUSE both are judged at the one point: a key + // configurable by nothing (env ignored, UI refused) would be a lockout only + // an env edit could clear. + const { logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_AUTH_PASSWORD_MIN_LENGTH: '1' }, logger }); + svc.registerManifest(authSettingsManifest); + + expect((await svc.get('auth', 'password_min_length')).locked).toBe(false); + await expect(svc.set('auth', 'password_min_length', 10)).resolves.toBeDefined(); + const after = await svc.get('auth', 'password_min_length'); + expect(after.value).toBe(10); + expect(after.source).toBe('global'); // the auth manifest is `scope: 'global'` + }); + + it('rejects a too-long text override by the same rule', async () => { + // `branding.workspace_name` declares `minLength: 1, maxLength: 60`. + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ + env: { OS_BRANDING_WORKSPACE_NAME: 'N'.repeat(61) }, + logger, + }); + svc.registerManifest(brandingSettingsManifest); + + const r = await svc.get('branding', 'workspace_name'); + expect(r.source).toBe('default'); + expect(r.locked).toBe(false); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('min 1, max 60 characters'); + expect(errors[0]).toContain('outside the declared length'); + }); + + it('reports the misconfiguration at registration, before anything reads the key', async () => { + // Same contract as the option table: a boot-time line for an override that + // will never take effect, including for keys nothing reads this process. + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_AUTH_PASSWORD_MIN_LENGTH: '1' }, logger }); + expect(errors).toHaveLength(0); + svc.registerManifest(authSettingsManifest); + expect(errors).toHaveLength(1); + }); + + it('registration REPORTS but never refuses — a stale pin must not block a boot', async () => { + const { logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_AUTH_PASSWORD_MIN_LENGTH: '1' }, logger }); + expect(() => svc.registerManifest(authSettingsManifest)).not.toThrow(); + await expect(svc.setMany('auth', { password_min_length: 10 })).resolves.toBeDefined(); + }); + + it('says it ONCE, not once per read', async () => { + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_AUTH_PASSWORD_MIN_LENGTH: '1' }, logger }); + svc.registerManifest(authSettingsManifest); + for (let i = 0; i < 5; i++) await svc.get('auth', 'password_min_length'); + await svc.getNamespace('auth'); + expect(errors).toHaveLength(1); + }); + + it('never echoes the rejected value for an encrypted specifier', async () => { + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_VAULT_TOKEN: 's3cr3t' }, logger }); + svc.registerManifest({ + namespace: 'vault', version: 1, label: 'Vault', scope: 'tenant', + readPermission: 'setup.access', writePermission: 'setup.access', + specifiers: [ + { type: 'text', key: 'token', label: 'Token', required: false, encrypted: true, minLength: 32 }, + ], + } as any); + + expect(errors).toHaveLength(1); + expect(errors[0]).not.toContain('s3cr3t'); + expect(errors[0]).toContain('OS_VAULT_TOKEN'); + expect(errors[0]).toContain('min 32 characters'); + }); + + it('leaves an env value it cannot compare alone', async () => { + // A `number` specifier with no declared default: `coerceEnvValue` has no + // type hint, so the raw string survives. A non-numeric one is not judged + // here — the same "different constraint, different owner" rule the save + // path takes. + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_LOOSE_COUNT: 'lots' }, logger }); + svc.registerManifest({ + namespace: 'loose', version: 1, label: 'Loose', scope: 'tenant', + readPermission: 'setup.access', writePermission: 'setup.access', + specifiers: [{ type: 'number', key: 'count', label: 'Count', min: 0, max: 10 }], + } as any); + + const r = await svc.get('loose', 'count'); + expect(r.value).toBe('lots'); + expect(r.source).toBe('env'); + expect(errors).toHaveLength(0); + }); +}); + describe('SettingsService — user-scoped values', () => { it('isolates writes by ctx.userId', async () => { const svc = new SettingsService({ env: {} }); diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index bcc8e1eaed..88ffc1e146 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -111,6 +111,150 @@ function firstRejectedOption(allowed: string[], value: unknown): { value: unknow return at === -1 ? null : { value: picked[at] }; } +/** + * The value bounds a specifier declares — `min`/`max` (numeric window) and + * `minLength`/`maxLength` (string length window), as `SpecifierSchema` spells + * them (#5932). + */ +interface DeclaredBounds { + min?: number; + max?: number; + minLength?: number; + maxLength?: number; +} + +/** A breached bound, in the form both call sites need to report it. */ +interface RangeViolation { + /** `FieldErrorCode`, ADR-0114 — the member that mirrors the breached property. */ + code: 'min_value' | 'max_value' | 'min_length' | 'max_length'; + /** `range` (numeric window) or `length` (character window) — picks the prose. */ + kind: 'range' | 'length'; + /** The declared window in machine form, for `FieldError.constraint`. */ + constraint: Record; + /** The declared window in prose (`min 6, max 64`), for the env log line. */ + declared: string; +} + +/** + * The bounds this specifier declares, or `null` when it declares none. + * + * Keyed on the DECLARATION, not on the specifier `type` — deliberately, and + * unlike the `options` check next door, which keys on `OPTION_BEARING_TYPES`. + * The difference is the spec's, not a preference: `SpecifierSchema`'s + * superRefine *ties* an option table to exactly `select`/`radio`/`multiselect` + * (it rejects those three without one), so "types that must declare a table" + * and "types whose value is checked against it" are the same set and no third + * list can drift. It ties `min`/`max`/`minLength`/`maxLength` to nothing — + * doc comments say `number`/`slider` and `text`/`textarea`, but the schema + * accepts them on any specifier and only checks their ordering. Inventing a + * type list here would therefore BE the third list, and it would recreate this + * issue one level down: a `min` authored on a type not on my list would parse, + * render, and quietly not be enforced. So: declared is enforced, wherever it is + * declared. The value's SHAPE decides applicability instead (see + * {@link firstRangeViolation}), which is exactly how the `pattern` branch in + * `validatePatch` has always worked. + */ +function declaredBounds(spec: { + min?: unknown; max?: unknown; minLength?: unknown; maxLength?: unknown; +}): DeclaredBounds | null { + const out: DeclaredBounds = {}; + let any = false; + for (const k of ['min', 'max', 'minLength', 'maxLength'] as const) { + const v = spec[k]; + if (typeof v === 'number' && Number.isFinite(v)) { + out[k] = v; + any = true; + } + } + return any ? out : null; +} + +/** + * The value as a number, or `null` when it is not a number at all. + * + * A string that parses is admitted for the same reason `declaredOptionValues` + * compares in string form: a stored value has been through JSON and, over the + * REST boundary, a form post, so a `number` specifier legitimately reads back + * as `'42'` and rejecting that would enforce the transport rather than the + * bound. Booleans, arrays and objects are NOT coerced (`Number(true)` is 1, + * `Number([])` is 0) — a value of the wrong shape is `invalid_type`, a + * different constraint with a different owner, and inventing it here would + * reject writes this check was never asked to touch. Same posture, same + * sentence, as `firstRejectedOption`. + */ +function numericValue(value: unknown): number | null { + if (typeof value === 'number') return Number.isFinite(value) ? value : null; + if (typeof value === 'string' && value.trim() !== '') { + const n = Number(value); + return Number.isFinite(n) ? n : null; + } + return null; +} + +/** + * The bound this value breaches, or `null` when it sits inside every declared + * window. + * + * ONE comparison, shared by both paths that produce an effective value — the + * save path ({@link SettingsService.validatePatch}) and the env path + * ({@link SettingsService.effectiveEnvOverride}) — for the same reason + * {@link firstRejectedOption} is shared: #5204 exists precisely because one of + * those two consulted the declared table and the other did not, and #5932's + * triage ruled that the env half must reuse the one judgment point rather than + * open a second implementation of the same comparison. + * + * A value that is not comparable against a declared window is left alone (a + * non-numeric value under `min`/`max`, a non-string under + * `minLength`/`maxLength`). Check ordering mirrors `record-validator.ts`'s + * equivalent branches so the two report the same bound for the same value; a + * value can breach only one side of a well-ordered window anyway, and + * `SpecifierSchema` rejects `min > max` at parse time. + */ +function firstRangeViolation(bounds: DeclaredBounds, value: unknown): RangeViolation | null { + const { min, max, minLength, maxLength } = bounds; + + if (typeof min === 'number' || typeof max === 'number') { + const n = numericValue(value); + if (n !== null) { + const constraint: Record = {}; + if (typeof min === 'number') constraint.min = min; + if (typeof max === 'number') constraint.max = max; + const declared = [ + typeof min === 'number' ? `min ${min}` : null, + typeof max === 'number' ? `max ${max}` : null, + ].filter(Boolean).join(', '); + if (typeof min === 'number' && n < min) { + return { code: 'min_value', kind: 'range', constraint, declared }; + } + if (typeof max === 'number' && n > max) { + return { code: 'max_value', kind: 'range', constraint, declared }; + } + } + } + + if (typeof minLength === 'number' || typeof maxLength === 'number') { + if (typeof value === 'string') { + const actual = value.length; + const constraint: Record = {}; + if (typeof minLength === 'number') constraint.minLength = minLength; + if (typeof maxLength === 'number') constraint.maxLength = maxLength; + constraint.actual = actual; + const declared = [ + typeof minLength === 'number' ? `min ${minLength}` : null, + typeof maxLength === 'number' ? `max ${maxLength}` : null, + ].filter(Boolean).join(', ') + ' characters'; + if (typeof maxLength === 'number' && actual > maxLength) { + return { code: 'max_length', kind: 'length', constraint, declared }; + } + if (typeof minLength === 'number' && actual < minLength) { + return { code: 'min_length', kind: 'length', constraint, declared }; + } + } + } + + return null; +} + interface RegisteredManifest { manifest: SettingsManifest; /** Resolved specifier scopes for fast lookup. */ @@ -134,6 +278,16 @@ interface RegisteredManifest { * "check against an empty set", which would reject everything. */ optionTables: Map; + /** + * Declared value bounds for every specifier that declares at least one of + * `min` / `max` / `minLength` / `maxLength`, keyed by specifier key (#5932). + * + * Precomputed for the same reason and read the same way as `optionTables`: + * `get()` is the hottest path, and an ABSENT key means "this specifier + * declares no window" — nothing to enforce, unchanged behaviour — rather + * than "an empty window", which would reject everything. + */ + bounds: Map; } /** @@ -286,12 +440,18 @@ export class SettingsService { const encryptedKeys = new Set(); const defaults = new Map(); const optionTables = new Map(); + const bounds = new Map(); const defaultScope = manifest.scope ?? 'tenant'; for (const spec of manifest.specifiers) { if (!spec.key || LAYOUT_ONLY_TYPES.has(spec.type)) continue; scopes.set(spec.key, spec.scope ?? defaultScope); if (spec.encrypted || spec.type === 'password') encryptedKeys.add(spec.key); if (typeof spec.default !== 'undefined') defaults.set(spec.key, spec.default); + // Declared bounds are recorded wherever they are declared — see + // `declaredBounds` for why this is keyed on the declaration and the + // option table is keyed on the type (#5932). + const declared = declaredBounds(spec); + if (declared) bounds.set(spec.key, declared); if (OPTION_BEARING_TYPES.has(spec.type)) { // A manifest with no option table cannot say what is legal. The spec // refuses that shape at parse time, but `registerManifest` takes @@ -311,6 +471,7 @@ export class SettingsService { defaults, actions, optionTables, + bounds, }); this.auditEnvOverrides(manifest.namespace); } @@ -334,11 +495,13 @@ export class SettingsService { */ private auditEnvOverrides(namespace: string): void { const reg = this.registry.get(namespace); - if (!reg || reg.optionTables.size === 0) return; - // Only the option-bearing keys can be rejected, so only they are worth - // walking. `effectiveEnvOverride` does the judging (and the reporting); - // the value it returns is of no interest here. - for (const key of reg.optionTables.keys()) { + if (!reg) return; + // Only the keys that declare something enforceable can be rejected, so only + // they are worth walking: an option table (#5131/#5204) or a value window + // (#5932). `effectiveEnvOverride` does the judging (and the reporting); the + // value it returns is of no interest here. + const enforceable = new Set([...reg.optionTables.keys(), ...reg.bounds.keys()]); + for (const key of enforceable) { this.effectiveEnvOverride(reg, namespace, key); } } @@ -364,6 +527,14 @@ export class SettingsService { * * Reporting lives here rather than at the call sites so no future fourth * caller can read an override without the rejection being heard. + * + * #5932 added the second family of declared constraints (`min`/`max`/ + * `minLength`/`maxLength`) HERE rather than on the env path's own terms, for + * the reason #5204 is on file: the same comparison in two places is how the + * env half came to disagree with the save half in the first place. Both + * families are judged at this one point, by the same helpers the save path + * calls, and both produce the same verdict — an override that is not in force + * contributes no value and pins nothing. */ private effectiveEnvOverride( reg: RegisteredManifest, @@ -375,15 +546,36 @@ export class SettingsService { if (typeof envRaw !== 'string') return null; const value = coerceEnvValue(envRaw, reg.defaults.get(key)); + // A key with no declared table has nothing to enforce — unchanged behaviour. const allowed = reg.optionTables.get(key); - if (!allowed) return { envName, value }; + if (allowed) { + const rejected = firstRejectedOption(allowed, value); + if (rejected) { + this.reportRejectedEnvOverride(reg, namespace, key, envName, rejected.value, { + what: 'is not a declared option for', + detail: `Allowed values: ${allowed.join(', ')}.`, + fix: 'one of the allowed values', + }); + return null; + } + } - const rejected = firstRejectedOption(allowed, value); - if (!rejected) return { envName, value }; + // Likewise a key with no declared window (#5932). + const bounds = reg.bounds.get(key); + if (bounds) { + const breach = firstRangeViolation(bounds, value); + if (breach) { + this.reportRejectedEnvOverride(reg, namespace, key, envName, value, { + what: `is outside the declared ${breach.kind} for`, + detail: `Allowed ${breach.kind}: ${breach.declared}.`, + fix: `a value within the allowed ${breach.kind}`, + }); + return null; + } + } - this.reportRejectedEnvOverride(reg, namespace, key, envName, allowed, rejected.value); - return null; + return { envName, value }; } /** @@ -422,8 +614,14 @@ export class SettingsService { namespace: string, key: string, envName: string, - allowed: string[], offending: unknown, + /** + * What this particular declaration refused, as the three fragments the one + * sentence template needs. Passed in rather than branched on here so a + * third constraint family reuses the dedupe, the redaction rule and the + * consequence/fix prose instead of growing a second reporter beside them. + */ + rejection: { what: string; detail: string; fix: string }, ): void { const dedupeAt = `${envName}=${String(offending)}`; if (this.reportedEnvOverrides.has(dedupeAt)) return; @@ -436,13 +634,13 @@ export class SettingsService { const secret = reg.encryptedKeys.has(key); const rejected = secret ? '' : ` Rejected value: '${String(offending)}'.`; const message = - `[SettingsService] env override ${envName} is not a declared option for ` + - `setting '${namespace}.${key}' — IGNORED.${rejected} Allowed values: ${allowed.join(', ')}. ` + + `[SettingsService] env override ${envName} ${rejection.what} ` + + `setting '${namespace}.${key}' — IGNORED.${rejected} ${rejection.detail} ` + `Consequence: this override does NOT take effect and nothing else looks wrong — ` + `'${namespace}.${key}' resolves from the next layer of the cascade instead ` + `(a stored global/tenant/user value, else the manifest default), and reads report ` + `THAT layer as the source rather than 'env'. ` + - `Fix: set ${envName} to one of the allowed values, or unset it and configure ` + + `Fix: set ${envName} to ${rejection.fix}, or unset it and configure ` + `'${namespace}.${key}' through the settings UI.`; if (this.logger?.error) this.logger.error(message); @@ -884,6 +1082,9 @@ export class SettingsService { * - `pattern` (text fields) + non-empty value that mismatches → rejected. * - `options` (`select`/`radio`/`multiselect`) + non-empty value outside * the declared table → rejected (`invalid_option`). + * - `min` / `max` / `minLength` / `maxLength` + non-empty value outside the + * declared window → rejected (`min_value` / `max_value` / `min_length` / + * `max_length`, #5932). * - All-null patches (namespace reset) and unparseable visibility * expressions skip validation rather than block the write. * @@ -894,7 +1095,15 @@ export class SettingsService { * alone is not rejected because a stale `provider` sits in the store — * otherwise every workspace carrying historical drift would be locked out * of its own settings page, unable to edit anything, which is worse than - * the gap this closes. + * the gap this closes. The value-window check (#5932) inherits that gate + * for the same reason and one more: bounds get TIGHTENED over a product's + * life (a `password_min_length` floor raised from 6 to 8), and a workspace + * sitting below the new floor must still be able to edit its unrelated + * settings — it is told about the key only when it writes that key. + * + * At most one `FieldError` per offending key: every branch above `continue`s + * once it has spoken, so a client is handed the first constraint the value + * broke rather than a pile it must rank itself. */ private async validatePatch( namespace: string, @@ -1021,6 +1230,45 @@ export class SettingsService { // rather than parsing ours (`FieldError.constraint`, ADR-0114). constraint: { pattern: spec.pattern }, }); + continue; + } + } + + // A declared value window is enforced at save time (#5932). Until this + // branch existed `min`/`max`/`minLength`/`maxLength` were, like the + // `options` table before #5131, a front-end convention: the console + // number input clamps to the declared bounds, so an admin going through + // the UI could not produce a bad one — but `PUT /api/settings/:ns` is an + // authorizable public surface, and a script, a migration or AI-authored + // bootstrap code could write anything at all. The load-bearing case is + // `auth.password_min_length`, which declares `min: 6` and accepted `1` + // (and negatives): the value reaches better-auth's password policy and + // is honoured there, so the declaration was the only thing claiming a + // floor existed and nothing was holding it. + if (!empty) { + const bounds = declaredBounds(spec); + const breach = bounds ? firstRangeViolation(bounds, value) : null; + if (breach) { + // Same redaction rule as `invalid_option`, same reason: a bound is + // not a secret, but `encrypted` is authorable on any specifier and + // this message travels back through the API (and into logs). + const secret = reg.encryptedKeys.has(key); + const got = secret ? '' : ` Received '${String(value)}'.`; + errors.push({ + field: key, + code: breach.code, + message: breach.kind === 'length' + ? `${label} must be within the declared length (${breach.declared}).${got}` + : `${label} must be within the declared range (${breach.declared}).${got}`, + label, + // The declared window as discrete values, so a client composes its + // own sentence instead of parsing ours (`FieldError.constraint`, + // ADR-0114) — `{ min, max }` for a numeric window, and + // `{ minLength, maxLength, actual }` for a length one, the keys + // the spec's own examples and the record validator already use. + constraint: breach.constraint, + ...(secret ? {} : { value }), + }); } } } diff --git a/packages/services/service-sms/src/sms-daily-quota.test.ts b/packages/services/service-sms/src/sms-daily-quota.test.ts index 4d9ab0673f..6fdcd5c12d 100644 --- a/packages/services/service-sms/src/sms-daily-quota.test.ts +++ b/packages/services/service-sms/src/sms-daily-quota.test.ts @@ -57,8 +57,12 @@ describe('normalizeDailyQuota — the clamp lives on the CONSUMER side (#5932)', }); it('rejects garbage to "no limit" and NAMES the offending value', () => { - // Manifest `min: 0` is inert today (#5932) — every one of these can reach - // this reader intact, so each is pinned rather than assumed impossible. + // #5932 has closed the producer half — `daily_quota: -1` is now refused at + // `PUT /api/settings/sms` — but every value below can still reach this + // reader: the window check judges only comparable NUMBERS (a shape verdict + // belongs to `invalid_type`), and it runs under the TOUCH gate, so rows + // stored before the gate survive. Each stays pinned rather than assumed + // impossible. expect(normalizeDailyQuota(-1)).toEqual({ limit: 0, rejected: '-1' }); expect(normalizeDailyQuota(Number.NaN)).toEqual({ limit: 0, rejected: 'NaN' }); expect(normalizeDailyQuota(Number.POSITIVE_INFINITY)).toEqual({ limit: 0, rejected: 'Infinity' }); diff --git a/packages/services/service-sms/src/sms-daily-quota.ts b/packages/services/service-sms/src/sms-daily-quota.ts index 9313749320..73ffe267b2 100644 --- a/packages/services/service-sms/src/sms-daily-quota.ts +++ b/packages/services/service-sms/src/sms-daily-quota.ts @@ -156,13 +156,32 @@ export interface NormalizedDailyQuota { /** * Clamp an authored `sms.daily_quota` into the value actually enforced. * - * **This lives on the CONSUMER side on purpose (#5932).** `SettingsService` - * declares `min`/`max` on a manifest specifier but `validatePatch` does not - * enforce them today, so a `min: 0` declaration is inert: negative, fractional - * and outright non-numeric values all reach a reader intact. Anything that - * depends on the manifest having filtered them is declared-but-unenforced - * (ADR-0049), so the clamp is here, where the value becomes behaviour, and is - * pinned by tests. + * **This lives on the CONSUMER side on purpose (#5932).** It was written while + * `min` was inert — `SettingsService.validatePatch` declared five value + * constraints and read only `pattern`, so a `min: 0` filtered nothing and + * negative, fractional and outright non-numeric values all reached a reader + * intact. #5932 has since closed the producer half: the declared window is + * enforced on the save path and on the `OS_*` env path, so `daily_quota: -1` + * is now refused at `PUT /api/settings/sms` with a `min_value` FieldError. + * + * The clamp stays, and not out of sentiment — three of its inputs are still + * genuinely reachable, so removing it would re-open a live hole rather than + * delete a dead one: + * + * - **Non-numeric values** (`true`, `'unlimited'`, `{ n: 5 }`). The window + * check deliberately does not judge a value's SHAPE — that is `invalid_type`, + * a different constraint with a different owner — so these still arrive. + * - **Fractional counts.** `100.5` is inside `min: 0` and always was; flooring + * is this reader's rule, not the manifest's. + * - **Historical rows.** The save-path check runs under the TOUCH gate, so a + * value stored before the gate existed survives until someone re-writes that + * key — deliberately, so a workspace carrying drift is not locked out of its + * own settings page. + * + * What changed is the failure mode this defends against, not whether it is + * needed: a bad value is now refused where it is authored (loudly, at the + * producer, per ADR-0049), and this clamp is what keeps a value that predates + * or sidesteps that gate from becoming behaviour. * * The rules, and why each is the safe direction for a paid channel: *