fix(health): keep credentials out of the response sample we persist

.changeset/health-response-sample-scrub.md

@@ -0,0 +1,16 @@
+---
+"executor": patch
+---
+
+**Credentials are kept out of the health-check result that gets persisted**
+
+A health check stores a sample of the probed operation's response body, plus the extracted identity, in `connection.last_health` — so whatever those carry is written to the database. The operation is user-chosen from the plugin's catalog, which means it can just as easily be a key-listing endpoint as a `/me`, and those return secrets that no scrub of the connection's own credential value can recognise, because they are different secrets entirely.
+
+Two passes now cover both kinds of secret:
+
+- **By key name.** Leaves whose key names a credential (`token`, `api_key`, `secret`, `authorization`, `session`, …) have their value replaced with `[redacted]`. The row itself is kept, so the live preview still shows the response shape and the identity picker still works. Keys that merely contain a matching substring, such as `author`, are left alone. camelCase spellings are recognised too: `accessToken`, `refreshToken`, `clientSecret`, `privateKey` and `sessionId` have no separator before the credential word, so a matcher that only looks for one reads them as innocent.
+- **By value.** The OpenAPI health check removes the connection's own credential value from each sampled value, covering the other direction: a body that echoes back the key it was authenticated with under an innocent-looking name. This runs before the sample's 120-char truncation, not after — truncating first leaves a prefix of a long credential that an exact-value scrub can no longer match, and that prefix is what would be persisted.
+
+The key check reads a dotted path two ways. It uses the nearest NAMED segment, because array elements are named by index: `{"tokens": ["sk-live-…"]}` produces the path `tokens.0`, and testing the literal `"0"` matches nothing. It also uses an enclosing array container, because a key listing returns `{"api_keys": [{"value": "sk-live-…"}]}`, whose path is `api_keys.0.value` — the nearest named segment there is the innocent `value`, and only the array's own key says what the collection holds. A collection whose key names nothing, such as `names.0`, is still shown in full.
+
+The extracted `identity` goes through both passes as well. It is read straight off the raw body, so it previously bypassed them even though it is persisted the same way, and `identityField` is user-chosen from whatever the picker listed.

packages/core/sdk/src/health-check.ts

@@ -379,19 +379,111 @@ export const projectResponseFields = (
   return fields;
 };
 
+/** Placeholder shown instead of a leaf whose key names it as secret-bearing. */
+export const REDACTED_SAMPLE_VALUE = "[redacted]";
+
+/**
+ * Leaf keys whose value is a credential rather than something worth previewing.
+ *
+ * The sample exists so a user can pick their identity field, and the keys that
+ * serve that (`email`, `login`, `username`, `name`, `id`) do not collide with
+ * any of these — so this can afford to be blunt.
+ *
+ * This catches the secrets we do NOT already know. Scrubbing the connection's
+ * own credential value out of the sample only helps when the body echoes the
+ * key we authenticated with; a health check pointed at a key-listing endpoint
+ * returns different secrets entirely, and no scrub of a known value can see
+ * those.
+ */
+/* The optional trailing `s` matters because of the array case below: a key that
+ * holds a COLLECTION of secrets is named in the plural (`tokens`, `api_keys`,
+ * `credentials`), and those are exactly the key-listing responses this guards.
+ * It stays inside the same letter boundary, so `author` is still not `auth`. */
+const SECRET_KEY_PATTERN =
+  /(^|[^a-z])(secret|token|password|passwd|apikey|api_key|credential|authorization|auth|session|cookie|private|signature|bearer|refresh)s?([^a-z]|$)/i;
+
+/** camelCase hides the separator the pattern looks for: `accessToken` has no
+ *  non-letter before `Token`, so `accessToken`, `refreshToken`, `clientSecret`,
+ *  `privateKey` and `sessionId` all read as innocent. Splitting on a
+ *  lowercase→uppercase transition exposes that boundary. */
+const splitCamelCase = (key: string): string => key.replaceAll(/([a-z0-9])([A-Z])/g, "$1 $2");
+
+/** True when a single key names a credential.
+ *
+ *  BOTH spellings are tested, not just the split one: `apiKey` matches only as
+ *  the contiguous word `apikey`, which splitting would destroy. Testing both
+ *  only widens the net where a case boundary exists, so `author` and
+ *  `authorName` — which have no boundary in the wrong place — stay visible. */
+const keyNamesASecret = (key: string): boolean =>
+  SECRET_KEY_PATTERN.test(key) || SECRET_KEY_PATTERN.test(splitCamelCase(key));
+
+const isIndexSegment = (segment: string): boolean => /^\d+$/.test(segment);
+
+/** True when a dotted path names a credential, under either of two readings.
+ *
+ *  THE NEAREST NAMED SEGMENT. The walker names array elements by index, so a
+ *  bare array of secrets — `{"tokens": ["sk-live-…"]}` — produces the path
+ *  `tokens.0`, whose literal last segment is `"0"` and matches nothing. Numeric
+ *  segments are skipped so the check lands on the key that named the
+ *  collection, which is the only place the secret is described. `keys.0.token`
+ *  is unaffected: its last segment already names the leaf.
+ *
+ *  AN ENCLOSING ARRAY CONTAINER. A key-listing endpoint returns
+ *  `{"api_keys": [{"value": "sk-live-…"}]}`, whose path is `api_keys.0.value`.
+ *  The nearest named segment is the innocent `value`, so the first reading
+ *  alone hands the secret to the database. A segment that DIRECTLY contains an
+ *  array — one immediately followed by an index — and names a credential
+ *  therefore covers everything under it.
+ *
+ *  That second reading also blanks a sibling like `api_keys.0.name`. This is
+ *  the right trade: a name inside a key list is never the connection's own
+ *  identity (`candidateIdentityTier` already refuses indexed paths), and the
+ *  alternative is a persisted secret. `names.0` is untouched, because its
+ *  container names nothing. */
+export const pathNamesASecret = (path: string): boolean => {
+  const segments = path.split(".");
+  const named = segments.filter((segment) => !isIndexSegment(segment));
+  const leaf = named[named.length - 1] ?? "";
+  if (keyNamesASecret(leaf)) return true;
+  return segments.some(
+    (segment, index) =>
+      !isIndexSegment(segment) &&
+      isIndexSegment(segments[index + 1] ?? "") &&
+      keyNamesASecret(segment),
+  );
+};
+
 /**
  * Walk an actual JSON response body and return its scalar leaves as
  * `{ path, value }` rows (value stringified + truncated). Drives the live
  * preview's "show me what this returns" list. Bounded to depth 4, 25 fields,
  * and ~120-char values.
+ *
+ * Leaves whose key names a credential are kept but their value is replaced,
+ * so the preview still shows the field exists without persisting its value —
+ * this result is written to `connection.last_health`.
+ *
+ * `scrub` removes credential values the caller already knows (the secret the
+ * connection authenticated with, which a body can echo back under an
+ * innocent-looking key). It runs INSIDE the walk, before truncation, because
+ * the two orders are not equivalent: a credential longer than the value cap
+ * would otherwise be cut to a 120-char prefix that an exact-substring scrub can
+ * no longer recognise, and that prefix is what gets persisted.
  */
-export const extractResponseFields = (data: unknown): HealthCheckResponseSample[] => {
+export const extractResponseFields = (
+  data: unknown,
+  options?: { readonly scrub?: (value: string) => string },
+): HealthCheckResponseSample[] => {
   const out: HealthCheckResponseSample[] = [];
   const MAX_DEPTH = 4;
   const MAX_FIELDS = 25;
   const MAX_VALUE = 120;
+  const scrub = options?.scrub;
 
-  const render = (v: string): string => (v.length > MAX_VALUE ? `${v.slice(0, MAX_VALUE)}...` : v);
+  const render = (raw: string): string => {
+    const v = scrub === undefined ? raw : scrub(raw);
+    return v.length > MAX_VALUE ? `${v.slice(0, MAX_VALUE)}...` : v;
+  };
 
   const visit = (node: unknown, path: string, depth: number) => {
     if (out.length >= MAX_FIELDS || node == null) return;
@@ -418,7 +510,10 @@ export const extractResponseFields = (data: unknown): HealthCheckResponseSample[
       path !== "" &&
       (typeof node === "string" || typeof node === "number" || typeof node === "boolean")
     ) {
-      out.push({ path, value: render(String(node)) });
+      out.push({
+        path,
+        value: pathNamesASecret(path) ? REDACTED_SAMPLE_VALUE : render(String(node)),
+      });
     }
   };
 

packages/core/sdk/src/health-response-sample-redaction.test.ts

@@ -0,0 +1,183 @@
+// A health check writes its response sample into `connection.last_health`, so
+// whatever the sample carries is persisted. The operation being probed is
+// user-chosen from the plugin's catalog, which means it can be a key-listing
+// endpoint just as easily as a `/me`.
+//
+// These use real response bodies of the shape those endpoints return, because
+// the property under test is what survives the walk into the database.
+
+import { describe, expect, it } from "@effect/vitest";
+
+import { extractResponseFields, REDACTED_SAMPLE_VALUE } from "./health-check";
+
+/** The sample as a path -> value lookup, which is how the assertions read. */
+const byPath = (data: unknown): Record<string, string> =>
+  Object.fromEntries(extractResponseFields(data).map((f) => [f.path, f.value]));
+
+describe("health-check response sample redaction", () => {
+  it("redacts credential-named leaves while keeping the identity fields", () => {
+    const fields = byPath({
+      email: "alex@example.com",
+      login: "alex",
+      id: 4711,
+      api_key: "sk-live-must-not-be-persisted",
+      refresh_token: "rt-must-not-be-persisted",
+      session: "sess-must-not-be-persisted",
+    });
+
+    // The reason the sample exists still works.
+    expect(fields.email).toBe("alex@example.com");
+    expect(fields.login).toBe("alex");
+    expect(fields.id).toBe("4711");
+
+    expect(fields.api_key).toBe(REDACTED_SAMPLE_VALUE);
+    expect(fields.refresh_token).toBe(REDACTED_SAMPLE_VALUE);
+    expect(fields.session).toBe(REDACTED_SAMPLE_VALUE);
+  });
+
+  it("redacts nested and array-borne credentials, not just top-level ones", () => {
+    // What a key-listing endpoint actually returns. This is the case a scrub
+    // of the connection's own value cannot catch: these are different secrets.
+    const fields = byPath({
+      keys: [
+        { name: "prod", token: "sk-prod-must-not-be-persisted" },
+        { name: "staging", token: "sk-staging-must-not-be-persisted" },
+      ],
+      account: { billing: { secret: "whsec-must-not-be-persisted" } },
+    });
+
+    expect(fields["keys.0.name"]).toBe("prod");
+    expect(fields["keys.0.token"]).toBe(REDACTED_SAMPLE_VALUE);
+    expect(fields["keys.1.token"]).toBe(REDACTED_SAMPLE_VALUE);
+    expect(fields["account.billing.secret"]).toBe(REDACTED_SAMPLE_VALUE);
+  });
+
+  it("redacts a bare array of secrets, where only the enclosing key names them", () => {
+    // `{"tokens": ["sk-…"]}` yields the paths `tokens.0`, `tokens.1`. Their last
+    // segment is an array index, so a check that reads the literal leaf finds
+    // "0" and lets the secret straight through into `connection.last_health`.
+    const fields = byPath({
+      tokens: ["sk-live-must-not-be-persisted", "sk-test-must-not-be-persisted"],
+      names: ["prod", "staging"],
+    });
+
+    expect(fields["tokens.0"]).toBe(REDACTED_SAMPLE_VALUE);
+    expect(fields["tokens.1"]).toBe(REDACTED_SAMPLE_VALUE);
+
+    // The index walk-back stops at the nearest NAMED segment, so an innocent
+    // collection is still shown in full.
+    expect(fields["names.0"]).toBe("prod");
+    expect(fields["names.1"]).toBe("staging");
+  });
+
+  it("redacts camelCase credential keys, in both spellings", () => {
+    // `accessToken` has no non-letter before `Token`, so a matcher that only
+    // looks for a separator reads it as innocent. camelCase is the dominant
+    // spelling in JSON APIs, so this is not an edge case.
+    const fields = byPath({
+      accessToken: "at-must-not-be-persisted",
+      refreshToken: "rt-must-not-be-persisted",
+      clientSecret: "cs-must-not-be-persisted",
+      privateKey: "pk-must-not-be-persisted",
+      sessionId: "sid-must-not-be-persisted",
+      // Still matched as one contiguous word, which is how it has always
+      // matched: splitting on the case boundary alone would lose it.
+      apiKey: "ak-must-not-be-persisted",
+    });
+
+    for (const value of Object.values(fields)) {
+      expect(value).toBe(REDACTED_SAMPLE_VALUE);
+    }
+  });
+
+  it("does not redact camelCase keys that merely start with a matching word", () => {
+    // The other direction of the same change: exposing the case boundary must
+    // not turn ordinary fields into blanks.
+    const fields = byPath({
+      author: "alex",
+      authors: "alex, sam",
+      authorName: "Alex",
+      privacyLevel: "public",
+      tokenizer: "bpe",
+    });
+
+    expect(fields.author).toBe("alex");
+    expect(fields.authors).toBe("alex, sam");
+    expect(fields.authorName).toBe("Alex");
+    expect(fields.privacyLevel).toBe("public");
+    expect(fields.tokenizer).toBe("bpe");
+  });
+
+  it("redacts a secret under an innocent leaf inside a credential-named array", () => {
+    // The shape a key-listing endpoint actually returns. The nearest named
+    // segment is the harmless `value`; only the array's own key says what the
+    // collection holds.
+    const fields = byPath({
+      api_keys: [{ name: "prod", value: "sk-live-must-not-be-persisted" }],
+      names: ["prod", "staging"],
+      results: [{ value: "42" }],
+    });
+
+    expect(fields["api_keys.0.value"]).toBe(REDACTED_SAMPLE_VALUE);
+
+    // An array whose key names nothing is untouched, at either depth.
+    expect(fields["names.0"]).toBe("prod");
+    expect(fields["results.0.value"]).toBe("42");
+  });
+
+  it("scrubs a known credential value before truncating, not after", () => {
+    // A credential can sit under a key that names nothing — the body echoing
+    // back the key it was authenticated with. Only the caller can recognise it,
+    // by exact value. Truncating first cuts it to a 120-char prefix that the
+    // exact-value scrub no longer matches, and that prefix is what reaches
+    // `connection.last_health`.
+    const secret = `sk-live-${"x".repeat(200)}`;
+    expect(secret.length).toBeGreaterThan(120);
+
+    const sample = extractResponseFields(
+      { data: secret },
+      { scrub: (value) => value.split(secret).join(REDACTED_SAMPLE_VALUE) },
+    );
+
+    expect(sample[0]?.value).toBe(REDACTED_SAMPLE_VALUE);
+    // Not merely "shorter than the secret": assert no recognisable prefix of it
+    // survived, which is the actual leak.
+    expect(sample[0]?.value).not.toContain("sk-live-");
+  });
+
+  it("still truncates a long value the scrub does not recognise", () => {
+    // Reordering must not disable the cap for everything else.
+    const long = "y".repeat(400);
+    const sample = extractResponseFields({ blob: long }, { scrub: (value) => value });
+
+    expect(sample[0]?.value).toBe(`${"y".repeat(120)}...`);
+  });
+
+  it("keeps the field visible so the preview still shows the shape", () => {
+    // Dropping the row would change what the picker displays. Redacting the
+    // value keeps the response shape legible without persisting the secret.
+    const sample = extractResponseFields({ api_key: "sk-live-x" });
+
+    expect(sample).toHaveLength(1);
+    expect(sample[0]?.path).toBe("api_key");
+  });
+
+  it("does not redact identity keys that merely contain a matching substring", () => {
+    // `author` contains "auth". Matching it would silently blank a normal
+    // field, which is how an over-eager redactor makes the feature useless.
+    const fields = byPath({ author: "alex", authorization: "Bearer x" });
+
+    expect(fields.author).toBe("alex");
+    expect(fields.authorization).toBe(REDACTED_SAMPLE_VALUE);
+  });
+
+  it("POSITIVE CONTROL: an unredacted body does come through verbatim", () => {
+    // Proves these assertions can fail. Without it, an extractor that returned
+    // nothing, or redacted everything, would satisfy the checks above.
+    const fields = byPath({ email: "alex@example.com", plan: "pro" });
+
+    expect(fields.email).toBe("alex@example.com");
+    expect(fields.plan).toBe("pro");
+    expect(Object.values(fields)).not.toContain(REDACTED_SAMPLE_VALUE);
+  });
+});

packages/core/sdk/src/index.ts

@@ -128,6 +128,8 @@ export {
   sortHealthCheckCandidatesByIdentity,
   projectResponseFields,
   extractResponseFields,
+  pathNamesASecret,
+  REDACTED_SAMPLE_VALUE,
   identityPathTier,
   rankResponseSample,
 } from "./health-check";

packages/plugins/openapi/src/sdk/backing.ts

@@ -12,7 +12,9 @@ import {
   sortHealthCheckCandidatesByIdentity,
   extractIdentity,
   extractResponseFields,
+  pathNamesASecret,
   projectResponseFields,
+  REDACTED_SAMPLE_VALUE,
   type HealthCheckCandidate,
   type HealthCheckResponseField,
   type HealthCheckResult,
@@ -972,13 +974,33 @@ export const checkHealthOpenApi = (input: {
     }
 
     const status = classifyHttpStatus(probe.result.status);
-    const identity =
+    const rawIdentity =
       status === "healthy" ? extractIdentity(probe.result.data, spec.identityField) : undefined;
+    // The identity is read straight off the raw body, so unlike the sample it
+    // passes through neither redaction pass — and it is persisted to
+    // `connection.last_health` just the same. `identityField` is user-chosen
+    // from whatever the picker listed, which on a key-listing endpoint includes
+    // `api_keys.0.value`. Run both passes over it: the key reading first, then
+    // the known-value scrub.
+    const identity =
+      rawIdentity === undefined
+        ? undefined
+        : pathNamesASecret(spec.identityField ?? "")
+          ? REDACTED_SAMPLE_VALUE
+          : scrubSecrets(rawIdentity);
     // Sample the returned body ONLY on a healthy probe: the sample exists to
     // pick an identity field, and error bodies (upstream internals, auth error
     // envelopes) have no business in the preview. Non-healthy runs carry the
     // classified `detail` instead.
-    const responseSample = status === "healthy" ? extractResponseFields(probe.result.data) : [];
+    // Same scrub the `detail` branch below uses, for the same reason: a body
+    // can echo back the key it was authenticated with. `extractResponseFields`
+    // already redacts leaves whose KEY names a credential; this covers the
+    // other direction, a credential value under an innocent-looking key. It is
+    // handed to the walker rather than mapped over the result so it runs before
+    // the 120-char truncation, which would otherwise leave an unrecognisable —
+    // and unscrubbable — prefix of a long secret.
+    const responseSample =
+      status === "healthy" ? extractResponseFields(probe.result.data, { scrub: scrubSecrets }) : [];
     return {
       status,
       httpStatus: probe.result.status,