diff --git a/README.md b/README.md index b906e10..ba6aafc 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,9 +309,15 @@ 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 - each other string that is the same value. A value pattern can change + body, the form fields count, also with a nested name such as + `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, 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 body again, and the body must be JSON. Each field must hold `<>` at its place, and no string can be one of the values. @@ -320,16 +328,28 @@ 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. + 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 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. 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/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..fb3c7f8 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"; @@ -85,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 @@ -96,6 +100,30 @@ const CREDENTIAL_NAME = new RegExp( */ 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 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 JWT_HEADER = /(? { } }; -/** - * 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 +297,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 +327,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 +358,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 +377,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 +436,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 +450,27 @@ 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, 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") { + 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 +503,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 +543,33 @@ 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 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`. + */ +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 + * (`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,22 +606,83 @@ 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, 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 = 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; + addValues(item, values); + } + } + if (item !== null && typeof item === "object") pending.push(item); + } + } + return { count, values }; +} + +/** + * Add each string in a parsed JSON value that is not empty or + * `<>`, 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 addValues(json: unknown, values: string[]): void { + const pending: unknown[] = [json]; + while (pending.length > 0) { + const value = pending.pop(); + 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); + } + } } /** * 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. + * 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. */ function redactFormBody( body: string, @@ -574,12 +695,22 @@ 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)) shows = true; + else if ( + isValue(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; + } return !shows; }); if (shows || count < expected) return undefined; @@ -587,6 +718,41 @@ 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`), + * 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 @@ -783,7 +949,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 +1047,66 @@ 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` (`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) || showsToken(body)) return true; + 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))) + ); +} + /** * 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 +1132,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..6ed13eb 100644 --- a/packages/service/tests/redaction.test.ts +++ b/packages/service/tests/redaction.test.ts @@ -368,6 +368,47 @@ describe("redaction", () => { expect(text).not.toContain("api_token=x"); }); + 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.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 }); @@ -471,6 +512,42 @@ 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}&password[]=${HIDDEN}-2&user[password][0]=${HIDDEN}-3¬e=hi`, + { "content-type": "application/x-www-form-urlencoded" }, + ); + await send( + run, + "/v1/fields", + JSON.stringify({ + api_key: [HIDDEN, { v: HIDDEN }], + password: { value: HIDDEN, hint: "" }, + 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=<>&password[]=<>&user[password][0]=<>¬e=hi", + ); + // Each string in the value is replaced at its place. + expect(json).toContain( + '"api_key":["<>",{"v":"<>"}],"note":"hi","password":{"hint":"","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 +1236,17 @@ 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, 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"], @@ -1169,6 +1254,46 @@ 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"], + [`{"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"], + [`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",}`; @@ -1183,9 +1308,21 @@ 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); + 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( + bodies.length - 1, + ); expect(samples.map((sample) => sample.text)).toContainEqual( expect.stringContaining(kept), );