diff --git a/package.json b/package.json index a3832d2ae..23ffb2123 100644 --- a/package.json +++ b/package.json @@ -97,7 +97,7 @@ "graphql-ws": "^5.13.1", "gt3-server-node-express-sdk": "https://github.com/GaloyMoney/gt3-server-node-express-bypass#master", "i18n": "^0.15.1", - "ibex-client": "^3.2.0", + "ibex-client": "^3.3.0", "invoices": "^3.0.0", "ioredis": "^5.3.2", "ioredis-cache": "^2.0.0", diff --git a/src/services/ibex/client.ts b/src/services/ibex/client.ts index 90819695c..976c06d99 100644 --- a/src/services/ibex/client.ts +++ b/src/services/ibex/client.ts @@ -250,8 +250,13 @@ const payInvoice = async ( // Call the generated SDK through withAuth directly (instead of // Ibex.payInvoiceV2) so a failed payment's FetchError — which carries the // parsed IBEX error body on `.data` — reaches httpErrorHandler intact. - // ibex-client@3.2.0's own ApiError wrapper discards that body, which is what - // made "insufficient balance" 400s unclassifiable (lnflash/ibex-client#6). + // This seam predates ibex-client@3.3.0: 3.2.0's ApiError wrapper discarded + // that body (lnflash/ibex-client#6), which made "insufficient balance" 400s + // unclassifiable. As of 3.3.0, ApiError carries the body itself and + // errorHandler classifies it through the standard path, so the seam is + // redundant — retained only as belt-and-braces until it is collapsed back + // to `Ibex.payInvoiceV2(bodyWithHooks).then(errorHandler)` in its own PR + // (lnflash/flash#478; a payment-path change doesn't belong in a deps bump). return Ibex.authentication .withAuth(() => Ibex.ibex.payInvoiceV2(bodyWithHooks)) .then(errorHandler) diff --git a/src/services/ibex/errors.ts b/src/services/ibex/errors.ts index d6c5c42e2..f3d09bbe7 100644 --- a/src/services/ibex/errors.ts +++ b/src/services/ibex/errors.ts @@ -87,22 +87,26 @@ export const errorHandler = ( ): T | IbexError => { if (e instanceof AuthenticationError) return new IbexError(e, ErrorLevel.Critical) if (e instanceof ApiError) { - // Classify against the structured body detail when the error carries one, - // and against `message` otherwise (flash's raw-fetch path embeds the body - // text in the message; ibex-client@3.2.0's ApiError message is only the - // wrapped stack, which is why body-carrying shapes are checked first). + // Classify against the structured body detail when the error carries one + // (ibex-client >= 3.3.0's ApiError extracts it onto `ibexMessage`), and + // against `message` otherwise — flash's raw-fetch path embeds the body + // text in the message. Body-carrying shapes are checked first so a stack + // that happens to contain a needle can't misclassify. const detail = ibexErrorDetail(e) const classified = classifyIbexErrorText(detail ?? e.message) if (classified === InsufficientIbexBalance) return new InsufficientIbexBalance(e, ErrorLevel.Info, detail) if (classified === CompletedInvoice) return new CompletedInvoice(e, ErrorLevel.Info) - // Unclassified path: mirror httpErrorHandler's carry — an unrecognized - // IBEX 400 whose ApiError has a stack-only message must still log what - // IBEX actually said. Build a new IbexError rather than mutating `e`, - // which the caller may still hold. + // Unclassified path: an unrecognized IBEX 400 must still log what IBEX + // actually said. ibex-client >= 3.3.0's ApiError already appends + // "IBEX response (): " to its own message, so only carry + // the detail when the message doesn't already contain it (older or raw + // shapes with a stack-only message). Build a new IbexError rather than + // mutating `e`, which the caller may still hold. if (detail !== undefined) { const generic = new IbexError(e, ErrorLevel.Warn) - generic.message = `${detail}\n${generic.message}` + if (!generic.message.includes(detail)) + generic.message = `${detail}\n${generic.message}` return generic } } @@ -111,29 +115,50 @@ export const errorHandler = ( } /** - * Classify a raw error thrown by the generated IBEX SDK (or fetch) before - * ibex-client's ApiError wrapper can discard the response body. With the - * pinned ibex-client@3.2.0, ApiError keeps only `httpCode` — the JSON error - * body that distinguishes e.g. "insufficient balance" from any other 400 only - * exists on the underlying FetchError's `.data` (lnflash/ibex-client#6). - * Call sites that need body-level classification invoke the SDK through - * `Ibex.authentication.withAuth` themselves and route the caught error here. + * Classify a raw error thrown by the generated IBEX SDK (or fetch) for call + * sites that invoke the SDK through `Ibex.authentication.withAuth` themselves + * and route the caught error here (the payInvoice raw-fetch seam). The seam + * predates ibex-client@3.3.0: 3.2.0's ApiError kept only `httpCode` and + * discarded the JSON error body that distinguishes e.g. "insufficient + * balance" from any other 400 (lnflash/ibex-client#6). As of 3.3.0, ApiError + * extracts the body itself (`ibexResponse` / `ibexMessage`) and errorHandler + * classifies it through the standard path — this handler remains as + * defense-in-depth for the raw-fetch seam until that seam is collapsed + * (lnflash/flash#478). */ export const httpErrorHandler = (e: unknown): IbexError => { const raw = e instanceof Error ? e : new Error(String(e)) if (raw instanceof AuthenticationError) return new IbexError(raw, ErrorLevel.Critical) - const detail = ibexErrorDetail(raw) // ApiError's constructor keeps `.status` as httpCode, which IbexError reads. const wrapped = raw instanceof IbexClientError ? raw : new ApiError(raw) + // Derive the detail from `wrapped`, never from `raw`. For a raw FetchError, + // ApiError's own extraction (`ibexMessage`) is capped at ibex-client's + // MAX_IBEX_MESSAGE_LENGTH, while reading straight off `raw.data` is not: + // comparing an uncapped detail against the capped copy embedded in + // `wrapped.message` would defeat the dedupe guard below for any body over + // the cap — exactly the Cloudflare-HTML-error-page outage the cap exists + // for — and prepend the full multi-KB body onto every failing call's + // message. When `raw` is already an IbexClientError, wrapped === raw and + // the extraction is unchanged. The uncapped body exists only on the local + // ApiError's `ibexResponse` and does not survive onto the returned + // IbexError — by design: pino serializes own enumerable properties, and + // attaching a multi-KB body would recreate the log bloat the cap prevents. + const detail = ibexErrorDetail(wrapped) const classified = classifyIbexErrorText(detail ?? raw.message) if (classified === InsufficientIbexBalance) return new InsufficientIbexBalance(wrapped, ErrorLevel.Info, detail) if (classified === CompletedInvoice) return new CompletedInvoice(wrapped, ErrorLevel.Info) - // Unclassified path: ApiError's message is only the wrapped stack, so carry - // the extracted body detail into it — an unrecognized IBEX 400 must still - // log what IBEX actually said, not just "FetchError: Bad Request" + stack. - if (detail !== undefined && !(raw instanceof IbexClientError)) + // Unclassified path: an unrecognized IBEX 400 must still log what IBEX + // actually said, not just "FetchError: Bad Request" + stack. The ApiError + // constructed above already embeds the extracted detail in its message + // (ibex-client >= 3.3.0), so only carry the detail when the message doesn't + // already contain it — never append the same text twice. + if ( + detail !== undefined && + !(raw instanceof IbexClientError) && + !wrapped.message.includes(detail) + ) wrapped.message = `${detail}\n${wrapped.message}` return new IbexError(wrapped, ErrorLevel.Warn) } diff --git a/test/flash/unit/services/ibex/errors.spec.ts b/test/flash/unit/services/ibex/errors.spec.ts index fa9283bb4..de480b7b8 100644 --- a/test/flash/unit/services/ibex/errors.spec.ts +++ b/test/flash/unit/services/ibex/errors.spec.ts @@ -1,5 +1,5 @@ import { ErrorLevel } from "@domain/shared" -import { ApiError, AuthenticationError } from "ibex-client" +import { ApiError, AuthenticationError, MAX_IBEX_MESSAGE_LENGTH } from "ibex-client" import { CompletedInvoice, @@ -56,6 +56,47 @@ describe("ibexErrorDetail", () => { }) }) +// ibex-client >= 3.3.0 populates ibexMessage/ibexResponse/httpCode itself — +// these construct the real ApiError with no simulated fields, pinning the +// integration the ^3.3.0 bump claims to deliver. +describe("ibex-client 3.3.0 integration", () => { + it("ApiError extracts the body detail on construction", () => { + const apiErr = new ApiError(fetchErrorShaped(400, { error: insufficientDetail })) + + expect(apiErr.httpCode).toBe(400) + expect(apiErr.ibexMessage).toBe(insufficientDetail) + expect(apiErr.ibexResponse).toEqual({ error: insufficientDetail }) + expect(ibexErrorDetail(apiErr)).toBe(insufficientDetail) + }) + + it("errorHandler classifies a real body-carrying ApiError, stripping the account id", () => { + const apiErr = new ApiError(fetchErrorShaped(400, { error: insufficientDetail })) + + const result = errorHandler(apiErr) + + expect(result).toBeInstanceOf(InsufficientIbexBalance) + const err = result as InsufficientIbexBalance + expect(err.message).toBe(insufficientDetailStripped) + expect(err.message).not.toContain("39c6e986-979b-40ab-9e7b-df18a9277a84") + expect(err.detail).toBe(insufficientDetail) + }) + + it("errorHandler carries a real unclassified detail onto the generic IbexError", () => { + const apiErr = new ApiError(fetchErrorShaped(400, { error: "invalid parameters" })) + + const result = errorHandler(apiErr) + + expect(result).toBeInstanceOf(IbexError) + expect(result).not.toBeInstanceOf(InsufficientIbexBalance) + const message = (result as IbexError).message + expect(message).toContain("invalid parameters") + // 3.3.0's ApiError already embeds the detail in its own message — the + // unclassified carry must not append the same text a second time (the + // duplicated detail would reach logs and Discord alert embeds) + expect(message.split("invalid parameters").length - 1).toBe(1) + }) +}) + describe("errorHandler", () => { it("classifies an ApiError whose message carries the insufficient-balance text", () => { // flash's raw-fetch path embeds the body text in the wrapped message @@ -107,8 +148,8 @@ describe("errorHandler", () => { expect(apiErr.message).toBe(originalMessage) }) - it("maps a pinned-version SDK-path ApiError (stack-only message) to a generic IbexError", () => { - // ibex-client@3.2.0 discards the response body: message is only the + it("maps an ApiError with no extractable body (stack-only message) to a generic IbexError", () => { + // when the response carries no body, 3.3.0's ApiError message is only the // wrapped FetchError stack ("FetchError: Bad Request\n at ...") const apiErr = new ApiError(fetchErrorShaped(400, undefined)) @@ -191,6 +232,43 @@ describe("httpErrorHandler", () => { // unrecognized IBEX 400 that logs only "FetchError: Bad Request" is the // debugging blindness this module exists to fix expect(err.message).toContain("invalid parameters") + // ... and exactly once: 3.3.0's ApiError wrapper already embeds the + // detail in its message, so the carry must not duplicate it + expect(err.message.split("invalid parameters").length - 1).toBe(1) + }) + + it("keeps the message bounded when the body exceeds ibex-client's cap", () => { + // Cloudflare-style outage: IBEX's proxy returns a full HTML error page, + // which arrives as a plain-text body far over MAX_IBEX_MESSAGE_LENGTH. + // ApiError embeds only the capped copy in its message; if the dedupe + // guard compared against an uncapped extraction of the same body, it + // would always miss and prepend the full multi-KB blob — once per + // failing call — into logs and Discord alert embeds. + const hugeBody = `cf-502 error page${"x".repeat( + MAX_IBEX_MESSAGE_LENGTH * 4, + )}` + const raw = fetchErrorShaped(502, hugeBody) + const rawStackLength = (raw.stack as string).length + + const result = httpErrorHandler(raw) + + expect(result).toBeInstanceOf(IbexError) + expect(result).not.toBeInstanceOf(InsufficientIbexBalance) + const err = result as IbexError + expect(err.httpCode).toBe(502) + // the truncated detail appears exactly once... + const truncatedDetail = `${hugeBody.slice(0, MAX_IBEX_MESSAGE_LENGTH)}... [truncated]` + expect(err.message.split(truncatedDetail).length - 1).toBe(1) + // ... the capped prefix itself is not duplicated (truncated + full copy)... + expect(err.message.split(hugeBody.slice(0, MAX_IBEX_MESSAGE_LENGTH)).length - 1).toBe( + 1, + ) + // ... the full uncapped body never reaches the message... + expect(err.message).not.toContain(hugeBody) + // ... and the whole message stays bounded: stack + capped detail + framing + expect(err.message.length).toBeLessThan( + rawStackLength + MAX_IBEX_MESSAGE_LENGTH + 100, + ) }) it("keeps the generic path unchanged when the error carries no body detail", () => { diff --git a/yarn.lock b/yarn.lock index 4d189dd80..c4a302261 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8878,10 +8878,10 @@ i18n@^0.15.1: math-interval-parser "^2.0.1" mustache "^4.2.0" -ibex-client@^3.2.0: - version "3.2.0" - resolved "https://registry.yarnpkg.com/ibex-client/-/ibex-client-3.2.0.tgz#2a9a707e1c250c1bd226e7be99c5d5baa2f24ca2" - integrity sha512-jteyXwSBUIwEyjEXFXdJMUYtBdoZ2HloDoFuBm20rThN6jlR/2XAaimlitZjmImKE1u1wCLrAcaAXWH8AcOStA== +ibex-client@^3.3.0: + version "3.3.0" + resolved "https://registry.yarnpkg.com/ibex-client/-/ibex-client-3.3.0.tgz#92b7098a326d830dfb975b281b08e5113fb054f1" + integrity sha512-gMP2Bm5nyMaaQkRAcj7IoIVyujUnc0DPgIOZaOG/QLtaPb2zCzc1Hu4K/ey6boG2cRODq1VipSeUYgwB870LwQ== dependencies: api "^6.1.2" node-cache "^5.1.2"