diff --git a/.changeset/logger-redact-word-boundary.md b/.changeset/logger-redact-word-boundary.md new file mode 100644 index 0000000000..93b06a6358 --- /dev/null +++ b/.changeset/logger-redact-word-boundary.md @@ -0,0 +1,45 @@ +--- +"@objectstack/core": patch +--- + +fix(core): `ObjectLogger` 的脱敏表按**词边界**匹配,不再按子串吃掉 `keys`/`tokens` 这类普通字段 (#5573) + +`redactSensitive` 此前的判定是 `key.toLowerCase().includes(pattern)` —— 只要字段名 +**含有** `password`/`token`/`secret`/`key` 子串,整个值就被换成 `***REDACTED***`。 +于是 `keys`、`keyword`、`keywords`、`keyboard`、`monkey`、`tokens`、`tokenizer`、 +`secretary` 全部中招:读者不但丢了事实,还被告知"这里挡住了一个秘密",比字段直接 +缺失更误导。仓库里已经有活的命中 —— `dispatcher-plugin.ts` 为了躲开脱敏器特意把 +`key` 改名成 `keyedBy`,而 `'keyedby'.includes('key')` 依然为真,那条限流日志的 +`keyedBy` 一直是 `***REDACTED***`。 + +匹配语义 FROM → TO: + +| | FROM(子串 `includes`) | TO(词边界) | +|:---|:---|:---| +| `apiKey` / `api_key` / `API_KEY` / `x-api-key` | 脱敏 | 脱敏(不变) | +| `apikey` / `APIKEY`(全小写连写) | 脱敏 | 脱敏(不变,见下) | +| `apiKeys` / `refresh_tokens`(复合词里的复数) | 脱敏 | 脱敏(不变) | +| `keys` / `tokens` / `keyword` / `monkey` / `secretary` | **脱敏** | **不脱敏** | +| `keyedBy` / `tokenizerName` | **脱敏** | **不脱敏** | +| `passwords` / `secrets`(裸复数) | **脱敏** | **不脱敏** | +| `api_key` 字段 + `redact: ['apiKey']` 配置 | **不脱敏** | **脱敏**(跨拼法命中) | + +字段名按 camelCase / snake_case / kebab-case / 字母-数字边界分词后逐词比对。默认脱敏表 +(`['password','token','secret','key']`)本身**没有变**,`packages/spec` 的 schema 默认值 +也没有变 —— 变的只是这张表怎么用。 + +两个边角是显式取舍,不是遗漏: + +- **全小写连写**没有词边界可分,`apikey` 分词后只有一个词。不能用"以 `key` 结尾"救, + 因为 `monkey`/`turkey`/`whiskey` 也以它结尾 —— 那正是本单要去掉的误报。所以连写只在 + 前缀是一张显式限定词表(`api`/`access`/`refresh`/`client`/`private`/`session`/…)里的 + 词时才算命中;表外的连写(`foobarkey`)不脱敏,按仓库命名惯例写成 `fooBarKey` / + `foo_bar_key` 即可通用命中。只认**后缀**连写,所以 `secretary`、`keyword` 保持干净。 +- **裸复数**是集合或计数而不是秘密(`keys` 来自 Zod 的 `unrecognized_keys` issue, + `tokens` 来自 LLM 用量),按维护者裁决不脱敏;复数**出现在复合词里**时仍然是秘密 + (`apiKeys: ['sk-…']`),照常脱敏。确实要脱敏裸复数的 host,写 + `redact: [..., 'passwords']` 显式加回。 + +**影响面**:host 侧自定义 `redact` 配置的匹配行为随之收紧 —— 依赖子串宽匹配"顺手"挡住 +某个字段的部署,需要把该字段名(或它的词)显式写进 `redact`。反向的收益是同一个词现在 +跨拼法命中:配 `redact: ['apiKey']` 也会挡住 `api_key` 和 `apikey`。 diff --git a/packages/core/src/logger.test.ts b/packages/core/src/logger.test.ts index 89f74472dd..b5a3395e5e 100644 --- a/packages/core/src/logger.test.ts +++ b/packages/core/src/logger.test.ts @@ -298,4 +298,131 @@ describe('ObjectLogger', () => { expect(record.connector).toBe('billing'); }); }); + + // #5573 — the redactor matched by SUBSTRING, so every field whose name + // merely contained `password`/`token`/`secret`/`key` was replaced with + // `***REDACTED***`: `keys`, `keyword`, `tokens`, `monkey`, `secretary`. + // That is worse than dropping the field, because `***REDACTED***` tells the + // reader a secret was withheld when there never was one. Matching is now on + // camelCase/snake_case word boundaries (maintainer ruling, option A). + describe('redaction matches whole words, not substrings (#5573)', () => { + const stdoutChunks: string[] = []; + + beforeEach(() => { + stdoutChunks.length = 0; + vi.spyOn(process.stdout, 'write').mockImplementation(((c: string | Uint8Array) => { + stdoutChunks.push(String(c)); + return true; + }) as never); + }); + afterEach(() => vi.restoreAllMocks()); + + /** The one JSON record `meta` renders to, under `redact` (default table when omitted). */ + const recordOf = (meta: Record, redact?: string[]): Record => { + stdoutChunks.length = 0; + const log = createLogger({ level: 'info', format: 'json', ...(redact ? { redact } : {}) }); + log.info('probe', meta); + const lines = stdoutChunks.join('').split('\n').filter(Boolean); + expect(lines, 'one call must write exactly one physical line').toHaveLength(1); + return JSON.parse(lines[0]) as Record; + }; + + /** What the redactor did to a field of this name, carrying a marker value. */ + const verdictFor = (field: string, redact?: string[]): 'redacted' | 'kept' => { + const value = recordOf({ [field]: 'MARKER-VALUE' }, redact)[field]; + return value === '***REDACTED***' ? 'redacted' : 'kept'; + }; + + // The half that must NOT regress: a real secret stays redacted whatever + // convention it is spelled in. Every entry here was redacted before the + // change too — this matrix is the guard that word boundaries did not buy + // precision by losing coverage. + const REAL_SECRETS = [ + // the redact words themselves + 'password', 'token', 'secret', 'key', + // camelCase + 'apiKey', 'accessToken', 'refreshToken', 'secretKey', 'privateKey', 'publicKey', + 'clientSecret', 'sessionToken', 'bearerToken', 'signingKey', 'encryptionKey', + 'passwordHash', 'dbPassword', 'sshKey', 'jwtSecret', + // snake_case + 'api_key', 'access_token', 'refresh_token', 'secret_key', 'private_key', + 'client_secret', 'user_password', 'auth_token', + // SCREAMING_SNAKE and kebab (headers, env vars) + 'API_KEY', 'OS_AUTH_SECRET', 'x-api-key', 'x-refresh-token', + // all-lowercase / all-caps concatenation — no boundary to split on + 'apikey', 'APIKEY', 'accesstoken', 'clientsecret', 'privatekey', + // plural, inside a compound: `apiKeys: ['sk-…']` is still secrets + 'apiKeys', 'api_keys', 'accessTokens', 'clientSecrets', 'userPasswords', 'apikeys', + ]; + + it.each(REAL_SECRETS)('still redacts %s', (field) => { + expect(verdictFor(field)).toBe('redacted'); + }); + + // The other half: the symptom this issue was filed for. Every entry was + // `***REDACTED***` before the change. + const NOT_SECRETS = [ + // #5573's own repro: Zod's `unrecognized_keys` issue names the keys in `keys` + 'keys', 'keyword', 'keywords', 'keyboard', 'monkey', 'tokens', 'tokenizer', 'secretary', + // other English words ending in a redact word + 'donkey', 'turkey', 'whiskey', 'hockey', + // dispatcher-plugin.ts renamed `key` -> `keyedBy` to dodge the redactor, + // and the substring rule ate that too ('keyedby'.includes('key')). + 'keyedBy', + 'tokenizerName', + ]; + + it.each(NOT_SECRETS)('no longer redacts %s', (field) => { + expect(verdictFor(field)).toBe('kept'); + }); + + // Honest residual, pinned rather than left for the next reader to + // discover: word boundaries cannot tell the *sense* of a word apart. + // A field whose words include the SINGULAR `token`/`key` is still + // redacted even when it is plainly a counter, and so is a compound + // plural (the `apiKeys` rule, applied to `promptTokens`). Both were + // redacted before this change as well — nothing regressed — but + // neither is fixed by it. Narrowing further needs the maintainer: + // it would mean ranking `prompt` against `api` as a qualifier. + it.each(['tokenCount', 'promptTokens', 'completionTokens'])( + 'still redacts %s — word boundaries do not disambiguate word SENSE', + (field) => { + expect(verdictFor(field)).toBe('redacted'); + }, + ); + + it('keeps a nested `keys` field readable — the exact #5573 repro', () => { + const record = recordOf({ + issues: [{ code: 'unrecognized_keys', keys: ['visibleIf'], path: ['nodes', 0] }], + }); + expect(record.issues).toEqual([ + { code: 'unrecognized_keys', keys: ['visibleIf'], path: ['nodes', 0] }, + ]); + }); + + it('still reaches a secret nested several levels down', () => { + const record = recordOf({ connector: { auth: { apiKey: 'sk-live-123', mode: 'header' } } }); + expect(record.connector).toEqual({ auth: { apiKey: '***REDACTED***', mode: 'header' } }); + }); + + // Bare plurals are collections or counts, not secrets — that is the + // maintainer's ruling on `keys`/`tokens`, applied uniformly. The + // plural only names a secret once something qualifies it (`apiKeys`, + // above). Pinned because it is a deliberate coverage change, not an + // oversight: a host that does log a bare `passwords` list opts back in + // with `redact: [..., 'passwords']`. + it('leaves a BARE plural alone, and honours an explicit opt-in for it', () => { + expect(verdictFor('passwords')).toBe('kept'); + expect(verdictFor('secrets')).toBe('kept'); + expect(verdictFor('passwords', ['password', 'passwords'])).toBe('redacted'); + }); + + it('honours a host-configured multi-word pattern, without widening it to its parts', () => { + expect(verdictFor('apiKey', ['apiKey'])).toBe('redacted'); + expect(verdictFor('api_key', ['apiKey'])).toBe('redacted'); + expect(verdictFor('apikey', ['apiKey'])).toBe('redacted'); + // `apiKey` is the configured secret; a plain `key` is not one. + expect(verdictFor('key', ['apiKey'])).toBe('kept'); + }); + }); }); diff --git a/packages/core/src/logger.ts b/packages/core/src/logger.ts index 6ad2f4274a..784915f89c 100644 --- a/packages/core/src/logger.ts +++ b/packages/core/src/logger.ts @@ -28,6 +28,145 @@ const LEVEL_COLORS: Record = { const RESET = '\x1b[0m'; +/** + * Split a field name into lowercase words on camelCase, `snake_case`, + * `kebab-case`, dot and letter/digit boundaries. + * + * `apiKey` / `api_key` / `API_KEY` / `x-api-key` all tokenize to + * `['api','key']`, while `monkey`, `keyword` and `tokenizer` stay a single + * word. That difference is the whole point: it is what makes the redactor a + * **word-boundary** matcher instead of the substring matcher it used to be + * (#5573) — a plain `keys` field no longer reads as a secret. + */ +function tokenizeFieldName(name: string): string[] { + return name + .replace(/([a-z0-9])([A-Z])/g, '$1 $2') // apiKey -> api Key + .replace(/([A-Z]+)([A-Z][a-z])/g, '$1 $2') // APIKey -> API Key + .replace(/([a-zA-Z])([0-9])/g, '$1 $2') // key2 -> key 2 + .split(/[^A-Za-z0-9]+/) // _ - . / space + .filter(Boolean) + .map((word) => word.toLowerCase()); +} + +/** + * Singular form of the plural spellings the redact vocabulary actually meets + * (`keys`, `tokens`, `secrets`, `passwords`, `passes`). Deliberately not a + * general inflector — it only has to be right for words that end up next to a + * redact word, and it must never turn `address`/`status` into a new word. + */ +function singularizeWord(word: string): string { + if (/(?:ss|us|is)$/.test(word)) return word; // address / status / axis + if (/(?:ch|sh|s|x|z)es$/.test(word)) return word.slice(0, -2); // passes / boxes + if (/[a-z0-9]s$/.test(word)) return word.slice(0, -1); // keys / tokens + return word; +} + +/** + * Words that mark the *secret* sense of a redact word when they are glued to + * it with no boundary to split on: `apikey`, `accesstoken`, `clientsecret`. + * + * Word-boundary matching covers every field name spelled the way this repo + * spells names (camelCase config keys / snake_case machine names — Prime + * Directive #3), but an all-lowercase concatenation has no boundary at all, so + * `apikey` would tokenize to one word and stop being redacted. A bare + * "ends with `key`" rule cannot be used to rescue it, because `monkey`, + * `turkey` and `whiskey` end with `key` too — the exact false positives #5573 + * exists to remove. So the rescue is scoped to this explicit qualifier list: + * `` is a secret, anything else glued to a redact word + * is not. + * + * Consequences, on purpose: + * - Only a **suffix** concatenation counts. `secretary` and `keyword` start + * with a redact word and stay clear. + * - An unlisted qualifier (`foobarkey`) is not redacted. The fix is to spell + * the field `fooBarKey` / `foo_bar_key`, which matches generically — or to + * add the word here. + */ +const CONCATENATED_SECRET_QUALIFIERS = new Set([ + 'access', + 'account', + 'admin', + 'api', + 'app', + 'auth', + 'bearer', + 'client', + 'csrf', + 'db', + 'database', + 'encryption', + 'id', + 'jwt', + 'master', + 'oauth', + 'private', + 'public', + 'refresh', + 'root', + 'secret', + 'service', + 'session', + 'shared', + 'sign', + 'signing', + 'ssh', + 'token', + 'user', + 'webhook', + 'xsrf', +]); + +/** `apikey`/`apikeys` vs `key` — see {@link CONCATENATED_SECRET_QUALIFIERS}. */ +function isQualifiedConcatenation(word: string, redactWord: string): boolean { + for (const base of [word, singularizeWord(word)]) { + if (base.length <= redactWord.length || !base.endsWith(redactWord)) continue; + if (CONCATENATED_SECRET_QUALIFIERS.has(base.slice(0, base.length - redactWord.length))) return true; + } + return false; +} + +/** Does `words` contain `run` as a consecutive sub-sequence? */ +function containsWordRun(words: string[], run: string[]): boolean { + for (let i = 0; i + run.length <= words.length; i++) { + if (run.every((word, offset) => words[i + offset] === word)) return true; + } + return false; +} + +/** + * Word-boundary match of one configured redact pattern against one field name, + * both already tokenized by {@link tokenizeFieldName}. + * + * The plural rule is the one subtlety, and it is the maintainer's ruling on + * #5573 made consistent with itself: a **bare** plural names a collection or a + * count, not a secret (`keys` on a Zod `unrecognized_keys` issue, `tokens` on + * an LLM usage record), so it is left alone; a plural **inside a compound** + * still names the secret (`apiKeys: ['sk-…']`, `refresh_tokens`) and is + * redacted. Singular words match everywhere, compound or not. + */ +function fieldWordsMatchPattern(nameWords: string[], patternWords: string[]): boolean { + if (patternWords.length === 0 || nameWords.length === 0) return false; + + // A multi-word pattern (`apiKey`, `api_key`) matches a consecutive run of + // the same words, or those words written as one concatenated token. + if (patternWords.length > 1) { + const glued = patternWords.join(''); + return ( + containsWordRun(nameWords, patternWords) || + nameWords.some((word) => word === glued || singularizeWord(word) === glued) + ); + } + + const redactWord = patternWords[0]; + const isCompound = nameWords.filter((word) => /[a-z]/.test(word)).length > 1; + return nameWords.some( + (word) => + word === redactWord || + (isCompound && singularizeWord(word) === redactWord) || + isQualifiedConcatenation(word, redactWord), + ); +} + /** * Whether ANSI color may be written to the given stream. * @@ -84,6 +223,8 @@ export class ObjectLogger implements Logger { name?: string; }; private bindings: Record; + /** `config.redact`, tokenized once — see {@link fieldWordsMatchPattern}. */ + private redactPatterns: string[][]; private fileStream?: any; /** Only the logger that opened the stream may close it — children share it. */ private ownsFileStream = false; @@ -100,6 +241,7 @@ export class ObjectLogger implements Logger { rotation: config.rotation ?? { maxSize: '10m', maxFiles: 5 }, }; this.bindings = bindings; + this.redactPatterns = this.config.redact.map(tokenizeFieldName).filter((words) => words.length > 0); if (this.config.file && typeof process !== 'undefined') { this.openFileStream(this.config.file); @@ -156,12 +298,26 @@ export class ObjectLogger implements Logger { return LEVEL_ORDER[level] >= LEVEL_ORDER[this.config.level]; } + /** + * Whether a meta field name names one of the configured secrets. + * + * Until #5573 this was `lower.includes(pattern)`, which redacted every + * field whose name merely *contained* a redact word — `keys`, `keyword`, + * `tokens`, `monkey`, `secretary` — and replaced its value with + * `***REDACTED***`, so the reader lost the fact AND was told a secret had + * been withheld. Matching is now on word boundaries: `key` matches + * `apiKey` / `api_key`, not `keys` / `monkey` / `keyword`. + */ + private isRedactedFieldName(key: string): boolean { + const nameWords = tokenizeFieldName(key); + return this.redactPatterns.some((pattern) => fieldWordsMatchPattern(nameWords, pattern)); + } + private redactSensitive(obj: any): any { if (!obj || typeof obj !== 'object') return obj; const redacted = Array.isArray(obj) ? [...obj] : { ...obj }; for (const key in redacted) { - const lower = key.toLowerCase(); - if (this.config.redact.some((p: string) => lower.includes(p.toLowerCase()))) { + if (this.isRedactedFieldName(key)) { redacted[key] = '***REDACTED***'; } else if (typeof redacted[key] === 'object' && redacted[key] !== null) { redacted[key] = this.redactSensitive(redacted[key]); diff --git a/packages/services/service-automation/src/thrown-cause-diagnostics.test.ts b/packages/services/service-automation/src/thrown-cause-diagnostics.test.ts index 7585e90410..3ab6efeff8 100644 --- a/packages/services/service-automation/src/thrown-cause-diagnostics.test.ts +++ b/packages/services/service-automation/src/thrown-cause-diagnostics.test.ts @@ -297,21 +297,34 @@ describe('the rendered warning is ONE line that still names the key (#5048)', () expect(lines[0]).not.toContain('REDACTED'); }); - it('forwarding Zod issues VERBATIM would be redacted — this is why they are re-shaped', () => { - // Evidence for the `unrecognized` naming in thrown-cause-diagnostics.ts: - // ObjectLogger redacts recursively by SUBSTRING, and its default list - // includes `key`, which `keys` contains. A `{ issues: err.issues }` meta - // therefore ships `"keys":"***REDACTED***"` — losing the one fact the - // reader came for. Pinned so a future "just pass err.issues" cleanup - // fails loudly instead of silently re-blinding the diagnostic. + it("a verbatim Zod `keys` field is no longer eaten by the redactor (#5573 re-judged this pin)", () => { + // RE-JUDGED, by the maintainer's ruling on #5573. Until then + // `ObjectLogger` redacted by SUBSTRING and its default list contains + // `key`, which `keys` contains — so this same meta rendered + // `"keys":"***REDACTED***"` and this test pinned that, as the evidence + // for naming the field `unrecognized` here. Redaction now matches on + // camelCase/snake_case WORD boundaries: `key` matches `apiKey`/`api_key`, + // not `keys`. So the verdict flips, and it flips onto a positive + // assertion — the key name is present in the output, not merely + // un-redacted-because-empty. + // + // NB the module docblock of `thrown-cause-diagnostics.ts` still + // describes the substring rule in its first bullet; that sentence is + // superseded here. The SECOND reason it gives for re-shaping is + // untouched and still load-bearing: a raw Zod issue can carry the whole + // rejected `input`, and a log record must stay bounded. That is why + // this helper keeps flattening issues rather than forwarding them. const lines = captureStdout((log) => { log.warn('verbatim', { issues: [{ code: 'unrecognized_keys', keys: ['visibleIf'], path: ['nodes', 0] }], }); }); expect(lines).toHaveLength(1); - expect(lines[0]).toContain('REDACTED'); - expect(lines[0]).not.toContain('visibleIf'); + expect(lines[0]).not.toContain('REDACTED'); + expect(lines[0]).toContain('visibleIf'); + // The whole field survives, not just the name fragment. + const record = JSON.parse(lines[0]) as { issues?: unknown }; + expect(record.issues).toEqual([{ code: 'unrecognized_keys', keys: ['visibleIf'], path: ['nodes', 0] }]); }); it('a multi-line NON-Zod message still occupies one line (JSON escapes the newlines)', () => { diff --git a/packages/services/service-automation/src/thrown-cause-diagnostics.ts b/packages/services/service-automation/src/thrown-cause-diagnostics.ts index 23e89b6176..9f9029d19b 100644 --- a/packages/services/service-automation/src/thrown-cause-diagnostics.ts +++ b/packages/services/service-automation/src/thrown-cause-diagnostics.ts @@ -63,22 +63,28 @@ * * ## Why the issues are re-shaped rather than forwarded verbatim * - * Two measured reasons, not taste: - * - * - `ObjectLogger` redacts recursively by substring: its default - * `redact: ['password', 'token', 'secret', 'key']` matches any field whose - * lowercased name *contains* one of them. A Zod `unrecognized_keys` issue - * names the offending keys in a field called `keys`, and `'keys'` contains - * `'key'` — forwarding `err.issues` untouched therefore renders - * `"keys":"***REDACTED***"`, i.e. it loses the one fact the reader came - * for. The field is named `unrecognized` here so the key names survive. - * Any field added to this record is subject to the same rule. - * - A Zod issue can carry the whole rejected `input` on some codes. A log - * record must stay bounded — the boot buffer drops any line that would - * overflow its budget, and a shipper truncates — which would resurrect the - * very failure mode above. This is also why the seams hand the *cause* here - * rather than passing the raw `Error` into the logger's `error` slot: that - * would ship the full multi-line dump plus a stack trace on every record. + * Two measured reasons, not taste. The first has since LAPSED (#5573); the + * second carries the decision on its own: + * + * - *Historical — superseded by #5573.* `ObjectLogger` used to redact + * recursively by SUBSTRING: its default + * `redact: ['password', 'token', 'secret', 'key']` matched any field whose + * lowercased name merely *contained* one of them. A Zod `unrecognized_keys` + * issue names the offending keys in a field called `keys`, and `'keys'` + * contains `'key'` — so forwarding `err.issues` untouched rendered + * `"keys":"***REDACTED***"` and lost the one fact the reader came for. + * Redaction now matches on camelCase/snake_case WORD boundaries, so a bare + * `keys` is no longer redacted and this reason no longer holds. The rule a + * field added to this record is judged by today: `apiKey` / `api_key` is + * redacted, `keys` / `tokens` is not. + * - *Still load-bearing.* A Zod issue can carry the whole rejected `input` + * on some codes. A log record must stay bounded — the boot buffer drops + * any line that would overflow its budget, and a shipper truncates — which + * resurrects the same failure the bullet above describes: a record that has + * lost the fact the reader came for. This is also why the seams hand the + * *cause* here rather than passing the raw `Error` into the logger's + * `error` slot: that would ship the full multi-line dump plus a stack + * trace on every record. * * So the fields are named deliberately and the list is capped, with the cap * *declared* in the record (`issueCount`) rather than silently applied. @@ -97,8 +103,10 @@ export interface LoggedCauseIssue { message: string; /** * The rejected key names of an `unrecognized_keys` issue. Named - * `unrecognized` rather than `keys` so `ObjectLogger`'s substring redactor - * does not replace it with `***REDACTED***`. + * `unrecognized` rather than `keys` because `ObjectLogger` redacted by + * substring when this was written; #5573 has since narrowed that to word + * boundaries, so a field called `keys` would survive today. The name is + * kept as-is — renaming a shipped log field back would be pure churn. */ unrecognized?: string[]; }