fix: sanitize shell-derived env values before building auth headers#398
Conversation
KYankee6
requested review from
davidarny,
robinebers and
validatedev
as code owners
OpenUsage falls back to reading whitelisted env vars from an interactive shell when the GUI process does not inherit them directly. Some zsh setups emit ANSI/control sequences on stdout during startup, which can stick to the resolved API key and make the Authorization header invalid. This change wraps shell reads in explicit start/end markers, sanitizes resolved env values, and adds regression tests for noisy shell output and inline terminal control sequences. Constraint: GUI launches may not inherit terminal-only env vars Constraint: Interactive shell startup output can contain ANSI/control bytes Rejected: Restrict fix to the Z.ai plugin | other providers share the same host env resolver Rejected: Rely on shell config hygiene alone | the app still needs defensive parsing Confidence: high Scope-risk: narrow Reversibility: clean Directive: Treat shell stdout as untrusted whenever it is used as structured data Tested: bun run build Tested: bun run test Tested: cargo test --manifest-path src-tauri/Cargo.toml Tested: cargo check --manifest-path src-tauri/Cargo.toml Not-tested: bun tauri dev manual UI verification Related: robinebers#358
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/plugin_engine/host_api.rs">
<violation number="1" location="src-tauri/src/plugin_engine/host_api.rs:84">
P1: Fallback parsing can misinterpret marker output for empty env vars and return `__OPENUSAGE_ENV_END__` as the value.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
KYankee6
force-pushed
the
fix/zai-env-header-noise
branch
from
050513d to
5a7de06
Compare
Interactive-shell env reads use start/end markers so GUI launches can recover terminal-only variables. An empty env var produced a complete marker block with no payload, but the parser treated that the same as a marker miss and fell back to whole-output parsing. That fallback could select the end marker itself as the resolved value. This change distinguishes complete marker blocks from true fallback cases, returning None for empty marked output while preserving the no-marker sanitization path. Constraint: GUI env recovery still needs a no-marker fallback for noisy shell output Constraint: Empty env vars must stay empty instead of becoming marker text Rejected: Remove fallback parsing entirely | would regress shell-noise recovery for GUI launches Rejected: Special-case only the exact end marker string | leaves the real state distinction ambiguous Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep marker-aware parsing separate from generic stdout sanitization so empty payloads stay empty Tested: cargo test --manifest-path src-tauri/Cargo.toml parse_interactive_shell_env_output -- --nocapture Tested: cargo test --manifest-path src-tauri/Cargo.toml extract_marked_value -- --nocapture Tested: cargo check --manifest-path src-tauri/Cargo.toml Not-tested: bun tauri dev manual UI verification Related: robinebers#358 Related: robinebers#398
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Hardens the host-side environment variable resolver to prevent interactive-shell output (ANSI/control noise) from contaminating API keys and producing invalid Authorization headers (notably impacting the Z.ai provider).
Changes:
- Adds start/end markers around interactive-shell env reads and parses only the marked payload.
- Introduces sanitization to strip ANSI/control characters and normalize env values for both process and shell-derived reads.
- Adds regression tests covering noisy shell output, inline control bytes, and empty marked payload handling.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut cleaned = if let Ok(ansi_re) = regex_lite::Regex::new(r"\x1B\[[0-?]*[ -/]*[@-~]") { | ||
| ansi_re.replace_all(text, "").to_string() | ||
| } else { | ||
| Some(trimmed.to_string()) | ||
| text.to_string() | ||
| }; |
There was a problem hiding this comment.
The ANSI/escape stripping regex only removes CSI sequences (ESC [ ...), but other common terminal escape sequences (e.g., OSC: ESC ] ... BEL / ESC \, single-char ESC sequences like ESC ( B) can survive. Because retain then drops the ESC control byte but leaves the rest of the sequence’s printable payload, sanitize_env_value can still return a contaminated key (e.g., "]0;title" prefix). Consider expanding sanitization to remove OSC and other ESC sequences (or stripping any ESC-initiated sequence) before filtering control chars.
| fn sanitize_env_value(text: &str) -> Option<String> { | ||
| let mut cleaned = if let Ok(ansi_re) = regex_lite::Regex::new(r"\x1B\[[0-?]*[ -/]*[@-~]") { | ||
| ansi_re.replace_all(text, "").to_string() |
There was a problem hiding this comment.
sanitize_env_value recompiles the same regex on every call. Since env resolution can happen frequently (and is used by both process and interactive-shell reads), consider compiling the regex once (e.g., via OnceLock/Lazy) and reusing it to avoid repeated compilation overhead.
| let script = format!( | ||
| "printf '{}\\n'; printenv {}; printf '{}\\n'", | ||
| START_MARKER, name, END_MARKER | ||
| ); |
There was a problem hiding this comment.
The interactive-shell script interpolates name directly into the command string (printenv {}) without quoting/escaping. Today this is mitigated by the JS-side whitelist, but this helper is generic and could be reused from Rust with non-whitelisted input later. Consider adding a strict env-var-name validation (e.g., [A-Z0-9_]+) before building the script and/or quoting the argument in a shell-safe way (printenv -- ...) to prevent command injection.
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Great, thank you! |
Interactive-shell env reads use start/end markers so GUI launches can recover terminal-only variables. An empty env var produced a complete marker block with no payload, but the parser treated that the same as a marker miss and fell back to whole-output parsing. That fallback could select the end marker itself as the resolved value. This change distinguishes complete marker blocks from true fallback cases, returning None for empty marked output while preserving the no-marker sanitization path. Constraint: GUI env recovery still needs a no-marker fallback for noisy shell output Constraint: Empty env vars must stay empty instead of becoming marker text Rejected: Remove fallback parsing entirely | would regress shell-noise recovery for GUI launches Rejected: Special-case only the exact end marker string | leaves the real state distinction ambiguous Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep marker-aware parsing separate from generic stdout sanitization so empty payloads stay empty Tested: cargo test --manifest-path src-tauri/Cargo.toml parse_interactive_shell_env_output -- --nocapture Tested: cargo test --manifest-path src-tauri/Cargo.toml extract_marked_value -- --nocapture Tested: cargo check --manifest-path src-tauri/Cargo.toml Not-tested: bun tauri dev manual UI verification Related: robinebers#358 Related: robinebers#398 (cherry picked from commit 7cf7a6f)
Description
Fixes the Z.ai failure mode where OpenUsage reads a valid API key from an interactive shell, but hidden ANSI/control bytes in shell startup output contaminate the resolved value and make the
Authorizationheader invalid.The fix hardens the shared host env resolver instead of patching only the Z.ai plugin:
Related Issue
Fixes #358
Type of Change
Testing
bun run buildand it succeededbun run testand all tests passbun tauri devcargo test --manifest-path src-tauri/Cargo.tomlcargo check --manifest-path src-tauri/Cargo.tomlChecklist
mainbranchSummary by cubic
Sanitizes env values from interactive shells and direct env reads to prevent ANSI/control noise from corrupting API keys and breaking
Authorizationheaders (notably for Z.ai). Hardens the shared host env resolver so all plugins benefit; also fixes empty-marker cases.Written for commit 7cf7a6f. Summary will update on new commits.