diff --git a/docs/docs/configure/mcp-servers.md b/docs/docs/configure/mcp-servers.md index 59547953f..e1c50d5d4 100644 --- a/docs/docs/configure/mcp-servers.md +++ b/docs/docs/configure/mcp-servers.md @@ -20,6 +20,21 @@ Run an MCP server as a local subprocess: } ``` +### Environment variable interpolation + +Both syntaxes work anywhere in the config: + +| Syntax | Injection mode | Example | +|--------|----------------|---------| +| `${VAR}` | String-safe (JSON-escaped) | `"API_KEY": "${MY_API_KEY}"` — shell / dotenv style | +| `${VAR:-default}` | String-safe with fallback | `"MODE": "${APP_MODE:-production}"` — used when `VAR` is unset or empty | +| `{env:VAR}` | Raw text | `"count": {env:NUM}` — use for unquoted structural injection | +| `$${VAR}` | Escape hatch | `"template": "$${VAR}"` — preserves literal `${VAR}` (docker-compose style) | + +If the variable is not set and no default is given, it resolves to an empty string. Bare `$VAR` (without braces) is **not** interpolated — use `${VAR}` or `{env:VAR}`. + +**Why two syntaxes?** `${VAR}` JSON-escapes the value so tokens containing quotes or braces can't break the config structure — the safe default for secrets. `{env:VAR}` does raw text injection for the rare case where you need to inject numbers or structure into unquoted JSON positions. + | Field | Type | Description | |-------|------|-------------| | `type` | `"local"` | Local subprocess server | diff --git a/packages/opencode/src/altimate/telemetry/index.ts b/packages/opencode/src/altimate/telemetry/index.ts index c30b0c2f0..33d296e83 100644 --- a/packages/opencode/src/altimate/telemetry/index.ts +++ b/packages/opencode/src/altimate/telemetry/index.ts @@ -659,6 +659,25 @@ export namespace Telemetry { error_message?: string } // altimate_change end + // altimate_change start — config env-var interpolation telemetry + | { + type: "config_env_interpolation" + timestamp: number + session_id: string + /** ${VAR} / ${VAR:-default} references encountered */ + dollar_refs: number + /** ${VAR} with no value and no default → resolved to empty string (footgun signal) */ + dollar_unresolved: number + /** ${VAR:-default} where default was used */ + dollar_defaulted: number + /** $${VAR} literal escape sequences found */ + dollar_escaped: number + /** legacy {env:VAR} references (raw injection syntax) */ + legacy_brace_refs: number + /** {env:VAR} with no value → empty string */ + legacy_brace_unresolved: number + } + // altimate_change end // altimate_change start — plan-agent model tool-call refusal detection | { type: "plan_no_tool_generation" diff --git a/packages/opencode/src/cli/cmd/tui/component/tips.tsx b/packages/opencode/src/cli/cmd/tui/component/tips.tsx index 818edfed0..4f30372b3 100644 --- a/packages/opencode/src/cli/cmd/tui/component/tips.tsx +++ b/packages/opencode/src/cli/cmd/tui/component/tips.tsx @@ -147,7 +147,7 @@ const TIPS = [ "Create JSON theme files in {highlight}.altimate-code/themes/{/highlight} directory", "Themes support dark/light variants for both modes", "Reference ANSI colors 0-255 in custom themes", - "Use {highlight}{env:VAR_NAME}{/highlight} syntax to reference environment variables in config", + "Use {highlight}${VAR_NAME}{/highlight} or {highlight}{env:VAR_NAME}{/highlight} to reference environment variables in config", "Use {highlight}{file:path}{/highlight} to include file contents in config values", "Use {highlight}instructions{/highlight} in config to load additional rules files", "Set agent {highlight}temperature{/highlight} from 0.0 (focused) to 1.0 (creative)", diff --git a/packages/opencode/src/config/paths.ts b/packages/opencode/src/config/paths.ts index 1629df517..f0526ff62 100644 --- a/packages/opencode/src/config/paths.ts +++ b/packages/opencode/src/config/paths.ts @@ -86,9 +86,69 @@ export namespace ConfigPaths { /** Apply {env:VAR} and {file:path} substitutions to config text. */ async function substitute(text: string, input: ParseSource, missing: "error" | "empty" = "error") { - text = text.replace(/\{env:([^}]+)\}/g, (_, varName) => { - return process.env[varName] || "" - }) + // altimate_change start — unified env-var interpolation + // Single-pass substitution against the ORIGINAL text prevents output of one + // pattern being re-matched by another (e.g. {env:A}="${B}" expanding B). + // Syntaxes (order tried, in one regex via alternation): + // 1. $${VAR} or $${VAR:-default} — literal escape (docker-compose style) + // 2. ${VAR} or ${VAR:-default} — string-safe, JSON-escaped (shell/dotenv) + // 3. {env:VAR} — raw text injection (backward compat) + // Users arriving from Claude Code / VS Code / dotenv / docker-compose expect + // ${VAR}. Use {env:VAR} for raw unquoted injection. See issue #635. + let dollarRefs = 0 + let dollarUnresolved = 0 + let dollarDefaulted = 0 + let dollarEscaped = 0 + let legacyBraceRefs = 0 + let legacyBraceUnresolved = 0 + text = text.replace( + /\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})|(? { + if (escaped !== undefined) { + // $${VAR} → literal ${VAR} + dollarEscaped++ + return "$" + escaped + } + if (dollarVar !== undefined) { + // ${VAR} / ${VAR:-default} → JSON-escaped string-safe substitution + dollarRefs++ + const envValue = process.env[dollarVar] + const resolved = envValue !== undefined && envValue !== "" + if (!resolved && dollarDefault !== undefined) dollarDefaulted++ + if (!resolved && dollarDefault === undefined) dollarUnresolved++ + const value = resolved ? envValue : (dollarDefault ?? "") + return JSON.stringify(value).slice(1, -1) + } + if (braceVar !== undefined) { + // {env:VAR} → raw text injection + legacyBraceRefs++ + const v = process.env[braceVar] + if (v === undefined || v === "") legacyBraceUnresolved++ + return v || "" + } + return match + }, + ) + // Emit telemetry if any env interpolation happened. Dynamic import avoids a + // circular dep with @/altimate/telemetry (which imports @/config/config). + if (dollarRefs > 0 || legacyBraceRefs > 0 || dollarEscaped > 0) { + import("@/altimate/telemetry") + .then(({ Telemetry }) => { + Telemetry.track({ + type: "config_env_interpolation", + timestamp: Date.now(), + session_id: Telemetry.getContext().sessionId, + dollar_refs: dollarRefs, + dollar_unresolved: dollarUnresolved, + dollar_defaulted: dollarDefaulted, + dollar_escaped: dollarEscaped, + legacy_brace_refs: legacyBraceRefs, + legacy_brace_unresolved: legacyBraceUnresolved, + }) + }) + .catch(() => {}) + } + // altimate_change end const fileMatches = Array.from(text.matchAll(/\{file:[^}]+\}/g)) if (!fileMatches.length) return text diff --git a/packages/opencode/test/config/paths-parsetext.test.ts b/packages/opencode/test/config/paths-parsetext.test.ts index b3c8d92c8..b91d48562 100644 --- a/packages/opencode/test/config/paths-parsetext.test.ts +++ b/packages/opencode/test/config/paths-parsetext.test.ts @@ -97,6 +97,210 @@ describe("ConfigPaths.parseText: {env:VAR} substitution", () => { }) }) +describe("ConfigPaths.parseText: ${VAR} substitution (shell/dotenv alias)", () => { + const envKey = "OPENCODE_TEST_SHELL_SYNTAX_KEY" + + beforeEach(() => { + process.env[envKey] = "shell-style-value" + }) + + afterEach(() => { + delete process.env[envKey] + }) + + test("substitutes ${VAR} with environment variable value", async () => { + const text = `{"apiKey": "\${${envKey}}"}` + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result).toEqual({ apiKey: "shell-style-value" }) + }) + + test("substitutes to empty string when env var is not set", async () => { + const text = '{"apiKey": "${OPENCODE_TEST_SHELL_NONEXISTENT_XYZ}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result).toEqual({ apiKey: "" }) + }) + + test("${VAR} and {env:VAR} both work in same config", async () => { + process.env.OPENCODE_TEST_MIXED_A = "alpha" + process.env.OPENCODE_TEST_MIXED_B = "beta" + try { + const text = '{"a": "${OPENCODE_TEST_MIXED_A}", "b": "{env:OPENCODE_TEST_MIXED_B}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result).toEqual({ a: "alpha", b: "beta" }) + } finally { + delete process.env.OPENCODE_TEST_MIXED_A + delete process.env.OPENCODE_TEST_MIXED_B + } + }) + + test("ignores ${...} with non-identifier names (spaces, special chars)", async () => { + // These should pass through unmodified — not valid POSIX identifiers + const text = '{"a": "${FOO BAR}", "b": "${foo-bar}", "c": "${foo.bar}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result).toEqual({ a: "${FOO BAR}", b: "${foo-bar}", c: "${foo.bar}" }) + }) + + test("does not match bare $VAR (without braces)", async () => { + process.env.OPENCODE_TEST_BARE = "should-not-match" + try { + const text = '{"value": "$OPENCODE_TEST_BARE"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + // Bare $VAR stays literal — only ${VAR} is interpolated + expect(result).toEqual({ value: "$OPENCODE_TEST_BARE" }) + } finally { + delete process.env.OPENCODE_TEST_BARE + } + }) + + test("JSON-safe: env value with quotes cannot inject JSON structure", async () => { + // Security regression test for C1 in consensus review of PR #655. + // {env:VAR} is raw injection (backward compat); ${VAR} is string-safe. + process.env.OPENCODE_TEST_INJECT = 'pwned", "isAdmin": true, "x": "y' + try { + const text = '{"token": "${OPENCODE_TEST_INJECT}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + // Value stays inside the "token" string — no injection into sibling keys + expect(result).toEqual({ token: 'pwned", "isAdmin": true, "x": "y' }) + expect(result.isAdmin).toBeUndefined() + } finally { + delete process.env.OPENCODE_TEST_INJECT + } + }) + + test("JSON-safe: env value with backslash and newline escaped properly", async () => { + process.env.OPENCODE_TEST_MULTILINE = 'line1\nline2\tpath\\to\\file' + try { + const text = '{"value": "${OPENCODE_TEST_MULTILINE}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result).toEqual({ value: "line1\nline2\tpath\\to\\file" }) + } finally { + delete process.env.OPENCODE_TEST_MULTILINE + } + }) + + test("default: ${VAR:-default} uses default when var unset", async () => { + // Variable is not set — default value should be used + const text = '{"mode": "${OPENCODE_TEST_UNSET_VAR:-production}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result).toEqual({ mode: "production" }) + }) + + test("default: ${VAR:-default} uses env value when var set", async () => { + process.env.OPENCODE_TEST_DEFAULT_OVERRIDE = "staging" + try { + const text = '{"mode": "${OPENCODE_TEST_DEFAULT_OVERRIDE:-production}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result).toEqual({ mode: "staging" }) + } finally { + delete process.env.OPENCODE_TEST_DEFAULT_OVERRIDE + } + }) + + test("default: ${VAR:-default} uses default when var is empty string", async () => { + // POSIX :- uses default for both unset AND empty (matches docker-compose) + process.env.OPENCODE_TEST_EMPTY_VAR = "" + try { + const text = '{"mode": "${OPENCODE_TEST_EMPTY_VAR:-fallback}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result).toEqual({ mode: "fallback" }) + } finally { + delete process.env.OPENCODE_TEST_EMPTY_VAR + } + }) + + test("default: empty default ${VAR:-} resolves to empty string", async () => { + const text = '{"value": "${OPENCODE_TEST_EMPTY_DEFAULT:-}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result).toEqual({ value: "" }) + }) + + test("default: default value with spaces and special chars", async () => { + const text = '{"msg": "${OPENCODE_TEST_MISSING:-Hello World 123}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result).toEqual({ msg: "Hello World 123" }) + }) + + test("default: default value is JSON-escaped (security)", async () => { + const text = '{"token": "${OPENCODE_TEST_MISSING:-pwned\\", \\"isAdmin\\": true, \\"x\\": \\"y}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result.token).toContain("pwned") + expect(result.isAdmin).toBeUndefined() + }) + + test("escape hatch: $${VAR:-default} stays literal", async () => { + process.env.OPENCODE_TEST_ESCAPED_DEFAULT = "should-not-be-used" + try { + const text = '{"template": "$${OPENCODE_TEST_ESCAPED_DEFAULT:-my-default}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result).toEqual({ template: "${OPENCODE_TEST_ESCAPED_DEFAULT:-my-default}" }) + } finally { + delete process.env.OPENCODE_TEST_ESCAPED_DEFAULT + } + }) + + test("escape hatch: $${VAR} stays literal (docker-compose convention)", async () => { + process.env.OPENCODE_TEST_SHOULD_NOT_SUB = "interpolated" + try { + const text = '{"template": "$${OPENCODE_TEST_SHOULD_NOT_SUB}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + // $${VAR} → literal ${VAR}, env value is NOT substituted + expect(result).toEqual({ template: "${OPENCODE_TEST_SHOULD_NOT_SUB}" }) + } finally { + delete process.env.OPENCODE_TEST_SHOULD_NOT_SUB + } + }) + + test("single-pass: {env:A} value containing ${B} stays literal (no cascade)", async () => { + // Regression test for cubic/coderabbit P1: previously the {env:VAR} pass ran + // first, then the ${VAR} pass expanded any ${...} in its output. Single-pass + // substitution evaluates both patterns against the ORIGINAL text only. + process.env.OPENCODE_TEST_CASCADE_A = "${OPENCODE_TEST_CASCADE_B}" + process.env.OPENCODE_TEST_CASCADE_B = "should-not-expand" + try { + const text = '{"value": "{env:OPENCODE_TEST_CASCADE_A}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + // {env:VAR} is raw injection — its output is NOT re-interpolated + expect(result.value).toBe("${OPENCODE_TEST_CASCADE_B}") + } finally { + delete process.env.OPENCODE_TEST_CASCADE_A + delete process.env.OPENCODE_TEST_CASCADE_B + } + }) + + test("single-pass: ${A} value containing {env:B} stays literal (no cascade)", async () => { + // Reverse direction: ${VAR} output must not be matched by {env:VAR} pass. + process.env.OPENCODE_TEST_CASCADE_C = "{env:OPENCODE_TEST_CASCADE_D}" + process.env.OPENCODE_TEST_CASCADE_D = "should-not-expand" + try { + const text = '{"value": "${OPENCODE_TEST_CASCADE_C}"}' + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result.value).toBe("{env:OPENCODE_TEST_CASCADE_D}") + } finally { + delete process.env.OPENCODE_TEST_CASCADE_C + delete process.env.OPENCODE_TEST_CASCADE_D + } + }) + + test("works inside MCP environment config (issue #635 regression)", async () => { + process.env.OPENCODE_TEST_GITLAB_TOKEN = "glpat-xxxxx" + try { + const text = `{ + "mcp": { + "gitlab": { + "type": "local", + "command": ["npx", "-y", "@modelcontextprotocol/server-gitlab"], + "environment": { "GITLAB_TOKEN": "\${OPENCODE_TEST_GITLAB_TOKEN}" } + } + } + }` + const result = await ConfigPaths.parseText(text, "/fake/config.json") + expect(result.mcp.gitlab.environment.GITLAB_TOKEN).toBe("glpat-xxxxx") + } finally { + delete process.env.OPENCODE_TEST_GITLAB_TOKEN + } + }) +}) + describe("ConfigPaths.parseText: {file:path} substitution", () => { test("substitutes {file:path} with file contents (trimmed)", async () => { await using tmp = await tmpdir() diff --git a/packages/opencode/test/telemetry/telemetry.test.ts b/packages/opencode/test/telemetry/telemetry.test.ts index 8500ac6c3..45966c510 100644 --- a/packages/opencode/test/telemetry/telemetry.test.ts +++ b/packages/opencode/test/telemetry/telemetry.test.ts @@ -246,11 +246,12 @@ const ALL_EVENT_TYPES: Telemetry.Event["type"][] = [ "feature_suggestion", "core_failure", "sql_pre_validation", + "config_env_interpolation", ] describe("telemetry.event-types", () => { test("all event types are valid", () => { - expect(ALL_EVENT_TYPES.length).toBe(43) + expect(ALL_EVENT_TYPES.length).toBe(44) }) })