From e5e9af5c3e8c6517afa81f7df250ff1c34654069 Mon Sep 17 00:00:00 2001 From: eleith Date: Thu, 12 Nov 2026 14:32:16 -0800 Subject: [PATCH] fix(config): report safe validation issues --- .changeset/config-diagnostics.md | 7 + apps/cli/src/cli.test.ts | 31 ++++- apps/cli/src/cli.ts | 4 +- apps/web/src/lib/server/application.test.ts | 25 ++++ apps/web/src/lib/server/application.ts | 7 +- apps/worker/src/index.ts | 9 +- packages/config/src/diagnostics.test.ts | 137 ++++++++++++++++++++ packages/config/src/errors.ts | 34 ++++- packages/config/src/index.ts | 2 +- packages/config/src/load.test.ts | 27 ++-- packages/config/src/load.ts | 38 ++++-- packages/config/src/resolve.test.ts | 4 +- 12 files changed, 293 insertions(+), 32 deletions(-) create mode 100644 .changeset/config-diagnostics.md create mode 100644 packages/config/src/diagnostics.test.ts diff --git a/.changeset/config-diagnostics.md b/.changeset/config-diagnostics.md new file mode 100644 index 0000000..b9b4c0e --- /dev/null +++ b/.changeset/config-diagnostics.md @@ -0,0 +1,7 @@ +--- +'@goto/web': patch +'@goto/worker': patch +'@goto/cli': patch +--- + +Report curated configuration issue paths and YAML locations in CLI and startup logs. Keep browser-facing errors generic and exclude configuration values, private dictionary keys, parser snippets and arbitrary error details. diff --git a/apps/cli/src/cli.test.ts b/apps/cli/src/cli.test.ts index d76c684..25edc19 100644 --- a/apps/cli/src/cli.test.ts +++ b/apps/cli/src/cli.test.ts @@ -134,7 +134,29 @@ describe('goto-cli config validate', () => { expect(await runCli(['config', 'validate'], output, { cwd, env: environment })).toBe(1); expect(output.stdout).not.toHaveBeenCalled(); - expect(output.stderr).toHaveBeenCalledExactlyOnceWith('Configuration validation failed.\n'); + expect(output.stderr).toHaveBeenCalledExactlyOnceWith( + expect.stringMatching(/^Configuration validation failed\.\n/) + ); + const diagnostic = output.stderr.mock.calls[0]?.[0]; + for (const sensitive of [...Object.values(env), ...Object.values(environment)]) { + if (sensitive !== undefined) expect(diagnostic).not.toContain(sensitive); + } + expect(diagnostic).not.toContain(source); + expect(diagnostic).not.toContain(cwd); + expect(diagnostic).not.toContain('config/goto.yaml'); + }); + + it('reports the site section for an unsupported site.url without printing its value', async () => { + const oldSiteUrl = 'https://old.example.test/private-do-not-print'; + await writeConfiguration('config/goto.yaml', `${validYaml}site:\n url: ${oldSiteUrl}\n`); + const output = createOutput(); + + expect(await runCli(['config', 'validate'], output, { cwd, env })).toBe(1); + expect(output.stdout).not.toHaveBeenCalled(); + expect(output.stderr).toHaveBeenCalledExactlyOnceWith( + expect.stringMatching(/^Configuration validation failed\.\n {2}\/site: \S[^\n]*\n$/) + ); + expect(output.stderr.mock.calls[0]?.[0]).not.toContain(oldSiteUrl); }); it('reports a missing file without disclosing the path', async () => { @@ -144,7 +166,12 @@ describe('goto-cli config validate', () => { await runCli(['config', 'validate', 'missing-private-path.yaml'], output, { cwd, env }) ).toBe(1); expect(output.stdout).not.toHaveBeenCalled(); - expect(output.stderr).toHaveBeenCalledExactlyOnceWith('Configuration validation failed.\n'); + expect(output.stderr).toHaveBeenCalledExactlyOnceWith( + 'Configuration validation failed.\n /: Unable to read configuration file\n' + ); + const diagnostic = output.stderr.mock.calls[0]?.[0]; + expect(diagnostic).not.toContain('missing-private-path.yaml'); + expect(diagnostic).not.toContain(cwd); }); it.each([ diff --git a/apps/cli/src/cli.ts b/apps/cli/src/cli.ts index 641a699..1c4aff8 100644 --- a/apps/cli/src/cli.ts +++ b/apps/cli/src/cli.ts @@ -1,4 +1,4 @@ -import { ConfigError, loadConfigFile, type ConfigOptions } from '@goto/config'; +import { ConfigError, formatConfigError, loadConfigFile, type ConfigOptions } from '@goto/config'; import { cli, define, type CommandContext } from 'gunshi'; interface CliOutput { @@ -57,7 +57,7 @@ export async function runCli( } catch (error) { const message = error instanceof ConfigError - ? 'Configuration validation failed.\n' + ? `${formatConfigError(error, 'Configuration validation failed.')}\n` : 'Command failed. Run goto-cli --help for usage.\n'; output.stderr(message); return 1; diff --git a/apps/web/src/lib/server/application.test.ts b/apps/web/src/lib/server/application.test.ts index ace1fed..15c7f9e 100644 --- a/apps/web/src/lib/server/application.test.ts +++ b/apps/web/src/lib/server/application.test.ts @@ -44,6 +44,7 @@ function prepared( beforeEach(() => { vi.clearAllMocks(); vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); vi.mocked(loadConfigFile).mockResolvedValue(prepared()); vi.mocked(createVisitRecorder).mockReturnValue({ recordVisit, stop: stopRecorder }); vi.mocked(startConfigReloading).mockImplementation(async (options) => { @@ -157,6 +158,30 @@ test('invalid startup fails before registering a monitor', async () => { vi.mocked(loadConfigFile).mockRejectedValue(new ConfigError()); await expect(createApplication(options)).rejects.toThrow('Invalid configuration'); + expect(console.error).toHaveBeenCalledExactlyOnceWith('Invalid configuration'); expect(startConfigReloading).not.toHaveBeenCalled(); expect(createVisitRecorder).not.toHaveBeenCalled(); }); + +test('reports only curated configuration issues during startup', async () => { + const failure = new ConfigError([ + { path: '/url/app', message: 'Expected a canonical app origin' } + ]); + failure.cause = new Error('private startup detail'); + vi.mocked(loadConfigFile).mockRejectedValue(failure); + + await expect(createApplication(options)).rejects.toBe(failure); + expect(console.error).toHaveBeenCalledExactlyOnceWith( + 'Invalid configuration\n /url/app: Expected a canonical app origin' + ); + expect(startConfigReloading).not.toHaveBeenCalled(); + expect(createVisitRecorder).not.toHaveBeenCalled(); +}); + +test('does not expose arbitrary startup errors as configuration diagnostics', async () => { + const failure = new Error('private connection detail'); + vi.mocked(loadConfigFile).mockRejectedValue(failure); + + await expect(createApplication(options)).rejects.toBe(failure); + expect(console.error).not.toHaveBeenCalled(); +}); diff --git a/apps/web/src/lib/server/application.ts b/apps/web/src/lib/server/application.ts index e837829..bf51049 100644 --- a/apps/web/src/lib/server/application.ts +++ b/apps/web/src/lib/server/application.ts @@ -2,6 +2,8 @@ import { isDeepStrictEqual } from 'node:util'; import { resolve } from 'node:path'; import type { NewVisit } from '@goto/db'; import { + ConfigError, + formatConfigError, loadConfigFile, resolveConfigPath, startConfigReloading, @@ -42,7 +44,10 @@ export async function createApplication(options: ConfigOptions = {}): Promise { + if (error instanceof ConfigError) console.error(formatConfigError(error)); + throw error; + }); const initialSettings = startupSettings(initial); const startup = structuredClone(initialSettings); let snapshot = prepareSnapshot(initial); diff --git a/apps/worker/src/index.ts b/apps/worker/src/index.ts index a94aa41..7c3bf5f 100644 --- a/apps/worker/src/index.ts +++ b/apps/worker/src/index.ts @@ -1,4 +1,5 @@ import process from 'node:process'; +import { ConfigError, formatConfigError } from '@goto/config'; import { startWorkerService, type WorkerService } from './services/startup.js'; import { logger } from './services/logger.js'; @@ -48,8 +49,12 @@ try { }); } } -} catch { +} catch (error) { process.exitCode = 1; - logger.error('Unable to start worker service'); + const message = + error instanceof ConfigError + ? formatConfigError(error, 'Unable to start worker service') + : 'Unable to start worker service'; + logger.error(message); await shutdown(); } diff --git a/packages/config/src/diagnostics.test.ts b/packages/config/src/diagnostics.test.ts new file mode 100644 index 0000000..06a8457 --- /dev/null +++ b/packages/config/src/diagnostics.test.ts @@ -0,0 +1,137 @@ +import { describe, expect, it } from 'vitest'; +import { configurationInput } from '../test/configuration.js'; +import { ConfigError, formatConfigError, parseConfig, validateConfig } from './index.js'; + +const options = { env: {} }; +const privateKey = 'private-authored-key-marker/~secret'; +const privateValue = 'private-authored-value-marker'; + +function captureConfigError(action: () => unknown): ConfigError { + try { + action(); + } catch (error) { + if (error instanceof ConfigError) return error; + throw error; + } + throw new Error('Expected configuration to fail'); +} + +describe('configuration diagnostics', () => { + it('keeps the error message generic and formats issues only when requested', () => { + const issues = [{ path: '/site', message: 'Expected object' }]; + const error = new ConfigError(issues); + expect(error.message).toBe('Invalid configuration'); + expect(String(error)).toBe('ConfigError: Invalid configuration'); + expect(error.issues).toEqual(issues); + expect(error).not.toHaveProperty('cause'); + expect(formatConfigError(error)).toBe('Invalid configuration\n /site: Expected object'); + expect(formatConfigError(error, 'Configuration validation failed.')).toBe( + 'Configuration validation failed.\n /site: Expected object' + ); + expect(new ConfigError().issues).toEqual([]); + expect(formatConfigError(new ConfigError())).toBe('Invalid configuration'); + expect(formatConfigError(new ConfigError(), 'Configuration validation failed.')).toBe( + 'Configuration validation failed.' + ); + }); + + it('identifies the site section and its allowed fields without echoing the old URL', () => { + const error = captureConfigError(() => + validateConfig( + { + ...configurationInput(), + site: { url: `https://${privateValue}.example.com` } + }, + options + ) + ); + expect(error.message).toBe('Invalid configuration'); + expect(error.issues).toEqual([ + { + path: '/site', + message: 'Unexpected property. Allowed fields: title, logo_path, favicon_path' + } + ]); + expect(JSON.stringify(error)).not.toContain(privateValue); + expect(formatConfigError(error)).not.toContain(privateValue); + }); + + it.each([ + { override: { [privateKey]: privateValue }, path: '/' }, + { override: { site: { [privateKey]: privateValue } }, path: '/site' } + ])('omits unknown property names and values at $path', ({ override, path }) => { + const error = captureConfigError(() => + validateConfig({ ...configurationInput(), ...override }, options) + ); + expect(error.issues).toEqual([ + { path, message: expect.stringContaining('Unexpected property') } + ]); + for (const output of [JSON.stringify(error), formatConfigError(error)]) { + expect(output).not.toContain('private-authored-key-marker'); + expect(output).not.toContain(privateValue); + } + }); + + it('redacts domain and link keys and reports invalid resolved values without printing them', () => { + const error = captureConfigError(() => + validateConfig( + { + ...configurationInput(), + domains: { + 'private-domain-marker.example.com': { + links: { + '/private-link-marker': { + url: '${DESTINATION}', + [privateKey]: privateValue + } + } + } + } + }, + { env: { DESTINATION: `https://user:${privateValue}@example.com` } } + ) + ); + expect(error.issues).toContainEqual({ + path: '/domains/*/links/*/url', + message: "Expected string to match 'goto-link-url' format" + }); + expect(error.issues).toContainEqual({ + path: '/domains/*/links/*', + message: expect.stringContaining('Unexpected property. Allowed fields: url, title') + }); + for (const output of [JSON.stringify(error), formatConfigError(error)]) { + for (const marker of [ + 'private-domain-marker', + 'private-link-marker', + 'private-authored-key-marker', + privateValue, + '${DESTINATION}' + ]) { + expect(output).not.toContain(marker); + } + } + }); + + it('reports YAML codes and line positions without parser snippets or authored keys', () => { + const source = `${privateKey}: ${privateValue}\n${privateKey}: ${privateValue}`; + const error = captureConfigError(() => parseConfig(source, options)); + expect(error.message).toBe('Invalid configuration'); + expect(error.issues).toEqual([ + { path: '/', message: 'Invalid YAML (DUPLICATE_KEY) at line 2, column 1' } + ]); + for (const output of [JSON.stringify(error), formatConfigError(error)]) { + expect(output).not.toContain('private-authored-key-marker'); + expect(output).not.toContain(privateValue); + expect(output).not.toContain(source); + } + }); + + it('leaves errors without safe context generic', () => { + const error = captureConfigError(() => + validateConfig({ ...configurationInput(), [privateKey]: '${MISSING}' }, options) + ); + expect(error.issues).toEqual([]); + expect(formatConfigError(error)).toBe('Invalid configuration'); + expect(JSON.stringify(error)).not.toContain('private-authored-key-marker'); + }); +}); diff --git a/packages/config/src/errors.ts b/packages/config/src/errors.ts index ec36a5f..4e4febe 100644 --- a/packages/config/src/errors.ts +++ b/packages/config/src/errors.ts @@ -1,6 +1,38 @@ +import { ValueErrorType, type ValueError } from '@sinclair/typebox/value'; + +export interface ConfigIssue { + path: string; + message: string; +} + export class ConfigError extends Error { - constructor() { + readonly issues: readonly ConfigIssue[]; + + constructor(issues: readonly ConfigIssue[] = []) { super('Invalid configuration'); this.name = 'ConfigError'; + this.issues = issues; } } + +export function toConfigIssue(error: ValueError): ConfigIssue { + const segments = error.path.split('/').slice(1); + let message = error.message; + if (error.type === ValueErrorType.ObjectAdditionalProperties) { + segments.pop(); + const allowedFields = Object.keys(error.schema.properties ?? {}); + message = + allowedFields.length > 0 + ? `Unexpected property. Allowed fields: ${allowedFields.join(', ')}` + : 'Unexpected property'; + } + if (segments[0] === 'domains') { + if (segments.length > 1) segments[1] = '*'; + if (segments[2] === 'links' && segments.length > 3) segments[3] = '*'; + } + return { path: `/${segments.join('/')}`, message }; +} + +export function formatConfigError(error: ConfigError, headline = error.message): string { + return [headline, ...error.issues.map((issue) => ` ${issue.path}: ${issue.message}`)].join('\n'); +} diff --git a/packages/config/src/index.ts b/packages/config/src/index.ts index 36b8999..c7197c6 100644 --- a/packages/config/src/index.ts +++ b/packages/config/src/index.ts @@ -1,4 +1,4 @@ -export { ConfigError } from './errors.js'; +export { ConfigError, formatConfigError, type ConfigIssue } from './errors.js'; export { loadConfigFile, parseConfig, validateConfig } from './load.js'; export { startConfigReloading } from './monitor.js'; export { isSelfRedirect } from './redirect.js'; diff --git a/packages/config/src/load.test.ts b/packages/config/src/load.test.ts index 0e2f49b..d6ee333 100644 --- a/packages/config/src/load.test.ts +++ b/packages/config/src/load.test.ts @@ -12,7 +12,7 @@ const yaml = 'auth: {credentials: {username: admin, password: test-password}}\nd const authoredKeyMarker = 'dummy-authored-key-marker'; describe('configuration loading', () => { - it('parses YAML 1.2 and applies root defaults without mutating the input', () => { + it('parses YAML and applies root defaults without mutating the input', () => { expect(parseConfig(`${yaml}\nsite: {title: yes}`, options).configuration.site.title).toBe( 'yes' ); @@ -86,14 +86,18 @@ describe('configuration loading', () => { expect(configuration.auth.secret).toBe(env.SECRET); expect(() => validateConfig(input, { env: { ...env, WORKER_BASE: 'http://worker.example.com?secret' } }) - ).toThrowError(new ConfigError()); + ).toThrowError( + expect.objectContaining({ constructor: ConfigError, message: 'Invalid configuration' }) + ); for (const destination of [ 'https://private:secret@example.com', 'https://example.com/\r\nsecret' ]) { expect(() => validateConfig(input, { env: { ...env, DESTINATION: destination } }) - ).toThrowError(new ConfigError()); + ).toThrowError( + expect.objectContaining({ constructor: ConfigError, message: 'Invalid configuration' }) + ); } expect(() => validateConfig({ ...input, auth: { ...input.auth, secret: '${MISSING}' } }, { env }) @@ -247,7 +251,6 @@ describe('configuration loading', () => { `${yaml}\nversion: 1\nversion: 1`, `${yaml}\n<<: {version: 1}`, `${yaml}\n---\n${yaml}`, - `%YAML 1.1\n---\n${yaml}`, `${yaml}\nsite: &cycle {title: *cycle}` ])('rejects non-strict YAML', (source) => { expect(() => parseConfig(source, options)).toThrow(ConfigError); @@ -263,9 +266,15 @@ describe('configuration loading', () => { }); it('does not expose YAML parser snippets containing secrets', () => { - expect(() => parseConfig(`${yaml}\nauth: private-secret`, options)).toThrowError( - new ConfigError() - ); + let error: unknown; + try { + parseConfig(`${yaml}\nauth: private-secret`, options); + } catch (caught) { + error = caught; + } + expect(error).toBeInstanceOf(ConfigError); + expect(error).toHaveProperty('message', 'Invalid configuration'); + expect(JSON.stringify(error)).not.toContain('private-secret'); }); it('allows small aliases but bounds repeated expansion', () => { @@ -350,7 +359,9 @@ describe('configuration loading', () => { expect(explicit).toEqual(loaded); await expect( loadConfigFile('missing-private-file.yaml', { cwd: root, env: {} }) - ).rejects.toThrowError(new ConfigError()); + ).rejects.toThrowError( + expect.objectContaining({ constructor: ConfigError, message: 'Invalid configuration' }) + ); } finally { await rm(root, { recursive: true, force: true }); } diff --git a/packages/config/src/load.ts b/packages/config/src/load.ts index 2a5a0c2..b0783e0 100644 --- a/packages/config/src/load.ts +++ b/packages/config/src/load.ts @@ -1,8 +1,8 @@ import { readFile } from 'node:fs/promises'; import { resolve } from 'node:path'; import { Value } from '@sinclair/typebox/value'; -import { parseAllDocuments } from 'yaml'; -import { ConfigError } from './errors.js'; +import { LineCounter, parseAllDocuments } from 'yaml'; +import { ConfigError, toConfigIssue } from './errors.js'; import { assertArrayShapesPreserved, assertSafeInput } from './input.js'; import { interpolate } from './interpolate.js'; import { resolveConfigPath, resolveDeploymentRoot } from './paths.js'; @@ -20,7 +20,7 @@ export async function loadConfigFile( try { source = await readFile(configPath, 'utf8'); } catch { - throw new ConfigError(); + throw new ConfigError([{ path: '/', message: 'Unable to read configuration file' }]); } const prepared = parseConfig(source, options); const deploymentRoot = resolveDeploymentRoot(configPath); @@ -33,8 +33,9 @@ export async function loadConfigFile( export function parseConfig(source: string, options: ConfigOptions = {}): PreparedConfiguration { let raw: unknown; try { + const lineCounter = new LineCounter(); const documents = parseAllDocuments(source, { - version: '1.2', + lineCounter, schema: 'core', stringKeys: true, uniqueKeys: true, @@ -43,17 +44,23 @@ export function parseConfig(source: string, options: ConfigOptions = {}): Prepar logLevel: 'silent' }); const document = documents[0]; - if ( - documents.length !== 1 || - !document || - document.errors.length > 0 || - document.warnings.length > 0 || - document.directives?.yaml.version !== '1.2' - ) { - throw new ConfigError(); + if (documents.length !== 1 || !document) { + throw new ConfigError([{ path: '/', message: 'Expected one YAML document' }]); + } + const yamlErrors = [...document.errors, ...document.warnings]; + if (yamlErrors.length > 0) { + const issues = yamlErrors.map((error) => { + const { line, col } = lineCounter.linePos(error.pos[0]); + return { + path: '/', + message: `Invalid YAML (${error.code}) at line ${line}, column ${col}` + }; + }); + throw new ConfigError(issues); } raw = document.toJS({ maxAliasCount: 100 }); - } catch { + } catch (error) { + if (error instanceof ConfigError) throw error; throw new ConfigError(); } return validateConfig(raw, options); @@ -69,7 +76,10 @@ export function validateConfig(raw: unknown, options: ConfigOptions = {}): Prepa const defaulted = Value.Default(GotoConfigurationSchema, configurationReferences, expanded); assertArrayShapesPreserved(raw, defaulted); if (!Value.Check(GotoConfigurationSchema, configurationReferences, defaulted)) { - throw new ConfigError(); + const issues = [ + ...Value.Errors(GotoConfigurationSchema, configurationReferences, defaulted) + ].map(toConfigIssue); + throw new ConfigError(issues); } const configuration = defaulted; const authoredDefaults = diff --git a/packages/config/src/resolve.test.ts b/packages/config/src/resolve.test.ts index 8c697d3..9d51fc2 100644 --- a/packages/config/src/resolve.test.ts +++ b/packages/config/src/resolve.test.ts @@ -185,7 +185,9 @@ describe('effective domains', () => { configurationInput({ '/docs': { url: 'https://example.com', [field]: instant } }), options ) - ).toThrowError(new ConfigError()); + ).toThrowError( + expect.objectContaining({ constructor: ConfigError, message: 'Invalid configuration' }) + ); } } ); -- 2.54.0