diff --git a/.changeset/graphql-introspection-credential-log.md b/.changeset/graphql-introspection-credential-log.md new file mode 100644 index 000000000..8c60b25eb --- /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 000000000..0716fe3ff --- /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 316547ef1..2a5971591 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"),