diff --git a/.changeset/graphql-introspection-credential-log.md b/.changeset/graphql-introspection-credential-log.md new file mode 100644 index 000000000..d7a146d28 --- /dev/null +++ b/.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=`. 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. diff --git a/packages/plugins/graphql/src/sdk/errors.ts b/packages/plugins/graphql/src/sdk/errors.ts index 40cb4c611..8f2d04d82 100644 --- a/packages/plugins/graphql/src/sdk/errors.ts +++ b/packages/plugins/graphql/src/sdk/errors.ts @@ -11,6 +11,7 @@ export class GraphqlIntrospectionError extends Schema.TaggedErrorClass`. 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) => + 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}`); + }), + ); + + 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 = []; + const seen: Array = []; + + 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); + }), + ); +}); diff --git a/packages/plugins/graphql/src/sdk/introspect-request-url.test.ts b/packages/plugins/graphql/src/sdk/introspect-request-url.test.ts new file mode 100644 index 000000000..d8d1a505e --- /dev/null +++ b/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): 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)(recordingFetch(seen))), + ); + +const silentLogger = Logger.layer([Logger.make(() => {})]); + +/** Runs introspection against a fetch that always fails, and returns the one + * URL it was asked to dial. */ +const dialedUrl = (endpoint: string, queryParams?: Record) => + Effect.gen(function* () { + const seen: Array = []; + 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®ion=eu`, { + token: "s3cr3t", + trace: "on", + }); + expect(url).toBe(`${ENDPOINT}?apiVersion=2®ion=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 = []; + const logged: Array = []; + + const error = yield* introspect("https://svc:hunter2@graph.example.test/graphql").pipe( + Effect.flip, + Effect.provide(clientLayer(seen)), + Effect.provide( + Logger.layer([ + Logger.make((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 = []; + 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); + }), + ); +}); diff --git a/packages/plugins/graphql/src/sdk/introspect.ts b/packages/plugins/graphql/src/sdk/introspect.ts index 316547ef1..d484dcdfb 100644 --- a/packages/plugins/graphql/src/sdk/introspect.ts +++ b/packages/plugins/graphql/src/sdk/introspect.ts @@ -235,18 +235,64 @@ 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. + // The query still reaches the upstream: 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. + // + // The split is NOT byte-transparent, and deliberately so. Recombination + // appends each pair through `URLSearchParams`, whose form-urlencoded + // serializer writes a space as `+` rather than `%20`, percent-encodes `~` and + // `!'()`, and gives a valueless `?flag` a trailing `=`. Key order, duplicate + // keys and already-encoded reserved characters survive unchanged. No request + // shape avoids this: the 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. Log safety + // wins. `introspect-request-url.test.ts` pins the exact resulting URLs. + // + // An endpoint that does not parse has no such split available: it would go + // verbatim into `request.url` — query, credential and all — and `queryParams` + // could not be applied to it at all. Reject it instead of dialing a request + // that silently omits the credential the caller asked us to send. The + // endpoint is deliberately left out of the message, since it may be carrying + // the secret. + if (!URL.canParse(endpoint)) { + return yield* new GraphqlIntrospectionError({ + message: "GraphQL endpoint is not a valid URL", + reason: "invalid-endpoint", + }); + } + + const requestUrl = new URL(endpoint); + + // Userinfo (`https://user:pass@host/…`) is a credential placement with no + // split of its own: `URL` keeps it in the origin, so it stays in + // `request.url` and renders into every `HttpClientError` message exactly the + // way a query-carried secret used to. Reject it rather than log it, with the + // same constant message that echoes no part of the endpoint. + if (requestUrl.username !== "" || requestUrl.password !== "") { + return yield* new GraphqlIntrospectionError({ + message: "GraphQL endpoint must not embed credentials in the URL", + reason: "invalid-endpoint", + }); + } + + for (const [name, value] of Object.entries(queryParams ?? {})) { + requestUrl.searchParams.set(name, value); + } - 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"), diff --git a/packages/plugins/graphql/src/sdk/plugin.test.ts b/packages/plugins/graphql/src/sdk/plugin.test.ts index 7a9ef3161..a9597fd72 100644 --- a/packages/plugins/graphql/src/sdk/plugin.test.ts +++ b/packages/plugins/graphql/src/sdk/plugin.test.ts @@ -886,6 +886,56 @@ describe("graphqlPlugin real protocol server", () => { }), ); + it.effect("reports an unparseable endpoint as a config problem, not a credential one", () => + Effect.gen(function* () { + const executor = yield* makeExecutor(); + yield* executor.graphql.addIntegration({ + endpoint: "not a url", + slug: "health_bad_endpoint", + }); + + const result = yield* executor.connections.validate({ + owner: "org", + integration: IntegrationSlug.make("health_bad_endpoint"), + template: AuthTemplateSlug.make("none"), + value: "unused", + }); + + expect(result).toMatchObject({ + status: "unknown", + detail: + "The GraphQL endpoint URL is invalid. Edit the integration configuration, then try again.", + }); + // No request was ever sent, so nothing upstream judged the credential. + // Telling the operator to check it would send them down a dead end. + expect(result.detail).not.toContain("credential"); + }), + ); + + it.effect("reports an endpoint with embedded userinfo as a config problem", () => + Effect.gen(function* () { + const executor = yield* makeExecutor(); + yield* executor.graphql.addIntegration({ + endpoint: "https://svc:hunter2@graph.example.test/graphql", + slug: "health_userinfo_endpoint", + }); + + const result = yield* executor.connections.validate({ + owner: "org", + integration: IntegrationSlug.make("health_userinfo_endpoint"), + template: AuthTemplateSlug.make("none"), + value: "unused", + }); + + expect(result).toMatchObject({ + status: "unknown", + detail: + "The GraphQL endpoint URL is invalid. Edit the integration configuration, then try again.", + }); + expect(result.detail).not.toContain("hunter2"); + }), + ); + it.effect("persists the introspection failure when tool sync is incomplete", () => Effect.gen(function* () { const server = yield* serveTestHttpApp(() => diff --git a/packages/plugins/graphql/src/sdk/plugin.ts b/packages/plugins/graphql/src/sdk/plugin.ts index e669b98db..fb53f28ac 100644 --- a/packages/plugins/graphql/src/sdk/plugin.ts +++ b/packages/plugins/graphql/src/sdk/plugin.ts @@ -99,6 +99,8 @@ const GRAPHQL_INVALID_SCHEMA_DETAIL = const GRAPHQL_AUTH_DETAIL = "Check the credential and selected authentication method."; const GRAPHQL_NETWORK_DETAIL = "The GraphQL endpoint could not be reached. Check the URL and upstream availability, then try again."; +const GRAPHQL_INVALID_ENDPOINT_DETAIL = + "The GraphQL endpoint URL is invalid. Edit the integration configuration, then try again."; const truncateHealthDetail = (text: string, max = 240): string => { const normalized = text.replaceAll(/\s+/g, " ").trim(); @@ -145,6 +147,18 @@ const healthFromIntrospectionError = ( const upstream = error.upstreamMessage; const httpStatus = error.status; + // Classified first, and never as a credential problem: the endpoint was + // rejected before any request went out, so nothing upstream judged the + // credential. Sending the operator to re-enter a working secret would be a + // dead end — the URL in the integration config is what needs the edit. + if (error.reason === "invalid-endpoint") { + return { + status: "unknown", + checkedAt, + detail: GRAPHQL_INVALID_ENDPOINT_DETAIL, + }; + } + if (httpStatus === 401 || httpStatus === 403 || isAuthMessage(upstream)) { const statusDetail = httpStatus === 401 || httpStatus === 403