fix(service-settings): 保存期强制 select 声明的 options —— 声明即强制 (#5131)

.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.

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', {

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: {} });

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<string, unknown>).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 {