diff --git a/src/renderer/pages/code/cliConfig/__tests__/dotenv.test.ts b/src/renderer/pages/code/cliConfig/__tests__/dotenv.test.ts index 797be16521..2c94baf84f 100644 --- a/src/renderer/pages/code/cliConfig/__tests__/dotenv.test.ts +++ b/src/renderer/pages/code/cliConfig/__tests__/dotenv.test.ts @@ -1,3 +1,4 @@ +import { parse as parseWithRealDotenv } from 'dotenv' import { describe, expect, it } from 'vitest' import { parseDotenv, renderDotenvFile } from '../dotenv' @@ -7,22 +8,51 @@ describe('renderDotenvFile', () => { expect(renderDotenvFile(new Map([['KEY', 'value']]))).toBe('KEY=value\n') }) - it('quotes a value containing #', () => { + it('single-quotes a value containing # (single quotes are read back 100% literally)', () => { expect(renderDotenvFile(new Map([['HTTPS_PROXY', 'http://user:p#ss@host']]))).toBe( - 'HTTPS_PROXY="http://user:p#ss@host"\n' + "HTTPS_PROXY='http://user:p#ss@host'\n" ) }) - it('quotes a value with leading/trailing whitespace', () => { - expect(renderDotenvFile(new Map([['KEY', ' value ']]))).toBe('KEY=" value "\n') + it('single-quotes a value with leading/trailing whitespace', () => { + expect(renderDotenvFile(new Map([['KEY', ' value ']]))).toBe("KEY=' value '\n") }) - it('quotes and escapes a value containing a double quote', () => { - expect(renderDotenvFile(new Map([['KEY', 'say "hi"']]))).toBe('KEY="say \\"hi\\""\n') + it('single-quotes a value containing a double quote instead of escaping it', () => { + // The real `dotenv` package only re-expands `\n`/`\r` in double-quoted values on read — it never + // unescapes `\"`, so a `\"`-escaped double-quoted value would come back with the backslash intact. + expect(renderDotenvFile(new Map([['KEY', 'say "hi"']]))).toBe(`KEY='say "hi"'\n`) }) - it('quotes an empty value', () => { - expect(renderDotenvFile(new Map([['KEY', '']]))).toBe('KEY=""\n') + it('single-quotes an empty value', () => { + expect(renderDotenvFile(new Map([['KEY', '']]))).toBe("KEY=''\n") + }) + + it('falls back to double-quoting (unescaped) a value containing a single quote', () => { + expect(renderDotenvFile(new Map([['KEY', "it's fine"]]))).toBe(`KEY="it's fine"\n`) + }) + + it('does not double a backslash in a Windows-style path value', () => { + // Regression: escaping `\` to `\\` here corrupted the value on read-back, since the real + // `dotenv` package never unescapes `\\` — it would come back with twice as many backslashes. + expect(renderDotenvFile(new Map([['KEY', 'C:\\Users\\me']]))).toBe("KEY='C:\\Users\\me'\n") + }) +}) + +describe('renderDotenvFile output round-trips through the real dotenv package', () => { + const cases = [ + 'plain-value', + 'http://user:p#ss@host', + ' leading-and-trailing ', + 'say "hi"', + '', + 'C:\\Users\\me', + 'sk-proj-\\backslash\\and-hash#mixed' + ] + + it.each(cases)('renders %j so the real dotenv package reads it back unchanged', (value) => { + const rendered = renderDotenvFile(new Map([['KEY', value]])) + expect(parseWithRealDotenv(rendered).KEY).toBe(value) }) }) diff --git a/src/renderer/pages/code/cliConfig/__tests__/draftFiles.test.ts b/src/renderer/pages/code/cliConfig/__tests__/draftFiles.test.ts index 0cc8499dee..d75e5cf3f2 100644 --- a/src/renderer/pages/code/cliConfig/__tests__/draftFiles.test.ts +++ b/src/renderer/pages/code/cliConfig/__tests__/draftFiles.test.ts @@ -1,7 +1,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -import { readAndParseDraftFile } from '../draftFiles' +import { readAndParseDraftFile, validateCliConfigDraftForWrite } from '../draftFiles' import { parseTomlOrThrow } from '../file' +import type { CliConfigFileDraft } from '../types' describe('readAndParseDraftFile (secret redaction on parse failure)', () => { beforeEach(() => { @@ -23,3 +24,19 @@ describe('readAndParseDraftFile (secret redaction on parse failure)', () => { await expect(readAndParseDraftFile('kimi-config', parseTomlOrThrow)).rejects.not.toThrow(/sk-ant-real-secret/) }) }) + +describe('validateCliConfigDraftForWrite (secret redaction when editing config text directly)', () => { + it('does not leak the raw secret from a malformed in-editor TOML draft into the thrown error', () => { + const files: CliConfigFileDraft[] = [ + { + target: 'kimi-config', + label: 'Kimi config', + path: '/resolved~/.kimi-code/config.toml', + language: 'toml', + content: 'api_key = "sk-ant-real-secret"\nbroken=====' + } + ] + expect(() => validateCliConfigDraftForWrite(files)).toThrow(/api_key = ""/) + expect(() => validateCliConfigDraftForWrite(files)).not.toThrow(/sk-ant-real-secret/) + }) +}) diff --git a/src/renderer/pages/code/cliConfig/__tests__/redact.test.ts b/src/renderer/pages/code/cliConfig/__tests__/redact.test.ts index 810b8c0ccd..ff32bb4845 100644 --- a/src/renderer/pages/code/cliConfig/__tests__/redact.test.ts +++ b/src/renderer/pages/code/cliConfig/__tests__/redact.test.ts @@ -1,7 +1,19 @@ +import { parse as parseToml } from 'smol-toml' import { describe, expect, it } from 'vitest' import { redactSecretsInMessage } from '../redact' +/** Parse malformed TOML and return smol-toml's own thrown message (which embeds a raw source + * codeblock), so redaction is tested against a real error shape rather than a hand-crafted string. */ +function realTomlParseErrorMessage(malformedToml: string): string { + try { + parseToml(malformedToml) + throw new Error('expected malformed TOML to throw') + } catch (err) { + return err instanceof Error ? err.message : String(err) + } +} + describe('redactSecretsInMessage', () => { it('redacts a quoted TOML-style api_key assignment', () => { expect(redactSecretsInMessage('unexpected character at line 3: api_key = "sk-ant-real-secret"')).toBe( @@ -42,4 +54,22 @@ describe('redactSecretsInMessage', () => { const result = redactSecretsInMessage(message) expect(result).not.toContain('sk-ant-real-secret') }) + + it('redacts a secret stranded after a broken/empty quoted value on a real smol-toml error', () => { + // A missing separator splits the value into an empty quoted pair followed by the real secret as + // a bare trailing token — smol-toml's own message embeds the raw source line verbatim. + const message = realTomlParseErrorMessage('api_key = "" sk-ant-REALSECRET') + expect(message).toContain('sk-ant-REALSECRET') // sanity: the real message does leak it pre-redaction + expect(redactSecretsInMessage(message)).not.toContain('sk-ant-REALSECRET') + }) + + it('redacts a double-quoted secret containing an apostrophe on a real smol-toml error', () => { + // smol-toml's codeblock includes the line before the actual error line too, so a perfectly valid + // secret line can still end up embedded in the message when a later line is what fails to parse. + // A naive ["'][^"']*["'] value match stops at the embedded apostrophe, leaking the tail (the part + // of the secret after it) even though the quoted value read as a whole is fully redacted. + const message = realTomlParseErrorMessage(`api_key = "sk-ant-don't-SECRET"\nbroken=====`) + expect(message).toContain('SECRET') // sanity: the real message does leak it pre-redaction + expect(redactSecretsInMessage(message)).not.toContain('SECRET') + }) }) diff --git a/src/renderer/pages/code/cliConfig/__tests__/writeCliConfigDraft.test.ts b/src/renderer/pages/code/cliConfig/__tests__/writeCliConfigDraft.test.ts index 63253962a2..d39da2c9ee 100644 --- a/src/renderer/pages/code/cliConfig/__tests__/writeCliConfigDraft.test.ts +++ b/src/renderer/pages/code/cliConfig/__tests__/writeCliConfigDraft.test.ts @@ -962,6 +962,43 @@ describe('writeCliConfigDraft', () => { ).rejects.toThrow(/missing the API key/) expect(writes).toEqual([]) }) + + it('rejects opencode with an API key but no resolvable endpoint base URL, and writes nothing', async () => { + const noEndpointProvider = { id: 'noendpoint', name: 'NoEndpoint', endpointConfigs: {} } as unknown as Provider + mockGet({ + '/providers/noendpoint': () => noEndpointProvider, + '/providers/noendpoint/api-keys': () => ({ keys: [enabledKey] }), + '/models/': () => ({ id: 'some-model', contextWindow: 65536 }) + }) + await expect( + writeCliConfigDraft({ cliTool: CodeCli.OPEN_CODE, modelId: 'noendpoint::some-model' }) + ).rejects.toThrow(/missing required fields/) + expect(writes).toEqual([]) + }) + + it('rejects qwen-code with an API key but no OpenAI-compatible endpoint base URL, and writes nothing', async () => { + mockGet({ + '/providers/anthropic': () => anthropicProvider, + '/providers/anthropic/api-keys': () => ({ keys: [enabledKey] }), + '/models/': () => ({ id: 'claude-sonnet-4-5', name: 'Claude Sonnet 4.5' }) + }) + await expect( + writeCliConfigDraft({ cliTool: CodeCli.QWEN_CODE, modelId: 'anthropic::claude-sonnet-4-5' }) + ).rejects.toThrow(/missing the OpenAI endpoint base URL/) + expect(writes).toEqual([]) + }) + + it('rejects kimi-code with an API key but no OpenAI-compatible endpoint base URL, and writes nothing', async () => { + mockGet({ + '/providers/anthropic': () => anthropicProvider, + '/providers/anthropic/api-keys': () => ({ keys: [enabledKey] }), + '/models/': () => ({ id: 'claude-sonnet-4-5', contextWindow: 200000 }) + }) + await expect( + writeCliConfigDraft({ cliTool: CodeCli.KIMI_CODE, modelId: 'anthropic::claude-sonnet-4-5' }) + ).rejects.toThrow(/missing the OpenAI endpoint base URL/) + expect(writes).toEqual([]) + }) }) describe('read-error safety (a real read failure must not be treated as "file missing")', () => { diff --git a/src/renderer/pages/code/cliConfig/dotenv.ts b/src/renderer/pages/code/cliConfig/dotenv.ts index 1343415b93..8cee52035a 100644 --- a/src/renderer/pages/code/cliConfig/dotenv.ts +++ b/src/renderer/pages/code/cliConfig/dotenv.ts @@ -25,8 +25,14 @@ function needsDotenvQuoting(value: string): boolean { return value === '' || /^\s|\s$|["'#\\]/.test(value) } +// The real `dotenv` package (used by the CLI tools that read these files back) only re-expands +// literal `\n`/`\r` sequences inside a double-quoted value on read — it does NOT unescape `\\` or +// `\"`, so injecting those escapes here would corrupt the value on read-back instead of preserving +// it. A single-quoted value, by contrast, is taken back 100% literally with no escape processing at +// all, so prefer it whenever the value has no embedded single quote to conflict with the wrapper. function quoteDotenvValue(value: string): string { - return `"${value.replace(/\\/g, '\\\\').replace(/"/g, '\\"')}"` + if (!value.includes("'")) return `'${value}'` + return `"${value.replace(/\r/g, '\\r').replace(/\n/g, '\\n')}"` } export function renderDotenvFile(envMap: Map): string { diff --git a/src/renderer/pages/code/cliConfig/draftFiles.ts b/src/renderer/pages/code/cliConfig/draftFiles.ts index 2653031c6c..be156f343e 100644 --- a/src/renderer/pages/code/cliConfig/draftFiles.ts +++ b/src/renderer/pages/code/cliConfig/draftFiles.ts @@ -1,6 +1,5 @@ import { parseDotenv } from './dotenv' import { parseJsonOrThrow, parseTomlOrThrow, readExternal, resolveAbs } from './file' -import { redactSecretsInMessage } from './redact' import { CLI_CONFIG_FILE_SPECS } from './targets' import type { CliConfigFileDraft, CliConfigTarget } from './types' @@ -39,10 +38,11 @@ export async function readAndParseDraftFile( try { return parseFn(content) } catch (err) { + // parseFn (parseJsonOrThrow/parseTomlOrThrow) already redacts its own message at the source. const spec = CLI_CONFIG_FILE_SPECS[target] const path = getDraftFile(files, target)?.path ?? (await resolveAbs(spec.path)) const rawMessage = err instanceof Error ? err.message : String(err) - throw new Error(`Failed to parse ${spec.label} at ${path}: ${redactSecretsInMessage(rawMessage)}`) + throw new Error(`Failed to parse ${spec.label} at ${path}: ${rawMessage}`) } } diff --git a/src/renderer/pages/code/cliConfig/file.ts b/src/renderer/pages/code/cliConfig/file.ts index 7e2fcab8b1..28842d02da 100644 --- a/src/renderer/pages/code/cliConfig/file.ts +++ b/src/renderer/pages/code/cliConfig/file.ts @@ -50,8 +50,10 @@ function parseOrThrow(content: string, label: string, absPath: string, parseF try { return parseFn(content) } catch (err) { + // parseFn (parseJsonOrThrow/parseTomlOrThrow) already redacts its own message at the source, so + // every caller — this wrapper, validateCliConfigDraftForWrite, draftUpdater.ts — is covered. const rawMessage = err instanceof Error ? err.message : String(err) - throw new Error(`Failed to parse ${label} at ${absPath}: ${redactSecretsInMessage(rawMessage)}`) + throw new Error(`Failed to parse ${label} at ${absPath}: ${rawMessage}`) } } @@ -79,7 +81,14 @@ export async function readValidatedTomlOrNull(absPath: string, label: string): P export function parseTomlOrThrow(content: string): Record { if (!content) return {} - return parseToml(content) as Record + try { + return parseToml(content) as Record + } catch (err) { + // smol-toml embeds a source codeblock (the offending line +/- 1) straight into its own message, + // so this must be redacted right here — every call site (direct or through parseOrThrow) inherits it. + const rawMessage = err instanceof Error ? err.message : String(err) + throw new Error(redactSecretsInMessage(rawMessage)) + } } export function parseJsonOrThrow(content: string): Record { diff --git a/src/renderer/pages/code/cliConfig/redact.ts b/src/renderer/pages/code/cliConfig/redact.ts index 90058c372c..ff9aa817e2 100644 --- a/src/renderer/pages/code/cliConfig/redact.ts +++ b/src/renderer/pages/code/cliConfig/redact.ts @@ -1,10 +1,13 @@ const BEARER_TOKEN_PATTERN = /\bBearer\s+\S+/gi -// Triple-quoted alternatives must come before the single-quote-pair alternative — otherwise that -// alternative matches only up to the first quote inside the triple-quoted block, leaving the rest -// of a multiline TOML secret unredacted. +// Triple-quoted alternatives must come before the bare-value fallback — otherwise a multiline TOML +// secret is only redacted up to the first embedded newline. The bare-value fallback intentionally +// consumes the rest of the line (not just one token): a malformed source line can put the real +// secret past a broken/empty quoted value (e.g. `api_key = "" sk-real-secret`), or inside a +// double-quoted value containing an apostrophe (e.g. `api_key = "sk-real's-secret"`) — matching only +// a single quoted pair or token would leave those trailing fragments unredacted. const SENSITIVE_MESSAGE_PATTERN = - /(["']?(?:api[_-]?key|token|secret|password|auth)\w*["']?\s*[:=]\s*)("""[\s\S]*?"""|'''[\s\S]*?'''|["'][^"']*["']|\S+)/gi + /(["']?(?:api[_-]?key|token|secret|password|auth)\w*["']?\s*[:=]\s*)("""[\s\S]*?"""|'''[\s\S]*?'''|[^\r\n]*)/gi /** Redact likely-sensitive key=value / "key": value / Bearer-token fragments embedded in a raw parser error message. */ export function redactSecretsInMessage(message: string): string {