Better handle cross-platform URIs#161
Conversation
Better handle cross-platform URIs
WalkthroughUpdated the regex for parsing file: URIs in src/Address.ts to handle Windows drive-letter and relative paths. No other logic, control flow, or exports were changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Address.ts (1)
59-61: Update regex and add tests for edge-case file URIs
- Plain
file:C:/…currently isn’t matchedfile://andfile:///capture an empty string and resolve to CWD instead of root (/)- UNC URLs (
file://host/share/…) are treated as relative paths—decide whether to support or reject and adjust the pattern/tests accordingly
| this.isLocal = true; | ||
| } else if (addressURI.substring(0, 5) == "file:") { | ||
| const match = addressURI.match(/^file:(?:\/\/)?(\.?\.?\/?.*)/); | ||
| const match = addressURI.match(/^file:(?:\/\/\/?)([A-Za-z]:\/.*|\.?\.?\/?.*)/); |
There was a problem hiding this comment.
Overly permissive regex pattern risks matching malformed URIs.
The second alternative \.?\.?\/?.* in the capture group matches almost anything (including empty strings), which undermines validation. For example:
file://xyzcapturesxyzas a relative pathfile://captures an empty stringfile://C:/pathcapturesC:/pathcorrectly, but so wouldfile://nonsense
This makes it difficult to distinguish valid file URIs from malformed ones.
Consider a more explicit pattern that separately handles each case:
- const match = addressURI.match(/^file:(?:\/\/\/?)([A-Za-z]:\/.*|\.?\.?\/?.*)/)
+ const match = addressURI.match(/^file:(?:\/\/\/?([A-Za-z]:[\/\\].*|\/.*)|\/\/?(\.\.?(?:[\/\\].*)?)|\/?(\.\.[\/\\].*))/)Or, for clarity, handle Windows and Unix paths separately:
// Match Windows: file:///C:/... or file://C:/... or file:C:/...
const winMatch = addressURI.match(/^file:(?:\/\/\/?)?([A-Za-z]:[\/\\].*)/);
// Match Unix absolute: file:///... or file://localhost/...
const unixMatch = addressURI.match(/^file:\/\/(?:localhost)?(\/.*)$/);
// Match relative: file:./... or file:../... or file://./... or file://../...
const relMatch = addressURI.match(/^file:(?:\/\/)?(\.\.[\/\\].*|\.[\/\\].*)/);
const match = winMatch || unixMatch || relMatch;This approach makes the intent clearer and reduces false positives.
🤖 Prompt for AI Agents
In src/Address.ts around line 49, the current single regex is overly permissive
and can capture empty or malformed paths; replace it with explicit, separate
checks for Windows absolute paths, Unix absolute/localhost paths, and relative
paths (e.g. three regexes for win, unix, rel), ensure each pattern requires a
non-empty path component (no empty-string captures), try them in a clear order
(windows → unix → relative) and use the first successful match as the canonical
capture; this makes intent explicit and prevents false positives like
"file://xyz" or "file://".




To handle both Linux and Windows URIs more reliably, consider this version:
/^file:(?:\/\/\/?)([A-Za-z]:\/.*|\.?\.?\/?.*)/This:
Small notice: this Regex is generated with AI as I can't get them in my head 😭
Summary by CodeRabbit