diff --git a/.changeset/telemetry-path-redaction-non-ascii.md b/.changeset/telemetry-path-redaction-non-ascii.md new file mode 100644 index 0000000000..03b8939e21 --- /dev/null +++ b/.changeset/telemetry-path-redaction-non-ascii.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": patch +--- + +Fix telemetry path redaction leaking home directory names that are not ASCII. Paths under a directory named e.g. `李明`, `иван`, or `josé` were only partially redacted, as were UNC paths and `C:/`-style spellings. The `node_modules/` tail that carries diagnostic value is now also preserved on Windows. diff --git a/packages/agent-core-v2/src/app/telemetry/privacy.ts b/packages/agent-core-v2/src/app/telemetry/privacy.ts index e7349cdf92..aaa8492120 100644 --- a/packages/agent-core-v2/src/app/telemetry/privacy.ts +++ b/packages/agent-core-v2/src/app/telemetry/privacy.ts @@ -6,6 +6,18 @@ * paths become labeled `` placeholders, while `node_modules/` * path tails are kept because they carry diagnostic value without user data. * App-scoped, no collaborators. + * + * Path segments are matched by exclusion rather than with `\w`, which is + * ASCII-only and so left the tail of any path under a `李明`, `иван` or `josé` + * home directory in the payload. A segment may contain interior spaces + * (`Program Files`, `alice chen`); the final segment may too, but only when it + * ends in a file extension, so a path followed by prose is redacted without + * swallowing the sentence. A POSIX path must not start right after an + * alphanumeric, or chained fractions such as `read 1/2 then 3/4` would read as + * one. Absolute paths match as a single alternation in one pass, because + * running the branches separately let a later branch re-scan an earlier one's + * replacement. The behaviour these rules produce is pinned case by case in + * `test/app/telemetry/privacy.test.ts`. */ const REDACTED_PATH = ''; @@ -21,20 +33,32 @@ const LABELED_PATTERNS: ReadonlyArray = [ [/\b(?:sk|pk|ak)-[A-Za-z0-9_-]{16,}\b/g, ''], ]; -const POSIX_PATH = /(?:\/[\w.~+-]+){2,}\/?/g; -const WINDOWS_PATH = /\b[A-Za-z]:\\(?:[\w.~ -]+\\?){2,}/g; +const SEGMENT = String.raw`(?:[^\\/\s"<>|:*?]+)`; +const SEGMENT_WITH_SPACES = String.raw`(?:${SEGMENT}(?: +${SEGMENT})*)`; +const INTERIOR = String.raw`(?:${SEGMENT_WITH_SPACES}[\\/])`; +const FINAL_SEGMENT = String.raw`(?:${SEGMENT_WITH_SPACES}\.[A-Za-z][A-Za-z0-9]{0,9}(?![^\s"'<>])|${SEGMENT})`; + +const ABSOLUTE_PATH = new RegExp( + [ + String.raw`(?:\\\\[?.]\\)?[A-Za-z]:[\\/]${INTERIOR}*${FINAL_SEGMENT}?`, + String.raw`\\\\${INTERIOR}+${FINAL_SEGMENT}?`, + String.raw`(? { - const index = match.indexOf(NODE_MODULES_MARKER); - return index === -1 ? REDACTED_PATH : match.slice(index); - }); - return out; + return out.replace(ABSOLUTE_PATH, redactPath); } export function cleanTelemetryProperties

>(properties: P): P { diff --git a/packages/agent-core-v2/test/app/telemetry/privacy.test.ts b/packages/agent-core-v2/test/app/telemetry/privacy.test.ts new file mode 100644 index 0000000000..d79ee93d3f --- /dev/null +++ b/packages/agent-core-v2/test/app/telemetry/privacy.test.ts @@ -0,0 +1,233 @@ +import { describe, expect, it } from 'vitest'; + +import { cleanTelemetryProperties, cleanTelemetryString } from '#/app/telemetry/privacy'; + +/** + * `cleanTelemetryString` is the last step before `CloudAppender.track` buffers an + * event and `CloudTransport` POSTs it (`cloudAppender.ts:115`), so anything that + * survives here leaves the machine. The path patterns therefore have to hold for + * home directories that are not ASCII, and for Windows spellings other than + * `C:\a\b`. + */ +describe('cleanTelemetryString', () => { + describe('non-ASCII home directories', () => { + // `\w` is ASCII-only, so a path pattern built from it stopped at the first + // non-ASCII byte and left the remainder — user name included — in the payload. + it('redacts a POSIX path under a CJK home directory', () => { + expect(cleanTelemetryString('ENOENT: /home/李明/proj/secret.txt')).toBe( + 'ENOENT: ', + ); + }); + + it('redacts a Windows path under a CJK home directory', () => { + expect(cleanTelemetryString(String.raw`ENOENT: C:\Users\李明\proj\secret.txt`)).toBe( + 'ENOENT: ', + ); + }); + + it('redacts a Cyrillic home directory', () => { + expect(cleanTelemetryString('/home/иван/proj/secret.txt')).toBe( + '', + ); + }); + + it('redacts an accented home directory on both platforms', () => { + expect(cleanTelemetryString('/home/josé/proj/secret.txt')).toBe( + '', + ); + expect(cleanTelemetryString(String.raw`C:\Users\josé\proj\secret.txt`)).toBe( + '', + ); + }); + + it('redacts a name containing an apostrophe', () => { + expect(cleanTelemetryString(String.raw`C:\Users\O'Brien\proj\secret.txt`)).toBe( + '', + ); + }); + + it('redacts a name containing a space', () => { + expect(cleanTelemetryString('/home/alice chen/proj/secret.txt')).toBe( + '', + ); + expect(cleanTelemetryString(String.raw`C:\Users\alice chen\proj\secret.txt`)).toBe( + '', + ); + expect(cleanTelemetryString(String.raw`C:\Program Files\app\config.json`)).toBe( + '', + ); + }); + + it('redacts a filename containing a space', () => { + // The last segment is where the filename lives, so a space in it must not + // end the match — that would leave the rest of the name on the wire. + expect(cleanTelemetryString(String.raw`C:\Users\alice\secret file.txt`)).toBe( + '', + ); + expect(cleanTelemetryString('/home/alice/proj/secret file.txt')).toBe( + '', + ); + expect(cleanTelemetryString(String.raw`\\fileserver\home\secret file.txt`)).toBe( + '', + ); + expect(cleanTelemetryString(String.raw`C:\Users\alice\my report v2.final.docx`)).toBe( + '', + ); + }); + + it('stops a space-tolerant final segment at the path, not mid-sentence', () => { + // A space may continue the final segment only when what follows it still + // ends in an extension. Trailing prose does not, so the match backtracks + // to the filename instead of swallowing the sentence. + expect(cleanTelemetryString(String.raw`C:\Program Files\a.txt could not be read`)).toBe( + ' could not be read', + ); + expect(cleanTelemetryString('/home/alice chen/proj/a.txt could not be read')).toBe( + ' could not be read', + ); + expect(cleanTelemetryString(String.raw`C:\proj\a.txt failed at step 2.5`)).toBe( + ' failed at step 2.5', + ); + }); + }); + + describe('Windows path spellings', () => { + it('redacts a UNC share path', () => { + // Matched neither the drive-letter pattern nor the POSIX one, so it passed + // through with the share name and the whole path intact. + const cleaned = cleanTelemetryString( + String.raw`ENOENT: \\fileserver\home\alice.chen\proj\secret.txt`, + ); + expect(cleaned).toBe('ENOENT: '); + expect(cleaned).not.toContain('fileserver'); + expect(cleaned).not.toContain('alice.chen'); + }); + + it('redacts the \\\\?\\ long-path form', () => { + const cleaned = cleanTelemetryString( + String.raw`ENOENT: \\?\C:\Users\alice.chen\proj\secret.txt`, + ); + expect(cleaned).not.toContain('alice.chen'); + expect(cleaned).not.toContain('secret.txt'); + }); + + it('redacts a drive-letter path written with forward slashes', () => { + // Node and many libraries normalize to `C:/...`; the drive letter used to + // survive as a stray `C:` before the placeholder. + expect(cleanTelemetryString('ENOENT: C:/Users/alice.chen/proj/secret.txt')).toBe( + 'ENOENT: ', + ); + }); + + it('redacts a path at the drive root', () => { + expect(cleanTelemetryString(String.raw`ENOENT: C:\alice-secrets.txt`)).toBe( + 'ENOENT: ', + ); + }); + }); + + describe('behaviour that must not regress', () => { + it('still redacts plain ASCII paths on both platforms', () => { + expect(cleanTelemetryString(String.raw`ENOENT: C:\Users\alice.chen\proj\secret.txt`)).toBe( + 'ENOENT: ', + ); + expect(cleanTelemetryString('ENOENT: /home/alice.chen/proj/secret.txt')).toBe( + 'ENOENT: ', + ); + }); + + it('keeps the node_modules tail, which carries diagnostic value', () => { + expect(cleanTelemetryString('/home/alice.chen/repo/node_modules/pkg/index.js')).toBe( + 'node_modules/pkg/index.js', + ); + }); + + it('keeps the node_modules tail on Windows too', () => { + // The marker is spelled with a forward slash, so a backslash path never + // matched it and the diagnostic tail was thrown away. + expect( + cleanTelemetryString(String.raw`C:\Users\alice.chen\repo\node_modules\pkg\index.js`), + ).toBe('node_modules/pkg/index.js'); + }); + + it('leaves consecutive fractions alone', () => { + // A space-tolerant segment lets `2 then 3` read as one segment, so without + // the alphanumeric lookbehind the run from `/2` to `/6` matches as a path. + expect(cleanTelemetryString('read 1/2 then 3/4 then 5/6')).toBe( + 'read 1/2 then 3/4 then 5/6', + ); + expect(cleanTelemetryString('ratio 1/2 and 3/4')).toBe('ratio 1/2 and 3/4'); + }); + + it('still redacts a two-segment absolute path', () => { + expect(cleanTelemetryString('/tmp/kimi-scratch.json')).toBe(''); + expect(cleanTelemetryString('/etc/passwd')).toBe(''); + }); + + it('leaves text that is not an absolute path alone', () => { + expect(cleanTelemetryString('read 3/4 of the file')).toBe('read 3/4 of the file'); + expect(cleanTelemetryString('the operation timed out after 30s')).toBe( + 'the operation timed out after 30s', + ); + expect(cleanTelemetryString('/tmp')).toBe('/tmp'); + expect(cleanTelemetryString('moonshotai/kimi-k2-instruct')).toBe( + 'moonshotai/kimi-k2-instruct', + ); + expect(cleanTelemetryString('')).toBe(''); + }); + + it('still applies the labeled patterns', () => { + expect(cleanTelemetryString('mail alice@example.com')).toBe('mail '); + expect(cleanTelemetryString('see https://example.com/a/b')).toBe('see '); + expect(cleanTelemetryString('key sk-abcdefghijklmnopqrstuv')).toBe( + 'key ', + ); + }); + + it('does not let the path pattern consume an earlier placeholder', () => { + // The URL is replaced first; the path pass must not then treat the + // placeholder or its surroundings as a path. + expect( + cleanTelemetryString('GET https://api.example.com/v1/x failed for /home/alice/p/s.txt'), + ).toBe('GET failed for '); + expect(cleanTelemetryString('alice@example.com opened /home/alice/p/s.txt')).toBe( + ' opened ', + ); + }); + + it('redacts every path in a value, not just the first', () => { + expect(cleanTelemetryString('copy /home/alice/a.txt to /home/alice/b.txt')).toBe( + 'copy to ', + ); + }); + + it('does not leak across repeated calls', () => { + // The patterns are module-level `/g` literals; `replace` resets `lastIndex` + // but `exec`/`test` would not, so pin the behaviour callers rely on. + const input = '/home/李明/proj/secret.txt'; + expect(cleanTelemetryString(input)).toBe(''); + expect(cleanTelemetryString(input)).toBe(''); + expect(cleanTelemetryString(input)).toBe(''); + }); + }); +}); + +describe('cleanTelemetryProperties', () => { + it('cleans string values and passes other primitives through', () => { + expect( + cleanTelemetryProperties({ + cwd: '/home/李明/proj', + count: 3, + ok: true, + missing: undefined, + empty: null, + }), + ).toEqual({ + cwd: '', + count: 3, + ok: true, + missing: undefined, + empty: null, + }); + }); +});