Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
neild opened this issue Dec 7, 2022 · 23 comments

Comments

@neild
Copy link
Contributor

neild commented Dec 7, 2022

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.
@bcmills
Copy link
Contributor

bcmills commented Dec 12, 2022

@rsc
Copy link
Contributor

rsc commented Feb 1, 2023

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

@rsc
Copy link
Contributor

rsc commented Feb 1, 2023

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

@rsc rsc moved this from Incoming to Active in Proposals Feb 1, 2023
@neild
Copy link
Contributor Author

neild commented Feb 1, 2023

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.

@martin-sucha
Copy link
Contributor

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.

@rsc
Copy link
Contributor

rsc commented Feb 22, 2023

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.

@rsc
Copy link
Contributor

rsc commented Mar 8, 2023

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.

@ianlancetaylor
Copy link
Contributor

filepath.Localize ?

@neild
Copy link
Contributor Author

neild commented Mar 29, 2023

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.

@rsc
Copy link
Contributor

rsc commented Apr 5, 2023

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?

@simar7
Copy link

simar7 commented Apr 6, 2023

+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.

@martin-sucha
Copy link
Contributor

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.

@neild
Copy link
Contributor Author

neild commented Apr 6, 2023

Localize and IsLocal are unrelated, which is indeed unfortunate.

@rsc
Copy link
Contributor

rsc commented Apr 12, 2023

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?

@rsc
Copy link
Contributor

rsc commented May 10, 2023

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?

@neild
Copy link
Contributor Author

neild commented May 10, 2023

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.

@rsc
Copy link
Contributor

rsc commented May 17, 2023

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).

@neild
Copy link
Contributor Author

neild commented May 17, 2023

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.

@rsc
Copy link
Contributor

rsc commented May 17, 2023

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

@rsc rsc changed the title proposal: path/filepath: add FromFS to safely convert a slash-separated path into an operating system path proposal: path/filepath: add Localize to safely convert a slash-separated path into an operating system path May 24, 2023
@rsc
Copy link
Contributor

rsc commented May 24, 2023

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

@rsc rsc moved this from Active to Likely Accept in Proposals May 24, 2023
@rsc rsc moved this from Likely Accept to Accepted in Proposals May 31, 2023
@rsc
Copy link
Contributor

rsc commented May 31, 2023

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

@rsc rsc changed the title proposal: path/filepath: add Localize to safely convert a slash-separated path into an operating system path path/filepath: add Localize to safely convert a slash-separated path into an operating system path May 31, 2023
@rsc rsc modified the milestones: Proposal, Backlog May 31, 2023
@neild neild self-assigned this Jun 27, 2023
qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Aug 17, 2023
Fixes golang#57151.

Change-Id: I98c6a211d77fa5c8733306b5ef39950bae07fb5c
@gopherbot
Copy link
Contributor

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

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Aug 17, 2023
Implemented filepath.Localize using internal/safefilepath.FromFS.

Fixes golang#57151

Change-Id: I98c6a211d77fa5c8733306b5ef39950bae07fb5c
@gopherbot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.

8 participants