Change https://go.dev/cl/444280 mentions this issue: path/filepath: handle all paths starting with \\ as UNC on Windows

Another mishandling: one can skip UNC path normalization by prefixing a UNC path (host\share) with \\?\UNC\, so filepath.VolumeName("\\?\UNC\a\b\c") should return \\?\UNC\a\b, not \\?\UNC.

You can test this in the command line:

echo 1234>\\?\UNC\localhost\c$\temp\foo.txt
type \\?\UNC\localhost\c$\temp\foo.txt

The same applies to the \\.\UNC\ prefix.

This seems like a breaking change to me. At Caddy, we're trying to build on Go 1.20, but our tests are failing on Windows now. caddyserver/caddy#5353

We run this code:

filepath.Clean("/"+input)

Before, on Go 1.19.5:

In: /../bar  |  Out: \bar
In: /../../  |  Out: \

After, on Go 1.20.0:

In: /../bar  |  Out: \\..\bar
In: /../../  |  Out: \\..\..\

This is a problem for us, because this would allow escaping the root.

The reason we add "/" before cleaning is that the input is an HTTP request path, and we need to guarantee that the input didn't have the leading slash dropped by some code that came before it. We relied on Clean() to get rid of the duplicate leading slash if present. This doesn't seem to work anymore on Windows, with these changes.

I figure we might be able to fix it by cleaning once, then prepending the slash and cleaning again. But that seems like a bad idea.

Using Clean alone is not sufficient to avoid escaping the root though. For example, Windows has reserved file names. See filepath.IsLocal

IsLocal is not a useful check for this usecase because a URL path will always be absolute (start with a /) so that function will always return false. And also it's only available in 1.20 but we need to retain support for 1.18 at minimum for now (yes we could copy the source from stdlib but that's not a solution).

The point is, this was a breaking change. It wasn't properly documented in the release notes as such, and is surprising behaviour.

Clean returns the shortest path name equivalent to path by purely lexical processing was not true for UNC paths before and now it is.

The point is, this was a breaking change. It wasn't properly documented in the release notes as such, and is surprising behaviour.

Yes, I agree with that. The documentation of filepath.Clean does not mention UNC paths at all and says that it will Replace multiple Separator elements with a single one. and Eliminate .. elements that begin a rooted path, which clearly does not happen for a UNC path at the start of the string.

Can this issue be reopened?

@francislavoie It sounds like you are using path/filepath with a URL, which sounds odd to me. URLs are normally handled by path or by net/url. path/filepath has always been intended to work with paths in the file system. Since paths starting with \\ have a special meaning on Windows, if you use path/filepath with such a path it shouldn't change.

The only room for adjustment I see here is that your path starts with // rather than \\. But my understanding is that on Windows paths starting with // are just like paths starting with \\.

So you're correct that the behavior changed, but it seems to me that it became more correct. I'm sorry for the breakage, but from a compatibility point of view this we've always reserved the right to change behavior in order to fix cases where the code does not match its documentation.

I agree that the release notes needs to be updated (ping @neild) but I'm not yet seeing an argument for rolling back this change. Such an argument would show us why the new behavior contradicts the documented behavior of the filepath functions.

We're doing this to take a user input HTTP URL path (which happens to always be using / separators), and map it to a filesystem path (i.e. a file server). I'm pretty sure our usage is correct for this usecase. We're working at the intersection of those two path systems.

We're doing this to take a user input HTTP URL path (which happens to always be using / separators), and map it to a filesystem path (i.e. a file server). I'm pretty sure our usage is correct for this usecase.

That turns out to be surprisingly subtle — see #57151.

I agree, it's a very subtle problem. See our function to try to do a safe join of a configured "webroot" with an input URL, mapping to a filesystem path:

https://github.com/caddyserver/caddy/blob/94b8d56096b2581d6739b057655e7b895c8fd3bb/modules/caddyhttp/caddyhttp.go#L223-L246

We'd gladly take a properly supported stdlib function to do this same operation in a smarter way.

Also before, on Go 1.19.5:

In: /a/b/c        | Out: \\a\b\c
In: /a/b/../../c  | Out: \\a\b\c

What's going on here is quite subtle.

On Windows, filepath.Clean preserves the volume name when present. Volume names are given special handling so, for example, C:\..\a is cleaned to C:\a rather than \a.

Previous versions of Go recognize paths such as \\server\share\a as having a volume name of \\server\share. However, they did not recognize the volume name in paths such as \\.\C:\a (a valid path with a volume name of \\.\C:) or \\?\UNC\server\share\a.

Part of the pre-Go 1.20 behavior was to detect no volume name when the third character of the path is ".", and to truncate the volume name to just before the first "." following the first four characters of the path. So in Go 1.19.5:

\\.\C:\a  no volume name
\\a.b\c\  volume name is \\a.b\c
\\a\b.c\  volume name is \\a\b

This parsing behavior doesn't really correspond to the actual Windows path semantics, but it does have the property which is that a volume name, considered as a slash-separated string, will never contain a ".." component.

Caddy contains this function:

func SanitizedPathJoin(root, reqPath string) string {
        if root == "" {
                root = "."
        }
        path := filepath.Join(root, filepath.Clean("/"+reqPath))
        if strings.HasSuffix(reqPath, "/") && len(reqPath) > 1 {
                path += separator
        }
        return path
}

I do not believe filepath.Clean("/"+reqPath) is correct. reqPath in this case is a /-separated path from a URL. The filepath.Clean function operates on operating system paths, and therefore attempts to handle volume names within reqPath as well as interpreting both \ and / as separator characters.

However, while incorrect, prior to Go 1.20 this function is (I think) safe from directory traversal attacks thanks to the fact that we never recognize .. as part of a volume name. It's not safe from surprising behaviors, though:

SanitizedPathJoin("root", "/a/b/../../c") = `root\a\b\c` (not `root\c` as one might expect)

I think this function should be changed to use path.Clean instead:

path := filepath.Join(root, path.Clean("/"+reqPath))

Note that we're still going from a /-separated path to an OS path here. We're still open to surprising behaviors when reqPath names a Windows device file, however. (Note that a/b/COM1 is still the COM1 device.) So better yet would be to use the new IsLocal function to further vet the path:

relPath := path.Clean("/"+reqPath)[1:] // clean path and trim the leading /
if !filepath.IsLocal(relPath) {
  return "", errors.New("path is unsafe")
}
path = filepath.Join(root, relPath)

If #57151 is accepted, we'll have a better approach to converting /-separated paths to OS paths:

reqOSPath, err := filepath.FromFS(reqPath)
if err != nil {
  return "", errors.New("path is unsafe")
}
// reqOSPath will not contain a volume name component or device name.
path := filepath.Join(root, filepath.Clean("/" + reqOSPath))

All that notwithstanding, I think we can do a better job of defending against mistakes by changing Windows volume name detection to explicitly reject any volume name where a path component is "..". So far as I know, no valid Windows volume names contain ".." (unlike "."), and this will ensure that filepath.Clean(s) won't unexpectedly return a path containing .. components.

I think this function should be changed to use path.Clean instead:

path := filepath.Join(root, path.Clean("/"+reqPath))

Ah yeah, that definitely makes more sense. 🤦‍♂️ Thanks for pointing that out.

So better yet would be to use the new IsLocal function to further vet the path:

If #57151 is accepted, we'll have a better approach to converting /-separated paths to OS paths:

Makes sense. We won't be able to change to minimum Go 1.20 for another 6 months to a year for packaging reasons, so that's not a solution we can use right now. Looking forwards to it, though.

Created a new issue #58451 to reject volume names containing a ".." path component, closing this issue in favor of that one.