From 786657b182748d1ebca760e39ad3a0f7c802ed60 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 5 Nov 2026 16:37:23 +0000 Subject: [PATCH 1/3] fix(secrets): Redact nested form names and non-string fields, fail closed on more bodies Each of these showed a password, a GitHub token, or a JWT in the text of a request sample and a miss, as a probe of `redactRequest()` on main shows: - A form field with a nested name, such as `user[password]` or `user%5Bpassword%5D`. The form fields now compare by the last part of the name. - A JSON credential field whose value is an array or an object, such as `{"api_key":["..."]}`. Each string in the value is now replaced at its place. A number stays, so a tool schema with a `password` property keeps its shape. A credential field with a number gives no text. - A body whose fields the proxy cannot find: JSON or XML with a form content type, a JSON string, `pass\u0077ord` in JSON that is not valid, and a bare `token` with a credential prefix in a body that is not JSON. The fail-closed check now covers forms without a credential field and JSON scalars, reads names with unicode and URL escapes, and looks for the credential prefixes of `TOKEN_VALUE`. The URL of a recording now also redacts a parameter that holds the value of a credential field of the body, as the sample already did. It reads the body only when the URL has a query or a fragment. `fieldKind()` starts its map again when it is full. Before, the names of one body could fill it for all later runs: 1 M form fields after another body with 5,000 names took 850 ms on main and 576 ms now. No key changes: `request-key.ts` and the redaction of responses do not change. A copy of a field value in other text, such as in a prompt or an escaped tool input, still shows. The README says so. Co-Authored-By: David Cramer <23610+dcramer@users.noreply.github.com> --- README.md | 32 +-- packages/service/src/recorder.ts | 21 +- packages/service/src/secrets.ts | 242 ++++++++++++++++------- packages/service/tests/redaction.test.ts | 81 +++++++- 4 files changed, 286 insertions(+), 90 deletions(-) diff --git a/README.md b/README.md index b906e10..f222f21 100644 --- a/README.md +++ b/README.md @@ -226,7 +226,9 @@ A run takes `RunConfig` (`packages/roach/src/types.ts`): `tenant`, `mode`, `rule Recordings show `<>` in place of each known value. The URL of a recording, a request sample, and the text of a miss in Sentry also show `<>` for the value of each such query parameter, at any length - (see [Misses](#misses)). The other parameters do not change. + (see [Misses](#misses)). They also show it for a parameter whose value is + the value of a credential field of the request body (see below). The + other parameters do not change. A run without the token learns at most 1,024 values, with a total of 1 Mi characters. A run with the token learns at most 16,384 values, with a total of 16 Mi characters, for credentials that change on each @@ -307,8 +309,11 @@ A run takes `RunConfig` (`packages/roach/src/types.ts`): `tenant`, `mode`, `rule `bearer_token`, `client_assertion`, `password`, `api_key`, `secret_key`, `private_key`, and `secret_access_key`. In JSON, these fields count at each depth. In an `application/x-www-form-urlencoded` - body, the form fields count. No key uses this text, so this list can be - longer. The proxy replaces the value at the place of each field, and + body, the form fields count, also with a nested name such as + `user[password]`. No key uses this text, so this list can be + longer. When the value of such a JSON field is an object or an array, + each string in it shows `<>`, at any depth. Its numbers stay. + When the value is a number, the request sample has no text. The proxy replaces the value at the place of each field, and each other string that is the same value. A value pattern can change the text of a key, so then the proxy checks the result. It parses the body again, and the body must be JSON. Each field must hold @@ -320,16 +325,19 @@ A run takes `RunConfig` (`packages/roach/src/types.ts`): `tenant`, `mode`, `rule value of its rule has `"`, `\`, `&`, `=`, `<`, `>`, or a control character, because its `<>` can add a field. A miss has no change then. -- The proxy cannot find the fields of a body that is not JSON or a form, - such as JSON with a comma at the end, JSON lines, or a multipart body. - So the request sample of such a body has no text when the body has the - name of a field above anywhere. The name can be in any case, with `_`, - `-`, or nothing between its words, such as `accessToken`. This is one - search of the body, so a long body costs little. +- The proxy cannot find the fields of a body that is not a JSON object or + array or a form, such as JSON with a comma at the end, JSON lines, a + multipart body, or a JSON string. It also cannot find them in a form body + that has no credential field, such as JSON or XML with a form content + type. So the request sample of such a body has no text when the body has + the name of a field above anywhere, or the start of a bare `token` above. + The name can be in any case, with `_`, `-`, or nothing between its words, + such as `accessToken`, and with JSON unicode escapes or URL escapes in + it. Each test is one search of the body, so a long body costs little. - The proxy parses a JSON body to find the fields, so a long string value - cannot stop it. It does not find a field whose value is not a string, - such as an array, or a field in JSON text that is itself a string, such - as the escaped arguments of a tool call. + cannot stop it. It does not find a field in JSON text that is itself a + string, such as the escaped arguments of a tool call, or a copy of a + value inside other text, such as in a prompt or with URL escapes. - Keys: redaction never changes the key of a request. The key is the hash of the request as the client sent it, so recordings from before stay valid. diff --git a/packages/service/src/recorder.ts b/packages/service/src/recorder.ts index 49c122e..04216cb 100644 --- a/packages/service/src/recorder.ts +++ b/packages/service/src/recorder.ts @@ -45,7 +45,7 @@ import { import { keyRequest, keyTimeLimit, type KeyedRequest } from "./request-key.ts"; import { createSecrets, - redactUrl, + redactRequestUrl, toRedact, type LearnLimit, } from "./secrets.ts"; @@ -257,10 +257,13 @@ function matches( * block, and each value of the request becomes a placeholder. A body that * is not text gets `redactBytes` before it is encoded, because a redaction * of the encoded text cannot find a credential in it. The URL has the - * value of each credential query parameter redacted. + * value of each credential query parameter redacted, and of each + * parameter that holds the value of a credential field of the body + * (`redactRequestUrl`). */ function toRecording( request: ProxyRequest, + rule: RecordingRule, keyed: KeyedRequest, session: string | undefined, response: ProxyResponse, @@ -286,7 +289,10 @@ function toRecording( } return { session, - request: { method: request.method, url: redactUrl(request.url.href) }, + request: { + method: request.method, + url: redactRequestUrl(request, rule), + }, response: { status: response.status, headers, body, bodyEncoding }, }; } @@ -848,7 +854,14 @@ export function createRecorder(config: RecorderConfig, store: RecordingStore) { const recording = JSON.parse( secrets.redact( JSON.stringify( - toRecording(request, keyed, owner?.name, live, secrets.redactBytes), + toRecording( + request, + rule, + keyed, + owner?.name, + live, + secrets.redactBytes, + ), ), ), ) as Recording; diff --git a/packages/service/src/secrets.ts b/packages/service/src/secrets.ts index d74b13b..5500175 100644 --- a/packages/service/src/secrets.ts +++ b/packages/service/src/secrets.ts @@ -14,9 +14,11 @@ * does not run a regular expression on JSON text, because a long string * value can overflow the stack of the expression, and it does not run a * pass for each value, because a client can send many. So it finds a - * field whose name or value has a unicode escape. It does not find a field - * whose value is not a string, such as an array, or a field in JSON text - * that is itself a string, such as the escaped arguments of a tool call. + * field whose name or value has a unicode escape. A field whose value is + * an object or an array has each string in it replaced, and a field whose + * value is a number gives no text. It does not find a field in JSON text + * that is itself a string, such as the escaped arguments of a tool call, + * or a copy of a value inside other text. */ import type { IncomingHttpHeaders } from "node:http"; @@ -96,6 +98,12 @@ const CREDENTIAL_NAME = new RegExp( */ const TOKEN_VALUE = /^(?:gh[pousr]_|github_pat_|xox[abprs]-|eyJ[^.\s"]*\.[^.\s"]*\.)/; +/** + * A credential prefix of `TOKEN_VALUE` at any place of a text. A JWT + * counts from a word start, with a header and a payload that both start + * with `eyJ`, so each character is read once. + */ +const TOKEN_TEXT = /gh[pousr]_|github_pat_|xox[abprs]-|\beyJ\w*\.eyJ/; /** * Tells whether a field or a parameter holds a credential, from its @@ -143,44 +151,6 @@ const parseJson = (text: string): unknown => { } }; -/** - * The string values of the credential fields of a JSON value. With `deep`, - * it looks in each object at each depth, and else only in the object at the - * top. It walks with a list, not with recursion, so a deep value cannot - * overflow the stack. It does not look in the value of a credential field - * that is not a string, such as an array. - */ -function credentialValues( - json: unknown, - test: CredentialTest, - deep: boolean, -): string[] { - const found: string[] = []; - const pending = [json]; - while (pending.length > 0) { - const value = pending.pop(); - if (value === null || typeof value !== "object") continue; - if (Array.isArray(value)) { - // An item of a list has no field name. Look only in its objects. - if (!deep) continue; - for (const item of value) { - if (item !== null && typeof item === "object") pending.push(item); - } - continue; - } - // `for...in`, not `Object.entries`, so an object with many fields does - // not make a list of pairs. A parsed value has only its own fields. - for (const name in value) { - const item = (value as Record)[name]; - if (typeof item === "string" && test(name, item)) found.push(item); - else if (deep && item !== null && typeof item === "object") { - pending.push(item); - } - } - } - return found; -} - const REDACTED_JSON = `"${REDACTED}"`; const QUOTE = 0x22; const BACKSLASH = 0x5c; @@ -307,8 +277,9 @@ function scanStrings( */ type FieldKind = "credential" | "token" | undefined; /** - * The kinds of the names that `fieldKind` saw, with `""` for neither, for - * at most `MAX_KINDS` names of at most `MAX_NAME_LENGTH` characters. + * The kinds of the names that `fieldKind` saw last, with `""` for + * neither, for at most `MAX_KINDS` names of at most `MAX_NAME_LENGTH` + * characters. */ const kinds = new Map(); const MAX_KINDS = 4096; @@ -336,8 +307,10 @@ function fieldKind(name: string): FieldKind { : undefined; // A form can have millions of fields with a few names, and the name of // each costs a regular expression. Only short names are kept, so the - // map stays small. - if (name.length <= MAX_NAME_LENGTH && kinds.size < MAX_KINDS) { + // map stays small. A full map starts again, so the names of one body do + // not take it from all later bodies. + if (name.length <= MAX_NAME_LENGTH) { + if (kinds.size >= MAX_KINDS) kinds.clear(); kinds.set(name, kind ?? ""); } return kind; @@ -365,10 +338,14 @@ function replaceStrings(text: string, values: Set): string { // kind of that field. let valueAt = -1; let field: "credential" | "token" | undefined; - scanStrings(text, (start, end, _depth, name, escapes, length) => { + // The depth of the name of a credential field whose value is an object + // or an array. Each string in that value is replaced. + let inside: number | undefined; + scanStrings(text, (start, end, depth, name, escapes, length) => { const of = valueAt === start ? field : undefined; valueAt = -1; field = undefined; + if (inside !== undefined && depth <= inside) inside = undefined; if (name && length <= MAX_NAME_LENGTH) { const decoded = decodeString(text, start, end, escapes); field = decoded === undefined ? undefined : fieldKind(decoded); @@ -380,10 +357,19 @@ function replaceStrings(text: string, values: Set): string { at += 1; while (isJsonSpace(text.charCodeAt(at))) at += 1; valueAt = at; + if ( + field === "credential" && + inside === undefined && + isOpen(text.charCodeAt(at)) + ) { + inside = depth; + } } } // An empty value shows nothing. - let replace = of === "credential" && end - start > 2; + let replace = + (of === "credential" || (inside !== undefined && !name)) && + end - start > 2; if (!replace && (of === "token" || lengths.has(length))) { const value = decodeString(text, start, end, escapes); replace = @@ -430,7 +416,13 @@ function showsNoField(json: unknown, values: Set): boolean { const item = (value as Record)[name]; if (typeof item === "string") { if (isValue(item) || isCredentialField(name, item)) return false; + } else if (typeof item === "number") { + // A number cannot hold `<>`. + if (fieldKind(name) === "credential") return false; } else if (item !== null && typeof item === "object") { + if (fieldKind(name) === "credential" && !allRedacted(item)) { + return false; + } pending.push(item); } } @@ -438,6 +430,24 @@ function showsNoField(json: unknown, values: Set): boolean { return true; } +/** + * Tells whether each string in a parsed JSON value, such as the value of + * a credential field that is an object, is `<>` or empty. The + * names of its fields and its numbers do not count. It walks with a list. + */ +function allRedacted(json: unknown): boolean { + const pending = [json]; + while (pending.length > 0) { + const value = pending.pop(); + if (typeof value === "string") { + if (value !== "" && value !== REDACTED) return false; + } else if (value !== null && typeof value === "object") { + for (const item of Object.values(value)) pending.push(item); + } + } + return true; +} + /** * Tells whether `redacted` holds `<>` at the place of each * credential field of `json`, the parsed body of the request. The text of @@ -470,6 +480,9 @@ function keepsFields(json: unknown, redacted: unknown): boolean { return false; } } else if (item !== null && typeof item === "object") { + if (fieldKind(name) === "credential" && !allRedacted(fields[name])) { + return false; + } pending.push([item, fields[name]]); } } @@ -507,9 +520,20 @@ function eachPart( } } -/** The decoded name and value of a part of a form body (`eachPart`). */ +/** + * The name of a form field as the field lists compare it: the last part of + * a nested name, such as `password` of `user[password]`, as Rails and PHP + * write it. + */ +const formName = (name: string) => + name.endsWith("]") ? (/\[([^[\]]+)\]$/.exec(name)?.[1] ?? name) : name; + +/** + * The decoded name (`formName`) and value of a part of a form body + * (`eachPart`). + */ function formField(text: string, start: number, equals: number, end: number) { - const name = decode(text.slice(start, equals < 0 ? end : equals)); + const name = formName(decode(text.slice(start, equals < 0 ? end : equals))); const value = equals < 0 ? "" : decode(text.slice(equals + 1, end)); return { name, value }; } @@ -546,11 +570,45 @@ function valueTest(values: string[]): (text: string) => boolean { /** * The credential fields of a parsed JSON body at any depth, as - * `formFields` gives them for a form. `keepsFields` checks each place. + * `formFields` gives them for a form: their number, and the values that + * are strings. A field whose value is a number, an object, or an array + * counts too. `keepsFields` checks each place. It walks with a list, not + * with recursion, so a deep value cannot overflow the stack. */ function jsonFields(json: unknown): { count: number; values: string[] } { - const values = credentialValues(json, isCredentialField, true); - return { count: values.length, values }; + const values: string[] = []; + let count = 0; + const pending = [json]; + while (pending.length > 0) { + const value = pending.pop(); + if (value === null || typeof value !== "object") continue; + if (Array.isArray(value)) { + for (const item of value) { + if (item !== null && typeof item === "object") pending.push(item); + } + continue; + } + // `for...in`, not `Object.entries`, so an object with many fields does + // not make a list of pairs. A parsed value has only its own fields. + for (const name in value) { + const item = (value as Record)[name]; + if (typeof item === "string") { + if (isCredentialField(name, item)) { + values.push(item); + count += 1; + } + continue; + } + if ( + typeof item === "number" || + (item !== null && typeof item === "object") + ) { + if (fieldKind(name) === "credential") count += 1; + } + if (item !== null && typeof item === "object") pending.push(item); + } + } + return { count, values }; } /** @@ -783,7 +841,7 @@ const isForm = (contentType: string | string[] | undefined) => * parameter whose name can mean a credential. The other parts do not * change, and a relative URL stays relative. */ -export function redactUrl( +function redactUrl( href: string, test: CredentialTest = isCredentialParam, ): string { @@ -881,6 +939,60 @@ export function toRedact( }; } +/** + * The body of `request` as `redactRequestWith` reads it: whether it is a + * form, its parsed JSON, its credential fields, and a test for their + * values. + */ +function bodyFields(request: RequestToRedact) { + const form = isForm(request.contentType); + const json = form ? undefined : parseJson(request.body); + const fields = form ? formFields(request.body) : jsonFields(json); + return { form, json, fields, isValue: valueTest(fields.values) }; +} + +/** + * The test of `redactUrl` for the URL of a request: each credential + * parameter, and each parameter whose value is the value of a credential + * field of its body (`isValue`). + */ +const urlTest = + (isValue: (text: string) => boolean): CredentialTest => + (name, value) => + isCredentialParam(name, value) || isValue(value); + +/** + * The URL of a proxied request as a recording keeps it (`redactUrl`), + * with the value of each parameter that is the value of a credential + * field of its body redacted too. It reads the body only for a URL with a + * query or a fragment. + */ +export function redactRequestUrl( + request: { url: URL; headers: IncomingHttpHeaders; body: Buffer }, + rule: RuleToRedact, +): string { + const { href, search, hash } = request.url; + if (search === "" && hash === "") return href; + return redactUrl(href, urlTest(bodyFields(toRedact(request, rule)).isValue)); +} + +/** + * Tells whether the text of a body that `redactRequestWith` cannot read + * field by field can show a credential: it has the name of a field of + * `CREDENTIAL_FIELDS`, also with JSON unicode escapes or URL escapes in + * it, or a credential prefix of a bare `token` (`TOKEN_TEXT`). Each test + * reads the body once. + */ +function mayShowCredential(body: string): boolean { + if (CREDENTIAL_NAME.test(body) || TOKEN_TEXT.test(body)) return true; + const unescaped = body.includes("\\u") + ? body.replaceAll(/\\u([0-9a-fA-F]{4})/g, (_, hex: string) => + String.fromCharCode(Number.parseInt(hex, 16)), + ) + : body; + return CREDENTIAL_NAME.test(unescaped) || CREDENTIAL_NAME.test(decode(body)); +} + /** * The text of `request`, such as the text that its key hashes, with each * of `values` replaced. Give longer values first. It also replaces the @@ -906,23 +1018,17 @@ export function redactRequestWith( text: string, request: RequestToRedact, ): string | undefined { - const form = isForm(request.contentType); - const json = form ? undefined : parseJson(request.body); - const fields = form ? formFields(request.body) : jsonFields(json); - const isValue = valueTest(fields.values); - const href = redactUrl( - request.href, - (name, value) => isCredentialParam(name, value) || isValue(value), - ); + const { form, json, fields, isValue } = bodyFields(request); + const href = redactUrl(request.href, urlTest(isValue)); const result = text.replace(request.href, () => href); if (fields.count === 0) { - // A body that is not JSON or a form, such as JSON with a comma at the - // end, can hold a credential field that the proxy cannot find. So it - // fails closed when the body has the name of one. This is one search - // of the body, so a long body costs little. - if (!form && json === undefined && CREDENTIAL_NAME.test(request.body)) { - return undefined; - } + // A body whose fields the proxy cannot find can hold a credential: + // JSON with a comma at the end, a JSON or XML body with a form content + // type, or JSON text in a JSON string. So it fails closed when the + // body has the name of a credential field or a credential prefix + // (`mayShowCredential`). A JSON object or array shows its fields. + const structured = !form && json !== null && typeof json === "object"; + if (!structured && mayShowCredential(request.body)) return undefined; return redactWith( values, form ? redactForm(result, isCredentialField) : result, diff --git a/packages/service/tests/redaction.test.ts b/packages/service/tests/redaction.test.ts index 266129d..dc93d5d 100644 --- a/packages/service/tests/redaction.test.ts +++ b/packages/service/tests/redaction.test.ts @@ -368,6 +368,20 @@ describe("redaction", () => { expect(text).not.toContain("api_token=x"); }); + it("redacts a parameter of the URL of a recording that holds a credential field of the body", async () => { + const run = await start("auto"); + await send( + run, + `/v1/login?user=me&check=${HIDDEN}`, + JSON.stringify({ password: HIDDEN }), + ); + const { text, file } = await onlyRecording(); + expect(file.request.url).toBe( + `${origin}/v1/login?user=me&check=<>`, + ); + expect(text).not.toContain("SECRET"); + }); + it("redacts a known credential in a binary body before it encodes it", async () => { const run = await start("auto"); const live = await send(run, "/v1/binary", "{}", { "x-api-key": API_KEY }); @@ -471,6 +485,43 @@ describe("redaction", () => { } }); + it("redacts nested form fields and credential fields that are not strings", async () => { + const run = await start("auto"); + await send( + run, + "/v1/login", + `user[name]=me&user%5Bpassword%5D=${HIDDEN}¬e=hi`, + { "content-type": "application/x-www-form-urlencoded" }, + ); + await send( + run, + "/v1/fields", + JSON.stringify({ + api_key: [HIDDEN, { v: HIDDEN }], + password: { value: HIDDEN, length: 18 }, + tools: [{ properties: { password: { type: "string" } } }], + note: "hi", + }), + ); + const samples = [...(await stored("_requests/api")).values()].map( + (text) => JSON.parse(text) as RequestSample, + ); + expect(samples.filter((sample) => sample.withheld)).toEqual([]); + const [form, json] = samples + .map((sample) => sample.text) + .toSorted((a, b) => a.length - b.length); + expect(form).toContain( + "user[name]=me&user%5Bpassword%5D=<>¬e=hi", + ); + // Each string in the value is replaced at its place. A number in it + // stays. + expect(json).toContain( + '"api_key":["<>",{"v":"<>"}],"note":"hi","password":{"length":18,"value":"<>"}', + ); + expect(json).toContain('"password":{"type":"<>"}'); + expect(JSON.stringify(samples)).not.toContain("SECRET"); + }); + it.each(Object.keys(TOOL_INPUTS))( "keeps a %s tool input of a model response, so the next turn replays", async (input) => { @@ -1159,9 +1210,12 @@ describe("redaction", () => { expect(replayed.source).toBe("replayed"); }); - it("writes no text in a request sample when a body that is not JSON has a credential field name", async () => { - // The proxy cannot find the fields of a body that is not JSON or a - // form, so the name of a credential field is enough. + it("writes no text in a request sample when a body without fields that it can read can show a credential", async () => { + // The proxy cannot find the fields of a body that is not a JSON object + // or array, or a form with a credential field. So the name of a + // credential field, also with escapes, or a credential prefix is + // enough. So is a credential field with a number. + const form = "application/x-www-form-urlencoded"; const bodies: [string, string][] = [ [`{"password":"${HIDDEN}",}`, "application/json"], [`{"a":1}\n{"accessToken":"${HIDDEN}"}`, "application/x-ndjson"], @@ -1169,6 +1223,17 @@ describe("redaction", () => { `--b\r\nContent-Disposition: form-data; name="Secret-Access-Key"\r\n\r\n${HIDDEN}\r\n--b--`, "multipart/form-data; boundary=b", ], + [`{"password":"${HIDDEN}"}`, form], + [`${HIDDEN}`, form], + [`payload=%7B%22pass%77ord%22%3A%22${HIDDEN}%22%7D`, form], + [ + JSON.stringify(JSON.stringify({ password: HIDDEN })), + "application/json", + ], + [`{"pass\\u0077ord":"${HIDDEN}",}`, "application/json"], + [`token=${GITHUB_TOKEN}`, "text/plain"], + [`{"token":"${JWT}",}`, "application/json"], + [`{"password":8675309123456789}`, "application/json"], ]; // A body without a name keeps its text. const kept = `{"note":"tokens and keys",}`; @@ -1183,9 +1248,13 @@ describe("redaction", () => { const samples = [...(await stored("_requests/api")).values()].map( (sample) => JSON.parse(sample) as RequestSample, ); - expect(samples).toHaveLength(4); - expect(JSON.stringify(samples)).not.toContain("SECRET"); - expect(samples.filter((sample) => sample.withheld)).toHaveLength(3); + expect(samples).toHaveLength(bodies.length); + for (const secret of ["SECRET", GITHUB_TOKEN, JWT, "8675309"]) { + expect(JSON.stringify(samples)).not.toContain(secret); + } + expect(samples.filter((sample) => sample.withheld)).toHaveLength( + bodies.length - 1, + ); expect(samples.map((sample) => sample.text)).toContainEqual( expect.stringContaining(kept), ); From 630dba4f805de2b41573dfc01dcd999df41b6b2a Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 5 Nov 2026 17:02:20 +0000 Subject: [PATCH 2/3] fix(secrets): Close the nested value, number, and form name gaps in request text Review of the first commit found three partial fixes: - A string in an object or array credential value was redacted in place, but its copies at other places of the body and in the recording URL stayed. Each such string is now a known value of the body. - A number inside an object or array credential value stayed, such as the bytes of a Buffer. Any number in the value now withholds the sample, as a number at the top does. - `password[]` and `user[password][0]` were not credential fields, and a form with another credential field passed `user.password=` and JSON with URL escapes through. The form name is now the last part that is not empty and not a number, and a part that can still show a credential withholds the sample. A value that is only a field name, such as `grant_type=password`, does not count. Also: one bad `%` no longer turns off the URL escape check, and a JWT with `-` in its header counts in text. Both stay linear on 16 MiB bodies. Recording keys do not change. Co-Authored-By: David Cramer <23610+dcramer@users.noreply.github.com> --- README.md | 20 +++- packages/service/src/secrets.ts | 142 ++++++++++++++++++----- packages/service/tests/redaction.test.ts | 78 +++++++++---- 3 files changed, 182 insertions(+), 58 deletions(-) diff --git a/README.md b/README.md index f222f21..1cdf246 100644 --- a/README.md +++ b/README.md @@ -310,11 +310,13 @@ A run takes `RunConfig` (`packages/roach/src/types.ts`): `tenant`, `mode`, `rule `secret_key`, `private_key`, and `secret_access_key`. In JSON, these fields count at each depth. In an `application/x-www-form-urlencoded` body, the form fields count, also with a nested name such as - `user[password]`. No key uses this text, so this list can be - longer. When the value of such a JSON field is an object or an array, - each string in it shows `<>`, at any depth. Its numbers stay. - When the value is a number, the request sample has no text. The proxy replaces the value at the place of each field, and - each other string that is the same value. A value pattern can change + `user[password]`, `password[]`, or `user[password][0]`. No key uses + this text, so this list can be longer. When the value of such a JSON + field is an object or an array, each string in it shows `<>`, + at any depth. When the value is a number, or has a number in it, the + request sample has no text. The proxy replaces the value at the place + of each field, and each other string that is the same value, also each + string of an object or an array value. A value pattern can change the text of a key, so then the proxy checks the result. It parses the body again, and the body must be JSON. Each field must hold `<>` at its place, and no string can be one of the values. @@ -331,13 +333,19 @@ A run takes `RunConfig` (`packages/roach/src/types.ts`): `tenant`, `mode`, `rule that has no credential field, such as JSON or XML with a form content type. So the request sample of such a body has no text when the body has the name of a field above anywhere, or the start of a bare `token` above. + A form field that the proxy cannot read, such as `user.password` or JSON + with URL escapes in a value, also gives no text, in a form with credential + fields too. A value that is only such a name, such as + `grant_type=password`, does not count. The name can be in any case, with `_`, `-`, or nothing between its words, such as `accessToken`, and with JSON unicode escapes or URL escapes in it. Each test is one search of the body, so a long body costs little. - The proxy parses a JSON body to find the fields, so a long string value cannot stop it. It does not find a field in JSON text that is itself a string, such as the escaped arguments of a tool call, or a copy of a - value inside other text, such as in a prompt or with URL escapes. + value inside other text, such as in a prompt or with URL escapes. The URL + of a recording keeps a copy of a value of the body when the proxy cannot + find the fields of the body. - Keys: redaction never changes the key of a request. The key is the hash of the request as the client sent it, so recordings from before stay valid. diff --git a/packages/service/src/secrets.ts b/packages/service/src/secrets.ts index 5500175..9707751 100644 --- a/packages/service/src/secrets.ts +++ b/packages/service/src/secrets.ts @@ -87,10 +87,12 @@ const CREDENTIAL_FIELDS = new Set(CREDENTIAL_NAMES.map(fieldName)); * with `_`, `-`, or nothing between two words, so `accessToken` and * `secret-access_key` match too. */ -const CREDENTIAL_NAME = new RegExp( - CREDENTIAL_NAMES.map((name) => name.split("_").join("[-_]?")).join("|"), - "i", -); +const CREDENTIAL_NAME_SOURCE = CREDENTIAL_NAMES.map((name) => + name.split("_").join("[-_]?"), +).join("|"); +const CREDENTIAL_NAME = new RegExp(CREDENTIAL_NAME_SOURCE, "i"); +/** A text that is only a name of `CREDENTIAL_NAME`. */ +const CREDENTIAL_WORD = new RegExp(`^(?:${CREDENTIAL_NAME_SOURCE})$`, "i"); /** * A value of a bare `token` field that is a credential: a GitHub token, a * JWT, or a Slack token. Model APIs also use `token` for text, such as in @@ -99,11 +101,14 @@ const CREDENTIAL_NAME = new RegExp( const TOKEN_VALUE = /^(?:gh[pousr]_|github_pat_|xox[abprs]-|eyJ[^.\s"]*\.[^.\s"]*\.)/; /** - * A credential prefix of `TOKEN_VALUE` at any place of a text. A JWT - * counts from a word start, with a header and a payload that both start - * with `eyJ`, so each character is read once. + * A credential prefix of `TOKEN_VALUE` at any place of a text: a JWT, with + * a header and a payload that both start with `eyJ`. A header with `-` + * in its base64url counts from the start of a run of base64url + * characters, and a header without `-` counts from a word start, so each + * character is read a few times at most. */ -const TOKEN_TEXT = /gh[pousr]_|github_pat_|xox[abprs]-|\beyJ\w*\.eyJ/; +const TOKEN_TEXT = + /gh[pousr]_|github_pat_|xox[abprs]-|(?): boolean { /** * Tells whether each string in a parsed JSON value, such as the value of - * a credential field that is an object, is `<>` or empty. The - * names of its fields and its numbers do not count. It walks with a list. + * a credential field that is an object, is `<>` or empty, and + * it has no number. A number cannot hold `<>`, and it can be a + * part of a credential, such as a byte of a key. The names of its fields + * do not count. It walks with a list. */ function allRedacted(json: unknown): boolean { const pending = [json]; while (pending.length > 0) { const value = pending.pop(); + if (typeof value === "number") return false; if (typeof value === "string") { if (value !== "" && value !== REDACTED) return false; } else if (value !== null && typeof value === "object") { @@ -522,11 +530,24 @@ function eachPart( /** * The name of a form field as the field lists compare it: the last part of - * a nested name, such as `password` of `user[password]`, as Rails and PHP - * write it. + * a nested name that is not empty and not a number, as Rails and PHP + * write it. So `user[password]`, `password[]`, and `user[password][0]` + * are all `password`. */ -const formName = (name: string) => - name.endsWith("]") ? (/\[([^[\]]+)\]$/.exec(name)?.[1] ?? name) : name; +function formName(name: string): string { + // Read the parts from the end, so a name with many parts costs little. + let close = name.length - 1; + while (name.charCodeAt(close) === 0x5d) { + const open = name.lastIndexOf("[", close); + if (open < 0) break; + const part = name.slice(open + 1, close); + if (part.includes("]")) break; + if (part && !/^\d+$/.test(part)) return part; + close = open - 1; + } + const open = name.indexOf("["); + return open > 0 && name.endsWith("]") ? name.slice(0, open) : name; +} /** * The decoded name (`formName`) and value of a part of a form body @@ -572,8 +593,10 @@ function valueTest(values: string[]): (text: string) => boolean { * The credential fields of a parsed JSON body at any depth, as * `formFields` gives them for a form: their number, and the values that * are strings. A field whose value is a number, an object, or an array - * counts too. `keepsFields` checks each place. It walks with a list, not - * with recursion, so a deep value cannot overflow the stack. + * counts too, and each string in an object or an array is a value, so + * its copies at other places are found. `keepsFields` checks each place. + * It walks with a list, not with recursion, so a deep value cannot + * overflow the stack. */ function jsonFields(json: unknown): { count: number; values: string[] } { const values: string[] = []; @@ -603,7 +626,10 @@ function jsonFields(json: unknown): { count: number; values: string[] } { typeof item === "number" || (item !== null && typeof item === "object") ) { - if (fieldKind(name) === "credential") count += 1; + if (fieldKind(name) === "credential") { + count += 1; + if (typeof item === "object") addStrings(item, values); + } } if (item !== null && typeof item === "object") pending.push(item); } @@ -611,15 +637,33 @@ function jsonFields(json: unknown): { count: number; values: string[] } { return { count, values }; } +/** + * Add each string in a parsed JSON value that is not empty or + * `<>` to `strings`, at any depth, without the names of its + * fields. It walks with a list. + */ +function addStrings(json: object, strings: string[]): void { + const pending: unknown[] = [json]; + while (pending.length > 0) { + const value = pending.pop(); + if (typeof value === "string") { + if (value !== "" && value !== REDACTED) strings.push(value); + } else if (value !== null && typeof value === "object") { + for (const item of Object.values(value)) pending.push(item); + } + } +} + /** * The text of a form body, after a value pattern changed it, with the * value of each credential field replaced, in one pass. It is `undefined` * when the result can still show a value: when a name or a value of - * another field is a credential value (`isValue`), or when fewer than - * `expected` fields hold `<>` (`formFields`). Then a value - * pattern took a field away, such as with its name. A field that held - * `<>` before counts in `expected`, so it cannot take the place - * of that field. + * another field is a credential value (`isValue`), when another field can + * show a credential (`mayShowCredential`), such as `user.password` or + * JSON with URL escapes, or when fewer than `expected` fields hold + * `<>` (`formFields`). Then a value pattern took a field away, + * such as with its name. A field that held `<>` before counts + * in `expected`, so it cannot take the place of that field. */ function redactFormBody( body: string, @@ -637,7 +681,15 @@ function redactFormBody( from = end; count += 1; } else if (value === REDACTED && credentialName(name)) count += 1; - else if (isValue(name) || isValue(value)) shows = true; + else if ( + isValue(name) || + isValue(value) || + // The name of a credential field with an empty value, or a bare + // `token` without a credential prefix, shows no credential. + (!credentialName(name) && partMayShow(body, start, equals, end, value)) + ) { + shows = true; + } return !shows; }); if (shows || count < expected) return undefined; @@ -645,6 +697,28 @@ function redactFormBody( return parts.join(""); } +/** + * Tells whether a part of a form body (`eachPart`) that is not a + * credential field can still show a credential (`mayShowCredential`), + * such as `user.password=` or JSON with URL escapes in its value. A value + * that is only the name of a credential field, such as the `password` of + * `grant_type=password`, shows none. + */ +function partMayShow( + body: string, + start: number, + equals: number, + end: number, + value: string, +): boolean { + if (equals < 0) return mayShowCredential(body.slice(start, end)); + return ( + mayShowCredential(body.slice(start, equals)) || + (!CREDENTIAL_WORD.test(value) && + mayShowCredential(body.slice(equals + 1, end))) + ); +} + /** * Tells whether a value name of a rule can change the structure of the * text of a key. Each value becomes `<>`, so a name with a quote, a @@ -980,17 +1054,23 @@ export function redactRequestUrl( * Tells whether the text of a body that `redactRequestWith` cannot read * field by field can show a credential: it has the name of a field of * `CREDENTIAL_FIELDS`, also with JSON unicode escapes or URL escapes in - * it, or a credential prefix of a bare `token` (`TOKEN_TEXT`). Each test - * reads the body once. + * it, or a credential prefix of a bare `token` (`TOKEN_TEXT`). It decodes + * each escape by itself, so an escape that is not valid does not stop the + * test of the others. Each test reads the body once. */ function mayShowCredential(body: string): boolean { if (CREDENTIAL_NAME.test(body) || TOKEN_TEXT.test(body)) return true; - const unescaped = body.includes("\\u") - ? body.replaceAll(/\\u([0-9a-fA-F]{4})/g, (_, hex: string) => - String.fromCharCode(Number.parseInt(hex, 16)), - ) - : body; - return CREDENTIAL_NAME.test(unescaped) || CREDENTIAL_NAME.test(decode(body)); + const unescape = (pattern: RegExp) => + body.replaceAll(pattern, (_, hex: string) => + String.fromCharCode(Number.parseInt(hex, 16)), + ); + // The names are ASCII, so a URL escape can give one character for each + // byte. + return ( + (body.includes("\\u") && + CREDENTIAL_NAME.test(unescape(/\\u([0-9a-fA-F]{4})/g))) || + (body.includes("%") && CREDENTIAL_NAME.test(unescape(/%([0-9a-fA-F]{2})/g))) + ); } /** diff --git a/packages/service/tests/redaction.test.ts b/packages/service/tests/redaction.test.ts index dc93d5d..60dff42 100644 --- a/packages/service/tests/redaction.test.ts +++ b/packages/service/tests/redaction.test.ts @@ -368,19 +368,33 @@ describe("redaction", () => { expect(text).not.toContain("api_token=x"); }); - it("redacts a parameter of the URL of a recording that holds a credential field of the body", async () => { - const run = await start("auto"); - await send( - run, - `/v1/login?user=me&check=${HIDDEN}`, - JSON.stringify({ password: HIDDEN }), - ); - const { text, file } = await onlyRecording(); - expect(file.request.url).toBe( - `${origin}/v1/login?user=me&check=<>`, - ); - expect(text).not.toContain("SECRET"); - }); + it.each([ + ["a string", HIDDEN], + ["an array", [HIDDEN]], + ["an object", { value: HIDDEN }], + ])( + "redacts a parameter of the URL of a recording that holds a credential field of the body, as %s", + async (_, password) => { + const run = await start("auto"); + await send( + run, + `/v1/login?user=me&check=${HIDDEN}`, + JSON.stringify({ copy: HIDDEN, password }), + ); + const { text, file } = await onlyRecording(); + expect(file.request.url).toBe( + `${origin}/v1/login?user=me&check=<>`, + ); + expect(text).not.toContain("SECRET"); + const [sample] = [...(await stored("_requests/api")).values()]; + const { text: sampleText, withheld } = JSON.parse( + sample!, + ) as RequestSample; + expect(withheld).toBeUndefined(); + expect(sampleText).toContain('"copy":"<>"'); + expect(sampleText).not.toContain("SECRET"); + }, + ); it("redacts a known credential in a binary body before it encodes it", async () => { const run = await start("auto"); @@ -490,7 +504,7 @@ describe("redaction", () => { await send( run, "/v1/login", - `user[name]=me&user%5Bpassword%5D=${HIDDEN}¬e=hi`, + `user[name]=me&user%5Bpassword%5D=${HIDDEN}&password[]=${HIDDEN}-2&user[password][0]=${HIDDEN}-3¬e=hi`, { "content-type": "application/x-www-form-urlencoded" }, ); await send( @@ -498,7 +512,7 @@ describe("redaction", () => { "/v1/fields", JSON.stringify({ api_key: [HIDDEN, { v: HIDDEN }], - password: { value: HIDDEN, length: 18 }, + password: { value: HIDDEN, hint: "" }, tools: [{ properties: { password: { type: "string" } } }], note: "hi", }), @@ -511,12 +525,11 @@ describe("redaction", () => { .map((sample) => sample.text) .toSorted((a, b) => a.length - b.length); expect(form).toContain( - "user[name]=me&user%5Bpassword%5D=<>¬e=hi", + "user[name]=me&user%5Bpassword%5D=<>&password[]=<>&user[password][0]=<>¬e=hi", ); - // Each string in the value is replaced at its place. A number in it - // stays. + // Each string in the value is replaced at its place. expect(json).toContain( - '"api_key":["<>",{"v":"<>"}],"note":"hi","password":{"length":18,"value":"<>"}', + '"api_key":["<>",{"v":"<>"}],"note":"hi","password":{"hint":"","value":"<>"}', ); expect(json).toContain('"password":{"type":"<>"}'); expect(JSON.stringify(samples)).not.toContain("SECRET"); @@ -1214,8 +1227,13 @@ describe("redaction", () => { // The proxy cannot find the fields of a body that is not a JSON object // or array, or a form with a credential field. So the name of a // credential field, also with escapes, or a credential prefix is - // enough. So is a credential field with a number. + // enough. So is a credential field with a number, also in an array or + // an object, and a form field that the proxy cannot read, such as + // `user.password` or JSON with URL escapes. const form = "application/x-www-form-urlencoded"; + // A JWT whose header has `-` in its base64url. + const dashJwt = + "eyJhbGciOiJSUzI1NiIsImtpZCI6In5-fj4-PiJ9.eyJzdWIiOiJhcHAifQ.c2ln"; const bodies: [string, string][] = [ [`{"password":"${HIDDEN}",}`, "application/json"], [`{"a":1}\n{"accessToken":"${HIDDEN}"}`, "application/x-ndjson"], @@ -1234,6 +1252,16 @@ describe("redaction", () => { [`token=${GITHUB_TOKEN}`, "text/plain"], [`{"token":"${JWT}",}`, "application/json"], [`{"password":8675309123456789}`, "application/json"], + [`{"password":[8675309123456789]}`, "application/json"], + [`{"password":{"pin":8675309123456789}}`, "application/json"], + [ + JSON.stringify({ private_key: Buffer.from(HIDDEN) }), + "application/json", + ], + [`user.password=${HIDDEN}&api_key=abc123`, form], + [`api_key=abc123&data=%7B%22password%22%3A%22${HIDDEN}%22%7D`, form], + [`payload=%7B%22pass%77ord%22%3A%22${HIDDEN}%22%7D&pct=100%`, form], + [`see ${dashJwt}`, "text/plain"], ]; // A body without a name keeps its text. const kept = `{"note":"tokens and keys",}`; @@ -1249,7 +1277,15 @@ describe("redaction", () => { (sample) => JSON.parse(sample) as RequestSample, ); expect(samples).toHaveLength(bodies.length); - for (const secret of ["SECRET", GITHUB_TOKEN, JWT, "8675309"]) { + const bytes = [...Buffer.from(HIDDEN)].join(","); + for (const secret of [ + "SECRET", + GITHUB_TOKEN, + JWT, + dashJwt, + "8675309", + bytes, + ]) { expect(JSON.stringify(samples)).not.toContain(secret); } expect(samples.filter((sample) => sample.withheld)).toHaveLength( From 7310ace75acf620da542f4bac76e0bfef8798b79 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Thu, 5 Nov 2026 17:20:12 +0000 Subject: [PATCH 3/3] fix(secrets): Check bare token values, all form name parts, and numeric copies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of the second commit found: - A form field named `token` without a credential prefix skipped the check for a credential in its value, so `token=%7B%22password…` kept its value when the form had another credential field. Only a field of `CREDENTIAL_FIELDS` now skips that check. - A credential value or a JWT in a part of a form name that `formName` does not keep, such as `S[password]=`, stayed. Each part of the name of each field is now checked. - A JWT right after `_`, a letter, a digit, or `-` was not found in text. `showsToken` now finds a run of base64url characters with `eyJ` in it before `.eyJ`, and it reads each run a few times at most. - A number of a credential value that the query copies stayed in the recording URL. The numbers are now known values of the body. Recording keys do not change. Co-Authored-By: David Cramer <23610+dcramer@users.noreply.github.com> --- README.md | 14 ++-- packages/service/src/secrets.ts | 90 ++++++++++++++++-------- packages/service/tests/redaction.test.ts | 32 +++++++++ 3 files changed, 103 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index 1cdf246..ba6aafc 100644 --- a/README.md +++ b/README.md @@ -314,7 +314,8 @@ A run takes `RunConfig` (`packages/roach/src/types.ts`): `tenant`, `mode`, `rule this text, so this list can be longer. When the value of such a JSON field is an object or an array, each string in it shows `<>`, at any depth. When the value is a number, or has a number in it, the - request sample has no text. The proxy replaces the value at the place + request sample has no text, and the URL of the recording shows + `<>` for a parameter with that number. The proxy replaces the value at the place of each field, and each other string that is the same value, also each string of an object or an array value. A value pattern can change the text of a key, so then the proxy checks the result. It parses the @@ -333,13 +334,16 @@ A run takes `RunConfig` (`packages/roach/src/types.ts`): `tenant`, `mode`, `rule that has no credential field, such as JSON or XML with a form content type. So the request sample of such a body has no text when the body has the name of a field above anywhere, or the start of a bare `token` above. - A form field that the proxy cannot read, such as `user.password` or JSON - with URL escapes in a value, also gives no text, in a form with credential - fields too. A value that is only such a name, such as - `grant_type=password`, does not count. The name can be in any case, with `_`, `-`, or nothing between its words, such as `accessToken`, and with JSON unicode escapes or URL escapes in it. Each test is one search of the body, so a long body costs little. + In a form with credential fields, the request sample also has no text + when a field that is not a credential field can show one, such as + `user.password=`, or a bare `token` or another field with JSON with URL + escapes in its value. A value that is only such a name, such as + `grant_type=password`, does not count. It also has no text when a part + of a nested name, such as `S` of `S[password]`, is a credential value, + or when a name has the start of a bare `token`. - The proxy parses a JSON body to find the fields, so a long string value cannot stop it. It does not find a field in JSON text that is itself a string, such as the escaped arguments of a tool call, or a copy of a diff --git a/packages/service/src/secrets.ts b/packages/service/src/secrets.ts index 9707751..fb3c7f8 100644 --- a/packages/service/src/secrets.ts +++ b/packages/service/src/secrets.ts @@ -100,15 +100,30 @@ const CREDENTIAL_WORD = new RegExp(`^(?:${CREDENTIAL_NAME_SOURCE})$`, "i"); */ const TOKEN_VALUE = /^(?:gh[pousr]_|github_pat_|xox[abprs]-|eyJ[^.\s"]*\.[^.\s"]*\.)/; +/** A credential prefix of `TOKEN_VALUE` that is not a JWT. */ +const TOKEN_PREFIX = /gh[pousr]_|github_pat_|xox[abprs]-/; /** - * A credential prefix of `TOKEN_VALUE` at any place of a text: a JWT, with - * a header and a payload that both start with `eyJ`. A header with `-` - * in its base64url counts from the start of a run of base64url - * characters, and a header without `-` counts from a word start, so each - * character is read a few times at most. + * A run of base64url characters that a `.` and `eyJ` follow: the header of + * a JWT, with its payload after it. It starts only where no such + * character comes before, so each run is read a few times at most. */ -const TOKEN_TEXT = - /gh[pousr]_|github_pat_|xox[abprs]-|(? boolean { * The credential fields of a parsed JSON body at any depth, as * `formFields` gives them for a form: their number, and the values that * are strings. A field whose value is a number, an object, or an array - * counts too, and each string in an object or an array is a value, so - * its copies at other places are found. `keepsFields` checks each place. - * It walks with a list, not with recursion, so a deep value cannot - * overflow the stack. + * counts too, and each string and number in its value is a value, so + * its copies at other places are found (`addValues`). `keepsFields` + * checks each place. It walks with a list, not with recursion, so a deep + * value cannot overflow the stack. */ function jsonFields(json: unknown): { count: number; values: string[] } { const values: string[] = []; @@ -628,7 +643,7 @@ function jsonFields(json: unknown): { count: number; values: string[] } { ) { if (fieldKind(name) === "credential") { count += 1; - if (typeof item === "object") addStrings(item, values); + addValues(item, values); } } if (item !== null && typeof item === "object") pending.push(item); @@ -639,15 +654,18 @@ function jsonFields(json: unknown): { count: number; values: string[] } { /** * Add each string in a parsed JSON value that is not empty or - * `<>` to `strings`, at any depth, without the names of its - * fields. It walks with a list. + * `<>`, and each number as decimal text, to `values`, at any + * depth, without the names of its fields. A number gives no text of a + * request sample, but a copy of it in the URL of a recording is + * redacted. It walks with a list. */ -function addStrings(json: object, strings: string[]): void { +function addValues(json: unknown, values: string[]): void { const pending: unknown[] = [json]; while (pending.length > 0) { const value = pending.pop(); - if (typeof value === "string") { - if (value !== "" && value !== REDACTED) strings.push(value); + if (typeof value === "number") values.push(String(value)); + else if (typeof value === "string") { + if (value !== "" && value !== REDACTED) values.push(value); } else if (value !== null && typeof value === "object") { for (const item of Object.values(value)) pending.push(item); } @@ -657,10 +675,11 @@ function addStrings(json: object, strings: string[]): void { /** * The text of a form body, after a value pattern changed it, with the * value of each credential field replaced, in one pass. It is `undefined` - * when the result can still show a value: when a name or a value of - * another field is a credential value (`isValue`), when another field can - * show a credential (`mayShowCredential`), such as `user.password` or - * JSON with URL escapes, or when fewer than `expected` fields hold + * when the result can still show a value: when a name of a field shows a + * credential (`nameShows`), when a value of another field is a credential + * value (`isValue`), when another field can show a credential + * (`partMayShow`), such as `user.password` or JSON with URL escapes, or + * when fewer than `expected` fields hold * `<>` (`formFields`). Then a value pattern took a field away, * such as with its name. A field that held `<>` before counts * in `expected`, so it cannot take the place of that field. @@ -676,17 +695,19 @@ function redactFormBody( let shows = false; eachPart(body, (start, equals, end) => { const { name, value } = formField(body, start, equals, end); - if (isCredentialField(name, value)) { + if (nameShows(body.slice(start, equals < 0 ? end : equals), isValue)) { + shows = true; + } else if (isCredentialField(name, value)) { parts.push(body.slice(from, equals + 1), REDACTED); from = end; count += 1; } else if (value === REDACTED && credentialName(name)) count += 1; else if ( - isValue(name) || isValue(value) || - // The name of a credential field with an empty value, or a bare - // `token` without a credential prefix, shows no credential. - (!credentialName(name) && partMayShow(body, start, equals, end, value)) + // The name of a credential field with an empty value shows no + // credential. A bare `token` without a credential prefix can. + (fieldKind(name) !== "credential" && + partMayShow(body, start, equals, end, value)) ) { shows = true; } @@ -697,6 +718,19 @@ function redactFormBody( return parts.join(""); } +/** + * Tells whether the name of a form field, as it is sent, shows a + * credential: a part of a nested name, such as `S` of `S[password]`, is a + * credential value (`isValue`), or the name has a credential prefix + * (`showsToken`). `formName` keeps only one part, so the other parts are + * checked here. + */ +function nameShows(raw: string, isValue: (text: string) => boolean): boolean { + const name = decode(raw); + if (showsToken(name)) return true; + return name.includes("[") ? name.split(/[[\]]/).some(isValue) : isValue(name); +} + /** * Tells whether a part of a form body (`eachPart`) that is not a * credential field can still show a credential (`mayShowCredential`), @@ -1054,12 +1088,12 @@ export function redactRequestUrl( * Tells whether the text of a body that `redactRequestWith` cannot read * field by field can show a credential: it has the name of a field of * `CREDENTIAL_FIELDS`, also with JSON unicode escapes or URL escapes in - * it, or a credential prefix of a bare `token` (`TOKEN_TEXT`). It decodes + * it, or a credential prefix of a bare `token` (`showsToken`). It decodes * each escape by itself, so an escape that is not valid does not stop the * test of the others. Each test reads the body once. */ function mayShowCredential(body: string): boolean { - if (CREDENTIAL_NAME.test(body) || TOKEN_TEXT.test(body)) return true; + if (CREDENTIAL_NAME.test(body) || showsToken(body)) return true; const unescape = (pattern: RegExp) => body.replaceAll(pattern, (_, hex: string) => String.fromCharCode(Number.parseInt(hex, 16)), diff --git a/packages/service/tests/redaction.test.ts b/packages/service/tests/redaction.test.ts index 60dff42..6ed13eb 100644 --- a/packages/service/tests/redaction.test.ts +++ b/packages/service/tests/redaction.test.ts @@ -396,6 +396,19 @@ describe("redaction", () => { }, ); + it.each([ + ["a number", 8675309], + ["a number in an array", [8675309]], + ])( + "redacts a parameter of the URL of a recording that holds a credential field of the body, as %s", + async (_, password) => { + const run = await start("auto"); + await send(run, "/v1/login?pin=8675309", JSON.stringify({ password })); + const { file } = await onlyRecording(); + expect(file.request.url).toBe(`${origin}/v1/login?pin=<>`); + }, + ); + it("redacts a known credential in a binary body before it encodes it", async () => { const run = await start("auto"); const live = await send(run, "/v1/binary", "{}", { "x-api-key": API_KEY }); @@ -1262,6 +1275,25 @@ describe("redaction", () => { [`api_key=abc123&data=%7B%22password%22%3A%22${HIDDEN}%22%7D`, form], [`payload=%7B%22pass%77ord%22%3A%22${HIDDEN}%22%7D&pct=100%`, form], [`see ${dashJwt}`, "text/plain"], + [`see x_${JWT}`, "text/plain"], + [`see 9${JWT}`, "text/plain"], + [`see -${dashJwt}`, "text/plain"], + // A bare `token` without a credential prefix is not a credential + // field, so its value is checked like any other. + ...[ + "token=%7B%22password%22%3A%22S%22%7D", + "Token=%7B%22password%22%3A%22S%22%7D", + "user[token]=%7B%22password%22%3A%22S%22%7D", + "token=user.password%3DS", + ].map((part): [string, string] => [ + `api_key=abc123&${part.replace("S", HIDDEN)}`, + form, + ]), + // A credential value or a JWT in a part of a form name that + // `formName` does not keep. + [`${HIDDEN}[password]=x&api_key=${HIDDEN}`, form], + [`a[${HIDDEN}][b]=x&api_key=${HIDDEN}`, form], + [`a[${JWT}][b]=x&api_key=abc123`, form], ]; // A body without a name keeps its text. const kept = `{"note":"tokens and keys",}`;