diff --git a/lwwallet/AGENTS.md b/lwwallet/AGENTS.md index 3f4f8b4b2..6bd768fde 100644 --- a/lwwallet/AGENTS.md +++ b/lwwallet/AGENTS.md @@ -41,7 +41,13 @@ base logic with the neutrino-backed `btcwbackend` sibling via the extracted (transactions, blocks, headers) are cached in LRU caches bounded by cumulative serialized byte size (see `esplora_cache.go`). Mutable live data (tip height, UTXOs, fee estimates) is never cached. Cache integrity: every - response is verified to hash to the requested key before insertion. + response is verified to hash to the requested key before insertion. Every + response also passes an endpoint-shape guard before reaching a parser; a + 200 carrying an HTML body is rejected with `ErrNotEsploraAPI`. +- `ErrNotEsploraAPI` — Sentinel error returned when an endpoint answers with + an HTML page. It marks a misconfigured base URL rather than a transient + failure, so callers can treat it as fatal instead of retrying. Match with + `errors.Is`; the message names the likely fix (the `/api` suffix). - `EsploraChainService` — `chain.Interface` adapter over `EsploraClient`, driven by a shared `TipPoller`. Feeds btcwallet's internal address-credit pipeline. Constructor: `NewEsploraChainService(esplora, tipPoller, logger)`. @@ -89,6 +95,16 @@ base logic with the neutrino-backed `btcwbackend` sibling via the extracted (known limitation; acceptable for confirmation-target use cases). - LRU caches only hold immutable, hash-addressed data; a verified hash prevents a compromised Esplora endpoint from injecting arbitrary cache entries. +- The HTML guard keys on `Content-Type` alone, never on the body bytes. + `/tx/:txid/raw` and `/block/:hash/raw` return arbitrary binary that can + legitimately begin with the same `<` byte an HTML document does, so + sniffing would reject valid responses. A missing or unparseable content + type falls through to the caller's own parser rather than being reported + as a misconfiguration. +- Response bodies quoted back in an error are collapsed onto one line and + bounded to 256 bytes, cut on a rune boundary. A base URL naming a web + frontend answers with a full HTML page, and the unbounded form carried + kilobytes of markup through every layer up to the RPC client. - `scriptHashHex` hex-encodes the SHA256 digest in its natural byte order. Esplora's REST API differs from the Electrum wire protocol here, which reverses it, and a wrong order fails silently because the API answers an diff --git a/lwwallet/CLAUDE.md b/lwwallet/CLAUDE.md index 3f4f8b4b2..6bd768fde 100644 --- a/lwwallet/CLAUDE.md +++ b/lwwallet/CLAUDE.md @@ -41,7 +41,13 @@ base logic with the neutrino-backed `btcwbackend` sibling via the extracted (transactions, blocks, headers) are cached in LRU caches bounded by cumulative serialized byte size (see `esplora_cache.go`). Mutable live data (tip height, UTXOs, fee estimates) is never cached. Cache integrity: every - response is verified to hash to the requested key before insertion. + response is verified to hash to the requested key before insertion. Every + response also passes an endpoint-shape guard before reaching a parser; a + 200 carrying an HTML body is rejected with `ErrNotEsploraAPI`. +- `ErrNotEsploraAPI` — Sentinel error returned when an endpoint answers with + an HTML page. It marks a misconfigured base URL rather than a transient + failure, so callers can treat it as fatal instead of retrying. Match with + `errors.Is`; the message names the likely fix (the `/api` suffix). - `EsploraChainService` — `chain.Interface` adapter over `EsploraClient`, driven by a shared `TipPoller`. Feeds btcwallet's internal address-credit pipeline. Constructor: `NewEsploraChainService(esplora, tipPoller, logger)`. @@ -89,6 +95,16 @@ base logic with the neutrino-backed `btcwbackend` sibling via the extracted (known limitation; acceptable for confirmation-target use cases). - LRU caches only hold immutable, hash-addressed data; a verified hash prevents a compromised Esplora endpoint from injecting arbitrary cache entries. +- The HTML guard keys on `Content-Type` alone, never on the body bytes. + `/tx/:txid/raw` and `/block/:hash/raw` return arbitrary binary that can + legitimately begin with the same `<` byte an HTML document does, so + sniffing would reject valid responses. A missing or unparseable content + type falls through to the caller's own parser rather than being reported + as a misconfiguration. +- Response bodies quoted back in an error are collapsed onto one line and + bounded to 256 bytes, cut on a rune boundary. A base URL naming a web + frontend answers with a full HTML page, and the unbounded form carried + kilobytes of markup through every layer up to the RPC client. - `scriptHashHex` hex-encodes the SHA256 digest in its natural byte order. Esplora's REST API differs from the Electrum wire protocol here, which reverses it, and a wrong order fails silently because the API answers an diff --git a/lwwallet/esplora.go b/lwwallet/esplora.go index 9c58ac536..b7b9bdc93 100644 --- a/lwwallet/esplora.go +++ b/lwwallet/esplora.go @@ -10,10 +10,12 @@ import ( "fmt" "io" "log/slog" + "mime" "net/http" "strconv" "strings" "time" + "unicode/utf8" "github.com/btcsuite/btcd/chainhash/v2" "github.com/btcsuite/btcd/wire/v2" @@ -689,6 +691,76 @@ func (c *EsploraClient) GetOutspend(ctx context.Context, txid chainhash.Hash, return &outspend, nil } +// ErrNotEsploraAPI is returned when an endpoint answers a request with an +// HTML page. It signals a misconfigured base URL rather than a transient +// failure, so callers can treat it as fatal instead of retrying. +var ErrNotEsploraAPI = errors.New("endpoint is not an Esplora REST API") + +// maxErrBodyBytes bounds how much of a response body an error quotes back. +// A base URL pointing at a web frontend answers with a full HTML page, and +// the unbounded form of these errors carried three kilobytes of markup +// through every layer up to the RPC client. +const maxErrBodyBytes = 256 + +// truncateBody renders a response body for an error message: collapsed onto +// one line, since a multi-line body is illegible inside a single-line log +// record, and bounded to maxErrBodyBytes. +// +// The cut lands on a rune boundary. A body is arbitrary bytes from an +// endpoint we have just decided we do not trust the shape of, so slicing it +// blindly would emit a half-encoded rune into the log. +func truncateBody(body []byte) string { + collapsed := strings.Join(strings.Fields(string(body)), " ") + if len(collapsed) <= maxErrBodyBytes { + return collapsed + } + + cut := maxErrBodyBytes + for cut > 0 && !utf8.RuneStart(collapsed[cut]) { + cut-- + } + + return collapsed[:cut] + "... (truncated)" +} + +// checkNotHTML rejects an otherwise-successful response that carries an HTML +// body. +// +// No Esplora endpoint answers with HTML, so such a response did not come +// from the API: something else is answering at this address. The case that +// motivates the check is a base URL naming a mempool.space web frontend +// rather than its REST root. That frontend is a single-page app, so it +// serves index.html with a 200 for every path it does not route, including +// every path this client asks for. The status check above therefore passes +// and the markup reaches a parser, surfacing as +// `strconv.ParseInt: parsing "…` from inside wallet startup +// — several layers away from the setting that caused it. +// +// The check keys on the content type alone. Body sniffing would be wrong +// here: /tx/:txid/raw and /block/:hash/raw return arbitrary binary, which +// can legitimately begin with the same '<' byte an HTML document does. +func (c *EsploraClient) checkNotHTML(resp *http.Response, path string) error { + contentType := resp.Header.Get("Content-Type") + if contentType == "" { + return nil + } + + // A parse failure is deliberately not treated as evidence either way. + // ParseMediaType still yields the media type when only the parameters + // are malformed, and yields the empty string otherwise, which falls + // through to the caller's own parser rather than being reported as a + // misconfiguration. + mediaType, _, _ := mime.ParseMediaType(contentType) + if mediaType != "text/html" { + return nil + } + + return fmt.Errorf("%w: %s%s answered with an HTML page; a "+ + "mempool.space or Esplora instance serves this API under "+ + "/api, so the URL likely needs that suffix (e.g. "+ + "https://mempool.space/api)", ErrNotEsploraAPI, c.baseURL, path) +} + // get performs an HTTP GET request and returns the response body. func (c *EsploraClient) get(ctx context.Context, path string) ([]byte, error) { req, err := http.NewRequestWithContext( @@ -713,7 +785,11 @@ func (c *EsploraClient) get(ctx context.Context, path string) ([]byte, error) { if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("HTTP %d: %s", resp.StatusCode, - string(body)) + truncateBody(body)) + } + + if err := c.checkNotHTML(resp, path); err != nil { + return nil, err } return body, nil @@ -773,7 +849,11 @@ func (c *EsploraClient) TestMempoolAccept(ctx context.Context, if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("HTTP %d: %s", resp.StatusCode, - string(respBody)) + truncateBody(respBody)) + } + + if err := c.checkNotHTML(resp, "/txs/test"); err != nil { + return nil, err } var results []testMempoolAcceptResult @@ -868,7 +948,11 @@ func (c *EsploraClient) SubmitPackage(ctx context.Context, if resp.StatusCode != http.StatusOK { return fmt.Errorf("submit package HTTP %d: %s", resp.StatusCode, - string(respBody)) + truncateBody(respBody)) + } + + if err := c.checkNotHTML(resp, "/txs/package"); err != nil { + return err } if len(respBody) > 0 { @@ -959,7 +1043,11 @@ func (c *EsploraClient) post(ctx context.Context, path string, body string) ( if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("HTTP %d: %s", resp.StatusCode, - string(respBody)) + truncateBody(respBody)) + } + + if err := c.checkNotHTML(resp, path); err != nil { + return nil, err } return respBody, nil diff --git a/lwwallet/esplora_test.go b/lwwallet/esplora_test.go index c9e52e372..cf0512a85 100644 --- a/lwwallet/esplora_test.go +++ b/lwwallet/esplora_test.go @@ -7,7 +7,9 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" + "unicode/utf8" "github.com/btcsuite/btcd/chainhash/v2" "github.com/btcsuite/btcd/wire/v2" @@ -456,3 +458,171 @@ func TestScriptHashEncoding(t *testing.T) { t, hex.EncodeToString(reversed), scriptHashHex(pkScript), ) } + +// mempoolFrontendHTML is the opening of the single-page app a mempool.space +// web frontend serves for any path it does not route. It is what the client +// receives when its base URL names the frontend rather than the REST root +// under /api. +const mempoolFrontendHTML = ` + + + + + mempool - Bitcoin Explorer + + + + + + + + +` + +// newHTMLFrontendServer returns a server that imitates that frontend: every +// path answers 200 with the HTML document rather than 404, which is what +// makes the misconfiguration survive the status check. +func newHTMLFrontendServer(t *testing.T) *httptest.Server { + t.Helper() + + srv := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/html; "+ + "charset=utf-8") + w.WriteHeader(http.StatusOK) + + _, err := w.Write([]byte(mempoolFrontendHTML)) + require.NoError(t, err) + }), + ) + t.Cleanup(srv.Close) + + return srv +} + +// TestEsploraRejectsHTMLResponse verifies that a 200 carrying an HTML body +// is reported as a misconfigured endpoint rather than handed to a parser. +// Without the check the markup reaches strconv.ParseInt and surfaces from +// inside wallet startup as a parse error quoting the whole document. +func TestEsploraRejectsHTMLResponse(t *testing.T) { + t.Parallel() + + srv := newHTMLFrontendServer(t) + + client := NewEsploraClient(srv.URL, btclog.Disabled) + _, err := client.GetTipHeight(t.Context()) + require.ErrorIs(t, err, ErrNotEsploraAPI) + + // The message has to name the fix, since the symptom points at a + // parser rather than at the setting that caused it. + require.Contains(t, err.Error(), "/api") + require.Contains(t, err.Error(), "/blocks/tip/height") + + // And it must not carry the page: the unbounded form of this error + // dragged kilobytes of markup up through every layer to the caller. + require.NotContains(t, err.Error(), "") + require.Less(t, len(err.Error()), 512) +} + +// TestEsploraRejectsHTMLResponseOnPost verifies the POST path is guarded +// too. A misconfigured base URL reaches broadcast the same way it reaches +// every read, and the SPA answers a POST with the same 200 and page. +func TestEsploraRejectsHTMLResponseOnPost(t *testing.T) { + t.Parallel() + + srv := newHTMLFrontendServer(t) + + client := NewEsploraClient(srv.URL, btclog.Disabled) + _, err := client.BroadcastTx(t.Context(), wire.NewMsgTx(2)) + require.ErrorIs(t, err, ErrNotEsploraAPI) + require.Contains(t, err.Error(), "/tx") +} + +// TestEsploraAcceptsBinaryBodyStartingWithAngleBracket pins that the guard +// keys on the content type and not on the body. The raw transaction and raw +// block endpoints return arbitrary binary, which can legitimately begin with +// the same '<' byte an HTML document does; sniffing the body would reject +// those responses. A transaction with version 60 serializes to a leading +// 0x3c, which is exactly that byte. +func TestEsploraAcceptsBinaryBodyStartingWithAngleBracket(t *testing.T) { + t.Parallel() + + tx := wire.NewMsgTx(60) + tx.AddTxIn(wire.NewTxIn(&wire.OutPoint{}, nil, nil)) + tx.AddTxOut(wire.NewTxOut(1000, []byte{0x00, 0x14})) + + var buf bytes.Buffer + require.NoError(t, tx.Serialize(&buf)) + require.Equal(t, byte('<'), buf.Bytes()[0]) + + srv := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set( + "Content-Type", "application/octet-stream", + ) + + _, err := w.Write(buf.Bytes()) + require.NoError(t, err) + }), + ) + defer srv.Close() + + client := NewEsploraClient(srv.URL, btclog.Disabled) + got, err := client.GetRawTx(t.Context(), tx.TxHash()) + require.NoError(t, err) + require.Equal(t, tx.TxHash(), got.TxHash()) +} + +// TestEsploraHTTPErrorBodyTruncated verifies a non-200 body is collapsed +// and bounded in the error. A misconfigured base URL naming a web frontend +// answers with a full HTML page rather than a short status line, so the +// error path needs a bound to stay readable. +func TestEsploraHTTPErrorBodyTruncated(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + + _, err := w.Write([]byte(strings.Repeat("x", 4096))) + require.NoError(t, err) + }), + ) + defer srv.Close() + + client := NewEsploraClient(srv.URL, btclog.Disabled) + _, err := client.GetTipHeight(t.Context()) + require.Error(t, err) + require.Contains(t, err.Error(), "404") + require.Contains(t, err.Error(), "truncated") + require.Less(t, len(err.Error()), 512) +} + +// TestTruncateBody covers the shapes the helper has to handle: a short body +// passes through unchanged, a multi-line body is collapsed onto one line so +// it stays legible inside a single-line log record, and a long body is cut. +func TestTruncateBody(t *testing.T) { + t.Parallel() + + require.Equal( + t, "already short", + truncateBody( + []byte("already short"), + ), + ) + + require.Equal(t, "a b c", truncateBody([]byte("a\n b\n\tc\n"))) + + long := truncateBody([]byte(strings.Repeat("y", maxErrBodyBytes+10))) + require.Len(t, long, maxErrBodyBytes+len("... (truncated)")) + require.Contains(t, long, "truncated") + + // A cut that would land inside a multi-byte rune backs up to the + // rune boundary rather than emitting a half-encoded rune. "€" is + // three bytes, so a body of them has no boundary at maxErrBodyBytes + // unless the helper looks for one. + runes := truncateBody( + []byte(strings.Repeat("€", maxErrBodyBytes)), + ) + require.True(t, utf8.ValidString(runes)) +}