fix(validate-refs): flag single-backslash Windows path leaks#2488
fix(validate-refs): flag single-backslash Windows path leaks#2488zied-jlassi wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe PR fixes a regex bug in ChangesAbsolute-path-leak regex fix and test coverage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 `@tools/validate-file-refs.js`:
- Line 72: The ABS_PATH_LEAK regex pattern only matches Windows drive letters in
uppercase (A-Z) with the pattern [A-Z]:[\/], which fails to detect absolute
paths with lowercase drive letters like c:\Users\me\notes.md. Update the regex
pattern in the ABS_PATH_LEAK constant to include both uppercase and lowercase
letters by changing [A-Z] to [A-Za-z] so that Windows paths with lowercase drive
letters are properly detected.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 9e0fdfee-7a9e-4267-94b1-8f2c79e21e41
📒 Files selected for processing (3)
package.jsontest/test-abs-path-leak.jstools/validate-file-refs.js
| const ABS_PATH_LEAK = /(?:\/Users\/|\/home\/|[A-Z]:\\\\)/; | ||
| // Windows drive paths use a single separator (C:\Users or C:/Users). In a regex | ||
| // literal `\\` already matches one backslash, so the class matches either separator. | ||
| const ABS_PATH_LEAK = /(?:\/Users\/|\/home\/|[A-Z]:[\\/])/; |
There was a problem hiding this comment.
Detect lowercase Windows drive letters too.
[A-Z]:[\\/] still misses valid paths like c:\Users\me\notes.md, so this validator can still leak false negatives.
Proposed patch
-const ABS_PATH_LEAK = /(?:\/Users\/|\/home\/|[A-Z]:[\\/])/;
+const ABS_PATH_LEAK = /(?:\/Users\/|\/home\/|[A-Za-z]:[\\/])/;📝 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.
| const ABS_PATH_LEAK = /(?:\/Users\/|\/home\/|[A-Z]:[\\/])/; | |
| const ABS_PATH_LEAK = /(?:\/Users\/|\/home\/|[A-Za-z]:[\\/])/; |
🤖 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 `@tools/validate-file-refs.js` at line 72, The ABS_PATH_LEAK regex pattern only
matches Windows drive letters in uppercase (A-Z) with the pattern [A-Z]:[\/],
which fails to detect absolute paths with lowercase drive letters like
c:\Users\me\notes.md. Update the regex pattern in the ABS_PATH_LEAK constant to
include both uppercase and lowercase letters by changing [A-Z] to [A-Za-z] so
that Windows paths with lowercase drive letters are properly detected.
|
Good catch on the lowercase drive letters — fixed in 3df821d. I went with |
The absolute-path-leak check used `[A-Z]:\\\\`, which in a regex literal needs two backslashes — so a normal Windows path like C:\Users\... slipped straight through. Widened it to match either separator (C:\ or C:/). While here, exported checkAbsolutePathLeaks and added a small test (test/test-abs-path-leak.js, wired into npm test) covering the single-backslash case plus the existing Unix and code-block behaviour.
Added JSDoc to the test/assert/leakCount helpers so the new test file documents its own helpers. No behaviour change — 6/6 still pass.
Following review feedback, widen the Windows branch to [A-Za-z] so lowercase paths (c:\Users\...) are caught too. Kept a \b anchor so URL schemes like https:// (which also contain "<letter>:/") aren't flagged — a plain [A-Za-z] would have matched every URL in the docs. Added lowercase and URL-not-flagged cases to the test (now 8/8).
zied-jlassi
force-pushed
the
fix/abs-path-leak-single-backslash
branch
from
3df821d to
1c1c6e9
Compare
|
Rebased onto the latest
Full local run is green: |
1 participant
Problem
The absolute-path-leak check misses normal Windows paths. A pasted
C:\Users\me\notes.mdisn't flagged, while
/Users/...and/home/...are.Cause
ABS_PATH_LEAKuses[A-Z]:\\\\. In a regex literal that's two backslashes, so it onlymatches the escaped
C:\\Usersform — single-backslash Windows paths slip through.Fix
Widen the Windows branch to
[A-Z]:[\\/](drive letter + either separator). Also exportedcheckAbsolutePathLeaksso it can be unit-tested, and addedtest/test-abs-path-leak.js(wired into
npm test) covering the single-backslash case plus the existing Unix /forward-slash paths and code-block stripping.
Fixes #2487
Testing
node test/test-abs-path-leak.js-> 6/6 passednode tools/validate-file-refs.js --strict->Absolute path leaks: 0, exit 0 (the widerpattern adds no new false positives on the repo's own content)