fix(config): enforce Zod simulator-config validation at boot (#1874) (#1903)
* 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.