fix(dotnet): add forbidden-arg checker — block MSBuild Custom*Targets / runsettings RCE

docs/audits/2026-05-22-e2e-review.md

@@ -0,0 +1,60 @@
+# ContextCrawler — End-to-End Review (2026-05-22)
+
+Third independent review pass, four parallel agents by app scope: security core,
+command filters, analytics/discovery/parser, and runtime-data mining. Prior
+audit passes: #100 (Codex), #111 (Claude). Verdict: the security *plumbing* is
+solid; the gaps are a soft centre in permission matching and one missed
+ecosystem. The bigger story is operational — the binary crashes daily and
+headline token savings are oversold.
+
+## Top tier — fix first
+
+| ID | Severity | Finding |
+|---|---|---|
+| P0-crash | operational | 33 SIGABRT crashes in 7 days, daily, on the current binary. ~30–100ms process lifetime ⇒ Rust panic in the startup path (an `unwrap()`/`expect()` against the no-unwrap rule). Symbolicate, reproduce with `RUST_BACKTRACE=1`, fix. Suspect: `core/config.rs`, `core/tracking.rs` (DB open), `hooks/` init. |
+| P0-grep | operational | `grep` filter's clap parser rejects standard flags (`-q -r -E -i -c -A -B`) → 1,255 parse-failures, 62% of all 2,016. Delivered savings only 10.3% on 17.4M tokens — worst filter in the suite. |
+| SEC-C1 | CRITICAL | `dotnet` has no argv deny-list checker on any of its 4 run paths (`dotnet_cmd.rs:34,78,119`). MSBuild `-p:CustomBefore*Targets=<path>` / `CustomAfter*` and `dotnet test --runsettings <file>` are unguarded RCE flags — the same class #111 closed for `go -toolexec`. Fix: add `check_forbidden_dotnet_args`, wire into all 4 paths. |
+| SEC-C2 | CRITICAL | Command substitution bypasses the permission/gate stack. `discover/lexer.rs` tokenises `` ` ``/`(`/`)` but `split_on_operators` never decomposes a `$(...)`, backtick, or `<(...)`/`>(...)` payload into its own segment, so `echo $(rm -rf /tmp/x)` evades a `Bash(rm -rf)` deny rule. Fix: lexer surfaces substitution payloads as first-class segments, or `check_command` fails closed on unparsed substitution. |
+| SEC-C3 | CRITICAL | Permission rules are byte-prefix matched (`permissions.rs:224-251`). `git  push  --force` (double space), quoting, or `--force=` evade a deny rule. Fix: tokenise both command and pattern via `shell_split`, compare token sequences. |
+
+## Important
+
+| ID | Area | Finding |
+|---|---|---|
+| SEC-I1 | integrity | `integrity::runtime_check` only validates the legacy `rtk-rewrite.sh` hook; the modern `settings.json` binary-command registration has zero tamper detection. The 27KB SHA-256 subsystem protects a near-dead path. |
+| SEC-I2 | supply-chain | No aggregate wall-clock budget or package-count cap on `supply_chain_gate::check`; 64MB-per-response cap. 20 packages × hostile registry ≈ 8-minute hook stall. |
+| SEC-I3 | supply-chain | `parse_iso8601` Z-suffix fallback can misparse a near-RFC3339 string; a far-past misparse skips the cooldown silently. Reject >1-day-future timestamps; treat ambiguous parses as `Unavailable`. |
+| SEC-I4 | tirith | `read_recent_downgrades` reads the whole `downgrades.jsonl` uncapped, O(n²) brace scan. Cap the read; split on newlines (the writer emits single-line JSON). |
+| CMD-I1 | ruby | `rake` has no argv checker; `rake -f /tmp/evil.rake` runs arbitrary Ruby. rspec/rubocop siblings are covered. Add `check_forbidden_rake_args` (`-r`/`--require`, `-R`, `-I`, `-f`/`--rakefile`). |
+| CMD-I2 | js | Node `--config`/`-c` (eslint/vitest/prettier) loads an executed JS config module — not in `NODE_FORBIDDEN_FLAGS_ALWAYS`. Consider a path-shape deny. |
+| AN-I1 | analytics | `gain` "By Command" Avg% (`tracking.rs:796`) is still an unweighted mean of per-invocation percentages — inconsistent with the volume-weighted headline. |
+| AN-I2 | analytics | `gain quota` divides lifetime saved tokens by a hardcoded *monthly* quota (`ESTIMATED_PRO_MONTHLY`) — mixed time horizons, can read >100%. |
+| AN-I3 | discover | `output_len` uses byte length as a token proxy; multibyte output overstates ~3×. Use `chars().count()`. |
+| AN-I4 | analytics | `ccusage` `--since 20250101` is a stale hardcoded literal (~16-month window and growing). Compute `now − 90d`. |
+| READ-I1 | filters | `read` filter delivers 13.7% savings; 1,209 of 1,653 runs returned literal 0%; 15.5M-token sink — second-largest. Effectively not filtering. |
+| GIT-I1 | filters | `git log` filter delivers 1.9% over 790 runs vs the 80% documented claim — not engaging on common flag variants. |
+| PERF-I1 | core | `cleanup_old` runs `VACUUM` inline in the `record()` write hot path; rewrites the whole 6.7MB DB on the first prune of each day. Use incremental auto-vacuum or schedule it. |
+
+## Runtime data — measured reality
+
+- **Blended token savings: 53.4%** (61.4M saved / 74.3M input, 14,789 commands, window 2026-04-25 → 05-21). Below the advertised 60–90%.
+- Delivering: `cargo` 66.5% (25.4M input — largest sink, working), `find` 73.9%, `ls` 72.2%.
+- Underperforming: `read` 13.7%, `grep` 10.3%, `git` 32.2% (`git log` 1.9%), `curl` 2.6% (77/90 runs zero), `gh` 24.4%, `pytest` 13.2%.
+- Supply-chain gate: 5,279 records, 99.5% no-op; logs 5,252 `skip` lines → 11.5MB JSONL noise. `RecentRelease` 3-day cooldown false-positives on mainstream packages (`requests`, `@types/node`); `UnvettableSource` false-positives on running an already-installed local plugin. `KnownVulnerability` blocks (pinned vulnerable `requests`/`urllib3`) are correct and valuable.
+- Tirith gate: 48 downgrades, no clear false positives. The homoglyph catch is genuinely good — blocked a Cyrillic-`і` `github.com` lookalike, 5 rules, 24ms. Keep it.
+- `err` wrapper writes 564 junk `parse_failures` rows (`cmd_ok2_*`, error `"err"`) — test DB likely leaking into the production DB.
+
+## Feature / uplift ideas
+
+- `gain --weak-filters` — rank tools by `input_tokens × (1 − savings)` ("where tokens leak"); `gain --failures` — render the ranked weakness map from `parse_failures`. Data is already collected.
+- Missing filters with real volume: `ps` (1.7M tokens passthrough), `rsync`, `du`, `log show`. Route `rg` → existing grep filter (trivial).
+- `learn` could auto-generate TOML `match_output`/`replace` filter suggestions from frequent correction rules — close the detection→DSL loop.
+- Stop logging supply-chain `skip` records; cache `ccusage` (slow npm subprocess) with a TTL.
+- Token-aware permission matching + substitution-aware lexer decomposition (SEC-C2/C3) close an entire evasion class.
+- Integrity for the binary-command era — hash/verify the `settings.json` hook registration.
+
+## Method
+
+Four read-only agents, app-scoped, run in parallel; findings consolidated here.
+No code changes were made by the review. Severity is the reviewer's assessment;
+"CRITICAL" = exploitable or correctness-breaking, "P0" = operational urgency.

src/cmds/dotnet/dotnet_cmd.rs

@@ -3,7 +3,7 @@
 use crate::binlog;
 use crate::core::stream::exec_capture;
 use crate::core::tracking;
-use crate::core::utils::{secure_dotnet_command, truncate};
+use crate::core::utils::{check_forbidden_dotnet_args, secure_dotnet_command, truncate};
 use crate::dotnet_format_report;
 use crate::dotnet_trx;
 use anyhow::{Context, Result};
@@ -32,6 +32,15 @@ pub fn run_restore(args: &[String], verbose: u8) -> Result<i32> {
 }
 
 pub fn run_format(args: &[String], verbose: u8) -> Result<i32> {
+    // `secure_dotnet_command` strips dangerous *env vars*, but dotnet/MSBuild
+    // also exposes RCE-grade *CLI flags* (`-p:CustomBefore*Targets=<path>`,
+    // `--runsettings`) that env-stripping does not touch. Reject those before
+    // any arg reaches the child process — fail closed (SEC-C1).
+    if let Err(msg) = check_forbidden_dotnet_args(args) {
+        eprintln!("{}", msg);
+        return Ok(2);
+    }
+
     let timer = tracking::TimedExecution::start();
     let (report_path, cleanup_report_path) = resolve_format_report_path(args);
     // `secure_dotnet_command` strips DOTNET_STARTUP_HOOKS /
@@ -80,6 +89,15 @@ pub fn run_passthrough(args: &[OsString], verbose: u8) -> Result<i32> {
         anyhow::bail!("dotnet: no subcommand specified");
     }
 
+    // Reject RCE-grade dotnet/MSBuild CLI flags (`-p:CustomBefore*Targets`,
+    // `--runsettings`) that `secure_dotnet_command`'s env-strip cannot defend.
+    // Fail closed before any arg reaches the child process (SEC-C1).
+    let arg_strs: Vec<String> = args.iter().map(|a| a.to_string_lossy().into_owned()).collect();
+    if let Err(msg) = check_forbidden_dotnet_args(&arg_strs) {
+        eprintln!("{}", msg);
+        return Ok(2);
+    }
+
     let timer = tracking::TimedExecution::start();
     let subcommand = args[0].to_string_lossy().to_string();
 
@@ -117,6 +135,14 @@ pub fn run_passthrough(args: &[OsString], verbose: u8) -> Result<i32> {
 }
 
 fn run_dotnet_with_binlog(subcommand: &str, args: &[String], verbose: u8) -> Result<i32> {
+    // Reject RCE-grade dotnet/MSBuild CLI flags (`-p:CustomBefore*Targets`,
+    // `--runsettings`) that `secure_dotnet_command`'s env-strip cannot defend.
+    // Fail closed before any arg reaches the child process (SEC-C1).
+    if let Err(msg) = check_forbidden_dotnet_args(args) {
+        eprintln!("{}", msg);
+        return Ok(2);
+    }
+
     let timer = tracking::TimedExecution::start();
     let binlog_path = build_binlog_path(subcommand);
     let should_expect_binlog = subcommand != "test" || has_binlog_arg(args);