feat(capi): expose host-directory mounts over the C ABI - #2371
Conversation
Bring the C API to parity with the JS/Python bindings' real filesystem
mounts, behind the same safety model:
- config v1 gains optional mounts: [{path, root, writable}] and
allowed_mount_paths; mounts are applied after build via the live
Bash::mount API (RealFs wrapped in PosixFs), and readonly_filesystem
continues to wrap mounted filesystems
- new bashkit_mount / bashkit_unmount exports attach and detach host
directories on a running session, preserving shell state
- every mount root must resolve under an allowed_mount_paths prefix;
roots are canonicalized before the prefix check so '..' segments and
symlinks cannot escape, and comparison is case-folded on Windows
- capabilities_json gains the realfs-mounts feature marker so embedders
can feature-detect support
- bashkit.def and include/bashkit.h extended additively; ABI v1
signatures are unchanged
- three new ABI tests: read-only mounts (host file provably absent after
denied writes), runtime mount/unmount round trip, and allowlist
enforcement (missing allowlist and out-of-prefix roots rejected)
|
Context that might be useful for review: this patch is already driving a complete Java binding for the C API — Bashkit4j (MIT, If it fits the project, we'd be glad to see the Java binding mentioned wherever you list integrations or language bindings — but that's entirely your call, no expectations. Either way, happy to adjust the patch however review requires. |
There was a problem hiding this comment.
Thanks for the contribution — the ABI additions (bashkit_mount/bashkit_unmount, additive .def/.h changes, capability marker) are clean and the round-trip/allowlist tests are a good start. Two blockers found by building this branch directly and adding a couple of verification tests:
-
Security gap vs. stated parity (
validate_mount_rootincrates/bashkit-capi/src/lib.rs): the config-timemounts+allowed_mount_pathspath bypasses the existing TM-FS-013 sensitive-path denylist (.ssh,.aws,.kube,.docker,.gnupg,.gcloud,/etc,/root,/home, …) thatbashkit-js/bashkit-pythoninherit for free by routing throughBashBuilder::mount_real_readonly_at+.allowed_mount_paths(...). I confirmed with a PoC test that allowlisting a home-style directory and mounting its.sshsubdirectory exposes private key contents through the shell — something the existing builder API already refuses in the equivalent scenario. See inline comment for the repro and suggested fix (route through the builder, or reuseis_sensitive_mount_path). The newTHREAT[TM-SBX-XXX]code comment also isn't backed by a real, registered entry inknowledge/security/threat-model.md(TM-SBXdoesn't exist yet, andXXXis a literal unfilled placeholder) — this should extend TM-FS-013 instead. -
Breaks the mandatory clippy gate:
RealFs::newis#[deprecated]; both new call sites lack the#[allow(deprecated)]the one existing sync call site uses. Confirmedcargo clippy --all-targets --all-features -- -D warningsfails to compile on this branch as-is.
Happy to take another pass once these are addressed. (Separately, re: the linked Java binding / Bashkit4j mention in the top-level comment — that's a call for the maintainers, not a review blocker.)
| } | ||
| let path = PathBuf::from(root); | ||
| let canonical = std::fs::canonicalize(&path).unwrap_or(path); | ||
| let candidate = fold_path(&canonical.to_string_lossy()); |
There was a problem hiding this comment.
validate_mount_root only checks that the (canonicalized) root falls under an allowed_mount_paths prefix — it never calls the existing Bash::is_sensitive_mount_path / SENSITIVE_MOUNT_PATHS / SENSITIVE_PATH_COMPONENTS denylist that apply_real_mounts enforces for the mount_real_readonly_at/allowed_mount_paths builder path used by the JS and Python config-time bindings (see crates/bashkit/src/lib.rs around L3418-3462, threat TM-FS-013).
Concretely: an embedder that sets allowed_mount_paths: ["/home/user"] (a very natural, broad allowlist entry) and then mounts /home/user/.ssh gets refused by mount_real_readonly_at + allowed_mount_paths (JS/Python config path), but is accepted here. I verified this with a PoC test added on top of this branch:
#[test]
fn poc_sensitive_subdir_not_blocked_by_config_allowlist() {
unsafe {
let home = temp_dir("poc-home");
let ssh_dir = home.join(".ssh");
std::fs::create_dir_all(&ssh_dir).unwrap();
std::fs::write(ssh_dir.join("id_rsa"), b"PRIVATE-KEY-BYTES").unwrap();
let config = serde_json::json!({
"schema_version": 1,
"allowed_mount_paths": [home.to_string_lossy()],
"mounts": [{"path": "/data", "root": ssh_dir.to_string_lossy()}],
}).to_string();
let mut bash = ptr::null_mut();
let mut error = ptr::null_mut();
assert_eq!(bashkit_create_json(bytes(config.as_bytes()), &mut bash, &mut error), BashkitStatus::Ok);
let mut result = ptr::null_mut();
bashkit_execute(bash, bytes(b"cat /data/id_rsa"), &mut result, &mut error);
assert_eq!(borrowed(bashkit_result_stdout(result)), b"PRIVATE-KEY-BYTES"); // passes today
}
}This reads .ssh/id_rsa straight through the mount. The PR description calls this "parity" with JS/Python, but for config-time mounts it's actually weaker than both: it drops TM-FS-013's defense-in-depth entirely and relies solely on the caller's own allowed_mount_paths precision.
Two asks:
- Route
apply_config_mountsthroughBashBuilder::mount_real_readonly_at/mount_real_readwrite_at+.allowed_mount_paths(...)(likebashkit-js/bashkit-pythonalready do for their JSON/dict config), or otherwise callis_sensitive_mount_path(needs to becomepub(crate)/exported) fromvalidate_mount_rootso config-time mounts get the same denylist. bashkit_mount(the live/runtime export) matches the existing live-mount precedent inbashkit-js'senforce_mount_policy(also allowlist-only, no sensitive-path check), so that asymmetry may be intentional/pre-existing — but please confirm that's a deliberate "explicit runtime call = informed consent" design choice and not an oversight, since it's not stated anywhere.
Either way, the new THREAT[TM-SBX-XXX] comment introduces a brand-new, never-registered threat ID (XXX is a literal placeholder, and TM-SBX doesn't exist in knowledge/security/threat-model.md) instead of referencing/extending TM-FS-013, which already covers exactly this class of issue for RealFs mounts. Per this repo's knowledge contract, a security-relevant behavior change needs a real entry in knowledge/security/threat-model.md, not an ad hoc placeholder ID left in code.
There was a problem hiding this comment.
Fixed in bd4f257. Two notes on the approach:
-
I went with reusing the denylist rather than routing through the builder, and one premise turned out to differ from the code:
apply_real_mountsonly consultsis_sensitive_mount_pathwhen no allowlist is configured (crates/bashkit/src/lib.rs, theelse if is_sensitivebranch). With an allowlist set, a covering entry mounts even a sensitive path — so the Rust builder,bashkit-js(enforce_mount_policyis allowlist-only), andbashkit-pythonall accept exactly your PoC scenario (allowlist~, mount~/.ssh). Routing through the builder would therefore not have blocked the repro. -
validate_mount_rootnow calls the denylist on the canonical root for both config-time and runtime mounts, with an explicit-consent rule: a sensitive root additionally requires an allowlist entry that names it exactly — a broad parent entry (the home directory itself) is not consent to expose.ssh. Naming the sensitive root itself in the allowlist still mounts it. This is deliberately stricter than builder/JS; recorded inknowledge/runtimes/c-api.mdand the TM-FS-013 row.is_sensitive_mount_pathis now a public free function (bashkit::is_sensitive_mount_path) so the denylist has a single home and embedders can reuse it.
Regressions added in crates/bashkit-capi/tests/abi.rs: your PoC scenario at config time, the same refusal at runtime (mount refused, path unresolved), and the exact-entry consent case.
| } else { | ||
| RealFsMode::ReadOnly | ||
| }; | ||
| let fs = RealFs::new(&root, mode).map_err(|error| { |
There was a problem hiding this comment.
RealFs::new is #[deprecated] ("blocks on host filesystem I/O; use RealFs::open(...).await"). This PR's two new call sites (here and the bashkit_mount one below) don't suppress it, so cargo clippy --all-targets --all-features -- -D warnings — which this repo's CI/pre-PR checklist runs — fails to compile:
error: use of deprecated associated function `bashkit::RealFs::new`: blocks on host filesystem I/O; use RealFs::open(...).await
--> crates/bashkit-capi/src/lib.rs:270:26
error: could not compile `bashkit-capi` (lib) due to 2 previous errors
(Confirmed by running clippy against this branch.) The one existing sync call site in crates/bashkit/src/lib.rs (apply_real_mounts) suppresses this deliberately with #[allow(deprecated)] // BashBuilder::build is intentionally synchronous. — please do the same here with a matching justification, or move these onto the async RealFs::open path via the session's tokio runtime.
There was a problem hiding this comment.
Fixed in bd4f257: both RealFs::new call sites now carry #[allow(deprecated)] with the same justification as apply_real_mounts — the C ABI boundary is synchronous and has no async context at these calls.
- expose bashkit::is_sensitive_mount_path as a public free function so embedder-side mount policies share one denylist with the builder - validate_mount_root now applies that denylist on the canonical root for both config-time mounts and bashkit_mount: a sensitive root (home trees, /etc, .ssh, ...) is only mountable when an allowlist entry names it exactly — a broad parent entry such as the home directory is not consent to expose credential stores - suppress the RealFs::new deprecation at both C-API call sites with the same justification as apply_real_mounts (synchronous FFI boundary) - replace the unregistered THREAT[TM-SBX-XXX] comment with TM-FS-013 and extend the threat-model row plus the C-API knowledge entry - regression tests: sensitive subdir refused at config time and runtime under a broad allowlist entry; exact-entry consent still mounts
|
Both blockers from the review are addressed in bd4f257:
Regressions added: the review's PoC (config time + runtime) and the exact-entry consent case. CI should confirm fmt/clippy/tests on this commit. |
Adds bashkit_cancel / bashkit_clear_cancel backed by the interpreter's shared cancellation token, kept outside the state mutex so cancel stays lock-free while bashkit_execute is blocked. A cancelled execution reports the new BASHKIT_CANCELLED (7) status, and the capabilities JSON gains a "cancellation" feature so bindings can feature-detect. Tests cancel a pending sleep: the request budget polls the token while the command is in flight, whereas loop-based scripts race the profile's command/iteration caps before the flag lands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The re-landed mounts work dropped the workflow_dispatch lib builder that Bashkit4j packaging uses; upstream's c-api-binaries workflow only builds from release tags. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cancellation only lands at command boundaries, so a cancelled sleep is not interrupted until the profile deadline ends it 30s later. Loop over 1-second sleeps instead: a boundary every second, negligible budget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chaliy
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest push (3e3caa6). Good news first: both blockers from the first round are properly fixed, and I verified this by actually building the branch and running the tests, not just reading the diff.
- TM-FS-013 sensitive-path denylist: now enforced for both config-time and runtime mounts via a shared
bashkit::is_sensitive_mount_path, with an exact-allowlist-entry consent rule that's genuinely stricter than the pre-existing builder/JS precedent. My original.ssh-under-$HOMEPoC is now correctly rejected — confirmed locally. Nice fix, and appreciated that it's more conservative than what I asked for, not just the minimum. - Deprecated
RealFs::newbreaking-D warnings: fixed with#[allow(deprecated)]matching the existing convention.cargo clippy -p bashkit-capi -- -D warningsis clean.
Two new things came in with this push that need attention before merge:
- New clippy failure (
crates/bashkit-capi/tests/abi.rs:367): the new cancellation test has a redundant nestedunsafeblock that failscargo clippy -p bashkit-capi --all-targets -- -D warnings(the flag this repo's CI/pre-PR checklist actually uses). Trivial fix — see inline comment. Functionally the test (and everything else) passes;cargo test -p bashkit-capiis 19/19 green. - New workflow file
.github/workflows/build-native-libs.ymlduplicates the existingc-api-binaries.yml, uses unpinned/mutable action tags where every other workflow here pins to a commit SHA, and skips thepermissions:block the rest of CI sets. It reads as infra for the external Bashkit4j binding's own builds rather than something this repo's C API tests need. Suggest dropping it from this PR — see inline comment for details.
Also flagged (non-blocking): bashkit_cancel/bashkit_clear_cancel is a second, unrelated feature riding along in a PR titled around host mounts. The implementation itself is solid and well-tested, so not blocking on it, but consider splitting future PRs like this.
Nice iteration overall — the mount security model is now in good shape. Once the clippy nit is fixed and the workflow file question is resolved, this looks close to mergeable from my side.
Generated by Claude Code
| let writer = observed.clone(); | ||
| let worker = std::thread::spawn(move || { | ||
| let bash = handle as *mut Bashkit; | ||
| unsafe { |
There was a problem hiding this comment.
Both previous blockers are fixed and verified (ran the branch locally):
validate_mount_rootnow enforcesbashkit::is_sensitive_mount_pathwith the exact-entry consent rule — confirmed my earlier PoC (.sshunder a broad$HOMEallowlist) is now rejected, and that it's a genuinely stronger rule than the builder/JS precedent (which only re-checks sensitivity when no allowlist is set at all, as the reply correctly points out). Good catch, and thanks for hoistingis_sensitive_mount_pathto a sharedpub fnrather than duplicating the list.RealFs::newdeprecation is now silenced with#[allow(deprecated)]matchingapply_real_mounts's justification. Confirmedcargo clippy -p bashkit-capi -- -D warnings(lib only) is clean.
However, the new cancellation test introduces a fresh clippy failure under --all-targets (the flag the project's own CI/pre-PR checklist actually runs):
error: unnecessary `unsafe` block
--> crates/bashkit-capi/tests/abi.rs:367:13
|
336 | unsafe {
| ------ because it's nested under this `unsafe` block
...
367 | unsafe {
| ^^^^^^ unnecessary `unsafe` block
The unsafe { ... } inside the std::thread::spawn(move || { ... }) closure at line 367 is redundant since the closure literal is written inside the outer unsafe block at line 336 and inherits it. Confirmed with cargo clippy -p bashkit-capi --all-targets -- -D warnings. Functionally everything passes (cargo test -p bashkit-capi: 19/19 green, including this test), it's purely the extra unsafe that needs to go.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 894720c — the closure body is lexically covered by the test's unsafe block, so the inner block is gone.
| @@ -0,0 +1,98 @@ | |||
| name: Build native libs | |||
There was a problem hiding this comment.
This new workflow is out of scope for a "C API host mounts" PR and looks like it should be dropped rather than merged:
- It duplicates existing infra.
.github/workflows/c-api-binaries.ymlalready builds the C ABI for the same 5 platforms (usingubuntu-24.04-armfor native aarch64 instead of zig cross-compilation) viascripts/build-c-api.sh, with a rigorous-Werror//WXsmoke test against the public header on every platform. This new workflow re-implements a weaker subset of that. - It doesn't follow this repo's action-pinning convention. Every other workflow in
.github/workflows/pins third-party actions to a commit SHA with a version comment (e.g.dtolnay/rust-toolchain@e081816240890017053eacbb1bdf337761dc5582 # 1.95.0,actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1). This file uses mutable tags throughout (dtolnay/rust-toolchain@1.95.0,actions/checkout@v4,Swatinem/rust-cache@v2,taiki-e/install-action@v2,mlugg/setup-zig@v2,actions/upload-artifact@v4), which is a real supply-chain regression versus the rest of CI. - No
permissions:block, unlike every other workflow here that explicitly scopes down tocontents: read. - Per the PR comment thread, this appears to exist to drive the external Bashkit4j Java binding's own release builds rather than anything this repo's C API tests need —
cargo test --release -p bashkit-capiin thetestjob already runs under the existingci.yml/c-api-binaries.ymlsetup, so the only new thing this adds is unpinned, unscoped cross-platform artifact building onworkflow_dispatch.
Given this repo's "small, incremental PR-sized changes" convention, I'd suggest dropping this file from this PR entirely — it's unrelated to the mount/cancellation feature work and would need its own justification (and SHA-pinning, and a decision on whether upstream wants to host bashkit4j's build pipeline at all) as a separate change.
Generated by Claude Code
There was a problem hiding this comment.
Dropped in 99e0ea7. Agreed on all four points. The workflow still lives on the fork's fork-main-backup branch so Bashkit4j packaging can keep using it ad hoc until it's either proposed upstream properly (SHA-pinned, scoped permissions, own justification) or moved into the binding's own repo.
| } | ||
| } | ||
|
|
||
| /// Requests cancellation of the execution currently running on `bash`. |
There was a problem hiding this comment.
Minor process note, not a code issue: bashkit_cancel/bashkit_clear_cancel is a second, independent feature (wiring up the already-existing Bash::cancellation_token()/Error::Cancelled to the ABI) bundled into a PR titled/scoped around host-directory mounts. The implementation itself looks correct and is well tested (lock-free AtomicBool outside the state mutex, sticky-until-cleared semantics, good test coverage), so I'm not blocking on it — but per this repo's preference for small, single-purpose PRs, consider splitting cancellation into its own PR next time so mounts and cancellation can be reviewed/merged/reverted independently.
Generated by Claude Code
There was a problem hiding this comment.
Acknowledged — keeping cancellation in this PR per your non-blocking note, but future feature work goes up as single-purpose PRs.
The worker closure is lexically nested under the test's unsafe block, so its own unsafe block triggers clippy's unused_unsafe under --all-targets.
Duplicates c-api-binaries.yml, uses unpinned action tags, and exists to drive the external Bashkit4j packaging pipeline; it needs its own justification (SHA pinning, permissions block, upstream-hosting decision). A copy is preserved on the fork's fork-main-backup branch.
|
Second round addressed in 894720c + 99e0ea7:
Also confirming your non-blocking note on scope: cancellation stays here per your call; follow-up features will be split out. CI note: upstream runs for this fork still need maintainer approval, so we've been verifying each push via a fork-internal scratch PR (tersePrompts#1) running the same |
What changed
The C API can now mount real host directories into a session, with an
allowlist-first safety model that is at least as strict as the JS/Python
bindings:
mounts: [{path, root, writable}]andallowed_mount_paths. Mounts are applied after build via the existinglive-mount API (
Bash::mountwithRealFswrapped inPosixFs), soreadonly_filesystemcontinues to wrap mounted filesystems.bashkit_mount/bashkit_unmount, attach and detachhost directories on a running session. Shell state (vars, cwd, history) is
preserved across both, matching live-mount semantics elsewhere.
allowed_mount_pathsprefix. Rootsare canonicalized before the prefix check, so
..segments and symlinkscannot escape an allowlisted prefix; comparison is case-folded on Windows.
No allowlist configured → every mount is rejected.
(config-time and runtime) must also clear the shared denylist, now exposed
as
bashkit::is_sensitive_mount_path. A sensitive root (home trees,/etc,.ssh, ...) additionally requires an allowlist entry that names itexactly — a broad parent entry such as the home directory itself is not
consent to expose credential stores. This is deliberately stricter than the
builder/JS live-mount precedent (where any covering entry overrides the
denylist); the decision is recorded in
knowledge/runtimes/c-api.mdand theTM-FS-013 row of the threat model.
capabilities_jsongains therealfs-mountsfeature marker so embedderscan feature-detect instead of failing on config parse.
bashkit.defandinclude/bashkit.hare extended additively — all ABI v1signatures are unchanged.
Why
bashkit-capiis the FFI surface for every non-Rust embedder, but it onlyexposed the in-memory VFS — while
bashkit-jsandbashkit-pythonexposereal filesystem mounts through direct crate bindings. C ABI consumers (Java
via JNA, C/C++, others) currently cannot use live mounts at all. This brings
the C API to parity and makes the allowlist mandatory, not optional.
Before / After
Before, a config containing
mountsis rejected outright bydeny_unknown_fields(invalid configuration), and there is no way toexpose a host directory through the C API.
After (
crates/bashkit-capi/tests/abi.rs, six new tests, all green):mounts+allowed_mount_paths→cat /data/note.txtreturns the host file's content
provably never appears
bashkit_mount→ exec sees the files →bashkit_unmount→ paths fallback, round trip clean
runtime
.ssh) under a broad allowlist entry ($HOME) → rejectedat config time and at runtime, path stays unresolved (review PoC)
Risk
keys, no signature changes. Old configs stay valid on new binaries; old
binaries reject only the new keys (same
deny_unknown_fieldsbehavior asany unknown field today).
the lexical path when
canonicalizefails (root doesn't exist yet); inthat case
RealFs::newfails immediately after, so no unverifiable rootis ever mounted. Windows case-insensitivity handled by path folding.
Checklist
crates/bashkit-capi/tests/abi.rs