runner: clear env on windows with a safe allowlist

[mooncitydev]

problem

On Unix,
elic run already clears the child environment and re-applies only a small allowlist (PATH, HOME, locale, etc.). On Windows the runner skipped that step, so the child inherited the entire parent process environment. That can leak unrelated secrets (API keys, tokens) that happen to be set in the shell into the target process, which defeats the isolation users expect from a secret runner.

change

• Apply the same pattern on Windows: env_clear() then copy only an allowlist of infrastructure variables (PATH, PATHEXT, COMSPEC, SYSTEMROOT, WINDIR, user profile paths, program files roots, TEMP/TMP, locale, TZ, HOME when set).
• Document this behavior in the crate-level module comment.

This matches the intent described in the previous inline comment that called for a Windows-specific allowlist.

---

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional findings.

[greptile-apps]

Greptile Summary

This PR closes the Windows env-isolation gap in the runner: it defines a INHERITED_ENV_KEYS allowlist for Windows (mirroring the existing Unix one) and applies env_clear() before re-applying only those keys, preventing the child process from inheriting the full parent environment on Windows. Two small housekeeping items surfaced: the #[cfg(unix)] and #[cfg(windows)] loop bodies are now byte-for-byte identical and can be collapsed into a single #[cfg(any(unix, windows))] block, and SYSTEMDRIVE is absent from the Windows allowlist despite being a commonly expected variable for path resolution.

Confidence Score: 5/5

Safe to merge; all remaining findings are P2 style/cleanup items that do not affect correctness or security.

The core security fix (env_clear on Windows) is correct and mirrors the proven Unix path. Both open findings are P2: duplicate cfg bodies (maintenance nuisance, not a runtime defect) and the missing SYSTEMDRIVE entry (edge-case tool breakage, not a secret-leakage regression). Neither blocks merge.

packages/runner/src/lib.rs — minor: collapsed cfg bodies and SYSTEMDRIVE gap

Important Files Changed

Filename	Overview
packages/runner/src/lib.rs	Adds Windows env-isolation by defining a platform-specific INHERITED_ENV_KEYS allowlist and a matching #[cfg(windows)] env_clear block; two minor issues: the cfg bodies are now identical and could be collapsed, and SYSTEMDRIVE is absent from the Windows allowlist.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[run_with_secrets called] --> B[Parse command + secrets JSON]
    B --> C[Validate secret keys]
    C --> D[Build Command]
    D --> E{Platform?}
    E -->|Unix| F[env_clear]
    E -->|Windows| G[env_clear]
    F --> H[Re-apply Unix INHERITED_ENV_KEYS\nPATH HOME USER SHELL TERM\nLANG LC_ALL LC_CTYPE TMPDIR TZ]
    G --> I[Re-apply Windows INHERITED_ENV_KEYS\nPATH PATHEXT COMSPEC SYSTEMROOT\nWINDIR USERPROFILE APPDATA TEMP …]
    H --> J[Inject secrets as env vars]
    I --> J
    J --> K[Spawn child process]
    K --> L[Wait for exit]
    L --> M[Return exit code]

Loading

Comments Outside Diff (1)

1. packages/runner/src/lib.rs, line 143-163 (link)

   Identical cfg blocks can be collapsed

   The #[cfg(unix)] and #[cfg(windows)] bodies are now byte-for-byte identical. Since the platform distinction is already fully encoded in the two INHERITED_ENV_KEYS constant definitions, both cfg guards on the body serve no purpose and add a maintenance hazard — a future change to the loop logic must be applied twice.

   You would also want to update the two constant attributes to #[cfg(any(unix, windows))] — #[cfg(unix)] / #[cfg(windows)] can stay as-is since they need to remain separate (different values).

_{Reviews (1): Last reviewed commit: "runner: clear env on windows with a safe..." | Re-trigger Greptile}

[greptile-apps]

SYSTEMDRIVE missing from Windows allowlist

SYSTEMDRIVE (typically C:) is used by many Windows tools and the runtime itself to resolve absolute paths (e.g., %SYSTEMDRIVE%\Users\...). Without it, child processes that construct paths from %SYSTEMDRIVE% will see an empty or absent variable. SYSTEMROOT is already included, but SYSTEMDRIVE is a distinct variable and is commonly expected.

Suggested change

	const INHERITED_ENV_KEYS: &[&str] = &[
	"PATH",
	"PATHEXT",
	"COMSPEC",
	"SYSTEMROOT",
	"WINDIR",
	"USERPROFILE",
	"USERNAME",
	"HOMEDRIVE",
	"HOMEPATH",
	"APPDATA",
	"LOCALAPPDATA",
	"PROGRAMFILES",
	"PROGRAMFILES(X86)",
	"PROGRAMW6432",
	"TEMP",
	"TMP",
	"LANG",
	"LC_ALL",
	"LC_CTYPE",
	"TZ",
	"HOME",
	];
	"SYSTEMROOT",
	"SYSTEMDRIVE",
	"WINDIR",

[icanvardar]

Hey, the security fix is spot-on and the allowlist looks comprehensive. A few suggestions before merging:

1. Collapse the duplicate cfg blocks
   The #[cfg(unix)] and #[cfg(windows)] loop bodies are now identical since the platform difference is already handled by the two separate INHERITED_ENV_KEYS constants. Could we collapse them into a single block?

#[cfg(any(unix, windows))]
{
    cmd.env_clear();
    for key in INHERITED_ENV_KEYS {
        if let Ok(val) = std::env::var(key) {
            cmd.env(key, val);
        }
    }
}

This way if the loop logic ever changes, we only need to update it in one place.

2. Add SYSTEMDRIVE to the Windows allowlist SYSTEMDRIVE (typically C:) is commonly expected by Windows tools for path resolution. Its absence could cause subtle breakage in some programs. Would be good to include it alongside SYSTEMROOT and WINDIR.

3. Add a test for the env-clearing behavior
   Right now there's no test that verifies a random parent env var doesn't leak into the child on Windows (or Unix for that matter). Something like setting a dummy env var in the parent, spawning a child that prints its environment, and asserting the dummy var is absent would give us confidence the isolation actually works. Would you be open to adding a test like that?

4. Minor nit on the PR description
   The description says "elic run" -- looks like a small typo for "relic run". Also it might be helpful to list the full set of allowlisted variables directly in the description (you have it partially) so reviewers don't have to cross-reference the diff.

[mooncitydev]

nice to meet you