harden(security): generalized secure_command primitive + universal env-strip + docs (#39) - #41
Merged
Merged
Conversation
…39) Ships the generic counterpart to secure_rg_command (PR #33) so future per-tool wrapped CLIs can opt in via a declarative ToolPolicy instead of copy-pasting the env-strip + arg-deny scaffolding. - UNIVERSAL_ENV_STRIP covers loader hijacks (LD_PRELOAD, DYLD_*), pager/editor (EDITOR, PAGER, LESS, ...), per-lang loader injection (PERL5OPT, LUA_INIT, ...), and shell metaprogramming (BASH_ENV, PROMPT_COMMAND, IFS, ...). Dynamic BASH_FUNC_* and DYLD_* are stripped by walking std::env::vars(). - ToolPolicy struct + secure_command_with_policy() and check_args_with_policy() mirror the rg helpers' semantics, including the codex-P1 short-bundle deny that catches '-cz'-style bypasses. - New docs/security/zero-trust-wrapped-cli.md documents the threat model (LLM mistakes + hostile inherited env, not local-shell attacker), the defense-in-depth layers, and the new-tool checklist. - 6 new unit tests in policy_registry_tests; full bin suite still 2040 passing. This PR deliberately does NOT refactor secure_rg_command or the per-tool helpers from in-flight PRs #34-#38 to use the new primitive yet -- that refactor is a follow-up so this lands cleanly while those PRs are also in flight. Closes #39 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P3 catch on PR #39: the secure_command_with_policy_strips_env test mutates process-global env (LD_PRELOAD, ECHO_EXTRA_STRIP, BASH_FUNC_*) without serialisation. Under cargo test's default parallel execution, other tests in the same binary could observe those values or spawn subprocesses while they were set. Fix: add a `static GLOBAL_ENV_LOCK: Mutex<()>` at the top of policy_registry_tests, take it at the start of the env-mutating test. Other env-mutating tests in this file should take the same lock. Codex P1 (PATH-based binary substitution in resolved_command) filed as a separate follow-up #40 — defending against tainted PATH requires a structural change to binary resolution (trusted-PATH allowlist or similar) that doesn't fit this PR's scope. Co-Authored-By: Codex review <noreply@openai.com>
This was referenced May 18, 2026
harden(pre-release): UNIVERSAL_ENV_STRIP wiring + remove unsafe env-mutation + 4 missed wire-ins
#51
Merged
noogalabs
pushed a commit
to noogalabs/contextcrawler
that referenced
this pull request
Jun 4, 2026
…n, plug 4 missed wire-ins Pre-release codex+claude review of harden PRs thehoff#41-thehoff#47 surfaced 5 must-fix items before tagging v0.1.8. This is the consolidation PR. ## F8 (architectural) — UNIVERSAL_ENV_STRIP was documentation-only `secure_command_with_policy` applied the universal list, but the 15 per-tool secure_*_command helpers (added by thehoff#42/thehoff#43/thehoff#44/thehoff#46/thehoff#47) called only `resolved_command(name)` + their OWN env_strip list. So LD_PRELOAD, DYLD_INSERT_LIBRARIES, BASH_ENV, BASH_FUNC_*, PROMPT_COMMAND, PERL5OPT, LUA_INIT etc. were NEVER stripped from any wrapped tool — the headline claim of PR thehoff#41 was unfulfilled. Fix: extract `apply_universal_env_strip(&mut cmd)` from `secure_command_with_policy` and call it as the first step of every per-tool helper (rg, git, cargo, node, python, ruby, jvm, dotnet, go, kubectl, docker, aws, psql, curl, wget — 15 sites). ## F9 (unsoundness) — `unsafe { env::remove_var }` race in fallback `cloud_fallback_hardening` (main.rs) called `unsafe { env::remove_var }` with a SAFETY note claiming "single-threaded at CLI dispatch". False: `maybe_ping()` at the top of `run_cli` spawns a telemetry thread BEFORE this dispatch point, and that thread calls `std::env::vars()`. Per Rust 1.81+ `env::remove_var` contract that's UB. Fix: drop the env mutation entirely. The per-tool secure_*_command helpers wired into every Commands::* dispatch path already strip env via the safe Command::env_remove method. The fallback path is rare (only fires on clap parse failure) and an unhardened fallback is an accepted residual surface — tracked in a follow-up if it matters. ## F1, F2, F4, F5 — missed wire-ins - F1 `src/cmds/system/format_cmd.rs:77` — `"black" | "ruff"` formatters switched from raw `resolved_command` to `secure_python_command`. - F2 `src/cmds/js/lint_cmd.rs:102` — python linter path switched to `secure_python_command`. - F4 `src/core/utils.rs::ruby_exec` — bundler branch (`Command::new("bundle")`) switched to `secure_ruby_command("bundle")`. - F5 `src/core/utils.rs::package_manager_exec` — all four branches (`pnpm`, `yarn`, `npx`, direct-tool) switched from `resolved_command` to `secure_node_command`. This transparently hardens prettier_cmd, vitest_cmd, format_cmd which call into package_manager_exec. ## Deferred to follow-up (filed separately) - F6 — env-mutating tests in `secure_git_tests` / `secure_cargo_tests` don't take `GLOBAL_ENV_LOCK` (only `policy_registry_tests` does). Race-prone but currently passing. - F7 — `check_forbidden_pytest_args` uses `looks_like_path` which doesn't recognise bare-relative paths like `sub/dir.py`. Plugin loader is Kernel.require-equivalent; should reject anything with `/` or `\` even without explicit `./` prefix. - gh/glab/gt unhardened (these wrap git under the hood; need secure_gh_command / secure_glab_command). ## Verified - `cargo build --bin contextcrawler` — clean - `cargo test --bin contextcrawler` — 2116 pass, 0 fail - All 7 integration suites pass: branding_lint(3), git_hardening(6), cargo_hardening(4), node_hardening(4), runtime_hardening(12), cloud_hardening(14), harness_standalone(1) Refs PRs thehoff#41 thehoff#42 thehoff#43 thehoff#44 thehoff#46 thehoff#47 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: Codex pre-release adversarial review <noreply@openai.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Foundation for the zero-trust wrapped-CLI hardening series. Generalizes the PR #33
secure_rg_command+check_forbidden_rg_argspattern into a policy-driven primitive that any future tool can register against.Changes
src/core/utils.rs:UNIVERSAL_ENV_STRIP— 24 vars (pager/editor, LD_/DYLD_, PERL5OPT/LUA_*, BASH_ENV/SHELLOPTS/PROMPT_COMMAND/IFS)ToolPolicystruct (4 declarative fields)secure_command_with_policy(&policy)— applies universal strip + dynamic BASH_FUNC_/DYLD_ + per-tool stripcheck_args_with_policy(&policy, args)— exact/prefix/short-bundle deny (mirrors PR SECURITY: close RIPGREP_CONFIG_PATH RCE (#32) + ship Security dashboard subcommand #33's anti-bypass logic)policy_registry_testsdocs/security/zero-trust-wrapped-cli.md— threat model (quotes user's framing), three defense layers, registration recipe, currently-registered-tools table (empty pending the followup refactor)Per-tool refactor of existing
secure_rg_commandto use the new primitive is INTENTIONALLY deferred so this PR lands cleanly while parallel per-tool harden PRs (#34/#35/#36/#37/#38) are also in flight.Reviewed by Codex
GLOBAL_ENV_LOCK: Mutex<()>taken at test entryresolved_command→ filed as follow-up harden(PATH): trusted-PATH resolution for wrapped binaries (defer trojan-on-PATH defense) #40 (requires trusted-PATH allowlist, structural change out of scope here)Test plan
cargo test --bin contextcrawler— 2040 passcargo test --bin contextcrawler policy_registry— 6 passCloses #39