🔒 Aisle Security Analysis

We found 2 potential security issue(s) in this PR:

1. 🔵 Log/control-character injection via reflected invalid env override keys in system.run denial message (Node)

Description

system.run now rejects blocked/invalid environment override keys and reflects the rejected key names back in the error message. Because rejected invalid keys are derived from untrusted JSON object keys and are not escaped/neutralized, an attacker can include control characters (e.g. newlines, tabs, ANSI escapes) in a key name and have them appear in downstream logs/UI.

Key points:

• Input: opts.params.env keys come from an external request.
• Taint propagation: invalid keys are collected as rawKey.trim() (preserves embedded control characters).
• Sink: the collected keys are interpolated into error.message and returned to the caller; this message is then propagated by the gateway (and may be logged by server/client components), enabling log forging / multiline injection.

Vulnerable code (sink):

message: `SYSTEM_RUN_DENIED: environment override rejected (${details.join("; ")})`

where details includes joined rejected keys.

Recommendation

Avoid reflecting untrusted key material verbatim in error messages/logs.

Options:

1. Escape/neutralize control characters before joining:

function escapeForDiagnostic(s: string): string {
  ​// Replace control chars (including newlines/escapes) with visible escapes
  return s.replace(/[\u0000-\u001F\u007F]/g, (ch) => {
    const code = ch.codePointAt(0)!.toString(16).padStart(2, "0");
    return `\\x${code}`;
  });
}

const safeInvalid = envOverrideDiagnostics.rejectedOverrideInvalidKeys.map(escapeForDiagnostic);

2. Or, do not include the actual keys; include only counts and a generic hint.

Apply the same escaping anywhere rejected keys are included in diagnostics (source lists are built in inspectHostExecEnvOverrides).

2. 🔵 Log/control-character injection via reflected invalid env override keys in system.run denial message (macOS Swift)

Description

The macOS node runtime rejects blocked/invalid environment override keys and reflects the rejected key names inside an error message returned to callers. The invalid key strings are derived from untrusted JSON dictionary keys (params.env) and are not escaped/neutralized before being embedded into the message.

Impact:

• An attacker can send an env override key containing control characters (e.g. "BAD\nKEY" or ANSI escapes).
• The key is collected into invalidKeys (using trimmingCharacters(in: .whitespacesAndNewlines) which does not remove embedded control characters).
• The key is then joined and interpolated into the denial message, which may be logged or displayed, enabling log forging/multiline injection or terminal escape injection depending on the consumer.

Vulnerable code:

invalid.append(candidate.isEmpty ? rawKey : candidate)
...
message: "SYSTEM_RUN_DENIED: environment override rejected (\(details.joined(separator: "; ")) )"

where details includes joined invalidKeys.

Recommendation

Do not embed untrusted strings verbatim into diagnostic messages.

Recommended fix:

• Escape control characters before joining/embedding, or replace the list with a count.

Example escaping helper:

func escapeForDiagnostic(_ s: String) -> String {
    var out = ""
    for scalar in s.unicodeScalars {
        if scalar.value < 0x20 || scalar.value == 0x7F {
            out += String(format: "\\x%02X", scalar.value)
        } else {
            out.unicodeScalars.append(scalar)
        }
    }
    return out
}

let safeInvalid = envOverrideDiagnostics.invalidKeys.map(escapeForDiagnostic)

Then use safeInvalid.joined(separator: ", ").

Also apply similar escaping in HostEnvSanitizer.inspectOverrides when collecting invalid keys, if those are used anywhere else.

Analyzed PR: #51207 at commit fb2b239

_{Last updated on: 2026-03-20T21:55:49Z}

Greptile Summary

This PR hardens host environment variable override handling across the gateway (bash-tools.exec.ts) and node (invoke-system-run.ts) execution paths by introducing a unified diagnostic sanitization API (sanitizeHostExecEnvWithDiagnostics) that surfaces rejected keys, then uses those diagnostics to fail closed with explicit error messages before execution begins. It also expands the override blocklist with 16 new runtime-loader and config keys (Java CLASSPATH, Go CGO/GOFLAGS, .NET CORECLR_PROFILER_PATH, PHP, Deno, Bun, Lua, Ruby, Composer, XDG_CONFIG_HOME) and regenerates the macOS companion Swift policy artifact.

Key changes and observations:

• Gateway path (bash-tools.exec.ts): Replaces the old sanitizeHostBaseEnv + validateHostEnv split with sanitizeHostExecEnvWithDiagnostics. The resulting hostEnvResult.env is correctly used as the actual execution env — clean and consistent.
• Node-host path (invoke-system-run.ts): sanitizeHostExecEnvWithDiagnostics is called without baseEnv, defaulting to process.env, but envSanitizeResult.env is never used — the actual execution env is still produced by opts.sanitizeEnv(envOverrides). This causes unnecessary computation (building a full merged env from process.env + accepted overrides only to discard it) and introduces a subtle dual-path inconsistency where the diagnostic and execution envs use different base env references.
• Shell-wrapper case: When shellPayload !== null, sanitizeSystemRunEnvOverrides filters opts.params.env down to only the shell-wrapper-allowed keys before the new check runs. A blocked key like CLASSPATH is silently dropped, never reaching the new rejection logic. The "fail closed with an explicit error" guarantee only applies to the direct-exec (non-shell-wrapper) path.
• Blocklist additions and the generated Swift file are correctly in sync.
• Test coverage is solid for the new diagnostic API and the direct-exec rejection paths, but the shell-wrapper behavior under blocked overrides is not exercised.

Confidence Score: 3/5

• Directionally correct security hardening, but the node-host path has an architectural inconsistency that warrants review before merge.
• The gateway path is well-implemented and the blocklist expansion is sound. The node-host path has two concerns: (1) sanitizeHostExecEnvWithDiagnostics builds a full merged env that is immediately discarded — the actual execution env uses a separate opts.sanitizeEnv path, creating a subtle divergence between the check and execution contexts; (2) the shell-wrapper sub-path silently drops blocked overrides rather than rejecting them, inconsistent with the stated "fail closed" goal. Neither is an immediate exploit, but they create ambiguity about what the security invariants actually are and could mask issues in future changes.
• src/node-host/invoke-system-run.ts — the diagnostic sanitization call discards its computed env and doesn't cover the shell-wrapper sub-path.

This is a comment left during a code review.
Path: src/node-host/invoke-system-run.ts
Line: 254-283

Comment:
**Computed env is built but immediately discarded**

`sanitizeHostExecEnvWithDiagnostics` is called here without a `baseEnv`, so it defaults to `process.env` and builds a full merged env object internally. However, `envSanitizeResult.env` is never referenced — only the rejection diagnostic arrays (`rejectedOverrideBlockedKeys`, `rejectedOverrideInvalidKeys`) are consumed. The actual execution env is still produced separately by `opts.sanitizeEnv(envOverrides)` on line 296.

This means every call through this path incurs the cost of building a merged env from `process.env` + accepted overrides only to throw it away. More importantly, it creates a subtle divergence: the diagnostic check implicitly uses `process.env` as its base, while the actual env used for execution is computed by a different code path (`opts.sanitizeEnv`). If those two paths ever diverge in their understanding of the base env, the rejection check could pass for a key that `opts.sanitizeEnv` would also allow.

Consider either:
- Passing the diagnostic result's `env` to `opts.sanitizeEnv` is replaced (since `sanitizeHostExecEnvWithDiagnostics` already produces a sanitized merged env), or
- Extracting only the override diagnostics without building the full merged env (e.g., a narrower helper that checks overrides without merging against a base).

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/node-host/invoke-system-run.ts
Line: 250-257

Comment:
**Shell-wrapper path silently drops blocked keys instead of failing closed**

When `shellPayload !== null` (i.e., a shell wrapper invocation), `sanitizeSystemRunEnvOverrides` filters `opts.params.env` down to only the shell-wrapper-allowed keys (`TERM`, `LANG`, `LC_ALL`, etc.) and returns that as `envOverrides`. As a result, a blocked key like `CLASSPATH` is silently discarded before `sanitizeHostExecEnvWithDiagnostics` even sees it — so the new rejection check never fires for that path.

This means the "fail closed on blocked override keys" behavior only applies to the direct-exec (non-shell-wrapper) path. In the shell-wrapper case, a caller passing `CLASSPATH: "/tmp/evil"` receives a silent drop rather than an explicit error. This inconsistency is worth documenting or addressing: either the shell-wrapper case should also reject (mirroring the non-shell-wrapper behavior), or the intent to silently drop in this path should be noted with a comment, and the new test coverage should explicitly exercise the shell-wrapper scenario to confirm the expected behavior.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "Exec: harden host en..."}

Follow-up pushed in ff0c326 to address review feedback:

• Validate env override diagnostics on raw params.env before shell-wrapper filtering in src/node-host/invoke-system-run.ts.
• Use override-only diagnostics (inspectHostExecEnvOverrides) in node parse flow to avoid building/discarding a merged env.
• Preserve inherited non-portable Windows-style keys in host env sanitization (ProgramFiles(x86) regression case).
• Add regressions for shell-wrapper blocked-key rejection and invalid-key rejection in src/node-host/invoke-system-run.test.ts.
• Add inherited non-portable key coverage in src/infra/host-env-security.test.ts and src/node-host/invoke.sanitize-env.test.ts.

Targeted tests run:
pnpm test -- src/infra/host-env-security.test.ts src/node-host/invoke.sanitize-env.test.ts src/node-host/invoke-system-run.test.ts src/agents/bash-tools.exec.path.test.ts