diff --git a/src/transport/BaseRpcTransport.ts b/src/transport/BaseRpcTransport.ts index ace7e92..9bb70cb 100644 --- a/src/transport/BaseRpcTransport.ts +++ b/src/transport/BaseRpcTransport.ts @@ -14,13 +14,13 @@ export interface JsonRpcEnvelope { /** * JSON-RPC 2.0 response. Either `result` or `error` is set. */ -type JsonRpcResponseEnvelope = - | { jsonrpc?: string; id: number | string | null; result: unknown } - | { - jsonrpc?: string; - id: number | string | null; - error: { code: number; message: string; data?: unknown }; - }; +type JsonRpcResponseEnvelope = { + jsonrpc?: string; + id: number | string | null; + result?: unknown; + // null is tolerated: some non-strict servers send "error": null on success + error?: { code: number; message: string; data?: unknown } | null; +}; /** * Optional convenience base class for users writing new wire-level @@ -98,9 +98,16 @@ export abstract class BaseRpcTransport implements Transport { throw new TransportRpcError(-32603, "malformed JSON-RPC response", raw); } const response = raw as JsonRpcResponseEnvelope; - if ("error" in response) { - const { code, message, data } = response.error; - throw new TransportRpcError(code, message, data); + // null check, not key presence — some servers include "error": null + // on successful responses + if (response.error != null) { + // non-object error payloads (e.g. a bare string) fall through to the + // malformed-response error below, which carries the raw response + if (typeof response.error === "object") { + const { code, message, data } = response.error; + throw new TransportRpcError(code, message, data); + } + throw new TransportRpcError(-32603, "malformed JSON-RPC response", raw); } if ("result" in response) { return response.result as T; diff --git a/src/transport/HttpTransport.ts b/src/transport/HttpTransport.ts index 22d7816..ea3ea19 100644 --- a/src/transport/HttpTransport.ts +++ b/src/transport/HttpTransport.ts @@ -1,5 +1,5 @@ import {BaseRpcTransport, type JsonRpcEnvelope} from "./BaseRpcTransport"; -import type {RequestOptions, Transport} from "./Transport"; +import {type RequestOptions, type Transport, TransportRpcError} from "./Transport"; /** * Construction options for {@link HttpTransport}. @@ -67,7 +67,36 @@ export class HttpTransport extends BaseRpcTransport { redirect: "follow", signal: options?.signal, }); - return await response.json(); + const responseText = await response.text(); + let parsed: unknown; + try { + parsed = JSON.parse(responseText); + } catch { + // non-JSON body (HTML error page, plain text) — the HTTP status is + // the real diagnostic, so surface it instead of a JSON parse error + throw new TransportRpcError( + -32603, + `HTTP ${response.status} ${response.statusText}: response body is not JSON`.trim(), + responseText.slice(0, 1000), + ); + } + const hasRpcError = + typeof parsed === "object" && + parsed != null && + "error" in parsed && + (parsed as {error: unknown}).error != null; + if (!response.ok && !hasRpcError) { + // On HTTP failure, only a JSON-RPC *error* envelope falls through + // (parseResponse reports the server's error, e.g. a 429 rate + // limit). Anything else — including a contradictory "result" — is + // not trusted; the status is the real diagnostic. + throw new TransportRpcError( + -32603, + `HTTP ${response.status} ${response.statusText}`.trim(), + parsed, + ); + } + return parsed; } } diff --git a/src/transport/JsonRpcNode.ts b/src/transport/JsonRpcNode.ts index e83b41c..6e0473e 100644 --- a/src/transport/JsonRpcNode.ts +++ b/src/transport/JsonRpcNode.ts @@ -296,6 +296,12 @@ export class JsonRpcNode implements Transport { } else if (gasPrice != null) { maxFeePerGas = scaleBigIntByGasLevel(gasPrice, gasLevel); priorityFee = maxFeePerGas; + } else if (maxPriorityFeePerGas != null) { + // eth_gasPrice unsupported but the node served a priority fee — + // use it rather than discarding it for the 1-gwei floor + // (the clamp below raises maxFeePerGas to at least priorityFee) + priorityFee = scaleBigIntByGasLevel(maxPriorityFeePerGas, gasLevel); + maxFeePerGas = scaleBigIntByGasLevel(1_000_000_000n, gasLevel); } else { maxFeePerGas = scaleBigIntByGasLevel(1_000_000_000n, gasLevel); priorityFee = maxFeePerGas; diff --git a/src/utils.ts b/src/utils.ts index 17a0723..ead4095 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -538,7 +538,43 @@ export async function sendJsonRpcRequest( body: raw, redirect: "follow", }); - const response = (await fetchResult.json()) as JsonRpcResponse; + const responseText = await fetchResult.text(); + let response: JsonRpcResponse; + try { + response = JSON.parse(responseText) as JsonRpcResponse; + } catch { + // non-JSON body (HTML error page, plain text) — the HTTP status is the + // real diagnostic, so surface it instead of a JSON parse error + throw new TransportRpcError( + -32603, + `HTTP ${fetchResult.status} ${fetchResult.statusText}: response body is not JSON`.trim(), + responseText.slice(0, 1000), + ); + } + if (typeof response !== "object" || response === null) { + // scalar or null payload — the `in` checks below would throw a + // TypeError; report it as a malformed response with the HTTP status + throw new TransportRpcError( + -32603, + `HTTP ${fetchResult.status} ${fetchResult.statusText}: malformed JSON-RPC response`.trim(), + response, + ); + } + if (!fetchResult.ok) { + // An HTTP failure status wins over any "result" in the body — a + // success payload delivered with a failure status is contradictory + // and not trusted. A JSON-RPC error envelope still reports the + // server's own error (e.g. a 429 rate limit). + const err = response.error as JsonRpcError | undefined; + if (err != null && typeof err === "object") { + throw new TransportRpcError(err.code, err.message); + } + throw new TransportRpcError( + -32603, + `HTTP ${fetchResult.status} ${fetchResult.statusText}`.trim(), + response, + ); + } if ("result" in response) { return response.result as JsonRpcResult; } @@ -548,6 +584,15 @@ export async function sendJsonRpcRequest( return response.simulation_results as JsonRpcResult; } const err = response.error as JsonRpcError; + if (err == null || typeof err !== "object") { + // no result and no error object — report the HTTP status rather than + // crashing on err.code + throw new TransportRpcError( + -32603, + `HTTP ${fetchResult.status} ${fetchResult.statusText}: malformed JSON-RPC response`.trim(), + response, + ); + } throw new TransportRpcError(err.code, err.message); } diff --git a/test/safe/isDeployed.test.js b/test/safe/isDeployed.test.js index fe5ad8f..68b457a 100644 --- a/test/safe/isDeployed.test.js +++ b/test/safe/isDeployed.test.js @@ -19,8 +19,13 @@ describe('SafeAccount.isDeployed', () => { function mockFetchReturning(code) { global.fetch = async (url, options) => { lastRequest = { url, body: JSON.parse(options.body) }; + const body = { jsonrpc: '2.0', id: 1, result: code }; return { - json: async () => ({ jsonrpc: '2.0', id: 1, result: code }), + ok: true, + status: 200, + statusText: 'OK', + text: async () => JSON.stringify(body), + json: async () => body, }; }; } diff --git a/test/transport/HttpTransport.test.js b/test/transport/HttpTransport.test.js index ea3f7d5..b39202c 100644 --- a/test/transport/HttpTransport.test.js +++ b/test/transport/HttpTransport.test.js @@ -18,6 +18,8 @@ describe("HttpTransport", () => { return { ok: status < 400, status, + statusText: "", + text: async () => JSON.stringify(body), json: async () => body, }; } @@ -93,6 +95,31 @@ describe("HttpTransport", () => { }); }); + test("treats a non-object error payload as a malformed response", async () => { + const fetch = makeMockFetch(() => + jsonResponse({ jsonrpc: "2.0", id: 1, error: "rate limited" }), + ); + const t = new ak.HttpTransport("https://example.test/rpc", { fetch }); + + await expect(t.request({ method: "eth_chainId" })).rejects.toMatchObject({ + name: "TransportRpcError", + code: -32603, + data: { error: "rate limited" }, + }); + }); + + test("preserves the message of an error object without a code", async () => { + const fetch = makeMockFetch(() => + jsonResponse({ jsonrpc: "2.0", id: 1, error: { message: "rate limited" } }), + ); + const t = new ak.HttpTransport("https://example.test/rpc", { fetch }); + + await expect(t.request({ method: "eth_chainId" })).rejects.toMatchObject({ + name: "TransportRpcError", + message: "rate limited", + }); + }); + test("merges user-supplied headers and pins Content-Type", async () => { const fetch = makeMockFetch(() => jsonResponse({ jsonrpc: "2.0", id: 1, result: "0x" })); const t = new ak.HttpTransport("https://example.test/rpc", { diff --git a/test/transport/httpErrorHandling.test.js b/test/transport/httpErrorHandling.test.js new file mode 100644 index 0000000..1be6224 --- /dev/null +++ b/test/transport/httpErrorHandling.test.js @@ -0,0 +1,142 @@ +// Offline tests for HTTP/JSON-RPC error handling: error:null successes, +// non-JSON error bodies, and enveloped-but-empty responses must surface +// useful diagnostics instead of TypeErrors or JSON parse errors. + +const ak = require("../../dist/index.cjs"); + +function mockFetch(status, body, statusText = "") { + return async () => ({ + ok: status >= 200 && status < 300, + status, + statusText, + text: async () => (typeof body === "string" ? body : JSON.stringify(body)), + json: async () => JSON.parse(typeof body === "string" ? body : JSON.stringify(body)), + }); +} + +function transportWith(status, body, statusText) { + return new ak.HttpTransport("https://example.test/rpc", { + fetch: mockFetch(status, body, statusText), + }); +} + +describe("HttpTransport error handling", () => { + test("treats error:null alongside result as success", async () => { + const t = transportWith(200, { jsonrpc: "2.0", id: 1, result: "0x1", error: null }); + expect(await t.request({ method: "eth_chainId", params: [] })).toBe("0x1"); + }); + + test("surfaces the HTTP status for a non-JSON error body", async () => { + const t = transportWith(502, "Bad Gateway", "Bad Gateway"); + await expect(t.request({ method: "eth_chainId", params: [] })).rejects.toMatchObject({ + name: "TransportRpcError", + message: expect.stringContaining("502"), + }); + }); + + test("surfaces the HTTP status for a JSON error body without an envelope", async () => { + const t = transportWith(401, { message: "unauthorized" }, "Unauthorized"); + await expect(t.request({ method: "eth_chainId", params: [] })).rejects.toMatchObject({ + name: "TransportRpcError", + message: expect.stringContaining("401"), + }); + }); + + test("still reports the server's JSON-RPC error from an HTTP failure", async () => { + const t = transportWith(429, { + jsonrpc: "2.0", + id: 1, + error: { code: -32005, message: "rate limited" }, + }); + await expect(t.request({ method: "eth_chainId", params: [] })).rejects.toMatchObject({ + code: -32005, + message: "rate limited", + }); + }); + + test("does not trust a result delivered with an HTTP failure status", async () => { + const t = transportWith(401, { jsonrpc: "2.0", id: 1, result: "ok" }, "Unauthorized"); + await expect(t.request({ method: "eth_chainId", params: [] })).rejects.toMatchObject({ + name: "TransportRpcError", + message: expect.stringContaining("401"), + }); + }); + + test("surfaces the HTTP status for an error:null envelope on an HTTP failure", async () => { + const t = transportWith(401, { jsonrpc: "2.0", id: 1, error: null }, "Unauthorized"); + await expect(t.request({ method: "eth_chainId", params: [] })).rejects.toMatchObject({ + name: "TransportRpcError", + message: expect.stringContaining("401"), + }); + }); +}); + +describe("sendJsonRpcRequest error handling", () => { + const realFetch = globalThis.fetch; + afterEach(() => { + globalThis.fetch = realFetch; + }); + + test("surfaces the HTTP status instead of crashing on a non-envelope body", async () => { + globalThis.fetch = mockFetch(401, { message: "unauthorized" }, "Unauthorized"); + await expect( + ak.sendJsonRpcRequest("https://example.test/rpc", "eth_chainId", []), + ).rejects.toMatchObject({ + name: "TransportRpcError", + message: expect.stringContaining("401"), + }); + }); + + test("surfaces the HTTP status for a non-JSON body", async () => { + globalThis.fetch = mockFetch(502, "Bad Gateway", "Bad Gateway"); + await expect( + ak.sendJsonRpcRequest("https://example.test/rpc", "eth_chainId", []), + ).rejects.toMatchObject({ + name: "TransportRpcError", + message: expect.stringContaining("502"), + }); + }); + + test("does not trust a result delivered with an HTTP failure status", async () => { + globalThis.fetch = mockFetch(401, { jsonrpc: "2.0", id: 1, result: "ok" }, "Unauthorized"); + await expect( + ak.sendJsonRpcRequest("https://example.test/rpc", "eth_chainId", []), + ).rejects.toMatchObject({ + name: "TransportRpcError", + message: expect.stringContaining("401"), + }); + }); + + test("still reports the server's JSON-RPC error from an HTTP failure", async () => { + globalThis.fetch = mockFetch(429, { + jsonrpc: "2.0", + id: 1, + error: { code: -32005, message: "rate limited" }, + }); + await expect( + ak.sendJsonRpcRequest("https://example.test/rpc", "eth_chainId", []), + ).rejects.toMatchObject({ + code: -32005, + message: "rate limited", + }); + }); + + test.each([['"ok"'], ["null"], ["42"]])( + "reports a scalar/null JSON payload (%s) as malformed instead of a TypeError", + async (body) => { + globalThis.fetch = async () => ({ + ok: true, + status: 200, + statusText: "OK", + text: async () => body, + json: async () => JSON.parse(body), + }); + await expect( + ak.sendJsonRpcRequest("https://example.test/rpc", "eth_chainId", []), + ).rejects.toMatchObject({ + name: "TransportRpcError", + message: expect.stringContaining("malformed"), + }); + }, + ); +});