Conversation
The shell-argv matching in kittyproxy/pattern.go is not kitty-specific: it models a fixed command grammar that any terminal proxy needs to vet what a sandbox asks the host to run. Move it to internal/cmdpattern so a second proxy can share it instead of copying 300 lines of security-critical matching logic. OwnedSet moves too and becomes generic over the id type. kitty identifies windows by int; other protocols use strings. No behavior change. kittyproxy keeps type and function aliases so existing call sites (notably tools/revdiff.go) compile unchanged, and alias_test.go guards those re-exports - a broken alias would otherwise surface only as a compile error in another package. The moved matcher tests are the behavioral contract and are carried over verbatim.
A launch pattern matched argv[0] on basename alone, so any path ending in the allowed program name was accepted - including one inside a directory the sandbox can write. That is exploitable. Most of the sandbox home is an overlay whose writes never reach the host, but the revdiff IPC directory is a write-through bind mounted at an *identical path* on both sides. Sandboxed code could drop its own executable at <ipc-dir>/revdiff, name it in a `kitty @ launch` request, pass validation on basename, and have kitty run it on the host as the host user - a full sandbox escape. CommandPattern now takes ResolvedBin, an absolute path obtained via exec.LookPath plus symlink evaluation, and requires argv[0] to equal it exactly. Reject additionally refuses any program under a caller-supplied prefix, as defense in depth for the write-through paths. Resolution failure denies every launch rather than falling back to the old behavior: a pattern that cannot pin its binary must not silently widen. TestRevdiff_LaunchPatternsRejectPlantedBinary covers the escape in all five argv shapes the launcher can emit.
Stop cancelled the context and closed the listener but never touched connections already accepted, and runHandler closes a connection only after its handler returns. A handler blocked in conn.Read therefore never woke up: context cancellation does not interrupt a blocking read. Stop then waited out the full 5s drain timeout and returned an error on every shutdown that had a live client. One-shot handlers (kitty) return on their own so this was latent, but any streaming protocol hits it immediately. The server now tracks accepted connections and closes them in Stop, and refuses connections accepted after shutdown has begun rather than leaking one. TestStopClosesIdleAcceptedConnections fails at 5.0s without the fix and passes in 0.008s with it.
Some launchers hand a terminal a generated script path (`sh /tmp/launch-abc`)
rather than an inline command, so what needs vetting is a multi-line script
body. CommandPattern cannot do it: it models a single argv, and shellMeta
deliberately rejects the `;`, `$`, `>` and `&` that any real script contains.
ScriptPattern recognizes one exact shape and nothing else - an allowlisted
shebang followed by a single statement of the form
[/usr/bin/env 'K=V'...] [K=V...] '<prog>' '<arg>'...; rc=$?; \
printf "%s" "$rc" > '<sentinel>'.tmp && mv -f '<sentinel>'.tmp '<sentinel>'
which is the completion-sentinel form the revdiff launcher emits. A second
statement, a heredoc, a pipeline, an unquoted redirect or substitution are
all rejected. It does not try to model the shell; it recognizes one sentence.
Deliberately does NOT loosen shellMeta, which would weaken every inline
pattern. Note that `$(...)` and backticks are accepted *inside* single
quotes: POSIX sh does not expand them there, so they reach the program as
literal text and never touch a shell. Unquoted forms are rejected.
herdr exposes 84 control methods over a unix socket, including pane.read
(read any pane's contents), pane.send_text and agent.send (type into any
pane), worktree.* , plugin.* and server.stop. Handing that socket to
sandboxed code grants full control of the user's workspace, so it is never
mounted; this proxy stands in front of it instead.
Everything not named by an enabled capability is denied. launch_overlay
permits tab.create plus pane.send_input and tab.close scoped to the tab and
pane that call returned - ownership is taken from the server's response,
never from anything the client claims. pane.send_input is generic keystroke
injection, so it is further restricted to exactly one declared command plus
Enter. ping is answered unconditionally: it observes and mutates nothing,
revealing less than the connect(2) a client already performed.
Two things differ from the kitty proxy and drive the design:
- herdr multiplexes. One connection carries many requests, replies can
arrive out of order, and subscriptions stream. The proxy runs two pumps
over a single upstream connection rather than a request/response
exchange, synthesizing denials correlated by request id. Ownership is
recorded *before* a tab.create reply is relayed, since the client issues
pane.send_input the moment it sees that reply.
- herdr runs launch payloads as `sh <path>`, so the payload is a file the
sandbox can still write while it executes on the host. Validating in
place would leave a swap window, so the proxy reads the script once,
validates those bytes, and copies them to a host-only directory before
handing over the path. Construction fails loudly if that directory is
reachable from inside the sandbox.
Protocol shapes verified against herdr v0.7.4 (protocol 16).
Adds the herdr tool: it starts the proxy when running inside a herdr session
(HERDR_ENV=1) and some enabled tool declares a capability, and points the
sandboxed CLI at it. revdiff is the first consumer, declaring launch_overlay
plus the script shape its launcher generates.
Notable details, each of which cost a real failure to find:
- Environment exports the socket path as the *sandbox* sees it. Start
creates the socket under the host-side sandbox home, but that directory
is mounted at the user's home inside the sandbox, so exporting the host
path names a file that does not exist there. Matches how kitty builds
KITTY_LISTEN_ON.
- The binary is bind-mounted only when it lives outside the user's home.
herdr is typically mise-managed, so its binary sits under
~/.local/share/mise, which is already mounted; binding it again
registered a child mount before its parent and the builder panicked.
- The proxy socket is also exposed at ~/.config/herdr/herdr.sock. Some
subcommands ignore HERDR_SOCKET_PATH and connect straight there -
`herdr session list` did, and reported the session "stopped" inside the
sandbox. That probe is connect(2) only with no protocol traffic, so this
corrects the status without any request crossing the filter. It is the
same filtered socket under a second name, not additional reach.
A tool that declares a launch capability without a script pattern has it
dropped rather than warned about, as kitty does: an unconstrained herdr
launch is arbitrary host execution, so degraded-but-open is not a safe
default. Relocated scripts are owned and cleaned up here, not by the proxy.
Adds a "herdr Terminal Workspace" section to docs/tools.md covering activation, the mode setting, the capability table, what is mounted, the exported environment, and the limitations - including why launch scripts are relocated (which changes the path a user sees in the pane) and why ping is permitted unconditionally. Also records the kitty program-pinning fix in that tool's section, since the guarantee it documents changed, and notes in CLAUDE.md the two invariants future terminal proxies must keep: pin the resolved binary path rather than matching a basename, and never validate a file the sandbox can still write. Both were violated once already.
Two unrelated lint-config problems, neither introduced by recent work.
`issues.exclude-dirs` is rejected by the v2 schema, so `golangci-lint config
verify` failed and the vendor/tmp exclusions were silently ignored rather
than applied. Moved to linters.exclusions.paths.
SA5011 reports a possible nil dereference for the standard table-test guard
`if x == nil { t.Fatal(...) }` followed by a use of x. t.Fatal calls
runtime.Goexit, so the dereference is unreachable; staticcheck does not model
that here. All 14 occurrences in this repo are guarded by t.Fatal or
t.Fatalf, none by t.Error, so all 14 are false positives. Excluded for
_test.go and SA5011 only - staticcheck stays fully enabled for production
code, verified with a probe file. The config comment records the residual
risk: a t.Error-guarded dereference in a test would now be hidden.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.