Skip to content

Commit b77ee69

Browse files
fix(health): keep credentials out of the response sample we persist (#1596)
* fix(health): keep credentials out of the response sample we persist 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. * Redact health-sample secrets that only the enclosing array key names * 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. --------- Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com>
1 parent 6225d5e commit b77ee69

6 files changed

Lines changed: 492 additions & 5 deletions

File tree

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
"executor": patch
3+
---
4+
5+
**Credentials are kept out of the health-check result that gets persisted**
6+
7+
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.
8+
9+
Two passes now cover both kinds of secret:
10+
11+
- **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.
12+
- **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.
13+
14+
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.
15+
16+
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‎

Lines changed: 98 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -379,19 +379,111 @@ export const projectResponseFields = (
379379
return fields;
380380
};
381381

382+
/** Placeholder shown instead of a leaf whose key names it as secret-bearing. */
383+
export const REDACTED_SAMPLE_VALUE = "[redacted]";
384+
385+
/**
386+
* Leaf keys whose value is a credential rather than something worth previewing.
387+
*
388+
* The sample exists so a user can pick their identity field, and the keys that
389+
* serve that (`email`, `login`, `username`, `name`, `id`) do not collide with
390+
* any of these — so this can afford to be blunt.
391+
*
392+
* This catches the secrets we do NOT already know. Scrubbing the connection's
393+
* own credential value out of the sample only helps when the body echoes the
394+
* key we authenticated with; a health check pointed at a key-listing endpoint
395+
* returns different secrets entirely, and no scrub of a known value can see
396+
* those.
397+
*/
398+
/* The optional trailing `s` matters because of the array case below: a key that
399+
* holds a COLLECTION of secrets is named in the plural (`tokens`, `api_keys`,
400+
* `credentials`), and those are exactly the key-listing responses this guards.
401+
* It stays inside the same letter boundary, so `author` is still not `auth`. */
402+
const SECRET_KEY_PATTERN =
403+
/(^|[^a-z])(secret|token|password|passwd|apikey|api_key|credential|authorization|auth|session|cookie|private|signature|bearer|refresh)s?([^a-z]|$)/i;
404+
405+
/** camelCase hides the separator the pattern looks for: `accessToken` has no
406+
* non-letter before `Token`, so `accessToken`, `refreshToken`, `clientSecret`,
407+
* `privateKey` and `sessionId` all read as innocent. Splitting on a
408+
* lowercase→uppercase transition exposes that boundary. */
409+
const splitCamelCase = (key: string): string => key.replaceAll(/([a-z0-9])([A-Z])/g, "$1 $2");
410+
411+
/** True when a single key names a credential.
412+
*
413+
* BOTH spellings are tested, not just the split one: `apiKey` matches only as
414+
* the contiguous word `apikey`, which splitting would destroy. Testing both
415+
* only widens the net where a case boundary exists, so `author` and
416+
* `authorName` — which have no boundary in the wrong place — stay visible. */
417+
const keyNamesASecret = (key: string): boolean =>
418+
SECRET_KEY_PATTERN.test(key) || SECRET_KEY_PATTERN.test(splitCamelCase(key));
419+
420+
const isIndexSegment = (segment: string): boolean => /^\d+$/.test(segment);
421+
422+
/** True when a dotted path names a credential, under either of two readings.
423+
*
424+
* THE NEAREST NAMED SEGMENT. The walker names array elements by index, so a
425+
* bare array of secrets — `{"tokens": ["sk-live-…"]}` — produces the path
426+
* `tokens.0`, whose literal last segment is `"0"` and matches nothing. Numeric
427+
* segments are skipped so the check lands on the key that named the
428+
* collection, which is the only place the secret is described. `keys.0.token`
429+
* is unaffected: its last segment already names the leaf.
430+
*
431+
* AN ENCLOSING ARRAY CONTAINER. A key-listing endpoint returns
432+
* `{"api_keys": [{"value": "sk-live-…"}]}`, whose path is `api_keys.0.value`.
433+
* The nearest named segment is the innocent `value`, so the first reading
434+
* alone hands the secret to the database. A segment that DIRECTLY contains an
435+
* array — one immediately followed by an index — and names a credential
436+
* therefore covers everything under it.
437+
*
438+
* That second reading also blanks a sibling like `api_keys.0.name`. This is
439+
* the right trade: a name inside a key list is never the connection's own
440+
* identity (`candidateIdentityTier` already refuses indexed paths), and the
441+
* alternative is a persisted secret. `names.0` is untouched, because its
442+
* container names nothing. */
443+
export const pathNamesASecret = (path: string): boolean => {
444+
const segments = path.split(".");
445+
const named = segments.filter((segment) => !isIndexSegment(segment));
446+
const leaf = named[named.length - 1] ?? "";
447+
if (keyNamesASecret(leaf)) return true;
448+
return segments.some(
449+
(segment, index) =>
450+
!isIndexSegment(segment) &&
451+
isIndexSegment(segments[index + 1] ?? "") &&
452+
keyNamesASecret(segment),
453+
);
454+
};
455+
382456
/**
383457
* Walk an actual JSON response body and return its scalar leaves as
384458
* `{ path, value }` rows (value stringified + truncated). Drives the live
385459
* preview's "show me what this returns" list. Bounded to depth 4, 25 fields,
386460
* and ~120-char values.
461+
*
462+
* Leaves whose key names a credential are kept but their value is replaced,
463+
* so the preview still shows the field exists without persisting its value —
464+
* this result is written to `connection.last_health`.
465+
*
466+
* `scrub` removes credential values the caller already knows (the secret the
467+
* connection authenticated with, which a body can echo back under an
468+
* innocent-looking key). It runs INSIDE the walk, before truncation, because
469+
* the two orders are not equivalent: a credential longer than the value cap
470+
* would otherwise be cut to a 120-char prefix that an exact-substring scrub can
471+
* no longer recognise, and that prefix is what gets persisted.
387472
*/
388-
export const extractResponseFields = (data: unknown): HealthCheckResponseSample[] => {
473+
export const extractResponseFields = (
474+
data: unknown,
475+
options?: { readonly scrub?: (value: string) => string },
476+
): HealthCheckResponseSample[] => {
389477
const out: HealthCheckResponseSample[] = [];
390478
const MAX_DEPTH = 4;
391479
const MAX_FIELDS = 25;
392480
const MAX_VALUE = 120;
481+
const scrub = options?.scrub;
393482

394-
const render = (v: string): string => (v.length > MAX_VALUE ? `${v.slice(0, MAX_VALUE)}...` : v);
483+
const render = (raw: string): string => {
484+
const v = scrub === undefined ? raw : scrub(raw);
485+
return v.length > MAX_VALUE ? `${v.slice(0, MAX_VALUE)}...` : v;
486+
};
395487

396488
const visit = (node: unknown, path: string, depth: number) => {
397489
if (out.length >= MAX_FIELDS || node == null) return;
@@ -418,7 +510,10 @@ export const extractResponseFields = (data: unknown): HealthCheckResponseSample[
418510
path !== "" &&
419511
(typeof node === "string" || typeof node === "number" || typeof node === "boolean")
420512
) {
421-
out.push({ path, value: render(String(node)) });
513+
out.push({
514+
path,
515+
value: pathNamesASecret(path) ? REDACTED_SAMPLE_VALUE : render(String(node)),
516+
});
422517
}
423518
};
424519

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
// A health check writes its response sample into `connection.last_health`, so
2+
// whatever the sample carries is persisted. The operation being probed is
3+
// user-chosen from the plugin's catalog, which means it can be a key-listing
4+
// endpoint just as easily as a `/me`.
5+
//
6+
// These use real response bodies of the shape those endpoints return, because
7+
// the property under test is what survives the walk into the database.
8+
9+
import { describe, expect, it } from "@effect/vitest";
10+
11+
import { extractResponseFields, REDACTED_SAMPLE_VALUE } from "./health-check";
12+
13+
/** The sample as a path -> value lookup, which is how the assertions read. */
14+
const byPath = (data: unknown): Record<string, string> =>
15+
Object.fromEntries(extractResponseFields(data).map((f) => [f.path, f.value]));
16+
17+
describe("health-check response sample redaction", () => {
18+
it("redacts credential-named leaves while keeping the identity fields", () => {
19+
const fields = byPath({
20+
email: "alex@example.com",
21+
login: "alex",
22+
id: 4711,
23+
api_key: "sk-live-must-not-be-persisted",
24+
refresh_token: "rt-must-not-be-persisted",
25+
session: "sess-must-not-be-persisted",
26+
});
27+
28+
// The reason the sample exists still works.
29+
expect(fields.email).toBe("alex@example.com");
30+
expect(fields.login).toBe("alex");
31+
expect(fields.id).toBe("4711");
32+
33+
expect(fields.api_key).toBe(REDACTED_SAMPLE_VALUE);
34+
expect(fields.refresh_token).toBe(REDACTED_SAMPLE_VALUE);
35+
expect(fields.session).toBe(REDACTED_SAMPLE_VALUE);
36+
});
37+
38+
it("redacts nested and array-borne credentials, not just top-level ones", () => {
39+
// What a key-listing endpoint actually returns. This is the case a scrub
40+
// of the connection's own value cannot catch: these are different secrets.
41+
const fields = byPath({
42+
keys: [
43+
{ name: "prod", token: "sk-prod-must-not-be-persisted" },
44+
{ name: "staging", token: "sk-staging-must-not-be-persisted" },
45+
],
46+
account: { billing: { secret: "whsec-must-not-be-persisted" } },
47+
});
48+
49+
expect(fields["keys.0.name"]).toBe("prod");
50+
expect(fields["keys.0.token"]).toBe(REDACTED_SAMPLE_VALUE);
51+
expect(fields["keys.1.token"]).toBe(REDACTED_SAMPLE_VALUE);
52+
expect(fields["account.billing.secret"]).toBe(REDACTED_SAMPLE_VALUE);
53+
});
54+
55+
it("redacts a bare array of secrets, where only the enclosing key names them", () => {
56+
// `{"tokens": ["sk-…"]}` yields the paths `tokens.0`, `tokens.1`. Their last
57+
// segment is an array index, so a check that reads the literal leaf finds
58+
// "0" and lets the secret straight through into `connection.last_health`.
59+
const fields = byPath({
60+
tokens: ["sk-live-must-not-be-persisted", "sk-test-must-not-be-persisted"],
61+
names: ["prod", "staging"],
62+
});
63+
64+
expect(fields["tokens.0"]).toBe(REDACTED_SAMPLE_VALUE);
65+
expect(fields["tokens.1"]).toBe(REDACTED_SAMPLE_VALUE);
66+
67+
// The index walk-back stops at the nearest NAMED segment, so an innocent
68+
// collection is still shown in full.
69+
expect(fields["names.0"]).toBe("prod");
70+
expect(fields["names.1"]).toBe("staging");
71+
});
72+
73+
it("redacts camelCase credential keys, in both spellings", () => {
74+
// `accessToken` has no non-letter before `Token`, so a matcher that only
75+
// looks for a separator reads it as innocent. camelCase is the dominant
76+
// spelling in JSON APIs, so this is not an edge case.
77+
const fields = byPath({
78+
accessToken: "at-must-not-be-persisted",
79+
refreshToken: "rt-must-not-be-persisted",
80+
clientSecret: "cs-must-not-be-persisted",
81+
privateKey: "pk-must-not-be-persisted",
82+
sessionId: "sid-must-not-be-persisted",
83+
// Still matched as one contiguous word, which is how it has always
84+
// matched: splitting on the case boundary alone would lose it.
85+
apiKey: "ak-must-not-be-persisted",
86+
});
87+
88+
for (const value of Object.values(fields)) {
89+
expect(value).toBe(REDACTED_SAMPLE_VALUE);
90+
}
91+
});
92+
93+
it("does not redact camelCase keys that merely start with a matching word", () => {
94+
// The other direction of the same change: exposing the case boundary must
95+
// not turn ordinary fields into blanks.
96+
const fields = byPath({
97+
author: "alex",
98+
authors: "alex, sam",
99+
authorName: "Alex",
100+
privacyLevel: "public",
101+
tokenizer: "bpe",
102+
});
103+
104+
expect(fields.author).toBe("alex");
105+
expect(fields.authors).toBe("alex, sam");
106+
expect(fields.authorName).toBe("Alex");
107+
expect(fields.privacyLevel).toBe("public");
108+
expect(fields.tokenizer).toBe("bpe");
109+
});
110+
111+
it("redacts a secret under an innocent leaf inside a credential-named array", () => {
112+
// The shape a key-listing endpoint actually returns. The nearest named
113+
// segment is the harmless `value`; only the array's own key says what the
114+
// collection holds.
115+
const fields = byPath({
116+
api_keys: [{ name: "prod", value: "sk-live-must-not-be-persisted" }],
117+
names: ["prod", "staging"],
118+
results: [{ value: "42" }],
119+
});
120+
121+
expect(fields["api_keys.0.value"]).toBe(REDACTED_SAMPLE_VALUE);
122+
123+
// An array whose key names nothing is untouched, at either depth.
124+
expect(fields["names.0"]).toBe("prod");
125+
expect(fields["results.0.value"]).toBe("42");
126+
});
127+
128+
it("scrubs a known credential value before truncating, not after", () => {
129+
// A credential can sit under a key that names nothing — the body echoing
130+
// back the key it was authenticated with. Only the caller can recognise it,
131+
// by exact value. Truncating first cuts it to a 120-char prefix that the
132+
// exact-value scrub no longer matches, and that prefix is what reaches
133+
// `connection.last_health`.
134+
const secret = `sk-live-${"x".repeat(200)}`;
135+
expect(secret.length).toBeGreaterThan(120);
136+
137+
const sample = extractResponseFields(
138+
{ data: secret },
139+
{ scrub: (value) => value.split(secret).join(REDACTED_SAMPLE_VALUE) },
140+
);
141+
142+
expect(sample[0]?.value).toBe(REDACTED_SAMPLE_VALUE);
143+
// Not merely "shorter than the secret": assert no recognisable prefix of it
144+
// survived, which is the actual leak.
145+
expect(sample[0]?.value).not.toContain("sk-live-");
146+
});
147+
148+
it("still truncates a long value the scrub does not recognise", () => {
149+
// Reordering must not disable the cap for everything else.
150+
const long = "y".repeat(400);
151+
const sample = extractResponseFields({ blob: long }, { scrub: (value) => value });
152+
153+
expect(sample[0]?.value).toBe(`${"y".repeat(120)}...`);
154+
});
155+
156+
it("keeps the field visible so the preview still shows the shape", () => {
157+
// Dropping the row would change what the picker displays. Redacting the
158+
// value keeps the response shape legible without persisting the secret.
159+
const sample = extractResponseFields({ api_key: "sk-live-x" });
160+
161+
expect(sample).toHaveLength(1);
162+
expect(sample[0]?.path).toBe("api_key");
163+
});
164+
165+
it("does not redact identity keys that merely contain a matching substring", () => {
166+
// `author` contains "auth". Matching it would silently blank a normal
167+
// field, which is how an over-eager redactor makes the feature useless.
168+
const fields = byPath({ author: "alex", authorization: "Bearer x" });
169+
170+
expect(fields.author).toBe("alex");
171+
expect(fields.authorization).toBe(REDACTED_SAMPLE_VALUE);
172+
});
173+
174+
it("POSITIVE CONTROL: an unredacted body does come through verbatim", () => {
175+
// Proves these assertions can fail. Without it, an extractor that returned
176+
// nothing, or redacted everything, would satisfy the checks above.
177+
const fields = byPath({ email: "alex@example.com", plan: "pro" });
178+
179+
expect(fields.email).toBe("alex@example.com");
180+
expect(fields.plan).toBe("pro");
181+
expect(Object.values(fields)).not.toContain(REDACTED_SAMPLE_VALUE);
182+
});
183+
});

‎packages/core/sdk/src/index.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,8 @@ export {
128128
sortHealthCheckCandidatesByIdentity,
129129
projectResponseFields,
130130
extractResponseFields,
131+
pathNamesASecret,
132+
REDACTED_SAMPLE_VALUE,
131133
identityPathTier,
132134
rankResponseSample,
133135
} from "./health-check";

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

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,9 @@ import {
1212
sortHealthCheckCandidatesByIdentity,
1313
extractIdentity,
1414
extractResponseFields,
15+
pathNamesASecret,
1516
projectResponseFields,
17+
REDACTED_SAMPLE_VALUE,
1618
type HealthCheckCandidate,
1719
type HealthCheckResponseField,
1820
type HealthCheckResult,
@@ -972,13 +974,33 @@ export const checkHealthOpenApi = (input: {
972974
}
973975

974976
const status = classifyHttpStatus(probe.result.status);
975-
const identity =
977+
const rawIdentity =
976978
status === "healthy" ? extractIdentity(probe.result.data, spec.identityField) : undefined;
979+
// The identity is read straight off the raw body, so unlike the sample it
980+
// passes through neither redaction pass — and it is persisted to
981+
// `connection.last_health` just the same. `identityField` is user-chosen
982+
// from whatever the picker listed, which on a key-listing endpoint includes
983+
// `api_keys.0.value`. Run both passes over it: the key reading first, then
984+
// the known-value scrub.
985+
const identity =
986+
rawIdentity === undefined
987+
? undefined
988+
: pathNamesASecret(spec.identityField ?? "")
989+
? REDACTED_SAMPLE_VALUE
990+
: scrubSecrets(rawIdentity);
977991
// Sample the returned body ONLY on a healthy probe: the sample exists to
978992
// pick an identity field, and error bodies (upstream internals, auth error
979993
// envelopes) have no business in the preview. Non-healthy runs carry the
980994
// classified `detail` instead.
981-
const responseSample = status === "healthy" ? extractResponseFields(probe.result.data) : [];
995+
// Same scrub the `detail` branch below uses, for the same reason: a body
996+
// can echo back the key it was authenticated with. `extractResponseFields`
997+
// already redacts leaves whose KEY names a credential; this covers the
998+
// other direction, a credential value under an innocent-looking key. It is
999+
// handed to the walker rather than mapped over the result so it runs before
1000+
// the 120-char truncation, which would otherwise leave an unrecognisable —
1001+
// and unscrubbable — prefix of a long secret.
1002+
const responseSample =
1003+
status === "healthy" ? extractResponseFields(probe.result.data, { scrub: scrubSecrets }) : [];
9821004
return {
9831005
status,
9841006
httpStatus: probe.result.status,

0 commit comments

Comments
 (0)