Repository navigation
feat(plugins): let a host command run only verified bytes - #2535
Conversation
A host command always ran the first matching program on PATH, so a plugin could not pin one to the executable it installed. Commands may now declare the same `artifact` descriptor as processes; each run verifies the installed bytes in place and never looks the program up on PATH. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Alex Rosenzweig <[email protected]>
Also spell out the command artifact descriptor and its PATH caveat for plugin authors. Co-Authored-By: Claude Opus 5.5 <[email protected]> Signed-off-by: Alex Rosenzweig <[email protected]>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 017b46d56d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tokio::task::spawn_blocking(move || crate::host_process::verified_artifact(artifact)) | ||
| .await | ||
| .ok()? | ||
| .ok()? |
There was a problem hiding this comment.
When another process can write the artifact directory—the threat this hash pinning is intended to mitigate—verified_artifact closes the verified files and returns only their path, and this code subsequently reopens that path during Command::spawn. An attacker can atomically replace the executable or a declared sibling between verification and spawning, causing unchecked bytes to run and potentially forge approval decisions. Launch from an immutable verified snapshot or otherwise bind execution to the same opened file rather than resolving the path again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[P1] Bind command execution to the verified bytes
I agree this remains an integrity gap: verification returns a pathname, while execution subsequently opens that pathname again. The new regression test checks modification between separate calls, not whether the verified bytes remain the bytes executed. Please use a mutation-protected verified generation or an equivalent guarantee covering the executable and declared siblings.
Posted by Codex (gpt-6.1-sol) on behalf of kylez
joahg
left a comment
joahg
left a comment
There was a problem hiding this comment.
🤖 Closes the PATH-hijack gap for host commands, with fail-closed behavior and tests for both the tampered and the valid path. Approving with the TOCTOU caveat in the inline comment.
| tokio::task::spawn_blocking(move || crate::host_process::verified_artifact(artifact)) | ||
| .await | ||
| .ok()? | ||
| .ok()? |
There was a problem hiding this comment.
🤖 P2 (not blocking): verify_artifact returns a path that spawn reopens, so a writer to the artifact directory can swap the file between verification and exec. This matches the existing uncontained-process behavior and the docs say verification is in place, and it fixes the PATH hijack the PR targets. A follow-up that execs a verified snapshot (or the same opened fd) would close the race Codex and Kyle flagged.
…-service * origin/main: (29 commits) Normalize profile names for verified-name matching (#2348) Add BuzzTerm font settings and compact glyph spacing (#2502) feat(plugins): let a host command run only verified bytes (#2535) NEXUS-4228: Add plugin toolbar actions and panel links (#2645) Keep plugin update controls stable and spaced (#2636) feat(inbox): rename to Activity and simplify the filter bar (#2543) fix(goose): track every replaced protected worker and check its security gate (#2528) docs(agents): say Pi agents don't take plugin notes yet (#2534) feat(agents): reorder agent cards with hold and drag (#1837) fix(messages): keep own sends from before the loaded history off the timeline (#2525) feat(github): rework the GitHub settings page (#2450) fix(mcp): accept create scopes in durable approvals (#2650) feat(bestie): Bestie tab and Remembers you for a hosted Bestie (#2478) fix(relay): p-tag a one-to-one DM's agent partner on new messages (#2475) fix(agents): show each message author's username and display name (#2628) fix(agents): preserve visible edits in conversation history (#2355) fix(canvas): keep todo edits from approving unrelated instructions (#2350) fix(messages): keep messages in their canonical channel (#2349) NEXUS-4165: Add Thinking to agent creation (#2409) feat(plugins): let a plugin paint a backdrop behind the shell (#2437) ... # Conflicts: # src/bundled/goose/runtime.ts
…leanup * origin/main: Normalize profile names for verified-name matching (#2348) Add BuzzTerm font settings and compact glyph spacing (#2502) feat(plugins): let a host command run only verified bytes (#2535) NEXUS-4228: Add plugin toolbar actions and panel links (#2645) Keep plugin update controls stable and spaced (#2636) feat(inbox): rename to Activity and simplify the filter bar (#2543) # Conflicts: # src/bundled/inbox/InboxPage.test.tsx # tests/browser/inbox-archive.spec.mjs # tests/fixtures/profiles.tsx
Fixes the "Hash-checked host commands" follow-up from #2334 (#1875 finding A).
Plugin host commands always ran the first matching program on PATH, and the host refuses absolute program paths. So the Buzz Security companion's
status,reviewand checker commands runenvfrom the user's PATH. Among other things, those commands record approval decisions. Anything that can put anenvearlier on PATH, such as~/.local/bin(first on macOS), would run in their place.Changes
crates/plugin-manager/src/lib.rs).host.commandsentries may declare the same optionalartifact: {path, sha256, siblings?}thathost.processesalready can. It is checked by the samevalid_artifact. Manifests without it parse and serialize unchanged.src-tauri/src/host_command.rs,src-tauri/src/host_process.rs).verify_artifactas uncontained processes, after the same~/expansion.null, with no PATH fallback.src/plugins/api.ts, FOUNDATION). One optionalartifactfield on commands, matching the process descriptor. The owner approved this.docs/agent-protection.mdsays commands may declare an artifact and are verified in place, not snapshotted.docs/plugin-authoring-guide.mdspells out the descriptor. It also notes that interpreters and programs the command calls by name are still found on PATH, so use an absolute shebang and absolute paths.Release order
artifact(deny_unknown_fields), and fail closed with an install error.Testing
cargo test validates_host_declarationsincrates/plugin-managerpasses. A valid and a~/descriptor are accepted; a relative path and an uppercase hash are refused.cargo test --lib host_command::insrc-tauripasses: 15 tests.artifact_commands_run_only_verified_bytesruns a pinned script.env, so the test also proves that a tampered script returnsnullinstead of falling back.pnpm typecheckpasses.host_commandtests failed once. They passed on two unloaded reruns. This change doesn't touch them.Review checklist
tauri devrun with the updated companion installed from folder. The status command should load, and approving or denying a pending request should record the decision.🤖 Generated with Claude Code