fix(listen): fix WebSocket HTTP/2 and path glob encoding for relayfile listen#334
Conversation
api.relayfile.dev negotiates HTTP/2 via ALPN; the Upgrade header used by nhooyr.io/websocket is rejected over h2. Cloning the default transport and clearing TLSNextProto forces HTTP/1.1 for the WebSocket dial only, leaving all other CLI calls unaffected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ob helper url.Values.Encode() percent-encodes /, *, and ? in path globs, causing the server-side glob matcher to receive a literal "%2Flinear%2F%2A%2A" and match nothing. Replace with manual raw query building via wsEncodeGlob(), which preserves /, *, ? as literal characters while still encoding everything else. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 12 minutes and 35 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a websocket-based ChangesCLI Feature and Auth Refactor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the listen and dev commands to stream live file events from a workspace over WebSockets, along with a supervisor command to manage the listener as a system service (systemd/launchd). It also removes several legacy commands (such as logout), simplifies delegated credential resolution, and removes exponential backoff for degraded mount recovery. The review feedback highlights a critical command injection vulnerability in the command execution template expansion, where untrusted event fields are not shell-escaped. Additionally, a potential panic was identified due to an unsafe type assertion on http.DefaultTransport when setting up the WebSocket client.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func listenExpandTemplate(tmpl string, evt listenEvent, raw json.RawMessage) string { | ||
| return strings.NewReplacer( | ||
| "{{path}}", evt.Path, | ||
| "{{type}}", evt.Type, | ||
| "{{provider}}", evt.Provider, | ||
| "{{revision}}", evt.Revision, | ||
| "{{event}}", string(raw), | ||
| ).Replace(tmpl) | ||
| } |
There was a problem hiding this comment.
Executing user-defined commands constructed via simple string replacement of untrusted event fields (like evt.Path or raw JSON) leads to a command injection vulnerability. A malicious file path or event payload containing shell metacharacters (e.g., backticks, semicolons, or subshells) could execute arbitrary commands on the host system.
To prevent this, shell-escape all replaced template variables before executing them via sh -c.
func shellEscape(s string) string {
if s == "" {
return "''"
}
return "'" + strings.ReplaceAll(s, "'", "'\\''") + "'"
}
func listenExpandTemplate(tmpl string, evt listenEvent, raw json.RawMessage) string {
return strings.NewReplacer(
"{{path}}", shellEscape(evt.Path),
"{{type}}", shellEscape(evt.Type),
"{{provider}}", shellEscape(evt.Provider),
"{{revision}}", shellEscape(evt.Revision),
"{{event}}", shellEscape(string(raw)),
).Replace(tmpl)
}| wsTransport := http.DefaultTransport.(*http.Transport).Clone() | ||
| wsTransport.TLSNextProto = make(map[string]func(string, *tls.Conn) http.RoundTripper) | ||
| wsHTTPClient := &http.Client{Transport: wsTransport} |
There was a problem hiding this comment.
The type assertion http.DefaultTransport.(*http.Transport) can panic if http.DefaultTransport has been wrapped or replaced by another http.RoundTripper implementation (e.g., by APM, tracing, or logging libraries). Use a comma-ok type assertion to safely handle cases where http.DefaultTransport is not a concrete *http.Transport.
var wsTransport *http.Transport
if t, ok := http.DefaultTransport.(*http.Transport); ok {
wsTransport = t.Clone()
} else {
wsTransport = &http.Transport{}
}
wsTransport.TLSNextProto = make(map[string]func(string, *tls.Conn) http.RoundTripper)
wsHTTPClient := &http.Client{Transport: wsTransport}
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
…kage The server-side glob filter is not applied to the historical-event replay (last 100 events sent on connection); without client-side enforcement, events for other providers leak through until the from=now server fix is deployed. Add matchListenPath (handles /** double-star suffix) and apply it in the receive loop after the existing type filter. Add unit tests for matchListenPath and wsEncodeGlob. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de3bbf59e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // server-side reconcile kicks. Both must be present on every credential | ||
| // so narrow prior credentials cannot silently break a subset of providers. | ||
| var defaultJoinScopes = []string{"fs:read", "fs:write", "ops:read", "sync:trigger"} | ||
| var defaultJoinScopes = []string{"fs:read", "fs:write"} |
There was a problem hiding this comment.
With only fs:read/fs:write minted here, commands that reuse defaultJoinScopes no longer have the scopes required by their own runtime calls: for example runPull posts to /v1/workspaces/{id}/sync/refresh, and internal/httpapi/server.go requires sync:trigger for that route; mount/writeback status polling similarly needs ops:read for /ops/{opId}. Newly bootstrapped credentials will therefore hit 403s in these flows even though the credential was minted by the CLI.
Useful? React with 👍 / 👎.
| if !ok { | ||
| return false | ||
| childArgs := append([]string{"listen"}, filtered...) | ||
| childArgs = append(childArgs, "--daemonized", "--pid-file", pidFile) |
There was a problem hiding this comment.
When users run relayfile listen --background, the parent starts a child with --pid-file, but runListen does not define or accept that flag, so the daemonized child exits during flag parsing while the parent still prints that listening started. This makes the advertised background mode fail immediately.
Useful? React with 👍 / 👎.
| "background": false, "daemonized": false, | ||
| })) | ||
|
|
||
| commandClient, err := prepareWorkspaceCommandClient("", *serverPeek, *tokenPeek, defaultInspectScopes) |
There was a problem hiding this comment.
For relayfile dev OTHER_WORKSPACE ..., this preflight always prepares credentials for the active workspace ("") instead of the positional workspace that runListen(args) would later parse. If the active workspace is unset or has different delegated credentials, dev aborts with onboarding/credential errors even though the explicit workspace argument is valid.
Useful? React with 👍 / 👎.
Review: PR #334 — relayfile
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/relayfile-cli/main.go`:
- Around line 5588-5603: The help text for the listen command in the
printListenUsage function contains an incorrect reference to the --listen flag,
which is not actually parsed by the supervisor or listen commands and will cause
failures if users follow this help. Remove the --listen flag from the help
documentation. Additionally, the --background flag is supported by the listen
command but is not currently documented in the printListenUsage function. Add
the --background flag to the Flags section of the help text to accurately
reflect all supported options.
- Around line 3043-3044: The provider segment (parts[0]) from the path is being
used directly in glob-style scope strings without validation, which could allow
unsafe patterns like *, ?, or .. to create overly broad or invalid scopes.
Before calling writebackPushJoinScopes and writebackPushRequiredRelayfileScopes
(and before ensureWritebackDelegatedCredentials), add validation to check that
the provider segment is safe and does not contain glob patterns or path
traversal sequences. Return an error immediately if the provider segment is
invalid, rejecting it before any delegated write scopes are derived. Apply this
same validation logic to the similar code block at lines 3488-3508.
- Around line 5820-5822: The websocket URL containing the sensitive token
parameter (appended to base.RawQuery) could be exposed in dial error messages
that get logged or persisted. When handling the dial error around line 5840
(where the error from the websocket connection is wrapped or returned), redact
the token parameter from the URL string before including it in the error
message, or return a sanitized error that does not include the full request URL.
This prevents the bearer token from being accidentally printed or persisted in
CLI or background logs.
- Around line 5884-5890: The listenExpandTemplate function is directly
interpolating event-controlled values like {{path}} and {{event}} into the shell
command string passed to sh -c, creating a shell injection vulnerability where
crafted paths or event data could escape the template and execute arbitrary
commands. Instead of embedding these values directly in the command string, pass
the event fields via environment variables that the sh command can reference
(using cmd.Env with the exec.CommandContext call), and update
listenExpandTemplate and any related help documentation to use environment
variable references instead of direct {{path}} and {{event}} substitutions. This
applies to both the main runCmd execution block and similar patterns elsewhere
in the file.
- Around line 5758-5770: The runListen function passes a --pid-file flag to the
daemonized child process at line 5946, but this flag is never defined in the
flagset fs, causing the child to fail during flag parsing while the parent
reports success. Add a flag definition for --pid-file to the runListen function
using fs.String (similar to how "background" and "daemonized" are defined), and
also add "pid-file" to the normalizeFlagArgs map passed to fs.Parse to ensure
the flag is properly recognized during argument parsing.
- Around line 5773-5776: The runListen function currently silently ignores extra
positional arguments beyond the first one. After extracting the workspaceValue
using fs.Arg(0), add validation to check if fs.NArg() is greater than 1, and if
so, return an error message to the user rejecting the extra positional
arguments. This ensures users are immediately alerted if they accidentally
provide multiple workspace arguments instead of being silently ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e094911-28f1-47e0-b30c-d545080f9ac6
📒 Files selected for processing (1)
cmd/relayfile-cli/main.go
| background := fs.Bool("background", false, "run in background; logs to ~/.relayfile/listen.log") | ||
| daemonized := fs.Bool("daemonized", false, "internal flag used by relayfile listen --background") | ||
| if err := fs.Parse(normalizeFlagArgs(args, map[string]bool{ | ||
| "server": true, | ||
| "token": true, | ||
| "path": true, | ||
| "depth": true, | ||
| "json": false, | ||
| "server": true, | ||
| "token": true, | ||
| "provider": true, | ||
| "path": true, | ||
| "event": true, | ||
| "run": true, | ||
| "format": true, | ||
| "background": false, | ||
| "daemonized": false, | ||
| })); err != nil { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Do not pass an undefined --pid-file flag to the daemonized child.
Line 5946 appends --pid-file, but runListen does not define that flag. The child exits during flag parsing, so relayfile listen --background can report success while the daemon immediately fails in the log.
Proposed hidden flag fix
background := fs.Bool("background", false, "run in background; logs to ~/.relayfile/listen.log")
daemonized := fs.Bool("daemonized", false, "internal flag used by relayfile listen --background")
+ pidFile := fs.String("pid-file", "", "internal flag used by relayfile listen --background")
if err := fs.Parse(normalizeFlagArgs(args, map[string]bool{
@@
"background": false,
"daemonized": false,
+ "pid-file": true,
})); err != nil {
@@
if *daemonized {
+ if p := strings.TrimSpace(*pidFile); p != "" {
+ if err := os.WriteFile(p, []byte(strconv.Itoa(os.Getpid())+"\n"), 0o600); err != nil {
+ return fmt.Errorf("write listen pid file: %w", err)
+ }
+ defer os.Remove(p)
+ }
if err := rotateLogFile(listenLogFile()); err != nil {
return err
}
}Also applies to: 5783-5787, 5945-5946
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/relayfile-cli/main.go` around lines 5758 - 5770, The runListen function
passes a --pid-file flag to the daemonized child process at line 5946, but this
flag is never defined in the flagset fs, causing the child to fail during flag
parsing while the parent reports success. Add a flag definition for --pid-file
to the runListen function using fs.String (similar to how "background" and
"daemonized" are defined), and also add "pid-file" to the normalizeFlagArgs map
passed to fs.Parse to ensure the flag is properly recognized during argument
parsing.
| var workspaceValue string | ||
| if fs.NArg() > 0 { | ||
| workspaceValue = strings.TrimSpace(fs.Arg(0)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject extra listen positional arguments.
runListen only consumes fs.Arg(0) and silently ignores the rest, so typos like relayfile listen workspace-a workspace-b run against workspace-a without warning.
Proposed validation
var workspaceValue string
+ if fs.NArg() > 1 {
+ return errors.New("usage: relayfile listen [WORKSPACE] [--provider PROVIDER] [--path GLOB] [--event TYPE] [--run CMD] [--format text|json] [--background]")
+ }
if fs.NArg() > 0 {
workspaceValue = strings.TrimSpace(fs.Arg(0))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var workspaceValue string | |
| if fs.NArg() > 0 { | |
| workspaceValue = strings.TrimSpace(fs.Arg(0)) | |
| } | |
| var workspaceValue string | |
| if fs.NArg() > 1 { | |
| return errors.New("usage: relayfile listen [WORKSPACE] [--provider PROVIDER] [--path GLOB] [--event TYPE] [--run CMD] [--format text|json] [--background]") | |
| } | |
| if fs.NArg() > 0 { | |
| workspaceValue = strings.TrimSpace(fs.Arg(0)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/relayfile-cli/main.go` around lines 5773 - 5776, The runListen function
currently silently ignores extra positional arguments beyond the first one.
After extracting the workspaceValue using fs.Arg(0), add validation to check if
fs.NArg() is greater than 1, and if so, return an error message to the user
rejecting the extra positional arguments. This ensures users are immediately
alerted if they accidentally provide multiple workspace arguments instead of
being silently ignored.
| // Token in query param for WS upgrade (server does not yet support Authorization on upgrade). | ||
| rawParts = append(rawParts, "token="+url.QueryEscape(commandClient.client.token)) | ||
| base.RawQuery = strings.Join(rawParts, "&") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact the websocket query token from dial errors.
base.RawQuery contains token=..., and dial failures can include the request URL in the returned error. Wrapping that error at Line 5840 can print or persist the bearer token in CLI/background logs.
Redact token from the error string before returning it, or return a sanitized connection error that does not include the URL-bearing wrapped error.
Also applies to: 5833-5840
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/relayfile-cli/main.go` around lines 5820 - 5822, The websocket URL
containing the sensitive token parameter (appended to base.RawQuery) could be
exposed in dial error messages that get logged or persisted. When handling the
dial error around line 5840 (where the error from the websocket connection is
wrapped or returned), redact the token parameter from the URL string before
including it in the error message, or return a sanitized error that does not
include the full request URL. This prevents the bearer token from being
accidentally printed or persisted in CLI or background logs.
| if runCmd != "" { | ||
| expanded := listenExpandTemplate(runCmd, evt, raw) | ||
| cmd := exec.CommandContext(rootCtx, "sh", "-c", expanded) | ||
| cmd.Stdout = stdout | ||
| cmd.Stderr = os.Stderr | ||
| if err := cmd.Run(); err != nil && !errors.Is(err, context.Canceled) { | ||
| fmt.Fprintf(os.Stderr, "run error for %s: %v\n", evt.Path, err) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Keep provider event data out of sh -c templates.
{{path}} and {{event}} are event-controlled values inserted before sh -c; a crafted path can escape the intended command template. Pass event fields via environment/stdin, or centrally shell-escape placeholders before invoking the shell.
Safer execution shape
- expanded := listenExpandTemplate(runCmd, evt, raw)
- cmd := exec.CommandContext(rootCtx, "sh", "-c", expanded)
+ cmd := exec.CommandContext(rootCtx, "sh", "-c", runCmd)
+ cmd.Env = append(os.Environ(),
+ "RELAYFILE_EVENT_PATH="+evt.Path,
+ "RELAYFILE_EVENT_TYPE="+evt.Type,
+ "RELAYFILE_EVENT_PROVIDER="+evt.Provider,
+ "RELAYFILE_EVENT_REVISION="+evt.Revision,
+ "RELAYFILE_EVENT_JSON="+string(raw),
+ )
cmd.Stdout = stdout
cmd.Stderr = os.StderrUpdate the help examples to reference these environment variables instead of raw {{...}} substitutions.
Also applies to: 5968-5975
🧰 Tools
🪛 OpenGrep (1.23.0)
[ERROR] 5886-5886: Dynamic command passed to exec.Command with a shell invocation. Pass arguments directly to exec.Command without a shell wrapper.
(coderabbit.command-injection.go-exec-command)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/relayfile-cli/main.go` around lines 5884 - 5890, The listenExpandTemplate
function is directly interpolating event-controlled values like {{path}} and
{{event}} into the shell command string passed to sh -c, creating a shell
injection vulnerability where crafted paths or event data could escape the
template and execute arbitrary commands. Instead of embedding these values
directly in the command string, pass the event fields via environment variables
that the sh command can reference (using cmd.Env with the exec.CommandContext
call), and update listenExpandTemplate and any related help documentation to use
environment variable references instead of direct {{path}} and {{event}}
substitutions. This applies to both the main runCmd execution block and similar
patterns elsewhere in the file.
Source: Linters/SAST tools
…rom listen branch The initial listen-command commit used a version of main.go that predated several additions merged to main: - ErrDelegatedScopeInsufficient / ErrDelegatedScopeInvalid + mapDelegatedTokenCloudError (storm-retry fix for bad delegated token scopes, tested by delegated_token_error_test.go) - writebackPushScopes / writebackPushProvider (provider-scoped scope derivation, tested by main_test.go) - workspaceShardKey / rawWorkspaceShardKey / canonicalWorkspaceShardValueStatus / workspaceRecordForShardValue (delegated credential cache keying, tested by main_test.go) Restores all removed symbols so the test package compiles and passes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Review: PR #334 —
|
…cleanly The initial listen-command commit replaced main.go with an older branch version, silently dropping logout, on/off aliases, --wait-sync, integration list runtime status, degraded-backoff, and other features added since the branch diverged. Reset main.go to origin/main (10275-line baseline with all current features), then grafted the listen-specific additions on top: - crypto/tls + nhooyr.io/websocket imports - listenEvent struct - matchListenPath — double-star glob matching for client-side path filtering - wsEncodeGlob — preserves / * ? in WS query params (BUG 7 fix) - runListen — HTTP/1.1 forced via TLSNextProto (BUG 1 fix), from=now, client filter (BUG 9 fix) - runDev — thin wrapper around runListen with setup hint - spawnBackgroundListenProcess, listenExpandTemplate, listenPIDFile, listenLogFile - listen/watch and dev dispatch in run(), help cases for both All existing tests pass; TestWsEncodeGlob and TestMatchListenPath also pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
Two bug fixes for the
relayfile listencommand (PR #332) discovered during E2E testing against Linear:BUG 1: WebSocket connection fails when the server negotiates HTTP/2 via ALPN TLS. The
Upgrade: websocketheader is incompatible with HTTP/2 framing. Fix: clone the default transport and setTLSNextProto = make(map[...])to disable h2 negotiation, forcing HTTP/1.1 for the WS dial.BUG 7: Path filter glob characters (
/,*,?) are percent-encoded byurl.Values.Encode(), causing/linear/**to arrive at the server as%2Flinear%2F%2A%2Awhich matches nothing. Fix: addwsEncodeGlob()helper that preserves these characters as literals while encoding everything else, then build the raw query string manually.Test plan
go build ./cmd/relayfile-cli/relayfile listen --provider linearconnects successfully and filters to/linear/**events onlywriteback.succeeded+file.updatedevents stream correctly with path filter active--provider linearfilter (before fix they appeared due to encoding issue)Follow-up (separate PRs/issues)
from=nowserver-side replay): fixed in paired cloud branchpear/relayfiletests-c0f635becreate-webhook.tsaction to be run🤖 Generated with Claude Code