Deno: regression: path containing `:` in first segment breaks filesystem APIs

[lucacasonato]

Deno 1.0.5

$ deno eval "Deno.lstatSync('abc:def')"
error: Uncaught NotFound: No such file or directory (os error 2)
    at unwrapResponse ($deno$/ops/dispatch_json.ts:43:11)
    at Object.sendSync ($deno$/ops/dispatch_json.ts:72:10)
    at Object.lstatSync ($deno$/ops/fs/stat.ts:77:15)
    at file:///mnt/f9/tests/__$deno$eval.ts:1:6

Deno 1.1.0

$ deno eval "Deno.lstatSync('abc:def')"
error: Uncaught TypeError: Must be a path string or file URL.
    at Object.pathFromURL ($deno$/util.ts:120:13)
    at Object.lstatSync ($deno$/ops/fs/stat.ts:79:10)
    at file:///mnt/f9/tests/__$deno$eval.ts:1:6

This most likely introduced by #5990

[ry]

cc @actual-size

[nayeemrmn]

Yep, pretty much any path with a colon in the first path segment is a valid URL, and will be identified as such. My original thought was "It's fine as long as there is a way to disambiguate, i.e. rewriting as "./abc:def"", but this is an unreasonable requirement in an automated system with dynamic paths...

I think @piscisaureus's comment was missed: https://github.com/denoland/deno/pull/5990#issuecomment-642356996.

What do you think we should do? We could alleviate the problem quite a bit by reinterpreting the URL as a path again when it's seen not to have the file protocol. That wouldn't fix file:abc.

We could insist on having the double slash after the protocol: file://, it's still a valid relative path but not a normalised one that is likely to be given from a dynamic source.

[lucacasonato]

What I would expect is that if I pass a path as a string, that it does not get interpreted as a URL. It should just be interpreted the same way my shell would interpret it. So Deno.lstatSync("file://abc:def") shouldn't work imo.

[caspervonb]

Uhm what? filepaths as strings given to file related syscalls can be interpreted as URLs? 🤔 That's a fairly major breaking change to all the "syscalls" this applies to 😭

[caspervonb]

So this was merged and put into a release really quickly, without even going through an unstable phase or internal testing which is unfortunate.

Best case scenario the whole commit would be reverted and overloads that do strange unexpected things would live elsewhere, like instd/fs/mod.ts.

But; since all the contributors seemed to really want to get this in I doubt I'd win that one so I've opted for the middle ground of just taking out the implicit hijacking of legal file-paths which will at-least not break valid code.

[actual-size]

100% behind @piscisaureus (and everyone else) that attempting to parse strings as a URLs should be removed, there are so many little edge cases that we won't be able to handle properly. I did want to remove it after he brought it up, but I didn't communicate that properly so @ry didn't know.

[ry]

This is my mistake - I should have been more careful when landing.

[caspervonb]

Just a strawpoll; how does everyone feel about moving these overloads from core into std/fs? I'd really like to push towards reverting this and supporting it elsewhere. It was a good thing that the truncate patch overload on rid did not land; overloads should not be in the core ops IMHO.

Also I really don't see the use case, using the path module is like 8 more characters.

Deno.open(new URL("file://"));
Deno.open(Path.fromFileUrl("file://"));

I'm confused, what problem does this overload solve? :confused:

[MarkTiedemann]

On a Windows NT file system, abc:def would be read as "the alternate data stream def of the file abc".

ADS' used to work:

C:\>deno upgrade --version 1.0.0
C:\>deno eval "Deno.writeTextFileSync('abc:def', 'ghi'); console.log(Deno.readTextFileSync('abc:def'))"
ghi

But are now broken:

C:\>deno upgrade --version 1.1.0
C:\>deno eval "Deno.writeTextFileSync('abc:def', 'ghi'); console.log(Deno.readTextFileSync('abc:def'))"
error: Uncaught TypeError: Must be a path string or file URL.
    at Object.pathFromURL ($deno$/util.ts:118:13)
    at Object.openSync ($deno$/ops/fs/open.ts:21:10)
    at Object.openSync ($deno$/files.ts:28:15)
    at Object.writeTextFileSync ($deno$/write_text_file.ts:5:16)
    at file:///C:/dev/__$deno$eval.ts:1:6