Keep a query-carried credential out of GraphQL introspection's failure log

.changeset/graphql-introspection-credential-log.md

@@ -0,0 +1,24 @@
+---
+"executor": patch
+---
+
+**GraphQL introspection no longer logs a credential carried in the endpoint URL**
+
+`query` is a supported credential carrier, so a GraphQL endpoint can be reached with `?token=<secret>`. 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. The credential still reaches the upstream: 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.
+
+**The query string is now normalized on the wire.** Recombination appends each pair through `URLSearchParams`, so the query is re-serialized in form-urlencoded form instead of passed through byte-for-byte:
+
+- a space written as `%20` is sent as `+`
+- `~` and `!'()` are percent-encoded
+- a valueless `?flag` is sent as `flag=`
+
+Key order, repeated keys, and already-encoded reserved characters are unchanged, and every parameter still decodes to the same value. This is not avoidable while the fix holds: raw query bytes only survive inside `request.url`, which is the one field every error message renders, so byte-transparency and keeping the credential out of the log cannot both hold. An upstream that signs its raw query string is the case to watch. The exact resulting URLs are pinned by test.
+
+Two endpoints are now rejected up front with an `invalid-endpoint` failure rather than dialed:
+
+- an endpoint that is not a valid URL, which cannot be split this way and would otherwise be sent without the query parameters it was asked to include
+- an endpoint carrying userinfo (`https://user:pass@host/…`), which `URL` keeps in the origin, so it would stay in `request.url` and leak into error messages exactly the way a query-carried secret used to
+
+Neither rejection echoes any part of the endpoint. A health check on such an integration now reports the invalid configuration and points the operator at the endpoint URL, instead of blaming the credential that was never sent.

packages/plugins/graphql/src/sdk/errors.ts

@@ -11,6 +11,7 @@ export class GraphqlIntrospectionError extends Schema.TaggedErrorClass<GraphqlIn
       Schema.Literals([
         "network",
         "http",
+        "invalid-endpoint",
         "invalid-json",
         "invalid-shape",
         "missing-schema",

packages/plugins/graphql/src/sdk/introspect-credential-logging.test.ts

@@ -0,0 +1,139 @@
+// ---------------------------------------------------------------------------
+// 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=<secret>`. 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 { GraphqlIntrospectionError } from "./errors";
+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<string>) =>
+  Logger.make<unknown, void>((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<string>): 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<string>) =>
+  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<string> = [];
+      const seen: Array<string> = [];
+
+      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<string> = [];
+      const seen: Array<string> = [];
+
+      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<string> = [];
+      const seen: Array<string> = [];
+
+      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}`);
+    }),
+  );
+
+  it.effect("fails a malformed endpoint instead of dialing it without the query params", () =>
+    Effect.gen(function* () {
+      // Only a parseable endpoint can carry the query in `urlParams`. An
+      // unparseable one must fail rather than quietly dial without the
+      // credential it was told to send.
+      const logged: Array<string> = [];
+      const seen: Array<string> = [];
+
+      const error = yield* introspect("not a url", undefined, { token: SECRET }).pipe(
+        Effect.flip,
+        Effect.provide(clientLayer(seen)),
+        Effect.provide(Logger.layer([capturingLogger(logged)])),
+      );
+
+      expect(error).toBeInstanceOf(GraphqlIntrospectionError);
+      expect(error.reason).toBe("invalid-endpoint");
+
+      // Nothing was sent: no request at all, rather than one missing the token.
+      expect(seen).toHaveLength(0);
+      // And the rejection itself does not echo the credential anywhere.
+      expect(Cause.pretty(Cause.fail(error))).not.toContain(SECRET);
+      expect(logged.join("\n")).not.toContain(SECRET);
+    }),
+  );
+});

packages/plugins/graphql/src/sdk/introspect-request-url.test.ts

@@ -0,0 +1,182 @@
+// ---------------------------------------------------------------------------
+// The exact URL introspection dials.
+//
+// Keeping a query-carried credential out of the log means splitting the query
+// off `request.url` and into `request.urlParams` (see
+// `introspect-credential-logging.test.ts`). The client recombines the two by
+// appending each pair through `URLSearchParams`, so the query is re-serialized
+// in form-urlencoded form rather than passed through byte-for-byte.
+//
+// That is a real, if narrow, wire change, and it is not recoverable: raw query
+// bytes only survive inside `request.url`, which is the single field every
+// `HttpClientError` message renders. So the normalization is pinned here by
+// exact string equality rather than described in prose — a `toContain` check
+// would let any of these encodings drift silently, and an upstream that signs
+// its query string would notice.
+// ---------------------------------------------------------------------------
+
+import { describe, expect, it } from "@effect/vitest";
+import { Cause, Effect, Layer, Logger } from "effect";
+import { FetchHttpClient } from "effect/unstable/http";
+
+import { GraphqlIntrospectionError } from "./errors";
+import { introspect } from "./introspect";
+
+const ENDPOINT = "https://graph.example.test/graphql";
+
+/** Records the URL handed to the platform, then fails the transport so the
+ *  effect finishes without a live host. */
+const recordingFetch = (seen: Array<string>): 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<string>) =>
+  FetchHttpClient.layer.pipe(
+    Layer.provide(Layer.succeed(FetchHttpClient.Fetch)(recordingFetch(seen))),
+  );
+
+const silentLogger = Logger.layer([Logger.make<unknown, void>(() => {})]);
+
+/** Runs introspection against a fetch that always fails, and returns the one
+ *  URL it was asked to dial. */
+const dialedUrl = (endpoint: string, queryParams?: Record<string, string>) =>
+  Effect.gen(function* () {
+    const seen: Array<string> = [];
+    yield* introspect(endpoint, undefined, queryParams).pipe(
+      Effect.flip,
+      Effect.provide(clientLayer(seen)),
+      Effect.provide(silentLogger),
+    );
+    expect(seen).toHaveLength(1);
+    return seen[0];
+  });
+
+describe("GraphQL introspection request URL", () => {
+  it.effect("merges query params into an endpoint that already has some", () =>
+    Effect.gen(function* () {
+      // Endpoint params keep their position and order; merged params are
+      // appended after them.
+      const url = yield* dialedUrl(`${ENDPOINT}?apiVersion=2&region=eu`, {
+        token: "s3cr3t",
+        trace: "on",
+      });
+      expect(url).toBe(`${ENDPOINT}?apiVersion=2&region=eu&token=s3cr3t&trace=on`);
+    }),
+  );
+
+  it.effect("adds a query to an endpoint that has none", () =>
+    Effect.gen(function* () {
+      const url = yield* dialedUrl(ENDPOINT, { token: "s3cr3t" });
+      expect(url).toBe(`${ENDPOINT}?token=s3cr3t`);
+    }),
+  );
+
+  it.effect("overwrites a duplicate param in place rather than appending it", () =>
+    Effect.gen(function* () {
+      // The supplied param wins, and it keeps the endpoint param's position —
+      // so the stale value is gone, not merely shadowed.
+      const url = yield* dialedUrl(`${ENDPOINT}?token=stale&keep=1`, { token: "fresh" });
+      expect(url).toBe(`${ENDPOINT}?token=fresh&keep=1`);
+    }),
+  );
+
+  it.effect("collapses repeats of an overwritten key to the supplied value", () =>
+    Effect.gen(function* () {
+      const url = yield* dialedUrl(`${ENDPOINT}?a=1&a=2`, { a: "3" });
+      expect(url).toBe(`${ENDPOINT}?a=3`);
+    }),
+  );
+
+  it.effect("keeps repeated keys the caller did not overwrite", () =>
+    Effect.gen(function* () {
+      const url = yield* dialedUrl(`${ENDPOINT}?a=1&a=2&b=3`);
+      expect(url).toBe(`${ENDPOINT}?a=1&a=2&b=3`);
+    }),
+  );
+
+  it.effect("re-encodes a %20 space as +", () =>
+    Effect.gen(function* () {
+      // Documented normalization, not an accident: `URLSearchParams` serializes
+      // a space in form-urlencoded form. The decoded value is unchanged.
+      const url = yield* dialedUrl(`${ENDPOINT}?token=a%20b`);
+      expect(url).toBe(`${ENDPOINT}?token=a+b`);
+    }),
+  );
+
+  it.effect("leaves a + space as +", () =>
+    Effect.gen(function* () {
+      const url = yield* dialedUrl(`${ENDPOINT}?token=a+b`);
+      expect(url).toBe(`${ENDPOINT}?token=a+b`);
+    }),
+  );
+
+  it.effect("percent-encodes a literal tilde", () =>
+    Effect.gen(function* () {
+      // `~` is legal in a raw query and is left alone by `URL`, but the
+      // form-urlencoded serializer escapes it.
+      const url = yield* dialedUrl(`${ENDPOINT}?sig=~tilde~`);
+      expect(url).toBe(`${ENDPOINT}?sig=%7Etilde%7E`);
+    }),
+  );
+
+  it.effect("gives a valueless flag a trailing =", () =>
+    Effect.gen(function* () {
+      const url = yield* dialedUrl(`${ENDPOINT}?debug`);
+      expect(url).toBe(`${ENDPOINT}?debug=`);
+    }),
+  );
+
+  it.effect("leaves an already-encoded reserved character encoded", () =>
+    Effect.gen(function* () {
+      const url = yield* dialedUrl(`${ENDPOINT}?a=%2B`);
+      expect(url).toBe(`${ENDPOINT}?a=%2B`);
+    }),
+  );
+
+  it.effect("rejects an endpoint carrying userinfo, without echoing it", () =>
+    Effect.gen(function* () {
+      // `user:pass@host` is a credential placement that `URL` keeps in the
+      // origin, so it would ride along in `request.url` and render into every
+      // `HttpClientError` message — the exact leak the query split closes.
+      const seen: Array<string> = [];
+      const logged: Array<string> = [];
+
+      const error = yield* introspect("https://svc:hunter2@graph.example.test/graphql").pipe(
+        Effect.flip,
+        Effect.provide(clientLayer(seen)),
+        Effect.provide(
+          Logger.layer([
+            Logger.make<unknown, void>((options) => {
+              logged.push(String(options.message));
+              logged.push(Cause.pretty(options.cause));
+            }),
+          ]),
+        ),
+      );
+
+      expect(error).toBeInstanceOf(GraphqlIntrospectionError);
+      expect(error.reason).toBe("invalid-endpoint");
+
+      // Nothing was dialed, and neither the error nor the log names the secret.
+      expect(seen).toHaveLength(0);
+      const rendered = `${Cause.pretty(Cause.fail(error))}\n${logged.join("\n")}`;
+      expect(rendered).not.toContain("hunter2");
+      expect(rendered).not.toContain("svc:");
+    }),
+  );
+
+  it.effect("rejects userinfo even when only a username is present", () =>
+    Effect.gen(function* () {
+      const seen: Array<string> = [];
+      const error = yield* introspect("https://svc@graph.example.test/graphql", undefined, {
+        token: "s3cr3t",
+      }).pipe(Effect.flip, Effect.provide(clientLayer(seen)), Effect.provide(silentLogger));
+
+      expect(error.reason).toBe("invalid-endpoint");
+      expect(seen).toHaveLength(0);
+    }),
+  );
+});