From 1069e7cc1ae20bf64f417deeff510c83e9a30330 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Thu, 13 Aug 2026 03:17:08 +0200 Subject: [PATCH] Keep a query-carried credential out of GraphQL introspection's failure log query is a supported credential carrier, so an endpoint can be reached with ?token=. Introspection built its request from a URL string, and setUrl keeps a string verbatim as request.url; every HttpClientError renders method + request.url into its message getter, and the failure cause is logged raw. So a transport failure or a non-JSON response wrote the secret to the log. Build the request from a URL object instead. setUrl then moves the query into request.urlParams and clears it from request.url, so the secret is absent from the message and from anything else rendering the URL. The client recombines the two when it executes, so nothing changes on the wire. Handles the endpoint's own query string too, since a configured endpoint can carry a credential. --- .../graphql-introspection-credential-log.md | 11 ++ .../sdk/introspect-credential-logging.test.ts | 113 ++++++++++++++++++ .../plugins/graphql/src/sdk/introspect.ts | 36 ++++-- 3 files changed, 149 insertions(+), 11 deletions(-) create mode 100644 .changeset/graphql-introspection-credential-log.md create mode 100644 packages/plugins/graphql/src/sdk/introspect-credential-logging.test.ts diff --git a/.changeset/graphql-introspection-credential-log.md b/.changeset/graphql-introspection-credential-log.md new file mode 100644 index 0000000000..8c60b25eb6 --- /dev/null +++ b/.changeset/graphql-introspection-credential-log.md @@ -0,0 +1,11 @@ +--- +"executor": patch +--- + +**GraphQL introspection no longer logs a credential carried in the query string** + +`query` is a supported credential carrier, so a GraphQL endpoint can be reached with `?token=`. Introspection built its request from a URL **string**, and `HttpClientRequest.setUrl` keeps a string verbatim as `request.url`. Every `HttpClientError` renders `${method} ${request.url}` into its `message` getter, and introspection logs the raw failure cause — so on any transport failure or non-JSON response, the connection's secret was written to the process log. + +The request is now built from a URL **object**, which moves the query into `request.urlParams` and clears it from `request.url`. The secret is therefore absent from the error message, and from anything else that renders the request URL. Nothing changes on the wire: the client recombines url and urlParams when it executes the request. + +The endpoint's own query string is handled the same way, not just the separately-supplied query parameters, since a configured endpoint can carry a credential too. diff --git a/packages/plugins/graphql/src/sdk/introspect-credential-logging.test.ts b/packages/plugins/graphql/src/sdk/introspect-credential-logging.test.ts new file mode 100644 index 0000000000..0716fe3fff --- /dev/null +++ b/packages/plugins/graphql/src/sdk/introspect-credential-logging.test.ts @@ -0,0 +1,113 @@ +// --------------------------------------------------------------------------- +// Introspection must not log a credential carried in the query string. +// +// `query` is a supported credential carrier, so a GraphQL endpoint can be +// reached with `?token=`. Introspection logs the raw failure cause on +// any transport error, and every `HttpClientError` renders `${method} +// ${request.url}` into its message — so if the request is built from a URL +// STRING, the secret is inside that message and goes straight to the log. +// +// Building the request from a URL OBJECT moves the query into +// `request.urlParams`, out of `request.url` and therefore out of the message, +// while the client still recombines the two when it executes. +// +// Both directions are asserted. A test that only checked "the secret is absent" +// would pass just as happily against a logger that captured nothing at all, or +// a change that stopped sending the parameter entirely. +// --------------------------------------------------------------------------- + +import { describe, expect, it } from "@effect/vitest"; +import { Cause, Effect, Layer, Logger } from "effect"; +import { FetchHttpClient } from "effect/unstable/http"; + +import { introspect } from "./introspect"; + +const SECRET = "tok_live_introspection_MUST_NOT_LOG"; +const ENDPOINT = "https://graph.example.test/graphql"; + +/** Collects everything a logger would have written, message and cause alike — + * `Cause.pretty` is the renderer that rebuilds the first line from the error's + * live `message` getter, which is the exact path the leak took. */ +const capturingLogger = (sink: Array) => + Logger.make((options) => { + sink.push(String(options.message)); + sink.push(Cause.pretty(options.cause)); + }); + +/** A fetch that records the URL it was handed and then fails at the transport + * layer, which is what drives introspection down its logging path. */ +const failingFetch = (seen: Array): typeof globalThis.fetch => + (async (input: RequestInfo | URL) => { + seen.push(input instanceof Request ? input.url : String(input)); + // oxlint-disable-next-line executor/no-try-catch-or-throw, executor/no-error-constructor -- boundary: simulates the platform fetch rejecting on a dead host + throw new Error("getaddrinfo ENOTFOUND graph.example.test"); + }) as typeof globalThis.fetch; + +const clientLayer = (seen: Array) => + FetchHttpClient.layer.pipe( + Layer.provide(Layer.succeed(FetchHttpClient.Fetch)(failingFetch(seen))), + ); + +describe("GraphQL introspection credential logging", () => { + it.effect("does not write a query-carried credential to the log", () => + Effect.gen(function* () { + const logged: Array = []; + const seen: Array = []; + + yield* introspect(ENDPOINT, undefined, { token: SECRET }).pipe( + Effect.flip, + Effect.provide(clientLayer(seen)), + Effect.provide(Logger.layer([capturingLogger(logged)])), + ); + + const output = logged.join("\n"); + + // Positive control FIRST: prove the logger actually captured the failure. + // Without this, an empty capture would satisfy every assertion below. + expect(output).toContain("graphql introspection request failed"); + expect(output).toContain("graph.example.test"); + + // The credential is absent from everything that was logged. + expect(output).not.toContain(SECRET); + expect(output).not.toContain("token="); + }), + ); + + it.effect("still sends the query-carried credential on the wire", () => + Effect.gen(function* () { + const logged: Array = []; + const seen: Array = []; + + yield* introspect(ENDPOINT, undefined, { token: SECRET }).pipe( + Effect.flip, + Effect.provide(clientLayer(seen)), + Effect.provide(Logger.layer([capturingLogger(logged)])), + ); + + // Keeping it out of the log is only correct if it still reaches the + // upstream — otherwise this "fix" silently breaks authentication. + expect(seen).toHaveLength(1); + expect(seen[0]).toContain(`token=${SECRET}`); + }), + ); + + it.effect("keeps a credential carried in the endpoint's own query out of the log", () => + Effect.gen(function* () { + // A configured endpoint can carry the secret itself, with no separate + // queryParams argument at all. + const logged: Array = []; + const seen: Array = []; + + yield* introspect(`${ENDPOINT}?token=${SECRET}`).pipe( + Effect.flip, + Effect.provide(clientLayer(seen)), + Effect.provide(Logger.layer([capturingLogger(logged)])), + ); + + const output = logged.join("\n"); + expect(output).toContain("graphql introspection request failed"); + expect(output).not.toContain(SECRET); + expect(seen[0]).toContain(`token=${SECRET}`); + }), + ); +}); diff --git a/packages/plugins/graphql/src/sdk/introspect.ts b/packages/plugins/graphql/src/sdk/introspect.ts index 316547ef1a..2a59715919 100644 --- a/packages/plugins/graphql/src/sdk/introspect.ts +++ b/packages/plugins/graphql/src/sdk/introspect.ts @@ -235,18 +235,32 @@ export const introspect = Effect.fn("GraphQL.introspect")(function* ( queryParams?: Record, ) { const client = yield* HttpClient.HttpClient; - const requestEndpoint = - queryParams && Object.keys(queryParams).length > 0 - ? (() => { - const url = new URL(endpoint); - for (const [name, value] of Object.entries(queryParams)) { - url.searchParams.set(name, value); - } - return url.toString(); - })() - : endpoint; + // Hand `post` a URL OBJECT rather than a string, deliberately. + // + // `HttpClientRequest.setUrl` keeps a string verbatim as `request.url`, and + // every `HttpClientError` renders `${method} ${request.url}` into its + // `message` getter. The `query` carrier is a supported credential placement, + // so an endpoint reached with `?token=…` put that secret inside the error + // message — and the `Effect.logError(…, cause)` below writes the message + // straight to the log on any transport failure or non-JSON response. + // + // Given a URL object, `setUrl` moves the query into `request.urlParams` and + // clears it from `request.url`, so the same failure logs the bare endpoint. + // Nothing is lost on the wire: the client recombines url + urlParams when it + // executes the request. Handling the endpoint's OWN query the same way (not + // just the `queryParams` argument) matters — a configured endpoint can carry + // a credential in its query string too. + const requestUrl: string | URL = URL.canParse(endpoint) + ? (() => { + const url = new URL(endpoint); + for (const [name, value] of Object.entries(queryParams ?? {})) { + url.searchParams.set(name, value); + } + return url; + })() + : endpoint; - let request = HttpClientRequest.post(requestEndpoint).pipe( + let request = HttpClientRequest.post(requestUrl).pipe( HttpClientRequest.setHeader("Content-Type", "application/json"), HttpClientRequest.setHeader("Accept", "application/json"), HttpClientRequest.setHeader("User-Agent", "executor-graphql"),