fix(nip46): refuse to dial relays through inherited *_PROXY env vars

nip46/nip46.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/url"
+	"os"
 	"strings"
 	"sync"
 	"time"
@@ -83,13 +84,101 @@ func dedupStrings(ss []string) []string {
 	return out
 }
 
+// proxyEnvVars is the set of Go stdlib proxy env vars that
+// net/http honors when resolving a Transport's proxy function.
+// If any of these are set we have to route the wss:// dial through
+// the proxy, and if the proxy isn't running (e.g. a Tor listener
+// that isn't started) the dial fails with "connect: connection
+// refused" pointing at the proxy port — which has been the
+// dominant "Amber sign-in failed" complaint in practice. We
+// check this here so we can surface a useful error message
+// instead of letting the dial error bubble up unannotated.
+//
+// Order matches Go's net/http/httputil.preferedProxyScheme lookup
+// (HTTPS_PROXY, then ALL_PROXY, then HTTP_PROXY, then scheme-
+// specific socks fallbacks) — the first env var with a parseable
+// URL wins. See golang.org/x/net/http/httpproxy for the canonical
+// resolution.
+var proxyEnvVars = []string{
+	"HTTPS_PROXY", "https_proxy",
+	"HTTP_PROXY", "http_proxy",
+	"ALL_PROXY", "all_proxy",
+	"SOCKS_PROXY", "socks_proxy",
+	"SOCKS5_PROXY", "socks5_proxy",
+}
+
+// detectProxy returns the first proxy env var that's set and the
+// resolved scheme://host:port it points at, or "" / nil if no
+// proxy env var is set. Empty values and malformed URLs are
+// skipped — they aren't usable by Go's net/http transport either,
+// so refusing to act on them matches stdlib behaviour.
+func detectProxy() (string, *url.URL) {
+	for _, name := range proxyEnvVars {
+		raw := os.Getenv(name)
+		if raw == "" {
+			continue
+		}
+		u, err := url.Parse(raw)
+		if err != nil || u.Scheme == "" || u.Host == "" {
+			continue
+		}
+		return name, u
+	}
+	return "", nil
+}
+
+// validateRelayURLs rejects blank URLs and anything that isn't
+// ws:// or wss://. Returns the first invalid URL it finds so the
+// caller can include it in the error message.
+func validateRelayURLs(urls []string) error {
+	if len(urls) == 0 {
+		return fmt.Errorf("no relay URLs configured for NIP-46 session")
+	}
+	for _, raw := range urls {
+		if raw == "" {
+			return fmt.Errorf("empty relay URL in session")
+		}
+		u, err := url.Parse(raw)
+		if err != nil || (u.Scheme != "ws" && u.Scheme != "wss") {
+			return fmt.Errorf("invalid relay URL %q: must be ws:// or wss://", raw)
+		}
+	}
+	return nil
+}
+
 // ConnectRelays dials every configured relay in parallel and
 // subscribes for NIP-46 kind 24133 events addressed to our client
 // pubkey. Call this from OnInitQR (while generating the QR) so
 // that by the time the user scans, the subscriptions are already
 // active. Errors on individual relays are skipped; only a total
 // failure (zero relays connected) returns an error.
 func (s *Session) ConnectRelays(ctx context.Context) error {
+	// Validate the configured relay URLs up front. A blank or
+	// malformed URL would otherwise surface as a confusing
+	// websocket error mid-dial.
+	if err := validateRelayURLs(s.RelayURLs); err != nil {
+		return err
+	}
+
+	// Surface inherited proxy env vars before we dial. The most
+	// common sign-in failure has been "connect: connection
+	// refused" against a proxy port (e.g. 9050) that the user
+	// isn't running because they don't actually want a Tor exit —
+	// they inherited HTTPS_PROXY from a launcher or shell.
+	// Refuse the dial up front so the user gets a clear, actionable
+	// message instead of a generic websocket error. Matches what
+	// every other Nostr TUI does — nak, lume, etc. — none of them
+	// proxy wss:// connections by default.
+	if name, proxy := detectProxy(); proxy != nil {
+		return fmt.Errorf(
+			"refusing to dial relays through %s=%s://%s: "+
+				"NIP-46 sign-in requires a direct connection to the relay. "+
+				"Unset %s (and any of HTTPS_PROXY/HTTP_PROXY/ALL_PROXY/SOCKS_PROXY) "+
+				"in the shell that launches hoot, or run hoot outside torsocks",
+			name, proxy.Scheme, proxy.Host, name,
+		)
+	}
+
 	const dialTimeout = 10 * time.Second
 
 	// Dial

nip46/nip46_test.go

@@ -150,3 +150,234 @@ func TestDialRelaysContinuesOnIndividualFailure(t *testing.T) {
 	// opened without a connection so WaitForConnection would error
 	// next — but we stop here.)
 }
+
+// ---------------------------------------------------------------------------
+// Proxy environment detection (regression guard for the "connect to port 9050"
+// failure mode that surfaces as "Amber sign-in failed").
+// ---------------------------------------------------------------------------
+
+// all the *_PROXY env vars we touch. Listed so tests can unset them
+// cleanly without affecting unrelated env state.
+var proxyEnvNames = []string{
+	"HTTPS_PROXY", "https_proxy",
+	"HTTP_PROXY", "http_proxy",
+	"ALL_PROXY", "all_proxy",
+	"SOCKS_PROXY", "socks_proxy",
+	"SOCKS5_PROXY", "socks5_proxy",
+}
+
+func clearProxyEnv(t *testing.T) {
+	t.Helper()
+	for _, name := range proxyEnvNames {
+		t.Setenv(name, "")
+	}
+}
+
+// TestDetectProxyNoEnv: no *_PROXY set -> detectProxy returns ("", nil).
+// Pins the baseline behaviour for every other test.
+func TestDetectProxyNoEnv(t *testing.T) {
+	clearProxyEnv(t)
+	name, u := detectProxy()
+	if name != "" {
+		t.Errorf("expected empty name, got %q", name)
+	}
+	if u != nil {
+		t.Errorf("expected nil URL, got %v", u)
+	}
+}
+
+// TestDetectProxyTor9050: the exact failure mode reported in the
+// bug report — HTTPS_PROXY points at a Tor SOCKS port that isn't
+// running, sign-in errors with "connect: connection refused" against
+// 127.0.0.1:9050. With this fix, detectProxy surfaces that fact
+// so nip46.ConnectRelays can refuse to dial instead of producing
+// the opaque websocket error.
+func TestDetectProxyTor9050(t *testing.T) {
+	clearProxyEnv(t)
+	t.Setenv("HTTPS_PROXY", "socks5h://127.0.0.1:9050")
+
+	name, u := detectProxy()
+	if name != "HTTPS_PROXY" {
+		t.Errorf("expected name=%q, got %q", "HTTPS_PROXY", name)
+	}
+	if u == nil {
+		t.Fatal("expected non-nil URL")
+	}
+	if u.Host != "127.0.0.1:9050" {
+		t.Errorf("expected host=127.0.0.1:9050, got %q", u.Host)
+	}
+	if u.Scheme != "socks5h" {
+		t.Errorf("expected scheme=socks5h, got %q", u.Scheme)
+	}
+}
+
+// TestDetectProxyHTTPSBeatsHTTP: Go's httpproxy uses HTTPS_PROXY for
+// https:// requests and HTTP_PROXY for http://. When only one is set,
+// detectProxy returns that one. Order in proxyEnvVars matches the
+// stdlib lookup (HTTPS_PROXY before HTTP_PROXY).
+func TestDetectProxyHTTPSBeatsHTTP(t *testing.T) {
+	clearProxyEnv(t)
+	t.Setenv("HTTP_PROXY", "http://h.local:8080")
+	t.Setenv("HTTPS_PROXY", "http://s.local:8443")
+
+	name, u := detectProxy()
+	if name != "HTTPS_PROXY" {
+		t.Errorf("expected HTTPS_PROXY to win, got %q", name)
+	}
+	if u == nil || u.Host != "s.local:8443" {
+		t.Errorf("expected s.local:8443, got %v", u)
+	}
+}
+
+// TestDetectProxyAllProxyFallback: when no scheme-specific proxy is
+// set but ALL_PROXY is, ALL_PROXY wins. (Go's httpproxy checks
+// scheme-specific vars first, then ALL_PROXY as a universal fallback.)
+func TestDetectProxyAllProxyFallback(t *testing.T) {
+	clearProxyEnv(t)
+	t.Setenv("ALL_PROXY", "http://a.local:3128")
+
+	name, u := detectProxy()
+	if name != "ALL_PROXY" {
+		t.Errorf("expected ALL_PROXY, got %q", name)
+	}
+	if u == nil || u.Host != "a.local:3128" {
+		t.Errorf("expected a.local:3128, got %v", u)
+	}
+}
+
+// TestDetectProxySkipsMalformed: garbage in HTTPS_PROXY should not
+// block detection of a valid HTTP_PROXY below it — Go's stdlib also
+// tolerates parse errors silently and falls through.
+func TestDetectProxySkipsMalformed(t *testing.T) {
+	clearProxyEnv(t)
+	t.Setenv("HTTPS_PROXY", "not a url at all")
+	t.Setenv("HTTP_PROXY", "http://valid.local:8080")
+
+	name, u := detectProxy()
+	if name != "HTTP_PROXY" {
+		t.Errorf("expected fallthrough to HTTP_PROXY, got %q", name)
+	}
+	if u == nil || u.Host != "valid.local:8080" {
+		t.Errorf("expected valid.local:8080, got %v", u)
+	}
+}
+
+// TestDetectProxyEmptyValue: env var set but empty is treated as unset.
+// Matches Go's httpproxy behaviour.
+func TestDetectProxyEmptyValue(t *testing.T) {
+	clearProxyEnv(t)
+	t.Setenv("HTTPS_PROXY", "")
+	t.Setenv("HTTP_PROXY", "http://e.local:8080")
+
+	name, u := detectProxy()
+	if name != "HTTP_PROXY" {
+		t.Errorf("expected fallthrough past empty HTTPS_PROXY, got %q", name)
+	}
+	if u == nil || u.Host != "e.local:8080" {
+		t.Errorf("expected e.local:8080, got %v", u)
+	}
+}
+
+// TestDetectProxySchemeRequiredRe: a value with no scheme is rejected
+// even if it parses with a host. url.Parse treats "example.com:8080"
+// as Path=... Host="" Scheme="", so the Scheme check guards this.
+func TestDetectProxySchemeRequiredRe(t *testing.T) {
+	clearProxyEnv(t)
+	t.Setenv("HTTPS_PROXY", "example.com:8080")
+
+	name, u := detectProxy()
+	if name != "" || u != nil {
+		t.Errorf("expected scheme-less URL to be rejected, got %s=%v", name, u)
+	}
+}
+
+// TestConnectRelaysRefusesProxyEnv: the public API. With any
+// *_PROXY pointing at an unreachable host, ConnectRelays returns
+// a non-nil error that mentions the env var name and the dial
+// target — not a generic websocket-dial error. This is the
+// user-visible fix for the "connect to port 9050" bug.
+func TestConnectRelaysRefusesProxyEnv(t *testing.T) {
+	clearProxyEnv(t)
+	t.Setenv("HTTPS_PROXY", "socks5h://127.0.0.1:9050")
+
+	s := &Session{
+		ClientPrivateKey: "0000000000000000000000000000000000000000000000000000000000000001",
+		ClientPublicKey:  "0000000000000000000000000000000000000000000000000000000000000002",
+		RelayURLs:        []string{"wss://relay.damus.io"},
+	}
+
+	err := s.ConnectRelays(nil) // ctx unused because we fail fast before dialing
+	if err == nil {
+		t.Fatal("expected error from ConnectRelays with HTTPS_PROXY set, got nil")
+	}
+
+	msg := err.Error()
+	if !strings.Contains(msg, "HTTPS_PROXY") {
+		t.Errorf("expected error to mention HTTPS_PROXY, got: %v", err)
+	}
+	if !strings.Contains(msg, "9050") {
+		t.Errorf("expected error to mention the proxy host:port (9050), got: %v", err)
+	}
+}
+
+// TestConnectRelaysRejectsBlankURL: regression guard. With no
+// relay URLs configured, ConnectRelays returns a clear error
+// instead of letting the dial code blow up later.
+func TestConnectRelaysRejectsBlankURL(t *testing.T) {
+	clearProxyEnv(t)
+	s := &Session{
+		ClientPrivateKey: "0000000000000000000000000000000000000000000000000000000000000001",
+		ClientPublicKey:  "0000000000000000000000000000000000000000000000000000000000000002",
+		RelayURLs:        nil,
+	}
+	err := s.ConnectRelays(nil)
+	if err == nil {
+		t.Fatal("expected error for blank relay URLs, got nil")
+	}
+	if !strings.Contains(err.Error(), "no relay URLs") {
+		t.Errorf("expected 'no relay URLs' in error, got: %v", err)
+	}
+}
+
+// TestConnectRelaysRejectsNonWSSURL: regression guard. A relay
+// URL that's not ws:// or wss:// (e.g. an http:// typo or a bare
+// host) is rejected up front.
+func TestConnectRelaysRejectsNonWSSURL(t *testing.T) {
+	clearProxyEnv(t)
+	cases := [][]string{
+		{"http://relay.damus.io"},
+		{"relay.damus.io"},
+		{"ftp://relay.damus.io"},
+		{"not a url"},
+		// Mixed: one good, one bad — should still fail on the bad one.
+		{"wss://relay.damus.io", "http://other.invalid"},
+	}
+	for _, urls := range cases {
+		t.Run(strings.Join(urls, ","), func(t *testing.T) {
+			s := &Session{
+				ClientPrivateKey: "0000000000000000000000000000000000000000000000000000000000000001",
+				ClientPublicKey:  "0000000000000000000000000000000000000000000000000000000000000002",
+				RelayURLs:        urls,
+			}
+			err := s.ConnectRelays(nil)
+			if err == nil {
+				t.Fatalf("expected error for relay URLs %v, got nil", urls)
+			}
+			if !strings.Contains(err.Error(), "invalid relay URL") {
+				t.Errorf("expected 'invalid relay URL' in error, got: %v", err)
+			}
+		})
+	}
+}
+
+// TestSessionCloseIsIdempotent: Close on a session that never
+// connected (no proxy error fired the dial, so s.relays is nil)
+// must not panic. Pins that the nil-relay guard added by the
+// upstream Close implementation is preserved.
+func TestSessionCloseIsIdempotent(t *testing.T) {
+	clearProxyEnv(t)
+	s := &Session{RelayURLs: []string{"wss://relay.damus.io"}}
+	// Should not panic.
+	s.Close()
+	s.Close() // twice
+}