The filepath.FromSlash function sounds like it converts a /-separated path into an operating system path. What it actually does is replace every / in its input with os.PathSeparator.

FromSlash can map an input path into semantically different paths on different operating systems. For example, FromSlash("a\b") returns the filename a\b on Unix and the file b in the directory a on Windows. FromSlash("C:/foo") returns a relative path on Unix and an absolute path on Windows.

56694 involves failures in the standard library to safely convert a non-platform-specific /-separated path into a semantically equivalent operating system path. The fix (https://go.dev/cl/455716) introduces an internal function to perform this operation. We should have a public API for this.

The proposal:

// FromFS converts a slash-separated path into an operating system path.
//
// FromFS returns an error if the path cannot be represented by the operating system.
// For example, paths containing '\' and ':' characters are rejected on Windows.
func FromFS(path string) (string, error)

FromFS rejects empty paths ("") and, on Windows, reserved device names such as NUL.

FromFS and IsLocal (#56219) are similar in that both involve performing safety checks on a potentially-untrusted input path. They serve different roles, however:

FromFS takes a /-separated path in the form operated on by the path package and safely converts it to a semantically-equivalent operating system path.
IsLocal takes an operating system path and reports whether it refers to something surprising.

Comment From: bcmills

A concrete example of where this could be useful: https://cs.opensource.google/go/x/build/+/master:cmd/buildlet/buildlet.go;l=1633-1670;drc=e73b639045d6176f6de4efb526c3d55475c065c5

Comment From: rsc

I'm starting to get confused about which of these new filepath functions should be used when.

Comment From: rsc

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

Comment From: neild

I'm starting to get confused about which of these new filepath functions should be used when.

I am very sympathetic to that confusion.

Counting IsLocal added in 1.20, we have three new functions:

filepath.IsLocal checks to see if a OS path is hinky. A hinky path is one that refers to something outside the current directory, or something weird like COM1 on Windows. I've run across a number of sanitization functions along the lines of strings.TrimPrefix(filepath.Clean("/"+p), "/") to ensure that a path is neither absolute nor contains .. components; IsLocal is intended to be a more robust and portable version of accomplishing the same goal. It's a somewhat specialized function that most users will not need, but there are so many subtleties (particularly surrounding Windows device names) that I'm still convinced it was worthy of inclusion into the standard library.
filepath.FromFS converts a non-OS-specific /-separated path as one might find in a URL and converts it into an OS-specific path. Unlike FromSlash, it reports an error if the path can't be represented locally. Everyone who uses FromSlash today should use FromFS instead.
filepath.IsReserved isn't particularly useful on its own, in my opinion, but both IsLocal and FromFS need to be aware of Windows reserved device names. A function which answers the limited question of whether a name is reserved seems like a useful building block to expose, especially since getting this right is quite tricky.

You should use FromFS when converting a /-separated path into an OS path.

You should use IsLocal to verify that a path from an untrusted source doesn't refer to anything surprising.

You should use IsReserved when building a sanitization function along these lines, if the above aren't sufficient.

Comment From: martin-sucha

It is unclear to me why the function is called FromFS.

The proposal says that it supports paths that path package supports. The path package supports absolute paths. So it seems that filepath.FromFS would support absolute paths. However, I'm just guessing here since the proposal does not specify the desired behavior.

Then there is fs.ValidPath that rejects absolute paths. Based on the name, I would expect that FromFS returns an error for any input where fs.ValidPath returns false.

If FromFS is supposed to support absolute paths and since FromFS is intended to replace FromSlash, perhaps it should just be called FromSlashV2? If FromFS is not supposed to support absolute paths, we should document that explicitly.

Comment From: rsc

I don't think it's a given that everyone who uses FromSlash today should use FromFS.

Today, FromSlash converts slashes to the canonical form for the host OS but preserves the meaning of the existing path. So for example on Windows today, FromSlash("c:/foo") gives you c:\foo, which is the canonical form of its input. Similarly on a Mac you get FromSlash("c:/foo") is c:/foo, and in both cases the result of os.Open(p) and os.Open(FromSlash(p)) are the same.

It sounds like FromFS would not do that. You should use FromFS when the input is meant to be a "portable" slash-separated path as opposed to a slash-separated path interpreted according to the local OS. Programs that accept a file name on the command line but want to convert to native conventions should probably keep using FromSlash. The compiler does this sometimes for arranging canonical outputs and then inverting them. It should keep using FromSlash and ToSlash.

Maybe something reading from a zip file should use FromFS, but why not just have it use IsLocal+FromSlash instead of FromFS?

Or is it just programs implementing an fs.FS that need to use FromFS?

I don't think the exact scope is clearly defined yet.

Comment From: rsc

It seems like we are stuck on the name here. I noticed that internal/safepath.FromFS is called with paths beginning with / (like /foo) but those are not actually io/fs paths. I'm not sure if the proposed FromFS rejects those or not, but safepath.FromFS does not.

Also it's probably too indirect a meaning to use "FS" here.

There are fundamentally two kinds of FromSlash: ones that are canonicalizing the OS interpretation and ones that are converting from "portable" to "local OS". The current FromSlash does the former. We need a name for the latter.

Maybe the From prefix is tripping us up and we should name this operation with some verb that can be the function name.

Comment From: ianlancetaylor

filepath.Localize ?

Comment From: neild

I can't come up with a verb, because filepath.Verb implies an operation on host OS paths. This is an operation on a non-host path.

filepath.SafeFromPath?

filepath.FromSlashPath?

FromSlash seems like the right name for this operation; the problem is that the existing function doesn't have an error return.

Probably impractical: Keep filepath.FromSlash, but change it to return "" if replacing slashes in the input path changes the path's meaning.

Comment From: rsc

Localize means "restrict (something) to a particular place" and can also be interpreted as converting to local syntax, which it also does. That seems like a good name. Any better names?

Comment From: simar7

+1 for this proposal. I always fear using the path/filepath packages because I never truly understand what to expect.

As for Localize, it makes perfect sense that it should return a value based on the OS.

Comment From: martin-sucha

How does Localize relate to IsLocal? I am worried that it might be confusing to mix those two. If I understand correctly, FromFS/Localize allows absolute paths, so the following would be possible:

s, err := Localize("/a/b/c")  // s = "/a/b/c", err = nil
IsLocal(s) // false

In other words, the result of Localize could be non-local, because local paths cannot be absolute.

Since @rsc mentioned

ones that are converting from "portable" to "local OS""

maybe we should define what a portable path is and use something like ParsePortable? FromPortable?

It seems having a definition of all the supported path variants in the package docs would be helpful in any case.

Comment From: neild

Localize and IsLocal are unrelated, which is indeed unfortunate.

Comment From: rsc

Given that io/fs does not accept /a/b/c as a path and we started at FromFS meaning "from io/fs paths", my understanding was that Localize would reject /a/b/c and ../a/b/c, at which point Localize would guarantee to return a path satisfying IsLocal.

If we do that, is Localize no longer useful enough?

Comment From: rsc

Suppose we have func Localize(string) (string, error), which converts the input to be a local file system path, satisfying IsLocal, or else returns an error. Localize("/a/b/c") and Localize("../a/b/c") are both errors, as are things like Localize("com1") on Windows.

Does that work for the use cases we want it for? Is that good enough?

Comment From: neild

Suppose we have func Localize(string) (string, error), which converts the input to be a local file system path, satisfying IsLocal, or else returns an error.

This is blending two different concerns: converting a slash-separated path into an operating system path, and enforcing constraints on the path (e.g., not absolute). It feels to me like we're arriving at a behavior because of the ambiguous meaning of "localize", not because we want that particular behavior.

But along those lines, how about the original name FromFS, defined to operate only on paths which fs.ValidPath considers valid? No ".." components, no leading or trailing "/". If you want, you can use path.Clean(path) to resolve relative path components before conversion.

Comment From: rsc

I wrote:

Given that io/fs does not accept /a/b/c as a path and we started at FromFS meaning "from io/fs paths", my understanding was that Localize would reject /a/b/c and ../a/b/c, at which point Localize would guarantee to return a path satisfying IsLocal.

Then Damien wrote:

But along those lines, how about the original name FromFS, defined to operate only on paths which fs.ValidPath considers valid? No ".." components, no leading or trailing "/". If you want, you can use path.Clean(path) to resolve relative path components before conversion.

Are these two descriptions of the same behavior with different names, or are the behaviors different? It appears they are the same.

But if the behaviors are the same, then Localize seems like a clearer name than FromFS, because lots of people using filepath are unfamiliar with io/fs and won't think of FS as indicating io/fs. They might think it means something about the local operating system's file system. filepath.Localize in contrast sounds very much like a conversion to local conventions (and it is), in addition to being a containment of what the path can refer to (and it is).

Comment From: neild

Are these two descriptions of the same behavior with different names, or are the behaviors different? It appears they are the same.

What should Localize("a/../b") return?

If it's an error, because the input isn't a valid path according to "io/fs".IsValid, then these are same behavior with different names.

I think this behavior works for the cases we care about. I'm okay with either the name Localize or FromFS for the function.

Comment From: rsc

Yes, I was assuming any mention of .. in Localize's argument is an error.

Comment From: rsc

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

Comment From: rsc

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

Comment From: gopherbot

Change https://go.dev/cl/520268 mentions this issue: path/filepath: add Localize

Comment From: gopherbot

Change https://go.dev/cl/531677 mentions this issue: path/filepath: add Localize

Golang path/filepath: add Localize to safely convert a slash-separated path into an operating system path

56694 involves failures in the standard library to safely convert a non-platform-specific /-separated path into a semantically equivalent operating system path. The fix (https://go.dev/cl/455716) introduces an internal function to perform this operation. We should have a public API for this.