Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions src/transport/BaseRpcTransport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -98,7 +98,9 @@ export abstract class BaseRpcTransport implements Transport {
throw new TransportRpcError(-32603, "malformed JSON-RPC response", raw);
}
const response = raw as JsonRpcResponseEnvelope;
if ("error" in response) {
// null check, not key presence — some servers include "error": null
// on successful responses
if (response.error != null) {
Comment on lines +101 to +103

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Validate JSON-RPC error fields at runtime.

Untrusted payloads such as {"error":"bad"} or {"error":{}} can produce a TransportRpcError with undefined code/message rather than the required malformed-response error.

  • src/transport/BaseRpcTransport.ts#L101-L103: validate that error is an object with numeric code and string message before destructuring.
  • src/utils.ts#L572-L580: extend the existing object guard to validate err.code and err.message; add malformed-error-envelope cases to the transport tests.
📍 Affects 2 files
  • src/transport/BaseRpcTransport.ts#L101-L103 (this comment)
  • src/utils.ts#L572-L580
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/transport/BaseRpcTransport.ts` around lines 101 - 103, Validate the
JSON-RPC error envelope before destructuring in BaseRpcTransport.ts lines
101-103: require error to be an object with numeric code and string message,
otherwise return the existing malformed-response error. Extend the object guard
in src/utils.ts lines 572-580 to validate err.code and err.message, and add
transport tests covering string and incomplete error payloads.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sherifahmed990 this is valid imo

const { code, message, data } = response.error;
throw new TransportRpcError(code, message, data);
}
Expand Down
32 changes: 30 additions & 2 deletions src/transport/HttpTransport.ts
Original file line number Diff line number Diff line change
@@ -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}.
Expand Down Expand Up @@ -67,7 +67,35 @@ 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),
);
}
if (
!response.ok &&
(typeof parsed !== "object" ||
parsed == null ||
(!("error" in parsed) && !("result" in parsed)))
Comment thread
sherifahmed990 marked this conversation as resolved.
Outdated
) {
// HTTP failure without a JSON-RPC envelope (e.g. a gateway's own
// error object) — keep the status; a proper envelope falls through
// to parseResponse which reports the server's JSON-RPC error
throw new TransportRpcError(
-32603,
`HTTP ${response.status} ${response.statusText}`.trim(),
parsed,
);
}
return parsed;
}
}

Expand Down
6 changes: 6 additions & 0 deletions src/transport/JsonRpcNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
32 changes: 31 additions & 1 deletion src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,28 @@ 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 ("result" in response) {
return response.result as JsonRpcResult;
}
Expand All @@ -548,6 +569,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);
}

Expand Down
7 changes: 6 additions & 1 deletion test/safe/isDeployed.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
};
}
Expand Down
2 changes: 2 additions & 0 deletions test/transport/HttpTransport.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ describe("HttpTransport", () => {
return {
ok: status < 400,
status,
statusText: "",
text: async () => JSON.stringify(body),
json: async () => body,
};
}
Expand Down
102 changes: 102 additions & 0 deletions test/transport/httpErrorHandling.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
// 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, "<html>Bad Gateway</html>", "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",
});
});
});

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, "<html>Bad Gateway</html>", "Bad Gateway");
await expect(
ak.sendJsonRpcRequest("https://example.test/rpc", "eth_chainId", []),
).rejects.toMatchObject({
name: "TransportRpcError",
message: expect.stringContaining("502"),
});
});

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"),
});
},
);
});
Loading