From d04569e37f1c63967ce7373557ded4e4f150e7de Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Sat, 1 Aug 2026 17:09:59 -0700 Subject: [PATCH] fix(core): redact any path in telemetry, not an allowlist of roots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit redactTelemetryString enumerated roots — /Users, /home, /opt, /tmp and a handful more — so a project on /data, /Volumes, an NFS mount or any root a user invented reached telemetry verbatim. Relative paths and bare basenames were never redacted at all, and audioPadTrim routes raw ffprobe stderr through this on every probe failure. Now redacts by shape: absolute paths under any root (two or more segments, so N/A and a 24/1 frame rate are not mistaken for one), relative paths including dash-prefixed ones, and bare basenames with an asset extension. URLs still keep their host and drop only the query. Co-Authored-By: Claude Opus 5 (1M context) --- packages/core/src/telemetryRedaction.test.ts | 49 ++++++++++++++ packages/core/src/telemetryRedaction.ts | 69 ++++++++++++++++++-- 2 files changed, 112 insertions(+), 6 deletions(-) diff --git a/packages/core/src/telemetryRedaction.test.ts b/packages/core/src/telemetryRedaction.test.ts index 670f1a4ea..bd5737033 100644 --- a/packages/core/src/telemetryRedaction.test.ts +++ b/packages/core/src/telemetryRedaction.test.ts @@ -16,4 +16,53 @@ describe("redactTelemetryString", () => { ), ).toBe("[path] [path] [path] [path] [file-url] https://example.com/video.mp4?…"); }); + + // The redactor used to enumerate roots (/Users, /home, /opt, /tmp, …). Any + // root outside that list reached telemetry verbatim, which is most of them. + it.each([ + "/data/media/interview.mov", + "/mnt2/nfs/share/take3.wav", + "/srv2/renders/2026/final.mp4", + "/nix/store/abc123/asset.png", + ])("redacts the non-allowlisted absolute root in %s", (path) => { + const out = redactTelemetryString(`ffprobe failed reading ${path}`); + expect(out).not.toContain("/"); + expect(out).toContain("[path]"); + }); + + it("redacts relative paths, including a dash-prefixed one", () => { + expect(redactTelemetryString("could not open ./assets/-weird-name.mp3")).toBe( + "could not open [path]", + ); + expect(redactTelemetryString("could not open ../-out.wav")).toBe("could not open [path]"); + expect(redactTelemetryString("could not open .\\tmp\\-x.aac")).toBe("could not open [path]"); + }); + + it("redacts a bare basename — a caller may pass one instead of a path", () => { + expect(redactTelemetryString("Invalid data found in my-client-cut.mp4")).toBe( + "Invalid data found in [file]", + ); + }); + + // Over-redaction is cheap; these are ordinary in ffprobe stderr and turning + // them into [path] would make a diagnostic string useless. + it.each(["N/A", "24/1", "Stream #0:0", "moov atom not found", "48000/1001"])( + "leaves %s alone", + (text) => { + expect(redactTelemetryString(`ffprobe: ${text}`)).toBe(`ffprobe: ${text}`); + }, + ); + + // A `?` is illegal in a Windows filename, so this is not a query string — + // the whole token is path, and must not survive by hiding behind a `?`. + it("consumes the rest of the token once a path is established", () => { + expect(redactTelemetryString("Navigation failed for C:\\Users\\A\\v.mov?not-a-query")).toBe( + "Navigation failed for [path]", + ); + }); + + it("truncates after redacting, so a long path cannot survive by being cut", () => { + const out = redactTelemetryString(`/data/${"x".repeat(500)}/a.mp4`, 40); + expect(out).not.toContain("xxx"); + }); }); diff --git a/packages/core/src/telemetryRedaction.ts b/packages/core/src/telemetryRedaction.ts index 5e05ab7a1..3a2de994e 100644 --- a/packages/core/src/telemetryRedaction.ts +++ b/packages/core/src/telemetryRedaction.ts @@ -9,13 +9,70 @@ function redactUrlQueryStrings(value: string): string { return value.replace(/\b(https?:\/\/[^\s?]+)\?[^\s]*/g, "$1?…"); } +/** + * Path characters we treat as part of a single segment. Space is deliberately + * excluded: including it would let a match run past the path and swallow the + * prose after it, and a path with a space still gets its remaining segments + * redacted, which is the part that carries the identifying information. + */ +const SEGMENT = String.raw`[\w.\-@+()~]+`; + +/** + * Once a match is established as a path, consume the rest of the token. + * Windows forbids `?` in a filename, so `video.mov?not-a-query` is not a real + * query string — but stopping at the `?` would emit the remainder verbatim. + * Redacting to the next delimiter cannot leak; stopping early can. + */ +const TOKEN_TAIL = String.raw`[^\s'")]*`; + +/** + * Absolute path, any root — NOT an allowlist of roots. + * + * The previous version enumerated `/Users`, `/home`, `/opt`, `/tmp`… which + * meant a project on `/data`, `/Volumes/External`, an NFS mount or any root a + * user invented reached telemetry verbatim. Two or more segments are required + * so `N/A` and a `24/1` frame rate — both ordinary in ffprobe stderr — are not + * mistaken for paths. + * + * The lookbehind keeps this off URLs: after `https:` the slash is preceded by + * `:`, the second by `/`, and the path segment by a word character, so no + * position inside a URL can start a match. URLs are handled above, where the + * host is kept and only the query is dropped. + */ +const ABSOLUTE_PATH = new RegExp( + String.raw`(?