Skip to content

fix(interpreter): contain CommandResolver panics - #2328

Merged
chaliy merged 1 commit into
mainfrom
2026-08-21-propose-fix-for-commandresolver-panic-issue
Aug 22, 2026
Merged

fix(interpreter): contain CommandResolver panics#2328
chaliy merged 1 commit into
mainfrom
2026-08-21-propose-fix-for-commandresolver-panic-issue

Conversation

@chaliy

@chaliy chaliy commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Motivation

  • CommandResolver::resolve ran embedder host code on attacker-controlled unresolved names without a panic boundary, allowing resolver panics to unwind out of Bash::exec and break the interpreter availability guarantees (TM-INT-001).
  • The goal is to ensure resolver failures behave like builtin failures: sanitized shell errors with non-zero exit codes instead of unwinding the process.

Description

  • Wrap the resolver call in std::panic::catch_unwind(AssertUnwindSafe(...)) in crates/bashkit/src/interpreter/mod.rs and return a sanitized ExecResult::err(..., 1) on panic.
  • Preserve the existing dispatch: a successfully returned Arc<dyn Builtin> still runs via execute_builtin_arc so before_tool and the builtin catch_unwind boundary remain in effect.
  • Add an integration regression test resolver_panic_becomes_a_sanitized_shell_error in crates/bashkit/tests/integration/command_resolver_tests.rs that asserts a resolver panic becomes a sanitized shell error and does not leak panic payloads.
  • Update API/docs and canonical knowledge (crates/bashkit/src/builtins/mod.rs, knowledge/foundations/builtins.md, knowledge/security/threat-model.md, crates/bashkit/docs/threat-model.md) to document that resolver resolution is panic-contained (TM-INT-010).

Testing

  • Ran the new integration test cargo test -p bashkit --test integration command_resolver_tests::resolver_panic_becomes_a_sanitized_shell_error -- --exact, which passed.
  • Ran the resolver integration subset cargo test -p bashkit --test integration command_resolver_tests::, and all resolver-related tests passed (12 passed, 0 failed).
  • Ran formatting and policy checks with cargo fmt --all and just check-okf, both of which exited successfully.
  • Ran cargo clippy -p bashkit --tests -- -D warnings, which completed without warnings.

Codex Task

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 21, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
bashkit bb97c2a Commit Preview URL Aug 22 2026, 02:08 AM

@chaliy
chaliy force-pushed the 2026-08-21-propose-fix-for-commandresolver-panic-issue branch from 9aa134e to bb97c2a Compare August 22, 2026 02:07

chaliy commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed and rebased onto main. Good fix, and the only one in this batch so far with a test that genuinely fails without the change:

test command_resolver_tests::resolver_panic_becomes_a_sanitized_shell_error ... FAILED
   (with main's interpreter/mod.rs, PR's tests)
test command_resolver_tests::resolver_panic_becomes_a_sanitized_shell_error ... ok
   (with the PR)

CommandResolver::resolve is embedder code reached with an attacker-controlled name, and it was the one host callback on that path not already inside a catch_unwind. Containing it and returning the sanitized resolver failed unexpectedly matches how TM-INT-002 already handles builtin panics, and the assertion that the panic payload is not in stderr is the right thing to pin.

Renumbered TM-INT-010 → TM-INT-011. #2331 (now merged) also claimed TM-INT-010; the rebase surfaced this as a direct conflict in both threat-model files. Resolved by keeping both rows and giving this one 011, updating the THREAT[...] marker in interpreter/mod.rs and the "Current Risk" prose to match.

Also merged the knowledge/foundations/builtins.md conflict by hand — main had reworded that paragraph's punctuation while this PR extended its content, so both changes are preserved rather than one overwriting the other.

Full command_resolver suite (15 tests) and the threat-model doc tests pass.


Generated by Claude Code

@chaliy
chaliy merged commit 5ab8272 into main Aug 22, 2026
48 checks passed
@chaliy
chaliy deleted the 2026-08-21-propose-fix-for-commandresolver-panic-issue branch August 22, 2026 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant