From 0c7e109cfe204fbd3bc74f94451414015b651bf2 Mon Sep 17 00:00:00 2001 From: GeiserX <9169332+GeiserX@users.noreply.github.com> Date: Sat, 15 Aug 2026 16:21:26 +0200 Subject: [PATCH 1/3] fix(health): keep credentials out of the response sample we persist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A health check stores its response sample in connection.last_health, and the operation being probed is picked by the user from the plugin's catalog. Point it at a key-listing endpoint, or a /me that returns an api_key, and those values land in the database. The field right next to it already knew about this: `detail` is scrubbed of every credential value, with a comment saying upstream text can echo the request back. `responseSample` was left out of that scrub. Two directions, because they catch different things. Leaves whose KEY names a credential are redacted in the walker — that covers secrets we have never seen, which is exactly what a key-listing endpoint returns and what no scrub of a known value could find. The connection's own credential value is then scrubbed from what remains, covering a secret under an innocent key. The row is kept and its value replaced, so the preview still shows the shape. --- packages/core/sdk/src/health-check.ts | 34 +++++++- .../health-response-sample-redaction.test.ts | 82 +++++++++++++++++++ packages/plugins/openapi/src/sdk/backing.ts | 12 ++- 3 files changed, 126 insertions(+), 2 deletions(-) create mode 100644 packages/core/sdk/src/health-response-sample-redaction.test.ts diff --git a/packages/core/sdk/src/health-check.ts b/packages/core/sdk/src/health-check.ts index d553f994fd..4648f24d5e 100644 --- a/packages/core/sdk/src/health-check.ts +++ b/packages/core/sdk/src/health-check.ts @@ -379,11 +379,40 @@ 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. + */ +const SECRET_KEY_PATTERN = + /(^|[^a-z])(secret|token|password|passwd|apikey|api_key|credential|authorization|auth|session|cookie|private|signature|bearer|refresh)([^a-z]|$)/i; + +/** True when the last segment of a dotted path names a credential. */ +const namesASecret = (path: string): boolean => { + const leaf = path.slice(path.lastIndexOf(".") + 1); + return SECRET_KEY_PATTERN.test(leaf); +}; + /** * 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`. */ export const extractResponseFields = (data: unknown): HealthCheckResponseSample[] => { const out: HealthCheckResponseSample[] = []; @@ -418,7 +447,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: namesASecret(path) ? REDACTED_SAMPLE_VALUE : render(String(node)), + }); } }; diff --git a/packages/core/sdk/src/health-response-sample-redaction.test.ts b/packages/core/sdk/src/health-response-sample-redaction.test.ts new file mode 100644 index 0000000000..674b954ced --- /dev/null +++ b/packages/core/sdk/src/health-response-sample-redaction.test.ts @@ -0,0 +1,82 @@ +// 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 => + 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("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); + }); +}); diff --git a/packages/plugins/openapi/src/sdk/backing.ts b/packages/plugins/openapi/src/sdk/backing.ts index 4e6e5ba95a..f5b444ae5e 100644 --- a/packages/plugins/openapi/src/sdk/backing.ts +++ b/packages/plugins/openapi/src/sdk/backing.ts @@ -978,7 +978,17 @@ export const checkHealthOpenApi = (input: { // 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. + const responseSample = + status === "healthy" + ? extractResponseFields(probe.result.data).map((field) => ({ + ...field, + value: scrubSecrets(field.value), + })) + : []; return { status, httpStatus: probe.result.status, From ee65a10d6de4b8ff71422306bfa4be355ca210ec Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 21:21:30 -0700 Subject: [PATCH 2/3] Redact health-sample secrets that only the enclosing array key names --- .changeset/health-response-sample-scrub.md | 13 +++++++++++++ packages/core/sdk/src/health-check.ts | 18 +++++++++++++++--- .../health-response-sample-redaction.test.ts | 18 ++++++++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 .changeset/health-response-sample-scrub.md diff --git a/.changeset/health-response-sample-scrub.md b/.changeset/health-response-sample-scrub.md new file mode 100644 index 0000000000..0251d0fd84 --- /dev/null +++ b/.changeset/health-response-sample-scrub.md @@ -0,0 +1,13 @@ +--- +"executor": patch +--- + +**Credentials are kept out of the health-check response sample that gets persisted** + +A health check stores a sample of the probed operation's response body in `connection.last_health`, so whatever the sample carries 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. + +Leaves whose key names a credential (`token`, `api_key`, `secret`, `authorization`, `session`, …) now 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. + +The key check reads the nearest named segment of the path rather than its literal last segment, and recognises a plural key. Array elements are named by index, so a bare array of secrets — `{"tokens": ["sk-live-…"]}` — produces the path `tokens.0`: testing the literal `"0"` matched nothing, and a collection of secrets is named in the plural anyway. Both were needed for the value to be redacted; `author` is still not treated as `auth`. + +The OpenAPI health check additionally scrubs the connection's own credential value out of each sampled value, covering the other direction: a body that echoes back the key it was authenticated with under an innocent-looking name. diff --git a/packages/core/sdk/src/health-check.ts b/packages/core/sdk/src/health-check.ts index 4648f24d5e..1c67159e1c 100644 --- a/packages/core/sdk/src/health-check.ts +++ b/packages/core/sdk/src/health-check.ts @@ -395,12 +395,24 @@ export const REDACTED_SAMPLE_VALUE = "[redacted]"; * 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)([^a-z]|$)/i; + /(^|[^a-z])(secret|token|password|passwd|apikey|api_key|credential|authorization|auth|session|cookie|private|signature|bearer|refresh)s?([^a-z]|$)/i; -/** True when the last segment of a dotted path names a credential. */ +/** True when the nearest NAMED segment of a dotted path names a credential. + * + * 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. */ const namesASecret = (path: string): boolean => { - const leaf = path.slice(path.lastIndexOf(".") + 1); + const named = path.split(".").filter((segment) => !/^\d+$/.test(segment)); + const leaf = named[named.length - 1] ?? ""; return SECRET_KEY_PATTERN.test(leaf); }; diff --git a/packages/core/sdk/src/health-response-sample-redaction.test.ts b/packages/core/sdk/src/health-response-sample-redaction.test.ts index 674b954ced..1e5124b80e 100644 --- a/packages/core/sdk/src/health-response-sample-redaction.test.ts +++ b/packages/core/sdk/src/health-response-sample-redaction.test.ts @@ -52,6 +52,24 @@ describe("health-check response sample redaction", () => { 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("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. From 52bc19b34b8bae7598021ce067d8c686dc646593 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Thu, 27 Aug 2026 23:35:38 -0700 Subject: [PATCH 3/3] Close the remaining paths a secret takes into last_health Scrub before truncating. The sample cut values to 120 chars inside the walk and the OpenAPI check scrubbed the connection's credential out afterwards, so a credential longer than the cap survived as a prefix the exact-value scrub could no longer match. The scrub now runs inside the walk, ahead of the cut. Match camelCase key names. accessToken, refreshToken, clientSecret, privateKey and sessionId carry no separator before the credential word, so the pattern read them as innocent. Both spellings are tested, since apiKey only matches as one contiguous word. author and authorName stay visible. Read an enclosing array container. A key listing returns api_keys.0.value, whose nearest named segment is the innocent value. A secret-named segment that directly contains an array now covers what is under it. names.0 is untouched. Redact the identity too. It is read straight off the raw body, so it passed through neither pass, and identityField is user-chosen from whatever the picker listed. --- .changeset/health-response-sample-scrub.md | 13 +- packages/core/sdk/src/health-check.ts | 77 ++++++-- .../health-response-sample-redaction.test.ts | 83 +++++++++ packages/core/sdk/src/index.ts | 2 + packages/plugins/openapi/src/sdk/backing.ts | 28 ++- .../src/sdk/health-check-redaction.test.ts | 169 ++++++++++++++++++ 6 files changed, 346 insertions(+), 26 deletions(-) create mode 100644 packages/plugins/openapi/src/sdk/health-check-redaction.test.ts diff --git a/.changeset/health-response-sample-scrub.md b/.changeset/health-response-sample-scrub.md index 0251d0fd84..82897a1d55 100644 --- a/.changeset/health-response-sample-scrub.md +++ b/.changeset/health-response-sample-scrub.md @@ -2,12 +2,15 @@ "executor": patch --- -**Credentials are kept out of the health-check response sample that gets persisted** +**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 in `connection.last_health`, so whatever the sample carries 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. +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. -Leaves whose key names a credential (`token`, `api_key`, `secret`, `authorization`, `session`, …) now 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. +Two passes now cover both kinds of secret: -The key check reads the nearest named segment of the path rather than its literal last segment, and recognises a plural key. Array elements are named by index, so a bare array of secrets — `{"tokens": ["sk-live-…"]}` — produces the path `tokens.0`: testing the literal `"0"` matched nothing, and a collection of secrets is named in the plural anyway. Both were needed for the value to be redacted; `author` is still not treated as `auth`. +- **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 OpenAPI health check additionally scrubs the connection's own credential value out of each sampled value, covering the other direction: a body that echoes back the key it was authenticated with under an innocent-looking name. +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. diff --git a/packages/core/sdk/src/health-check.ts b/packages/core/sdk/src/health-check.ts index 1c67159e1c..67bfb553dc 100644 --- a/packages/core/sdk/src/health-check.ts +++ b/packages/core/sdk/src/health-check.ts @@ -402,18 +402,55 @@ export const REDACTED_SAMPLE_VALUE = "[redacted]"; 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; -/** True when the nearest NAMED segment of a dotted path names a credential. +/** 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. * - * 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. */ -const namesASecret = (path: string): boolean => { - const named = path.split(".").filter((segment) => !/^\d+$/.test(segment)); + * 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] ?? ""; - return SECRET_KEY_PATTERN.test(leaf); + if (keyNamesASecret(leaf)) return true; + return segments.some( + (segment, index) => + !isIndexSegment(segment) && + isIndexSegment(segments[index + 1] ?? "") && + keyNamesASecret(segment), + ); }; /** @@ -425,14 +462,28 @@ const namesASecret = (path: string): boolean => { * 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; @@ -461,7 +512,7 @@ export const extractResponseFields = (data: unknown): HealthCheckResponseSample[ ) { out.push({ path, - value: namesASecret(path) ? REDACTED_SAMPLE_VALUE : render(String(node)), + value: pathNamesASecret(path) ? REDACTED_SAMPLE_VALUE : render(String(node)), }); } }; diff --git a/packages/core/sdk/src/health-response-sample-redaction.test.ts b/packages/core/sdk/src/health-response-sample-redaction.test.ts index 1e5124b80e..ee39dac65f 100644 --- a/packages/core/sdk/src/health-response-sample-redaction.test.ts +++ b/packages/core/sdk/src/health-response-sample-redaction.test.ts @@ -70,6 +70,89 @@ describe("health-check response sample redaction", () => { 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. diff --git a/packages/core/sdk/src/index.ts b/packages/core/sdk/src/index.ts index aa241f371f..d113ef7018 100644 --- a/packages/core/sdk/src/index.ts +++ b/packages/core/sdk/src/index.ts @@ -128,6 +128,8 @@ export { sortHealthCheckCandidatesByIdentity, projectResponseFields, extractResponseFields, + pathNamesASecret, + REDACTED_SAMPLE_VALUE, identityPathTier, rankResponseSample, } from "./health-check"; diff --git a/packages/plugins/openapi/src/sdk/backing.ts b/packages/plugins/openapi/src/sdk/backing.ts index f5b444ae5e..34cc1d9da3 100644 --- a/packages/plugins/openapi/src/sdk/backing.ts +++ b/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,8 +974,20 @@ 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 @@ -981,14 +995,12 @@ export const checkHealthOpenApi = (input: { // 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. + // 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).map((field) => ({ - ...field, - value: scrubSecrets(field.value), - })) - : []; + status === "healthy" ? extractResponseFields(probe.result.data, { scrub: scrubSecrets }) : []; return { status, httpStatus: probe.result.status, diff --git a/packages/plugins/openapi/src/sdk/health-check-redaction.test.ts b/packages/plugins/openapi/src/sdk/health-check-redaction.test.ts new file mode 100644 index 0000000000..6f56a2d211 --- /dev/null +++ b/packages/plugins/openapi/src/sdk/health-check-redaction.test.ts @@ -0,0 +1,169 @@ +// --------------------------------------------------------------------------- +// What a health check persists. +// +// `connections.validate` writes its result into `connection.last_health`, so +// every field it returns is stored. The probed operation is user-chosen from +// the plugin's own catalog, which means it can be a key-listing endpoint just +// as easily as a `/me` — and the identity field can be pointed at anything the +// picker offered, including a key inside that listing. +// +// This runs the real probe against a real server rather than unit-testing the +// walkers, because the property under test is what survives the whole path into +// the database. The core walkers are covered separately in +// `packages/core/sdk/src/health-response-sample-redaction.test.ts`. +// +// Each `it` shares ONE server and executor across its probes: this file runs +// beside a Graph-scale spec compile that is already near its time budget, so it +// deliberately keeps its own setup cost down. +// --------------------------------------------------------------------------- + +import { describe, expect, it } from "@effect/vitest"; +import { Effect, Schema } from "effect"; +import { FetchHttpClient } from "effect/unstable/http"; +import { HttpApi, HttpApiBuilder, HttpApiEndpoint, HttpApiGroup } from "effect/unstable/httpapi"; + +import { + AuthTemplateSlug, + IntegrationSlug, + REDACTED_SAMPLE_VALUE, + createExecutor, + type HealthCheckResult, +} from "@executor-js/sdk"; +import { makeTestConfig, memoryCredentialsPlugin } from "@executor-js/sdk/testing"; +import { variable } from "@executor-js/sdk/http-auth"; + +import { serveOpenApiHttpApiTestServer } from "../testing"; +import { openApiPlugin } from "./plugin"; + +/** The credential the connection authenticates with. Longer than the sample's + * 120-char value cap on purpose: truncating before scrubbing would leave a + * 120-char prefix of it that an exact-value scrub can no longer match. */ +const CONNECTION_SECRET = `sk-conn-${"c".repeat(200)}`; + +/** A DIFFERENT secret, the kind a key-listing endpoint returns. No scrub of the + * connection's own value can recognise this one; only its key can. */ +const LISTED_SECRET = "sk-live-listed-must-not-be-persisted"; + +const AccountBody = Schema.Struct({ + plan: Schema.String, + api_keys: Schema.Array(Schema.Struct({ name: Schema.String, value: Schema.String })), + /** The body echoing back the key it was authenticated with, under a name that + * says nothing about it. */ + lastRequestNote: Schema.String, +}); + +const AccountApi = HttpApi.make("keyListingApi").add( + HttpApiGroup.make("account").add( + HttpApiEndpoint.get("getAccount", "/account", { success: AccountBody }), + ), +); + +const AccountLive = HttpApiBuilder.group(AccountApi, "account", (handlers) => + handlers.handle("getAccount", () => + Effect.succeed({ + plan: "pro", + api_keys: [{ name: "prod", value: LISTED_SECRET }], + lastRequestNote: `authenticated with ${CONNECTION_SECRET}`, + }), + ), +); + +/** Serve the key-listing API and build an executor once, then hand back a + * `probe` that registers the spec under its own slug (the identity field is + * baked into the integration config) and validates a credential against it. + * The returned value is the `HealthCheckResult` exactly as it is persisted. */ +const withProbe = ( + use: ( + probe: (identityField: string) => Effect.Effect, + ) => Effect.Effect, +) => + Effect.gen(function* () { + const server = yield* serveOpenApiHttpApiTestServer({ + api: AccountApi, + handlersLayer: AccountLive, + }); + const executor = yield* createExecutor( + makeTestConfig({ + plugins: [ + openApiPlugin({ httpClientLayer: FetchHttpClient.layer }), + memoryCredentialsPlugin(), + ] as const, + }), + ); + + let counter = 0; + const probe = (identityField: string) => + Effect.gen(function* () { + const slug = `key_listing_${counter++}`; + yield* executor.openapi.addSpec({ + spec: { kind: "blob", value: server.specJson }, + slug, + baseUrl: server.baseUrl, + healthCheck: { operation: "account.getAccount", identityField }, + authenticationTemplate: [ + { slug: "apiKey", type: "apiKey", headers: { authorization: [variable("token")] } }, + ], + }); + return yield* executor.connections.validate({ + owner: "org", + integration: IntegrationSlug.make(slug), + template: AuthTemplateSlug.make("apiKey"), + value: CONNECTION_SECRET, + }); + }); + + return yield* use(probe); + }).pipe(Effect.scoped); + +describe("openapi health check redaction", () => { + it.effect("redacts the persisted identity, whichever way the secret arrives", () => + withProbe((probe) => + Effect.gen(function* () { + // The identity is read straight off the raw body by `extractIdentity`, + // so it passes through neither the key redaction nor the value scrub + // unless it is put through them explicitly. + + // Key reading: the identity field points inside a credential-named + // array, where only the array's own key says what it holds. + const listed = yield* probe("api_keys.0.value"); + expect(listed.status).toBe("healthy"); + expect(listed.identity).toBe(REDACTED_SAMPLE_VALUE); + + // Value reading: an innocent key name, but the value IS the secret we + // authenticated with. + const echoed = yield* probe("lastRequestNote"); + expect(echoed.identity).toContain(REDACTED_SAMPLE_VALUE); + expect(echoed.identity).not.toContain("sk-conn-"); + + // POSITIVE CONTROL. Without it, a checker that blanked everything would + // satisfy both assertions above. + const plan = yield* probe("plan"); + expect(plan.identity).toBe("pro"); + }), + ), + ); + + it.effect("redacts both listed and echoed secrets in the persisted sample", () => + withProbe((probe) => + Effect.gen(function* () { + const result = yield* probe("plan"); + const sample = Object.fromEntries( + (result.responseSample ?? []).map((row) => [row.path, row.value]), + ); + + // The listed secret: only its enclosing array key names it. + expect(sample["api_keys.0.value"]).toBe(REDACTED_SAMPLE_VALUE); + + // The echoed connection secret: only its exact value identifies it, and + // it is longer than the 120-char cap. Scrubbing after truncation would + // leave a recognisable prefix here. + const note = sample["lastRequestNote"] ?? ""; + expect(note).toContain(REDACTED_SAMPLE_VALUE); + expect(note).not.toContain("sk-conn-"); + + // The preview still works. + expect(sample["plan"]).toBe("pro"); + }), + ), + ); +});