-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix(cli): block file/env references in untrusted project config #11886
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b793bf7
ac62aef
7d312ee
47da231
6dfbc54
ebb0029
1d49502
e9f6b02
47a40fe
faa2bae
c7f0ccf
10e519d
e06feff
8dad071
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@kilocode/cli": patch | ||
| --- | ||
|
|
||
| Harden config credential substitution against untrusted project config. Environment references (`{env:VAR}`) now resolve only in trusted config (global config, `KILO_CONFIG`, `KILO_CONFIG_CONTENT`, and org/MDM-managed config); a project-committed `kilo.json` / `opencode.json` can no longer use them. File references (`{file:...}`) still work in project config but are confined to the project root, so absolute paths, `../` traversal, and symlink escapes are rejected. This closes a path where a malicious repository could exfiltrate local secrets to an attacker-controlled `baseURL`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,8 +134,8 @@ export const Info = AgentSchema.pipe( | |
| ).annotate({ identifier: "AgentConfig" }) | ||
| export type Info = Schema.Schema.Type<typeof Info> | ||
|
|
||
| // kilocode_change start | ||
| export async function load(dir: string, warnings?: Warning[]) { | ||
| // kilocode_change start - trusted gates {env:}; fileScope confines untrusted agent prompt {file:} reads | ||
| export async function load(dir: string, warnings?: Warning[], trusted?: boolean, fileScope?: ConfigVariable.FileScope) { | ||
| // kilocode_change end | ||
| const result: Record<string, Info> = {} | ||
| for (const item of await Glob.scan("{agent,agents}/**/*.md", { | ||
|
|
@@ -168,15 +168,27 @@ export async function load(dir: string, warnings?: Warning[]) { | |
|
|
||
| const name = configEntryNameFromPath(path.relative(dir, item), ["agent/", "agents/"]) | ||
|
|
||
| // kilocode_change start - substitute agent prompt variables relative to the agent file | ||
| // kilocode_change start - substitute agent prompt variables relative to the agent file. Project agents are | ||
| // untrusted (no {env:}, {file:} confined to fileScope.root); a rejected substitution must skip only this | ||
|
markijbema marked this conversation as resolved.
|
||
| // agent with a warning, not fail the whole config load, mirroring the frontmatter-parse handling above. | ||
| const prompt = await ConfigVariable.substitute({ | ||
| text: md.content.trim(), | ||
| type: "virtual", | ||
| dir: path.dirname(item), | ||
| source: item, | ||
| missing: "empty", | ||
| escapeJson: false, | ||
| trusted, | ||
| fileScope, | ||
| }).catch((err): string | undefined => { | ||
| const message = | ||
| (ConfigError.InvalidError.isInstance(err) ? err.data.message : undefined) ?? | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Soft-fail {env:} instead of throwing. Right now an untrusted {env:VAR} throws InvalidError, which caughtWarning turns into "skip the entire config file." A repo with one {env:} token loses all its project config (agents, MCP servers, everything) with only a warning. Security-wise, leaving the token unresolved (literal {env:VAR} string, or empty) plus a warning is exactly as safe — no secret is read — and strictly better UX. The MCP docs (using-in-cli.md:170) make project-level {env:} in headers a plausible existing pattern, so this failure mode will generate confused bug reports. Note the {file:} out-of-scope case is different: there a hard BlockedError is right, since silently emptying could mask an attack; and it only kills that one substitution path anyway. |
||
| `Failed to substitute variables in agent ${item}` | ||
| if (warnings) warnings.push({ path: item, message }) | ||
| log.error("failed to substitute agent prompt", { agent: item, err }) | ||
| return undefined | ||
| }) | ||
| if (prompt === undefined) continue | ||
| const config = { | ||
| name, | ||
| ...md.data, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.