From d32bbafb35216792e9e18ee6a9fedf179a486a8a Mon Sep 17 00:00:00 2001 From: ysyneu Date: Thu, 8 Oct 2026 20:26:18 -0700 Subject: [PATCH] fix(cli): fail fast when the broker credential fd is not inherited In broker mode fduty reaches Flashduty through a control socket the runner passes as an inherited fd (FLASHDUTY_CRED_FD). Programs that spawn fduty without passing inherited fds - Python's subprocess (close_fds=True by default), Node's child_process, sudo - leave that fd closed or reused by an unrelated file. The failure then only surfaced at the first request as "broker handshake send: bad file descriptor" (or "socket operation on non-socket"), after the SDK's request URL prefix, with no hint of the fix. newBrokerHTTPClient now checks that the fd is an open socket before building the client and returns an error that names the fd and how to keep it open (run from the shell, Python pass_fds, Node stdio entry). It returns (*http.Client, error), so the non-unix stub returns errBrokerUnsupported directly and root.go drops its nil check. --- internal/cli/broker_dial_other.go | 2 +- internal/cli/broker_dial_unix.go | 23 ++++++++++---- internal/cli/broker_dial_unix_test.go | 46 ++++++++++++++++++++++++--- internal/cli/root.go | 6 ++-- 4 files changed, 62 insertions(+), 15 deletions(-) diff --git a/internal/cli/broker_dial_other.go b/internal/cli/broker_dial_other.go index 0b7cb51..74fc4a1 100644 --- a/internal/cli/broker_dial_other.go +++ b/internal/cli/broker_dial_other.go @@ -7,7 +7,7 @@ import ( "net/http" ) -func newBrokerHTTPClient(int) *http.Client { return nil } +func newBrokerHTTPClient(int) (*http.Client, error) { return nil, errBrokerUnsupported } var errBrokerUnsupported = errors.New("flashduty: broker mode is not supported on this platform") diff --git a/internal/cli/broker_dial_unix.go b/internal/cli/broker_dial_unix.go index 1669151..c70d09d 100644 --- a/internal/cli/broker_dial_unix.go +++ b/internal/cli/broker_dial_unix.go @@ -14,11 +14,6 @@ import ( "time" ) -// errBrokerUnsupported is returned when broker mode is requested on a build that -// cannot provide it. On unix this is effectively unreachable (newBrokerHTTPClient -// never returns nil), but defaultNewClient references it on every platform. -var errBrokerUnsupported = errors.New("flashduty: broker mode is not supported on this platform") - // errBrokerClosed is returned (wrapped) when the runner-side broker control // channel is gone: the runner exited, or reclaimed the channel once the // command that started this process finished, so fduty calls from a @@ -110,7 +105,21 @@ func (d *brokerDialer) dial(_ context.Context, _, _ string) (net.Conn, error) { // every connection over the inherited control fd. Timeout matches the SDK's // historical default (30s) so behavior is unchanged for non-streaming calls; // streaming export relies on request context like before. -func newBrokerHTTPClient(credFD int) *http.Client { +// +// It first checks that credFD is an open socket in this process. The runner +// hands the control end to bash, and only processes that inherit fd credFD +// reach fduty with it intact: Python's subprocess (close_fds=True by default), +// Node's child_process and sudo all close it. Without the check that surfaces +// as a handshake EBADF/ENOTSOCK at the first request, after the SDK's URL +// prefix, with nothing saying how to fix it. +func newBrokerHTTPClient(credFD int) (*http.Client, error) { + if _, err := syscall.GetsockoptInt(credFD, syscall.SOL_SOCKET, syscall.SO_TYPE); err != nil { + return nil, fmt.Errorf("FLASHDUTY_CRED_FD=%d is not an open socket in this process (%v): "+ + "the program that started fduty did not pass the credential channel down. "+ + "Run fduty from the shell, or keep fd %d open when spawning it: "+ + "Python subprocess.run(cmd, pass_fds=(%d,)); Node: set entry %d of spawn's stdio array to %d", + credFD, err, credFD, credFD, credFD, credFD) + } d := &brokerDialer{credFD: credFD} return &http.Client{ Timeout: 30 * time.Second, @@ -125,5 +134,5 @@ func newBrokerHTTPClient(credFD int) *http.Client { IdleConnTimeout: 90 * time.Second, ResponseHeaderTimeout: 0, }, - } + }, nil } diff --git a/internal/cli/broker_dial_unix_test.go b/internal/cli/broker_dial_unix_test.go index 1ac1ba3..7c8589b 100644 --- a/internal/cli/broker_dial_unix_test.go +++ b/internal/cli/broker_dial_unix_test.go @@ -106,9 +106,9 @@ func TestBrokerHTTPClient_DialAndRewrite(t *testing.T) { defer func() { _ = syscall.Close(childFD) }() defer stop() - client := newBrokerHTTPClient(childFD) - if client == nil { - t.Fatal("newBrokerHTTPClient returned nil") + client, err := newBrokerHTTPClient(childFD) + if err != nil { + t.Fatalf("newBrokerHTTPClient: %v", err) } defer client.CloseIdleConnections() // release dispatched keep-alive conns // The CLI's base URL is an http placeholder; broker rewrites host. @@ -208,6 +208,41 @@ func TestDefaultNewClient_RejectsStdioFD(t *testing.T) { } } +// TestDefaultNewClient_CredFDNotInherited covers a caller that drops the +// inherited control fd before exec (Python's subprocess closes fds >= 3 by +// default): the fd number is then closed, or reused by an unrelated file. +// Either way defaultNewClient must fail up front with an error that names the +// fd and the fix, instead of a handshake errno at the first request. +func TestDefaultNewClient_CredFDNotInherited(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("FLASHDUTY_APP_KEY", "") + + f, err := os.CreateTemp(t.TempDir(), "not-a-socket") + if err != nil { + t.Fatal(err) + } + defer func() { _ = f.Close() }() + pair, err := syscall.Socketpair(syscall.AF_UNIX, controlSockType, 0) + if err != nil { + t.Fatalf("socketpair: %v", err) + } + closedFD := pair[0] + _ = syscall.Close(pair[0]) + _ = syscall.Close(pair[1]) + + for name, fd := range map[string]int{"closed fd": closedFD, "regular file": int(f.Fd())} { + t.Setenv("FLASHDUTY_CRED_FD", strconv.Itoa(fd)) + _, err := defaultNewClient() + if err == nil { + t.Fatalf("%s: defaultNewClient must fail", name) + } + want := "FLASHDUTY_CRED_FD=" + strconv.Itoa(fd) + " is not an open socket" + if msg := err.Error(); !strings.HasPrefix(msg, want) || !strings.Contains(msg, "pass_fds=("+strconv.Itoa(fd)+",)") { + t.Fatalf("%s: error must start with %q and name pass_fds, got: %v", name, want, msg) + } + } +} + // TestBrokerHTTPClient_RefusedReturnsError verifies the dialer surfaces the // broker's 0xFF refusal (e.g. the runner failed to mint a connection) as a real // error instead of hanging or wrapping a nil conn. @@ -232,7 +267,10 @@ func TestBrokerHTTPClient_RefusedReturnsError(t *testing.T) { } }() - client := newBrokerHTTPClient(childFD) + client, err := newBrokerHTTPClient(childFD) + if err != nil { + t.Fatalf("newBrokerHTTPClient: %v", err) + } req, _ := http.NewRequestWithContext(context.Background(), "GET", "http://flashduty.broker.local/x?app_key=SENTINEL", nil) if _, err := client.Do(req); err == nil { diff --git a/internal/cli/root.go b/internal/cli/root.go index d89a676..cc9f097 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -234,9 +234,9 @@ func defaultNewClient() (*flashduty.Client, error) { if perr != nil || fd < 3 { return nil, fmt.Errorf("invalid FLASHDUTY_CRED_FD=%q", fdStr) } - hc := newBrokerHTTPClient(fd) - if hc == nil { - return nil, errBrokerUnsupported + hc, err := newBrokerHTTPClient(fd) + if err != nil { + return nil, err } opts = append(opts, flashduty.WithHTTPClient(hc)) appKey = "broker-sentinel" // non-empty: go-flashduty rejects ""; broker overwrites it