Use an absolute thv path in the Claude Desktop helper shim - #6354
Use an absolute thv path in the Claude Desktop helper shim#6354jerm-dro wants to merge 3 commits into
Conversation
The bare "thv llm token" token helper never runs for GUI-launched clients. macOS GUI apps inherit launchd's environment, not the user's shell PATH, and thv installs to ~/.toolhive/bin, which launchd's default PATH does not include. Claude Desktop is broken unconditionally since it is only ever GUI-launched; its setup still reports success and the failure surfaces later as an auth error. Claude Code is broken whenever it is started from the Dock or Spotlight. The same PATH dependency also lets any writable directory earlier on PATH shadow thv and return an attacker-chosen token. Single-quote the absolute path from os.Executable() on POSIX. Quoting is what the previous attempt could not do portably, but the two consumers here only ever run through /bin/sh, where single-quoting is a total transform: no byte inside a single-quoted run is special, so no path needs rejecting and the shim's metacharacter blocklist is deleted rather than widened. Claude Desktop's shim switches to the TokenHelperPath already plumbed to it and previously ignored. Windows keeps the bare command. cmd.exe has no equivalent quoting for backslash-bearing paths, and leaving that branch byte-identical to what ships today means it cannot regress; Dock-equivalent launches there still need thv on the GUI PATH.
github-actions
Bot
added
size/M
|
Follow-up for the deliberately-deferred Windows half of this: #6355 (token helper still resolves via PATH on Windows — GUI-launch gap plus PATH-shadowing vector, unfixed here because that branch is the one no CI runner covers). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6354 +/- ##
==========================================
- Coverage 72.97% 72.95% -0.02%
==========================================
Files 742 742
Lines 78398 78392 -6
==========================================
- Hits 57214 57194 -20
- Misses 17194 17210 +16
+ Partials 3990 3988 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Scope the absolute-path fix to Claude Desktop, which is the only client that cannot use a PATH-resolved command: it is exclusively GUI-launched, so it inherits launchd's environment and never finds thv on PATH. Claude Code keeps the bare "thv llm token". It is terminal-oriented, and the bare form re-resolves on every invocation, so upgrading or relocating thv keeps working without re-running "thv llm setup". This also drops the platform branch: with no direct-mode client interpolating a path, there is nothing that needs different quoting in /bin/sh and cmd.exe. Move the shell escaper to pkg/client alongside its only remaining caller, unexported, rather than leaving new exported surface in pkg/llmgateway for a single consumer.
jerm-dro
changed the title
github-actions
Bot
added
size/S
|
Scope corrected in 7d7dc7d after maintainer feedback: direct-mode clients (Claude Code) keep the bare The absolute path is now scoped to Claude Desktop alone, the one client that cannot use a PATH-resolved command since it is exclusively GUI-launched. Consequences:
Title and description updated to match. #6355 closed as not planned; its premise was mine, and it was wrong. |
github-actions
Bot
added
size/S
There was a problem hiding this comment.
Pull request overview
This PR fixes Claude Desktop’s LLM gateway credential-helper shim on macOS by avoiding reliance on PATH (GUI-launched apps inherit launchd’s minimal environment). It updates the shim generator to embed an absolute thv path with POSIX-safe quoting, while keeping direct-mode clients on the bare thv llm token command to preserve relocatability.
Changes:
- Claude Desktop shim now uses
ApplyConfig.TokenHelperPathand POSIX single-quoting to safely execthvwith arguments. - Removes the previous token-helper command validator in favor of a total quoting transform and stronger execution-based tests.
- Updates documentation/comments around why direct-mode keeps the bare command.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/client/llm_gateway_credential_helper.go | Generate Claude Desktop shim using absolute TokenHelperPath with POSIX quoting; removes old validation logic. |
| pkg/llm/setup.go | Clarifies rationale/tradeoffs for keeping direct-mode helper as bare thv resolved via PATH. |
| pkg/client/llm_gateway_credential_helper_test.go | Adds quoting and real-shell execution tests to validate shim safety and behavior. |
Suppressed comments (1)
pkg/client/llm_gateway_credential_helper_test.go:339
- Same loop-variable capture issue here:
inis thefor rangevariable and the subtests are parallel, so each closure should capture its own copy (in := in) to avoid flakiness/incorrect assertions.
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")
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // 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. | ||
| // tokenHelperShellCommandFor in pkg/llm). Windows support is a follow-up. | ||
| return "", fmt.Errorf("claude-desktop LLM gateway setup is not supported on Windows yet") |
There was a problem hiding this comment.
Correct, and thanks — fixed in d6011a5. Leftover from renaming this to tokenHelperShellCommandFor for a platform branch, then dropping the branch after maintainer feedback; the reference reverted everywhere except here. Now reads tokenHelperShellCommand, which is the symbol that exists.
| func (cm *ClientManager) writeCredentialHelperShim(tokenHelperPath string) (string, error) { | ||
| if tokenHelperPath == "" { | ||
| // A caller bug (os.Executable() failed upstream), not user input: fail | ||
| // closed rather than emitting a shim that cannot locate thv. | ||
| return "", fmt.Errorf("no token-helper path available for credential helper shim") | ||
| } | ||
| shimPath := cm.credentialHelperShimPath() |
There was a problem hiding this comment.
Good catch — adopted in d6011a5. The guard is now filepath.IsAbs rather than a non-empty check:
if !filepath.IsAbs(tokenHelperPath) {
return "", fmt.Errorf(
"credential helper shim requires an absolute token-helper path, got %q", tokenHelperPath)
}Your framing is exactly right: defeating PATH resolution is the whole point of the shim, so accepting a relative path would silently reintroduce the ambiguity — resolved against whatever working directory Claude Desktop happens to have, which is worse than the bare-thv case since it is not even predictable.
Two notes:
filepath.IsAbs("")is false, so this subsumes the previous empty-string check rather than adding a second guard.- Verified it cannot break real usage:
os.Executable()is documented to return an absolute path unless it errors. Confirmed empirically by invoking thv through a relative path (./bin/thv llm setup) — setup still succeeds, so the recorded path is absolute regardless of how thv was launched.
TestWriteCredentialHelperShim_RequiresAbsolutePath now covers "", thv, ./thv and ../bin/thv, and additionally asserts no shim file is left behind when the path is rejected.
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| assert.Equal(t, tc.want, quoteForPOSIXShell(tc.in)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Respectfully declining this one — the loop-capture bug it describes was fixed in the language itself.
Go 1.22 changed for loops to create a new variable per iteration (spec, wiki), so tc := tc has been a no-op since then. This module is go 1.26 (see go.mod), so the per-iteration semantics apply.
Verified rather than assumed — a table of 3 cases with parallel subtests under go 1.26:
distinct values observed: 3
--- PASS: TestCapture/a
--- PASS: TestCapture/c
--- PASS: TestCapture/b
All three observe their own tc, in non-deterministic completion order. Under the pre-1.22 semantics this would have reported 1 distinct value.
Worth noting the file still contains one tc := tc at line 270, in a test that predates this PR. That is now dead code for the same reason — but removing it is unrelated to this change, so I have left it rather than widening the diff.
Defeating PATH resolution is the entire point of the shim, but the writer accepted any non-empty string. A relative path would have silently reintroduced PATH-style ambiguity, resolved against whatever working directory Claude Desktop happens to run with. os.Executable() is documented to return an absolute path unless it errors, so a non-absolute value is a caller bug: reject it instead of writing a shim that cannot reliably locate thv. filepath.IsAbs also rejects the empty string, so this subsumes the previous empty check. Also fix a comment naming a symbol that no longer exists.
github-actions
Bot
added
size/S
|
All three Copilot comments addressed in d6011a5 — two adopted, one declined with evidence (replies inline):
Note on the red check: |
Summary
Claude Desktop's LLM gateway auth is broken in every case on macOS. Its credential-helper shim runs
exec thv llm token, but Claude Desktop is only ever GUI-launched, and macOS GUI apps inherit launchd's environment rather than the user's shell PATH. thv installs to~/.toolhive/bin/thv, which is not in launchd's default/usr/bin:/bin:/usr/sbin:/sbin:thv llm setupreports success; the failure only surfaces later, as an auth error, when Desktop invokes the helper. This regressed in #6326, which replaced the interpolatedos.Executable()path with a barethvto fixthv llm setupon Windows.The absolute path was already plumbed and simply unused.
pkg/llm/setup.gosetsTokenHelperPathon theApplyConfigpassed to every client, butconfigureCredentialHelperread onlyTokenHelperCommand.What changed:
pkg/client: the Claude Desktop shim now consumescfg.TokenHelperPathand single-quotes it into the exec lines. Adds an unexportedquoteForPOSIXShellnext to its only caller.isSafeTokenHelperCommandand its metacharacter blocklist (−1 function, net −54 lines in that file). The blocklist barred\,'and", so it would in fact have rejected a legitimately quoted path.buildTokenHelperCommandreferences left by Use a bare thv command as the LLM token helper #6326.Deliberately unchanged: direct-mode clients (Claude Code) keep the bare
thv llm token. They are terminal-oriented, and the bare form re-resolves on every invocation, so upgrading, reinstalling, or relocating thv keeps working without re-runningthv llm setup. That property is worth more there than closing the Dock-launch gap. Claude Desktop is the one client that cannot make that trade, since it is only ever GUI-launched.Because no direct-mode client interpolates a path any more, there is no platform branch: nothing needs different quoting for
/bin/shandcmd.exe. The shim is POSIX-only by construction —configureCredentialHelperhard-errors on Windows — so single-quoting is the only scheme required.Why dropping validation is safe: on POSIX, single-quoting is a total transform. Inside a single-quoted run no byte is special to the shell — not
$, backtick, backslash, or even a newline — so there is no input to reject and no metacharacter validation is needed to keep the 0700 script injection-free.Type of change
Test plan
task test)task test-e2e)task lint-fix)Manual, end-to-end with the real binary. Installed
thvat/tmp/e2e/bin dir's/thv— a path containing both a space and an apostrophe — and ran realthv llm setupagainst a sandboxedHOME.Claude Desktop's shim, executed under a launchd-like PATH that excludes thv:
The remaining secrets error is an unconfigured sandbox, far past the point of failure. Also confirmed Claude Code's
apiKeyHelperis still written as the barethv llm token, unchanged by this PR.New automated coverage:
TestQuoteForPOSIXShell_SurvivesRealShellround-trips 12 hostile strings through/bin/shand asserts byte-identical output, plus a control proving an unquoted string does not survive (so it cannot pass trivially).TestWriteCredentialHelperShim_ExecutesWithHostilePathexecutes the generated shim with a path containing',",$(id), backticks,;and a newline, and asserts the received argv — this is the evidence that justifies deleting the validator.TestWriteCredentialHelperShim_UsesAbsolutePathpins that the interactive branch does not pass--skip-browser.task docsproduces no diff (no CLI surface changed).Changes
pkg/client/llm_gateway_credential_helper.goTokenHelperPath; addquoteForPOSIXShell; deleteisSafeTokenHelperCommand; fix stale commentspkg/llm/setup.gopkg/client/llm_gateway_credential_helper_test.goDoes this introduce a user-facing change?
Yes.
thv llm setupnow writes an absolute path to thv in Claude Desktop's credential-helper shim, fixing LLM gateway auth for Claude Desktop on macOS, where it was previously broken in every case. Affected users should re-runthv llm setup. No change for Claude Code, Codex, Gemini CLI, or any Windows client.Special notes for reviewers
Deleting
isSafeTokenHelperCommandremoves a defense-in-depth layer. I believe it is correct — the escaper makes every byte inert, so the blocklist protected nothing an attacker controllingTokenHelperPathcouldn't reach by controlling the wholeApplyConfig— but it is a net reduction in belt-and-braces and deserves a deliberate look rather than being buried in the diff.TestWriteCredentialHelperShim_ExecutesWithHostilePathis the replacement guarantee, and it is strictly stronger: it asserts behavior under a real shell instead of the shape of a string.The shim's path is now a setup-time snapshot. If a user moves or deletes the binary, the Desktop helper breaks until
thv llm setupis re-run. Verified favorable:os.Executable()does not resolve symlinks on darwin, so Homebrew's/opt/homebrew/bin/thvsymlink is recorded rather than the versioned Cellar target, andbrew upgradekeeps working. Failure is loud and local, versus today's silent-and-unconditional break. This staleness risk is precisely why direct-mode clients keep the bare, re-resolving command.quoteForPOSIXShellis unexported inpkg/client, not shared. With one consumer, exported surface inpkg/llmgatewaywasn't warranted. Notepkg/tui/inspector.gohas its own equivalent; unifying them is unrelated scope.Generated with Claude Code