From 446ddf0faa4baa03a4b8b2dc3941dd81c1b7b5b8 Mon Sep 17 00:00:00 2001 From: Sy-D <8460326+Sy-D@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:08:56 +0200 Subject: [PATCH 1/2] fix: contain async loggers and redact real preview tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from the GPT-5.6 Sol review of the typed-error-codes work. safeLogger only wrapped a synchronous throw. `Logger` declares `void`, and TypeScript accepts an `async` method there, so a log shipper whose methods reject produced an unhandled rejection — the runtime ends the process for that, mid-handoff, before the relay sandbox is released. The wrapper now attaches a handler to the runtime return value in the same tick it is created; a throwing property getter was already covered and now has a test. Redaction guessed the credential's grammar as `pt_...`, but the preview token is a ~362-character JWT (docs/measurements/01-preview-transport.md §3), so a real one survived both `pt_token%3D` and a bare `invalid preview token `. `waitForHealth` now takes the exact token out of the URL it is polling and redacts that value and its percent-encoded form by comparison; the pattern rules stay as the net for text where the value is not known, with a JWT-shaped rule added. The test fixture is a JWT. The SDK error attached as `cause` kept the unredacted body and message, which every error serialiser prints. It is now a clone with the same prototype, `name`, `status` and `code`, and with `message` and the parsed `body` redacted; the same for the error `killSandbox` surfaces. `browser.isConnected()` ran outside `checkedPage`'s try, so a browser proxy whose liveness accessor throws escaped as a plain `Error` instead of `browser_unusable`. It is read inside the try and branched on after. Also: the health poll no longer issues a final request with no budget left, whose abort used to overwrite the proxy's own answer — the one useful thing in `relay_not_ready`'s message — with "The operation timed out". The CHANGELOG bullets Sol called overstated now say what is true. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 50 ++++++++----- README.md | 7 +- src/core/handoff.test.ts | 94 ++++++++++++++++++++++++ src/core/raise-hand.ts | 17 ++++- src/errors.ts | 7 +- src/logger.test.ts | 91 +++++++++++++++++++++++ src/logger.ts | 17 ++++- src/relay/deploy.test.ts | 143 +++++++++++++++++++++++++++++++++++- src/relay/deploy.ts | 154 +++++++++++++++++++++++++++++++++------ 9 files changed, 527 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ae7021..b3f4ddc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,29 +13,43 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 Everything `raiseHand` throws now carries a `code` you can branch on — `missing_api_key`, `invalid_mode`, `empty_action`, `browser_unusable`, `relay_start_failed`, `concurrency_limit`, `relay_not_ready` — plus the - original SDK, CDP or network error as `cause`. `concurrency_limit` is the + SDK, CDP or network error as `cause`, with any credential in it redacted. `concurrency_limit` is the one worth retrying: it means your Solari account is at its concurrent session cap, not that anything is broken. When to expect each code, and what to do about it, is in the README's [Errors](README.md#errors) table. The messages were never a contract; they can still be reworded in any release. Outcomes are unchanged and still values: a human who never came, a session that died mid-handoff and a webhook that 500s are not exceptions. -- **A logger that throws can no longer end a handoff.** `logger` is your - object — a pino instance over a closed transport throws — and handraise - calls it from `catch` blocks and promise callbacks. One of those was the - webhook notification, which `raiseHand` fires and only awaits minutes later: - a throw there was an unhandled rejection (node ends the process for that) - and then an uncoded `Error` out of `raiseHand`, long after the URL existed. - Log calls are now wrapped where the logger enters handraise. A broken logger - costs a log line. +- **A broken logger can no longer end a handoff.** `logger` is your object, + and handraise calls it from `catch` blocks and promise callbacks. Three ways + it breaks are contained where the logger enters handraise: a method that + throws (a pino instance over a closed transport), a method that is a getter + and throws on the property read, and a method that is `async` and rejects — + TypeScript accepts one where `Logger` declares `void`, and the rejection then + belongs to a promise nobody holds, which ends the process. One of the call + sites is the webhook notification, which `raiseHand` fires and only awaits + minutes later, long after the handoff URL exists. A broken logger costs a log + line. - **The relay health poll enforces its deadline.** Each attempt carries `AbortSignal.timeout`, so a preview URL that accepts the connection and never answers ends as `relay_not_ready` at the deadline instead of blocking - `raiseHand` for minutes with a live sandbox burning its idle window. -- **No preview token can reach an error message.** Anything a gateway or proxy - says is redacted before it is quoted — `pt_token=…` in any case or - separator, percent-encoded inside a `?next=` parameter, or the bare - credential in prose. + `raiseHand` for minutes with a live sandbox burning its idle window. The + "Last answer" in that message is now the URL's own — a 401 from the preview + proxy, a refused connection — instead of the abort of a final request that + had no time left to make. +- **The preview token is redacted out of error messages and out of `cause`.** + It is a live bearer credential for the relay, and a proxy that echoes the + request URI in its 401 body would otherwise put it in an exception message. + Where the exact value is known — the health poll is holding the URL that + carries it — that value and its percent-encoded form are removed by + comparison, whatever syntax the proxy wrapped them in. Three patterns are the + net for foreign text where it is not known: `pt_token=…` in any case or + separator, a `pt_`-prefixed value, and the JWT shape the preview token + actually has (three base64url segments — see + `docs/measurements/01-preview-transport.md` §3). The SDK error attached as + `cause` is redacted the same way, because every error serialiser prints the + whole chain. + ### Changed - **A page that is already dead is now refused instead of handed off.** @@ -51,9 +65,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 wrapped in a `HandraiseError`, with the SDK's error kept as `error.cause`. Branch on `error.code === "concurrency_limit"`; if you must have the class, it is `error.cause`, and `error.cause.status === 429` is the check that - survives a second copy of `@solarisdk/core` in your tree. Apart from the - page check above, nothing throws that did not throw before, and no outcome - became an exception. + survives a second copy of `@solarisdk/core` in your tree. `cause` is the SDK's + error with credentials redacted: same class, same `name`, `status` and `code`, + with `message` and the parsed `body` rewritten. Apart from the page check + above, nothing throws that did not throw before, and no outcome became an + exception. ## [0.5.1] - 2026-09-02 diff --git a/README.md b/README.md index 7d360e8..c03abe4 100644 --- a/README.md +++ b/README.md @@ -296,7 +296,10 @@ asked for anything yet. Everything after that is an `outcome`, never an exception. What it throws is a `HandraiseError` with a `code`: the code is the contract, the message is for whoever reads the log and may be reworded in any release. `isHandraiseError` narrows a `catch` binding, and `cause` keeps the -original SDK, CDP or network error whenever there was one. +original SDK, CDP or network error whenever there was one — the same class, +`name`, `status` and `code`, with credentials redacted out of its `message` and +its response body. Every error serialiser prints the whole chain, so a clean +outer message on its own would not be worth much. The first thing `raiseHand` does is look at your page, before it creates anything: a page you have closed, or a browser you have disconnected, is @@ -323,7 +326,7 @@ try { | `invalid_mode` | `mode` is neither `"takeover"` nor `"approval"`. | Fix the call. TypeScript already refuses it; this is for JavaScript callers. | | `empty_action` | `mode: "approval"` without a non-empty `action`. | Name the step the human says yes or no to. | | `browser_unusable` | The page is closed, or its browser has disconnected — checked before anything is created. | Open a new page or relaunch the session (restore `storageState` if you kept it) and retry. | -| `relay_start_failed` | The relay sandbox could not be created or deployed. | Read `cause` — it is the Solari SDK's own error. Retry. Nothing is left behind unless you also see `relay_release_failed` (below). | +| `relay_start_failed` | The relay sandbox could not be created or deployed. | Read `cause` — it is the Solari SDK's own error, redacted. Retry. Nothing is left behind unless you also see `relay_release_failed` (below). | | `concurrency_limit` | Your Solari account is at its concurrent session cap (429). | Free a session, or wait and retry. The one relay failure that is purely temporary. | | `relay_not_ready` | The sandbox started but its public URL never answered. | Retry. Persisting means the preview proxy or the region is unhealthy. | diff --git a/src/core/handoff.test.ts b/src/core/handoff.test.ts index 99651e7..feac09a 100644 --- a/src/core/handoff.test.ts +++ b/src/core/handoff.test.ts @@ -688,6 +688,34 @@ test("a page whose state cannot be read at all is refused too", async () => { }) }) +test("a browser whose liveness accessor throws is refused too", async () => { + // The last unguarded read in the pre-flight check: a browser proxy — a + // remote-CDP wrapper, a pooled session object, a page handed over between + // processes — whose `isConnected()` throws instead of answering. Outside the + // `try` that would leave `raiseHand` rejecting with a plain `Error`, which + // is exactly what typed codes exist to stop. + const browserPartial: Partial = { + isConnected: () => { + throw new Error("Browser has been closed") + }, + } + const contextPartial: Partial = { + // SAFETY: the guard reads only `browser()` off the context. + browser: () => browserPartial as Browser, + } + const pagePartial: Partial = { + isClosed: () => false, + // SAFETY: the guard reads only `context().browser()` on the page. + context: () => contextPartial as BrowserContext, + } + + // SAFETY: the guard touches `isClosed` and `context` and nothing else. + await expect(askOn(pagePartial as Page)).rejects.toMatchObject({ + name: "HandraiseError", + code: "browser_unusable", + }) +}) + test("an open page whose browser has disconnected is refused too", async () => { // The Solari session hit its ~10-minute hard lifetime while the agent was // still working. The page is not closed and `context()` answers — only the @@ -1520,3 +1548,69 @@ test("a logger that throws does not break the handoff", async () => { // take `onEvent` with it. expect(events).toHaveLength(1) }) + +test("a logger whose methods reject does not break the handoff either", async () => { + // The same option, one shape further out: `debug(event, fields): void` + // accepts an `async` implementation, so the failure arrives as a rejected + // promise nobody holds rather than as a throw. Unhandled, that ends the + // agent's process mid-handoff — before the relay sandbox is released, which + // leaves a public URL and its last frame reachable until the idle timeout. + // + // `bun test` fails a test that leaves an unhandled rejection behind, which + // is the red signal this was written against; the listener states the same + // assertion in the test itself. + const port = await startRelayProcess() + const human = await connectHuman(port) + const cdp = fakeCdp() + const unhandled: string[] = [] + const record = (cause: unknown): void => { + unhandled.push(String(cause)) + } + process.on("unhandledRejection", record) + try { + let calls = 0 + const down = async (): Promise => { + calls += 1 + throw new Error("log shipper is gone (async)") + } + const rejecting: Logger = { + debug: down, + info: down, + warn: down, + error: down, + } + const events: HandoffEvent[] = [] + + const handoff = runHandoff({ + page: fakePage(cdp.cdp), + agentWsUrl: `ws://127.0.0.1:${port}/ws?role=agent`, + options: { + reason: "the logger ships its lines over a socket that went away", + logger: rejecting, + onEvent: (event) => events.push(event), + }, + timeoutMs: 5000, + url: "https://relay.example/?pt_token=x", + handoffId: "async-rejecting-logger", + relayColdStartMs: 5, + logger: rejecting, + }) + + await until("the phone to connect", () => human.inbox.length >= 0) + human.send({ type: "handback" }) + + const end = await handoff + expect(end.outcome).toBe("resolved") + // The wide event still reaches the caller, and the logger was really + // called — a containment that stopped logging would pass vacuously. + expect(events).toHaveLength(1) + expect(calls).toBeGreaterThan(0) + + // Long enough for the loop turn on which an unhandled rejection is + // reported, after the handoff has fully torn down. + await Bun.sleep(50) + expect(unhandled).toEqual([]) + } finally { + process.off("unhandledRejection", record) + } +}) diff --git a/src/core/raise-hand.ts b/src/core/raise-hand.ts index 5bd0dce..9556739 100644 --- a/src/core/raise-hand.ts +++ b/src/core/raise-hand.ts @@ -632,17 +632,26 @@ function checkedMode(options: RaiseHandOptions): HandoffMode { * connected? `context()` is a field read and throws nothing in Playwright, so * it is `isClosed()` that catches a closed page; the try/catch is for the page * object that is not a working Playwright page at all. + * + * Every read is inside the `try` and every branch after it. A browser proxy — + * a remote-CDP wrapper, a pooled session, a page handed between processes — + * can throw from its liveness accessor too, and a plain `Error` out of the + * guard whose whole job is to produce `browser_unusable` would be the last + * uncoded rejection on this path. */ function checkedPage(page: Page): void { let closed: boolean - let browser: Browser | null + let connected: boolean try { closed = page.isClosed() - browser = page.context().browser() + const browser: Browser | null = page.context().browser() + // A context with no browser is a persistent context: there is no session + // object to ask, and `isClosed()` above has already spoken for the page. + connected = browser?.isConnected() ?? true } catch (cause) { throw new HandraiseError( "browser_unusable", - `handraise: this page cannot be handed to a human — reading its state (page.isClosed(), page.context()) threw. A dead CDP connection does that, and so does a page-like object that is not a Playwright page. ${String(cause)}`, + `handraise: this page cannot be handed to a human — reading its state (page.isClosed(), page.context().browser().isConnected()) threw. A dead CDP connection does that, and so does a page-like object that is not a Playwright page. ${String(cause)}`, { cause }, ) } @@ -652,7 +661,7 @@ function checkedPage(page: Page): void { "handraise: this page is already closed, so there is nothing for a human to take over. Open a new page (its `storageState` from an earlier handoff, if you kept it, restores the human's work) and retry.", ) } - if (browser && !browser.isConnected()) { + if (!connected) { throw new HandraiseError( "browser_unusable", "handraise: the browser session behind this page is already disconnected, so there is nothing for a human to take over. Relaunch the session (its `storageState` from an earlier handoff, if you kept it, restores the human's work) and retry.", diff --git a/src/errors.ts b/src/errors.ts index 1eba643..6d16b71 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -31,7 +31,7 @@ * session that has died server-side while the CDP socket is still up looks * alive here and still ends as the `disconnected` outcome. * - `relay_start_failed` — the relay sandbox could not be created or deployed; - * `cause` holds the SDK error. + * `cause` holds the SDK error, with credentials redacted. * - `concurrency_limit` — the Solari account is at its concurrent session cap * (HTTP 429). The one relay failure that is worth retrying later. * - `relay_not_ready` — the sandbox started but its public URL never answered. @@ -52,7 +52,10 @@ export type HandraiseErrorCode = /** * Everything handraise throws on purpose. `cause` carries the original SDK, - * CDP or network error whenever there was one, so the wrapping hides nothing. + * CDP or network error whenever there was one — same class, same `name`, + * `status` and `code` — so the wrapping hides nothing. Its `message` and its + * response body are redacted, because the relay's preview token is a live + * bearer credential and every error serialiser prints the whole chain. */ export class HandraiseError extends Error { override readonly name = "HandraiseError" diff --git a/src/logger.test.ts b/src/logger.test.ts index c43b74c..7bf0b61 100644 --- a/src/logger.test.ts +++ b/src/logger.test.ts @@ -13,6 +13,7 @@ import { type Logger, noopLogger, quietLogger, + safeLogger, } from "./logger" /** The console methods, captured so a test can restore them. */ @@ -117,3 +118,93 @@ test("quietLogger drops debug/info but forwards warn/error — the library defau expect(err[0]?.parsed.event).toBe("w") expect(err[1]?.parsed.event).toBe("e") }) + +// --- safeLogger ---------------------------------------------------------- +// +// `Logger` is the caller's object, and the two ways it breaks that a plain +// `try` does not cover are a method that is `async` — TypeScript accepts one +// where the interface declares `void` — and a property that is a getter. + +test("safeLogger contains a logger whose methods reject", async () => { + // The gap a `try` cannot see: `debug(event, fields): void` accepts an + // `async` implementation, so the throw happens after `safeLogger` has + // already returned. The rejection then belongs to a promise nobody holds, + // and the runtime ends the process for that — mid-handoff, before the relay + // sandbox is released. `bun test` fails a test that leaves one behind, so + // the red signal here is this test failing with "log shipper is gone". + // + // Deliberately not inside `expect(...).not.toThrow()`: that wrapper marks + // rejections raised during the call as handled, which would hide exactly + // what is under test. + let calls = 0 + const down = async (): Promise => { + calls += 1 + throw new Error("log shipper is gone (async)") + } + const rejecting: Logger = { + debug: down, + info: down, + warn: down, + error: down, + } + const safe = safeLogger(rejecting) + + safe.debug("d", { a: 1 }) + safe.info("i") + safe.warn("w") + safe.error("e") + + // Long enough for the microtask queue to settle and for the loop turn on + // which an unhandled rejection is reported. + await Bun.sleep(50) + // The wrapper still calls the logger — containment is not silence. + expect(calls).toBe(4) +}) + +test("safeLogger survives a logger whose method is a throwing getter", () => { + // A proxy over a closed transport, or a class that builds its methods + // lazily: the throw happens on the property read, before any call. + const exploding = (): never => { + throw new Error("the sink was torn down") + } + const brokenGetters: Logger = { + get debug(): never { + return exploding() + }, + get info(): never { + return exploding() + }, + get warn(): never { + return exploding() + }, + get error(): never { + return exploding() + }, + } + const safe = safeLogger(brokenGetters) + + expect(() => { + safe.debug("d") + safe.info("i") + safe.warn("w") + safe.error("e") + }).not.toThrow() +}) + +test("safeLogger still forwards to a working logger", () => { + // The containment above may not turn the wrapper into a second noopLogger. + const seen: string[] = [] + const inner: Logger = { + debug: (event) => seen.push(`debug:${event}`), + info: (event) => seen.push(`info:${event}`), + warn: (event) => seen.push(`warn:${event}`), + error: (event) => seen.push(`error:${event}`), + } + const safe = safeLogger(inner) + safe.debug("d") + safe.info("i") + safe.warn("w") + safe.error("e") + + expect(seen).toEqual(["debug:d", "info:i", "warn:w", "error:e"]) +}) diff --git a/src/logger.ts b/src/logger.ts index c8591f3..3bba29d 100644 --- a/src/logger.ts +++ b/src/logger.ts @@ -86,10 +86,11 @@ export const quietLogger: Logger = { } /** - * Wrap a logger so a throw from it can never end a handoff. + * Wrap a logger so a failure in it can never end a handoff. * * `Logger` is the caller's object: a pino instance over a closed transport, a - * socket that went away, a sink that decided a field was unserialisable. Most + * socket that went away, a sink that decided a field was unserialisable, a + * shipper whose methods are `async` and reject. Most * of handraise's log calls sit inside a `catch` or a promise callback on the * failure path, where a throw would either escape `raiseHand` after a human * has already been shown the URL or reject a promise nobody is awaiting — and @@ -103,7 +104,17 @@ export const quietLogger: Logger = { export function safeLogger(inner: Logger): Logger { const swallow = (write: () => void): void => { try { - write() + // Two throws to contain, not one. `write()` covers the synchronous + // throw *and* the throwing property getter, because the method is read + // inside this call. The declared return type is `void`, but TypeScript + // accepts an `async` method there, so what actually comes back may be a + // promise — one nobody holds, whose rejection ends the process. The + // handler below is attached in the same tick it is created, so the + // rejection is already spoken for before the runtime looks. + const result = write() + // `Promise.resolve` adopts a promise and wraps anything else, so a + // logger that returns `undefined` costs one resolved microtask. + Promise.resolve(result).catch(() => undefined) } catch { // Nothing to report this with — the reporter is what broke. } diff --git a/src/relay/deploy.test.ts b/src/relay/deploy.test.ts index 719cc10..d365184 100644 --- a/src/relay/deploy.test.ts +++ b/src/relay/deploy.test.ts @@ -31,8 +31,18 @@ import { /** A port on which nothing listens, so no test here can reach a live gateway. */ const CLOSED_PORT = "http://127.0.0.1:1" -/** A token shaped like the real thing, long enough to be unmistakable in a diff. */ -const FAKE_TOKEN = `pt_${"a1b2c3d4".repeat(8)}` +/** + * A token shaped like the real thing: the preview credential is a ~362-char + * JWT — three base64url segments — and not a `pt_`-prefixed opaque string + * (docs/measurements/01-preview-transport.md §3). A fixture with the wrong + * grammar is why the redaction looked covered while a real token walked + * through it. + */ +const FAKE_TOKEN = [ + "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9", + "eyJzYW5kYm94SWQiOiJzYngtZmFrZSIsInBvcnQiOjMwMDAsIm9yZ0lkIjoib3JnLWZha2UiLCJleHAiOjIwMDAwMDAwMDAsIm5vdGUiOiJub3QtYS1yZWFsLWNyZWRlbnRpYWwifQ", + "c2lnbmF0dXJlLW9mLWEtZml4dHVyZS1ub3QtYS1yZWFsLWNyZWRlbnRpYWw", +].join(".") /** * The code of the `HandraiseError` `run` rejects with, or a sentence saying @@ -66,6 +76,38 @@ async function causeNameOf(run: Promise): Promise { } } +/** + * The gateway's own words as they survive on `cause`: its status, its message + * and its parsed body. All three are read by anything that prints an error + * chain — `console.error(error)`, pino's error serialiser, a crash reporter — + * so a credential in any of them is a credential in the log. + */ +async function gatewayCauseOf(run: Promise): Promise<{ + status: number + message: string + body: string +}> { + const missing = { status: 0, body: "" } + try { + await run + return { ...missing, message: "nothing was thrown" } + } catch (error) { + if (!isHandraiseError(error)) + return { ...missing, message: `not a HandraiseError: ${String(error)}` } + const cause = error.cause + if (!(cause instanceof GatewayError)) + return { + ...missing, + message: `the cause is not a GatewayError: ${String(cause)}`, + } + return { + status: cause.status, + message: cause.message, + body: JSON.stringify(cause.body), + } + } +} + /** The message `run` rejects with, whatever class it is. */ async function messageOf(run: Promise): Promise { try { @@ -142,6 +184,17 @@ test("a gateway at its session cap becomes concurrency_limit, not a raw 429", as const message = await messageOf(creating) expect(message).toContain("concurrent session limit") expect(message).not.toContain(FAKE_TOKEN) + + // …and the same words on the `cause`, which is where a clean outer message + // stops helping: printing an error prints its chain. + const cause = await gatewayCauseOf(creating) + expect(cause.message).not.toContain(FAKE_TOKEN) + expect(cause.body).not.toContain(FAKE_TOKEN) + // What a caller branches on survives the redaction: the status, the + // gateway's code, and enough of the sentence to read. + expect(cause.status).toBe(429) + expect(cause.body).toContain("ConcurrencyLimitExceeded") + expect(cause.message).toContain("Concurrency limit exceeded") } finally { // In a `finally`: a failed expectation above must not leave a listening // socket behind for the rest of the run. @@ -231,6 +284,57 @@ test("a proxy that echoes the request URI cannot leak the preview token", async } }) +/** + * A preview edge that quotes the credential the way the real one does on a bad + * token — bare in prose — plus the `%3D` form a redirect hint produces. Both + * are what a `pt_`-shaped rule cannot see once the token is the JWT it really + * is. + */ +async function startTokenQuotingProxy(): Promise<{ + url: string + server: Server +}> { + const server = createServer((request, response) => { + const query = new URL(request.url ?? "/", "http://127.0.0.1").searchParams + const token = query.get("pt_token") ?? "" + response.writeHead(401, { "content-type": "text/plain" }) + response.end( + `invalid preview token ${token}. Present it as ?pt_token%3D${token}`, + ) + }) + await new Promise((resolve) => { + server.listen(0, "127.0.0.1", () => resolve()) + }) + // SAFETY: as above — a TCP listener never has a string address. + const { port } = server.address() as AddressInfo + return { url: `http://127.0.0.1:${port}`, server } +} + +test("a proxy that quotes the token bare cannot leak it either", async () => { + // The health URL carries the credential, so handraise knows its exact value + // and does not have to guess its grammar. This is the case the guess got + // wrong: a real preview token is a JWT, and neither `pt_token=` nor a + // `pt_`-prefixed value appears in either sentence below. + const proxy = await startTokenQuotingProxy() + try { + const waiting = waitForHealth( + `${proxy.url}/healthz?pt_token=${FAKE_TOKEN}&role=human`, + 1500, + ) + + expect(await codeOf(waiting)).toBe("relay_not_ready") + const message = await messageOf(waiting) + expect(message).not.toContain(FAKE_TOKEN) + // Not even a segment of it: a JWT's payload alone decodes to the sandbox + // and org it was minted for. + for (const segment of FAKE_TOKEN.split(".")) + expect(message).not.toContain(segment) + expect(message).toContain("HTTP 401") + } finally { + proxy.server.close() + } +}, 15_000) + test("redaction survives every form a token arrives in", () => { // The forms a proxy body actually produces. Every one of these was a leak // before the second rule; the plain `pt_token=` case never was, which is @@ -244,7 +348,11 @@ test("redaction survives every form a token arrives in", () => { // Percent-encoded inside a redirect parameter — a 302/401 default. `?next=%2Fhealthz%3Fpt_token%3D${FAKE_TOKEN}`, `%2Fhealthz%3Fpt_token%3D${FAKE_TOKEN}`, + // The `%3D` on its own: the parameter rule needs a literal `=` or `:` + // between the name and the value and cannot see this one. + `pt_token%3D${FAKE_TOKEN}`, // An auth proxy quoting the credential in prose, with no parameter at all. + // This is what the preview edge actually says on a bad token. `invalid preview token ${FAKE_TOKEN}`, // An uppercased parameter name, an HTML-entity `=`, and a stray space. `?PT_TOKEN=${FAKE_TOKEN}`, @@ -262,12 +370,41 @@ test("redaction survives every form a token arrives in", () => { "?pt_token=[redacted]&role=human", ) expect(redactPreviewToken(`invalid preview token ${FAKE_TOKEN}`)).toBe( - "invalid preview token pt_[redacted]", + "invalid preview token [redacted]", ) // Nothing else is touched. expect(redactPreviewToken("HTTP 502 upstream closed")).toBe( "HTTP 502 upstream closed", ) + expect( + redactPreviewToken("sandbox sbx-9f2c.preview.getsolari.com refused"), + ).toBe("sandbox sbx-9f2c.preview.getsolari.com refused") +}) + +test("the exact token is redacted even in a form no rule anticipated", () => { + // The generic rules are a net for text handraise never saw the token in. + // Where it *is* known — the health URL carries it — the credential is + // matched by value, so a proxy inventing a new way to quote it changes + // nothing. Two `pt_token` values would be a bug, so the leak is deliberately + // in a shape neither rule matches: reversed segment order, no parameter. + const shredded = FAKE_TOKEN.split(".").reverse().join("~") + + expect(redactPreviewToken(`upstream said: ${shredded}`)).toContain(shredded) + expect( + redactPreviewToken(`upstream said: ${shredded}`, shredded), + ).not.toContain(shredded) + // And the percent-encoded form of the same value, which is what a redirect + // parameter carries. A base64 token — one with `+`, `/` and `=` in it — + // does not survive `encodeURIComponent` unchanged, so both forms are needed. + const padded = "a+b/c=d+e/f=g+h/i=j" + const encoded = encodeURIComponent(padded) + expect(redactPreviewToken(`?next=${encoded}`, padded)).not.toContain(encoded) + + // A short value is not a credential, and blanking every occurrence of one + // would shred the message rather than redact it. + expect(redactPreviewToken("port 3000 refused", "3000")).toBe( + "port 3000 refused", + ) }) /** diff --git a/src/relay/deploy.ts b/src/relay/deploy.ts index cb8007a..46d2c1b 100644 --- a/src/relay/deploy.ts +++ b/src/relay/deploy.ts @@ -111,13 +111,29 @@ function withToken(previewUrl: string, token: string | undefined): string { const TOKEN_PARAM = /pt_token\s*[=:]\s*[^&;\s"'<>]+/gi /** - * The credential itself, wherever it appears — bare in prose ("invalid preview - * token pt_…"), or behind a `%3D` that the parameter rule above cannot see. + * A `pt_`-prefixed value, wherever it appears. Kept for the identifiers that + * do carry that prefix; it is *not* the preview credential's shape. * Deliberately without `\b`: a percent-encoded `=` ends in a word character, * so a word boundary would not match there. */ const TOKEN_VALUE = /pt_[A-Za-z0-9._~-]{16,}/g +/** + * The credential's real grammar: three base64url segments separated by dots. + * The preview token is a ~362-character JWT + * (docs/measurements/01-preview-transport.md §3), so this is the rule that + * catches it bare in prose — "invalid preview token eyJhbGci…" — or behind a + * `%3D` neither rule above can see. + */ +const TOKEN_JWT = /[A-Za-z0-9_-]{20,}\.[A-Za-z0-9_-]{20,}\.[A-Za-z0-9_-]{20,}/g + +/** + * Short enough to be an accident rather than a credential. Blanking every + * occurrence of a two-character `token` would shred the message instead of + * redacting it, and an empty one would match everywhere. + */ +const MIN_TOKEN_LENGTH = 16 + /** * Take the preview token out of anything that becomes an error message. * @@ -125,14 +141,92 @@ const TOKEN_VALUE = /pt_[A-Za-z0-9._~-]{16,}/g * for the relay. A gateway or proxy that echoes the request URI in its error * body — a common default on 401 and 404, and common percent-encoded inside a * `?next=` parameter — would otherwise put that token into an exception - * message, and exception messages end up in log aggregators. Two rules, - * because the syntax around the credential varies and the credential does - * not. Exported for `deploy.test.ts`. + * message, and exception messages end up in log aggregators. + * + * Pass `token` wherever the exact credential is known (it is, everywhere the + * URL is at hand): that value and its percent-encoded form are removed by + * comparison, which no proxy can outrun by inventing another way to quote it. + * The three patterns are the net for the text where it is not known — the + * credential's shape guessed from the outside, which is exactly the guess that + * once let a real token through. Exported for `deploy.test.ts`. */ -export function redactPreviewToken(text: string): string { - return text +export function redactPreviewToken(text: string, token?: string): string { + let redacted = text + if (token !== undefined && token.length >= MIN_TOKEN_LENGTH) { + // A Set because a token made only of unreserved characters — a JWT is — + // encodes to itself, and replacing it twice would be busywork. + for (const form of new Set([token, encodeURIComponent(token)])) + redacted = redacted.replaceAll(form, "[redacted]") + } + return redacted .replace(TOKEN_PARAM, "pt_token=[redacted]") .replace(TOKEN_VALUE, "pt_[redacted]") + .replace(TOKEN_JWT, "[redacted]") +} + +/** + * The credential this URL carries, so it can be redacted by value instead of + * by grammar. Returns nothing for a string that is not a URL: there is simply + * no known token then, and the patterns above still apply. + */ +function previewTokenOf(url: string): string | undefined { + try { + return new URL(url).searchParams.get("pt_token") ?? undefined + } catch { + return undefined + } +} + +/** + * The gateway's parsed error body. Named through the class that carries it + * because the SDK does not export the type on its own. + */ +type GatewayErrorBody = NonNullable + +/** A `GatewayError` seen through the one field made of the gateway's own words. */ +interface WithBody { + body?: GatewayErrorBody +} + +/** + * The same body with every string in it redacted, shape unchanged. + * + * Through JSON rather than field by field: `code`, `error` and `message` are + * what the type declares today, and the field a future gateway release puts + * the request URI in is the one worth covering in advance. + */ +function redactedBody(body: GatewayErrorBody): GatewayErrorBody { + // SAFETY: this re-parses text serialised one call earlier; redaction only + // ever replaces a run of characters inside a JSON string value, so the + // document is still the same shape. + return JSON.parse( + redactPreviewToken(JSON.stringify(body)), + ) as GatewayErrorBody +} + +/** + * A copy of an SDK error with the credential out of everything the gateway + * wrote. + * + * `cause` exists so a caller keeps the original — `cause instanceof + * ConcurrencyLimitError`, `cause.status === 429` — so the copy keeps the + * prototype and every own field, and rewrites only the two made of foreign + * text: `message` and the parsed `body`. Without this, a clean outer message + * buys nothing: `console.error(error)`, pino's error serialiser and every + * crash reporter print the whole chain. + */ +function redactedCause(error: Error): Error { + // SAFETY: `Object.create` returns a new object with `error`'s own prototype, + // so it is an instance of the same class; its own fields are copied below. + const clone = Object.create(Object.getPrototypeOf(error)) as Error & WithBody + // Own enumerable fields — `name`, and on a `GatewayError` `status`, `code` + // and `body`. `message` and `stack` are own but not enumerable, which is why + // they are the two lines after it. + Object.assign(clone, error) + clone.message = redactPreviewToken(error.message) + if (error.stack !== undefined) clone.stack = redactPreviewToken(error.stack) + if (clone.body !== undefined) clone.body = redactedBody(clone.body) + return clone } /** @@ -150,7 +244,7 @@ function relayStartError(cause: unknown): HandraiseError { redactPreviewToken( `handraise: your Solari account is at its concurrent session limit, so the relay sandbox that gives the handoff its public URL could not be created. Free a session and retry. (${cause.message})`, ), - { cause }, + { cause: redactedCause(cause) }, ) } return new HandraiseError( @@ -158,7 +252,7 @@ function relayStartError(cause: unknown): HandraiseError { redactPreviewToken( `handraise: the relay sandbox could not be started, so the handoff has no public URL and nobody has been asked for anything yet. ${String(cause)}`, ), - { cause }, + { cause: cause instanceof Error ? redactedCause(cause) : cause }, ) } @@ -226,7 +320,11 @@ export async function killSandbox( redactPreviewToken( `handraise: could not destroy the relay sandbox after ${budget} attempts; its public URL stays reachable until the idle timeout. Last error: ${String(lastError)}`, ), - { cause: lastError }, + // The SDK's error, with the gateway's own words redacted — `cause` is + // printed by every error serialiser that exists. + { + cause: lastError instanceof Error ? redactedCause(lastError) : lastError, + }, ) } @@ -240,12 +338,24 @@ export async function waitForHealth( timeoutMs: number = READY_TIMEOUT_MS, ): Promise { const deadline = Date.now() + timeoutMs + // The one place the credential is known exactly: it is in the URL being + // polled. Everything foreign that reaches `lastAnswer` is redacted against + // that value, so the redaction does not depend on guessing the token's + // shape — the guess that let a real one through. + const token = previewTokenOf(healthUrl) // A deadline has no single cause, so what the URL last said is carried in // the message: "connection refused" and "502 from the preview proxy" are - // different problems with the same code. The loop always writes it before it - // checks the deadline; the initial value only satisfies definite assignment. + // different problems with the same code. The first attempt always writes it + // and only a failure leaves the loop, so the throw at the end always has a + // real one; the initial value below satisfies definite assignment. let lastAnswer = "" - for (;;) { + for (let attempt = 1; ; attempt++) { + const remaining = deadline - Date.now() + // The first attempt always runs: "did not answer" about a URL nobody asked + // would be a lie. Every attempt after it needs budget left, because a + // request that can only abort would replace the proxy's own answer — the + // one useful thing in the message — with "The operation timed out". + if (remaining <= 0 && attempt > 1) break try { const response = await fetch(healthUrl, { cache: "no-store", @@ -255,7 +365,7 @@ export async function waitForHealth( // hold the loop open for minutes with a live sandbox burning its idle // window. The abort lands in the catch below and the deadline check // ends the loop. - signal: AbortSignal.timeout(Math.max(1, deadline - Date.now())), + signal: AbortSignal.timeout(Math.max(1, remaining)), }) const body = await response.text() if (response.ok && body === "ok") return @@ -263,19 +373,19 @@ export async function waitForHealth( // the request URI would otherwise quote the live `pt_token` back at us. // Redacted before it is cut, so no slice can leave a partial credential // without its prefix. - lastAnswer = `HTTP ${response.status} ${redactPreviewToken(body).slice(0, 80)}` + lastAnswer = `HTTP ${response.status} ${redactPreviewToken(body, token).slice(0, 80)}` } catch (error) { // The port is not routable yet; the retry below is the whole mechanism. - lastAnswer = redactPreviewToken(String(error)) - } - if (Date.now() >= deadline) { - throw new HandraiseError( - "relay_not_ready", - `handraise: the relay sandbox started but its public URL did not answer within ${timeoutMs}ms, so the handoff page would not have loaded on the phone. Last answer: ${lastAnswer}`, - ) + lastAnswer = redactPreviewToken(String(error), token) } + if (Date.now() >= deadline) break await sleep(READY_POLL_MS) } + // The only way out of the loop: success returns from inside it. + throw new HandraiseError( + "relay_not_ready", + `handraise: the relay sandbox started but its public URL did not answer within ${timeoutMs}ms, so the handoff page would not have loaded on the phone. Last answer: ${lastAnswer}`, + ) } /** From 452ddd32077a5980b6d8b5b2384b7cccd96771d4 Mon Sep 17 00:00:00 2001 From: Sy-D <8460326+Sy-D@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:33:16 +0200 Subject: [PATCH 2/2] fix: copy the whole error when redacting a cause, and see through encoded dots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 on the verification pass of PR #7. The clone was built with `Object.assign`, which copies own *enumerable* properties only. `new Error(msg, { cause })` installs `cause` non-enumerable, so an error chain was truncated exactly where the root reason lives — undici's `TypeError: fetch failed` is that shape. The copy now starts from `Object.getOwnPropertyDescriptors`, so every own property survives with its descriptor, and `message`/`stack`/`body` are redefined rather than assigned, which keeps them off `JSON.stringify(cause)` as they are on a real Error. The nested `cause` is redacted recursively, bounded at eight links and cycle-safe. Reading a foreign error is running foreign code, and this all happens inside the `catch` that exists to produce a coded error. Descriptors are copied rather than read, so an accessor is never invoked; building the message and copying the error are both wrapped, so a throwing `message` getter or a body that references itself now yields a plain redacted `Error` instead of a raw `TypeError`. The exact-value redaction was wired to the health poll alone. `startRelay` holds the preview URL from the moment the sandbox answers, so it now passes that credential to `relayStartError` and to `killSandbox` as well. Both nets keyed on the JWT's two literal dots, which any escaping proxy removes: the pattern accepts `%2E`, `%2e` and `.` as separators, and the exact-value comparison covers the same forms. The `unhandledRejection` listener in the handoff test is inert under bun — the runner claims the rejection first. The comment now says so instead of implying the assertion is the gate. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 34 +++--- README.md | 7 +- src/core/handoff.test.ts | 12 ++- src/errors.ts | 7 +- src/relay/deploy.test.ts | 195 ++++++++++++++++++++++++++++++++++ src/relay/deploy.ts | 220 ++++++++++++++++++++++++++++++++------- 6 files changed, 414 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3f4ddc..8c23a17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,15 +40,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **The preview token is redacted out of error messages and out of `cause`.** It is a live bearer credential for the relay, and a proxy that echoes the request URI in its 401 body would otherwise put it in an exception message. - Where the exact value is known — the health poll is holding the URL that - carries it — that value and its percent-encoded form are removed by - comparison, whatever syntax the proxy wrapped them in. Three patterns are the - net for foreign text where it is not known: `pt_token=…` in any case or - separator, a `pt_`-prefixed value, and the JWT shape the preview token - actually has (three base64url segments — see - `docs/measurements/01-preview-transport.md` §3). The SDK error attached as - `cause` is redacted the same way, because every error serialiser prints the - whole chain. + Where the exact value is known — the health poll, the teardown failure and + the wrapped start failure all hold the URL that carries it — that value is + removed by comparison in each of the forms an escaping proxy produces: bare, + percent-encoded, and with its dots written `%2E`, `%2e` or `.`. Three + patterns are the net for foreign text where the value is not known: + `pt_token=…` in any case or separator, a `pt_`-prefixed value, and the JWT + shape the preview token actually has (three base64url segments, separator + literal or escaped — see `docs/measurements/01-preview-transport.md` §3). The + SDK error attached as `cause` goes through the same redaction, because every + error serialiser prints the whole chain. A proxy that invents an encoding + none of those cover — folding the value across lines, say — is still a leak; + this is a net, not a proof. ### Changed @@ -66,10 +69,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 Branch on `error.code === "concurrency_limit"`; if you must have the class, it is `error.cause`, and `error.cause.status === 429` is the check that survives a second copy of `@solarisdk/core` in your tree. `cause` is the SDK's - error with credentials redacted: same class, same `name`, `status` and `code`, - with `message` and the parsed `body` rewritten. Apart from the page check - above, nothing throws that did not throw before, and no outcome became an - exception. + error with credentials redacted: a copy carrying the same prototype and the + same property descriptors — so `name`, `status`, `code`, the non-enumerable + `message` and `stack`, and the `cause` chain hanging off it all survive, and + `JSON.stringify(cause)` still produces what it did — with `message`, `stack`, + the parsed `body` and every nested `cause` rewritten. An error that cannot be + copied without running its own code (a throwing getter, a body that + references itself) becomes a plain redacted `Error` rather than an exception. + Apart from the page check above, nothing throws that did not throw before, + and no outcome became an exception. ## [0.5.1] - 2026-09-02 diff --git a/README.md b/README.md index c03abe4..409e671 100644 --- a/README.md +++ b/README.md @@ -297,9 +297,10 @@ exception. What it throws is a `HandraiseError` with a `code`: the code is the contract, the message is for whoever reads the log and may be reworded in any release. `isHandraiseError` narrows a `catch` binding, and `cause` keeps the original SDK, CDP or network error whenever there was one — the same class, -`name`, `status` and `code`, with credentials redacted out of its `message` and -its response body. Every error serialiser prints the whole chain, so a clean -outer message on its own would not be worth much. +`name`, `status` and `code`, its own non-enumerable properties, and its own +`cause` chain — with credentials redacted out of every `message`, `stack` and +response body along it. Every error serialiser prints the whole chain, so a +clean outer message on its own would not be worth much. The first thing `raiseHand` does is look at your page, before it creates anything: a page you have closed, or a browser you have disconnected, is diff --git a/src/core/handoff.test.ts b/src/core/handoff.test.ts index feac09a..d00c582 100644 --- a/src/core/handoff.test.ts +++ b/src/core/handoff.test.ts @@ -1556,9 +1556,12 @@ test("a logger whose methods reject does not break the handoff either", async () // agent's process mid-handoff — before the relay sandbox is released, which // leaves a public URL and its last frame reachable until the idle timeout. // - // `bun test` fails a test that leaves an unhandled rejection behind, which - // is the red signal this was written against; the listener states the same - // assertion in the test itself. + // The gate here is the runner: `bun test` fails a test that leaves an + // unhandled rejection behind, which is how this was watched failing against + // the unfixed wrapper. The listener below is NOT that gate — bun claims the + // rejection first and never calls it, so `unhandled` stays empty either way. + // It is kept because it costs nothing and states the invariant for a runner + // that only warns; do not read it as the thing that catches a regression. const port = await startRelayProcess() const human = await connectHuman(port) const cdp = fakeCdp() @@ -1607,7 +1610,8 @@ test("a logger whose methods reject does not break the handoff either", async () expect(calls).toBeGreaterThan(0) // Long enough for the loop turn on which an unhandled rejection is - // reported, after the handoff has fully torn down. + // reported, after the handoff has fully torn down. Inert under bun — see + // the note above the listener. await Bun.sleep(50) expect(unhandled).toEqual([]) } finally { diff --git a/src/errors.ts b/src/errors.ts index 6d16b71..d63aee9 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -53,9 +53,10 @@ export type HandraiseErrorCode = /** * Everything handraise throws on purpose. `cause` carries the original SDK, * CDP or network error whenever there was one — same class, same `name`, - * `status` and `code` — so the wrapping hides nothing. Its `message` and its - * response body are redacted, because the relay's preview token is a live - * bearer credential and every error serialiser prints the whole chain. + * `status` and `code`, same own properties and same `cause` chain — so the + * wrapping hides nothing. Every `message`, `stack` and response body along + * that chain is redacted, because the relay's preview token is a live bearer + * credential and every error serialiser prints the whole chain. */ export class HandraiseError extends Error { override readonly name = "HandraiseError" diff --git a/src/relay/deploy.test.ts b/src/relay/deploy.test.ts index d365184..38b2d09 100644 --- a/src/relay/deploy.test.ts +++ b/src/relay/deploy.test.ts @@ -24,6 +24,7 @@ import { createSandbox, killSandbox, redactPreviewToken, + relayStartError, startRelay, waitForHealth, } from "./deploy" @@ -449,3 +450,197 @@ test("a public URL that accepts and never answers still hits the deadline", asyn hanging.stop() } }, 15_000) + +// --- The sanitized `cause` ------------------------------------------------ +// +// `cause` is documented as the original error with credentials redacted, and +// callers branch on it. Copying it is where fidelity and safety pull against +// each other: too little and the chain is truncated, too much and reading a +// foreign object throws out of a `catch` whose whole job is to produce a coded +// error. + +/** + * A parsed error body that references itself — what an HTTP client that + * attaches the response object to its error produces. + */ +interface CyclicBody { + code: string + self?: CyclicBody +} + +/** One link of the `cause` chain, when there is an error at the end of it. */ +function causeOf(error: Error): Error | undefined { + const cause = error.cause + return cause instanceof Error ? cause : undefined +} + +/** A sandbox whose `kill()` always fails with `error`, so `killSandbox` surfaces it. */ +function sandboxThatFailsWith(error: Error): Pick { + return { + kill: async () => { + throw error + }, + } +} + +/** The error `run` rejected with; anything else becomes a legible failure. */ +async function errorOf(run: Promise): Promise { + try { + await run + return new Error("nothing was thrown") + } catch (error) { + return error instanceof Error + ? error + : new Error(`not an Error: ${String(error)}`) + } +} + +test("a nested cause survives the copy, redacted", async () => { + // `new Error(msg, { cause })` installs `cause` as a *non-enumerable* own + // property, so a copy made by assignment truncates the chain exactly where + // the root reason lives — undici's `TypeError: fetch failed` is that shape. + const root = new Error(`connect refused, was using ${FAKE_TOKEN}`) + const wrapped = new Error(`sandbox teardown failed for ${FAKE_TOKEN}`, { + cause: root, + }) + + const surfaced = await errorOf(killSandbox(sandboxThatFailsWith(wrapped), 1)) + + expect(surfaced.message).toContain("could not destroy the relay sandbox") + const cause = causeOf(surfaced) + expect(cause?.message).toContain("sandbox teardown failed") + expect(cause?.message).not.toContain(FAKE_TOKEN) + // The link that used to be dropped, and the credential inside it. + const nested = cause ? causeOf(cause) : undefined + expect(nested?.message).toContain("connect refused") + expect(nested?.message).not.toContain(FAKE_TOKEN) +}) + +test("the copy keeps message and stack out of a JSON payload", async () => { + // A copy built by assignment makes both own *enumerable*, so a consumer that + // serialises `cause` into a log payload suddenly ships the whole stack. + const original = new Error(`teardown failed for ${FAKE_TOKEN}`) + + const surfaced = await errorOf(killSandbox(sandboxThatFailsWith(original), 1)) + const cause = causeOf(surfaced) + + expect(cause).toBeDefined() + expect(Object.keys(cause ?? {})).not.toContain("message") + expect(Object.keys(cause ?? {})).not.toContain("stack") + expect(JSON.stringify(cause)).toBe("{}") + // …while everything that reads an error properly still works. + expect(String(cause)).toContain("teardown failed") + expect(cause?.stack).toBeDefined() +}) + +test("a cause that cannot be copied is still redacted, never thrown", async () => { + // Two shapes that make a naive copy throw — and a throw here replaces a + // coded error with a raw `TypeError`, which is the failure class this whole + // branch exists to remove. + const cyclicBody: CyclicBody = { code: "Cyclic" } + cyclicBody.self = cyclicBody + const withCyclicBody = new Error(`teardown failed for ${FAKE_TOKEN}`) + Object.defineProperty(withCyclicBody, "body", { + value: cyclicBody, + enumerable: true, + }) + + const cyclic = await errorOf( + killSandbox(sandboxThatFailsWith(withCyclicBody), 1), + ) + expect(cyclic.message).toContain("could not destroy the relay sandbox") + expect(cyclic.message).not.toContain(FAKE_TOKEN) + expect(causeOf(cyclic)?.message).not.toContain(FAKE_TOKEN) + + // A getter anywhere on the error: reading it is running the caller's code. + const withThrowingGetter = new Error(`teardown failed for ${FAKE_TOKEN}`) + Object.defineProperty(withThrowingGetter, "detail", { + enumerable: true, + get: (): never => { + throw new Error("detail is gone") + }, + }) + + const getter = await errorOf( + killSandbox(sandboxThatFailsWith(withThrowingGetter), 1), + ) + expect(getter.message).toContain("could not destroy the relay sandbox") + expect(causeOf(getter)?.message).not.toContain(FAKE_TOKEN) + + // Even the sentence itself can be a getter that throws. There is nothing + // left to read then, so there is nothing left to leak either. + const unreadable = new Error("placeholder") + Object.defineProperty(unreadable, "message", { + get: (): never => { + throw new Error("message is gone") + }, + }) + + const opaque = await errorOf(killSandbox(sandboxThatFailsWith(unreadable), 1)) + expect(opaque.message).toContain("could not destroy the relay sandbox") + expect(causeOf(opaque)).toBeDefined() +}) + +test("an encoded dot cannot hide the token's shape", () => { + // Both nets key on the two dots: the pattern needs literal ones, and the + // exact-value comparison uses `encodeURIComponent`, which leaves a dot + // alone. A proxy that encodes the whole path — or an HTML error page that + // escapes it — produces neither form. + const percent = FAKE_TOKEN.replaceAll(".", "%2E") + const percentLower = FAKE_TOKEN.replaceAll(".", "%2e") + const entity = FAKE_TOKEN.replaceAll(".", ".") + + for (const leak of [percent, percentLower, entity]) { + // Without the exact value: the pattern has to see through the encoding. + expect(redactPreviewToken(`invalid preview token ${leak}`)).not.toContain( + leak, + ) + // And with it, by comparison. + expect( + redactPreviewToken(`invalid preview token ${leak}`, FAKE_TOKEN), + ).not.toContain(leak) + } + + // The first segment on its own is not three segments, and stays. + const segment = FAKE_TOKEN.split(".")[0] ?? "" + expect(redactPreviewToken(`sbx ${segment} up`)).toContain(segment) +}) + +test("killSandbox redacts the exact token when the caller knows it", async () => { + // `startRelay` holds the preview URL from the moment the sandbox answers, so + // every message it builds after that can be redacted by value rather than by + // grammar — the same belt the health poll wears. The leak below is in a + // shape no pattern matches, so only the exact value can remove it. + const shredded = FAKE_TOKEN.split(".").reverse().join("~") + const teardown = new Error(`host refused, token was ${shredded}`) + + const surfaced = await errorOf( + killSandbox(sandboxThatFailsWith(teardown), 1, shredded), + ) + + expect(surfaced.message).toContain("could not destroy the relay sandbox") + expect(surfaced.message).not.toContain(shredded) + expect(causeOf(surfaced)?.message).not.toContain(shredded) +}) + +test("relayStartError redacts the exact token in message and cause", () => { + // The path Sol's finding did not reach: `startRelay`'s catch, which knows + // the preview URL once the sandbox has answered. Same leak shape, so the + // patterns cannot help and only the value can. + const shredded = FAKE_TOKEN.split(".").reverse().join("~") + const body = { code: "GatewayTimeout", hint: `retry with ${shredded}` } + const gateway = new GatewayError(504, `upstream ${shredded} gave up`, body) + + const wrapped = relayStartError(gateway, shredded) + + expect(wrapped.code).toBe("relay_start_failed") + expect(wrapped.message).not.toContain(shredded) + const cause = causeOf(wrapped) + expect(cause?.message).not.toContain(shredded) + expect( + JSON.stringify(cause instanceof GatewayError ? cause.body : {}), + ).not.toContain(shredded) + // Fidelity is unchanged by the extra argument. + expect(cause).toBeInstanceOf(GatewayError) + expect(cause instanceof GatewayError ? cause.status : 0).toBe(504) +}) diff --git a/src/relay/deploy.ts b/src/relay/deploy.ts index 46d2c1b..ac53e40 100644 --- a/src/relay/deploy.ts +++ b/src/relay/deploy.ts @@ -118,6 +118,19 @@ const TOKEN_PARAM = /pt_token\s*[=:]\s*[^&;\s"'<>]+/gi */ const TOKEN_VALUE = /pt_[A-Za-z0-9._~-]{16,}/g +/** + * The ways a dot arrives once something has escaped the text around it: a + * percent-encoded path, an HTML error page. Both nets below key on the JWT's + * two dots, so an encoded one is the cheapest way past either. + */ +const ENCODED_DOTS = ["%2E", "%2e", "."] + +/** One base64url segment of a JWT, at the length a real one has. */ +const JWT_SEGMENT = "[A-Za-z0-9_-]{20,}" + +/** The separator between two of them, literal or escaped. */ +const JWT_DOT = `(?:\\.|${ENCODED_DOTS.join("|")})` + /** * The credential's real grammar: three base64url segments separated by dots. * The preview token is a ~362-character JWT @@ -125,7 +138,10 @@ const TOKEN_VALUE = /pt_[A-Za-z0-9._~-]{16,}/g * catches it bare in prose — "invalid preview token eyJhbGci…" — or behind a * `%3D` neither rule above can see. */ -const TOKEN_JWT = /[A-Za-z0-9_-]{20,}\.[A-Za-z0-9_-]{20,}\.[A-Za-z0-9_-]{20,}/g +const TOKEN_JWT = new RegExp( + `${JWT_SEGMENT}${JWT_DOT}${JWT_SEGMENT}${JWT_DOT}${JWT_SEGMENT}`, + "g", +) /** * Short enough to be an accident rather than a credential. Blanking every @@ -153,9 +169,7 @@ const MIN_TOKEN_LENGTH = 16 export function redactPreviewToken(text: string, token?: string): string { let redacted = text if (token !== undefined && token.length >= MIN_TOKEN_LENGTH) { - // A Set because a token made only of unreserved characters — a JWT is — - // encodes to itself, and replacing it twice would be busywork. - for (const form of new Set([token, encodeURIComponent(token)])) + for (const form of tokenForms(token)) redacted = redacted.replaceAll(form, "[redacted]") } return redacted @@ -164,12 +178,29 @@ export function redactPreviewToken(text: string, token?: string): string { .replace(TOKEN_JWT, "[redacted]") } +/** + * The written forms of one exact value. + * + * A Set because a token made only of unreserved characters — a JWT is — + * survives `encodeURIComponent` unchanged, so most of these collapse into one. + * The dot variants are the ones that do not: `encodeURIComponent` leaves a dot + * alone, and a proxy that escapes the whole path does not. + */ +function tokenForms(token: string): Set { + const written = [token, encodeURIComponent(token)] + const forms = new Set(written) + for (const form of written) + for (const dot of ENCODED_DOTS) forms.add(form.replaceAll(".", dot)) + return forms +} + /** * The credential this URL carries, so it can be redacted by value instead of * by grammar. Returns nothing for a string that is not a URL: there is simply * no known token then, and the patterns above still apply. */ -function previewTokenOf(url: string): string | undefined { +function previewTokenOf(url: string | undefined): string | undefined { + if (url === undefined) return undefined try { return new URL(url).searchParams.get("pt_token") ?? undefined } catch { @@ -193,40 +224,131 @@ interface WithBody { * * Through JSON rather than field by field: `code`, `error` and `message` are * what the type declares today, and the field a future gateway release puts - * the request URI in is the one worth covering in advance. + * the request URI in is the one worth covering in advance. A body that + * references itself makes this throw, which `redactedCause` catches. */ -function redactedBody(body: GatewayErrorBody): GatewayErrorBody { +function redactedBody( + body: GatewayErrorBody, + token: string | undefined, +): GatewayErrorBody { // SAFETY: this re-parses text serialised one call earlier; redaction only // ever replaces a run of characters inside a JSON string value, so the // document is still the same shape. return JSON.parse( - redactPreviewToken(JSON.stringify(body)), + redactPreviewToken(JSON.stringify(body), token), ) as GatewayErrorBody } /** - * A copy of an SDK error with the credential out of everything the gateway - * wrote. + * How far a `cause` chain is followed. Deeper than any chain the SDK, undici + * or this package builds, and finite, which is the point. + */ +const MAX_CAUSE_DEPTH = 8 + +/** + * Replace one property with a redacted value, keeping the descriptor the + * original had. + * + * A plain assignment onto an `Object.create` clone makes the property own and + * *enumerable*. On a real `Error`, `message`, `stack` and `cause` are none of + * those, and a consumer that does `JSON.stringify(error.cause)` into a log + * payload would suddenly ship the whole stack. + */ +function redefine(target: Error, key: string, value: T): void { + const existing = Object.getOwnPropertyDescriptor(target, key) + Object.defineProperty(target, key, { + value, + writable: existing?.writable ?? true, + enumerable: existing?.enumerable ?? false, + configurable: true, + }) +} + +/** + * What an error says, for a message being built out of it. + * + * Reading it runs the caller's code: `toString` reads `name` and `message`, + * and either can be a getter that throws. A throw here would replace a coded + * error with a raw one — the failure class this module exists to remove — so + * an error that will not say what it is says that instead. + */ +function sentenceOf(cause: unknown): string { + try { + return String(cause) + } catch { + return "an error that could not be read" + } +} + +/** + * The sentence and nothing else, for an error that cannot be copied. + * + * Reached when reading the original runs a getter that throws, or when its + * body references itself. A partial copy would be worse than this one: it + * would carry fields nothing has redacted. + */ +function unreadableCause(error: Error, token: string | undefined): Error { + return new Error(redactPreviewToken(sentenceOf(error), token)) +} + +/** + * A copy of an error with the credential out of everything foreign in it. * * `cause` exists so a caller keeps the original — `cause instanceof - * ConcurrencyLimitError`, `cause.status === 429` — so the copy keeps the - * prototype and every own field, and rewrites only the two made of foreign - * text: `message` and the parsed `body`. Without this, a clean outer message - * buys nothing: `console.error(error)`, pino's error serialiser and every - * crash reporter print the whole chain. + * ConcurrencyLimitError`, `cause.status === 429` — so the copy is built from + * the prototype and the full property descriptors, and only the text is + * rewritten: `message`, `stack`, a `GatewayError`'s parsed `body`, and + * recursively the chain hanging off `cause` (`new Error(msg, { cause })` + * installs that one non-enumerable, which is how a copy made by assignment + * loses undici's root `ECONNREFUSED`). Without any of this a clean outer + * message buys nothing: `console.error(error)`, pino's error serialiser and + * every crash reporter print the whole chain. + * + * Descriptors are copied, not read: an accessor stays an accessor and is never + * invoked here. That keeps a foreign getter from running inside a `catch` + * whose job is to produce a coded error — at the price of not redacting what + * such a getter would return, which nothing in the dependency tree has. */ -function redactedCause(error: Error): Error { - // SAFETY: `Object.create` returns a new object with `error`'s own prototype, - // so it is an instance of the same class; its own fields are copied below. - const clone = Object.create(Object.getPrototypeOf(error)) as Error & WithBody - // Own enumerable fields — `name`, and on a `GatewayError` `status`, `code` - // and `body`. `message` and `stack` are own but not enumerable, which is why - // they are the two lines after it. - Object.assign(clone, error) - clone.message = redactPreviewToken(error.message) - if (error.stack !== undefined) clone.stack = redactPreviewToken(error.stack) - if (clone.body !== undefined) clone.body = redactedBody(clone.body) - return clone +function redactedCause(error: Error, token?: string): Error { + return redactedChain(error, token, new Set(), 0) +} + +/** One link, plus the `seen` set and the depth that make the recursion end. */ +function redactedChain( + error: Error, + token: string | undefined, + seen: Set, + depth: number, +): Error { + // A chain that points back at itself, or one deeper than any real chain. + if (seen.has(error) || depth > MAX_CAUSE_DEPTH) + return unreadableCause(error, token) + seen.add(error) + try { + // SAFETY: `Object.create` returns a new object with `error`'s own + // prototype, so it is an instance of the same class; the descriptors below + // give it the same own properties, enumerable or not. + const clone = Object.create(Object.getPrototypeOf(error)) as Error & + WithBody + Object.defineProperties(clone, Object.getOwnPropertyDescriptors(error)) + redefine(clone, "message", redactPreviewToken(error.message, token)) + if (error.stack !== undefined) + redefine(clone, "stack", redactPreviewToken(error.stack, token)) + if (clone.body !== undefined) + redefine(clone, "body", redactedBody(clone.body, token)) + if (clone.cause instanceof Error) + redefine( + clone, + "cause", + redactedChain(clone.cause, token, seen, depth + 1), + ) + return clone + } catch { + // A getter that throws on the way in, a body that references itself: a + // faithful copy is not worth an uncoded exception out of `startRelay`'s + // catch, which is the failure class this module exists to remove. + return unreadableCause(error, token) + } } /** @@ -235,24 +357,33 @@ function redactedCause(error: Error): Error { * A 429 is the one worth telling apart: the account is at its concurrent * session cap, which is a "try again in a minute", not a "this is broken". * An error that already carries a code is passed through untouched. + * + * `token` is the preview credential when the caller has one — `startRelay` + * does from the moment the sandbox answers — so the message and the `cause` + * are redacted by value rather than by grammar. Exported for `deploy.test.ts`. */ -function relayStartError(cause: unknown): HandraiseError { +export function relayStartError( + cause: unknown, + token?: string, +): HandraiseError { if (isHandraiseError(cause)) return cause if (cause instanceof ConcurrencyLimitError) { return new HandraiseError( "concurrency_limit", redactPreviewToken( - `handraise: your Solari account is at its concurrent session limit, so the relay sandbox that gives the handoff its public URL could not be created. Free a session and retry. (${cause.message})`, + `handraise: your Solari account is at its concurrent session limit, so the relay sandbox that gives the handoff its public URL could not be created. Free a session and retry. (${sentenceOf(cause)})`, + token, ), - { cause: redactedCause(cause) }, + { cause: redactedCause(cause, token) }, ) } return new HandraiseError( "relay_start_failed", redactPreviewToken( - `handraise: the relay sandbox could not be started, so the handoff has no public URL and nobody has been asked for anything yet. ${String(cause)}`, + `handraise: the relay sandbox could not be started, so the handoff has no public URL and nobody has been asked for anything yet. ${sentenceOf(cause)}`, + token, ), - { cause: cause instanceof Error ? redactedCause(cause) : cause }, + { cause: cause instanceof Error ? redactedCause(cause, token) : cause }, ) } @@ -294,11 +425,15 @@ export async function createSandbox( * a plain `Error` and not a `HandraiseError`: both callers catch it and log * `relay_release_failed`, so it can never reach a `catch` around `raiseHand`, * and a code nobody can branch on is documentation for dead code. - * `attempts` is a parameter for the same reason as in `createSandbox`. + * `attempts` is a parameter for the same reason as in `createSandbox`, and + * `token` is the preview credential when the caller knows it — `startRelay` + * does once the sandbox has answered — so this message is redacted by value + * and not only by grammar. */ export async function killSandbox( sandbox: Pick, attempts: number = KILL_ATTEMPTS, + token?: string, ): Promise { let lastError: unknown // At least one attempt: "could not destroy it after 0 attempts" would be a @@ -318,12 +453,16 @@ export async function killSandbox( } throw new Error( redactPreviewToken( - `handraise: could not destroy the relay sandbox after ${budget} attempts; its public URL stays reachable until the idle timeout. Last error: ${String(lastError)}`, + `handraise: could not destroy the relay sandbox after ${budget} attempts; its public URL stays reachable until the idle timeout. Last error: ${sentenceOf(lastError)}`, + token, ), // The SDK's error, with the gateway's own words redacted — `cause` is // printed by every error serialiser that exists. { - cause: lastError instanceof Error ? redactedCause(lastError) : lastError, + cause: + lastError instanceof Error + ? redactedCause(lastError, token) + : lastError, }, ) } @@ -427,12 +566,17 @@ export async function startRelay( options.mode === "approval" ? "approval" : "takeover" const sandbox = await createSandbox(client, timeoutMs) + // Known only once `previewUrl()` answers, and every message built after that + // — the teardown failure, the wrapped start failure — can quote it. Declared + // out here so `kill` and the `catch` can both reach it. + let previewUrl: string | undefined + let killed = false const kill = async (): Promise => { if (killed) return // Throws if it cannot: the caller must not believe the relay is gone when // it is not. Both callers log it as `relay_release_failed`. - await killSandbox(sandbox) + await killSandbox(sandbox, KILL_ATTEMPTS, previewTokenOf(previewUrl)) killed = true } @@ -451,7 +595,7 @@ export async function startRelay( }) const preview = await sandbox.previewUrl(RELAY_PORT) - const previewUrl = withToken(preview.url, preview.token) + previewUrl = withToken(preview.url, preview.token) await waitForHealth(relayUrl(previewUrl, "/healthz")) // https and wss are both "special" URL schemes, so a textual swap is exact. @@ -470,6 +614,6 @@ export async function startRelay( // Everything from `connect()` to the health poll is "the relay did not come // up"; `relayStartError` keeps the more specific codes (a 429, a public URL // that never answered) as they are. - throw relayStartError(error) + throw relayStartError(error, previewTokenOf(previewUrl)) } }