Skip to content

Commit 1f9911d

Browse files
authored
fix(shellwrap): quote arguments containing # so the login shell keeps them (#1756)
## Bug Stdio upstreams are launched as `$SHELL -l -c "<joined command>"` (`shellwrap.WrapWithUserShell`). Each argument goes through `Shellescape`, but the POSIX branch only quotes when the argument contains one of `" \t\n\r\"'\\$`;&|<>(){}[]?*~"`, and `#` is not in that set. A word that begins with `#` starts a comment in `sh`/`bash`/`zsh -c`, so the shell silently drops that argument and everything after it. ## Trigger ```json {"name": "slack", "command": "npx", "args": ["-y", "slack-mcp", "#general", "--port", "3000"]} ``` The shell runs `npx -y slack-mcp`. `#general --port 3000` never reaches the child. ## Fix Add `#` to the POSIX special-character set, so such arguments are single-quoted like every other metacharacter. The POSIX escaper copy in `internal/oauth/spawnargv_round2_test.go` is updated to match, since `TestPosixShellescape_MatchesProduction` pins the two together. ## Test `TestShellescape_HashSurvivesShell` checks that `#general` is quoted, then runs `sh -c "printf '%s\n' <escaped args>"` and asserts that all four arguments (`#general --port 3000 a#b`) come back unchanged. ## Commands run - `CGO_ENABLED=0 go test -count=1 ./internal/shellwrap`: ok - `CGO_ENABLED=0 go test -count=1 ./internal/oauth`: ok - `CGO_ENABLED=0 go test -count=1 -run 'Shell|Wrap|Escape|Docker' ./internal/upstream/core`: ok - With the fix reverted (`git stash` of `shellwrap.go`), `-run HashSurvives` fails: it expected `'#general'` but got `#general`, and the shell output came back as `[""]`. - `gofmt -l` on the edited files: no output
1 parent a2ac978 commit 1f9911d

3 files changed

Lines changed: 31 additions & 2 deletions

File tree

‎internal/oauth/spawnargv_round2_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ func posixShellescape(s string) string {
3030
if s == "" {
3131
return "''"
3232
}
33-
if !strings.ContainsAny(s, " \t\n\r\"'\\$`;&|<>(){}[]?*~") {
33+
if !strings.ContainsAny(s, " \t\n\r\"'\\$`;&|<>(){}[]?*~#") {
3434
return s
3535
}
3636
return "'" + strings.ReplaceAll(s, "'", "'\"'\"'") + "'"
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
package shellwrap
2+
3+
import (
4+
"os/exec"
5+
"strings"
6+
"testing"
7+
8+
"github.com/stretchr/testify/assert"
9+
"github.com/stretchr/testify/require"
10+
)
11+
12+
// A word that starts with '#' begins a comment in `sh -c`, so an unquoted
13+
// "#general" would silently drop it and every argument after it.
14+
func TestShellescape_HashSurvivesShell(t *testing.T) {
15+
args := []string{"#general", "--port", "3000", "a#b"}
16+
assert.Equal(t, "'#general'", shellescapeFor("#general", false))
17+
18+
sh, err := exec.LookPath("sh")
19+
if err != nil {
20+
t.Skip("no POSIX sh on PATH")
21+
}
22+
parts := []string{"printf", "'%s\\n'"}
23+
for _, a := range args {
24+
parts = append(parts, shellescapeFor(a, false))
25+
}
26+
out, err := exec.Command(sh, "-c", strings.Join(parts, " ")).Output()
27+
require.NoError(t, err)
28+
assert.Equal(t, args, strings.Split(strings.TrimSuffix(string(out), "\n"), "\n"))
29+
}

‎internal/shellwrap/shellwrap.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ func shellescapeFor(s string, windowsStyle bool) string {
7373
}
7474

7575
// Unix shell special characters: if none present, return as-is.
76-
if !strings.ContainsAny(s, " \t\n\r\"'\\$`;&|<>(){}[]?*~") {
76+
if !strings.ContainsAny(s, " \t\n\r\"'\\$`;&|<>(){}[]?*~#") {
7777
return s
7878
}
7979
// Use single quotes and escape any embedded single quotes.

0 commit comments

Comments
 (0)