From c6d3890af1ae6c8b84c23e65547cc1b8b09ebd4c Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 4 Oct 2026 17:50:47 +0000 Subject: [PATCH] harden: debug mode logs only a request's origin and JSON-RPC method (closes #410) With debug mode on, debugFetch logged every request's full URL and body, so an RPC endpoint with an API key in its path or query string printed that key to the console on every request. It now logs the HTTP method, the URL's origin and, for a JSON-RPC body, the method name. The balance refresh and token lookup, which logged the RPC URL at debug level, log its origin too. The error lines for failed RPC calls print ethers' short message, since its full message for an HTTP error carries the request URL. The README's DEBUG Mode Policy says what debug mode logs. Model: opus-5-5 --- README.md | 10 ++++ TODO.md | 10 ++++ src/popup/views/confirmTx.js | 2 +- src/popup/views/txStatus.js | 2 +- src/shared/addressWarnings.js | 4 +- src/shared/balances.js | 13 +++-- src/shared/ens.js | 6 ++- src/shared/log.js | 29 +++++++++-- tests/debugFetch.test.js | 44 ++++++++++++++++ tests/rpcErrorLog.test.js | 92 ++++++++++++++++++++++++++++++++++ tests/sendDisplayFloor.test.js | 1 + tests/symbolSpoof.test.js | 1 + 12 files changed, 199 insertions(+), 15 deletions(-) create mode 100644 tests/debugFetch.test.js create mode 100644 tests/rpcErrorLog.test.js diff --git a/README.md b/README.md index 9c4364e..c739ddc 100644 --- a/README.md +++ b/README.md @@ -2168,6 +2168,16 @@ the log level and turns the banner on, and that is all it may ever do: it feeds constant directly, so no runtime toggle in a release build can reach the hardcoded test phrase. +At the raised log level the console also shows the wallet's addresses with their +balances and ENS names, the token contracts looked up, and a line for each +request made through `debugFetch` in `src/shared/log.js` (the explorer, the +price feed, the RPC calls a site makes, and the endpoint checks in settings) and +for its response. A request is logged by its HTTP method, the origin of its URL +(scheme, host and port) and, for a JSON-RPC call, the method name; the balance +refresh and token lookup name the RPC endpoint by its origin too. The URL's path +and query string, where RPC providers put API keys, and the request body are +never logged. + ### Key Decisions - **No framework**: The popup UI is vanilla JS and HTML. The extension is small diff --git a/TODO.md b/TODO.md index ab2810c..89a4edd 100644 --- a/TODO.md +++ b/TODO.md @@ -45,6 +45,16 @@ but the review is broader than any of them. # Completed Steps +- 2026-10-04: Debug mode no longer writes RPC API keys to the console + ([#410](https://git.eeqj.de/sneak/AutistMask/issues/410)). `debugFetch` logged + every request's full URL and body, so an RPC endpoint with a key in its path + or query string printed that key on every request. It now logs the HTTP + method, the URL's origin and, for a JSON-RPC call, the method name. The + balance refresh and token lookup log the RPC endpoint by its origin too. A + failed RPC call's error line prints the error's short message, which names the + HTTP status, not its full message, which carries the request URL. The README's + DEBUG Mode Policy says what debug mode logs. + - 2026-10-04: A site has at most one connection prompt and one signature prompt open at a time ([#405](https://git.eeqj.de/sneak/AutistMask/issues/405)). Each `eth_requestAccounts` or `personal_sign` call opened another approval window, diff --git a/src/popup/views/confirmTx.js b/src/popup/views/confirmTx.js index 5603865..0c8cacb 100644 --- a/src/popup/views/confirmTx.js +++ b/src/popup/views/confirmTx.js @@ -395,7 +395,7 @@ async function estimateGas(txInfo) { feeWei = gasCostWei; renderValidation(txInfo); } catch (e) { - log.errorf("gas estimation failed:", e.message); + log.errorf("gas estimation failed:", e.shortMessage || e.message); if (pendingTx !== txInfo) return; $("confirm-fee-amount").textContent = "Unable to estimate"; setVisible("confirm-fee-reserve", false); diff --git a/src/popup/views/txStatus.js b/src/popup/views/txStatus.js index 2c2a101..56537f4 100644 --- a/src/popup/views/txStatus.js +++ b/src/popup/views/txStatus.js @@ -133,7 +133,7 @@ function startWait(txInfo, txHash, broadcastTime, pollNow) { // failed — which matters most on a resumed wait, where the // first poll is already past the deadline. answered = false; - log.errorf("poll receipt failed:", e.message); + log.errorf("poll receipt failed:", e.shortMessage || e.message); } // The lookup is async: the wait may have ended while it was in // flight, in which case this result must not touch the view. diff --git a/src/shared/addressWarnings.js b/src/shared/addressWarnings.js index 5300b0e..d1a5bc5 100644 --- a/src/shared/addressWarnings.js +++ b/src/shared/addressWarnings.js @@ -75,7 +75,7 @@ async function getFullWarnings(address, provider, options = {}) { }); } } catch (e) { - log.errorf("contract check failed:", e.message); + log.errorf("contract check failed:", e.shortMessage || e.message); } // Skip tx count check for contracts — they may legitimately have @@ -92,7 +92,7 @@ async function getFullWarnings(address, provider, options = {}) { }); } } catch (e) { - log.errorf("tx count check failed:", e.message); + log.errorf("tx count check failed:", e.shortMessage || e.message); } } diff --git a/src/shared/balances.js b/src/shared/balances.js index fcff065..dcbbabf 100644 --- a/src/shared/balances.js +++ b/src/shared/balances.js @@ -10,7 +10,7 @@ const { } = require("ethers"); const { ERC20_ABI } = require("./constants"); const { NETWORKS } = require("./networks"); -const { log, debugFetch } = require("./log"); +const { log, debugFetch, urlOrigin } = require("./log"); const { deriveAddressFromXpub } = require("./wallet"); const { TOKEN_BY_ADDRESS } = require("./tokenList"); const { LOW_HOLDER_THRESHOLD, parseHoldersCount } = require("./holders"); @@ -203,7 +203,7 @@ async function refreshBalances( trackedTokens, networkId, ) { - log.debugf("refreshBalances start, rpc:", rpcUrl); + log.debugf("refreshBalances start, rpc:", urlOrigin(rpcUrl)); const provider = getProvider(rpcUrl, networkId); const updates = []; @@ -246,7 +246,7 @@ async function refreshBalances( log.errorf( "ENS reverse failed", addr.address, - e.message, + e.shortMessage || e.message, ); // Keep existing addr.ensName if we had one }), @@ -280,7 +280,7 @@ async function refreshBalances( // Look up token metadata from its contract. // Calls symbol() and decimals() to verify it implements ERC-20. async function lookupTokenInfo(contractAddress, rpcUrl, networkId) { - log.debugf("lookupTokenInfo", contractAddress, "rpc:", rpcUrl); + log.debugf("lookupTokenInfo", contractAddress, "rpc:", urlOrigin(rpcUrl)); const provider = getProvider(rpcUrl, networkId); const contract = new Contract(contractAddress, ERC20_ABI, provider); @@ -305,7 +305,10 @@ async function lookupTokenInfo(contractAddress, rpcUrl, networkId) { name = await contract.name(); log.debugf("name() =", name); } catch (e) { - log.warnf("name() failed, using symbol as name:", e.message); + log.warnf( + "name() failed, using symbol as name:", + e.shortMessage || e.message, + ); name = symbol; } diff --git a/src/shared/ens.js b/src/shared/ens.js index 4b95660..62e400f 100644 --- a/src/shared/ens.js +++ b/src/shared/ens.js @@ -42,7 +42,11 @@ async function resolveEnsName(address, rpcUrl, networkId) { setCache(address, name); return name; } catch (e) { - log.errorf("ENS reverse lookup failed", address, e.message); + log.errorf( + "ENS reverse lookup failed", + address, + e.shortMessage || e.message, + ); // Don't cache failures — let subsequent lookups retry return null; } diff --git a/src/shared/log.js b/src/shared/log.js index 551fb20..15c2a1c 100644 --- a/src/shared/log.js +++ b/src/shared/log.js @@ -42,14 +42,33 @@ const log = { }, }; -// Fetch wrapper that debug-logs every request and response. +// The origin (scheme, host and port) of a URL, for logging in place of the +// URL: RPC providers put API keys in the path or the query string. A URL that +// does not parse gives "", so logging never stops a request. +function urlOrigin(url) { + try { + return new URL(url).origin; + } catch { + return ""; + } +} + +// Fetch wrapper that debug-logs every request and response. It logs the +// URL's origin and, for a JSON-RPC body, the method name: never the full URL +// or body, which can carry an API key or a signed transaction. async function debugFetch(url, opts) { const method = (opts && opts.method) || "GET"; - const body = opts && opts.body; - log.debugf("fetch →", method, url, body || ""); + const origin = urlOrigin(url); + let rpcMethod = ""; + try { + rpcMethod = JSON.parse(opts.body).method || ""; + } catch { + // no body, or a body that is not JSON + } + log.debugf("fetch →", method, origin, rpcMethod); const resp = await fetch(url, opts); - log.debugf("fetch ←", resp.status, url); + log.debugf("fetch ←", resp.status, origin); return resp; } -module.exports = { log, debugFetch, setRuntimeDebug, isDebug }; +module.exports = { log, debugFetch, urlOrigin, setRuntimeDebug, isDebug }; diff --git a/tests/debugFetch.test.js b/tests/debugFetch.test.js new file mode 100644 index 0000000..20f5165 --- /dev/null +++ b/tests/debugFetch.test.js @@ -0,0 +1,44 @@ +// What debugFetch writes to the console in debug mode. +// +// RPC providers put the API key in the URL's path or query string, and the +// debug log used to print the whole URL and request body, so turning debug +// mode on wrote the key to the console +// (https://git.eeqj.de/sneak/AutistMask/issues/410). The log now names the +// HTTP method, the URL's origin and the JSON-RPC method, and nothing else of +// the request. + +const { debugFetch, setRuntimeDebug } = require("../src/shared/log"); + +const realFetch = globalThis.fetch; + +afterEach(() => { + globalThis.fetch = realFetch; + setRuntimeDebug(false); + jest.restoreAllMocks(); +}); + +test("logs the origin and JSON-RPC method, not the key in the URL", async () => { + setRuntimeDebug(true); + const consoleLog = jest.spyOn(console, "log").mockImplementation(() => {}); + globalThis.fetch = jest.fn(async () => ({ status: 200 })); + + await debugFetch( + "https://rpc.example.invalid/v3/PATHKEY123?token=QUERYTOKEN456", + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "eth_chainId", + params: [], + }), + }, + ); + + const logged = consoleLog.mock.calls.flat().join(" "); + expect(logged).not.toContain("PATHKEY123"); + expect(logged).not.toContain("QUERYTOKEN456"); + expect(logged).toContain("https://rpc.example.invalid"); + expect(logged).toContain("eth_chainId"); +}); diff --git a/tests/rpcErrorLog.test.js b/tests/rpcErrorLog.test.js new file mode 100644 index 0000000..9fcbd2f --- /dev/null +++ b/tests/rpcErrorLog.test.js @@ -0,0 +1,92 @@ +// What reaches the console when the RPC endpoint answers with an HTTP error. +// +// RPC providers put the API key in the endpoint URL's path or query string. +// When the endpoint answers with an HTTP error (a wrong or expired key, a rate +// limit, a server error), the error ethers throws carries the full request URL +// in its message, so a line logging that message printed the key +// (https://git.eeqj.de/sneak/AutistMask/issues/410). Those lines log the +// error's short message, which names the HTTP status and not the URL. +// +// The real ethers provider runs; only its HTTP transport is replaced, by one +// that answers every request with 401 Unauthorized. Debug mode is on, so +// every log level is printed. + +const { FetchRequest } = require("ethers"); +const { getProvider, refreshBalances } = require("../src/shared/balances"); +const { getFullWarnings } = require("../src/shared/addressWarnings"); +const { resolveEnsName } = require("../src/shared/ens"); +const { setRuntimeDebug } = require("../src/shared/log"); + +const RPC_URL = "https://rpc.example.invalid/v3/PATHKEY123?token=QUERYTOKEN456"; +const ADDRESS = "0x1111111111111111111111111111111111111111"; + +const realFetch = globalThis.fetch; +let printed; + +beforeEach(() => { + setRuntimeDebug(true); + printed = []; + for (const method of ["log", "warn", "error"]) { + jest.spyOn(console, method).mockImplementation((...args) => { + printed.push(args.map(String).join(" ")); + }); + } + FetchRequest.registerGetUrl(async () => ({ + statusCode: 401, + statusMessage: "Unauthorized", + headers: {}, + body: new Uint8Array(), + })); + // The explorer requests the balance refresh makes go nowhere. + globalThis.fetch = jest.fn(async () => { + throw new Error("tests must not perform network requests"); + }); +}); + +afterEach(() => { + FetchRequest.registerGetUrl(FetchRequest.createGetUrlFunc()); + globalThis.fetch = realFetch; + setRuntimeDebug(false); + jest.restoreAllMocks(); +}); + +// The line carrying `label` was printed and names the HTTP status, and nothing +// printed carries the key. +function expectFailureLoggedWithoutKey(label) { + const line = printed.find((text) => text.includes(label)); + expect(line).toContain("401"); + const all = printed.join("\n"); + expect(all).not.toContain("PATHKEY123"); + expect(all).not.toContain("QUERYTOKEN456"); +} + +test("ethers puts the URL in the error message, not in the short message", async () => { + const provider = getProvider(RPC_URL, "mainnet"); + const error = await provider.getCode(ADDRESS).catch((e) => e); + expect(error.message).toContain("PATHKEY123"); + expect(error.shortMessage).not.toContain("PATHKEY123"); +}); + +test("the recipient checks before a send", async () => { + await getFullWarnings(ADDRESS, getProvider(RPC_URL, "mainnet")); + expectFailureLoggedWithoutKey("contract check failed"); + expectFailureLoggedWithoutKey("tx count check failed"); +}); + +test("the ENS reverse lookup", async () => { + expect(await resolveEnsName(ADDRESS, RPC_URL, "mainnet")).toBeNull(); + expectFailureLoggedWithoutKey("ENS reverse lookup failed"); +}); + +test("the balance refresh", async () => { + const wallets = [{ addresses: [{ address: ADDRESS }] }]; + await refreshBalances( + wallets, + RPC_URL, + "https://explorer.example.invalid/api/v2", + [], + "mainnet", + ); + expectFailureLoggedWithoutKey("ETH balance failed"); + expectFailureLoggedWithoutKey("ENS reverse failed"); +}); diff --git a/tests/sendDisplayFloor.test.js b/tests/sendDisplayFloor.test.js index c366314..1e0b885 100644 --- a/tests/sendDisplayFloor.test.js +++ b/tests/sendDisplayFloor.test.js @@ -66,6 +66,7 @@ jest.mock("../src/shared/log", () => ({ status: 200, json: async () => mockExplorer.items, })), + urlOrigin: () => "", setRuntimeDebug: () => {}, isDebug: () => false, })); diff --git a/tests/symbolSpoof.test.js b/tests/symbolSpoof.test.js index 94163be..b0b6090 100644 --- a/tests/symbolSpoof.test.js +++ b/tests/symbolSpoof.test.js @@ -44,6 +44,7 @@ jest.mock("../src/shared/log", () => ({ errorf: () => {}, }, debugFetch: jest.fn(), + urlOrigin: () => "", setRuntimeDebug: () => {}, isDebug: () => false, }));