Repository navigation
fix(telemetry): mask error text after credential words #1619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c1e089e
4f2ddd6
f4cf548
2c99e1a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import stripAnsi from "strip-ansi" | ||
|
|
||
| // JavaScript string lengths are UTF-16 code units, matching maskString's historical limits. | ||
| export const MAX_INPUT = 8192 | ||
| export const LOOKAHEAD = 2048 | ||
|
|
||
| /** | ||
| * Shared bounded ANSI entry point with explicit compatibility profiles. | ||
| * `legacy` preserves maskString's 7-bit rules and existing lookahead window, so hashes | ||
| * do not change. `template` handles the broader strip-ansi grammar on the original first | ||
| * line. An oversized first line returns undefined: callers must redact the whole line, | ||
| * rather than emit a prefix whose ANSI sequence or credential marker may have been cut. | ||
| */ | ||
| export function normalize(raw: string, profile: "legacy"): string | ||
| export function normalize(raw: string, profile: "template"): string | undefined | ||
| export function normalize(raw: string, profile: "legacy" | "template"): string | undefined { | ||
| if (profile === "legacy") { | ||
| // maskString already supplies at most MAX_INPUT + LOOKAHEAD code units. Keep this | ||
| // historical regex frozen; adopting the template profile here would change hashes. | ||
| return raw | ||
| .slice(0, MAX_INPUT + LOOKAHEAD) | ||
| .replace(/\x1b(?:\[[0-?]*[ -\/]*[@-~]|\][^\x07\x1b]*(?:\x07|\x1b\\))/g, "") | ||
| } | ||
| // One extra code unit distinguishes an exact-limit line followed by a newline from | ||
| // an oversized line. Never scan the unbounded input or strip across a line boundary. | ||
| const first = raw.slice(0, MAX_INPUT + 1).split(/[\r\n\u2028\u2029]/, 1)[0] | ||
| if (first.length > MAX_INPUT) return undefined | ||
| return stripAnsi(first) | ||
| } | ||
|
|
||
| export * as Ansi from "./ansi" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,42 @@ | ||
| import { Telemetry } from "./index" | ||
| import { Ansi } from "./ansi" | ||
|
|
||
| const MAX_MESSAGE = 200 | ||
| const PLACEHOLDER = "<x>" | ||
| export const CREDENTIAL_WORDS: ReadonlySet<string> = new Set([ | ||
| "accesstoken", | ||
| "apikey", | ||
| "appsecret", | ||
| "auth", | ||
| "authorization", | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
Resolved
|
||
| "authtoken", | ||
| "bearer", | ||
| "bearertoken", | ||
| "clientsecret", | ||
| "connectionstring", | ||
| "cookie", | ||
| "credential", | ||
| "credentials", | ||
| "jwtsecret", | ||
| "jwttoken", | ||
| "key", | ||
| "pass", | ||
| "passphrase", | ||
| "passwd", | ||
| "password", | ||
| "passwords", | ||
| "privatekey", | ||
| "pwd", | ||
| "refreshtoken", | ||
| "secret", | ||
| "secretkey", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: The camelCase expansion skips
|
||
| "session", | ||
| "sessionid", | ||
| "sig", | ||
| "signature", | ||
| "token", | ||
| ]) | ||
|
|
||
| /** | ||
| * Fixed vocabulary of common error and diagnostic words. Credential words (password, | ||
| * secret, token, key, auth, cookie, session, ...) are deliberately absent. Keep sorted. | ||
|
|
@@ -373,20 +408,29 @@ export const ERROR_VOCABULARY: ReadonlySet<string> = new Set([ | |
| ]) | ||
|
|
||
| /** | ||
| * Fixed-vocabulary template of an error's first line. Words are split on any non-letter | ||
| * character and lowercased; a word survives only if it is in ERROR_VOCABULARY, every other | ||
| * run becomes `<x>`. The output holds only vocabulary words, `<x>` and spaces, so user | ||
| * text, identifiers, paths, values and SQL cannot appear. | ||
| * Fixed-vocabulary template of an error's first line, with ANSI escapes removed. | ||
| * Words are split on any non-letter character and lowercased; a word survives only if | ||
| * it is in ERROR_VOCABULARY, every other run becomes `<x>`. A credential word and the | ||
| * rest of the line become one `<x>` too, because a credential value may itself consist | ||
| * of vocabulary words. This deliberately loses diagnostic text after ambiguous words | ||
| * such as key or session: privacy takes priority over template detail. The output holds | ||
| * only vocabulary words, `<x>` and spaces. Raw first lines over 8192 UTF-16 code units | ||
| * become `<x>` in full; partially normalized prefixes could cut ANSI or credential keys. | ||
| */ | ||
| export function templateErrorMessage(raw: string): string { | ||
| const first = raw.split(/[\r\n\u2028\u2029]/, 1)[0] | ||
| const line = Ansi.normalize(raw, "template") | ||
| if (line === undefined) return PLACEHOLDER | ||
| // Use the tokenizer's letter boundaries; spaced keys require an assignment separator. | ||
| const first = line | ||
| .replace(/(?<![A-Za-z])connection[_-]string(?![A-Za-z])/gi, "connectionstring") | ||
| .replace(/(?<![A-Za-z])connection[ \t]+string["'`]?[ \t]*(?=[:=])/gi, "connectionstring") | ||
| const out: string[] = [] | ||
| for (const token of first.split(/[^A-Za-z]+/)) { | ||
| if (!token) continue | ||
| const word = token.toLowerCase() | ||
| const piece = ERROR_VOCABULARY.has(word) ? word : PLACEHOLDER | ||
| if (piece === PLACEHOLDER && out[out.length - 1] === PLACEHOLDER) continue | ||
| out.push(piece) | ||
| if (piece !== PLACEHOLDER || out[out.length - 1] !== PLACEHOLDER) out.push(piece) | ||
| if (CREDENTIAL_WORDS.has(word)) break | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Breaking at every credential word collapses several common error templates the vocabulary was explicitly built for. The tradeoff is intentional per the PR description; consider stating that cost in the doc comment above, or excluding the ambiguous non-value nouns (
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
| } | ||
| // non-letter characters (digits, symbols) only separate words and are dropped; unknown words they separated collapse into one placeholder | ||
| return out.join(" ").slice(0, MAX_MESSAGE) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| import { describe, expect, test } from "bun:test" | ||
| import { createHash } from "node:crypto" | ||
| import { Telemetry } from "../../src/altimate/telemetry" | ||
| import { ErrorDetail } from "../../src/altimate/telemetry/error-detail" | ||
| import { Ansi } from "../../src/altimate/telemetry/ansi" | ||
|
|
||
| const cases = [ | ||
| ["plain", "request failed: password is connection refused"], | ||
| ["csi", "request failed: \x1b[31mpassword\x1b[0m is connection refused"], | ||
| ["c1", "request failed: \x9b31mpassword\x9b0m is connection refused"], | ||
| ["osc-bel", "\x1b]0;synthetic title\x07password=wrong user"], | ||
| ["osc-st", "\x1b]0;synthetic title\x1b\\password=wrong user"], | ||
| ["osc-c1-st", "\x1b]0;synthetic title\x9cpassword=wrong user"], | ||
| ["incomplete-osc", "request failed \x1b]8;;wrong user"], | ||
| ["incomplete-csi", "password=wrong user \x1b[31"], | ||
| ["multiline", "connection refused\npassword=wrong user"], | ||
| ["at-cap", "error ".repeat(1638) + "xx"], | ||
| ["beyond-cap", "error ".repeat(2000) + "password=wrong user"], | ||
| ["open-quote", "error ".repeat(1300) + '"' + "connection ".repeat(500)], | ||
| ["path-cut", "error ".repeat(1300) + "/Users/synthetic/a ".repeat(200)], | ||
| ["lookahead", "x ".repeat(4093) + "password=wrong user"], | ||
| ["unbroken", "x".repeat(12000)], | ||
| ] | ||
|
|
||
| const pieces = [ | ||
| "connection refused", | ||
| "password=wrong user", | ||
| "'wrong user'", | ||
| '"connection refused"', | ||
| "/Users/synthetic/file.sql", | ||
| "C:\\Users\\synthetic\\file.sql", | ||
| "\x1b[31m", | ||
| "\x9b31m", | ||
| "\x1b]8;;https://example.test\x07", | ||
| "\n", | ||
| "token=wrong user", | ||
| "connection string=wrong user", | ||
| ] | ||
| const corpus = Array.from({ length: 96 }, (_, i) => | ||
| pieces | ||
| .map((_, j) => pieces[(i * (j + 1) + j * 7) % pieces.length]) | ||
| .join(i % 2 ? " " : "_") | ||
| .repeat(i % 5 === 0 ? 300 : 1), | ||
| ) | ||
|
|
||
| const hashes: Record<string, string> = { | ||
| plain: "ae7e754cfb2d72a2", | ||
| csi: "ae7e754cfb2d72a2", | ||
| c1: "40678db384156848", | ||
| "osc-bel": "914eb30c45c8b094", | ||
| "osc-st": "914eb30c45c8b094", | ||
| "osc-c1-st": "bd974ba379a9b36f", | ||
| "incomplete-osc": "821db0d35dce78e7", | ||
| "incomplete-csi": "bb977ba58d3d2d21", | ||
| multiline: "7ee1b11bcf0c4db1", | ||
| "at-cap": "80b18d6fbcfa3063", | ||
| "beyond-cap": "80b18d6fbcfa3063", | ||
| "open-quote": "80b18d6fbcfa3063", | ||
| "path-cut": "80b18d6fbcfa3063", | ||
| lookahead: "bf3ce936232f73c1", | ||
| unbroken: "e3b0c44298fc1c14", | ||
| } | ||
|
|
||
| describe("bounded ANSI profiles", () => { | ||
| test("shares the historical input and lookahead limits", () => { | ||
| expect(Ansi.MAX_INPUT).toBe(8192) | ||
| expect(Ansi.LOOKAHEAD).toBe(2048) | ||
| }) | ||
|
|
||
| test.each([ | ||
| ["CSI", "\x1b[31mpassword\x1b[0m=wrong user"], | ||
| ["C1 CSI", "\x9b31mpassword\x9b0m=wrong user"], | ||
| ["OSC BEL", "\x1b]0;synthetic\x07password=wrong user"], | ||
| ["OSC ST", "\x1b]0;synthetic\x1b\\password=wrong user"], | ||
| ["OSC C1 ST", "\x1b]0;synthetic\x9cpassword=wrong user"], | ||
| ])("normalizes the template's %s sequences", (_, raw) => { | ||
| expect(Ansi.normalize(raw, "template")).toBe("password=wrong user") | ||
| }) | ||
|
|
||
| test.each([ | ||
| ["text", "x".repeat(8193)], | ||
| ["ANSI-only text", "\x1b[0m".repeat(3000)], | ||
| ["unfinished CSI", "\x1b[" + "0;".repeat(5000)], | ||
| ["OSC closing past the cap", "\x1b]0;" + "x".repeat(8192) + "\x07"], | ||
| ["credential marker crossing the cap", " ".repeat(8190) + "password=wrong user"], | ||
| ["large unfinished OSC", "\x1b]0;" + "x".repeat(2_000_000)], | ||
| ])("rejects oversized %s before stripping", (_, raw) => { | ||
| expect(Ansi.normalize(raw, "template")).toBeUndefined() | ||
| }) | ||
|
|
||
| test.each(["\n", "\r", "\r\n", "\u2028", "\u2029"])( | ||
| "preserves the original first-line boundary for %j", | ||
| (newline) => { | ||
| const first = "x".repeat(8192) | ||
| expect(Ansi.normalize(first + newline + "password=wrong user", "template")).toBe(first) | ||
| expect(Ansi.normalize(newline + "x".repeat(100000), "template")).toBe("") | ||
| }, | ||
| ) | ||
|
|
||
| test("does not emit a surrogate split by the input cap", () => { | ||
| expect(Ansi.normalize(" ".repeat(8191) + "😀", "template")).toBeUndefined() | ||
| expect(Ansi.normalize(" ".repeat(8190) + "😀", "template")).toBe(" ".repeat(8190) + "😀") | ||
| }) | ||
|
|
||
| test("bounds the legacy normalization window without dropping line separators", () => { | ||
| expect(Ansi.normalize("x".repeat(10240) + "wrong user", "legacy")).toBe("x".repeat(10240)) | ||
| expect(Ansi.normalize("connection\nrefused", "legacy")).toBe("connection\nrefused") | ||
| }) | ||
|
|
||
| test("keeps the historical C1 hash profile explicitly separate", () => { | ||
| const raw = "request failed: \x9b31mpassword\x9b0m is connection refused" | ||
| const cleaned = Ansi.normalize(raw, "template")! | ||
| expect(cleaned).toBe("request failed: password is connection refused") | ||
| expect(ErrorDetail.describeToolError(raw).errorMessage).toBe("request failed <x>") | ||
| expect(ErrorDetail.describeToolError(raw).errorHash).toBe("40678db384156848") | ||
| expect(Telemetry.hashError(Telemetry.maskString(cleaned).slice(0, 500))).toBe("ae7e754cfb2d72a2") | ||
| }) | ||
| }) | ||
|
|
||
| describe("pre-V4 maskString compatibility", () => { | ||
| test.each(cases)("pins the historical error hash for %s", (name, raw) => { | ||
| expect(ErrorDetail.describeToolError(raw).errorHash).toBe(hashes[name]) | ||
| }) | ||
|
|
||
| test("preserves complete masked output over the frozen 96-input corpus", () => { | ||
| expect(corpus.length).toBe(96) | ||
| const digest = createHash("sha256") | ||
| .update(JSON.stringify(corpus.map((raw) => Telemetry.maskString(raw)))) | ||
| .digest("hex") | ||
| expect(digest).toBe("1fe645e8c04b5f30482bce034f9ddde64a36ac20beb475696a384edb39e1028c") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUGGESTION: The 8 KB guard measures the raw line before ANSI stripping, so escape-heavy lines lose their whole template even when almost no visible text was cut
The reject happens on
first.length > MAX_INPUTbeforestripAnsi, and the pinned case["ANSI-only text", "\x1b[0m".repeat(3000)]locks that in. A tool error whose first line is"connection refused " + "\x1b[0m".repeat(2400)(about 9.6K code units, 19 visible characters) used to template toconnection refusedand now becomes<x>: the diagnostics are dropped for no privacy gain, because everything past the cut is discarded whole, so a cut key or escape cannot leak its value.Basing the decision on the stripped length (strip the already-sliced window and reject only when the result still reaches
MAX_INPUT) keeps the template for coloured tool logs while keeping the unbounded-input bound.