diff --git a/.changeset/env-door-enforces-pattern.md b/.changeset/env-door-enforces-pattern.md new file mode 100644 index 0000000000..5e94df367c --- /dev/null +++ b/.changeset/env-door-enforces-pattern.md @@ -0,0 +1,14 @@ +--- +'@objectstack/service-settings': patch +--- + +The settings env door now enforces declared `pattern` constraints (#6580). An +`OS_*` override whose value the specifier's `pattern` rejects is loudly +reported (`error` log, once per var+value) and ignored — the key resolves from +the next cascade layer and is not locked — exactly the #5204 contract the +option-table, value-window/step and valueDomain families already honor. The +write gate's judgment is hoisted into shared helpers (`declaredPattern` / +`firstPatternMiss`) called by both doors, so `PUT /api/settings/:ns` behavior +is unchanged byte-for-byte (same `invalid_format` envelope, same tolerance for +uncompilable pattern declarations) and the two doors can no longer drift. +Family ordering agrees between doors: options → pattern → valueDomain → bounds. diff --git a/packages/services/service-settings/src/settings-env-pattern.test.ts b/packages/services/service-settings/src/settings-env-pattern.test.ts new file mode 100644 index 0000000000..27cae4ac98 --- /dev/null +++ b/packages/services/service-settings/src/settings-env-pattern.test.ts @@ -0,0 +1,297 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6580 — the env door enforces `pattern`, at the ONE decision point. + * + * `pattern` was the last declared constraint family judged on one door only: + * `validatePatch` refused a shape-illegal value (`invalid_format`) while + * `effectiveEnvOverride` let the same value through an `OS_*` override and + * pinned the key (`locked: true`) on top of it — #5204's original asymmetry, + * one family later. Both doors now call the same helpers (`declaredPattern` / + * `firstPatternMiss`), so the comparison cannot drift between them again. + * + * Every fixture here is SYNTHETIC on purpose: #6579 is retuning + * `company.manifest.ts` (`company.country` gains `valueDomain`) in parallel, + * so these tests must not depend on any shipped manifest's declarations + * landing in either order. + */ + +import { describe, expect, it } from 'vitest'; +import { SettingsService } from './settings-service.js'; +import { SettingsManifestSchema } from '@objectstack/spec/system'; + +const spyLogger = () => { + const errors: string[] = []; + return { errors, logger: { error: (m: string) => void errors.push(m) } }; +}; + +/** One key, one declared constraint: `pattern` and nothing else. */ +const patternOnlyManifest = { + namespace: 'pattern_lab', + version: 1, + label: 'Pattern Lab', + scope: 'global', + specifiers: [ + { type: 'text', key: 'code', label: 'Code', pattern: '^[A-Za-z]{2}$', default: 'US' }, + ], +} as any; + +/** + * A pattern that does not compile (`[` is an unterminated character class). + * The write gate has always answered this with "nothing to enforce" rather + * than a refusal or a crash; the env door must inherit exactly that tolerance, + * because both doors now obtain the declaration through `declaredPattern`. + */ +const invalidPatternManifest = { + namespace: 'pattern_tolerance', + version: 1, + label: 'Pattern Tolerance', + scope: 'global', + specifiers: [ + { type: 'text', key: 'freeform', label: 'Freeform', pattern: '[', default: 'anything' }, + ], +} as any; + +/** + * Keys that declare `pattern` AND a second family, to pin the ordering + * between doors: options (when no domain) → pattern → valueDomain → bounds. + * A value that breaks several declarations must be rejected for the SAME + * reason at both doors, not merely rejected at both. + */ +const orderingManifest = { + namespace: 'pattern_order', + version: 1, + label: 'Pattern Ordering', + scope: 'global', + specifiers: [ + // pattern + length window: `^[a-z]+$` and `minLength: 5`. + { type: 'text', key: 'slug', label: 'Slug', pattern: '^[a-z]+$', minLength: 5, default: 'validslug' }, + // pattern + standard value domain: shape says two letters, membership says + // an ASSIGNED two letters — `ZZ` satisfies the pattern and not the domain. + { + type: 'text', key: 'country_like', label: 'Country-like', + pattern: '^[A-Za-z]{2}$', valueDomain: 'iso_3166_alpha2', default: 'US', + }, + ], +} as any; + +describe('synthetic fixtures are spec-valid authoring surfaces', () => { + it('pattern_lab / pattern_order parse under SettingsManifestSchema', () => { + // The guard that keeps these tests honest: a fixture spelling a key the + // schema rejects would pin behaviour no author can reach. (The + // invalid-RegExp fixture is deliberately NOT parsed here — `pattern: '['` + // is type-valid to Zod, which does not compile patterns; the tolerance + // under test is the service's, not the schema's.) + expect(() => SettingsManifestSchema.parse(patternOnlyManifest)).not.toThrow(); + expect(() => SettingsManifestSchema.parse(orderingManifest)).not.toThrow(); + }); +}); + +describe('env door — OS_* overrides are judged against the declared pattern (#6580)', () => { + it('ignores a pattern-illegal override loudly and resolves the next cascade layer', async () => { + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_PATTERN_LAB_CODE: 'ZZZ9' }, logger }); + svc.registerManifest(patternOnlyManifest); + + const r = await svc.get('pattern_lab', 'code'); + expect(r.value).toBe('US'); // the manifest default, not the override + expect(r.source).toBe('default'); + // Not in force, so it pins nothing either — read and write agree (#5204). + expect(r.locked).toBe(false); + expect(r.cascadeChain?.some((e) => e.scope === 'env')).toBe(false); + + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('OS_PATTERN_LAB_CODE'); + expect(errors[0]).toContain('does not match the declared pattern'); + expect(errors[0]).toContain("Rejected value: 'ZZZ9'"); + expect(errors[0]).toContain('^[A-Za-z]{2}$'); // the declaration, for the operator + expect(errors[0]).toContain('IGNORED'); + expect(errors[0]).toContain('does NOT take effect'); + }); + + it('a pattern-legal override still wins the cascade and locks the key', async () => { + // The regression pin for the untouched path — the check must not turn + // into "env never applies to a pattern-bearing key". + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_PATTERN_LAB_CODE: 'CH' }, logger }); + svc.registerManifest(patternOnlyManifest); + + const r = await svc.get('pattern_lab', 'code'); + expect(r.value).toBe('CH'); + expect(r.source).toBe('env'); + expect(r.locked).toBe(true); + expect(errors).toHaveLength(0); + }); + + it('reports the misconfiguration at registration, and says it ONCE', async () => { + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_PATTERN_LAB_CODE: 'ZZZ9' }, logger }); + expect(errors).toHaveLength(0); + svc.registerManifest(patternOnlyManifest); + expect(errors).toHaveLength(1); // a pattern-bearing key is walked at boot + for (let i = 0; i < 5; i++) await svc.get('pattern_lab', 'code'); + await svc.getNamespace('pattern_lab'); + expect(errors).toHaveLength(1); // said ONCE (#5204 dedupe) + }); + + it('a REJECTED override pins nothing — the key stays editable', async () => { + // #5204's `locked` coherence rule, inherited for free BECAUSE the pattern + // is judged at the one point: a key configurable by nothing (env ignored, + // UI refused) would be a lockout only an env edit could clear. + const { logger } = spyLogger(); + const svc = new SettingsService({ env: { OS_PATTERN_LAB_CODE: 'ZZZ9' }, logger }); + svc.registerManifest(patternOnlyManifest); + + expect((await svc.get('pattern_lab', 'code')).locked).toBe(false); + await expect(svc.setMany('pattern_lab', { code: 'DE' })).resolves.toBeDefined(); + const after = await svc.get('pattern_lab', 'code'); + expect(after.value).toBe('DE'); + expect(after.source).toBe('global'); + }); +}); + +describe('write door — the #6580 hoist changes nothing at PUT /api/settings/:ns', () => { + it('still refuses the same value as invalid_format with constraint.pattern', async () => { + const { logger } = spyLogger(); + const svc = new SettingsService({ env: {}, logger }); + svc.registerManifest(patternOnlyManifest); + + let caught: any; + try { + await svc.setMany('pattern_lab', { code: 'ZZZ9' }); + } catch (e) { + caught = e; + } + // Rejection-class case: assert the envelope, not the throw. The service + // layer's envelope is `code` + `fields[]` (the HTTP status mapping is + // pinned in envelope.conformance.test.ts). + expect(caught).toBeDefined(); + expect(caught.code).toBe('SETTINGS_VALIDATION'); + expect(caught.fields).toHaveLength(1); + expect(caught.fields[0]).toMatchObject({ + field: 'code', + code: 'invalid_format', + constraint: { pattern: '^[A-Za-z]{2}$' }, + }); + expect(caught.fields[0].message).toContain('does not match the expected format'); + // The pre-#6580 branch never echoed the value on invalid_format — + // byte-for-byte means byte-for-byte. + expect(caught.fields[0]).not.toHaveProperty('value'); + + await expect(svc.setMany('pattern_lab', { code: 'FR' })).resolves.toBeDefined(); + }); +}); + +describe('invalid-RegExp declaration — the shared tolerance, pinned on both doors', () => { + it('registration does not crash, and the env door enforces nothing', async () => { + const { errors, logger } = spyLogger(); + const svc = new SettingsService({ + env: { OS_PATTERN_TOLERANCE_FREEFORM: '!!not a match for anything!!' }, logger, + }); + expect(() => svc.registerManifest(invalidPatternManifest)).not.toThrow(); + + // Nothing to enforce, so the override is simply in force — unchanged + // pre-#6580 behaviour for an uncompilable declaration. + const r = await svc.get('pattern_tolerance', 'freeform'); + expect(r.value).toBe('!!not a match for anything!!'); + expect(r.source).toBe('env'); + expect(r.locked).toBe(true); + expect(errors).toHaveLength(0); + }); + + it('the write door tolerates it identically — no enforcement, no crash', async () => { + const { logger } = spyLogger(); + const svc = new SettingsService({ env: {}, logger }); + svc.registerManifest(invalidPatternManifest); + await expect( + svc.setMany('pattern_tolerance', { freeform: '!!still not a match!!' }), + ).resolves.toBeDefined(); + expect((await svc.get('pattern_tolerance', 'freeform')).value).toBe('!!still not a match!!'); + }); +}); + +describe('family ordering agrees between doors: options → pattern → valueDomain → bounds', () => { + it('pattern vs length window: a value breaking both is a pattern miss at BOTH doors', async () => { + // 'A2' misses `^[a-z]+$` AND sits under `minLength: 5`. The write door has + // always let `pattern` speak before the window; the env door must name the + // same family for the same value. + const { logger } = spyLogger(); + const svc = new SettingsService({ env: {}, logger }); + svc.registerManifest(orderingManifest); + await expect(svc.setMany('pattern_order', { slug: 'A2' })).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [{ field: 'slug', code: 'invalid_format', constraint: { pattern: '^[a-z]+$' } }], + }); + + const { errors, logger: envLogger } = spyLogger(); + const env = new SettingsService({ env: { OS_PATTERN_ORDER_SLUG: 'A2' }, logger: envLogger }); + env.registerManifest(orderingManifest); + expect((await env.get('pattern_order', 'slug')).source).toBe('default'); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('does not match the declared pattern'); + expect(errors[0]).not.toContain('length'); + }); + + it('…and a pattern-legal value still falls to the window family, at BOTH doors', async () => { + // 'ab' satisfies the pattern and breaks `minLength: 5` — proof the hoist + // did not swallow the families ordered after it. + const { logger } = spyLogger(); + const svc = new SettingsService({ env: {}, logger }); + svc.registerManifest(orderingManifest); + await expect(svc.setMany('pattern_order', { slug: 'ab' })).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [{ field: 'slug', code: 'min_length' }], + }); + + const { errors, logger: envLogger } = spyLogger(); + const env = new SettingsService({ env: { OS_PATTERN_ORDER_SLUG: 'ab' }, logger: envLogger }); + env.registerManifest(orderingManifest); + expect((await env.get('pattern_order', 'slug')).source).toBe('default'); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('is outside the declared length'); + }); + + it('pattern vs valueDomain: a value breaking both is a pattern miss at BOTH doors', async () => { + // 'ZZZ' misses `^[A-Za-z]{2}$` AND is no ISO 3166-1 member; shape speaks + // first on the write door (`pattern` has always run before the #5712 + // domain branch), so it must speak first on the env door too. + const { logger } = spyLogger(); + const svc = new SettingsService({ env: {}, logger }); + svc.registerManifest(orderingManifest); + await expect(svc.setMany('pattern_order', { country_like: 'ZZZ' })).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [{ field: 'country_like', code: 'invalid_format' }], + }); + + const { errors, logger: envLogger } = spyLogger(); + const env = new SettingsService({ + env: { OS_PATTERN_ORDER_COUNTRY_LIKE: 'ZZZ' }, logger: envLogger, + }); + env.registerManifest(orderingManifest); + expect((await env.get('pattern_order', 'country_like')).source).toBe('default'); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('does not match the declared pattern'); + expect(errors[0]).not.toContain('ISO 3166-1'); + }); + + it('…and a shape-legal non-member still falls to the domain family, at BOTH doors', async () => { + // 'ZZ' is the schema's own worked example: admitted by the pattern, + // assigned to nobody. Membership must still refuse it on both doors. + const { logger } = spyLogger(); + const svc = new SettingsService({ env: {}, logger }); + svc.registerManifest(orderingManifest); + await expect(svc.setMany('pattern_order', { country_like: 'ZZ' })).rejects.toMatchObject({ + code: 'SETTINGS_VALIDATION', + fields: [{ field: 'country_like', code: 'invalid_value', constraint: { valueDomain: 'iso_3166_alpha2' } }], + }); + + const { errors, logger: envLogger } = spyLogger(); + const env = new SettingsService({ + env: { OS_PATTERN_ORDER_COUNTRY_LIKE: 'ZZ' }, logger: envLogger, + }); + env.registerManifest(orderingManifest); + expect((await env.get('pattern_order', 'country_like')).source).toBe('default'); + expect(errors).toHaveLength(1); + expect(errors[0]).toContain('is not a valid ISO 3166-1'); + }); +}); diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index cca1ffa94e..2286305679 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -377,6 +377,68 @@ function firstRangeViolation(bounds: DeclaredBounds, value: unknown): RangeViola return null; } +/** + * A declared `pattern` in enforceable form (#6580): the source string as the + * manifest spelled it (for `FieldError.constraint` and the env log line) and + * the compiled `RegExp`. Compiled once and shared safely — `new RegExp(source)` + * takes no flags here, and a flagless RegExp's `test()` is stateless + * (`lastIndex` only advances under `g`/`y`). + */ +interface DeclaredPattern { + source: string; + re: RegExp; +} + +/** + * The `pattern` this specifier declares as something enforceable, or `null` + * when it declares none — where "none" deliberately includes a declaration + * that does not compile. + * + * The invalid-RegExp tolerance is the write gate's own, hoisted verbatim: the + * `validatePatch` branch has always answered an uncompilable `pattern` with + * `re = undefined` ("invalid manifest pattern — don't block writes"), and it + * is the same disposition {@link declaredBounds} gives an impossible `step` + * (#6199): such a manifest rejects no values and misconfigures no deployment, + * it merely fails to constrain. Because BOTH doors obtain the declaration + * through this one function, the tolerance can no longer drift between them. + */ +function declaredPattern(pattern: unknown): DeclaredPattern | null { + if (typeof pattern !== 'string') return null; + try { + return { source: pattern, re: new RegExp(pattern) }; + } catch { + return null; // invalid manifest pattern — nothing to enforce, never a refusal + } +} + +/** + * The value, wrapped, when it is a string the declared pattern does not admit; + * `null` when the pattern has nothing to say about it. + * + * ONE comparison, shared by both paths that produce an effective value — the + * save path ({@link SettingsService.validatePatch}) and the env path + * ({@link SettingsService.effectiveEnvOverride}) — for the reason #5204 is on + * file and #5932's triage turned into a ruling: the same comparison living in + * one door only is how `PUT /api/settings/:ns` came to refuse the very value + * an `OS_*` override slid straight through (#6580). `pattern` was the last + * declared constraint family judged on one door. + * + * A non-string value is left alone: a `pattern` constrains character shape, so + * the value's SHAPE decides applicability — exactly how the write gate's + * branch has always behaved (`typeof value === 'string'`), and the same + * posture, same sentence, as `firstRangeViolation`'s length window. Policing + * the value's type is a different constraint (`invalid_type`) with a different + * owner. + * + * Returns a WRAPPER rather than a boolean for symmetry with + * {@link firstRejectedOption}: the caller reports the offending value, and the + * wrapper is the shape every family hands back at both call sites. + */ +function firstPatternMiss(declared: DeclaredPattern, value: unknown): { value: string } | null { + if (typeof value !== 'string') return null; + return declared.re.test(value) ? null : { value }; +} + interface RegisteredManifest { manifest: SettingsManifest; /** Resolved specifier scopes for fast lookup. */ @@ -423,6 +485,17 @@ interface RegisteredManifest { * standard's membership and degrades `options` to a UI convenience list. */ valueDomains: Map; + /** + * Declared, compilable `pattern` for every specifier that has one (#6580), + * keyed by specifier key. + * + * Precomputed for the same reason and read the same way as `optionTables`: + * `get()` is the hottest path, and an ABSENT key means "nothing to enforce" + * — no pattern declared, or a declaration that does not compile (the write + * gate's own tolerance, see {@link declaredPattern}) — rather than "an + * impossible pattern", which would reject everything. + */ + patterns: Map; } /** @@ -577,6 +650,7 @@ export class SettingsService { const optionTables = new Map(); const bounds = new Map(); const valueDomains = new Map(); + const patterns = new Map(); const defaultScope = manifest.scope ?? 'tenant'; for (const spec of manifest.specifiers) { if (!spec.key || LAYOUT_ONLY_TYPES.has(spec.type)) continue; @@ -597,6 +671,12 @@ export class SettingsService { // unenforceable claim or an accept-everything hole. const domain = knownValueDomain((spec as { valueDomain?: unknown }).valueDomain); if (domain) valueDomains.set(spec.key, domain); + // The declared `pattern`, compiled once (#6580). `declaredPattern` + // carries the write gate's own tolerance — an uncompilable declaration + // records nothing to enforce — so an absent entry means unchanged + // behaviour at both call sites, never "reject everything". + const pattern = declaredPattern(spec.pattern); + if (pattern) patterns.set(spec.key, pattern); if (OPTION_BEARING_TYPES.has(spec.type)) { // A manifest with no option table cannot say what is legal. The spec // refuses that shape at parse time, but `registerManifest` takes @@ -618,6 +698,7 @@ export class SettingsService { optionTables, bounds, valueDomains, + patterns, }); this.auditEnvOverrides(manifest.namespace); } @@ -644,13 +725,14 @@ 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) or a standard value domain (#5712). `effectiveEnvOverride` does - // the judging (and the reporting); the value it returns is of no interest - // here. + // (#5932), a standard value domain (#5712) or a pattern (#6580). + // `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(), ...reg.valueDomains.keys(), + ...reg.patterns.keys(), ]); for (const key of enforceable) { this.effectiveEnvOverride(reg, namespace, key); @@ -689,6 +771,11 @@ export class SettingsService { * family rather than opening a third branch: it rides `DeclaredBounds` and * `firstRangeViolation`, so it arrives on both paths at once by construction * and cannot be the next constraint that is enforced on one door only. + * #6580 closed the set out: `pattern`, the LAST declared constraint family + * 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. */ private effectiveEnvOverride( reg: RegisteredManifest, @@ -701,6 +788,11 @@ export class SettingsService { const value = coerceEnvValue(envRaw, reg.defaults.get(key)); + // Families are judged in the SAME order `validatePatch` judges them — + // options (when no domain is declared) → pattern → valueDomain → bounds — + // so a value that breaks several declarations is rejected for the same + // reason at both doors, not just rejected at both (#6580). + // // A declared standard value domain (#5712) REPLACES the option table as // the membership boundary: the standard's membership is what the override // is judged against, and `options` is a UI convenience list this door does @@ -708,18 +800,7 @@ export class SettingsService { // is on file: the same comparison in two places is how the env half came to // disagree with the save half in the first place. const domain = reg.valueDomains.get(key); - if (domain) { - const rejected = firstRejectedDomainMember(domain, value); - if (rejected) { - const { member, example } = valueDomainPhrasing(domain); - this.reportRejectedEnvOverride(reg, namespace, key, envName, rejected.value, { - what: `is not a valid ${member} for`, - detail: `Allowed values: any ${member} (e.g. '${example}').`, - fix: `a valid ${member}`, - }); - return null; - } - } else { + if (!domain) { // A key with no declared table has nothing to enforce — unchanged // behaviour (#5131's exhaustive-options semantics, untouched when no // domain is declared). @@ -737,6 +818,39 @@ export class SettingsService { } } + // The declared `pattern` (#6580) — the last declared constraint family + // that was judged on one door only. Judged by the same helper the save + // path calls ({@link firstPatternMiss}), in the same position it holds + // there: after the option table, before the domain membership and the + // value window. A key with no compilable pattern has nothing to enforce + // ({@link declaredPattern} — the write gate's invalid-RegExp tolerance, + // shared by construction). + const pattern = reg.patterns.get(key); + if (pattern) { + const miss = firstPatternMiss(pattern, value); + if (miss) { + this.reportRejectedEnvOverride(reg, namespace, key, envName, miss.value, { + what: 'does not match the declared pattern for', + detail: `Allowed values: strings matching /${pattern.source}/.`, + fix: 'a value matching the declared pattern', + }); + return null; + } + } + + if (domain) { + const rejected = firstRejectedDomainMember(domain, value); + if (rejected) { + const { member, example } = valueDomainPhrasing(domain); + this.reportRejectedEnvOverride(reg, namespace, key, envName, rejected.value, { + what: `is not a valid ${member} for`, + detail: `Allowed values: any ${member} (e.g. '${example}').`, + fix: `a valid ${member}`, + }); + return null; + } + } + // Likewise a key with no declared window (#5932). const bounds = reg.bounds.get(key); if (bounds) { @@ -1420,14 +1534,13 @@ export class SettingsService { } } - if (!empty && typeof spec.pattern === 'string' && typeof value === 'string') { - let re: RegExp | undefined; - try { - re = new RegExp(spec.pattern); - } catch { - re = undefined; // invalid manifest pattern — don't block writes - } - if (re && !re.test(value)) { + // Shared with the env path (#6580) — see `firstPatternMiss` for the + // string-shape applicability and `declaredPattern` for the + // invalid-RegExp tolerance (unchanged: an uncompilable manifest pattern + // never blocks writes). + if (!empty) { + const declared = declaredPattern(spec.pattern); + if (declared && firstPatternMiss(declared, value)) { const hint = typeof spec.description === 'string' ? ` ${spec.description}` : ''; errors.push({ field: key, @@ -1436,7 +1549,7 @@ export class SettingsService { label, // The declared pattern, so a client can format its own message // rather than parsing ours (`FieldError.constraint`, ADR-0114). - constraint: { pattern: spec.pattern }, + constraint: { pattern: declared.source }, }); continue; }