mirror of
https://github.com/CherryHQ/cherry-studio.git
synced 2026-07-08 08:27:39 +08:00
fix(code-cli): fix dotenv escaping and redaction gaps found in re-review of 4fc8948
Re-review of the previous CLI-config fix batch (4fc8948) surfaced three
regressions introduced by that batch itself, plus two sinks the secret
redaction still missed:
- dotenv.ts: quoting a value that needed escaping (contains #, quotes, or
leading/trailing whitespace) injected \\ and \" escapes that the real
`dotenv` package (used by gemini-cli to read the file back) never
unescapes — it only re-expands \n/\r inside double-quoted values. This
silently corrupted Windows-style paths (doubled backslashes) and any
value containing a literal double quote. Fixed by preferring a
single-quoted value (read back 100% literally) whenever it has no
embedded single quote, and only escaping \n/\r in the double-quote
fallback.
- redact.ts: the value-matching regex stopped at the first embedded
apostrophe in a double-quoted value, or at an already-broken/empty
quoted pair, leaving the real secret exposed as an unredacted trailing
fragment in both cases. Fixed by matching the rest of the source line
(smol-toml embeds raw source lines verbatim in its own error messages)
instead of a single quoted token.
- Centralized secret redaction inside parseTomlOrThrow/parseJsonOrThrow
themselves (instead of only at the two call sites that remembered to
wrap it), closing two previously-unredacted sinks:
validateCliConfigDraftForWrite (manual config-text editing) and
draftUpdater.ts's direct parse calls.
- Added missing assertCliConfigCredentials coverage for the
missing-base-URL branches of OpenCode/Qwen/Kimi (only the missing-API-key
branch had a test before).
Every fix has a regression test verified to fail without it; the dotenv
and redact tests exercise the real `dotenv`/`smol-toml` packages directly
rather than hand-crafted strings, to catch this class of drift going
forward.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
This commit is contained in:
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -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 = "<redacted>"/)
|
||||
expect(() => validateCliConfigDraftForWrite(files)).not.toThrow(/sk-ant-real-secret/)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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")', () => {
|
||||
|
||||
@@ -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, string>): string {
|
||||
|
||||
@@ -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<T>(
|
||||
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}`)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -50,8 +50,10 @@ function parseOrThrow<T>(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<string, any> {
|
||||
if (!content) return {}
|
||||
return parseToml(content) as Record<string, any>
|
||||
try {
|
||||
return parseToml(content) as Record<string, any>
|
||||
} 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<string, any> {
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user