fix(lab): reject file URI privacy bypasses - #3432
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe event structure validator now rejects case-insensitive ChangesFilesystem path validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Lab event validation now blocks file URI path variants before event retention while continuing to allow tested non-file URI schemes. No actionable current-head merge risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
리뷰 · 우선순위 64 / 80설명 이 PR은 Compatibility Lab이 이벤트를 JSONL로 남기기 전에 돌리는 개인정보 검사에서, 지금까지 빠지던 변경은 두 곳뿐입니다. 라인 단위로 보면 방향은 맞습니다. 다만 라인 src/lab/events/limits.ts FILE_URI_RE - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
![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: f1856eb683
ℹ️ 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 RAW_POSIX_PATH_RE = | ||
| /(?:^|[^A-Za-z0-9._~/])\/(?:(?=$|[^A-Za-z0-9._~/])|(?!\/)(?![ \t\r\n])(?:\/|[^/\0\r\n]+)+\/?(?=$|[^A-Za-z0-9._~/]))/u; | ||
| const FILE_URI_RE = /(?:^|[^A-Za-z0-9+.-])file:\/\//i; |
There was a problem hiding this comment.
When event text contains a Windows-style value such as file:\\server\share\secret or file:\C:\secret\data, Bun's WHATWG URL parser normalizes it to file://server/share/secret or file:///C:/secret/data, but this regex matches only forward slashes. In enforceEventStructureLimits, these forms also evade the existing Windows checks because the drive prefix is not at the start and the path need not contain \Users\, so a raw local or UNC path can still enter retained Lab evidence. Recognize backslashes as file-URL separators as well and add both local-drive and remote-share regression cases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 320f4e6. FILE_URI_RE now accepts one or more forward or backslash separators after file:, covering both UNC-style and drive-qualified forms. Added String.raw regressions for both cases; the focused 8-test file, typecheck, privacy scan, and diff check all pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 320f4e6545
ℹ️ 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 RAW_POSIX_PATH_RE = | ||
| /(?:^|[^A-Za-z0-9._~/])\/(?:(?=$|[^A-Za-z0-9._~/])|(?!\/)(?![ \t\r\n])(?:\/|[^/\0\r\n]+)+\/?(?=$|[^A-Za-z0-9._~/]))/u; | ||
| const FILE_URI_RE = /(?:^|[^A-Za-z0-9+.-])file:[\\/]+/i; |
There was a problem hiding this comment.
When an event contains a WHATWG-valid value such as file:C:\private\secret or file:etc/passwd, FILE_URI_RE does not match because it requires a slash or backslash immediately after the colon; the other path checks also allow these forms, while Bun normalizes them to file:///C:/private/secret and file:///etc/passwd. The fresh gap left after the prior backslash fix is specifically this separator-less file: syntax, so it can still put a local filesystem path into retained Lab evidence. Match the file: scheme without requiring a separator and add separator-less Windows and POSIX regression cases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 77025dd. FILE_URI_RE now matches the standalone file: scheme without requiring a separator. Added separator-less Windows and POSIX regressions while retaining the scheme-boundary negatives; the focused 8-test file (41 expectations), typecheck, privacy scan, and diff check all pass.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77025ddfe1
ℹ️ 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 RAW_POSIX_PATH_RE = | ||
| /(?:^|[^A-Za-z0-9._~/])\/(?:(?=$|[^A-Za-z0-9._~/])|(?!\/)(?![ \t\r\n])(?:\/|[^/\0\r\n]+)+\/?(?=$|[^A-Za-z0-9._~/]))/u; | ||
| const FILE_URI_RE = /(?:^|[^A-Za-z0-9+.-])file:/i; |
There was a problem hiding this comment.
When an event string contains embedded ASCII URL whitespace, such as fi\nle:///etc/passwd, fil\te:///etc/passwd, or file\n:///etc/passwd, this regex does not match and the existing POSIX-path check also misses the repeated slash form. Bun's WHATWG URL parser normalizes each of these to file:///etc/passwd, so the follow-up slash and separator-less fixes still allow a file-URL-encoded local path into retained Lab evidence. Strip tabs, CR, and LF according to URL preprocessing before applying FILE_URI_RE, and add focused regression cases for these forms.
Useful? React with 👍 / 👎.
|
Triaged in the 260904 bug-backlog closeout. The direction is right and the scope is appropriately narrow, but the privacy boundary is still passable. Whitespace inside the scheme normalizes away before the URL is parsed, so To land: strip or normalize URL whitespace before testing the scheme, and add regressions for the tab and newline forms alongside the POSIX, Windows, UNC, and separator-less cases already covered in |
77025dd to
2b0653f
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Thanks — the whitespace bypass is addressed and the PR is now rebased and ready for review. What changed in the new fourth commit
The change is 2 production lines and 7 test lines; no other file is touched. Rebased from Local gates on the rebased head:
One thing worth flagging separately: That is the literal Muse Code key added by #3437, so the scanner flags it on |
lidge-jun
left a comment
lidge-jun
left a comment
There was a problem hiding this comment.
Approving. The whitespace bypass I raised in triage is closed, and closed at the right layer.
Verified on this head: ASCII_URL_WHITESPACE_RE strips tab/CR/LF before the scheme test, so fi\nle:///etc/passwd, fil\te:... and file\r:... are all rejected as raw_path — I ran those three directly against enforceEventStructureLimits rather than trusting the test names. https://example.com/path and profile:///etc/passwd still pass, so the scheme boundary in FILE_URI_RE is not over-matching on a substring.
bun test tests/lab-post-merge-hardening.test.ts is 8 pass / 0 fail, and the negative cases are asserted rather than only the positive ones. Thanks for the quick turnaround on the finding.
2 participants
Summary
file:scheme, including separator-less, forward-slash, backslash, and mixed-separator forms, before Compatibility Lab event data is admittedfi\nle:///etc/passwdandfil\te:///etc/passwdcan no longer slide past admission and become validfile:///URLs downstreamprofile://Motivation
The existing raw path detectors did not recognize every URI-encoded or Windows-style
file:path. Values such asfile:///etc/passwd,file://server/share/secret, andfile:\\server\share\secretcould therefore pass event privacy admission and enter retained lab evidence.Per maintainer review, the same bypass class was still reachable through whitespace inside the scheme: WHATWG URL parsing strips tabs, CRs, and LFs, so
"fi\nle:///etc/passwd"normalized into a validfile:///URL while the raw string missedFILE_URI_RE. The admission test now runs against both the original value and its whitespace-stripped form, which keeps delimiter-prefixed detection intact while closing the split-scheme forms.Validation
All commands were run in an isolated worktree on the rebased head
2b0653f4a3297353ef45ebdb3404259fda838230, whose base is currentdev5364ce01d2156e8784bf2be42944ef983aa27b33.bun test --isolate ./tests/lab-post-merge-hardening.test.ts— 8 pass, 0 fail, 48 expect() calls (includes the 7 new tab/CR/LF regressions)bun run typecheck— passedgit diff --check 5364ce01d2..HEAD— passedgit diff --stat 5364ce01d2..HEAD— 2 files changed, 25 insertions(+), 1 deletion(-), limited tosrc/lab/events/limits.tsandtests/lab-post-merge-hardening.test.ts20C045BF054DACA4DD9E7931A7D590E76DE6EA90F1FFB8E4280D7586E0FE9312), so the rebase carried no content changebun run privacy:scancurrently fails on this branch, but the failure is not from this PR. A clean checkout ofdevat5364ce01d2reproduces the identical failure:That file and line come from #3437, not from either file changed here. On the previous base
20011a1c48the same scan passed against this change set.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit