From ca6959eac95a100143f1765aa3dfbb1c5b3ad83d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 06:20:14 +0000 Subject: [PATCH] fix(service-settings): fail closed on an unevaluable `visible` predicate (#7169) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validatePatch` answered a `VisibilityParseError` with `catch { continue }`, which skipped the whole specifier. `visible` is the gate every other check hangs off, so an unparseable predicate silently switched off `required`, `options`, `pattern`, `valueDomain` and the value window on that key at once. Per the maintainer ruling of 2026-08-10, the save is now refused: `SettingsValidationError` (`SETTINGS_VALIDATION`, HTTP 400) with one `FieldError` per offending specifier — `invalid_value`, the parse reason in `message`, the predicate under `constraint.visible`. Unconditional, not touch-gated: the console posts only its dirty keys, so a touch gate would never fire on the incident this fixes. All-null resets still bypass it. Also adds `>`, `>=`, `<`, `<=` to the evaluator with the console's JS semantics. `auth` already ships `visible: '${data.lockout_threshold > 0}'`, which this grammar refused — measured on origin/main, `auth.lockout_duration_minutes` accepted `-5` and `99999` against its declared `min: 1, max: 1440`. Without this, failing closed would refuse every write to the `auth` namespace. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015fkdTyGmMD5s8ZtEifvuGy --- .changeset/settings-visible-fail-closed.md | 40 ++++ .../services/service-settings/src/index.ts | 5 + .../src/settings-routes.test.ts | 42 ++++ .../src/settings-service.test.ts | 202 ++++++++++++++++++ .../service-settings/src/settings-service.ts | 81 ++++++- .../src/settings-service.types.ts | 8 + .../src/visibility-eval.test.ts | 52 +++++ .../service-settings/src/visibility-eval.ts | 84 ++++++-- 8 files changed, 496 insertions(+), 18 deletions(-) create mode 100644 .changeset/settings-visible-fail-closed.md diff --git a/.changeset/settings-visible-fail-closed.md b/.changeset/settings-visible-fail-closed.md new file mode 100644 index 0000000000..ce2e64ccca --- /dev/null +++ b/.changeset/settings-visible-fail-closed.md @@ -0,0 +1,40 @@ +--- +"@objectstack/service-settings": patch +--- + +fix(service-settings): refuse a save whose `visible` predicate cannot be evaluated, instead of silently skipping the field's `required` gate (#7169) + +**Before:** a settings specifier whose `visible` predicate the save-time +evaluator could not parse was skipped entirely — `validatePatch` answered the +parse failure with `catch { continue }`. Because `visible` is the gate every +other check hangs off, that `continue` switched off `required`, `options`, +`pattern`, `valueDomain` **and** the value window on that key at once, silently +and permanently, with no diagnostic anywhere. A half-filled provider form saved +clean. + +**After:** the write is refused. `setMany` throws `SettingsValidationError` +(`SETTINGS_VALIDATION`, HTTP 400) carrying one `FieldError` per offending +specifier — `code: 'invalid_value'`, the parse reason in `message`, and the +predicate itself under `constraint.visible`, so a client can name which +expression refused without parsing prose. Refusal is **unconditional**, not +gated on the patch touching the key: the console posts only its dirty keys, so a +touch gate would never fire on the incident this fixes. All-null patches +(namespace reset) still return before the check, so a namespace whose manifest +is broken can always be cleared. + +This is the interim stop-the-bleed half of the maintainer's 2026-08-10 ruling on +#7169. The declaration/implementation gap it stems from is still open: +`packages/spec` types both settings `visible` slots as `ExpressionInputSchema`, +which labels their contents **CEL**, while this service evaluates a hand-rolled +JS-ish subset. Measured over the 94 `visible` predicates in the bundled +manifests, wiring CEL into evaluation would break 93 of them and narrowing the +declared type would break 1 — see the PR for the numbers. Narrowing is +recommended and lands separately in `packages/spec`. + +**Also fixed, and load-bearing for the above:** the evaluator now supports the +relational operators `>`, `>=`, `<`, `<=`, with the same JS semantics the +console's client-side evaluator applies to the same strings. The auth manifest +already shipped `visible: '${data.lockout_threshold > 0}'`, which this grammar +refused — so on the old fail-open path `auth.lockout_duration_minutes` accepted +`-5` and `99999` against its declared `min: 1, max: 1440`. That window is +enforced again. diff --git a/packages/services/service-settings/src/index.ts b/packages/services/service-settings/src/index.ts index 541ee2cb67..bb6ad09504 100644 --- a/packages/services/service-settings/src/index.ts +++ b/packages/services/service-settings/src/index.ts @@ -40,6 +40,11 @@ export { export { evaluateVisibility, referencedKeys, + // #7169 — the `${…}` / envelope unwrapper. Published alongside the evaluator + // because a caller that now REFUSES an unparseable predicate has to be able + // to quote the source it refused, and re-deriving the unwrap in the consumer + // is how the two spellings drift apart. + visibilitySource, VisibilityParseError, } from './visibility-eval.js'; export { diff --git a/packages/services/service-settings/src/settings-routes.test.ts b/packages/services/service-settings/src/settings-routes.test.ts index 1a399b4085..e2747629bf 100644 --- a/packages/services/service-settings/src/settings-routes.test.ts +++ b/packages/services/service-settings/src/settings-routes.test.ts @@ -261,4 +261,46 @@ describe('settings-routes', () => { }), ]); }); + + /** + * #7169 — the STATUS half of the fail-closed refusal. + * + * `SettingsValidationError` carries `code` and the service suite pins that; a + * rejection case's other required assertion is the HTTP `status`, and this + * surface mints it here rather than on the error (ADR-0112 envelope, same + * 400 + `SETTINGS_VALIDATION` mapping every other save-time refusal takes). + * Restore `catch { continue }` in `validatePatch` and this case answers 200. + */ + it('PUT rejects an unparseable `visible` predicate with 400 + SETTINGS_VALIDATION + invalid_value', async () => { + const http = new MockHttp(); + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'celns', + label: 'CEL namespace', + specifiers: [ + { type: 'text', key: 'note', label: 'Note' }, + // CEL the spec's `ExpressionInputSchema` declares legal and this + // service's evaluator cannot parse. + { type: 'text', key: 'api_key', label: 'API key', required: true, + visible: "${data.note in ['x']}" }, + ], + } as any); + registerSettingsRoutes(http, svc, { contextFromRequest: adminProvider }); + + const h = http.routes.get('PUT /api/settings/:namespace')!; + const { req, res, state } = makeReqRes({ + params: { namespace: 'celns' }, + body: { note: 'anything' }, + }); + await h(req, res); + expect(state.status).toBe(400); + expect(state.body.error.code).toBe('SETTINGS_VALIDATION'); + expect(state.body.error.details.fields).toEqual([ + expect.objectContaining({ + field: 'api_key', + code: 'invalid_value', + constraint: { visible: "data.note in ['x']" }, + }), + ]); + }); }); diff --git a/packages/services/service-settings/src/settings-service.test.ts b/packages/services/service-settings/src/settings-service.test.ts index bca1572e43..651f7328c7 100644 --- a/packages/services/service-settings/src/settings-service.test.ts +++ b/packages/services/service-settings/src/settings-service.test.ts @@ -11,6 +11,8 @@ import { localizationSettingsManifest } from './manifests/localization.manifest. import { companySettingsManifest } from './manifests/company.manifest.js'; import { brandingSettingsManifest } from './manifests/branding.manifest.js'; import { featureFlagsSettingsManifest } from './manifests/feature-flags.manifest.js'; +import { builtinSettingsManifests } from './manifests/index.js'; +import { evaluateVisibility, visibilitySource } from './visibility-eval.js'; import { SettingsManifestSchema } from '@objectstack/spec/system'; describe('reference manifests are spec-valid', () => { @@ -2378,3 +2380,203 @@ describe('SettingsService — company.country adopts iso_3166_alpha2 (#6579)', ( expect(okErrors).toHaveLength(0); }); }); + +/** + * #7169 — an unparseable `visible` predicate REFUSES the save. + * + * The producer/consumer split: `packages/spec`'s `settings-manifest.zod.ts` + * types both visibility slots as `ExpressionInputSchema`, which normalizes a + * bare string to `{ dialect: 'cel', source }` — so the spec LABELS these values + * CEL. `visibility-eval.ts` is not CEL. An author writing the dialect the spec + * declares got a `VisibilityParseError`, and `validatePatch` used to answer it + * with `catch { continue }` — skipping the whole specifier, so `required`, + * `options`, `pattern`, `valueDomain` and the value window all stopped being + * enforced on that key, with no diagnostic anywhere. + * + * Maintainer ruling (2026-08-10): fail closed at save time. These cases pin + * that, and they are written to go RED on the fail-open implementation — + * restoring `catch { continue }` turns every one of them into an accepted + * write. A `rejects.toThrow()`-shaped assertion could not do that (the + * fail-open path throws nothing at all), so each asserts the ADR-0112/0114 + * envelope the surface actually emits: `code: 'SETTINGS_VALIDATION'` on the + * error, `code: 'invalid_value'` on the field entry, and the offending + * predicate under `constraint.visible`. The matching HTTP `status` (400) is + * pinned at the route boundary in `settings-routes.test.ts`, which is where + * this surface's status lives. + */ +describe('SettingsService — unparseable `visible` fails closed (#7169)', () => { + /** A manifest whose `api_key` predicate is CEL the evaluator cannot parse. */ + function celService(): SettingsService { + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'celns', + label: 'CEL namespace', + specifiers: [ + { type: 'select', key: 'provider', label: 'Provider', + options: [{ value: 'openai', label: 'OpenAI' }, { value: 'memory', label: 'Memory' }] }, + // CEL membership via the stdlib — the exact spelling `ExpressionInputSchema` + // declares is legal, and the grammar reports `unsupported identifier "in"`. + { type: 'text', key: 'api_key', label: 'API key', required: true, + visible: "${data.provider in ['openai']}" }, + ], + } as any); + return svc; + } + + it('refuses the write and names the predicate', async () => { + const svc = celService(); + const err = await svc.setMany('celns', { provider: 'openai', api_key: 'sk-live' }).catch((e) => e); + expect(err.code).toBe('SETTINGS_VALIDATION'); + expect(err.fields).toEqual([ + expect.objectContaining({ + field: 'api_key', + code: 'invalid_value', + label: 'API key', + constraint: { visible: "data.provider in ['openai']" }, + }), + ]); + // The parse reason travels in the sentence, so the author is told WHAT to fix. + expect(err.fields[0].message).toContain('unsupported identifier "in"'); + expect(err.fields[0].message).toContain('visible'); + }); + + it('fires on the half-filled form that never touches the broken key — the incident', async () => { + // The console posts only its dirty keys, so the empty `required` field is + // absent from the patch. This is the case a TOUCH gate would let through, + // and it is the one #7169 measured: fail-open, the save lands clean with + // `api_key` empty and `required` never consulted. + const svc = celService(); + const err = await svc.setMany('celns', { provider: 'openai' }).catch((e) => e); + expect(err.code).toBe('SETTINGS_VALIDATION'); + expect(err.fields[0]).toMatchObject({ field: 'api_key', code: 'invalid_value' }); + // Nothing was persisted — the batch is atomic. + expect((await svc.get('celns', 'provider')).source).toBe('default'); + }); + + it('refuses a predicate rooted anywhere but `data`, which carries no deps to gate on', async () => { + // `referencedKeys` is a `data.` scan, so a `record.`-rooted predicate + // yields no dependencies at all. Nothing about the patch can make this key + // look relevant, which is why the refusal is unconditional. + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'rootns', + label: 'Root namespace', + specifiers: [ + { type: 'text', key: 'unrelated', label: 'Unrelated' }, + { type: 'text', key: 'gated', label: 'Gated', required: true, + visible: "${record.status == 'open'}" }, + ], + } as any); + const err = await svc.setMany('rootns', { unrelated: 'x' }).catch((e) => e); + expect(err.code).toBe('SETTINGS_VALIDATION'); + expect(err.fields[0]).toMatchObject({ + field: 'gated', + code: 'invalid_value', + constraint: { visible: "record.status == 'open'" }, + }); + }); + + it('still lets a broken namespace be RESET — the escape hatch', async () => { + // An all-null patch returns before the specifier loop, so a workspace whose + // manifest carries a bad predicate is never locked out of clearing it. + const svc = celService(); + await expect(svc.resetNamespace('celns')).resolves.toBeGreaterThanOrEqual(0); + await expect(svc.setMany('celns', { provider: null, api_key: null })).resolves.toBeDefined(); + }); + + it('leaves parseable predicates alone', async () => { + // Control: the same manifest with the grammar's own spelling. `==` IS in the + // grammar, so plain CEL that stays inside it was never affected. + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'okns', + label: 'OK namespace', + specifiers: [ + { type: 'select', key: 'provider', label: 'Provider', + options: [{ value: 'openai', label: 'OpenAI' }, { value: 'memory', label: 'Memory' }] }, + { type: 'text', key: 'api_key', label: 'API key', required: true, + visible: "${data.provider == 'openai'}" }, + ], + } as any); + await expect(svc.setMany('okns', { provider: 'memory' })).resolves.toBeDefined(); + await expect(svc.setMany('okns', { provider: 'openai' })).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [expect.objectContaining({ field: 'api_key', code: 'required' })], + }); + }); + + it('reports every broken specifier, not just the first', async () => { + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'twobad', + label: 'Two bad', + specifiers: [ + { type: 'text', key: 'a', label: 'A', visible: '${data.x.map(y => y)}' }, + { type: 'text', key: 'b', label: 'B', visible: '${window.location}' }, + ], + } as any); + const err = await svc.setMany('twobad', { a: '1' }).catch((e) => e); + expect(err.fields.map((f: any) => f.field)).toEqual(['a', 'b']); + expect(err.fields.every((f: any) => f.code === 'invalid_value')).toBe(true); + }); +}); + +/** + * #7169 — the in-repo instance of the same fail-open, and its repair. + * + * `auth.lockout_duration_minutes` declares `min: 1, max: 1440` and carries + * `visible: '${data.lockout_threshold > 0}'`. The grammar had no relational + * operator, so on `origin/main` the predicate threw, `catch { continue }` + * skipped the specifier, and the declared window was enforced on nobody — + * measured: `-5` and `99999` both saved clean, while the `visible`-free sibling + * `rate_limit_max` was refused correctly by the same branch. + */ +describe('auth lockout window is enforced again (#7169)', () => { + function authService(): SettingsService { + const svc = new SettingsService({ env: {} }); + svc.registerManifest(authSettingsManifest); + return svc; + } + + it('enforces min/max once the lockout is switched on', async () => { + await expect( + authService().setMany('auth', { lockout_threshold: 5, lockout_duration_minutes: 99999 }), + ).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [expect.objectContaining({ field: 'lockout_duration_minutes', code: 'max_value' })], + }); + await expect( + authService().setMany('auth', { lockout_threshold: 5, lockout_duration_minutes: -5 }), + ).rejects.toMatchObject({ + fields: [expect.objectContaining({ field: 'lockout_duration_minutes', code: 'min_value' })], + }); + await expect( + authService().setMany('auth', { lockout_threshold: 5, lockout_duration_minutes: 30 }), + ).resolves.toBeDefined(); + }); + + it('still skips the window while the field is genuinely hidden', async () => { + // `lockout_threshold: 0` disables the lockout, so the duration field is not + // rendered and not validated — the predicate now EVALUATES to false instead + // of failing to parse, which is a different reason for the same outcome and + // the one the manifest intended. + await expect( + authService().setMany('auth', { lockout_threshold: 0, lockout_duration_minutes: 99999 }), + ).resolves.toBeDefined(); + }); + + it('every bundled manifest predicate parses, so no builtin namespace is refused', async () => { + // The corpus measurement, kept as a regression: 94 `visible` predicates + // across the 10 bundled manifests, and a fail-closed save path means any + // one of them falling outside the grammar bricks writes to its namespace. + for (const manifest of builtinSettingsManifests as Array>) { + for (const spec of manifest.specifiers ?? []) { + if (typeof spec.visible === 'undefined') continue; + expect( + () => evaluateVisibility(spec.visible, {}), + `${manifest.namespace}.${spec.key ?? '(layout)'}: ${visibilitySource(spec.visible)}`, + ).not.toThrow(); + } + } + }); +}); diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index 2286305679..ca040d62ce 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -35,7 +35,12 @@ import { knownValueDomain, valueDomainPhrasing, } from './value-domains.js'; -import { evaluateVisibility, referencedKeys } from './visibility-eval.js'; +import { + evaluateVisibility, + referencedKeys, + visibilitySource, + VisibilityParseError, +} from './visibility-eval.js'; const DEFAULT_OBJECT = 'sys_setting'; @@ -1398,8 +1403,12 @@ export class SettingsService { * - `step` + non-empty numeric value that misses the declared grid * (`min + k * step`, or `k * step` where no `min` is declared) → rejected * (`invalid_value`, #6199). - * - All-null patches (namespace reset) and unparseable visibility - * expressions skip validation rather than block the write. + * - All-null patches (namespace reset) skip validation rather than block the + * write. + * - A `visible` predicate `evaluateVisibility` cannot parse → the write is + * REFUSED (`invalid_value`, #7169). See §Unparseable `visible` below for + * why this one is not lenient like its neighbours, and why it carries no + * TOUCH gate. * * The TOUCH gate is what keeps the options check from being a regression * for existing workspaces: a value that pre-dates a manifest's current @@ -1417,6 +1426,39 @@ export class SettingsService { * 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. + * + * ## Unparseable `visible` — the one branch that fails CLOSED (#7169) + * + * Maintainer ruling, 2026-08-10: *"save-time validation must refuse a + * `visible` predicate the actual evaluator cannot parse — a silent skip of + * the `required` gate is the worst failure class this repo names."* + * + * Until then this branch read `catch { continue; // stay lenient }`, and the + * `continue` skipped the **whole specifier**. That is the asymmetry that + * justifies breaking ranks with the lenient neighbours listed above: an + * uncompilable `pattern` or a missing `options` table disables ONE check on + * ONE key, and the rest of the specifier is still judged. `visible` is the + * gate every other check hangs off, so an unparseable one switches off + * `required`, `options`, `pattern`, `valueDomain` and the value window at + * once — invisibly, with no diagnostic anywhere, for as long as the manifest + * stands. + * + * **No TOUCH gate here, deliberately.** The gate above exists for stored + * VALUES that pre-date a manifest's current constraints — data a workspace + * cannot fix from a settings page it would be locked out of. This is not a + * value fault: the defect is in the MANIFEST, it is the same for every + * workspace running that code, and the fix is a one-line edit by whoever + * ships the manifest. Gating it on touch would hide it again in exactly the + * shape #7169 measured — the console posts only its dirty keys, so a + * half-filled form never touches the empty `required` field whose predicate + * is broken, and the refusal would never fire on the incident it exists for. + * Resets still work: an all-null patch returns before this loop, so a + * namespace whose manifest is broken can always be cleared. + * + * The predicate source travels in `constraint.visible` and is NOT redacted + * for `encrypted` keys, unlike the value-bearing branches below: a `visible` + * expression is manifest content that `GET /api/settings/:ns` already serves + * to the console in full. It is the author's own text, never a stored secret. */ private async validatePatch( namespace: string, @@ -1453,21 +1495,48 @@ export class SettingsService { const type = String(spec.type ?? ''); if (!key || LAYOUT_ONLY_TYPES.has(type)) continue; + const label = typeof spec.label === 'string' ? spec.label : key; + let visible = true; let deps: string[] = []; if (typeof spec.visible !== 'undefined') { try { visible = evaluateVisibility(spec.visible, data); deps = referencedKeys(spec.visible); - } catch { - continue; // can't determine visibility — stay lenient + } catch (err) { + // Fail CLOSED (#7169) — see §Unparseable `visible` in the doc comment + // above for the ruling, the asymmetry with the lenient branches, and + // why there is no TOUCH gate on this one. + const source = visibilitySource(spec.visible) ?? String(spec.visible); + const detail = err instanceof VisibilityParseError + ? err.detail + : err instanceof Error ? err.message : String(err); + errors.push({ + field: key, + // `invalid_value` is ADR-0114's declared slot for "rejected for a + // reason no other member names" — the same reading #5712 and #6199 + // took. Nothing in the closed catalog names a manifest-side + // declaration fault, and inventing a member for one service's + // manifest format is precisely the friction that catalog is for. + code: 'invalid_value', + message: + `${label} declares a visibility predicate this service cannot evaluate: ${detail}. ` + + `The write is refused rather than accepted with this field's declared constraints ` + + `silently unenforced — fix the manifest's \`visible\` expression.`, + label, + // The predicate as a discrete value, so a client can name WHICH + // expression refused without parsing the sentence (`FieldError. + // constraint`, ADR-0114). Spelled `visible` — the manifest property + // it comes from — like every other constraint key here. + constraint: { visible: source }, + }); + continue; } } if (!visible) continue; if (!patchKeys.has(key) && !deps.some((d) => patchKeys.has(d))) continue; const value = data[key]; - const label = typeof spec.label === 'string' ? spec.label : key; const empty = value === null || typeof value === 'undefined' || (typeof value === 'string' && value.trim() === ''); diff --git a/packages/services/service-settings/src/settings-service.types.ts b/packages/services/service-settings/src/settings-service.types.ts index 24f110cfb8..d9c09bb1e6 100644 --- a/packages/services/service-settings/src/settings-service.types.ts +++ b/packages/services/service-settings/src/settings-service.types.ts @@ -308,6 +308,14 @@ export class SettingsForbiddenError extends Error { * batch is rejected; `fields` carries one entry per offending key, which * the UI can render inline against the input it addresses. * + * Since #7169 it also carries the one MANIFEST-side fault this surface refuses: + * a specifier whose `visible` predicate the save-time evaluator cannot parse + * (`invalid_value`, with the predicate in `constraint.visible`). Every other + * entry names something wrong with a submitted value; that one names something + * wrong with the manifest, and is here rather than in an error class of its own + * because it is refused on the same write, on the same envelope, and renders + * against the same input. + * * `fields` is `FieldError[]` — the field-level vocabulary ADR-0114 closed * (#3977) — rather than the `Record` map it was until #4224. * The map predated that catalog and named the constraint only in prose, so diff --git a/packages/services/service-settings/src/visibility-eval.test.ts b/packages/services/service-settings/src/visibility-eval.test.ts index cd2714c84f..89a4d68c3e 100644 --- a/packages/services/service-settings/src/visibility-eval.test.ts +++ b/packages/services/service-settings/src/visibility-eval.test.ts @@ -31,6 +31,58 @@ describe('evaluateVisibility', () => { expect(() => evaluateVisibility('${window.location}', {})).toThrow(VisibilityParseError); expect(() => evaluateVisibility("${data.a === 'unterminated}", {})).toThrow(VisibilityParseError); }); + + /** + * #7169 — `>` / `>=` / `<` / `<=`. Not a speculative widening: the auth + * manifest already ships `visible: '${data.lockout_threshold > 0}'`, the + * console renders it (its client-side evaluator is a `new Function`), and + * this grammar refused it — which, before this issue's fail-closed change, + * silently switched off `lockout_duration_minutes`' declared `min: 1, + * max: 1440`. The `settings-service` suite pins that end of it; this pins the + * grammar, including that the semantics match the console's JS. + */ + describe('relational operators (#7169)', () => { + it('evaluates the manifest predicate that used to be unparseable', () => { + expect(evaluateVisibility('${data.lockout_threshold > 0}', { lockout_threshold: 5 })).toBe(true); + expect(evaluateVisibility('${data.lockout_threshold > 0}', { lockout_threshold: 0 })).toBe(false); + }); + + it('covers all four, and tokenizes >= / <= ahead of > / <', () => { + expect(evaluateVisibility('${data.n >= 3}', { n: 3 })).toBe(true); + expect(evaluateVisibility('${data.n > 3}', { n: 3 })).toBe(false); + expect(evaluateVisibility('${data.n <= 3}', { n: 3 })).toBe(true); + expect(evaluateVisibility('${data.n < 3}', { n: 3 })).toBe(false); + }); + + it('composes with the rest of the grammar', () => { + expect(evaluateVisibility("${data.n > 0 && data.mode === 'on'}", { n: 1, mode: 'on' })).toBe(true); + expect(evaluateVisibility("${data.n > 0 && data.mode === 'on'}", { n: 0, mode: 'on' })).toBe(false); + expect(evaluateVisibility('${!(data.n >= 10)}', { n: 2 })).toBe(true); + }); + + it('matches the console evaluator on an absent key rather than inventing a verdict', () => { + // `new Function('data', 'with (data) { return (data.n > 0); }')({})` is + // `false` — `undefined > 0` is `false` in JS, and so is `undefined < 0`. + // The two evaluators disagreeing about a predicate IS the #7169 bug class, + // so this is pinned rather than left to read as an accident. + expect(evaluateVisibility('${data.n > 0}', {})).toBe(false); + expect(evaluateVisibility('${data.n < 0}', {})).toBe(false); + expect(evaluateVisibility('${data.n >= 0}', {})).toBe(false); + }); + + it('carries the parse reason separately from the composed sentence', () => { + // `VisibilityParseError.detail` exists so a caller refusing a save can + // embed the reason in its own message (#7169) instead of nesting ours. + const err = (() => { + try { evaluateVisibility("${data.provider in ['openai']}", {}); return null; } + catch (e) { return e as VisibilityParseError; } + })(); + expect(err).toBeInstanceOf(VisibilityParseError); + expect(err!.source).toBe("data.provider in ['openai']"); + expect(err!.detail).toBe('unsupported identifier "in"'); + expect(err!.message).toContain(err!.detail); + }); + }); }); describe('referencedKeys', () => { diff --git a/packages/services/service-settings/src/visibility-eval.ts b/packages/services/service-settings/src/visibility-eval.ts index 983f8f5092..df0bf59f3c 100644 --- a/packages/services/service-settings/src/visibility-eval.ts +++ b/packages/services/service-settings/src/visibility-eval.ts @@ -15,28 +15,66 @@ * orExpr := andExpr ('||' andExpr)* * andExpr := unary ('&&' unary)* * unary := '!' unary | comparison - * compare := primary (('===' | '!==' | '==' | '!=') primary)? + * compare := primary (('===' | '!==' | '==' | '!=' | + * '>=' | '<=' | '>' | '<') primary)? * primary := '(' orExpr ')' | string | number | true | false | null | data. * - * Anything outside the grammar throws `VisibilityParseError`; callers - * should treat that as "cannot determine visibility" and skip validation - * for the field (lenient) rather than block the save. + * ## The relational operators are here because a manifest already used one + * + * `>` / `>=` / `<` / `<=` were added in #7169, not speculatively: the auth + * manifest ships `visible: '${data.lockout_threshold > 0}'`, the console's + * client-side evaluator (a `new Function(...)` over the same string) rendered + * it correctly, and THIS grammar refused it — so the save path threw and, under + * the lenient contract this header used to state, skipped the whole + * `lockout_duration_minutes` specifier. Measured on `origin/main`: that key + * declares `min: 1, max: 1440` and accepted `-5` and `99999`, while its + * `visible`-free sibling `rate_limit_max` was refused correctly. The two + * evaluators disagreeing about what parses IS the bug class this file sits in, + * so the server grammar catches up to the operator the manifests demonstrably + * reach for, with JS relational semantics — the console's — deliberately. + * + * ## Callers must fail CLOSED (#7169, maintainer ruling 2026-08-10) + * + * Anything outside the grammar throws `VisibilityParseError`. This header used + * to tell callers to treat that as "cannot determine visibility" and skip + * validation for the field, lenient. That is exactly backwards for a save-time + * gate: `visible` does not gate one check, it gates EVERY check on the + * specifier (`required`, `options`, `pattern`, `valueDomain`, the value + * window), so a predicate this evaluator cannot parse silently switches all of + * them off. A caller enforcing declared constraints MUST refuse the write and + * report the parse failure — see `validatePatch` in `settings-service.ts`. */ export class VisibilityParseError extends Error { - constructor(expr: string, detail: string) { - super(`Cannot parse visibility expression "${expr}": ${detail}`); + constructor( + /** The unwrapped predicate source that failed to parse. */ + readonly source: string, + /** Why it failed, without the surrounding sentence — for embedding in a caller's own message. */ + readonly detail: string, + ) { + super(`Cannot parse visibility expression "${source}": ${detail}`); this.name = 'VisibilityParseError'; } } type Token = - | { kind: 'punct'; value: '(' | ')' | '!' | '&&' | '||' | '===' | '!==' | '==' | '!=' } + | { + kind: 'punct'; + value: '(' | ')' | '!' | '&&' | '||' | '===' | '!==' | '==' | '!=' | '>=' | '<=' | '>' | '<'; + } | { kind: 'string'; value: string } | { kind: 'number'; value: number } | { kind: 'keyword'; value: boolean | null } | { kind: 'ref'; value: string }; +/** + * The comparison operators, longest-first so `>=` is never tokenized as `>` + * followed by a stray `=`. `!` is matched separately (after this list) for the + * same reason: `!==` and `!=` must win over the unary `!`. + */ +const COMPARISON_OPERATORS = ['===', '!==', '==', '!=', '>=', '<=', '>', '<'] as const; +type ComparisonOperator = (typeof COMPARISON_OPERATORS)[number]; + /** * Unwrap the manifest forms a `visible` field can take: a bare string, * a `${…}` template string, or a `{ dialect, source }` envelope. @@ -68,7 +106,7 @@ function tokenize(expr: string): Token[] { if (/\s/.test(ch)) { i++; continue; } if (ch === '(' || ch === ')') { tokens.push({ kind: 'punct', value: ch }); i++; continue; } let matchedOp = false; - for (const op of ['===', '!==', '==', '!=', '&&', '||'] as const) { + for (const op of [...COMPARISON_OPERATORS, '&&', '||'] as const) { if (expr.startsWith(op, i)) { tokens.push({ kind: 'punct', value: op }); i += op.length; @@ -150,12 +188,34 @@ export function evaluateVisibility(visible: unknown, data: Record 0` is `false` + // there and `false` here. + case '>': + return (left as number) > (right as number); + case '>=': + return (left as number) >= (right as number); + case '<': + return (left as number) < (right as number); + case '<=': + return (left as number) <= (right as number); + } } return left; }