Skip to content

Port upstream 0.70.0: redact stored process environments - #722

Open
Finesssee wants to merge 1 commit into
port/upstream-0.70.0from
port/micro-0.70.0-debug-env-redaction
Open

Finesssee wants to merge 1 commit into
port/upstream-0.70.0from
port/micro-0.70.0-debug-env-redaction

Conversation

@Finesssee

Copy link
Copy Markdown
Collaborator

Summary

Five structs store a process environment (API keys, tokens, cookies). Three of them derive Debug, so {:?}, dbg!, a tracing field capture or an assert_eq! failure printed every variable name and value. This PR ports upstream 0.70.0 steipete#4106:

  • New codexbar::process_environment::ProcessEnvironment<T> wrapper. Its Debug output is ProcessEnvironment(N entries; redacted). Deref, DerefMut, IntoIterator and into_inner still return the original values for the child process.
  • All five stored environments now use it. Child processes receive exactly the same variables as before.
  • A repository test fails when a new environment-named field stores an unwrapped string map or string pair list.
Field Debug before this PR
TtyCommandOptions.env (cli/tty_runner.rs) derived: printed every name and value
ManagedProcessConfig.env (managed_process.rs) derived: printed every name and value
TokenAccountOverride.env_override (core/token_accounts.rs) derived: printed the token-account key, for example OPENROUTER_API_KEY
CommandRunner.env_additions (host/command_runner.rs) no Debug today; wrapped so a future derive stays safe
PiFamilyScanInput.environment (agent_sessions/pi_family/mod.rs) no Debug today; wrapped so a future derive stays safe

Upstream reference

  • steipete/CodexBar fix(security): redact remaining stored process environments steipete/CodexBar#4106, commit ee6a89d903, release v0.70.0 (Security: "Redact every remaining stored process environment and guard against new unredacted environment properties with a repository check").
  • Files at tag v0.70.0: Sources/CodexBarCore/ProcessEnvironment.swift, Tests/CodexBarTests/ProcessEnvironmentTests.swift, Tests/CodexBarTests/ProcessEnvironmentStorageTests.swift, docs/DEVELOPMENT.md.

Ported / Deferred

Ported:

  • Wrapper. Swift's @ProcessEnvironment property wrapper with count-only description and mirror becomes a Rust newtype with a count-only Debug. Rust has no reflection, so Debug is the only automatic rendering path. Wrapping an Option keeps None distinct from an empty map, and equality still compares the original contents, as upstream.
  • Tests (process_environment/tests.rs), mirroring ProcessEnvironmentTests with the same synthetic sentinel (CODEXBAR_TEST_SENTINEL_SECRET = sentinel-environment-value-must-not-be-rendered, plus ORDINARY_NAME):
    • {:?} and {:#?} of TtyCommandOptions, ManagedProcessConfig and TokenAccountOverride show neither names nor values;
    • the panic message of a failed assert_eq! hides the environment;
    • map access and the rendered count follow mutations;
    • optional environments keep absence, mutation and value equality;
    • equal counts render identically whatever the contents.
  • Repository guard (process_environment/storage_guard.rs), mirroring ProcessEnvironmentStorageTests:
    • It scans rust/src and apps/desktop-tauri/src-tauri/src for struct, union and enum-variant fields whose name contains env and whose type is a string map (HashMap, BTreeMap, IndexMap) or a string pair list (Vec<(OsString, OsString)>, slices, arrays). It looks through Option, Box, Arc, Rc, locks and cells, references, multiline declarations, path-qualified names and local type aliases. That last one is needed because PiFamilyScanInput stores an EnvMap.
    • Comments, strings and char literals are blanked before scanning, keeping line numbers.
    • REVIEWED_EXCEPTIONS is an exact-source allowlist (empty today). Stale or duplicate entries fail, as upstream.
    • A self-test pins every recognized spelling and the declarations it must ignore.
  • Docs. docs/BUILDING.md gains "Stored process environments", the counterpart of the upstream docs/DEVELOPMENT.md paragraphs.

Deviations and deferred:

  • Locals are not scanned. Upstream also scans local dictionaries and allowlists each transient one. Here only fields count as storage: a Rust local cannot reach Debug output without an explicit {:?}, and that is explicit logging, which upstream also leaves to code review. Function parameters are skipped, as upstream.
  • Credential Debug outside environments (follow-up). TokenAccountOverride also derives Debug over account.token (via TokenAccount) and cookie_header. Those are credentials, not process environments, so fix(security): redact remaining stored process environments steipete/CodexBar#4106 does not cover them. They need their own hardening change.
  • Upstream's harness scrubbing (Scripts/test_environment.sh, 0.69.0 fix(security): redact fetcher environments and scrub tests steipete/CodexBar#4097) was a SKIP in the 0.69 triage: it scrubs the Swift test runner, which has no counterpart here.

Validation

Run in W:\wcb-wt\worker on this branch (base port/upstream-0.70.0 at b585d488), through the local build gate:

Check Result
cargo +1.98.0 fmt --all --check pass
cargo +1.98.0 clippy --workspace --all-targets -- -D warnings pass
cargo test -p codexbar process_environment 12 passed, 0 failed
cargo test -p codexbar 2172 passed, 0 failed, 1 ignored
cargo test -p codexbar-desktop-tauri 461 passed, 0 failed (bootstrap_payload_exposes_every_provider_variant filtered; it needs #711)

Guard check: before the five fields were wrapped, shipped_environment_storage_uses_the_redacting_wrapper failed and listed exactly these five fields, with no false positives:

Unprotected environment storage: rust/src/agent_sessions/pi_family/mod.rs:135: PiFamilyScanInput.environment: EnvMap
Unprotected environment storage: rust/src/cli/tty_runner.rs:73: TtyCommandOptions.env: HashMap<String, String>
Unprotected environment storage: rust/src/core/token_accounts.rs:756: TokenAccountOverride.env_override: Option<HashMap<String, String>>
Unprotected environment storage: rust/src/host/command_runner.rs:113: CommandRunner.env_additions: HashMap<String, String>
Unprotected environment storage: rust/src/managed_process.rs:73: ManagedProcessConfig.env: Vec<(OsString, OsString)>

The same run showed the old derived Debug printing the sentinel (env: {"CODEXBAR_TEST_SENTINEL_SECRET": "sentinel-environment-value-must-not-be-rendered", ...}). After wrapping, all 12 tests pass.

Affected areas

  • New: rust/src/process_environment.rs, rust/src/process_environment/storage_guard.rs, rust/src/process_environment/tests.rs.
  • Field types: cli/tty_runner.rs, managed_process.rs, core/token_accounts.rs, host/command_runner.rs, agent_sessions/pi_family/mod.rs.
  • Construction sites (.into()): providers/claude/mod.rs (Claude PTY probe), providers/antigravity/mod.rs (managed agy), agent_sessions.rs, plus two managed_process tests and one Pi-family test helper.
  • docs/BUILDING.md.
  • The Tauri shell reads TokenAccountOverride.env_override.as_ref() in commands/providers.rs. It compiles unchanged through deref and gets the same Option<&HashMap>.

UI proof

Not applicable: backend-only. No UI, settings or tray change. Child processes get the same environment as before; only Debug output changes.

Upstream 0.70.0 (steipete#4106) wraps every stored process-environment
dictionary in a ProcessEnvironment property wrapper whose description
and mirror show only the entry count, and adds a source scan that fails
when an environment-named dictionary is stored without it.

Port it to Rust:

- process_environment::ProcessEnvironment<T> keeps the original
  environment for the child process (Deref, DerefMut, IntoIterator,
  into_inner) and its Debug prints "ProcessEnvironment(N entries;
  redacted)", so {:?}, dbg!, tracing captures and assert_eq! failures no
  longer print variable names or values. Option keeps None distinct
  from an empty map; equality still compares contents.
- Wrap the five stored environments: TtyCommandOptions.env,
  ManagedProcessConfig.env, TokenAccountOverride.env_override,
  CommandRunner.env_additions and PiFamilyScanInput.environment.
- process_environment::storage_guard scans rust/src and the Tauri shell
  for struct, union and enum-variant fields named *env* whose type is a
  string map or string pair list (through Option/Box/Arc, references,
  slices and local type aliases) and fails on unwrapped ones. Reviewed
  exceptions must match exactly one declaration. Only fields count as
  storage: upstream also scans locals and allowlists each transient one,
  while here function parameters and locals are not scanned.
- Document the rule in docs/BUILDING.md.
@coderabbitai

coderabbitai Bot commented Oct 1, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 499ce557-60a5-40ed-aba9-8c50c0196ed2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Finesssee

Copy link
Copy Markdown
Collaborator Author

Adversarial validation passed at 526f218

Scope: redact stored process environments (#722, upstream 0.70.0 steipete#4106), validated as merged into release/v0.70.0 (merge 526f218 = merge of 2aeec9e into 504bd67).

Attacks (highest-risk semantics, from the merged tree):

  • Count-only redaction is real: ProcessEnvironment<T>'s Debug impl prints ProcessEnvironment(N entries; redacted) — the values used to build the child process are untouched via explicit access (Deref/iter/get/insert), so behavior is preserved while {:?}/dbg!/tracing captures stop leaking keys/tokens/cookies.
  • The lexical tripwire is enforced, not aspirational: storage_guard.rs scans the shipped Rust sources and FAILS when an env-named field holds a plain HashMap<String, String> or string pair list (including Option/Box/Arc wrappers) — and the reviewed-allowlist covers exactly the fields that may keep a plain environment, with the scan pinned by scanner_recognizes_storage_spellings_without_flagging_parameters_or_wrapped_fields.
  • Coverage claim verified: the five stored envs from the audit (TtyCommandOptions, ManagedProcessConfig, TokenAccountOverride, CommandRunner, PiFamilyScanInput) are each touched in the merged diff (tty_runner, managed_process, token_accounts, command_runner, pi_family).
  • Windows-specific: environment passing to child processes on Windows uses into_inner() at spawn sites — explicit access still works, so no child behavior changes.

No defects found. READY for the un-draft rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant