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