Conversation
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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
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):
No defects found. READY for the un-draft rule. |
Summary
Five structs store a process environment (API keys, tokens, cookies). Three of them derive
Debug, so{:?},dbg!, atracingfield capture or anassert_eq!failure printed every variable name and value. This PR ports upstream 0.70.0 steipete#4106:codexbar::process_environment::ProcessEnvironment<T>wrapper. ItsDebugoutput isProcessEnvironment(N entries; redacted). Deref,DerefMut,IntoIteratorandinto_innerstill return the original values for the child process.Debugbefore this PRTtyCommandOptions.env(cli/tty_runner.rs)ManagedProcessConfig.env(managed_process.rs)TokenAccountOverride.env_override(core/token_accounts.rs)OPENROUTER_API_KEYCommandRunner.env_additions(host/command_runner.rs)Debugtoday; wrapped so a future derive stays safePiFamilyScanInput.environment(agent_sessions/pi_family/mod.rs)Debugtoday; wrapped so a future derive stays safeUpstream reference
ee6a89d903, release v0.70.0 (Security: "Redact every remaining stored process environment and guard against new unredacted environment properties with a repository check").Sources/CodexBarCore/ProcessEnvironment.swift,Tests/CodexBarTests/ProcessEnvironmentTests.swift,Tests/CodexBarTests/ProcessEnvironmentStorageTests.swift,docs/DEVELOPMENT.md.Ported / Deferred
Ported:
@ProcessEnvironmentproperty wrapper with count-onlydescriptionand mirror becomes a Rust newtype with a count-onlyDebug. Rust has no reflection, soDebugis the only automatic rendering path. Wrapping anOptionkeepsNonedistinct from an empty map, and equality still compares the original contents, as upstream.process_environment/tests.rs), mirroringProcessEnvironmentTestswith the same synthetic sentinel (CODEXBAR_TEST_SENTINEL_SECRET=sentinel-environment-value-must-not-be-rendered, plusORDINARY_NAME):{:?}and{:#?}ofTtyCommandOptions,ManagedProcessConfigandTokenAccountOverrideshow neither names nor values;assert_eq!hides the environment;process_environment/storage_guard.rs), mirroringProcessEnvironmentStorageTests:rust/srcandapps/desktop-tauri/src-tauri/srcfor struct, union and enum-variant fields whose name containsenvand whose type is a string map (HashMap,BTreeMap,IndexMap) or a string pair list (Vec<(OsString, OsString)>, slices, arrays). It looks throughOption,Box,Arc,Rc, locks and cells, references, multiline declarations, path-qualified names and localtypealiases. That last one is needed becausePiFamilyScanInputstores anEnvMap.REVIEWED_EXCEPTIONSis an exact-source allowlist (empty today). Stale or duplicate entries fail, as upstream.docs/BUILDING.mdgains "Stored process environments", the counterpart of the upstreamdocs/DEVELOPMENT.mdparagraphs.Deviations and deferred:
Debugoutput without an explicit{:?}, and that is explicit logging, which upstream also leaves to code review. Function parameters are skipped, as upstream.Debugoutside environments (follow-up).TokenAccountOverridealso derivesDebugoveraccount.token(viaTokenAccount) andcookie_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.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\workeron this branch (baseport/upstream-0.70.0atb585d488), through the local build gate:cargo +1.98.0 fmt --all --checkcargo +1.98.0 clippy --workspace --all-targets -- -D warningscargo test -p codexbar process_environmentcargo test -p codexbarcargo test -p codexbar-desktop-tauribootstrap_payload_exposes_every_provider_variantfiltered; it needs #711)Guard check: before the five fields were wrapped,
shipped_environment_storage_uses_the_redacting_wrapperfailed and listed exactly these five fields, with no false positives:The same run showed the old derived
Debugprinting the sentinel (env: {"CODEXBAR_TEST_SENTINEL_SECRET": "sentinel-environment-value-must-not-be-rendered", ...}). After wrapping, all 12 tests pass.Affected areas
rust/src/process_environment.rs,rust/src/process_environment/storage_guard.rs,rust/src/process_environment/tests.rs.cli/tty_runner.rs,managed_process.rs,core/token_accounts.rs,host/command_runner.rs,agent_sessions/pi_family/mod.rs..into()):providers/claude/mod.rs(Claude PTY probe),providers/antigravity/mod.rs(managedagy),agent_sessions.rs, plus twomanaged_processtests and one Pi-family test helper.docs/BUILDING.md.TokenAccountOverride.env_override.as_ref()incommands/providers.rs. It compiles unchanged through deref and gets the sameOption<&HashMap>.UI proof
Not applicable: backend-only. No UI, settings or tray change. Child processes get the same environment as before; only
Debugoutput changes.