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

io/fs: add func Sub(fsys FS, dir string) FS #42322

Closed
carlmjohnson opened this issue Nov 1, 2020 · 27 comments
Closed

io/fs: add func Sub(fsys FS, dir string) FS #42322

carlmjohnson opened this issue Nov 1, 2020 · 27 comments

Comments

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Nov 1, 2020

Originally posted by @zikaeroh in #41191 (comment)

Trying this out now; one wart with the go:embed directive is that if I embed build/*, the filenames still have the prefix build/. If I want to then serve that directory via http.FS, there's no easy way to add the prefix that's required to access them if needed (without writing a wrapper, which then hits the problem of needing to list out every potential method that the FS may have...).

e.g.:

//go:embed build/*
var buildDir embed.FS

// Serve some SPA build dir as the app; oops, needs to be build/index.html
http.Handle("/", http.FileServer(http.FS(buildDir)))

// or

//go:embed static/*
var staticDir embed.FS

// Oops; needs to have a static prefix.
http.Handle("/static/*, http.StripPrefix("/static", http.FileServer(http.FS(staticDir))))

// Could be this, but only because the prefix happens to match:
http.Handle("/static/*, http.FileServer(http.FS(staticDir)))

I know the intent is that one could write go:embed foo/* bar/* baz.ext and get all of those files, but I think it's going to be very common to simply embed a directory and serve it as static assets via the http package. I expect this to be a gotcha as people switch from things like http.Dir("static") or pkger.Dir("/internal/web/static") where the prefix is already handled, to the new embed.FS.

I'm not really sure how to file this, as it's sort of an interplay with embed, io/fs, and net/http.


My comment on @zikaeroh's comment:

I think the best way to resolve this is by adding a general purpose fs.WithPrefix helper that creates a new FS that is restricted to the given subdirectory prefix. The example above would become:

//go:embed build/*
var buildDir embed.FS

http.Handle("/", http.FileServer(http.FS(fs.WithPrefix("build", buildDir))))

I think this should be in FS so that it can implement optional interfaces as they're invented. I think it will have general applicability for things like creating a zipfile FS and restricting it to a subdirectory and whatnot.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Nov 1, 2020

The main issue with this approach is the whole optional method thing; the moment you use this helper all additional FS methods are lost. To do it "correctly", you end up having to do what httpsnoop had to do to deal with the optional interface problem with ResponseWriter, or if the ErrNotSupported value becomes standard, make io/fs somehow implement every possible method (because you couldn't just embed the result of a new fs.WithPrefix and add on without losing the extra methods entirely). This all was brought up during the design of io/fs on Reddit and the issue thread.

For net/http, this doesn't matter, since http.FS only uses Open, but for more generic uses of io/fs, this might matter more. I mentioned in a followup comment that I could achieve the same thing with an AddPrefix counterpart to StripPrefix in net/http (#41191 (comment)), which might work better, but doesn't solve the overall problem of "I can't subtree an FS".

I'm still not certain if this is a "problem" with how static serving works in net/http (the awkward set of needing all of the prefix strippers to modify Request), or the general problem that the design of io/fs doesn't lend itself to subtreeing if you want retain efficiency. Maybe the later isn't 100% true, though, since who knows how any given FS would really want to handle prefixes.

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Nov 1, 2020

In the comments on the io/fs proposal, as I understood it, the consensus was that the "right" way to deal with the optional interfaces was to implement them all and then return ErrNotSupported if the underlying FS didn't have the method in question.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Nov 1, 2020

Forgive me, I meant ErrUnsupported (#41198), not ErrNotSupported (which is a different thing...). Note that while ErrUnsupported is accepted, it's not implemented and not used in io/fs.

@Merovius
Copy link

@Merovius Merovius commented Nov 1, 2020

@zikaeroh The general recommendation for optional methods is to add them all and fall-back to the appropriate helper in io/fs if you can't implement them specifically. That helper should then do the correct thing (i.e. type-assert OR fallback-implementation OR return ErrUnsupported). That it was not necessary to use ErrUnsupported (or equivalent) in io/fs so far is not a reason it couldn't be used.

If there is an optional interface for which this approach doesn't work, details would be very useful (probably in a separate bug) because it would point to a serious design issue which should be discussed before go 1.16 is released. I don't know of any such instance yet, but that doesn't mean it doesn't exist. In particular, I don't see any problems in interactions with optional interfaces I know about and a hypothetical StripPrefixFS.

I think this proposal is a good idea and I would very much like to see it happen. To me, this is a fundamental primitive of composability for file systems and I would like to see it in io/fs - just like io contains MultiReader, TeeReader and other fundamental composability primitives of I/O-streams.

@toothrot toothrot added this to the Backlog milestone Nov 2, 2020
@toothrot
Copy link
Contributor

@toothrot toothrot commented Nov 2, 2020

/cc @rsc

@ianlancetaylor ianlancetaylor changed the title io/fs: add func WithPrefix(string) FS proposal: io/fs: add func WithPrefix(string) FS Nov 4, 2020
@ianlancetaylor ianlancetaylor modified the milestones: Backlog, Proposal Nov 4, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Nov 4, 2020
@rsc rsc moved this from Incoming to Active in Proposals Nov 4, 2020
@frioux
Copy link

@frioux frioux commented Nov 4, 2020

My initial instinct was that this would be useful because it could add more safety when trying to "sandbox" parts of the filesystem. But then I remembered that in the io/fs draft design .. is forbidden already. In theory this seems like a pretty easy to implement proposal (string concatenation and all the stuff needed to handle optional methods, right?) Is there some subtle reason this should be in the standard lib?

(apologies for poorly formatted example; written directly in browser.)

type WPFS struct {
   inner  FS
   prefix string
}

func (fs WPFS) Open(p string) (File, error) {
   return fs.inner.Open(inner.prefix + "/" + p)
}

// same thing but for ReadFile, Stat, ReadDir, Glob, and maybe Rename, OpenFile?

func WithPrefix(f FS, p string) WPFS {
   return WPFS{f, p}
}
@Merovius
Copy link

@Merovius Merovius commented Nov 4, 2020

Is there some subtle reason this should be in the standard lib?

No, not a subtle reason. The non-subtle reason is what I mentioned above: It's a fundamental primitive of composition, so making it available is akin to other top-level functions in the io package, for example.

@icholy
Copy link

@icholy icholy commented Nov 10, 2020

An alternative name could be Chroot

@rsc
Copy link
Contributor

@rsc rsc commented Nov 11, 2020

When we talked about adding this during the proposal discussion on Reddit, I was thinking it could be called just fs.Sub, as in sub-tree. Chroot is a bit too obscure (but accurate!), and WithPrefix is maybe too much about the mechanics (and maybe inaccurate! The argument to Open is without the prefix).

I was thinking we could put this off until the next release and get more experience with io/fs, but I agree that it is a critical piece to have to use with embedding.

Does anyone object to adding func Sub(fsys FS, dir string) FS to io/fs and also the corresponding SubFS interface?

@rsc rsc changed the title proposal: io/fs: add func WithPrefix(string) FS proposal: io/fs: add func Sub(fsys FS, dir string) FS Nov 11, 2020
@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Nov 11, 2020

What should the SubFS interface look like? Sub(string) (FS, error)? Will it work with the not implemented error (I forget which name won for that)? SGTM as long as it returns some error.

@Merovius
Copy link

@Merovius Merovius commented Nov 11, 2020

I agree that the interface should return an error, because if, for example, dir does not exist, it makes sense to report that sooner rather than later. And if the method returns an error, so should fs.Sub itself.
However, I don't think the method would ever need to return ErrNotImplemented (or somesuch) because it can always safely fall back to fs.Sub.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 18, 2020

It's a lot more awkward to call Sub compared to http.StripPrefix if it returns an error.
And it's easy to check if you want the error: call Stat(".") on the result.
I'm leaning toward leaving the error off.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 18, 2020

@carlmjohnson, no there is no "not implemented". If an implementation wants to provide a Sub but doesn't know how, it can call fs.Sub.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 18, 2020

Based on the discussion, this seems like a likely accept, and for Go 1.16 so that it is part of the initial FS API.

@icholy

This comment has been hidden.

@Merovius
Copy link

@Merovius Merovius commented Nov 18, 2020

@rsc I'm thinking about an io/fs using openat and similar syscalls. The simplest implementation of that would just use a single uintptr of the dirfd. Sub would then just open the directory and return the resulting file-descriptor. ISTM that if Sub can't return an error, you need additional bookkeeping information and extra checks to make sure the dirfd is actually valid and you still have to actually do something with the error returned by open. Not a total dealbreaker, but IMO returning an error from Sub is cleaner in this example.

@Merovius
Copy link

@Merovius Merovius commented Nov 18, 2020

@icholy No, you pass the wrapped file-system to fs.Sub, not yourself.

@rsc rsc moved this from Active to Likely Accept in Proposals Nov 19, 2020
@rasky
Copy link
Member

@rasky rasky commented Nov 19, 2020

I think the proposal is useful, but I think it should be documented that this is not a security features, as even just reading a symlinked file allows to escape the filesystem root (in fact, at some point we might want to add a fs.Jail in the future with security implications, but that's for another proposal).

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Nov 19, 2020

If fs.Jail is a thing, maybe it matters less that fs.Sub has no error checking. fs.Jail could complain that you don't have permission to use a directory in the first place, etc. Having them separate would also mean that fs.Sub wouldn't have to clean all paths of .. on each and every use, which might be nice for performance.

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Nov 19, 2020

But if fs.Sub is less secure than fs.Jail, would we want to encourage its use in http.FS?

@Merovius
Copy link

@Merovius Merovius commented Nov 19, 2020

http.Dir has the same caveat. I think that's fine.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 2, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Dec 2, 2020
@rsc rsc changed the title proposal: io/fs: add func Sub(fsys FS, dir string) FS io/fs: add func Sub(fsys FS, dir string) FS Dec 2, 2020
@rsc rsc modified the milestones: Proposal, Backlog Dec 2, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Dec 2, 2020

I went to implement this, and it does seem like we need the error result if only for diagnosing invalid arguments properly.
So that's what the CL has.

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 2, 2020

Change https://golang.org/cl/274856 mentions this issue: io/fs: add Sub

@gopherbot gopherbot closed this in 478bde3 Dec 4, 2020
@shibumi

This comment was marked as off-topic.

@Merovius

This comment was marked as off-topic.

@shibumi

This comment was marked as off-topic.

@golang golang locked and limited conversation to collaborators Apr 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet