search

mvdan / sh

A shell parser, formatter, and interpreter with bash support; includes shfmt
https://pkg.go.dev/mvdan.cc/sh/v3
BSD 3-Clause "New" or "Revised" License
7.28k stars 346 forks source link

Add interp.ReadDirHandler for intercepting glob expansion. #783

Closed alecthomas closed 2 years ago

alecthomas commented 2 years ago

I believe this is one of the last locations where the interpreter can bypass sandboxing, though there are a few explicit calls to os.* that remain.

As an aside related to #630, it would be amazing if for v4 the interp package could be refactored to solely (or as close as possible) interact through the fs.FS interface. This would then allow the interpreter to be completely sandboxed. For example the existing Runner.stat() function, and so on. This would also remove the need for OpenHandler and ReadDirHandler.

mvdan commented 2 years ago

I fully agree that a better design with io/fs can be had; the API predates that package :)

I wonder if we could deprecate the current FS handlers and add a new io/fs handler. Presumably this would be a simpler API to use, as one would set one handler instead of 3 or more.

mvdan commented 2 years ago

What I mean by deprecate is to still accept the old narrow option, and add the new io/fs option that covers all FS interactions. Using both old and new options at once would be an error/panic. Most new users would want the new option; the old option would continue working until v4.

mvdan commented 2 years ago

Let me know if you would like to have a try at a generic fs.FS option, or if you'd like someone else to try instead. I'm not 100% sure it will just work - perhaps there will be some internal incompatibility that will make supporting both old and new APIs hard, but I'm hoping not.

alecthomas commented 2 years ago

I'd be interested, but I don't really have the bandwidth at the moment. This PR was a straightforward change, but I suspect that shoehorning fs.FS in will be quite a bit more work.

mvdan commented 2 years ago

That's fair. I'll see if I can take a crack at it soon.

alecthomas commented 2 years ago

I might have a friend interested in taking a crack. If you're okay to wait I'll let you know by th end of week?

mvdan commented 2 years ago

Yep sounds good, no rush :)

mvdan commented 2 years ago

@alecthomas I'm happy to help the person write that change, by the way - they can reach me on the Gophers Slack too, where we have a #shell channel :)

alecthomas commented 2 years ago

I took a bit more of a look at this and there are a few issues:

  1. Yet another variant of LookPath called LookPathDirFS will need to be added.
  2. fs.FS is read-only, so there will still be a split of responsibilities between the FS and OpenHandler
  3. fs.FS does not allow "rooted" paths, so these paths will need to be remapped, eg. "/" => ".", "/etc/passwd" => "etc/passwd" - returned errors that include the path will use these remapped forms.
mvdan commented 2 years ago

Did you mean to close this, or are you intending to open a new PR?

alecthomas commented 2 years ago

Nope, not even sure how that happened... I just pushed to my fork... bizarre! I'll reopen if I can, or send another PR.

alecthomas commented 2 years ago

(realised what happened - I had moved it to a branch to work on the FS branch and master ended up with zero commit differences)

mvdan commented 2 years ago

Friendly nudge; happy to merge when the nit is addressed :) Or if you're busy, I'll apply the change myself and squash-merge tomorrow.

alecthomas commented 2 years ago

Ah whoops! Done.