From 225159487916788c59a9276f474826ccdcb372e1 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=B4me=20Benoit?= Date: Mon, 15 Jun 2026 21:20:06 +0200 Subject: [PATCH] fix(config): enforce Zod simulator-config validation at boot (#1874) (#1903) MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit * fix(config): enforce Zod simulator-config validation at boot (#1874) Audit pass over uiServer.* primitives that PR #1901 left under-constrained. Each constraint closes a concrete fail-fast gap surfaced by a 3-angle design review (schema completeness, fail-fast wiring, adversarial risk). UIServerAuthenticationSchema: - username: z.string().min(1).optional() — empty username currently passes; AbstractUIServer falls back to '' and authenticates against the empty configured value, silently bypassing Basic-Auth. - password: z.string().min(1).optional() — same hole on password. - username .refine(no ':') (RFC 7617) — lifts the runtime check at UIServerFactory.ts:48-54 into the schema layer. - object-level .refine: enabled === true requires both username and password. Closes the silent-bypass when authentication is enabled with no creds. UIServerAccessPolicySchema: - object-level .refine: allowLoopbackProxy === true requires trustedProxies.length >= 1. With allowLoopbackProxy: true and an empty trustedProxies list, the proxy-trust check at runtime always evaluates against the empty allowlist, contradicting the operator's intent. No wiring change: Configuration.ts:160 (boot, process.exit(1)) and Configuration.ts:386-431 (hot-reload, rollback) already route ConfigurationValidationError correctly. Schema tightening lands at parse time without rewiring. Test fixture update: tests/utils/ConfigurationSchema.test.ts unknown-key test for uiServer.authentication now includes valid username/password alongside bogusAuthKey so the unknown-key intent stays unambiguous after the new object-level superRefine. 9 new test cases (5 reject + 3 accept regression-anchors + 1 cross-field reject) covering rejection paths and the docker/config.json-shape acceptance. Designs that did NOT survive cross-validation are documented as out-of-scope follow-ups: type×version coupling (coupled to in-place mutation bug at UIServerFactory.ts:69), type×auth coupling (defense in depth at runtime is intentional), options.{path,readableAll,writableAll, ipv6Only} refinements (no shipped config exercises these), and the TLS-non-loopback wildcard refine (would break docker/config.json:18). * fix(config): refine auth schema error path and tighten test path checks Address review feedback on #1903: src/utils/ConfigurationSchema.ts - UIServerAuthenticationSchema object-level .refine now carries path: ['username'], producing an error path of 'uiServer.authentication.username' rather than 'uiServer.authentication'. Mirrors the path: ['allowLoopbackProxy'] convention already used on UIServerAccessPolicySchema and yields a more actionable message. - JSDoc clarifies that the field-level constraints (min(1), no ':') apply unconditionally even when authentication.enabled is false: the schema is intentionally stricter than the runtime guard in UIServerFactory so a credentials value cannot ship under enabled: false and become a bypass the moment enabled flips on (e.g. via hot-reload). tests/utils/ConfigurationSchema.test.ts - Three .includes('uiServer.authentication') substring checks tightened to .startsWith('uiServer.authentication') so they cannot match a hypothetical sibling key like 'uiServer.authenticationFoo'. Affects the two object-level coupling tests and the unknown-key fixture assertion. Exact-element checks at lines 630/651 (specific leaf paths) and the leaf-specific .includes at line 671 are left as-is. No behavior change. 114/114 schema tests pass, typecheck + lint clean. * docs(config): tighten UIServerAuthenticationSchema JSDoc Rewrite the JSDoc as a single factual paragraph matching the file convention (LogConfigurationSchema, WorkerConfigurationSchema, UIServerListenOptionsObjectSchema, ...) — 12 lines collapsed to 7, no redundant phrasing, structured as: constraint definitions → required-when-enabled coupling → unconditional-field-level rationale. Per AGENTS.md 'Documentation conventions' (concision, structure). * fix(config): emit per-field error paths on auth-required refine Address sub-agent review feedback on #1903. src/utils/ConfigurationSchema.ts - Replace the .refine() with a fixed path: ['username'] by a .superRefine() that emits one issue per actually-missing field with the correct path. Configurations supplying only 'username' now surface the issue at 'uiServer.authentication.password' rather than 'uiServer.authentication.username', and vice versa. Configurations missing both produce two issues, one per field. tests/utils/ConfigurationSchema.test.ts - 'should reject authentication enabled without username and password' now asserts both 'uiServer.authentication.username' and 'uiServer.authentication.password' paths are present, locking the per-field issue contract. - 'should reject authentication enabled with username but no password' now asserts 'uiServer.authentication.password' specifically — would catch the prior path-misanchoring regression. - The 'should reject unknown key in uiServer.authentication section' regression-anchor now asserts i.code === 'unrecognized_keys' at the 'uiServer.authentication' path, locking the unknown-key intent so a hypothetical future refine cannot satisfy the assertion by accident. 114/114 schema tests pass, typecheck + lint clean. No production- runtime behavior change beyond per-field error paths. * fix(ui-common): align authenticationConfigSchema with simulator auth schema Apply the same defense-in-depth + per-field-error-path pattern shipped on the simulator-side UIServerAuthenticationSchema in this PR. The Web UI client schema (consumed by ui/web/src/main.ts and transitively by ui/cli/src/config/loader.ts) carried the same set of defects: - password lacked min(1) at the field level - username regex /^[^:]*$/ accepted the empty string - a single .refine combined type-check + presence + length, producing a generic error at the auth object root with no path - empty creds shipped under enabled: false would become a Basic-Auth bypass the moment enabled is flipped on ui/common/src/config/schema.ts - password: z.string().min(1).optional() - username: z.string().min(1).refine(no ':' RFC 7617).optional() (replaces the regex which silently accepted '') - .refine(...) replaced by .superRefine(...) emitting one issue per actually-missing field with the correct path, only when enabled === true && type === PROTOCOL_BASIC_AUTH (existing type-check short-circuit preserved as future-proofing). - JSDoc mirrors the simulator-side sibling, documenting the intentional stricter-than-runtime semantics. ui/common/tests/config.test.ts - 4 new tests: empty password rejected (path 'authentication.password'), username with ':' rejected (RFC 7617 message at 'authentication.username'), password-only and username-only enabled configs each rejected at the missing field's path. - 2 existing tests tightened: 'missing credentials' now asserts both username and password paths; 'empty username' now asserts the 'authentication.username' path. - 6 redundant 'if (result.success) return' narrowing guards removed — assert.strictEqual already narrows the result type. 118/118 ui-common tests pass, typecheck + lint clean. No production behavior change for the only valid-config consumer ('admin'/'admin' shape regression-anchored). * fix(config): point accessPolicy refine error path at the actionable field Address review feedback on #1903. The .refine() rejecting 'allowLoopbackProxy: true' with empty 'trustedProxies' attached its issue at path ['allowLoopbackProxy'] — the trigger of the rejection, but not the field the operator must edit. Move the path to ['trustedProxies'] so structured log scrapers and IDE jump-to-field tooling steer the operator to the field that needs populating. The message wording (which already names both fields) is unchanged. Test 'should reject allowLoopbackProxy=true with empty trustedProxies' tightened from a permissive substring check ('allowLoopbackProxy or trustedProxies') to an exact-path check on 'uiServer.accessPolicy.trustedProxies' so the path contract is locked. A second review suggestion (gate the auth credentials-required superRefine on type === PROTOCOL_BASIC_AUTH) was investigated and declined: the server-side AuthenticationType enum has BASIC_AUTH AND PROTOCOL_BASIC_AUTH (unlike the Web UI client's single-member enum), both runtime paths require username/password (AbstractUIServer.ts:358-381), and adding the type-gate would re-open the silent Basic-Auth bypass on the legacy 'basic-auth' path that this PR is closing on the modern path. 114/114 schema tests pass, typecheck + lint clean. * docs(config): refine auth-schema JSDoc rationale and tighten RFC 7617 path check Address sub-agent review feedback on #1903. src/utils/ConfigurationSchema.ts + ui/common/src/config/schema.ts - Drop the 'across hot-reload toggles of enabled' phrasing in the UIServerAuthenticationSchema and authenticationConfigSchema JSDoc. Hot-reload of uiServer.* is documented as a no-op in the PR body (R4: Bootstrap.ts:103,161 instantiates the UI server once), so the unconditional field-level constraints are an early-detection signal rather than a hot-reload mitigation. The accurate rationale is that empty placeholders cannot ship under enabled: false and become a Basic-Auth bypass on the next boot with enabled: true. - The asymmetry between the two superRefines (server gates only on enabled; Web UI also gates on type === PROTOCOL_BASIC_AUTH) is preserved and intentional — server-side AuthenticationType has legacy BASIC_AUTH which is also credential-required at runtime (AbstractUIServer.ts isBasicAuthEnabled / isValidBasicAuth). The rationale is captured in this commit body and the prior 86a6d4a2 commit body for git blame discoverability. tests/utils/ConfigurationSchema.test.ts - RFC 7617 username-rejection test now matches the issue path with i.path.join('.') === 'uiServer.authentication.username' (exact) instead of String.prototype.includes (substring), aligning with every other test in the file and with the ui-common sibling. 114/114 schema tests + 118/118 ui-common tests pass, typecheck + lint clean. No behavior change. --- src/utils/ConfigurationSchema.ts | 47 +++++- tests/utils/ConfigurationSchema.test.ts | 199 ++++++++++++++++++++++++ ui/common/src/config/schema.ts | 47 ++++-- ui/common/tests/config.test.ts | 99 ++++++++++++ 4 files changed, 372 insertions(+), 20 deletions(-) diff --git a/src/utils/ConfigurationSchema.ts b/src/utils/ConfigurationSchema.ts index cabf7ef6..c3bca037 100644 --- a/src/utils/ConfigurationSchema.ts +++ b/src/utils/ConfigurationSchema.ts @@ -80,18 +80,44 @@ export const StorageConfigurationSchema = z .strict() /** - * UIServerAuthentication — credentials for the UI server. - * `enabled` and `type` are required; `username`/`password` are optional and - * depend on the chosen authentication scheme. + * UIServerAuthentication — credentials for the UI server. `username` is a + * non-empty string without `':'` (RFC 7617); `password` is a non-empty + * string. Both are required when `enabled` is true. Field-level constraints + * fire unconditionally — intentionally stricter than the runtime guard in + * `UIServerFactory` so empty placeholders cannot ship under `enabled: false` + * and become a Basic-Auth bypass on the next boot with `enabled: true`. */ export const UIServerAuthenticationSchema = z .object({ enabled: z.boolean(), - password: z.string().optional(), + password: z.string().min(1).optional(), type: z.enum(AuthenticationType), - username: z.string().optional(), + username: z + .string() + .min(1) + .refine(value => !value.includes(':'), { + message: "must not contain ':' (RFC 7617)", + }) + .optional(), }) .strict() + .superRefine((value, ctx) => { + if (!value.enabled) return + if (value.username == null) { + ctx.addIssue({ + code: 'custom', + message: "'username' is required when 'authentication.enabled' is true", + path: ['username'], + }) + } + if (value.password == null) { + ctx.addIssue({ + code: 'custom', + message: "'password' is required when 'authentication.enabled' is true", + path: ['password'], + }) + } + }) export const UI_SERVER_ACCESS_POLICY_DEFAULTS = { allowedHosts: [], @@ -138,6 +164,17 @@ export const UIServerAccessPolicySchema = z .optional(), }) .strict() + .refine( + value => + !( + value.allowLoopbackProxy === true && + (value.trustedProxies == null || value.trustedProxies.length === 0) + ), + { + message: "'allowLoopbackProxy' requires at least one entry in 'trustedProxies'", + path: ['trustedProxies'], + } + ) /** * UIServerListenOptionsObjectSchema — typed object layer for `node:net` diff --git a/tests/utils/ConfigurationSchema.test.ts b/tests/utils/ConfigurationSchema.test.ts index b73f9883..38bf434b 100644 --- a/tests/utils/ConfigurationSchema.test.ts +++ b/tests/utils/ConfigurationSchema.test.ts @@ -609,6 +609,197 @@ await describe('ConfigurationSchema', async () => { ) assert.ok(!result.success) }) + + await describe('uiServer.authentication credentials', async () => { + await it('should reject empty username', () => { + const result = ConfigurationSchema.safeParse( + buildMinimalConfiguration({ + uiServer: { + authentication: { + enabled: true, + password: 'p', + type: 'protocol-basic-auth', + username: '', + }, + }, + }) + ) + assert.ok(!result.success) + const paths = result.error.issues.map(i => i.path.join('.')) + assert.ok( + paths.includes('uiServer.authentication.username'), + `Expected error path 'uiServer.authentication.username' in ${JSON.stringify(paths)}` + ) + }) + + await it('should reject empty password', () => { + const result = ConfigurationSchema.safeParse( + buildMinimalConfiguration({ + uiServer: { + authentication: { + enabled: true, + password: '', + type: 'protocol-basic-auth', + username: 'u', + }, + }, + }) + ) + assert.ok(!result.success) + const paths = result.error.issues.map(i => i.path.join('.')) + assert.ok( + paths.includes('uiServer.authentication.password'), + `Expected error path 'uiServer.authentication.password' in ${JSON.stringify(paths)}` + ) + }) + + await it("should reject username containing ':' (RFC 7617)", () => { + const result = ConfigurationSchema.safeParse( + buildMinimalConfiguration({ + uiServer: { + authentication: { + enabled: true, + password: 'p', + type: 'protocol-basic-auth', + username: 'a:b', + }, + }, + }) + ) + assert.ok(!result.success) + const usernameIssues = result.error.issues.filter( + i => i.path.join('.') === 'uiServer.authentication.username' + ) + assert.ok(usernameIssues.length > 0) + assert.ok( + usernameIssues.some(i => i.message.includes('RFC 7617')), + `Expected an issue mentioning 'RFC 7617' in ${JSON.stringify(usernameIssues)}` + ) + }) + + await it('should reject authentication enabled without username and password', () => { + const result = ConfigurationSchema.safeParse( + buildMinimalConfiguration({ + uiServer: { + authentication: { + enabled: true, + type: 'protocol-basic-auth', + }, + }, + }) + ) + assert.ok(!result.success) + const paths = result.error.issues.map(i => i.path.join('.')) + assert.ok( + paths.includes('uiServer.authentication.username'), + `Expected error path 'uiServer.authentication.username' in ${JSON.stringify(paths)}` + ) + assert.ok( + paths.includes('uiServer.authentication.password'), + `Expected error path 'uiServer.authentication.password' in ${JSON.stringify(paths)}` + ) + }) + + await it('should reject authentication enabled with username but no password', () => { + const result = ConfigurationSchema.safeParse( + buildMinimalConfiguration({ + uiServer: { + authentication: { + enabled: true, + type: 'protocol-basic-auth', + username: 'u', + }, + }, + }) + ) + assert.ok(!result.success) + const paths = result.error.issues.map(i => i.path.join('.')) + assert.ok( + paths.includes('uiServer.authentication.password'), + `Expected error path 'uiServer.authentication.password' in ${JSON.stringify(paths)}` + ) + }) + + await it('should accept authentication disabled without credentials', () => { + const result = ConfigurationSchema.safeParse( + buildMinimalConfiguration({ + uiServer: { + authentication: { + enabled: false, + type: 'protocol-basic-auth', + }, + }, + }) + ) + assert.ok( + result.success, + `Expected disabled auth without credentials to be accepted: ${result.success ? '' : JSON.stringify(result.error.issues)}` + ) + }) + + await it('should accept real shipped credentials shape (admin/admin)', () => { + const result = ConfigurationSchema.safeParse( + buildMinimalConfiguration({ + uiServer: { + authentication: { + enabled: true, + password: 'admin', + type: 'protocol-basic-auth', + username: 'admin', + }, + enabled: true, + type: 'ws', + }, + }) + ) + assert.ok( + result.success, + `Expected admin/admin to be accepted: ${result.success ? '' : JSON.stringify(result.error.issues)}` + ) + }) + }) + + await describe('uiServer.accessPolicy allowLoopbackProxy/trustedProxies coupling', async () => { + await it('should reject allowLoopbackProxy=true with empty trustedProxies', () => { + const result = ConfigurationSchema.safeParse( + buildMinimalConfiguration({ + uiServer: { + accessPolicy: { + allowLoopbackProxy: true, + trustedProxies: [], + }, + enabled: true, + type: 'ws', + }, + }) + ) + assert.ok(!result.success) + const paths = result.error.issues.map(i => i.path.join('.')) + assert.ok( + paths.includes('uiServer.accessPolicy.trustedProxies'), + `Expected error path 'uiServer.accessPolicy.trustedProxies' in ${JSON.stringify(paths)}` + ) + }) + + await it('should accept allowLoopbackProxy=false with empty trustedProxies (docker shape)', () => { + const result = ConfigurationSchema.safeParse( + buildMinimalConfiguration({ + uiServer: { + accessPolicy: { + allowLoopbackProxy: false, + trustedProxies: [], + }, + enabled: true, + type: 'ws', + }, + }) + ) + assert.ok( + result.success, + `Expected docker-shape accessPolicy to be accepted: ${result.success ? '' : JSON.stringify(result.error.issues)}` + ) + }) + }) }) await describe('mixed-type fields', async () => { @@ -854,7 +1045,9 @@ await describe('ConfigurationSchema', async () => { authentication: { bogusAuthKey: 'x', enabled: true, + password: 'p', type: 'protocol-basic-auth', + username: 'u', }, enabled: true, type: 'ws', @@ -862,6 +1055,12 @@ await describe('ConfigurationSchema', async () => { }) ) assert.ok(!result.success) + assert.ok( + result.error.issues.some( + i => i.code === 'unrecognized_keys' && i.path.join('.') === 'uiServer.authentication' + ), + `Expected an 'unrecognized_keys' issue at 'uiServer.authentication' in ${JSON.stringify(result.error.issues)}` + ) }) }) diff --git a/ui/common/src/config/schema.ts b/ui/common/src/config/schema.ts index 98e0adb3..38cd8e59 100644 --- a/ui/common/src/config/schema.ts +++ b/ui/common/src/config/schema.ts @@ -14,30 +14,47 @@ export const THEME_IDS = [ 'tokyo-night-storm', ] as const +/** + * authenticationConfig — Web UI client credentials. `username` is a non-empty + * string without `':'` (RFC 7617); `password` is a non-empty string. Both are + * required when `enabled` is true and `type === 'protocol-basic-auth'`. + * Field-level constraints fire unconditionally — intentionally stricter than + * the runtime auth flow so empty placeholders cannot ship under + * `enabled: false` and become a Basic-Auth bypass on the next boot with + * `enabled: true`. + */ export const authenticationConfigSchema = z .object({ enabled: z.boolean(), - password: z.string().optional(), + password: z.string().min(1).optional(), type: z.enum(AuthenticationType), username: z .string() - .regex(/^[^:]*$/, 'must not contain ":"') + .min(1) + .refine(value => !value.includes(':'), { + message: "must not contain ':' (RFC 7617)", + }) .optional(), }) - .refine( - data => - !data.enabled || - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - data.type !== AuthenticationType.PROTOCOL_BASIC_AUTH || - (data.username != null && - data.username.length > 0 && - data.password != null && - data.password.length > 0), - { - message: - 'username and password are required when authentication is enabled with protocol-basic-auth', + .superRefine((value, ctx) => { + if (!value.enabled) return + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + if (value.type !== AuthenticationType.PROTOCOL_BASIC_AUTH) return + if (value.username == null) { + ctx.addIssue({ + code: 'custom', + message: "'username' is required when authentication is enabled with protocol-basic-auth", + path: ['username'], + }) } - ) + if (value.password == null) { + ctx.addIssue({ + code: 'custom', + message: "'password' is required when authentication is enabled with protocol-basic-auth", + path: ['password'], + }) + } + }) export const uiServerConfigSchema = z.object({ authentication: authenticationConfigSchema.optional(), diff --git a/ui/common/tests/config.test.ts b/ui/common/tests/config.test.ts index ab1c4e48..32d0b681 100644 --- a/ui/common/tests/config.test.ts +++ b/ui/common/tests/config.test.ts @@ -118,6 +118,15 @@ await describe('config schema validation', async () => { version: '0.0.1', }) assert.strictEqual(result.success, false) + const paths = result.error.issues.map(i => i.path.join('.')) + assert.ok( + paths.includes('authentication.username'), + `Expected error path 'authentication.username' in ${JSON.stringify(paths)}` + ) + assert.ok( + paths.includes('authentication.password'), + `Expected error path 'authentication.password' in ${JSON.stringify(paths)}` + ) }) await it('should reject auth config when enabled with protocol-basic-auth but empty username', () => { @@ -134,6 +143,96 @@ await describe('config schema validation', async () => { version: '0.0.1', }) assert.strictEqual(result.success, false) + const paths = result.error.issues.map(i => i.path.join('.')) + assert.ok( + paths.includes('authentication.username'), + `Expected error path 'authentication.username' in ${JSON.stringify(paths)}` + ) + }) + + await it('should reject auth config when enabled with protocol-basic-auth but empty password', () => { + const result = uiServerConfigSchema.safeParse({ + authentication: { + enabled: true, + password: '', + type: 'protocol-basic-auth', + username: 'admin', + }, + host: 'localhost', + port: 8080, + protocol: 'ui', + version: '0.0.1', + }) + assert.strictEqual(result.success, false) + const paths = result.error.issues.map(i => i.path.join('.')) + assert.ok( + paths.includes('authentication.password'), + `Expected error path 'authentication.password' in ${JSON.stringify(paths)}` + ) + }) + + await it("should reject auth config when username contains ':' (RFC 7617)", () => { + const result = uiServerConfigSchema.safeParse({ + authentication: { + enabled: true, + password: 'admin', + type: 'protocol-basic-auth', + username: 'a:b', + }, + host: 'localhost', + port: 8080, + protocol: 'ui', + version: '0.0.1', + }) + assert.strictEqual(result.success, false) + const usernameIssues = result.error.issues.filter( + i => i.path.join('.') === 'authentication.username' + ) + assert.ok(usernameIssues.length > 0) + assert.ok( + usernameIssues.some(i => i.message.includes('RFC 7617')), + `Expected an issue mentioning 'RFC 7617' in ${JSON.stringify(usernameIssues)}` + ) + }) + + await it('should reject auth config when enabled with password but no username', () => { + const result = uiServerConfigSchema.safeParse({ + authentication: { + enabled: true, + password: 'admin', + type: 'protocol-basic-auth', + }, + host: 'localhost', + port: 8080, + protocol: 'ui', + version: '0.0.1', + }) + assert.strictEqual(result.success, false) + const paths = result.error.issues.map(i => i.path.join('.')) + assert.ok( + paths.includes('authentication.username'), + `Expected error path 'authentication.username' in ${JSON.stringify(paths)}` + ) + }) + + await it('should reject auth config when enabled with username but no password', () => { + const result = uiServerConfigSchema.safeParse({ + authentication: { + enabled: true, + type: 'protocol-basic-auth', + username: 'admin', + }, + host: 'localhost', + port: 8080, + protocol: 'ui', + version: '0.0.1', + }) + assert.strictEqual(result.success, false) + const paths = result.error.issues.map(i => i.path.join('.')) + assert.ok( + paths.includes('authentication.password'), + `Expected error path 'authentication.password' in ${JSON.stringify(paths)}` + ) }) await it('should accept auth config when enabled with protocol-basic-auth and credentials present', () => { -- 2.53.0