fix: mask filesystem paths in telemetry error text - #1117
fix: mask filesystem paths in telemetry error text#1117ralphstodomingo wants to merge 45 commits into
Conversation
maskString deliberately covers API keys, bearer tokens, emails, internal hosts and quoted strings — but had no rule for filesystem paths, so UNQUOTED paths in error messages reached App Insights raw. Home-directory paths carry the OS username and project-rooted paths leak client repo structure (a live 32-machine core_failure/file_not_found cluster carried full /Users/<name>/... paths). Quoted paths were coincidentally destroyed by the quote rule, which is why the asymmetry went unnoticed. Adds three ordered rules (Windows drive/UNC, POSIX absolute with 2+ segments, home-relative ~/) that replace the whole path with <path> — matching the chain's existing "over-masking is the correct failure mode" doctrine. Ordered after the URL rule (a public URL's path segment is never word-anchored, so it cannot match) and before quote masking. Bare single-segment tokens like "/mcp", MIME types, dates and version specs are proven untouched by the test matrix. Side effect, intended: hashError operates on the masked message, so grouping keys for affected messages change once — and path-variant messages that previously split now collapse into one group.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesTelemetry path masking
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🟡 Moderate · up to The current masking rules can leave semicolon-attached filename or object-key suffixes unmasked, allowing sensitive path structure to reach telemetry. This bounded privacy issue should be fixed and covered by regression tests before merging. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
@codex review |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 167a5f2b94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review + a live-data sweep hardened the path rules. Ran all 800 distinct
30-day telemetry messages through the patched maskString hunting for
residual path content:
- 1 true leak survived: a gs:// URI carrying a client bucket name + data
layout — cloud-storage URIs (gs, s3, abfss, wasbs, adl, dbfs, hdfs,
file) are not http so the URL rule never sees them, and not
slash-anchored so the path rule missed them. New leading rule masks the
whole URI.
- Review caught unquoted paths with embedded spaces (macOS
"/Users/Jane Doe/client repo/…") escaping the [^\s] character classes.
Paths now continue across a space whenever a later chunk carries another
separator, plus one optional trailing spaced filename — while trailing
prose ("… /app/a.sql was deleted upstream") is proven untouched.
Public https doc-links (provider help URLs) remain deliberately unmasked —
the other 32 sweep hits were all of that class. After both fixes the
800-message sweep reports zero residual path content.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4899189e74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Second review round, all three confirmed by construction:
- Anchor classes include [ and { so bracketed paths ("failed [/Users/…]")
match; quote masking never applied to those.
- Cloud-URI rule gets the same embedded-whitespace continuation as
filesystem paths, and continuation chunks may span MULTIPLE spaced words
before the next separator ("dir with spaces/part …") — the single-word
unit broke the chain.
- One trailing spaced WORD is consumed at end-of-string or before
punctuation ("Directory not found: /Users/jdoe/client repo"). The
general "path + space + word" case is undecidable against trailing
prose ("client repo" vs "x.sql was"), so mid-sentence prose stays
protected; the accepted residue is a single end-of-string prose word
eaten (over-masking doctrine), and a username can never leak through
this case — it is always slash-terminated and therefore always inside
the masked span.
15 unit tests; 800-message live sweep still reports zero residual leaks;
adjacent suites + typecheck green.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d56682f10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Third review round. The P1 caught a real overclaim: "a username can never leak" was false for SPACED usernames — "/Users/Jane Doe does not exist" left the surname outside the masked span mid-sentence. Home-rooted paths (/Users, /home, C:\Users — where the spaced-username stake lives) now consume one unconditional trailing word, suppressed by an extension-lookbehind when the path already ended in a dotted file so "x.sql was deleted" prose stays intact. Honest residue, now documented in the rule comment: one prose word may be over-masked after an extensionless home path; a NON-home path's terminal spaced component can still leak one structure word mid-sentence (no personal names there). P2: the Windows span consumed closing delimiters — "(C:\Users\x.sql)" became "(<path>". Aligned its character classes with the POSIX/cloud rules (Program Files-style spaced dirs still continue via the word chunks, which keep parens). 19 unit tests; 800-message live sweep still zero residuals; adjacent suites + typecheck green.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40cb7fea4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fourth review round — three real findings and one improvement they implied:
- RFC 8089 single-slash file URIs (file:/Users/...) matched neither the
scheme rule (required //) nor the POSIX rule (cannot anchor after ':').
The scheme rule now accepts file:/ file:// file:///.
- Windows home roots match case-insensitively (C:\users\Jane Doe) — both
home rules carry the i flag now.
- The extension lookbehind covered 1-4 chars, so .jsonl/.parquet paths ate
a following prose word (and skewed hashError grouping). Replaced with a
single variable-length (1-8) lookbehind — and applied the same lookbehind
to the generic rules' end-of-string trailing-word tails, so prose after
any dotted filename now survives everywhere ("dbt.exe ENOENT" keeps
ENOENT, improving on the previous round's accepted loss).
The fourth reported finding (terminal spaced components mid-sentence) is
stale for its own example — /Users/Jane Doe masks since the previous round
(probe-verified) — and remains only as the documented no-personal-names
residue on ~ and cloud paths.
22 unit tests; 800-message live sweep zero residuals; suites + typecheck
green.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b466d176df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fifth review round, both real (probe-confirmed before fixing):
- The continuation/terminal word classes were ASCII \w, so a spaced
unicode component ("/Users/Jane García/…") broke the chain mid-word and
leaked "ía/client/model.sql". All six path rules now use \p{L}\p{N}
classes with the u flag (continuation words, terminal words, and the
POSIX base segments so "/données/…" roots match at all). Unspaced
unicode already worked via the permissive tails.
- ":" joins the anchor classes so "ENOENT:/Users/…" and
"source:s3://bucket/key" mask. URL interiors stay safe structurally:
after "https:" comes "//", and a double slash can never start the POSIX
segment chain — locked with negative tests for public URLs,
protocol-relative refs, and host:port/db strings.
25 unit tests; 800-message live sweep zero residuals; adjacent suites +
typecheck green.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12728e0a1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Sixth review round:
- Word classes gain \p{M} and curly apostrophes: NFD-decomposed macOS
names ("Garci" + combining accent) and "O'Connor"-style components mask
whole instead of splitting mid-word (test constructs NFD explicitly via
normalize("NFD") so the decomposition is real, not editor-normalized).
- ";" and "<" join the anchor classes ("ENOENT;/Users/…",
"failed </Users/…/model.sql>"), with ";" and ">" joining the closing
exclusions so delimiters survive ("<<path>>", "…;<path>;done").
- The ">" exclusion exposed a latent ordering hazard: the email rule ran
BEFORE the path rules and fragmented URI userinfo
("abfss://container@account…") into "<email>" mid-URI. Path rules now
run after credentials but before email/internal-host — whole URIs mask
before anything can fragment them, which is strictly better privacy.
28 unit tests; 800-message live sweep zero residuals; suites + typecheck
green.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acef14f0e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Seventh review round, one finding: "/Users/Jane/O'Connor/…" stopped at the
apostrophe and leaked the surname + repo layout. The subtlety is that '
is both the quote-rule delimiter and legitimate path content; the
disambiguator is what follows — a word character means path content
(O'Connor), space/punctuation means a closing quote. Implemented as a
guarded alternation '(?=[\p{L}\p{N}_]) in the span, continuation, and
terminal classes, so quoted paths keep their closing quote for the quote
rule ("open '/Users/…/x.sql' failed" still collapses to "open ? failed",
username-free — locked with a test).
30 unit tests; 800-message live sweep zero residuals; suites + typecheck
green.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e1e2a32b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| windows: new RegExp(PM_ANCHOR + "(?:[A-Za-z]:" + SEP_W + "|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + ")|[A-Za-z]:(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\|[\\p{L}_][\\p{L}\\p{N}_-]*\\/)|\\\\\\\\|\\.{1,2}\\\\(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\)|\\\\(?=(?:[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)*\\\\){2}|[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)*\\\\[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])|[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)+\\\\[\\p{L}\\p{N}]))" + pmSpan(SEP_W) + "+" + pmChunks(SEP_W) + pmSpFile(SEP_W) + PM_TERM_COND, "gu"), | ||
| posixHome: new RegExp(PM_ANCHOR + "\\/(?:" + PM_R_P + "+(?:" + PM_SP + "{1,2}" + PM_R_P + "+)*\\/)*(?:" + pmCI("users") + "|" + pmCI("home") + ")\\/" + pmTail(SEP_P, PM_TERM_UNC), "gu"), | ||
| posix: new RegExp(PM_ANCHOR + "(?:\\.{0,2}\\/(?:" + PM_R_P + "+(?:" + PM_SP + "{1,2}" + PM_R_P + "+)*\\/)+" + pmTail(SEP_P, PM_TERM_COND) + "|(?:\\.{1,2}\\/|\\/(?!\\/))" + pmSpan(SEP_P) + "+" + pmSpFile(SEP_P) + "(?<=" + PM_EXT + ")(?=$|[\\s.,;:)\\]}!?]))", "gu"), | ||
| tilde: new RegExp(PM_ANCHOR + "~[\\p{L}\\p{M}\\p{N}_.-]*(?:\\/|\\\\(?=[\\p{L}\\p{N}_-]{2,}))" + pmTail(SEP_W, PM_TERM_UNC), "gu"), |
There was a problem hiding this comment.
Fresh evidence beyond the resolved generic PowerShell tilde case is that this lookahead requires the first two characters after ~\ to be alphanumeric, _, or -. Common home-relative paths such as ~\.config\altimate\secret.json therefore remain completely unchanged and can expose configuration layout or filenames to telemetry; allow dot-prefixed and other path-specific components when a subsequent backslash proves the path shape.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bdbd1b1e — the guard alternation gains a dotfile shape: \. + 2+ word chars + a further separator (~\.config\altimate\secret.json masks) while escaped-dot regex prose (~\.\d) still fails it. Pinned both ways.
| const PATH_RULES = { | ||
| cloud: new RegExp(PM_ANCHOR + "(?:(?:" + [pmCI("gs"), pmCI("s3") + "[anAN]?", pmCI("abfs") + "[sS]?", pmCI("wasb") + "[sS]?", pmCI("adl"), pmCI("dbfs"), pmCI("hdfs")].join("|") + "):\\/\\/|" + pmCI("file") + ":\\/{1,3})" + pmTail(SEP_P, PM_TERM_UNC), "gu"), | ||
| windowsHome: new RegExp(PM_ANCHOR + "(?:(?:\\\\\\\\\\?\\\\)?[A-Za-z]:" + SEP_W + "?|(?:\\\\\\\\(?:\\?\\\\" + pmCI("unc") + "\\\\)?|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + "))(?:" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*" + SEP_W + ")*|" + SEP_W + ")(?:" + pmCI("users") + "|" + pmCI("documents") + " " + pmCI("and") + " " + pmCI("settings") + ")" + SEP_W + pmTail(SEP_W, PM_TERM_UNC), "gu"), | ||
| windows: new RegExp(PM_ANCHOR + "(?:[A-Za-z]:" + SEP_W + "|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + ")|[A-Za-z]:(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\|[\\p{L}_][\\p{L}\\p{N}_-]*\\/)|\\\\\\\\|\\.{1,2}\\\\(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\)|\\\\(?=(?:[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)*\\\\){2}|[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)*\\\\[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])|[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)+\\\\[\\p{L}\\p{N}]))" + pmSpan(SEP_W) + "+" + pmChunks(SEP_W) + pmSpFile(SEP_W) + PM_TERM_COND, "gu"), |
There was a problem hiding this comment.
Fresh evidence beyond the resolved forward-slash drive-relative case is that this branch still requires a letter-first, unspaced component. Valid Windows paths such as C:.client/models/private.sql, C:#client/models/private.sql, and C:client repo/models/private.sql consequently pass through unchanged; permit these path-specific component shapes while retaining the ratio guard for inputs such as C:8/10.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bdbd1b1e — the slash proof's discriminator is now 'contains a letter' instead of 'letter-first word run': C:.client/…, C:#client/…, and C:client repo/… (spaced continuation included) all mask, while C:8/10 and decimal ratios (C:8.5/10) contain no letter and never can. All four pinned.
| const pmSpFile = (sep: string) => "(?:(?:" + PM_SP + "{1,2}" + pmR(sep) + "+){1,4}(?<=" + PM_EXT + "))?" | ||
| const PM_TERM_COND = "(?:(?<!" + PM_EXT + ")" + PM_SP + "{1,2}" + PM_WORD + "+(?=$|[.,;:)\\]}!?]))?" | ||
| const PM_TERM_UNC = | ||
| "(?:(?<!" + PM_EXT + ")" + PM_SP + "{1,2}" + PM_WORD + "+(?:" + PM_SP + "{1,2}(?:[\\p{Lu}\\p{Lo}]" + PM_WORD + "*|(?:v[ao]n|de[nrl]?|d[aiou]|dos|la|les?|los|bin|ibn|al|el|te[nr])(?= )))*)?" |
There was a problem hiding this comment.
When a multiword home name uses nonbreaking spaces and a lowercase particle, the particle alternative's literal (?= ) does not recognize the same horizontal spaces accepted by PM_SP. For example, /Users/Mary van der Berg does not exist becomes <path> der Berg does not exist, leaking the remaining surname; use the shared horizontal-space fragment in this continuation check as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6bdbd1b1e — the particle lookahead uses the shared PM_SP fragment; your NBSP example masks whole. Pinned.
- tilde backslash guard accepts dotfile-shaped first components (~\.config\...); escaped-dot regex prose still fails it - the forward-slash drive-relative proof requires only a letter somewhere in the first component (C:.client/, C:#client/, C:client repo/ mask; C:8/10 and C:8.5/10 ratios never can) - the name-particle lookahead uses the shared horizontal-space fragment, so particles work across NBSP
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bdbd1b1e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| windows: new RegExp(PM_ANCHOR + "(?:[A-Za-z]:" + SEP_W + "|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + ")|[A-Za-z]:(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\|(?=[^\\s\\/\\\\]{0,64}[\\p{L}])" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\/)|\\\\\\\\|\\.{1,2}\\\\(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\)|\\\\(?=(?:[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)*\\\\){2}|[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)*\\\\[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])|[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)+\\\\[\\p{L}\\p{N}]))" + pmSpan(SEP_W) + "+" + pmChunks(SEP_W) + pmSpFile(SEP_W) + PM_TERM_COND, "gu"), | ||
| posixHome: new RegExp(PM_ANCHOR + "\\/(?:" + PM_R_P + "+(?:" + PM_SP + "{1,2}" + PM_R_P + "+)*\\/)*(?:" + pmCI("users") + "|" + pmCI("home") + ")\\/" + pmTail(SEP_P, PM_TERM_UNC), "gu"), | ||
| posix: new RegExp(PM_ANCHOR + "(?:\\.{0,2}\\/(?:" + PM_R_P + "+(?:" + PM_SP + "{1,2}" + PM_R_P + "+)*\\/)+" + pmTail(SEP_P, PM_TERM_COND) + "|(?:\\.{1,2}\\/|\\/(?!\\/))" + pmSpan(SEP_P) + "+" + pmSpFile(SEP_P) + "(?<=" + PM_EXT + ")(?=$|[\\s.,;:)\\]}!?]))", "gu"), | ||
| tilde: new RegExp(PM_ANCHOR + "~[\\p{L}\\p{M}\\p{N}_.-]*(?:\\/|\\\\(?=[\\p{L}\\p{N}_-]{2,}|\\.[\\p{L}\\p{N}_-]{2,}[\\\\\\/]))" + pmTail(SEP_W, PM_TERM_UNC), "gu"), |
There was a problem hiding this comment.
When a PowerShell home-relative path starts with a one-character directory, this lookahead rejects it because the first alternative requires at least two word characters and the dotfile alternative does not apply. For example, ~\a\client\private.sql remains completely unchanged and can expose repository structure through masked_args; allow a one-character component when the following backslash proves that this is a path while retaining the guard against regex prose such as ~\d+.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ddf1dad5d — a one-char first component is accepted when the next component passes the established 2+ word-char / dotfile test: ~\a\client\private.sql masks, while escape chains fail at the second hop (~\w\d+ — d+ isn't a word run; ~\n\r\t — r is one char). All three pinned.
A one-character first component is proven by the NEXT component passing the 2+ word-char or dotfile test, so ~\a\client\private.sql masks while escape chains (~\w\d+, ~\n\r\t) still fail the guard.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddf1dad5d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const PATH_RULES = { | ||
| cloud: new RegExp(PM_ANCHOR + "(?:(?:" + [pmCI("gs"), pmCI("s3") + "[anAN]?", pmCI("abfs") + "[sS]?", pmCI("wasb") + "[sS]?", pmCI("adl"), pmCI("dbfs"), pmCI("hdfs")].join("|") + "):\\/\\/|" + pmCI("file") + ":\\/{1,3})" + pmTail(SEP_P, PM_TERM_UNC), "gu"), | ||
| windowsHome: new RegExp(PM_ANCHOR + "(?:(?:\\\\\\\\\\?\\\\)?[A-Za-z]:" + SEP_W + "?|(?:\\\\\\\\(?:\\?\\\\" + pmCI("unc") + "\\\\)?|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + "))(?:" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*" + SEP_W + ")*|" + SEP_W + ")(?:" + pmCI("users") + "|" + pmCI("documents") + " " + pmCI("and") + " " + pmCI("settings") + ")" + SEP_W + pmTail(SEP_W, PM_TERM_UNC), "gu"), | ||
| windows: new RegExp(PM_ANCHOR + "(?:[A-Za-z]:" + SEP_W + "|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + ")|[A-Za-z]:(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\|(?=[^\\s\\/\\\\]{0,64}[\\p{L}])" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\/)|\\\\\\\\|\\.{1,2}\\\\(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\)|\\\\(?=(?:[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)*\\\\){2}|[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)*\\\\[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])|[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)+\\\\[\\p{L}\\p{N}]))" + pmSpan(SEP_W) + "+" + pmChunks(SEP_W) + pmSpFile(SEP_W) + PM_TERM_COND, "gu"), |
There was a problem hiding this comment.
Fresh evidence beyond the resolved one-character ~\ case is that every current-drive-rooted proof still requires the first component to contain at least two characters. Valid paths such as \a\client\private.sql and \x\customer\models\private.sql therefore remain completely unchanged and can expose repository structure in telemetry; allow a one-character first component when a subsequent component provides the same path-shape proof used by the tilde rule.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0b437165c — the rooted proof gains the same one-hop-deferred alternative as the tilde rule: \a\client\private.sql masks, escape chains (\n\r\t) still fail at the second hop. Pinned.
| // slash-delimited variant: backslash is path content, not a separator | ||
| const PM_R_P = "(?:[^\\s\\/'\"`]|'(?=[\\p{L}\\p{N}_]))" | ||
| const PM_WORD = "(?:[\\p{L}\\p{M}\\p{N}_‘’-]|'(?=[\\p{L}\\p{N}_]))" | ||
| const PM_ANCHOR = "(^|[\\s\"'`=(,[{:;<|>)\\]])" |
There was a problem hiding this comment.
Fresh evidence beyond the fixed ] and ) anchor cases is that } is still absent from this boundary class. Structured messages such as {ENOENT}/Users/jdoe/client/a.sql and field}/home/jane/private.sql failed consequently pass through unchanged, exposing the username and repository layout; accept } when the following text satisfies an existing path rule.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0b437165c — } completes the closer set in the anchor class: {ENOENT}/Users/… masks; brace templates ({a}/{b}) have no path shape and never match. Pinned.
| const PM_WORD = "(?:[\\p{L}\\p{M}\\p{N}_‘’-]|'(?=[\\p{L}\\p{N}_]))" | ||
| const PM_ANCHOR = "(^|[\\s\"'`=(,[{:;<|>)\\]])" | ||
| const PM_SP = "[^\\S\\t\\n\\r\\v\\f]" | ||
| const PM_EXT = "\\.[A-Za-z0-9-]{0,13}[A-Za-z0-9]" |
There was a problem hiding this comment.
Fresh evidence beyond the length and hyphen extension fixes is that PM_EXT still permits only ASCII characters. Explicit terminal paths such as ./customer.配置, ../秘密.配置, and /秘密.配置 therefore remain completely unchanged and can expose customer filenames; accept Unicode letters, marks, and numbers in the bounded extension proof.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0b437165c — PM_EXT accepts Unicode letters/numbers: ./customer.配置 and /秘密.配置 mask. Pinned.
- the rooted proof gains the one-hop-deferred alternative from the
tilde rule, so \a\client\private.sql masks while escape chains fail
- } completes the closing-delimiter anchor set ({ENOENT}/path)
- PM_EXT accepts unicode letters/numbers (.配置)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b437165c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // slash-delimited variant: backslash is path content, not a separator | ||
| const PM_R_P = "(?:[^\\s\\/'\"`]|'(?=[\\p{L}\\p{N}_]))" | ||
| const PM_WORD = "(?:[\\p{L}\\p{M}\\p{N}_‘’-]|'(?=[\\p{L}\\p{N}_]))" | ||
| const PM_ANCHOR = "(^|[\\s\"'`=(,[{:;<|>)\\]}])" |
There was a problem hiding this comment.
When a failed Bash command uses compact control-operator syntax such as cmd&&/Users/jdoe/client/private.sh, Bash executes the pathname as the next command, but PM_ANCHOR does not accept &; consequently maskArgs leaves the username and repository path unchanged in telemetry. Include ampersands among the boundaries when the following text satisfies an existing path rule.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f902eea91 — & joins the anchor class: cmd&&/Users/… masks; AT&T report has no path shape and never matches. Pinned.
| const PATH_RULES = { | ||
| cloud: new RegExp(PM_ANCHOR + "(?:(?:" + [pmCI("gs"), pmCI("s3") + "[anAN]?", pmCI("abfs") + "[sS]?", pmCI("wasb") + "[sS]?", pmCI("adl"), pmCI("dbfs"), pmCI("hdfs")].join("|") + "):\\/\\/|" + pmCI("file") + ":\\/{1,3})" + pmTail(SEP_P, PM_TERM_UNC), "gu"), | ||
| windowsHome: new RegExp(PM_ANCHOR + "(?:(?:\\\\\\\\\\?\\\\)?[A-Za-z]:" + SEP_W + "?|(?:\\\\\\\\(?:\\?\\\\" + pmCI("unc") + "\\\\)?|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + "))(?:" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*" + SEP_W + ")*|" + SEP_W + ")(?:" + pmCI("users") + "|" + pmCI("documents") + " " + pmCI("and") + " " + pmCI("settings") + ")" + SEP_W + pmTail(SEP_W, PM_TERM_UNC), "gu"), | ||
| windows: new RegExp(PM_ANCHOR + "(?:[A-Za-z]:" + SEP_W + "|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + ")|[A-Za-z]:(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\|(?=[^\\s\\/\\\\]{0,64}[\\p{L}])" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\/)|\\\\\\\\|\\.{1,2}\\\\(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\)|\\\\(?=(?:[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)*\\\\){2}|[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)*\\\\[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])|[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)+\\\\[\\p{L}\\p{N}]|[\\p{L}\\p{N}_-]\\\\(?:[\\p{L}\\p{N}_-]{2,}|\\.[\\p{L}\\p{N}_-]{2,})))" + pmSpan(SEP_W) + "+" + pmChunks(SEP_W) + pmSpFile(SEP_W) + PM_TERM_COND, "gu"), |
There was a problem hiding this comment.
When a forward-slash drive-relative path has its first letter after the 64-character window, such as C: followed by 65 digits and a/models/private.sql, this lookahead fails and the complete path reaches telemetry unchanged. Such a component is still within Windows' filesystem limit; use the same component-sized bound applied by the delimiter proofs rather than this shorter arbitrary window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f902eea91 — the letter scan uses the same 256 component bound as the delimiter proofs; your 65-digit example masks. Pinned.
| // terminal dotted filename: up to four spaced words that END in an extension | ||
| const pmSpFile = (sep: string) => "(?:(?:" + PM_SP + "{1,2}" + pmR(sep) + "+){1,4}(?<=" + PM_EXT + "))?" |
There was a problem hiding this comment.
When an explicit shallow path has five or more spaced continuations, for example read ./Q1 final audited customer revenue report.sql failed, the bounded repetition rejects the terminal filename and the entire customer-specific path remains unchanged. Filesystems do not impose a four-word limit, so keep the match linear using a component-length bound rather than limiting the number of words.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f902eea91 — with an important scope line: explicit shallow paths (./-prefixed, where everything up to the boundary is filename) now use a length-bounded variant (12 words × 64 chars), so your example masks. Deep tails keep {1,4} deliberately — there the bound is a prose cap, not a filesystem claim: read /opt/x error reading the project config.yml here must not chase the dotted token through five prose words (this exact trade-off was probed and documented in the earlier spfile thread; the control is re-pinned alongside your fix).
| const PM_WORD = "(?:[\\p{L}\\p{M}\\p{N}_‘’-]|'(?=[\\p{L}\\p{N}_]))" | ||
| const PM_ANCHOR = "(^|[\\s\"'`=(,[{:;<|>)\\]}])" | ||
| const PM_SP = "[^\\S\\t\\n\\r\\v\\f]" | ||
| const PM_EXT = "\\.[\\p{L}\\p{N}-]{0,13}[\\p{L}\\p{N}]" |
There was a problem hiding this comment.
Fresh evidence after the Unicode-extension fix is that decomposed and mark-bearing extensions are still excluded: ./customer.é and ./customer.क़ remain completely unchanged because PM_EXT allows letters and numbers but not \p{M}. Include Unicode marks in the bounded extension proof so these explicit paths do not expose customer filenames.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f902eea91 — \p{M} joins both positions of PM_EXT, so decomposed ./customer.é proves. Pinned.
…sions
- & joins the anchor class (cmd&&/path shell syntax)
- the drive-relative letter scan uses the 256 component bound instead
of an arbitrary 64
- explicit shallow paths (./ prefix) use a length-bounded spaced-
filename variant (12 words x 64 chars); deep tails keep the {1,4}
prose cap deliberately — everything after ./ is filename, prose
after a deep path is not
- PM_EXT accepts combining marks (decomposed accents)
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f902eea917
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const PATH_RULES = { | ||
| cloud: new RegExp(PM_ANCHOR + "(?:(?:" + [pmCI("gs"), pmCI("s3") + "[anAN]?", pmCI("abfs") + "[sS]?", pmCI("wasb") + "[sS]?", pmCI("adl"), pmCI("dbfs"), pmCI("hdfs")].join("|") + "):\\/\\/|" + pmCI("file") + ":\\/{1,3})" + pmTail(SEP_P, PM_TERM_UNC), "gu"), | ||
| windowsHome: new RegExp(PM_ANCHOR + "(?:(?:\\\\\\\\\\?\\\\)?[A-Za-z]:" + SEP_W + "?|(?:\\\\\\\\(?:\\?\\\\" + pmCI("unc") + "\\\\)?|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + "))(?:" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*" + SEP_W + ")*|" + SEP_W + ")(?:" + pmCI("users") + "|" + pmCI("documents") + " " + pmCI("and") + " " + pmCI("settings") + ")" + SEP_W + pmTail(SEP_W, PM_TERM_UNC), "gu"), | ||
| windows: new RegExp(PM_ANCHOR + "(?:[A-Za-z]:" + SEP_W + "|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + ")|[A-Za-z]:(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\|(?=[^\\s\\/\\\\]{0,256}[\\p{L}])" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\/)|\\\\\\\\|\\.{1,2}\\\\(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\)|\\\\(?=(?:[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)*\\\\){2}|[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)*\\\\[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])|[\\p{L}\\p{N}_#@().'-]{2,}(?:" + PM_SP + "{1,2}[\\p{L}\\p{N}_#@().'-]+)+\\\\[\\p{L}\\p{N}]|[\\p{L}\\p{N}_-]\\\\(?:[\\p{L}\\p{N}_-]{2,}|\\.[\\p{L}\\p{N}_-]{2,})))" + pmSpan(SEP_W) + "+" + pmChunks(SEP_W) + pmSpFile(SEP_W) + PM_TERM_COND, "gu"), |
There was a problem hiding this comment.
Fresh evidence after the earlier punctuation allowlist fix is that valid current-drive-rooted paths whose first component contains other Windows-valid symbols still bypass masking. For example, Node recognizes \C++ Projects\client\secret.sql and \R&D\client\secret.sql as absolute Windows paths, but maskString leaves both completely unchanged because every rooted-path proof rejects + and &; use the later separator/component structure to prove these paths without restricting the first component to this allowlist.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a811a2325 — components accept +/& with a length-3 floor: \C++ Projects\… and \R&D\… mask, while quantified escapes (\n+\r+, \d+) are exactly the length-2 letter+quantifier shape the floor excludes — that's why it's a floor rather than the unrestricted proof you suggested. Both directions pinned.
| const PM_WORD = "(?:[\\p{L}\\p{M}\\p{N}_‘’-]|'(?=[\\p{L}\\p{N}_]))" | ||
| const PM_ANCHOR = "(^|[\\s\"'`=(,[{:;<|>)\\]}&])" | ||
| const PM_SP = "[^\\S\\t\\n\\r\\v\\f]" | ||
| const PM_EXT = "\\.[\\p{L}\\p{M}\\p{N}-]{0,13}[\\p{L}\\p{M}\\p{N}]" |
There was a problem hiding this comment.
Fresh evidence after raising the previous cutoff to 14 characters is the standard Sublime Text filename ./customer.sublime-workspace: its 17-character extension exceeds PM_EXT, so this explicit dot-relative path—and the equally unambiguous /customer.sublime-workspace—passes through unchanged. Because these shallow path branches rely on PM_EXT as their proof, the arbitrary cap still exposes customer filenames; bound the whole component for performance rather than limiting extension length.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a811a2325 — window raised to 30 chars (.sublime-workspace = 17 proves). Kept as a bounded window rather than component-length because PM_EXT also serves in variable-length lookbehinds where an unbounded form regresses performance; at 30 the suppression direction only gets more conservative (pinned).
- rooted-proof components accept + and & at length >=3 (\C++ Projects\, \R&D\) — the length floor keeps quantified escapes (\n+, \d+) out - PM_EXT window raised to 30 (.sublime-workspace); suppression direction stays conservative
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a811a23258
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const PATH_RULES = { | ||
| cloud: new RegExp(PM_ANCHOR + "(?:(?:" + [pmCI("gs"), pmCI("s3") + "[anAN]?", pmCI("abfs") + "[sS]?", pmCI("wasb") + "[sS]?", pmCI("adl"), pmCI("dbfs"), pmCI("hdfs")].join("|") + "):\\/\\/|" + pmCI("file") + ":\\/{1,3})" + pmTail(SEP_P, PM_TERM_UNC), "gu"), | ||
| windowsHome: new RegExp(PM_ANCHOR + "(?:(?:\\\\\\\\\\?\\\\)?[A-Za-z]:" + SEP_W + "?|(?:\\\\\\\\(?:\\?\\\\" + pmCI("unc") + "\\\\)?|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + "))(?:" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*" + SEP_W + ")*|" + SEP_W + ")(?:" + pmCI("users") + "|" + pmCI("documents") + " " + pmCI("and") + " " + pmCI("settings") + ")" + SEP_W + pmTail(SEP_W, PM_TERM_UNC), "gu"), | ||
| windows: new RegExp(PM_ANCHOR + "(?:[A-Za-z]:" + SEP_W + "|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + ")|[A-Za-z]:(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\|(?=[^\\s\\/\\\\]{0,256}[\\p{L}])" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\/)|\\\\\\\\|\\.{1,2}\\\\(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\)|\\\\(?=(?:" + PM_WC + "(?:" + PM_SP + "{1,2}" + PM_WCC + ")*\\\\){2}|" + PM_WC + "(?:" + PM_SP + "{1,2}" + PM_WCC + ")*\\\\[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])|" + PM_WC + "(?:" + PM_SP + "{1,2}" + PM_WCC + ")+\\\\[\\p{L}\\p{N}]|[\\p{L}\\p{N}_-]\\\\(?:[\\p{L}\\p{N}_-]{2,}|\\.[\\p{L}\\p{N}_-]{2,})))" + pmSpan(SEP_W) + "+" + pmChunks(SEP_W) + pmSpFile(SEP_W) + PM_TERM_COND, "gu"), |
There was a problem hiding this comment.
When stderr contains an extension-bearing single-component Windows path such as read .\customer_secret.sql failed, ..\customer_secret.sql, or \customer_secret.sql, every opener alternative here requires a later separator, so the string remains completely unchanged. These explicit prefixes already distinguish filesystem paths, and the analogous POSIX forms are masked, so accept a dotted terminal component after the Windows dot-relative and current-drive-rooted prefixes to prevent customer filenames from reaching telemetry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in abd0ff463 — see the cubic/codex threads at this line; the dotted-terminal proof covers all three shallow forms.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
A run of 4+ separators is never a real path shape, but the composed windows rule paid ~10µs/char consuming it in the regex interpreter — 20k backslashes cost ~200ms and tripped the upstream DoS-budget suite (v140-merge-chaos). One cheap anchored pre-rule collapses such runs to <path> before the heavy rules run; the chaos suite passes with margin.
…ndows terminals - layer zero: paths under the local user's home and cwd are replaced by EXACT literal match before any structural rule runs (the approach used by Salesforce's telemetry GDPR scrub and gatsby-telemetry's cleanPaths) — every username shape masks with zero false positives; variants cover JSON-doubled backslashes and swapped separators - fast path: strings without a separator skip the whole path stack; the two windows rules (widest opener alternation) additionally skip unless a backslash, boundary drive-colon, or non-scheme // exists — coverage-neutral, mean corpus cost 252µs -> 213µs - shallow explicit windows terminals mask (.\secret.sql, \secret.sql) via the dotted-terminal proof; regex escape prose still fails it
The suite had accumulated as one describe per review round — a changelog, not a spec. Reorganized into thirteen subject sections (preprocessing, home roots, tails, windows/posix/tilde/cloud families, anchors, in-component delimiters, extensions, must-not-mask, composition, performance) with every test body preserved verbatim: same 115 tests, same 241 assertions.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- known-prefix literals run AFTER the structural rules: replacing the prefix first stripped the opener and orphaned terminal spaced components ($HOME/client repo leaked 'repo'); structure now sees the original string and literals mop up what it missed - literal matchers carry a token-boundary lookbehind so they never fire mid-URL or mid-token - drive-relative single-component files (C:secret.sql) mask via a letter-first dotted-terminal proof; the main gate opens on a boundary drive-colon since such strings have no separator
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ced57f47df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| windows: new RegExp(PM_ANCHOR + "(?:[A-Za-z]:" + SEP_W + "|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + ")|[A-Za-z]:(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\|(?=[^\\s\\/\\\\]{0,256}[\\p{L}])" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\/|[\\p{L}_][^\\s\\/\\\\]{0,254}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?]))|\\\\\\\\|\\.{1,2}\\\\(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\|[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?]))|\\\\(?=(?:" + PM_WC + "(?:" + PM_SP + "{1,2}" + PM_WCC + ")*\\\\){2}|" + PM_WC + "(?:" + PM_SP + "{1,2}" + PM_WCC + ")*\\\\[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])|" + PM_WC + "(?:" + PM_SP + "{1,2}" + PM_WCC + ")+\\\\[\\p{L}\\p{N}]|[\\p{L}\\p{N}_-]\\\\(?:[\\p{L}\\p{N}_-]{2,}|\\.[\\p{L}\\p{N}_-]{2,})|[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])))" + pmSpan(SEP_W) + "+" + pmChunks(SEP_W) + pmSpFile(SEP_W) + PM_TERM_COND, "gu"), | ||
| posixHome: new RegExp(PM_ANCHOR + "\\/(?:" + PM_R_P + "+(?:" + PM_SP + "{1,2}" + PM_R_P + "+)*\\/)*(?:" + pmCI("users") + "|" + pmCI("home") + ")\\/" + pmTail(SEP_P, PM_TERM_UNC), "gu"), | ||
| posix: new RegExp(PM_ANCHOR + "(?:\\.{0,2}\\/(?:" + PM_R_P + "+(?:" + PM_SP + "{1,2}" + PM_R_P + "+)*\\/)+" + pmTail(SEP_P, PM_TERM_COND) + "|(?:\\.{1,2}\\/|\\/(?!\\/))" + pmSpan(SEP_P) + "+" + pmSpFileX(SEP_P) + "(?<=" + PM_EXT + ")(?=$|[\\s.,;:)\\]}!?]))", "gu"), | ||
| tilde: new RegExp(PM_ANCHOR + "~[\\p{L}\\p{M}\\p{N}_.-]*(?:\\/|\\\\(?=[\\p{L}\\p{N}_-]{2,}|\\.[\\p{L}\\p{N}_-]{2,}[\\\\\\/]|[\\p{L}\\p{N}_-]\\\\(?:[\\p{L}\\p{N}_-]{2,}|\\.[\\p{L}\\p{N}_-]{2,})))" + pmTail(SEP_W, PM_TERM_UNC), "gu"), |
There was a problem hiding this comment.
When PowerShell reports a home-relative path whose first directory contains Windows-valid symbols, such as ~\C++ Projects\client\secret.sql or ~\R&D\client\secret.sql, this lookahead rejects the opener and maskString leaves the entire repository path unchanged. Fresh evidence beyond the resolved generic backslash-tilde and rooted-symbol cases is that symbol support was added only to the current-drive-rooted component proof, not this tilde branch; apply the same guarded component proof here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a32d716a5 — the tilde-backslash proof now reuses the rooted proof's component classes (+/& at length 3+, spaced continuations): ~\C++ Projects\client\secret.sql and ~\R&D\… mask; quantified escapes still fail the length floor. Pinned.
| const PATH_RULES = { | ||
| cloud: new RegExp(PM_ANCHOR + "(?:(?:" + [pmCI("gs"), pmCI("s3") + "[anAN]?", pmCI("abfs") + "[sS]?", pmCI("wasb") + "[sS]?", pmCI("adl"), pmCI("dbfs"), pmCI("hdfs")].join("|") + "):\\/\\/|" + pmCI("file") + ":\\/{1,3})" + pmTail(SEP_P, PM_TERM_UNC), "gu"), | ||
| windowsHome: new RegExp(PM_ANCHOR + "(?:(?:\\\\\\\\\\?\\\\)?[A-Za-z]:" + SEP_W + "?|(?:\\\\\\\\(?:\\?\\\\" + pmCI("unc") + "\\\\)?|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + "))(?:" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*" + SEP_W + ")*|" + SEP_W + ")(?:" + pmCI("users") + "|" + pmCI("documents") + " " + pmCI("and") + " " + pmCI("settings") + ")" + SEP_W + pmTail(SEP_W, PM_TERM_UNC), "gu"), | ||
| windows: new RegExp(PM_ANCHOR + "(?:[A-Za-z]:" + SEP_W + "|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + ")|[A-Za-z]:(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\|(?=[^\\s\\/\\\\]{0,256}[\\p{L}])" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\/|[\\p{L}_][^\\s\\/\\\\]{0,254}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?]))|\\\\\\\\|\\.{1,2}\\\\(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\|[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?]))|\\\\(?=(?:" + PM_WC + "(?:" + PM_SP + "{1,2}" + PM_WCC + ")*\\\\){2}|" + PM_WC + "(?:" + PM_SP + "{1,2}" + PM_WCC + ")*\\\\[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])|" + PM_WC + "(?:" + PM_SP + "{1,2}" + PM_WCC + ")+\\\\[\\p{L}\\p{N}]|[\\p{L}\\p{N}_-]\\\\(?:[\\p{L}\\p{N}_-]{2,}|\\.[\\p{L}\\p{N}_-]{2,})|[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])))" + pmSpan(SEP_W) + "+" + pmChunks(SEP_W) + pmSpFile(SEP_W) + PM_TERM_COND, "gu"), |
There was a problem hiding this comment.
When a drive-relative path is a single dotted file whose basename starts with a digit or punctuation, or contains a space—for example C:123_customer_secret.sql, C:.env.local, or C:customer secret.sql—the terminal-file proof accepts only a letter/underscore-leading unspaced name, so the filename reaches telemetry unchanged. The fast-path gate also skips the digit/punctuation-leading forms entirely; widen both checks using the dotted terminal component itself as the path discriminator.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a32d716a5 — the terminal proof now accepts any spaced run using the discriminator that actually separates the classes: a letter in the extension. C:123_customer_secret.sql, C:.env.local, and C:customer secret.sql mask, while C:8.5 and C:1.2.3 have all-numeric extensions and structurally never qualify (all five pinned). The fast-path gate opens on any non-space after a boundary drive-colon.
- the backslash-tilde proof reuses the rooted proof's component classes (+& at length 3+, spaced continuations): ~\C++ Projects\... masks - the drive-relative terminal proof accepts any (spaced) run whose EXTENSION carries a letter — C:123_report.sql, C:.env.local, and C:customer secret.sql mask while versions and ratios (C:8.5, C:1.2.3) have all-numeric extensions and never qualify; the fast-path gate opens on any non-space after a boundary drive-colon
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
| const PATH_RULES = { | ||
| cloud: new RegExp(PM_ANCHOR + "(?:(?:" + [pmCI("gs"), pmCI("s3") + "[anAN]?", pmCI("abfs") + "[sS]?", pmCI("wasb") + "[sS]?", pmCI("adl"), pmCI("dbfs"), pmCI("hdfs")].join("|") + "):\\/\\/|" + pmCI("file") + ":\\/{1,3})" + pmTail(SEP_P, PM_TERM_UNC), "gu"), | ||
| windowsHome: new RegExp(PM_ANCHOR + "(?:(?:\\\\\\\\\\?\\\\)?[A-Za-z]:" + SEP_W + "?|(?:\\\\\\\\(?:\\?\\\\" + pmCI("unc") + "\\\\)?|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + "))(?:" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*" + SEP_W + ")*|" + SEP_W + ")(?:" + pmCI("users") + "|" + pmCI("documents") + " " + pmCI("and") + " " + pmCI("settings") + ")" + SEP_W + pmTail(SEP_W, PM_TERM_UNC), "gu"), |
There was a problem hiding this comment.
WARNING: JSON-doubled separators defeat the windowsHome server loop — doubled UNC/drive home paths leak spaced personal names
stat \\fileserver\Users\Jane Doe gone masks whole via windowsHome + PM_TERM_UNC (your test pins <path> does not exist). But the JSON-embedded form \\fileserver\\Users\\Jane Doe gone does not: every iteration of (?:PM_R+(?:PM_SP{1,2}PM_R+)*SEP_W)* ends with exactly one SEP_W character, and PM_R excludes backslash, so the second backslash of a doubled pair can never be crossed before users must match (the \\?\ extended-length prefix and the ?\UNC\ variant don't help either). The only surviving opener is the generic windows rule (\\ opener + pmSpan(SEP_W)+), whose tail is PM_TERM_COND — end-of-string/punctuation only — so the result is read <path> Jane Doe gone, leaking the personal name this rule family exists to mask. Same shape for doubled drive homes (C:\\Users\\Mary Jane Smith gone → <path> Mary Jane Smith gone) whenever the user is not the local one PM_KNOWN_PREFIXES covers. Doubled separators demonstrably occur in the corpus — the JSON-doubled PM_KNOWN_PREFIXES variant and the [\\/]{4,} collapse rule exist for exactly that form — and the in-code residue note promises "no personal names" for this leak class. Consider SEP_W{1,2} in the server/home separator positions, or document this as residue.
Reply with @kilo-code-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Confirmed and fixed in 35320edc2 — probe reproduced the leak exactly (\\fileserver\\Users\\Jane Doe gone → <path> Doe gone). windowsHome's structured separators (segment loop, drive branch, the Users-adjacent one) now accept {1,2}, so JSON-doubled homes keep the high-PII tail; the tail itself already consumed doubled backslashes as span content. Both doubled forms pinned.
| }) | ||
|
|
||
| it("nonbreaking spaces continue components; tabs stay boundaries", () => { | ||
| expect(mask("read /Users/Jane Doe/client/model.sql failed")).toBe("read <path> failed") |
There was a problem hiding this comment.
WARNING: Both "nonbreaking spaces" tests use regular U+0020 spaces — NBSP continuation is not actually covered
Verified there is no U+00A0 byte anywhere in this file: nonbreaking spaces continue components (this line) and name particles work across nonbreaking spaces (line 215) both pass plain-space inputs. So the PM_SP ([^\S\t\n\r\v\f]) NBSP behavior — the thing commits af81386/6bdbd1b1e claim to harden, and the PR description sells ("handles every username shape — spaces, NBSP, Unicode") — would regress silently if PM_SP were ever narrowed to a literal space; the suite stays green either way. Additionally, line 215 is an exact duplicate of line 203's assertion in "PII tails cross lowercase name particles" (same input, same expectation), so as written it adds no coverage at all. Suggest explicit \u00A0 escapes in these inputs, e.g. mask("read /Users/Jane\u00A0Doe/client/model.sql failed") and "/Users/Mary\u00A0van\u00A0der\u00A0Berg does not exist".
Reply with @kilo-code-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Half-confirmed, fully fixed in 35320edc2 — line 210 actually DID carry a genuine U+00A0 (an invisible literal byte, which your scan missed — proving your own point about unreviewability), while the particles test was plain-spaced exactly as you said. Both now construct \u00A0 explicitly, with a comment explaining why no invisible bytes are allowed in this file. Runtime behavior verified correct with constructed NBSPs either way.
- windowsHome's structured separators accept doubling ({1,2}), so
JSON-printed UNC/drive homes keep spaced-name protection
- the NBSP tests now construct U+00A0 explicitly (one relied on an
invisible literal byte, the other tested plain spaces)
- layer-ordering comment corrected (literals mop up AFTER structure);
the trailing-word doctrine now states the spaced-filename interior
reach; a duplicated timing probe got a distinct spaced-run shape
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Review summary — request changes
The privacy goal here is real and the write-up is unusually rigorous. The high-PII coverage genuinely works: WSL mounts (/mnt/c/… and \\wsl$\…), Documents and Settings, extended-length UNC, backslash UNC with FQDN hosts, NBSP usernames, NFD-decomposed names, 8.3 short names, and the file: URI forms all mask correctly in my testing. The \p{Lu} / /iu engine-divergence catch is a genuinely subtle find, and hand-expanding the case classes is the right call.
Two things block merge, both left as inline comments:
- Over-masking (
pmChunks) — any later slash in the message makes the path run swallow everything in between. 9 of 12 realistic error messages lost the majority of their content, and distinct errors now collide underhashError(masked). This works directly against the clustering value the PR is written to preserve. - Quadratic in
PATH_RULES.windows— 2.4 s on a 32 KB drive-colon string, reachable from ordinary Snowflakev:colSQL viaregister.ts:433, which masks the full statement before truncating.
Plus three correctness items inline: the performance tests can't detect the class of bug they're named for; FQDN forward-slash UNC leaks; process.cwd() goes stale after chdir.
Everything below is non-blocking.
Leaks worth closing
| input | output |
|---|---|
Error: could not read %2FUsers%2Fjsmith%2Fsecret%2Fa.sql |
unchanged |
GET https://example.com/?file=%2FUsers%2Fjdoe%2Fclient%2Fsecret.sql |
unchanged |
stat /Volumes/Clients/Jane Doe does not exist |
stat <path> Doe does not exist |
stat /mnt/shared/Jane Doe does not exist |
stat <path> Doe does not exist |
ENOENT: $HOME/projects/jsmith/a.sql |
unchanged |
ENOENT: %USERPROFILE%\Documents\jsmith.sql |
unchanged |
read $XDG_CONFIG_HOME/app/config.json |
unchanged |
stat ~jane does not exist |
unchanged |
cannot stat Users/jsmith/a.sql |
unchanged |
open vscode://file/Users/jdoe/x.sql failed |
unchanged |
Notes on a few of these:
- Percent-encoded separators. Rules inspect only literal
/and\. These show up in HTTP-layer errors and are routinely decoded by whoever is reading App Insights. Detecting case-insensitive%2f/%5cwould close it. - Generic non-home paths carrying names. The terminal rule takes only one trailing word, and the comment claims "no personal names in that class" — external volumes, network mounts, and container mounts routinely carry them. Applying the capitalized/caseless name-tail logic to generic absolute paths would fix it.
- Env-var roots. Note that adding
$%toPM_ANCHORdoes not fix$HOME/...— the$sits beforeHOME, not before the separator. Explicit root alternatives are needed:\$\{?HOME\}?,\$PWD,%USERPROFILE%,%HOMEDRIVE%%HOMEPATH%. - Bare
~user. The tilde rule requires a following separator, so a bare home reference — which the description lists as covered — is missed.
Over-masking beyond the blocking case
invalid pattern /^abc$/ in config → invalid pattern <path> in config. Validation errors that echo the offending pattern lose it.
Tests
Two assertions are environment-dependent. Lines 42 and 51 bake in the assumption that posixHome matched and consumed a trailing word. When home isn't under /Users/ or /home/, only the literal pass fires and the assertion breaks:
os.homedir() |
line 41 | line 42 | line 46 | line 51 |
|---|---|---|---|---|
/Users/jdoe |
pass | pass | pass | pass |
/home/runner |
pass | pass | pass | pass |
/root |
pass | fail | pass | fail |
/github/home |
pass | fail | pass | pass |
/root is the default for containers running as root, /github/home for GitHub Actions container jobs. The suite is green today because the runner happens to use /home/runner. Asserting the invariant instead — not.toContain(home) plus toContain("<path>") — or stubbing os.homedir() would make it portable.
Line 706 pins a repo-structure leak as correct. mask("/Users/jdoe/client\tsecret/models/a.sql") → "<path> secret/models/a.sql". It's documented as the undecidable-fragment boundary, but repo structure is the second thing this PR sets out to protect, and the doctrine elsewhere is that over-masking is the correct failure mode. Worth making that call deliberately rather than inheriting it from a regex boundary.
Two assertions can't fail. expect(mask("open '/Users/jdoe/x.sql' failed")).not.toContain("jdoe") passes for almost any output — thin, given the PR history cites a quote-pairing regression. And it("separator-free strings take the fast path unchanged") asserts only that output equals input, which is equally true if the whole stack ran and matched nothing; it cannot fail if the gate is deleted.
Gaps
- Path + a second slash in one message — the shape behind the blocking over-masking bug. No test contains both, which is why it got this far.
- Growth-rate assertions rather than thresholds.
- Drive-colon-without-separator input at any size.
- Old-vs-new differential on the live corpus, gating on over-mask count, not only leak count.
- FQDN UNC servers;
chdirafter import; non-home mounts with multi-word names;$VAR/%VAR%roots; percent-encoded separators;homedir()stubbing. - Multi-line messages — stack traces with a path per line are the dominant real shape, and only one test touches it despite the
\s+collapse interacting with it.
Smaller things
/Users/j/x sk-abcdefghij…→<path>***(was<path> sk-***). Credential still masked; only the marker identifying the secret class is lost.dbfs:/…andhdfs:/…leave the scheme visible (dbfs:<path>) wherefile:handles:\/{1,3}. Cosmetic, no leak.- The
out = out/.replace(...)chains around lines 1453-1495 sit at mixed indentation levels, which makes the gated Windows block harder to read than it should be — in the one place where a misread costs a leak.
Method
Findings were verified by extracting the shipped PATH_RULES and maskString verbatim and running them on Node 25 / V8, with per-rule isolation for the timing attribution. Happy to share the probe scripts if useful.
| const pmR = (sep: string) => (sep === SEP_P ? PM_R_P : PM_R) | ||
| const pmSpan = (sep: string) => | ||
| "(?:[^\\s'\"`)\\]},;>]|'(?=[\\p{L}\\p{N}_])|[,;)\\]}>](?=" + pmR(sep) + "{0,256}(?:" + PM_SP + "{1,2}" + pmR(sep) + "{1,64}){0,6}(?:" + sep + "|" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])))|[\"'`](?=" + pmR(sep) + "{1,256}(?:" + PM_SP + "{1,2}" + pmR(sep) + "{1,64}){0,6}(?:" + sep + "|" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?]))))" | ||
| const pmChunks = (sep: string) => "(?:(?:" + PM_SP + "{1,2}" + pmR(sep) + "+)+" + sep + pmSpan(sep) + "*)*" |
There was a problem hiding this comment.
Blocking — any later slash in the message makes the path run swallow everything in between.
pmChunks is (?:(?:SP{1,2} R+)+ sep span*)*. A spaced word-run continues the path whenever any separator appears later in the string — not just an adjacent one. A URL, a date, a fraction, a MIME type, or a second path all "prove" the run, so the chain eats the prose between them:
open /app/x.sql failed, see https://docs.example.com/e/123 -> open <path>
open /app/x.sql failed on 8/17/2026 today -> open <path>
open /app/x.sql failed at ratio 1/2 today -> open <path>
open /app/x.sql failed: application/json expected -> open <path>
read C:\proj\x.sql then see https://docs.example.com/e/1 -> read <path>
read ~/proj/x.sql then see … -> read <path>
read s3://b/k.parquet then see … -> read <path>
It hits every rule family, and the span is unbounded — 40 intervening words are still swallowed, since (?:SP{1,2} R+)+ has no cap.
On a 12-message realistic dbt/warehouse corpus, 9 lost the majority of their content:
Model /app/models/a.sql references missing source raw/orders
-> "Model <path>"
MCP server crashed: spawn /usr/local/bin/server ENOENT; see https://modelcontextprotocol.io/docs
-> "MCP server crashed: spawn <path>"
Permission denied: /var/lib/warehouse/data.parquet (uid/gid mismatch)
-> "Permission denied: <path>)"
Runtime Error: Database Error in model x (/Users/j/proj/models/x.sql). Docs: https://docs.snowflake.com/…
-> "Runtime Error: Database Error in model x (<path>"
This works against the PR's own rationale. src/session/prompt.ts:1319 computes errorHash: Telemetry.hashError(masked) — the hash of the masked text — so distinct errors now collide:
A: "Model /app/models/a.sql references missing source raw/orders"
B: "Model /app/models/b.sql references missing column raw/customers"
both -> "Model <path>" => identical errorHash
The description argues masking rather than dropping is justified because the triage pipeline clusters on message content, so "the message content is the product value being protected." That is what this rule destroys. It also contradicts "public https links are deliberately preserved" (they aren't, once a path precedes them), and the "one prose word" residue note — entire clauses go.
The safety argument in the comment — "after https: comes //, which cannot start a segment chain" — reasons about rule opening only. Nothing stops an already-open chain from continuing through a scheme, which is where this lives.
Same root cause, worth noting separately: the <internal-host> classification is lost. read /opt/x.sql then GET http://10.0.0.5/api/y failed previously yielded <internal-host>; it now yields read <path>. Not a leak, but a signal the codebase has a dedicated rule for.
Suggested fix: make the proving separator local rather than anywhere-downstream — bound the (?:SP{1,2} R+)+ repetition (pmSpFile already caps at 4 words) and require the proving separator within a bounded window. Also reject a continuation component containing : followed by // so scheme tokens can't be crossed.
On the corpus sweep: it counted residual path content only, so it structurally cannot see this. Worth re-running as an old-vs-new diff and gating on over-mask count as well as leak count — the "328 over-masked" figure in the table was measured for the VS Code strategy, not this one.
| const PATH_RULES = { | ||
| cloud: new RegExp(PM_ANCHOR + "(?:(?:" + [pmCI("gs"), pmCI("s3") + "[anAN]?", pmCI("abfs") + "[sS]?", pmCI("wasb") + "[sS]?", pmCI("adl"), pmCI("dbfs"), pmCI("hdfs")].join("|") + "):\\/\\/|" + pmCI("file") + ":\\/{1,3})" + pmTail(SEP_P, PM_TERM_UNC), "gu"), | ||
| windowsHome: new RegExp(PM_ANCHOR + "(?:(?:\\\\\\\\\\?\\\\)?[A-Za-z]:" + SEP_W + "{0,2}|(?:\\\\\\\\(?:\\?\\\\" + pmCI("unc") + "\\\\)?|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + "))(?:" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*" + SEP_W + "{1,2})*|" + SEP_W + "{1,2})(?:" + pmCI("users") + "|" + pmCI("documents") + " " + pmCI("and") + " " + pmCI("settings") + ")" + SEP_W + "{1,2}" + pmTail(SEP_W, PM_TERM_UNC), "gu"), | ||
| windows: new RegExp(PM_ANCHOR + "(?:[A-Za-z]:" + SEP_W + "|(?<!:)\\/\\/(?=[^\\s\\/\\\\.]+" + SEP_W + ")|[A-Za-z]:(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\|(?=[^\\s\\/\\\\]{0,256}[\\p{L}])" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\/|" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+){0,6}(?<=\\.[\\p{L}\\p{M}\\p{N}-]{0,29})(?<=[\\p{L}\\p{M}][\\p{L}\\p{M}\\p{N}-]{0,29})(?=$|[\\s.,;:)\\]}!?]))|\\\\\\\\|\\.{1,2}\\\\(?=" + PM_R + "+(?:" + PM_SP + "{1,2}" + PM_R + "+)*\\\\|[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?]))|\\\\(?=(?:" + PM_WC + "(?:" + PM_SP + "{1,2}" + PM_WCC + ")*\\\\){2}|" + PM_WC + "(?:" + PM_SP + "{1,2}" + PM_WCC + ")*\\\\[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])|" + PM_WC + "(?:" + PM_SP + "{1,2}" + PM_WCC + ")+\\\\[\\p{L}\\p{N}]|[\\p{L}\\p{N}_-]\\\\(?:[\\p{L}\\p{N}_-]{2,}|\\.[\\p{L}\\p{N}_-]{2,})|[^\\s\\\\]{1,256}" + PM_EXT + "(?=$|[\\s.,;:)\\]}!?])))" + pmSpan(SEP_W) + "+" + pmChunks(SEP_W) + pmSpFile(SEP_W) + PM_TERM_COND, "gu"), |
There was a problem hiding this comment.
Blocking — this rule is quadratic on drive-colon input, and it's reachable from ordinary product input.
The fast-path gate at line 1459 deliberately admits separator-free strings so drive-relative paths (c:file.txt) can match. This opener then runs [A-Za-z]:(?= R+ (SP{1,2} R+)* \\ | … ) — forward lookaheads that rescan the remainder at every candidate position.
Replacing each rule individually against "a:".repeat(n) isolates the cost entirely here; every other new rule and the pre-existing email regex are free:
| len | cloud | windowsHome | windows | posixHome | posix | tilde | full chain | |
|---|---|---|---|---|---|---|---|---|
| 8 KB | 0.0 | 0.0 | 154.5 | 0.0 | 0.0 | 0.0 | 0.0 | 144.2 |
| 16 KB | 0.0 | 0.0 | 628.2 | 0.0 | 0.0 | 0.0 | 0.0 | 607.4 |
| 32 KB | 0.0 | 0.0 | 2401.3 | 0.0 | 0.0 | 0.0 | 0.1 | 2397.3 |
Doubling the input quadruples the time. full ≈ windows. This is distinct from the pre-existing email-regex quadratic, which only shows on separator-free word runs like "x".repeat(n) (there old and new are both ~120 ms at 16 KB).
Reachability: Snowflake semi-structured access (v:col, v:a:b:c) is idiomatic SQL here, and v: matches (?<![A-Za-z0-9])[A-Za-z]:\S exactly. src/altimate/native/connections/register.ts:433 masks the full SQL and truncates afterwards (Telemetry.maskString(params.sql).slice(0, 2000)); src/altimate/tools/sql-execute.ts:126 does the same via hashError(maskString(sql)).
| generated SQL | length | before | after |
|---|---|---|---|
SELECT v:f0, v:f1, … FROM t (1,000 cols) |
7.9 KB | 0.06 ms | 15.9 ms |
| same, 4,000 cols | 35 KB | 0.25 ms | 325 ms |
| same, 10,000 cols | 89 KB | 0.63 ms | 2,014 ms |
nested v:a0:b0:c0, 4,000 |
81 KB | 0.46 ms | 911 ms |
A wide or generated SELECT over a VARIANT column stalls the CLI for seconds on every failed query.
The "adversarial inputs are bounded (a 20k-character separator run costs <1ms)" claim holds for separator runs. The drive-colon shape has no separator and no bound.
Suggested fix — both:
- Truncate before masking (
maskString(s.slice(0, 8192))at the entry point). This alone caps the worst case regardless of rule shape. - Tighten the gate so a separator-free string only enters the Windows rules with a plausible short filename:
/(?<![A-Za-z0-9])[A-Za-z]:[^\s:]{1,255}\.[A-Za-z]/.
Worth fixing while here: the pre-existing email regex is quadratic on long separator-free runs (2.2 s at 64 KB). A left boundary fixes it — /(?<![A-Za-z0-9._%+-])[A-Za-z0-9._%+-]+@…/.
| // (os.homedir() literal) and gatsby-telemetry's cleanPaths (cwd prefixes). | ||
| const pmEscape = (v: string) => v.replace(/[-[\]/{}()*+?.\\^$|]/g, "\\$&") | ||
| const PM_KNOWN_PREFIXES: RegExp[] = [] | ||
| for (const root of [process.cwd(), os.homedir()]) { |
There was a problem hiding this comment.
Blocking — process.cwd() is captured once at import and goes stale.
PM_KNOWN_PREFIXES is built at module evaluation. The CLI then changes directory at three call sites:
src/cli/cmd/tui.ts:132src/cli/cmd/attach.ts:58src/cli/cmd/run.ts:412
So the active project root never enters the prefix list. That matters because a shallow, extensionless project root is exactly the case where the structural rules don't fire and the literal fallback is the only thing standing between the client's directory name and App Insights:
stat /customer-repo failed -> unchanged
The test at line 45 can't catch this — it uses the same cwd captured at import, and its /models/private.sql suffix would be masked structurally anyway, so the literal pass isn't actually being exercised.
Note the root.length < 4 guard also silently skips / — if telemetry is imported while cwd is / and the process later chdirs into the project, nothing is registered at all.
Suggested fix: cache only os.homedir() at module load; rebuild the cwd literal when process.cwd() changes, or thread the active project root into maskString. Worth a test that imports telemetry, chdirs to a shallow extensionless directory, then masks that directory.
| // inner classes (unambiguous parse => linear time); proof scans are | ||
| // bounded by the filesystem's 255-byte component limit; the nested- | ||
| // quantifier ReDoS shape is banned. | ||
| describe("maskString paths — performance — linearity and DoS budgets", () => { |
There was a problem hiding this comment.
Blocking — these tests can't detect the class of bug they're named for.
All 10 tests in this block have the same shape: run one input size, assert < 500 ms. Nine are named "stays linear". None measures a growth rate, and a single sample point cannot distinguish O(n) from O(n²) — it only distinguishes "fast enough at this one size".
This isn't hypothetical. The quadratic shape in PATH_RULES.windows ("a:".repeat(n)) costs ~60 ms at the 5,000-char sizes used here — comfortably under the budget. It would pass all 10.
There's a failure mode in the other direction too: wall-clock thresholds on shared CI runners are flaky, so a loaded runner can breach 500 ms on genuinely linear input.
Suggested fix — assert the ratio, not the wall clock:
const t = (s: string) => { const a = performance.now(); mask(s); return performance.now() - a }
const growth = (gen: (n: number) => string) => t(gen(8000)) / Math.max(t(gen(1000)), 0.05)
expect(growth(n => "a:".repeat(n))).toBeLessThan(16) // 8x input => <16x timeRun each shape at two sizes and bound t(2n)/t(n) < ~3. The drive-colon shape should be added explicitly — no test currently exercises it at any size.
| expect(mask(String.raw`match \bword\b boundary`)).toBe(String.raw`match \bword\b boundary`) | ||
| }) | ||
|
|
||
| it("forward-slash UNC masks; dotted hosts stay protocol-relative URLs", () => { |
There was a problem hiding this comment.
Blocking — this pin locks in a username + share-layout leak.
The forward-slash UNC branch in PATH_RULES.windows is (?<!:)\/\/(?=[^\s\/\\.]+SEP_W). The . exclusion in the host class rejects any dotted host so that protocol-relative URLs survive — which also means an FQDN UNC server never matches:
ENOENT: //server.example.com/share/Users/jdoe/secret.txt -> unchanged
FQDN server names are normal in corporate UNC. The backslash spelling of the same path masks correctly, so the two spellings disagree on the same input.
I want to be fair about this: it is a deliberate tradeoff — the regex and this test both choose to treat dotted hosts as protocol-relative URLs, and the reasoning is stated. But it lands on under-masking for a username-bearing path, which is the one place the PR's own doctrine ("over-masking is the correct failure mode") argues the other way.
Suggested fix: keep the dotted-host carve-out only when no Users / home / Documents and Settings component follows. This test would then pin two cases: an FQDN UNC path (masked) and an explicitly schemed public URL (preserved).
2 participants
Closes #1118
Why this PR exists
While triaging this product's error telemetry, we found a live 32-machine
core_failure/file_not_foundcluster whose messages carried complete/Users/<name>/…paths — the OS username and the customer's repository layout, verbatim, in App Insights.maskString(the telemetry redactor every error message andmasked_argsvalue passes through) deliberately covered API keys, bearer tokens, emails, internal hosts, and quoted strings — but had no rule for filesystem paths. The gap had stayed invisible for a subtle reason: quoted paths were coincidentally destroyed by the quoted-string rule, so only unquoted paths (File not found: /Users/…— the most common way tools print them) leaked.This PR makes path-shaped content in telemetry text mask to
<path>while keeping the surrounding words intact.Why surgical masking instead of the industry shortcuts
Most mature CLIs never face this problem: the .NET SDK, AWS SAM, Homebrew, and dbt Core send no free text at all (exception names, enumerated events, timings). That is the strongest design, but this codebase sends error messages by design — the downstream error-triage pipeline clusters on them, so the message content is the product value being protected.
The other established shortcut is VS Code's: one crude segment-chain regex that turns anything
word/word-shaped into<REDACTED: user-file-path>, plus nuking any property that merely contains the words password/token/key/secret. We replayed that strategy over 800 distinct live error messages from this product's own telemetry:VS Code's telemetry is stack traces, where the words around a path carry no value. Ours is error prose, where those words — package names, support URLs, "a password must be specified" — are the diagnostic payload. So this PR does the harder thing: mask the path, keep the sentence.
How it works
Masking runs in layers, ordered after the credential rules and before the email/internal-host rules (so whole URIs mask before userinfo can fragment):
s3gsabfsswasbsadldbfshdfsfile:), Windows homes, Windows drive/UNC/drive-relative/rooted forms, POSIX homes, POSIX absolute/dot-relative/shallow forms, and tildes (incl.~userand PowerShell~\). Home-rooted paths and cloud keys get an unconditional spaced-tail (usernames and object keys are the high-PII class); everything else takes a trailing word only at end-of-string or punctuation. Delimiters and quotes inside components count as path content only when a later separator or attached dotted filename proves them — otherwise they are boundaries, which is what keepscd /a/b;ls, closing parens, and quote pairing intact.Fast-path gates keep the cost honest: separator-free strings skip the whole path stack, and the two widest Windows rules additionally skip unless a backslash, boundary drive-colon, or non-scheme
//is present.Doctrine, applied consistently: over-masking is the correct failure mode; public https links are deliberately preserved (support-doc URLs in errors are triage signal); past an unproven delimiter, "path content vs prose" is undecidable and stays a documented boundary rather than a heuristic guess.
How it was hardened
The rules went through ~30 rounds of adversarial AI review (Codex, cubic, Kilo, CodeRabbit), with every finding probe-verified against the real corpus before any fix — several were rejected with evidence instead. Every round re-ran all 800 distinct 30-day live telemetry messages through the patched chain: zero residual path content throughout (intentional survivors: public https doc-links). Catches worth naming:
/iu, spec-conformant engines case-fold\p{Lu}(V8 folds, JSC does not), which would have turned the capitalized-tail gate into "eat all prose" on any V8 runtime while tests stayed green on Bun — theiflag is gone, case-insensitivity is hand-expanded per letter, behavior verified identical on both engines;/mnt/c/Users/Jane Doe), JSON-doubled separators, NBSP-bearing usernames, and legacyDocuments and Settingsroots — all real-world spellings of the same high-PII class.Performance
Mean over the live corpus: ~220µs/message; ~4 masked fields per telemetry event, so well under 1ms per event, entirely off the interactive path. Adversarial inputs are bounded (a 20k-character separator run costs <1ms). Timing regression tests pin the adversarial shapes.
Verification
test/telemetry/mask-file-paths.test.ts: ~120 tests / 250+ assertions, organized by subject (preprocessing, each path family, anchors, in-component delimiters, extensions, the must-not-mask set, performance budgets) with doctrine comments per section.tsgo --noEmitpass; cross-engine spot-checks on JSC and V8 agree.Documented residue (by design, not defects): one prose word may be over-masked after an extensionless home/cloud path; a non-home path's terminal spaced component can leak one structure word mid-sentence (no personal names in that class); an unproven delimiter is a permanent boundary; spaced terminal filenames may span interior words only when the run ends in a dotted extension (4 words on deep paths, 12 on explicit
./-style paths).🤖 Generated with Claude Code
https://claude.ai/code/session_01LKJeLDMhBaYu16LrjGCf25
Summary by cubic
Masks filesystem, UNC, cloud-storage, and tilde paths in telemetry error text to prevent leaking usernames and repo structure. Previously, unquoted non-HTTP paths were sent verbatim; now they are replaced with . Public https links remain, and error-grouping keys may change once as variants collapse.
packages/opencode) using composed PATH_RULES; replaces recognized paths with .gs://,s3:///s3a://,abfss://,wasbs://,adl://,dbfs://,hdfs://,file:/file://file:///); Windows drive/UNC (incl. extended-length, forward-slash UNC, current-drive rooted, drive-relative, and JSON-doubled home separators); POSIX absolute/dot-relative/single-file; and tildes (~,~user, PowerShell~\). Continues across spaces, Unicode/combining marks, and NBSP.packages/opencode/test/telemetry/mask-file-paths.test.tswith must-not-mask pins, Unicode and NBSP variants, and performance/adversarial cases.Written for commit 22f0c54. Summary will update on new commits.
Summary by CodeRabbit