fix: reject Windows file paths in can_parse#957
Conversation
| if (helpers::is_windows_file_path(input)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Can you point us to where in the URL spec this is returning false? Chrome and Safari returns true...
> URL.canParse("file:///C:/path/file.node")
true
> URL.canParse("C:\\path\\file.node")
true
There was a problem hiding this comment.
This is actually allowed. https://url.spec.whatwg.org/#file-host-state
If state override is not given and buffer is a Windows drive letter, file-invalid-Windows-drive-letter-host validation error, set state to path state.
There was a problem hiding this comment.
If you apply the same change to URL parser, not the can_parse() method, you'll see that it breaks web-platform tests.
There was a problem hiding this comment.
thank you, instead of rejecting Windows file paths
Is it a healthy method to normalize them with the file:/// protocol ?
There was a problem hiding this comment.
What the library provides is WHATWG URL support. If the result disagrees with your expectations make sure first to check the WHATWG URL specification. If you disagree with it, you should take your concerns with WHATWG.
We may have bugs but a bug is a disagreement with WHATWG.
|
At a glance this PR appears to break the specification. |
@lemire Thank you for the clear guidance. I understand now that we should follow the WHATWG URL spec strictly. I've opened an issue in WHATWG to discuss the Windows file path handling: whatwg/url#873 I'll keep the library's behavior aligned with the spec while we discuss this in WHATWG. |
|
I am closing this PR since @mertcanaltin reported the issue to WHATWG. It is understood that if the standard changes, we will reopen such a PR. |
3 participants
reject Windows file paths in can_parse
I tried to solve this place out : nodejs/node#58578 (comment)