From 97585df09d39eb6e7d88510a633a6bc181198c99 Mon Sep 17 00:00:00 2001 From: hoot-dev Date: Tue, 15 Sep 2026 12:38:44 -0400 Subject: [PATCH] fix(nip46): refuse to dial relays through inherited *_PROXY env vars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hoot's NIP-46 sign-in flow uses github.com/nbd-wtf/go-nostr's nostr.RelayConnect, which delegates to net/http and therefore honors HTTPS_PROXY / HTTP_PROXY / ALL_PROXY / SOCKS_PROXY env vars inherited from the parent shell. If the user runs hoot inside torsocks, a launcher that exports HTTPS_PROXY, or a shell that already has Tor configured for monerod / curl, the wss:// dials to relay.damus.io / relay.nostr.band / nostr.wine (or whatever the user picked) get routed through 127.0.0.1:9050. When nothing is listening there — which is the typical case on a workstation without an active Tor — every dial fails with 'dial tcp 127.0.0.1:9050: connect: connection refused', which bubbles up as 'failed to connect to port 9050' in the TUI and gives the user no actionable signal. Fix it two ways: - nip46.ConnectRelays now refuses to dial at all when any *_PROXY env var is set, returning a clear error that names the offending env var and the proxy host:port. The user can either unset it (or run hoot outside torsocks). This matches what every other Nostr TUI does — nak, lume, etc. — none of them proxy wss:// connections by default. - Up-front validation of every configured relay URL (rejecting blanks and non-ws:// / non-wss:// schemes) so a misconfigured relays.txt surfaces a clear error instead of a confusing websocket one. Anchored against master after PR #69 / #70 / #72 restructured the NIP-46 dial path (RelayURL string -> RelayURLs []string, WaitForConnection -> ConnectRelays + CheckConnection). Tests updated accordingly: ConnectRelays is the new dial entry point, and validateRelayURLs is exercised with single-bad-URL and mixed-good-and-bad inputs. Tests in nip46/nip46_test.go pin: - the proxy env var detection order (HTTPS_PROXY beats HTTP_PROXY beats ALL_PROXY; socks fallbacks come last) - that malformed or empty proxy values fall through to the next valid var (matching Go's stdlib httpproxy behaviour) - that ConnectRelays surfaces the env var name and port on the user-visible error path (this is the bug's regression guard) - that blank and non-wss relay URLs are rejected up front instead of producing a confusing websocket error later - that Session.Close is idempotent when no dial was attempted --- nip46/nip46.go | 89 +++++++++++++++++ nip46/nip46_test.go | 231 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 320 insertions(+) diff --git a/nip46/nip46.go b/nip46/nip46.go index b23c5bc..a99ecbe 100644 --- a/nip46/nip46.go +++ b/nip46/nip46.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/url" + "os" "strings" "sync" "time" @@ -83,6 +84,68 @@ 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 @@ -90,6 +153,32 @@ func dedupStrings(ss []string) []string { // 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 diff --git a/nip46/nip46_test.go b/nip46/nip46_test.go index e371f42..e604978 100644 --- a/nip46/nip46_test.go +++ b/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 +}