fix(lab): reject POSIX file URIs - #243
Conversation
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
changed the title
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughRaw path validation now rejects case-insensitive ChangesRaw path privacy validation
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/lab-post-merge-hardening.test.ts`:
- Around line 194-195: Update the file-URI test cases in the focused regression
test to include a mixed-case `file:///` detail value, while retaining the
existing `raw_path` assertion and lowercase cases. Ensure the test exercises the
case-insensitive matching behavior implemented by `FILE_POSIX_URI_RE` in the
limits logic.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3183a92d-f17c-4332-a173-3cc6051e173c
📒 Files selected for processing (2)
src/lab/events/limits.tstests/lab-post-merge-hardening.test.ts
| "detail=file:///etc/passwd", | ||
| "detail=file:///home/alice/secret.txt", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a mixed-case file:/// regression case.
The current values are lowercase, so they verify rejection but not case-insensitive matching. If the i flag is removed from FILE_POSIX_URI_RE, these tests still pass. Add a mixed-case value and retain the existing raw_path assertion.
As per path instructions, the focused regression test should cover the behavior changed in src/lab/events/limits.ts.
Proposed test addition
"detail=file:///etc/passwd",
"detail=file:///home/alice/secret.txt",
+ "detail=FiLe:///etc/passwd",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "detail=file:///etc/passwd", | |
| "detail=file:///home/alice/secret.txt", | |
| "detail=file:///etc/passwd", | |
| "detail=file:///home/alice/secret.txt", | |
| "detail=FiLe:///etc/passwd", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/lab-post-merge-hardening.test.ts` around lines 194 - 195, Update the
file-URI test cases in the focused regression test to include a mixed-case
`file:///` detail value, while retaining the existing `raw_path` assertion and
lowercase cases. Ensure the test exercises the case-insensitive matching
behavior implemented by `FILE_POSIX_URI_RE` in the limits logic.
Source: Path instructions
![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: 07f53ada90
ℹ️ 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_POSIX_URI_RE = /\bfile:\/\/\//i; |
There was a problem hiding this comment.
When an event contains a URI such as file://localhost/home/alice/secret.txt, this expression does not match because it requires the path's third slash immediately after file://, while RAW_POSIX_PATH_RE also skips the slashes following an authority. The URL API normalizes this standard local-file form to file:///home/alice/secret.txt, but enforceEventStructureLimits accepts it, so an unsanitized event can still persist the same sensitive local path through the ledger backstop. Match local file URIs with either an empty or localhost authority and add the authority form to the focused regression cases.
Useful? React with 👍 / 👎.
luvs01
changed the title
|
Closing as superseded: this protection now exists upstream in a strictly broader form. Upstream #3432 merged as const FILE_URI_RE = /(?:^|[^A-Za-z0-9+.-])file:/i;
const ASCII_URL_WHITESPACE_RE = /[\t\r\n]/g;applied to both the original value and its whitespace-stripped form. This PR proposed
No residual value remains, so no upstream PR is warranted for this change. |
1 participant
Motivation
file:///absolute POSIX URIs, allowing sensitive local paths such asfile:///etc/passwdto pass the lab event privacy admission checks.Description
FILE_POSIX_URI_REregex and include it in the string admission checks insideenforceEventStructureLimitssofile:///…forms are rejected asraw_pathalongside other raw path forms. 【F:src/lab/events/limits.ts†L34-L36】【F:src/lab/events/limits.ts†L82-L89】tests/lab-post-merge-hardening.test.tsto assert thatfile:///etc/passwdandfile:///home/alice/secret.txtare rejected by the admission checks. 【F:tests/lab-post-merge-hardening.test.ts†L182-L203】Testing
./node_modules/.bin/bun scripts/test.ts tests/lab-post-merge-hardening.test.ts— the focused test file passed.bun run typecheck— TypeScript typecheck passed.bun run privacy:scan— privacy scan passed.bun run test(full suite) surfaced an unrelated environment timeout in a long-runningkey-logintest during one run; focused tests,typecheck, andprivacy:scanwere green and the targeted regression is covered by the focused test.Codex Task
Summary by CodeRabbit
file:///URIs that reference system or home-directory paths.raw_pathvalidation error.