Support Git-style external subcommands (but-<name>) by schacon · Pull Request #13885 · gitbutlerapp/gitbutler

schacon · 2026-05-19T13:24:21Z

Allow unknown subcommands to invoke PATH helpers named but-
(e.g. but forecast runs but-forecast) while preserving legacy
behavior for filesystem paths. Add argument/docs updates, an external
dispatcher (command/external.rs), wiring into the command dispatch and
metrics, tests for Unix PATH delegation, and update help tests and docs.
Map spawning NotFound errors to the friendly “not a command”
message.

Allow unknown subcommands to invoke PATH helpers named but-<command> (e.g. `but forecast` runs `but-forecast`) while preserving legacy behavior for filesystem paths. Add argument/docs updates, an external dispatcher (command/external.rs), wiring into the command dispatch and metrics, tests for Unix PATH delegation, and update help tests and docs. Map spawning NotFound errors to the friendly “not a command” message.

Copilot

Pull request overview

This PR adds Git-style external subcommand delegation to the but CLI, allowing unknown commands to invoke but-<command> helpers from PATH while retaining existing path-opening behavior.

Changes:

Adds hidden clap external-subcommand parsing and dispatch logic for but-<name> helpers.
Updates CLI error/help tests and adds a Unix delegation test.
Updates metrics matching and CLI reference documentation for external commands.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`crates/but/src/args/mod.rs`	Adds hidden external subcommand parsing and updates top-level CLI docs.
`crates/but/src/command/external.rs`	Implements external helper/path dispatch.
`crates/but/src/command/mod.rs`	Wires in the external command module.
`crates/but/src/lib.rs`	Delegates parsed external subcommands before normal command handling.
`crates/but/src/utils/metrics.rs`	Handles the external subcommand variant in metrics command mapping.
`crates/but/tests/but/command/external.rs`	Adds Unix test coverage for PATH helper delegation.
`crates/but/tests/but/command/help.rs`	Updates friendly error test for path-like unknown input.
`crates/but/tests/but/command/mod.rs`	Registers the external command test module.
`crates/but/skill/references/reference.md`	Documents external command helper behavior.

+
+fn is_plain_command_prefix(token: &OsStr) -> bool {
+    let s = token.to_string_lossy();
+    !s.contains('/') && !s.contains(std::path::MAIN_SEPARATOR) && s != ".."


Byron

I took a quick look and tried really hard to abuse this, also in comparison to how Git implements it. Codex helped with that, and I filtered it down to just one possible issue.

It would be great to also summon @slarse to the devsec party.

research by codex

Summary for GitHub:

git foo dispatches through Git’s top-level wrapper. Git parses global options, prepends its trusted exec path (GIT_EXEC_PATH / compiled gitexecdir) to PATH, tries builtins first, then invokes an external git-foo via argv-based process execution. It does not run a shell for normal external commands. Shell execution is limited to explicit ! aliases. Git also prevents aliases from overriding normal commands, except deprecated commands, and repository safety checks such as safe.directory limit reading config/hooks from untrusted ownership contexts. External git-foo helpers are still trusted code from the effective PATH.

The Rust but-{name} dispatcher has the important no-shell property too: Command::new(...).args(...) passes arguments directly. The larger difference is that it relies on the ambient PATH without first prepending or prioritizing a trusted helper directory. That makes execution sensitive to PATH order, relative PATH entries, current-directory entries, or writable directories ahead of the intended install location. It also constructs the helper name with to_string_lossy(), which can alter non-UTF-8 input, and uses a permissive command-name check.

Suggested hardening:

Try builtins before external helpers.
Resolve helpers from a trusted install/libexec directory before ambient PATH.
Resolve the executable to an absolute path before setting current_dir.
Build but-<name> as an OsString, avoiding lossy Unicode conversion.
Restrict helper names to a simple ASCII allowlist such as [A-Za-z0-9-]+.
Treat but-* helpers as trusted plugin code and document that clearly.
Consider sanitizing environment/file descriptor inheritance if helpers may run in sensitive contexts.

Byron · 2026-05-19T14:38:49Z

+        Err(e) => {
+            return Err(e).with_context(|| format!("could not invoke `{prefixed}` from PATH"));
+        }
+    };


I wondered if there was anything else to implementing this, and if this somehow could be abused.
But overall, I think there is "absolutely nothing to worry about"™️.

I don't really see anything either. This seems fine.

Byron · 2026-05-19T14:46:05Z

+    if extra.len() == 1 {
+        let candidate = Path::new(&extra[0]);
+        let resolved = resolve_against_current_dir(current_dir, candidate);
+        if resolved.exists() {
+            return crate::command::gui::open(&resolved)
+                .with_context(|| format!("failed to open path `{}`", resolved.display()));
+        }
+    }


This could be problematic as attacker-controlled repos with submodules maybe can now bait users to open their repos.
It's far-fetched though as this gui::open just launches the GUI on a given directory that also must be a repo to get anywhere, and the question is if this is abusable. Maybe it is if the GUI is vulnerable to certain injection attacks.

Git doesn't have that problem.

It feels like to mitigate this, one would have to search the PATH by hand, and if a program exists one would always have to execute it.

There's an easy solution to this: Kill it. We decided on a removal of this feature in the last engineering meeting as it's a low-frequency command that really doesn't deserve top-level placement. Doing but gui <path> instead is perfectly serviceable.

slarse

I don't really have any major reservations here. While of course it opens up for some amount of misuse, the utility of the feature outweighs any such concerns IMO. Also, if a but-malicious is on the path, whether it's accidentally executed through but or by just running but-malicious outright doesn't seem like a great distinction.

If we want to be extra prudent about it, we could have a command allow-list in our app settings, or potentially just an "allow external commands" setting (e.g. but config external enable). That would limit any potential security concerns to the probably very small subset of people that would ever use this feature. I think that kind of toggle might be worthwhile.

Other than that I think we should get this in ASAP. @schacon @Byron wdyt?

slarse · 2026-05-20T06:05:32Z

+    if extra.len() == 1 {
+        let candidate = Path::new(&extra[0]);
+        let resolved = resolve_against_current_dir(current_dir, candidate);
+        if resolved.exists() {
+            return crate::command::gui::open(&resolved)
+                .with_context(|| format!("failed to open path `{}`", resolved.display()));
+        }
+    }


There's an easy solution to this: Kill it. We decided on a removal of this feature in the last engineering meeting as it's a low-frequency command that really doesn't deserve top-level placement. Doing but gui <path> instead is perfectly serviceable.

slarse · 2026-05-20T06:11:56Z

+        Err(e) => {
+            return Err(e).with_context(|| format!("could not invoke `{prefixed}` from PATH"));
+        }
+    };


I don't really see anything either. This seems fine.

slarse · 2026-05-20T06:18:50Z

We need a lot more testing for the negative cases

slarse · 2026-05-20T06:20:37Z

+#[cfg(all(unix, feature = "legacy"))]
+mod external;


What's legacy about this? And should we also support Windows?

Indeed, no need.

Copilot AI review requested due to automatic review settings May 19, 2026 13:24

github-actions Bot added rust Pull requests that update Rust code CLI The command-line program `but` labels May 19, 2026

schacon force-pushed the sc-external-commands branch from 60b0cf3 to e63a3ed Compare May 19, 2026 13:25

Copilot AI reviewed May 19, 2026

View reviewed changes

Comment thread crates/but/src/command/external.rs

fn is_plain_command_prefix(token: &OsStr) -> bool {

let s = token.to_string_lossy();

!s.contains('/') && !s.contains(std::path::MAIN_SEPARATOR) && s != ".."

Byron reviewed May 19, 2026

View reviewed changes

slarse reviewed May 20, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Git-style external subcommands (but-<name>)#13885

Support Git-style external subcommands (but-<name>)#13885
schacon wants to merge 1 commit into
masterfrom
sc-external-commands

schacon commented May 19, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Byron left a comment

Uh oh!

Byron May 19, 2026

Uh oh!

slarse May 20, 2026

Uh oh!

Byron May 19, 2026

Uh oh!

slarse May 20, 2026

Uh oh!

slarse left a comment

Uh oh!

slarse May 20, 2026

Uh oh!

slarse May 20, 2026

Uh oh!

slarse May 20, 2026

Uh oh!

slarse May 20, 2026

Uh oh!

Byron May 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

schacon commented May 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slarse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants