Bug: hackpadfs/os fails to root file name

[infogulch]

Given the conditions:

1. Current working directory is not /. (Say /tmp for example.)
2. Get fs with: fsys, _ := hackpadfs_os.NewFS().Sub("mydir") (Assume /tmp/mydir/ exists.)
3. Call fsys.Stat(".")

Stat fails with:

stat : no such file or directory

While debugging this, it is apparent that this is caused by fs.rootedPath assuming that the sub path is an absolute path, not a path relative to the cwd. I.e. given the example situation, it runs os.Stat("/mydir"), not os.Stat("/tmp/mydir") which is what I would expect.

The following go playground demonstrates: https://go.dev/play/p/hYuq0VNVybc

It seems Sub is not safe to use with paths relative to the cwd, and given that you can't use rooted paths (gofs.ValidPath disallows them), it is not usable in general.

[JohnStarich]

Hey @infogulch, thanks for the issue.

*os.FS intentionally removes the behavior of the current working directory to satisfy the goal that file paths passed to an io/fs.FS interface always provide the same file, even when os.Chdir() is called.

It’s documented on the constructor here: https://pkg.go.dev/github.com/hack-pad/hackpadfs@v0.2.1/os#NewFS

NewFS returns a new FS. All file paths are relative to the root path. Root is '/' on Unix and 'C:\' on Windows. Use fs.Sub() to select a different root path. SubVolume on Windows can set the volume name.

[JohnStarich]

It seems Sub is not safe to use with paths relative to the cwd, and given that you can't use rooted paths (gofs.ValidPath disallows them), it is not usable in general.

At least right now, that’s correct. The way I interpret the fs.SubFS interface docs in Go’s standard library, it should append valid paths to the current FS root. Since the root of NewFS() is the real filesystem’s root, then that should result in a sub path after / on Unix systems.

I’m open to adding a constructor which uses the current working directory as a root though.

[infogulch]

*os.FS intentionally removes the behavior of the current working directory to satisfy the goal that file paths passed to an io/fs.FS interface always provide the same file, even when os.Chdir() is called.

A laudable goal, but this method of achieving it seems to throw the baby out with the bathwater.

I’m open to adding a constructor which uses the current working directory as a root though.

I'm not sure this is what I'd want. I'm getting the path from user configuration and using it to construct an FS. I think my cli app users would find it natural if they could provide a relative path like ./mydir or ../mydir or an absolute path like /tmp/mydir. Honestly I like the behavior that once an FS is created it always refers to the same absolute path, a good way to do that would be to root relative paths at construction time.

[JohnStarich]

I agree, that's a sensible use case. The tricky part is sticking to the principles laid out by the standard library's interfaces.

One option is to use *os.FS's FromOSPath() method to translate from an input file path into a Go FS compatible path. To handle any kind of file path input, it might look like this:

import "github.com/hack-pad/hackpadfs/os"

fs := os.NewFS()

osPathFlag := myCLIFramework.String("path")
absoluteOSPath, err := filepath.Abs(osPathFlag)
if err != nil {
    return err
}
fsPath, err := fs.FromOSPath(absoluteOSPath)
if err != nil {
    return err
}
file, err := fs.Open(fsPath)
// ...

I'm open to the idea of making this easier to convert the file path. Maybe another method like fs.FromOSPath() which permits relative file paths and handles filepath.Abs() internally.

It's worth noting the standard library's os.DirFS(prefix) also only accepts fs.ValidPath() compatible paths.

Interestingly, os.DirFS() does mention behavior changes due to calls on os.Chdir() but it also requires a non empty prefix. This implies some surprising behavior when changing the current process's directory during execution. A CLI app may be safer since it's unusual to change directory at runtime, so maybe I can still be convinced this needs a solution anyway. :thinking:

[JohnStarich]

I could see something like this being helpful for a CLI, perhaps one like yours. What do you think?

import "github.com/hack-pad/hackpadfs/os"

fs := os.NewWorkingDirectoryFS()

osPathFlag := myCLIFramework.String("path")
fsPath, err := fs.FromRelativeOSPath(osPathFlag)
if err != nil {
    return err
}
file, err := fs.Open(fsPath)

I'm not totally sure how much is feasible, but it might be worth trying anyway. One unknown is how to treat relative paths that point outside the current root (after calling Sub).

[infogulch]

To reveal a bit more of my use case, I'm writing a web server. I'm expecting the server admin to select some directory where they want to access and potentially upload files to at startup, and then it runs for a while with the FS somewhat sandboxed into that directory. I accept configuration options to select that directory from the cli, config files, startup config options, etc.

For me the ideal way for this to work would be for the FS constructor itself to be the boundary between "cli app that should be able to refer to whatever directory the admin wants" and "long running process that receives untrusted requests and should be sandboxed to prevent shenanigans". Maybe something like this:

import "github.com/hack-pad/hackpadfs/os"

// at startup:
osPathFlag := myCLIFramework.String("path")
fs, err := os.NewOSFS(osPathFlag) // internally: path = filepath.Abs(path)
if err != nil {
    return err
}

// later on, to serve an http request:
file, err := fs.Open(r.Query().Get("path")) //   /hello?path=somepath