diff --git a/.changeset/settings-select-options-enforced.md b/.changeset/settings-select-options-enforced.md new file mode 100644 index 0000000000..a9624f8484 --- /dev/null +++ b/.changeset/settings-select-options-enforced.md @@ -0,0 +1,53 @@ +--- +"@objectstack/service-settings": patch +--- + +fix(service-settings): a settings `select` now rejects values outside its declared `options` (#5131) + +`SettingsService.validatePatch` enforced two of the constraints a settings +manifest declares — `required` and `pattern` — and skipped the third. A +specifier's `options` table never took part in save-time validation, so any +string at all could be written into a dropdown field: + +```ts +await svc.setMany('mail', { provider: 'sendgrid', from_email: 'a@b.com' }); // stored +``` + +Going through the console this was unreachable: the dropdown only ever emits a +value from the table. But `PUT /api/settings/:ns` is an authorizable public +surface, and scripts, migration tools and AI-authored bootstrap code write it +directly — where the bad value was accepted, persisted and read back **in +silence**, leaving every consumer to improvise its own answer for an +enumeration member that does not exist. It was not `mail`-specific: +`storage.adapter`, `sms.provider`, `ai.provider`, `localization.date_format` and +every other `select` behaved the same way. + +This is the API-side gate that #5094 was missing. That change retired +`sendgrid` / `ses` from the `mail` provider table because this server cannot +deliver through them — with no write-side enforcement, the values it had just +retired could be written straight back in the same afternoon. + +**Now:** a `select` / `radio` / `multiselect` value that is not a member of the +declared table is rejected with a `FieldError` whose `code` is `invalid_option` +and whose `constraint` carries the allowed set (`{ allowed: 'smtp, resend, +postmark, log' }`), so a client composes its own message instead of parsing +ours. The enforced set is the spec's own: `SpecifierSchema` already *requires* a +non-empty `options` on exactly those three types, so declared and enforced name +one list rather than two that can drift. + +Two deliberate limits keep this from breaking workspaces that already carry +drift: + +- **The check is gated on TOUCH**, like `required` and `pattern` before it. A + value that pre-dates the current option table only fails the patch that + writes that key — editing `from_name` is not rejected because a stale + `provider` sits in the store. The opposite rule would lock every workspace + with historical drift out of its own settings page entirely, which is worse + than the gap being closed. Resets (all-null patches) are never blocked. +- **A specifier that declares no option table is left alone.** It cannot say + what is legal, so it stays lenient rather than rejecting every write. + +Values are compared in string form, so an option declared `value: 30` still +matches after a round trip through JSON or a form post. There is no opt-out: a +manifest that needs to accept custom values would declare that explicitly in +the spec, not rely on a tolerant consumer. diff --git a/packages/services/service-settings/src/envelope.conformance.test.ts b/packages/services/service-settings/src/envelope.conformance.test.ts index 44ad37a3af..4c65409f47 100644 --- a/packages/services/service-settings/src/envelope.conformance.test.ts +++ b/packages/services/service-settings/src/envelope.conformance.test.ts @@ -430,6 +430,33 @@ describe('settings envelope (#4224) — SETTINGS_VALIDATION speaks the field-lev expect(fields.map((f: any) => f.code).sort()).toEqual(['invalid_format', 'required']); }); + it('an out-of-table select reaches the client as a parseable invalid_option (#5131)', async () => { + const { http, service } = mount(); + service.registerManifest({ + namespace: 'enumerated', + label: 'Enumerated', + writePermission: 'setup.write', + readPermission: 'setup.access', + specifiers: [ + { key: 'provider', type: 'select', label: 'Provider', + options: [{ value: 'smtp', label: 'SMTP' }, { value: 'log', label: 'Log' }] }, + ], + } as any); + const { status, body } = await drive(http, 'PUT /api/settings/:namespace', { + params: { namespace: 'enumerated' }, + body: { provider: 'sendgrid' }, + }); + expect(status).toBe(400); + expect(body.error.code).toBe('SETTINGS_VALIDATION'); + + const [field] = body.error.details.fields; + expect(FieldErrorSchema.safeParse(field).success).toBe(true); + // The constraint kind is stamped where the check failed, so the route + // never has to infer it back out of the prose (ADR-0114). + expect(field.code).toBe('invalid_option'); + expect(field.constraint).toEqual({ allowed: 'smtp, log' }); + }); + it('the pre-#4224 map is gone from both of its old spellings', async () => { const http = lockedPattern(); const { body } = await drive(http, 'PUT /api/settings/:namespace', { diff --git a/packages/services/service-settings/src/settings-service.test.ts b/packages/services/service-settings/src/settings-service.test.ts index 05f0434486..1e4e43d5d3 100644 --- a/packages/services/service-settings/src/settings-service.test.ts +++ b/packages/services/service-settings/src/settings-service.test.ts @@ -413,6 +413,188 @@ describe('SettingsService — save-time validation (required/visible/pattern)', }); }); +/** + * #5131 — a manifest's `options` table is enforced at SAVE time. + * + * Until this suite existed the enumeration was a front-end convention: the + * console dropdown only ever emitted legal values, 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 any string at all into a `select` and have it + * stored, read back, and improvised over by each consumer in turn. + * + * The load-bearing case is `mail.provider`: #5094/#5133 retired `sendgrid` + * and `ses` from the option table because this server cannot deliver through + * them, and that manifest-side tightening had no matching gate on the API + * side — the very values just retired could be written straight back in. + */ +describe('SettingsService — save-time validation (declared options are enforced)', () => { + const mailService = () => { + const svc = new SettingsService({ env: {} }); + svc.registerManifest(mailSettingsManifest); + return svc; + }; + + it('refuses a provider outside the declared table, naming the allowed set', async () => { + const svc = mailService(); + // `sendgrid` left the table in #5094; before this gate it could be written + // back the same afternoon it was retired. + await expect( + svc.setMany('mail', { provider: 'sendgrid', from_email: 'a@b.com' }), + ).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [ + { + field: 'provider', + code: 'invalid_option', + label: 'Provider', + // The allowed set travels as a discrete constraint (ADR-0114), so a + // client branches on the machine value instead of parsing our prose. + constraint: { allowed: 'smtp, resend, postmark, log' }, + value: 'sendgrid', + }, + ], + }); + // Atomic: the rejected batch persisted nothing, not even the legal key. + expect((await svc.get('mail', 'provider')).source).toBe('default'); + expect((await svc.get('mail', 'from_email')).value).toBeNull(); + }); + + it('accepts every value the table does declare', async () => { + for (const [provider, extra] of [ + ['smtp', { smtp_host: 'smtp.example.com' }], + ['resend', { api_key: 're-key' }], + ['postmark', { api_key: 'pm-key' }], + ['log', {}], + ] as const) { + const svc = mailService(); + await expect( + svc.setMany('mail', { provider, ...extra, from_email: 'a@b.com' }), + ).resolves.toBeDefined(); + expect((await svc.get('mail', 'provider')).value).toBe(provider); + } + }); + + it('checks the option table only when the patch TOUCHES the key', async () => { + // A workspace that saved `sendgrid` while the option existed still carries + // it. Simulated exactly as it happened: write under the OLD table, then + // re-register the narrowed manifest (#5094) over the same namespace. + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + ...mailSettingsManifest, + specifiers: mailSettingsManifest.specifiers.map((s: any) => + s.key === 'provider' + ? { ...s, options: [...s.options, { value: 'sendgrid', label: 'SendGrid' }] } + : s, + ), + } as any); + await svc.setMany('mail', { provider: 'sendgrid', api_key: 'sg-key', from_email: 'a@b.com' }); + svc.registerManifest(mailSettingsManifest); + + // The stale value is still there … + expect((await svc.get('mail', 'provider')).value).toBe('sendgrid'); + // … and it does NOT lock the workspace out of editing anything else. A + // patch that never mentions `provider` is not rejected on its account — + // the opposite rule would make the settings page unusable for every + // workspace carrying historical drift, which is worse than the gap. + await expect(svc.setMany('mail', { from_name: 'Acme Ops' })).resolves.toBeDefined(); + expect((await svc.get('mail', 'from_name')).value).toBe('Acme Ops'); + // Only re-writing the key itself is refused. + await expect(svc.setMany('mail', { provider: 'sendgrid' })).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [{ field: 'provider', code: 'invalid_option' }], + }); + // And a reset still clears it — an all-null patch is never blocked. + await expect(svc.resetNamespace('mail')).resolves.toBeGreaterThan(0); + }); + + it('leaves the value alone when the specifier declares no option table', async () => { + // `registerManifest` takes manifests as given (no Zod pass), so a + // hand-built select without `options` reaches the validator. It cannot say + // what is legal, so it stays lenient rather than rejecting every write. + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'freeform', + label: 'Freeform', + specifiers: [{ type: 'select', key: 'mode', label: 'Mode' }], + } as any); + await expect(svc.setMany('freeform', { mode: 'whatever' })).resolves.toBeDefined(); + }); + + it('enforces radio and multiselect from the same table, element-wise', async () => { + // All three types are covered because the SPEC requires an `options` table + // on all three; `radio`/`multiselect` have no producer manifest today and + // would otherwise be a hole the first one to author them falls into. + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'shapes', + label: 'Shapes', + specifiers: [ + { type: 'radio', key: 'tier', label: 'Tier', + options: [{ value: 'free', label: 'Free' }, { value: 'pro', label: 'Pro' }] }, + { type: 'multiselect', key: 'channels', label: 'Channels', + options: [{ value: 'email', label: 'Email' }, { value: 'sms', label: 'SMS' }] }, + ], + } as any); + + await expect(svc.setMany('shapes', { tier: 'enterprise' })).rejects.toMatchObject({ + fields: [{ field: 'tier', code: 'invalid_option', constraint: { allowed: 'free, pro' } }], + }); + await expect(svc.setMany('shapes', { tier: 'pro' })).resolves.toBeDefined(); + + // Every element is checked, and the rejected one is the one reported. + await expect( + svc.setMany('shapes', { channels: ['email', 'carrier-pigeon'] }), + ).rejects.toMatchObject({ + fields: [{ field: 'channels', code: 'invalid_option', value: 'carrier-pigeon' }], + }); + await expect(svc.setMany('shapes', { channels: ['email', 'sms'] })).resolves.toBeDefined(); + await expect(svc.setMany('shapes', { channels: [] })).resolves.toBeDefined(); + }); + + it('matches option values by string form, so a number option survives JSON', async () => { + // A stored value has been through JSON and, over REST, a form post: an + // option declared `value: 30` legitimately reads back as '30'. Rejecting + // that would enforce the transport rather than the enumeration. + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'retention', + label: 'Retention', + specifiers: [ + { type: 'select', key: 'days', label: 'Days', + options: [{ value: 7, label: '7' }, { value: 30, label: '30' }] }, + { type: 'select', key: 'archive', label: 'Archive', + options: [{ value: true, label: 'On' }, { value: false, label: 'Off' }] }, + ], + } as any); + await expect(svc.setMany('retention', { days: 30 })).resolves.toBeDefined(); + await expect(svc.setMany('retention', { days: '30' })).resolves.toBeDefined(); + await expect(svc.setMany('retention', { archive: false })).resolves.toBeDefined(); + await expect(svc.setMany('retention', { days: 45 })).rejects.toMatchObject({ + fields: [{ field: 'days', code: 'invalid_option', constraint: { allowed: '7, 30' } }], + }); + }); + + it('never echoes the rejected value for an encrypted specifier', async () => { + // `encrypted` is authorable on any specifier, and this message lands in + // logs — so the offending value is named only where it is safe to name. + const svc = new SettingsService({ env: {} }); + svc.registerManifest({ + namespace: 'vault', + label: 'Vault', + specifiers: [ + { type: 'select', key: 'key_ref', label: 'Key', encrypted: true, + options: [{ value: 'primary', label: 'Primary' }] }, + ], + } as any); + const err = await svc.setMany('vault', { key_ref: 's3cr3t-handle' }).catch((e) => e); + expect(err.code).toBe('SETTINGS_VALIDATION'); + expect(err.fields[0]).toMatchObject({ field: 'key_ref', code: 'invalid_option' }); + expect(err.fields[0].value).toBeUndefined(); + expect(err.message).not.toContain('s3cr3t-handle'); + }); +}); + 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 1a5f07913f..2e6c0dff19 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -46,6 +46,43 @@ const LAYOUT_ONLY_TYPES = new Set([ 'action_button', ]); +/** + * Specifier types whose stored value must be a member of the declared + * `options` table. + * + * THE list is the spec's, not a judgement call made here: `SpecifierSchema`'s + * superRefine (`settings-manifest.zod.ts`) rejects a manifest that authors one + * of exactly these three types without a non-empty `options`. So "the types + * that must declare an option table" and "the types whose value is checked + * against it" name the same set — declared IS enforced, with no third list to + * drift. `radio` and `multiselect` have no producer manifest today; they are + * covered anyway because the alternative is that the first manifest to author + * one silently re-opens this exact hole. + */ +const OPTION_BEARING_TYPES = new Set(['select', 'radio', 'multiselect']); + +/** + * The declared option values, in string form. + * + * String form because a stored value has been through JSON (and, over the REST + * boundary, a form post): an option declared `value: 30` is legitimately read + * back as `'30'`, and rejecting that would be enforcing the transport rather + * than the enumeration. Same rule the record validator applies to + * `select`/`multiselect` field options (objectui#2729). + */ +function declaredOptionValues(options: unknown): string[] { + if (!Array.isArray(options)) return []; + const out: string[] = []; + for (const opt of options) { + if (!opt || typeof opt !== 'object') continue; + const v = (opt as Record).value; + if (typeof v === 'string' || typeof v === 'number' || typeof v === 'boolean') { + out.push(String(v)); + } + } + return out; +} + interface RegisteredManifest { manifest: SettingsManifest; /** Resolved specifier scopes for fast lookup. */ @@ -613,8 +650,19 @@ export class SettingsService { * is (switching provider must validate that provider's fields). * - `required` + visible + empty → rejected. * - `pattern` (text fields) + non-empty value that mismatches → rejected. + * - `options` (`select`/`radio`/`multiselect`) + non-empty value outside + * the declared table → rejected (`invalid_option`). * - All-null patches (namespace reset) and unparseable visibility * expressions skip validation rather than block the write. + * + * 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 + * option table (a `mail.provider` of `sendgrid`, retired in #5094) only + * fails the patch that writes that key. A patch changing `from_name` + * 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. */ private async validatePatch( namespace: string, @@ -679,6 +727,58 @@ export class SettingsService { }); continue; } + + // A `select`/`radio`/`multiselect` value must be a member of the option + // table the manifest declares. Until this check existed the `options` + // list was a front-end convention only — the console dropdown emitted + // legal values, but `PUT /api/settings/:ns` accepted any string at all, + // so a script, a migration or AI-authored bootstrap code could write + // `provider: 'sendgrid'` into a namespace that has no such provider and + // the write would succeed silently, leaving each consumer to improvise. + if (!empty && OPTION_BEARING_TYPES.has(type)) { + const allowed = declaredOptionValues(spec.options); + // A manifest with no option table cannot say what is legal. The spec + // refuses that shape at parse time, but `registerManifest` takes + // manifests as given (no Zod pass), so skip rather than reject every + // write to a hand-built manifest — same leniency the unparseable + // `visible` and invalid `pattern` branches already take. + if (allowed.length > 0) { + // `multiselect` stores an array, `select`/`radio` a scalar; both are + // checked element-wise against the one table. A scalar arriving at a + // multiselect is wrapped rather than rejected — policing the value's + // SHAPE is a different constraint (`invalid_type`) with a different + // owner, and inventing it here would reject writes this change was + // never asked to touch. + const picked = Array.isArray(value) ? value : [value]; + // `findIndex`, not `find`: a `find` returning `undefined` cannot say + // whether nothing was rejected or whether the rejected element WAS + // `undefined` — and the latter would slip through the check. + const at = picked.findIndex((v) => !allowed.includes(String(v))); + if (at !== -1) { + const offending = picked[at]; + // An option value is not a secret, but `encrypted` is authorable on + // any specifier — so never echo the rejected value for a key whose + // contents are held encrypted, in a message that lands in logs. + const secret = reg.encryptedKeys.has(key); + const got = secret ? '' : ` Received '${String(offending)}'.`; + errors.push({ + field: key, + code: 'invalid_option', + message: `${label} must be one of: ${allowed.join(', ')}.${got}`, + label, + // The allowed set as a discrete constraint, so a client composes + // its own sentence instead of parsing ours (`FieldError. + // constraint`, ADR-0114). Key and comma-joined form are the ones + // the spec's own `{ allowed: 'draft, sent' }` example documents + // and the record validator already emits for this same code. + constraint: { allowed: allowed.join(', ') }, + ...(secret ? {} : { value: String(offending) }), + }); + continue; + } + } + } + if (!empty && typeof spec.pattern === 'string' && typeof value === 'string') { let re: RegExp | undefined; try { diff --git a/packages/services/service-settings/src/settings-service.types.ts b/packages/services/service-settings/src/settings-service.types.ts index c2f851d34d..1ac690795e 100644 --- a/packages/services/service-settings/src/settings-service.types.ts +++ b/packages/services/service-settings/src/settings-service.types.ts @@ -268,8 +268,11 @@ export class SettingsForbiddenError extends Error { /** * Thrown when a write would leave the namespace in an invalid state — * a `required` field that is visible under the post-write values is - * empty (e.g. provider=cloudflare saved without an API key), or a value - * that does not match its specifier's declared `pattern`. The whole + * empty (e.g. provider=cloudflare saved without an API key), a value + * that does not match its specifier's declared `pattern`, or a value a + * `select`/`radio`/`multiselect` specifier does not list in its declared + * `options` (#5131 — those enumerations used to be a front-end + * convention that the write path never checked). The whole * batch is rejected; `fields` carries one entry per offending key, which * the UI can render inline against the input it addresses. * @@ -277,8 +280,8 @@ export class SettingsForbiddenError extends Error { * (#3977) — rather than the `Record` map it was until #4224. * The map predated that catalog and named the constraint only in prose, so * a consumer could render the sentence but not branch on *which* constraint - * failed; `code` (`required` / `invalid_format`) now says it in the one - * spelling every other validator in the platform uses. `label` and + * failed; `code` (`required` / `invalid_format` / `invalid_option`) now + * says it in the one spelling every other validator in the platform uses. `label` and * `constraint` carry what the message interpolates, so a form can compose * its own text instead of parsing ours. */