From 84a1c62e93689c5bfc5847ddec8c904c0ad862e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 00:35:01 +0000 Subject: [PATCH 1/3] feat(service-settings)!: refuse a settings value that misses the format its type declares, at both doors The save door answers one FieldError per key with the code the format verdict carries (invalid_email / invalid_url / invalid_phone / invalid_format) through the existing SETTINGS_VALIDATION envelope, and stores nothing. The env door rejects a malformed OS_* override through reportRejectedEnvOverride: not in force, reported once, never printing the value. The pin file is re-pointed from warning to refusal. Claude-Session: https://claude.ai/code/session_01UmqzD97e14iws6hmVhgZ5y Co-authored-by: Claude --- .../src/settings-format-refusal.test.ts | 477 ++++++++++++++++++ .../src/settings-format-warning.test.ts | 312 ------------ .../service-settings/src/settings-service.ts | 184 +++---- 3 files changed, 552 insertions(+), 421 deletions(-) create mode 100644 packages/services/service-settings/src/settings-format-refusal.test.ts delete mode 100644 packages/services/service-settings/src/settings-format-warning.test.ts diff --git a/packages/services/service-settings/src/settings-format-refusal.test.ts b/packages/services/service-settings/src/settings-format-refusal.test.ts new file mode 100644 index 00000000000..833536bb26e --- /dev/null +++ b/packages/services/service-settings/src/settings-format-refusal.test.ts @@ -0,0 +1,477 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #24882 — the format each specifier TYPE declares (ADR-0007: `email` / `url` / + * `phone` "format validation", `color` "hex string") is judged at both doors + * and REFUSED. + * + * Refusing these values narrows what the settings door accepted, and no + * ADR-0087 conversion can carry that: stored values are runtime state and an + * `OS_*` override is a deployment's environment. Under the standing rule 「甲」 + * as the maintainer amended it, such a narrowing ships now, as a breaking + * minor. These pins hold each part of it: + * + * - SAVE DOOR: a malformed value answers `400 SETTINGS_VALIDATION` with one + * `FieldError` naming the key and the ADR-0114 code the verdict carries + * (`invalid_email` / `invalid_url` / `invalid_phone` / `invalid_format`), and + * nothing is stored. A well-formed value saves. + * - ENV DOOR: a malformed override is rejected the way every other family's + * miss is — not in force, reported once at `error`, never printing the + * value — and the key resolves from the stored value, else the default. A + * well-formed override is in force. + * - STORED ROWS: a value stored before the check existed is still served by + * every read, and it does not block a save that does not touch its key. + * + * The save door is driven through the real route (`registerSettingsRoutes` + * over a real `SettingsService`, as `settings-routes.test.ts` does); the env + * door through the service's own read and write paths, which is where an + * override takes effect. Shipped manifests are used wherever a shipped key has + * the type; `phone`, a secret-bearing key and a `visible`-gated key are + * synthetic because no shipped manifest authors them, and each fixture is + * parsed against `SettingsManifestSchema` below so it pins a shape an author + * can write. + */ + +import { describe, expect, it, vi } from 'vitest'; +import type { IHttpServer, IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; +import { FieldErrorSchema } from '@objectstack/spec/api'; +import { SettingsManifestSchema, type SettingsManifest } from '@objectstack/spec/system'; +import { SettingsService } from './settings-service.js'; +import { registerSettingsRoutes } from './settings-routes.js'; +import { brandingSettingsManifest } from './manifests/branding.manifest.js'; +import { companySettingsManifest } from './manifests/company.manifest.js'; +import { mailSettingsManifest } from './manifests/mail.manifest.js'; +import { LocalCryptoProvider } from './local-crypto-provider.js'; + +class MockHttp implements IHttpServer { + routes = new Map(); + private add(method: string, path: string, handler: RouteHandler) { + this.routes.set(`${method} ${path}`, handler); + } + get(path: string, h: RouteHandler) { this.add('GET', path, h); return this as any; } + post(path: string, h: RouteHandler) { this.add('POST', path, h); return this as any; } + put(path: string, h: RouteHandler) { this.add('PUT', path, h); return this as any; } + delete(path: string, h: RouteHandler) { this.add('DELETE', path, h); return this as any; } + patch(path: string, h: RouteHandler) { this.add('PATCH', path, h); return this as any; } + use() { return this as any; } + listen() { return Promise.resolve(); } + close() { return Promise.resolve(); } + getInstance() { return null; } +} + +/** A diagnostics sink that records both levels, so a stray `warn` would be seen too. */ +function spyLogger() { + const warns: string[] = []; + const errors: string[] = []; + return { + warns, + errors, + logger: { + warn: (m: string) => void warns.push(m), + error: (m: string) => void errors.push(m), + }, + }; +} + +/** Synthetic: a `phone` specifier (no shipped manifest authors one) and a secret-bearing `email`. */ +const formatLabManifest = { + namespace: 'format_lab', + version: 1, + label: 'Format Lab', + scope: 'global', + readPermission: 'setup.access', + writePermission: 'setup.write', + specifiers: [ + { type: 'phone', key: 'support_phone', label: 'Support phone', required: false }, + { type: 'email', key: 'billing_email', label: 'Billing email', required: false, encrypted: true }, + { type: 'url', key: 'docs_url', label: 'Docs URL', required: false, maxLength: 12 }, + ], +} as unknown as SettingsManifest; + +/** Synthetic: a format-typed key whose `visible` reads another key, so a save of that key TOUCHES it. */ +const hookLabManifest = { + namespace: 'hook_lab', + version: 1, + label: 'Hook Lab', + scope: 'global', + readPermission: 'setup.access', + writePermission: 'setup.write', + specifiers: [ + { type: 'select', key: 'mode', label: 'Mode', required: false, default: 'off', + options: [{ value: 'off', label: 'Off' }, { value: 'hook', label: 'Hook' }] }, + { type: 'url', key: 'callback_url', label: 'Callback URL', required: false, + visible: "${data.mode === 'hook'}" }, + ], +} as unknown as SettingsManifest; + +/** The value a secret-bearing pin must never see in a log line or an answer. */ +const SECRET_MALFORMED = 'billing-desk-at-corp-internal'; + +const ADMIN = () => ({ + enforced: true, + permissions: ['setup.access', 'setup.write', 'manage_platform_settings'], + userId: 'usr_admin', + organizationId: 'org_a', +}); + +function makeStack(env: Record = {}) { + const spy = spyLogger(); + const secretRows = new Map(); + const svc = new SettingsService({ + env, + logger: spy.logger, + // A confidentiality-providing crypto pair, so a write to the secret-bearing + // key is accepted by the fail-closed encryption gate and actually stored. + cryptoProvider: new LocalCryptoProvider(), + secretStore: { + async insert(row) { secretRows.set(row.id, row); return { id: row.id }; }, + async get(id) { return secretRows.get(id) ?? null; }, + async update(id, patch) { secretRows.set(id, { ...secretRows.get(id), ...patch }); }, + }, + }); + svc.registerManifest(brandingSettingsManifest); + svc.registerManifest(companySettingsManifest); + svc.registerManifest(mailSettingsManifest); + svc.registerManifest(formatLabManifest); + svc.registerManifest(hookLabManifest); + const http = new MockHttp(); + registerSettingsRoutes(http, svc, { contextFromRequest: ADMIN }); + return { svc, http, ...spy }; +} + +/** Drive one route; status starts at 0 so `200` is asserted, not defaulted. */ +async function call(http: MockHttp, route: string, namespace: string, body?: Record) { + const req: IHttpRequest = { params: { namespace }, query: {}, body, headers: {}, method: route.split(' ')[0], path: '/' }; + const state: { status: number; body?: any } = { status: 0 }; + const res: IHttpResponse = { + json: vi.fn((data) => { state.body = data; }) as any, + send: vi.fn() as any, + status: vi.fn((code: number) => { state.status = code; return res; }) as any, + header: vi.fn(() => res) as any, + }; + await http.routes.get(route)!(req, res); + return state; +} + +/** `PUT /api/settings/:namespace` through the real route. */ +const put = (http: MockHttp, namespace: string, body: Record) => + call(http, 'PUT /api/settings/:namespace', namespace, body); + +/** One malformed and one well-formed value per type, each on a key whose type declares it. */ +const PER_TYPE = [ + { type: 'url', code: 'invalid_url', ns: 'branding', key: 'logo_url', bad: 'not a url', good: 'https://cdn.example.com/logo.svg' }, + { type: 'url', code: 'invalid_url', ns: 'company', key: 'website', bad: 'example dot com', good: 'https://example.com' }, + { type: 'color', code: 'invalid_format', ns: 'branding', key: 'accent_color', bad: 'e11d48', good: '#e11d48' }, + { type: 'email', code: 'invalid_email', ns: 'company', key: 'primary_contact_email', bad: 'ops at example.com', good: 'ops@example.com' }, + { type: 'email', code: 'invalid_email', ns: 'mail', key: 'from_email', bad: 'no-reply@localhost', good: 'no-reply@example.com' }, + { type: 'phone', code: 'invalid_phone', ns: 'format_lab', key: 'support_phone', bad: 'call the desk', good: '+1 415 555 0100' }, +] as const; + +/** A key in the same namespace, with no declared format, that a save can write instead. */ +const OTHER_KEY: Record = { + branding: { key: 'workspace_name', value: 'Acme' }, + company: { key: 'legal_name', value: 'Acme Ltd' }, + mail: { key: 'from_name', value: 'Acme Ops' }, + format_lab: { key: 'docs_url', value: '/docs' }, +}; + +/** The manifest a namespace is registered with in {@link makeStack}. */ +const MANIFEST: Record = { + branding: brandingSettingsManifest, + company: companySettingsManifest, + mail: mailSettingsManifest, + format_lab: formatLabManifest, + hook_lab: hookLabManifest, +}; + +/** + * Store `value` under `key` the way a row stored before this check existed got + * there: save it while the key is declared `text` (no format), then register + * the real manifest over the namespace again — the method the option-table and + * value-domain TOUCH pins in `settings-service.test.ts` use. + */ +async function storeUnjudged(stack: ReturnType, ns: string, patch: Record) { + const real = MANIFEST[ns]; + const FORMAT_TYPES = new Set(['email', 'url', 'phone', 'color']); + stack.svc.registerManifest({ + ...real, + specifiers: (real.specifiers as Array>).map((s) => + Object.keys(patch).includes(String(s.key)) && FORMAT_TYPES.has(String(s.type)) ? { ...s, type: 'text' } : s), + } as unknown as SettingsManifest); + const seeded = await put(stack.http, ns, patch); + expect(seeded.status, JSON.stringify(seeded.body)).toBe(200); + stack.svc.registerManifest(real); +} + +describe('synthetic fixtures are a spec-valid authoring surface', () => { + it('format_lab and hook_lab parse under SettingsManifestSchema', () => { + // A fixture spelling a key the schema rejects would pin behaviour no author + // can reach. + expect(() => SettingsManifestSchema.parse(formatLabManifest)).not.toThrow(); + expect(() => SettingsManifestSchema.parse(hookLabManifest)).not.toThrow(); + }); +}); + +describe('save door — a format miss is refused with one FieldError, and nothing is stored (#24882)', () => { + for (const c of PER_TYPE) { + it(`${c.ns}.${c.key} (${c.type}): a malformed value answers 400 SETTINGS_VALIDATION with one ${c.code}, and is not stored`, async () => { + const { svc, http, warns, errors } = makeStack(); + const r = await put(http, c.ns, { [c.key]: c.bad }); + + expect(r.status).toBe(400); + expect(r.body.success).toBe(false); + expect(r.body.error.code).toBe('SETTINGS_VALIDATION'); + expect(r.body.error.details.namespace).toBe(c.ns); + const fields = r.body.error.details.fields; + expect(fields).toHaveLength(1); + expect(FieldErrorSchema.safeParse(fields[0]).success).toBe(true); + expect(fields[0]).toMatchObject({ field: c.key, code: c.code, constraint: { type: c.type } }); + // The answer names what to send, never what was sent. + expect(fields[0]).not.toHaveProperty('value'); + expect(JSON.stringify(r.body)).not.toContain(c.bad); + + // Nothing stored: the key still resolves from its default. + const after = await svc.get(c.ns, c.key, ADMIN()); + expect(after.source).toBe('default'); + expect(after.value).not.toBe(c.bad); + // A refusal handed to the caller is not a degradation: no log line. + expect(warns).toEqual([]); + expect(errors).toEqual([]); + }); + + it(`${c.ns}.${c.key} (${c.type}): a well-formed value saves`, async () => { + const { svc, http, warns, errors } = makeStack(); + const r = await put(http, c.ns, { [c.key]: c.good }); + expect(r.status).toBe(200); + expect(r.body.data.values[c.key].value).toBe(c.good); + const stored = await svc.get(c.ns, c.key, ADMIN()); + expect(stored.value).toBe(c.good); + expect(stored.source).not.toBe('default'); + expect(warns).toEqual([]); + expect(errors).toEqual([]); + }); + } + + it('the other shapes each grammar admits save too', async () => { + // The readers' own contracts: `logoUrl` is "absolute or relative", storage + // hands back a root-relative path for an upload, and CSS hex colours come + // in four lengths. + const { http } = makeStack(); + for (const logo of ['/api/v1/storage/files/abc123', 'data:image/svg+xml;base64,PHN2Zy8+', 'http://intranet.local/logo.png']) { + expect((await put(http, 'branding', { logo_url: logo })).status).toBe(200); + } + for (const colour of ['#abc', '#abcd', '#AABBCC', '#aabbccdd']) { + expect((await put(http, 'branding', { accent_color: colour })).status).toBe(200); + } + // Surrounding whitespace is trimmed by both live readers before use, so it + // is not a format miss; inner whitespace still is (the per-type cases above). + expect((await put(http, 'company', { primary_contact_email: ' ops@example.com ' })).status).toBe(200); + }); + + it('an empty or blank value is not a format miss', async () => { + const { http } = makeStack(); + expect((await put(http, 'branding', { logo_url: '' })).status).toBe(200); + expect((await put(http, 'branding', { logo_url: ' ' })).status).toBe(200); + }); + + it('a secret-bearing key: the malformed save is refused, and the answer never carries the value', async () => { + const { svc, http, warns, errors } = makeStack(); + const r = await put(http, 'format_lab', { billing_email: SECRET_MALFORMED }); + expect(r.status).toBe(400); + expect(r.body.error.code).toBe('SETTINGS_VALIDATION'); + expect(r.body.error.details.fields).toHaveLength(1); + expect(r.body.error.details.fields[0]).toMatchObject({ field: 'billing_email', code: 'invalid_email' }); + const answer = JSON.stringify(r.body); + expect(answer).not.toContain(SECRET_MALFORMED); + // Not a fragment of it either. + expect(answer).not.toContain('billing-desk'); + expect(answer).not.toContain('corp-internal'); + expect((await svc.get('format_lab', 'billing_email', ADMIN())).source).toBe('default'); + expect([...warns, ...errors]).toEqual([]); + }); + + it('precedence: a value that breaks a declared bound is refused for the bound, once', async () => { + const { svc, http } = makeStack(); + // `docs_url` declares `maxLength: 12`: the length refusal keeps precedence + // over the format, and the one FieldError for the key is the length one. + const r = await put(http, 'format_lab', { docs_url: 'not a url at all' }); + expect(r.status).toBe(400); + expect(r.body.error.code).toBe('SETTINGS_VALIDATION'); + expect(r.body.error.details.fields).toHaveLength(1); + expect(r.body.error.details.fields[0]).toMatchObject({ field: 'docs_url', code: 'max_length' }); + // Each key speaks once, for its own first breach; the batch stores nothing. + const r2 = await put(http, 'format_lab', { docs_url: 'not a url at all', support_phone: 'call the desk' }); + expect(r2.status).toBe(400); + expect(r2.body.error.details.fields.map((f: { field: string; code: string }) => [f.field, f.code])).toEqual([ + ['support_phone', 'invalid_phone'], + ['docs_url', 'max_length'], + ]); + expect((await svc.get('format_lab', 'support_phone', ADMIN())).source).toBe('default'); + expect((await svc.get('format_lab', 'docs_url', ADMIN())).source).toBe('default'); + }); + + it('a batch with one malformed key is refused whole: its well-formed neighbour is not stored', async () => { + const { svc, http } = makeStack(); + const r = await put(http, 'branding', { workspace_name: 'Acme', logo_url: 'not a url' }); + expect(r.status).toBe(400); + expect(r.body.error.details.fields.map((f: { field: string }) => f.field)).toEqual(['logo_url']); + expect((await svc.get('branding', 'workspace_name', ADMIN())).source).toBe('default'); + }); +}); + +describe('env door — a format-missing override is rejected: not in force, reported once, never the value (#24882)', () => { + for (const c of PER_TYPE) { + const envName = `OS_${c.ns}_${c.key}`.toUpperCase(); + + it(`${envName} (${c.type}): a malformed override is not in force, falls back to the default, and is reported once`, async () => { + const { svc, http, warns, errors } = makeStack({ [envName]: c.bad }); + // Reported at boot — `registerManifest` audits the overrides — not on a + // first read that may never come. + expect(errors).toHaveLength(1); + + // Not in force: no stored value yet, so the default answers. + const r = await svc.get(c.ns, c.key, ADMIN()); + expect(r.source).toBe('default'); + expect(r.value).not.toBe(c.bad); + expect(r.locked).toBe(false); + expect(r.cascadeChain.some((e) => e.scope === 'env')).toBe(false); + // Re-reads and the namespace payload do not repeat the line. + await svc.get(c.ns, c.key, ADMIN()); + await svc.getNamespace(c.ns, ADMIN()); + expect((await call(http, 'GET /api/settings/:namespace', c.ns)).body.data.values[c.key].source).toBe('default'); + + expect(errors).toHaveLength(1); + expect(errors[0]).toContain(envName); + expect(errors[0]).toContain(`'${c.ns}.${c.key}'`); + expect(errors[0]).toContain(`type '${c.type}'`); + expect(errors[0]).toContain('IGNORED'); + // An environment variable's value never reaches the log line. + expect(errors[0]).not.toContain(c.bad); + expect(warns).toEqual([]); + + // Not locking either: the key is configurable through the settings door, + // and a stored value now answers in the override's place. + const w = await put(http, c.ns, { [c.key]: c.good }); + expect(w.status).toBe(200); + const stored = await svc.get(c.ns, c.key, ADMIN()); + expect(stored.value).toBe(c.good); + expect(stored.source).not.toBe('env'); + expect(stored.source).not.toBe('default'); + expect(errors).toHaveLength(1); + }); + + it(`${envName} (${c.type}): a well-formed override is in force and locks the key`, async () => { + const { svc, http, warns, errors } = makeStack({ [envName]: c.good }); + const r = await svc.get(c.ns, c.key, ADMIN()); + expect(r.value).toBe(c.good); + expect(r.source).toBe('env'); + expect(r.locked).toBe(true); + const w = await put(http, c.ns, { [c.key]: c.good }); + expect(w.status).toBe(409); + expect(w.body.error.code).toBe('SETTINGS_LOCKED'); + expect(warns).toEqual([]); + expect(errors).toEqual([]); + }); + } + + it('a malformed override set over a stored value: the stored value answers', async () => { + // `env` is read live (it may be `process.env`), so an override can arrive + // after a value was saved — the deployment that sets a bad variable on a + // workspace that already configured the key. + const env: Record = {}; + const { svc, http, errors } = makeStack(env); + expect((await put(http, 'branding', { logo_url: 'https://cdn.example.com/logo.svg' })).status).toBe(200); + env.OS_BRANDING_LOGO_URL = 'not a url'; + + const r = await svc.get('branding', 'logo_url', ADMIN()); + expect(r.value).toBe('https://cdn.example.com/logo.svg'); + expect(r.source).toBe('tenant'); + expect(r.locked).toBe(false); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('OS_BRANDING_LOGO_URL'); + expect(errors[0]).not.toContain('not a url'); + }); + + it('a secret-bearing key: the override is rejected, and its line never carries the value', async () => { + const { svc, warns, errors } = makeStack({ OS_FORMAT_LAB_BILLING_EMAIL: SECRET_MALFORMED }); + const r = await svc.get('format_lab', 'billing_email', ADMIN()); + expect(r.source).toBe('default'); + expect(r.locked).toBe(false); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('OS_FORMAT_LAB_BILLING_EMAIL'); + expect(errors[0]).toContain("'format_lab.billing_email'"); + expect(errors[0]).toContain("type 'email'"); + expect(errors[0]).not.toContain(SECRET_MALFORMED); + expect(errors[0]).not.toContain('billing-desk'); + expect(errors[0]).not.toContain('corp-internal'); + expect(warns).toEqual([]); + }); + + it('precedence: an override that breaks a declared bound is rejected for the bound, once', async () => { + // `docs_url` declares `maxLength: 12`; a 16-character override is ignored + // for its length, the family the save door names first too. + const { svc, warns, errors } = makeStack({ OS_FORMAT_LAB_DOCS_URL: 'not a url at all' }); + const r = await svc.get('format_lab', 'docs_url', ADMIN()); + expect(r.source).toBe('default'); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('IGNORED'); + expect(errors[0]).toContain('outside the declared length'); + expect(errors[0]).not.toContain("type 'url'"); + expect(warns).toEqual([]); + }); +}); + +describe('stored rows — a value stored before the check existed (#24882)', () => { + for (const c of PER_TYPE) { + const other = OTHER_KEY[c.ns]; + + it(`${c.ns}.${c.key} (${c.type}): a read still serves it, and a save of ${other.key} is not blocked by it`, async () => { + const stack = makeStack(); + await storeUnjudged(stack, c.ns, { [c.key]: c.bad }); + const { svc, http } = stack; + + // Reads judge nothing: the service and the route both serve it. + const read = await svc.get(c.ns, c.key, ADMIN()); + expect(read.value).toBe(c.bad); + expect(read.source).not.toBe('default'); + const viaRoute = await call(http, 'GET /api/settings/:namespace', c.ns); + expect(viaRoute.status).toBe(200); + expect(viaRoute.body.data.values[c.key].value).toBe(c.bad); + + // A save that does not touch the key is not refused on its account. + const w = await put(http, c.ns, { [other.key]: other.value }); + expect(w.status, JSON.stringify(w.body)).toBe(200); + expect((await svc.get(c.ns, other.key, ADMIN())).value).toBe(other.value); + expect((await svc.get(c.ns, c.key, ADMIN())).value).toBe(c.bad); + + // Only a save of the key itself is judged: the malformed value is + // refused, a well-formed one replaces it. + const again = await put(http, c.ns, { [c.key]: c.bad }); + expect(again.status).toBe(400); + expect(again.body.error.details.fields).toMatchObject([{ field: c.key, code: c.code }]); + expect((await put(http, c.ns, { [c.key]: c.good })).status).toBe(200); + expect((await svc.get(c.ns, c.key, ADMIN())).value).toBe(c.good); + }); + } + + it('a reset clears a stored malformed value: an all-null patch is never blocked', async () => { + const stack = makeStack(); + await storeUnjudged(stack, 'branding', { logo_url: 'not a url' }); + expect((await put(stack.http, 'branding', { logo_url: null })).status).toBe(200); + expect((await stack.svc.get('branding', 'logo_url', ADMIN())).source).toBe('default'); + }); + + it('a save TOUCHES a key through its `visible` predicate, and that judges the stored value', async () => { + // `validatePatch`'s TOUCH gate counts a key whose `visible` reads a patched + // key (switching a mode must validate the fields it reveals). No shipped + // format-typed key declares `visible`; this synthetic one does. + const stack = makeStack(); + await storeUnjudged(stack, 'hook_lab', { mode: 'hook', callback_url: 'not a url' }); + const { http } = stack; + const touched = await put(http, 'hook_lab', { mode: 'hook' }); + expect(touched.status).toBe(400); + expect(touched.body.error.details.fields).toMatchObject([{ field: 'callback_url', code: 'invalid_url' }]); + // Hidden by the patch, the key is not judged (hidden specifiers never are). + expect((await put(http, 'hook_lab', { mode: 'off' })).status).toBe(200); + }); +}); diff --git a/packages/services/service-settings/src/settings-format-warning.test.ts b/packages/services/service-settings/src/settings-format-warning.test.ts deleted file mode 100644 index 2a112a02328..00000000000 --- a/packages/services/service-settings/src/settings-format-warning.test.ts +++ /dev/null @@ -1,312 +0,0 @@ -// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. - -/** - * #24882 — the format each specifier TYPE declares (ADR-0007: `email` / `url` / - * `phone` "format validation", `color` "hex string") is judged at both doors - * and, under the standing rule 「甲」, REPORTED rather than refused. - * - * Refusing these values narrows what the settings door has always accepted, - * and no ADR-0087 conversion can carry that: stored values are runtime state - * and an `OS_*` override is a deployment's environment. So this major version - * ships only the loud half, and these pins hold both of its halves: - * - * - ACCEPTED, unchanged: a malformed save still answers `200` and the value is - * stored and served as written; a malformed override is still in force, - * still served with `source: 'env'` and still locks the key. - * - REPORTED: exactly one `warn` line, naming namespace, key and type — and - * never the value, for any key, so a secret-bearing key's value cannot reach - * a log line through it. - * - * The save door is driven through the real route (`registerSettingsRoutes` - * over a real `SettingsService`, as `settings-routes.test.ts` does); the env - * door through the service's own read and write paths, which is where an - * override takes effect. Shipped manifests are used wherever a shipped key has - * the type; `phone` and a secret-bearing key are synthetic because no shipped - * manifest authors either, and both fixtures are parsed against - * `SettingsManifestSchema` below so they pin a shape an author can write. - */ - -import { describe, expect, it, vi } from 'vitest'; -import type { IHttpServer, IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; -import { SettingsManifestSchema, type SettingsManifest } from '@objectstack/spec/system'; -import { SettingsService } from './settings-service.js'; -import { registerSettingsRoutes } from './settings-routes.js'; -import { brandingSettingsManifest } from './manifests/branding.manifest.js'; -import { companySettingsManifest } from './manifests/company.manifest.js'; -import { mailSettingsManifest } from './manifests/mail.manifest.js'; -import { LocalCryptoProvider } from './local-crypto-provider.js'; - -class MockHttp implements IHttpServer { - routes = new Map(); - private add(method: string, path: string, handler: RouteHandler) { - this.routes.set(`${method} ${path}`, handler); - } - get(path: string, h: RouteHandler) { this.add('GET', path, h); return this as any; } - post(path: string, h: RouteHandler) { this.add('POST', path, h); return this as any; } - put(path: string, h: RouteHandler) { this.add('PUT', path, h); return this as any; } - delete(path: string, h: RouteHandler) { this.add('DELETE', path, h); return this as any; } - patch(path: string, h: RouteHandler) { this.add('PATCH', path, h); return this as any; } - use() { return this as any; } - listen() { return Promise.resolve(); } - close() { return Promise.resolve(); } - getInstance() { return null; } -} - -/** A diagnostics sink that records both levels, so an `error` line would be seen too. */ -function spyLogger() { - const warns: string[] = []; - const errors: string[] = []; - return { - warns, - errors, - logger: { - warn: (m: string) => void warns.push(m), - error: (m: string) => void errors.push(m), - }, - }; -} - -/** Synthetic: a `phone` specifier (no shipped manifest authors one) and a secret-bearing `email`. */ -const formatLabManifest = { - namespace: 'format_lab', - version: 1, - label: 'Format Lab', - scope: 'global', - readPermission: 'setup.access', - writePermission: 'setup.write', - specifiers: [ - { type: 'phone', key: 'support_phone', label: 'Support phone', required: false }, - { type: 'email', key: 'billing_email', label: 'Billing email', required: false, encrypted: true }, - { type: 'url', key: 'docs_url', label: 'Docs URL', required: false, maxLength: 12 }, - ], -} as unknown as SettingsManifest; - -/** The value a secret-bearing pin must never see in a log line. */ -const SECRET_MALFORMED = 'billing-desk-at-corp-internal'; - -const ADMIN = () => ({ - enforced: true, - permissions: ['setup.access', 'setup.write', 'manage_platform_settings'], - userId: 'usr_admin', - organizationId: 'org_a', -}); - -function makeStack(env: Record = {}) { - const spy = spyLogger(); - const secretRows = new Map(); - const svc = new SettingsService({ - env, - logger: spy.logger, - // A confidentiality-providing crypto pair, so a write to the secret-bearing - // key is accepted by the fail-closed encryption gate and actually stored. - cryptoProvider: new LocalCryptoProvider(), - secretStore: { - async insert(row) { secretRows.set(row.id, row); return { id: row.id }; }, - async get(id) { return secretRows.get(id) ?? null; }, - async update(id, patch) { secretRows.set(id, { ...secretRows.get(id), ...patch }); }, - }, - }); - svc.registerManifest(brandingSettingsManifest); - svc.registerManifest(companySettingsManifest); - svc.registerManifest(mailSettingsManifest); - svc.registerManifest(formatLabManifest); - const http = new MockHttp(); - registerSettingsRoutes(http, svc, { contextFromRequest: ADMIN }); - return { svc, http, ...spy }; -} - -/** `PUT /api/settings/:namespace` through the real route; status starts at 0 so `200` is asserted, not defaulted. */ -async function put(http: MockHttp, namespace: string, body: Record) { - const req: IHttpRequest = { params: { namespace }, query: {}, body, headers: {}, method: 'PUT', path: '/' }; - const state: { status: number; body?: any } = { status: 0 }; - const res: IHttpResponse = { - json: vi.fn((data) => { state.body = data; }) as any, - send: vi.fn() as any, - status: vi.fn((code: number) => { state.status = code; return res; }) as any, - header: vi.fn(() => res) as any, - }; - await http.routes.get('PUT /api/settings/:namespace')!(req, res); - return state; -} - -/** One malformed and one well-formed value per type, each on a key whose type declares it. */ -const PER_TYPE = [ - { type: 'url', ns: 'branding', key: 'logo_url', bad: 'not a url', good: 'https://cdn.example.com/logo.svg' }, - { type: 'url', ns: 'company', key: 'website', bad: 'example dot com', good: 'https://example.com' }, - { type: 'color', ns: 'branding', key: 'accent_color', bad: 'e11d48', good: '#e11d48' }, - { type: 'email', ns: 'company', key: 'primary_contact_email', bad: 'ops at example.com', good: 'ops@example.com' }, - { type: 'email', ns: 'mail', key: 'from_email', bad: 'no-reply@localhost', good: 'no-reply@example.com' }, - { type: 'phone', ns: 'format_lab', key: 'support_phone', bad: 'call the desk', good: '+1 415 555 0100' }, -] as const; - -describe('synthetic fixture is a spec-valid authoring surface', () => { - it('format_lab parses under SettingsManifestSchema', () => { - // A fixture spelling a key the schema rejects would pin behaviour no author - // can reach. - expect(() => SettingsManifestSchema.parse(formatLabManifest)).not.toThrow(); - }); -}); - -describe('save door — a format miss is accepted and stored, and warned about once (#24882, 「甲」)', () => { - for (const c of PER_TYPE) { - it(`${c.ns}.${c.key} (${c.type}): a malformed value answers 200, is stored as written, and draws exactly one warning`, async () => { - const { svc, http, warns, errors } = makeStack(); - const r = await put(http, c.ns, { [c.key]: c.bad }); - - // ACCEPTED: no refusal anywhere — the status, the envelope and the store. - expect(r.status).toBe(200); - expect(r.body.success).toBe(true); - expect(r.body.error).toBeUndefined(); - expect(r.body.data.values[c.key].value).toBe(c.bad); - const stored = await svc.get(c.ns, c.key, ADMIN()); - expect(stored.value).toBe(c.bad); - expect(stored.source).not.toBe('default'); - - // REPORTED: exactly one line, at warn, naming namespace, key and type. - expect(warns).toHaveLength(1); - expect(warns[0]).toContain(`'${c.ns}.${c.key}'`); - expect(warns[0]).toContain(`(type '${c.type}')`); - expect(warns[0]).toContain('The save is accepted'); - // Never the value itself. - expect(warns[0]).not.toContain(c.bad); - expect(errors).toHaveLength(0); - }); - - it(`${c.ns}.${c.key} (${c.type}): a well-formed value draws no warning`, async () => { - const { svc, http, warns, errors } = makeStack(); - const r = await put(http, c.ns, { [c.key]: c.good }); - expect(r.status).toBe(200); - expect((await svc.get(c.ns, c.key, ADMIN())).value).toBe(c.good); - expect(warns).toHaveLength(0); - expect(errors).toHaveLength(0); - }); - } - - it('the other shapes each grammar admits draw no warning either', async () => { - // The readers' own contracts: `logoUrl` is "absolute or relative", storage - // hands back a root-relative path for an upload, and CSS hex colours come - // in four lengths. - const { http, warns } = makeStack(); - for (const logo of ['/api/v1/storage/files/abc123', 'data:image/svg+xml;base64,PHN2Zy8+', 'http://intranet.local/logo.png']) { - expect((await put(http, 'branding', { logo_url: logo })).status).toBe(200); - } - for (const colour of ['#abc', '#abcd', '#AABBCC', '#aabbccdd']) { - expect((await put(http, 'branding', { accent_color: colour })).status).toBe(200); - } - // Surrounding whitespace is trimmed by both live readers before use, so it - // is not a format miss; inner whitespace still is (the per-type cases above). - expect((await put(http, 'company', { primary_contact_email: ' ops@example.com ' })).status).toBe(200); - expect(warns).toEqual([]); - }); - - it('a secret-bearing key: the malformed save is accepted, and its warning never carries the value', async () => { - const { svc, http, warns } = makeStack(); - const r = await put(http, 'format_lab', { billing_email: SECRET_MALFORMED }); - expect(r.status).toBe(200); - // Stored: the in-process read decrypts the real plaintext back. - expect((await svc.get('format_lab', 'billing_email', ADMIN())).value).toBe(SECRET_MALFORMED); - - expect(warns).toHaveLength(1); - expect(warns[0]).toContain("'format_lab.billing_email'"); - expect(warns[0]).toContain("(type 'email')"); - expect(warns[0]).not.toContain(SECRET_MALFORMED); - // Not a fragment of it either. - expect(warns[0]).not.toContain('billing-desk'); - expect(warns[0]).not.toContain('corp-internal'); - }); - - it('a refused patch carries exactly the errors it carried before, and draws no format warning', async () => { - const { svc, http, warns } = makeStack(); - // `docs_url` declares `maxLength: 12`: the length refusal keeps precedence - // over the format, and the one FieldError for the key is the length one. - const r = await put(http, 'format_lab', { docs_url: 'not a url at all' }); - expect(r.status).toBe(400); - expect(r.body.error.code).toBe('SETTINGS_VALIDATION'); - expect(r.body.error.details.fields).toHaveLength(1); - expect(r.body.error.details.fields[0].field).toBe('docs_url'); - expect(r.body.error.details.fields[0].code).toBe('max_length'); - // A batch refused for one key is refused whole: the malformed neighbour is - // not stored, so it is not reported either. - const r2 = await put(http, 'format_lab', { docs_url: 'not a url at all', support_phone: 'call the desk' }); - expect(r2.status).toBe(400); - expect(r2.body.error.details.fields.map((f: { field: string }) => f.field)).toEqual(['docs_url']); - expect((await svc.get('format_lab', 'support_phone', ADMIN())).source).toBe('default'); - expect(warns).toEqual([]); - }); - - it('an empty or blank value is not a format miss', async () => { - const { http, warns } = makeStack(); - expect((await put(http, 'branding', { logo_url: '' })).status).toBe(200); - expect((await put(http, 'branding', { logo_url: ' ' })).status).toBe(200); - expect(warns).toEqual([]); - }); -}); - -describe('env door — a format-missing override stays in force, and is warned about once (#24882, 「甲」)', () => { - for (const c of PER_TYPE) { - const envName = `OS_${c.ns}_${c.key}`.toUpperCase(); - - it(`${envName} (${c.type}): a malformed override is in force, served and locking, with exactly one warning`, async () => { - const { svc, http, warns, errors } = makeStack({ [envName]: c.bad }); - // Reported at boot — `registerManifest` audits the overrides — not on a - // first read that may never come. - expect(warns).toHaveLength(1); - - const r = await svc.get(c.ns, c.key, ADMIN()); - expect(r.value).toBe(c.bad); - expect(r.source).toBe('env'); - expect(r.locked).toBe(true); - // Still locking: a save of the key is refused 409 exactly as before. - const w = await put(http, c.ns, { [c.key]: c.good }); - expect(w.status).toBe(409); - expect(w.body.error.code).toBe('SETTINGS_LOCKED'); - // Re-reads and the namespace payload do not repeat the line. - await svc.get(c.ns, c.key, ADMIN()); - await svc.getNamespace(c.ns, ADMIN()); - - expect(warns).toHaveLength(1); - expect(warns[0]).toContain(envName); - expect(warns[0]).toContain(`'${c.ns}.${c.key}'`); - expect(warns[0]).toContain(`(type '${c.type}')`); - expect(warns[0]).toContain('It IS in force'); - // An environment variable's value never reaches the log line. - expect(warns[0]).not.toContain(c.bad); - expect(errors).toHaveLength(0); - }); - - it(`${envName} (${c.type}): a well-formed override draws no warning`, async () => { - const { svc, warns, errors } = makeStack({ [envName]: c.good }); - const r = await svc.get(c.ns, c.key, ADMIN()); - expect(r.value).toBe(c.good); - expect(r.source).toBe('env'); - expect(warns).toHaveLength(0); - expect(errors).toHaveLength(0); - }); - } - - it('a secret-bearing key: the override is in force, and its warning never carries the value', async () => { - const { svc, warns } = makeStack({ OS_FORMAT_LAB_BILLING_EMAIL: SECRET_MALFORMED }); - const r = await svc.get('format_lab', 'billing_email', ADMIN()); - expect(r.value).toBe(SECRET_MALFORMED); - expect(r.source).toBe('env'); - expect(warns).toHaveLength(1); - expect(warns[0]).toContain('OS_FORMAT_LAB_BILLING_EMAIL'); - expect(warns[0]).toContain("'format_lab.billing_email'"); - expect(warns[0]).toContain("(type 'email')"); - expect(warns[0]).not.toContain(SECRET_MALFORMED); - expect(warns[0]).not.toContain('billing-desk'); - expect(warns[0]).not.toContain('corp-internal'); - }); - - it('an override REJECTED by a refusal family is reported as rejected, not as a format miss', async () => { - // `docs_url` declares `maxLength: 12`; a 16-character override is ignored - // with the existing `error` line, and the format judgment never runs on a - // value that is not in force. - const { svc, warns, errors } = makeStack({ OS_FORMAT_LAB_DOCS_URL: 'not a url at all' }); - const r = await svc.get('format_lab', 'docs_url', ADMIN()); - expect(r.source).not.toBe('env'); - expect(errors).toHaveLength(1); - expect(errors[0]).toContain('IGNORED'); - expect(warns).toEqual([]); - }); -}); diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index f3caa344f9a..ae561682b08 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -626,10 +626,10 @@ function firstPatternMiss(declared: DeclaredPattern, value: unknown): { value: s * `#rrggbbaa`): ADR-0007 declares "hex string", the colour picker emits the * six-digit form, and the reader serves the value as a browser theme colour. * - * `code` is the ADR-0114 `FieldErrorCode` the refusal of this format carries - * (the catalog's closed set has no colour member, so `color` takes - * `invalid_format`, the member for "a declared pattern/format the value does - * not match"). It travels in the verdict so the refusal half reads it from + * `code` is the ADR-0114 `FieldErrorCode` the save door's refusal of this + * format carries (the catalog's closed set has no colour member, so `color` + * takes `invalid_format`, the member for "a declared pattern/format the value + * does not match"). It travels in the verdict so the call site reads it from * here rather than choosing a second time. */ const DECLARED_VALUE_FORMATS: ReadonlyMap< @@ -685,17 +685,23 @@ interface FormatMiss { * bounds and pattern judges next door are shared: one comparison living on one * door only is how #5204 and #6580 happened. * - * ## The verdict WARNS in this major version; it does not refuse + * ## Both doors REFUSE a miss * - * Under the standing rule 「甲」 a narrowing that no ADR-0087 conversion can - * carry ships its loud half first, and refusing these values narrows what the - * settings door has always accepted. Stored values are runtime state in - * `sys_setting`, and an `OS_*` override is a deployment's environment, so no - * conversion can rewrite either. Both call sites therefore REPORT a miss and - * keep the value: the save is accepted and stored, the override stays in - * force. The refusal is the next major version's, and it reads `code` from - * this verdict, so turning the warning into a `FieldError` is a change at each - * call site and nowhere else. + * Refusing these values narrows what the settings door accepted, and no + * ADR-0087 conversion can carry that: stored values are runtime state in + * `sys_setting`, and an `OS_*` override is a deployment's environment. Under + * the standing rule 「甲」 as the maintainer amended it, such a narrowing ships + * now, as a breaking minor, rather than waiting on a later major. So a miss is + * refused at both doors: the save door answers one `FieldError` for the key, + * with `code` (`validatePatch`), and stores nothing; the env door rejects the + * override the way it rejects every other family's miss + * (`reportRejectedEnvOverride`), so it is not in force and the key resolves + * from the next layer of the cascade. + * + * Neither door judges a value it is not handed. A row already stored that + * misses its format is still served by every read, and it does not block a + * save that does not TOUCH its key — the gate `validatePatch` applies to every + * family, for the reason it gives there. * * A non-string value is left alone, as `firstPatternMiss` leaves it: a format * constrains character shape, and policing the value's type is a different @@ -804,14 +810,6 @@ export class SettingsService { * the value is part of the key and not just the var name. */ private readonly reportedEnvOverrides = new Set(); - /** - * `OS_*` overrides already reported for missing their type's declared format - * (#24882), keyed `=` for the reason - * {@link reportedEnvOverrides} is: `get()` runs the env judgment on every - * read, and a genuinely new value must still be heard. The key lives only in - * memory; the warning itself never carries the value. - */ - private readonly reportedEnvFormatMisses = new Set(); /** * `.` pairs whose fail-closed encryption refusal (#8026) has * already been reported to the logger. Deduped for the same reason @@ -1198,11 +1196,10 @@ export class SettingsService { if (!reg) return; // Only the keys that declare something enforceable can be rejected, so only // they are worth walking: an option table (#5131/#5204), a value window - // (#5932), a standard value domain (#5712) or a pattern (#6580). A key whose - // TYPE declares a format (#24882) is walked too: its override is never - // rejected for the format, but a miss is reported here, at boot, beside the - // rest of the startup output. `effectiveEnvOverride` does the judging (and - // the reporting); the value it returns is of no interest here. + // (#5932), a standard value domain (#5712), a pattern (#6580), or a format + // its TYPE declares (#24882) — that last one has no per-key table, so its + // keys are found by type. `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(), @@ -1252,7 +1249,9 @@ export class SettingsService { * still judged on one door only, now arrives through the same shared helper * the save path calls ({@link firstPatternMiss}), in the same family order * the save path applies — options → pattern → valueDomain → bounds — so the - * two doors report the same family for the same value. + * two doors report the same family for the same value. #24882 appends the + * format a key's TYPE declares (`email` / `url` / `phone` / `color`) to the + * end of that order, on both doors, through {@link firstFormatMiss}. */ private effectiveEnvOverride( reg: RegisteredManifest, @@ -1353,56 +1352,26 @@ export class SettingsService { } } - // The type's declared format (#24882) — judged LAST and never a rejection. - // Every refusal family above has passed, so this override IS in force; - // under 「甲」 a format miss is reported (`reportEnvFormatMiss`) and the - // override keeps winning the cascade and pinning the key, exactly as it did - // before this judgment existed. Same helper as the save path - // ({@link firstFormatMiss}). + // The format the key's TYPE declares (#24882), judged LAST — after every + // family above, in the position it holds in `validatePatch`, so a value + // that breaks a declared bound and its format is rejected for the bound at + // both doors. Same helper as the save path ({@link firstFormatMiss}). The + // line never prints the value, for any key: it carries the value's length + // instead (`shape`). const formatMiss = firstFormatMiss(declaredTypeOf(reg.manifest, key), value); - if (formatMiss) this.reportEnvFormatMiss(namespace, key, envName, value, formatMiss); + if (formatMiss) { + this.reportRejectedEnvOverride(reg, namespace, key, envName, value, { + what: `does not match the format its type '${formatMiss.type}' declares for`, + detail: `Allowed values: ${formatMiss.expected}.`, + fix: formatMiss.expected, + shape: formatMiss.shape, + }); + return null; + } return { envName, value }; } - /** - * Emit the one warning an in-force `OS_*` override owes its operator when its - * value misses the type's declared format (#24882), at most once per - * (env var, value) for the life of the service — the dedupe - * {@link reportRejectedEnvOverride} applies, for the same reason. - * - * **Level is `warn`, not `error`.** AGENTS.md → "Degradation log levels": the - * override is in force and is served as configured, so nothing the system - * claims has failed to land. That is the line between this report and the - * `error` a REJECTED override gets. - * - * **The value is never printed**, for any key — secret-bearing or not: an - * environment variable's value does not go into a log line. The line carries - * the variable's name, the setting, the type, what the format expects and the - * value's length, which is enough to find and fix it. No structured `meta`, - * for the reason {@link reportRejectedEnvOverride} gives. - */ - private reportEnvFormatMiss( - namespace: string, - key: string, - envName: string, - value: unknown, - miss: FormatMiss, - ): void { - const dedupeAt = `${envName}=${String(value)}`; - if (this.reportedEnvFormatMisses.has(dedupeAt)) return; - this.reportedEnvFormatMisses.add(dedupeAt); - const message = - `[SettingsService] env override ${envName} for setting '${namespace}.${key}' ` + - `(type '${miss.type}') is not ${miss.expected} — the value is ${miss.shape}. ` + - `It IS in force: reads serve it with source 'env' and the key stays locked against saves. ` + - `The declared format is reported, not enforced, in this major version; a later major ` + - `version refuses it. Fix: set ${envName} to ${miss.expected}, or unset it and ` + - `configure '${namespace}.${key}' through the settings UI.`; - if (this.logger?.warn) this.logger.warn(message); - else console.warn(message); - } - /** * Emit the one loud line a rejected `OS_*` override owes its operator, at * most once per (env var, value) for the life of the service. @@ -1445,8 +1414,12 @@ export class SettingsService { * 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. + * + * `shape`, when given, replaces the value in the line for EVERY key, not + * only an encrypted one: the format family (#24882) reports what the value + * looks like (its length) and never the value itself. */ - rejection: { what: string; detail: string; fix: string }, + rejection: { what: string; detail: string; fix: string; shape?: string }, ): void { const dedupeAt = `${envName}=${String(offending)}`; if (this.reportedEnvOverrides.has(dedupeAt)) return; @@ -1457,7 +1430,9 @@ export class SettingsService { // held encrypted, in a message that lands in logs. Same rule, same reason, // as the save path's `invalid_option` error. const secret = reg.encryptedKeys.has(key); - const rejected = secret ? '' : ` Rejected value: '${String(offending)}'.`; + const rejected = typeof rejection.shape === 'string' + ? ` The rejected value is ${rejection.shape}; it is not printed.` + : secret ? '' : ` Rejected value: '${String(offending)}'.`; const message = `[SettingsService] env override ${envName} ${rejection.what} ` + `setting '${namespace}.${key}' — IGNORED.${rejected} ${rejection.detail} ` + @@ -2457,10 +2432,10 @@ export class SettingsService { * (`invalid_value`, #6199). * - The format the specifier TYPE declares (`email` / `url` / `phone` / * `color`, ADR-0007; #24882) + non-empty string value that misses it → - * ACCEPTED, and reported: one `warn` line per key, emitted only when the - * patch as a whole is accepted. Never a `FieldError` in this major version - * (「甲」 — see {@link firstFormatMiss}); judged after every refusal family, - * so it never changes which errors a refused patch carries. + * rejected (`invalid_email` / `invalid_url` / `invalid_phone` / + * `invalid_format`, the code the verdict carries — see + * {@link firstFormatMiss}). Judged after every other family, so a value + * that also breaks one of those is refused for that one. * - All-null patches (namespace reset) skip validation rather than block the * write. * - A `visible` predicate `evaluateVisibility` cannot parse → the write is @@ -2479,7 +2454,9 @@ export class SettingsService { * 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. + * settings — it is told about the key only when it writes that key. The + * format check (#24882) inherits it for the first reason: a value stored + * before that check existed is refused only by a patch that touches its 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 @@ -2547,9 +2524,6 @@ export class SettingsService { // a `key → message` map and could only re-emit the prose — so the code is // stamped at the point the check fails rather than inferred from the text. const errors: FieldError[] = []; - // Format misses (#24882), reported — never refused — once the patch is - // known to be accepted. See the format branch at the bottom of the loop. - const formatMisses: Array<{ key: string; miss: FormatMiss }> = []; for (const spec of (reg.manifest.specifiers ?? []) as Array>) { const key = spec.key as string | undefined; @@ -2799,40 +2773,32 @@ export class SettingsService { } // The format the specifier TYPE declares (ADR-0007; #24882), judged last - // so every refusal family above keeps precedence and the errors a refused - // patch carries are exactly what they were. Under 「甲」 a miss is - // RECORDED, not refused: the save is accepted and the value is stored as - // written. The refusal half turns this push into an `errors.push` of a - // `FieldError` built from `miss.code` — this branch and the env door's are - // the only two sites (`firstFormatMiss`). + // so every family above keeps precedence. The code is the one the + // verdict carries (`firstFormatMiss`), the same helper the env door + // calls. The message names what the format expects and never the value, + // for any key, so neither `value` nor a `Received` echo is attached: the + // caller sent the value and needs only to be told what to send instead. + // `constraint.type` names the declaring property, so a client can tell a + // colour's `invalid_format` from a declared pattern's (`constraint. + // pattern`) without parsing the sentence (ADR-0114). if (!empty) { const miss = firstFormatMiss(type, value); - if (miss) formatMisses.push({ key, miss }); + if (miss) { + errors.push({ + field: key, + code: miss.code, + message: `${label} must be ${miss.expected}.`, + label, + constraint: { type: miss.type }, + }); + continue; + } } } if (errors.length > 0) { throw new SettingsValidationError(namespace, errors); } - - // The patch is accepted. A key whose value misses its declared format is - // reported once per save, at `warn`: the value lands and is served, so - // nothing the system claims has failed (AGENTS.md → "Degradation log - // levels"). The value itself is never printed — not for a secret-bearing - // key, and not for any other: the line carries the setting, the type, what - // the format expects and the value's length, which is what an operator - // needs to find it. No structured `meta`, for the reason - // `reportRejectedEnvOverride` gives. - for (const { key, miss } of formatMisses) { - const message = - `[SettingsService] Setting '${namespace}.${key}' (type '${miss.type}') holds a value ` + - `that is not ${miss.expected} after this save — the value is ${miss.shape}. ` + - `The save is accepted: the value is stored and served as written. The declared format ` + - `is reported, not enforced, in this major version; a later major version refuses it. ` + - `Fix: save ${miss.expected} for '${namespace}.${key}', or clear it.`; - if (this.logger?.warn) this.logger.warn(message); - else console.warn(message); - } } /** From 4f9294a3e286ff1128bec36751b43835dc6fa78d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 00:39:35 +0000 Subject: [PATCH 2/3] chore(changeset): the format refusal replaces the pending warning changeset for service-settings The warning changeset described behaviour this branch changes before any release consumed it, so it is replaced by one BREAKING minor changeset that states the save door and the env door refusal, the env fallback and what an operator does. Claude-Session: https://claude.ai/code/session_01UmqzD97e14iws6hmVhgZ5y Co-authored-by: Claude --- .changeset/24882-settings-format-refusal.md | 31 +++++++++++++++++++++ .changeset/24882-settings-format-warning.md | 26 ----------------- 2 files changed, 31 insertions(+), 26 deletions(-) create mode 100644 .changeset/24882-settings-format-refusal.md delete mode 100644 .changeset/24882-settings-format-warning.md diff --git a/.changeset/24882-settings-format-refusal.md b/.changeset/24882-settings-format-refusal.md new file mode 100644 index 00000000000..3692eb97bf7 --- /dev/null +++ b/.changeset/24882-settings-format-refusal.md @@ -0,0 +1,31 @@ +--- +'@objectstack/service-settings': minor +--- + +fix(service-settings)!: a settings value that misses the format its `email`, `url`, `phone` or `color` type declares is refused, both when it is saved and when it arrives as an `OS_*` override (#24882) + +ADR-0007 declares a format for four specifier types: `email`, `url` and `phone` take "format validation", and `color` takes a hex string. Until now nothing enforced it. `PUT /api/settings/branding` stored a Logo URL of `not a url` and answered `200`, and an `OS_*` override was served whatever it held. Both doors now judge the format through one shared check, after every other declared constraint, and refuse a value that misses it. + +Clause-②: no (narrowing) + + + +**BREAKING** (the settings doors' accept set narrows), shipped as `minor` under the launch-window convention for breaking changes. + +The formats are the platform's existing grammars for the same words: + +- `email`: an address with a dotted domain, the grammar the mail transport sends with. `no-reply@localhost` misses it. +- `url`: `scheme://…`, a root-relative `/path`, a `data:` URI or a `blob:` URL. A relative logo path that the branding reader serves is accepted. +- `phone`: digits, spaces and `+ ( ) - .`, at least five characters. +- `color`: `#rgb`, `#rgba`, `#rrggbb` or `#rrggbbaa`. + +Empty values and non-string values are not judged. Surrounding whitespace is ignored, because the readers trim it before use. + +What changes: + +- **Saving, FROM:** a save naming a value that misses its key's format answered `200` and stored the value as written. +- **Saving, TO:** the save answers `400` with `error.code: 'SETTINGS_VALIDATION'` and one entry per such key in `error.details.fields`, carrying `code` `invalid_email`, `invalid_url`, `invalid_phone` or `invalid_format` (for `color`), and `constraint.type`. Nothing in the save is stored. The answer names the expected format and never echoes the value. +- **Environment override, FROM:** a malformed `OS_*` override was in force: reads served it with source `env`, and it locked the key against saves. +- **Environment override, TO:** a malformed `OS_*` override is no longer in force. The server logs one `error` line per variable and value, naming the variable, the setting, the type and the expected format, never the value. Reads serve the next layer instead: the stored value when one exists, otherwise the manifest default, with that layer as the source. The key is no longer locked, so it can be saved through Setup. +- **Unchanged:** a well-formed value saves, and a well-formed override is in force and locks the key. A value that breaks another declared constraint (a length bound, a pattern) is still refused for that constraint. A value stored before this release is still served by every read, and it does not block a save of the namespace's other keys; only a save that writes that key, or a key its `visible` condition reads, judges it. An all-null save (reset) still clears it. +- **What to do:** fix the value. For an `OS_*` override, set the variable to a well-formed value or unset it and configure the setting in Setup. For a stored value, save a well-formed value for the key, or clear it. diff --git a/.changeset/24882-settings-format-warning.md b/.changeset/24882-settings-format-warning.md deleted file mode 100644 index 038d99db690..00000000000 --- a/.changeset/24882-settings-format-warning.md +++ /dev/null @@ -1,26 +0,0 @@ ---- -'@objectstack/service-settings': patch ---- - -feat(service-settings): a settings value that misses the format its `email`, `url`, `phone` or `color` type declares is now reported in the server log; the save is still accepted (#24882) - -Clause-②: no (warning only: every save and every `OS_*` override accepted before is still accepted, with the same value stored or served, the same status and the same errors) - -ADR-0007 declares a format for four specifier types: `email`, `url` and `phone` take "format validation", and `color` takes a hex string. Until now nothing judged that format. `PUT /api/settings/branding` stored a Logo URL of `not a url` and answered 200, and an `OS_*` override was served unchecked. - -Both doors now judge the format through one shared check, and report a miss instead of refusing it: - -- **Save.** When a save is accepted and a key it touches holds a value that misses its type's format, the service logs one `warn` line for that key. The line names the setting, the type, the expected format and the value's length. -- **Environment override.** An in-force `OS_*` override whose value misses the format draws one `warn` line, at boot or on the first read. The line names the variable, the setting, the type and the expected format. The override stays in force, is served with source `env`, and still locks the key. -- **The value is never printed** in either line, for any key. - -The formats are the platform's existing grammars for the same words: - -- `email`: an address with a dotted domain, the grammar the mail transport sends with. -- `url`: `scheme://…`, a root-relative `/path`, a `data:` URI or a `blob:` URL. An http(s)-only grammar would flag a relative logo path that the branding reader serves. -- `phone`: digits, spaces and `+ ( ) - .`, at least five characters. -- `color`: `#rgb`, `#rgba`, `#rrggbb` or `#rrggbbaa`. - -Empty values and non-string values are not judged. Surrounding whitespace is ignored, because the readers trim it before use. - -**Unchanged.** Status codes, response bodies, stored values, the `FieldError`s a refused save carries, and which overrides are in force. Under the standing rule 「甲」, refusing these values waits for the next major version. From b3d83aad4c69578700d2eb49c916ca235b4445da Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 00:40:46 +0000 Subject: [PATCH 3/3] test(service-settings): read the cascade chain as optional in the env refusal pin Claude-Session: https://claude.ai/code/session_01UmqzD97e14iws6hmVhgZ5y Co-authored-by: Claude --- .../service-settings/src/settings-format-refusal.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/services/service-settings/src/settings-format-refusal.test.ts b/packages/services/service-settings/src/settings-format-refusal.test.ts index 833536bb26e..07d04abe1ed 100644 --- a/packages/services/service-settings/src/settings-format-refusal.test.ts +++ b/packages/services/service-settings/src/settings-format-refusal.test.ts @@ -334,7 +334,7 @@ describe('env door — a format-missing override is rejected: not in force, repo expect(r.source).toBe('default'); expect(r.value).not.toBe(c.bad); expect(r.locked).toBe(false); - expect(r.cascadeChain.some((e) => e.scope === 'env')).toBe(false); + expect((r.cascadeChain ?? []).map((e) => e.scope)).not.toContain('env'); // Re-reads and the namespace payload do not repeat the line. await svc.get(c.ns, c.key, ADMIN()); await svc.getNamespace(c.ns, ADMIN());