dhall-haskell
dhall-haskell copied to clipboard
dhall-lang
Enable translation of maps into directory trees
Thought. It might make sense to add a check on paths not being absolute and not containing .. segments to ensure that it's impossible to affect the filesystem outside of the working dir.
Opinions?
@nikita-volkov I do have a pending PR https://github.com/dhall-lang/dhall-haskell/pull/2437 with another possibility of constructing a directory tree and I am interested in you feedback there. Would it help you with your use case? Also, I like your ideas of prohibiting some file paths by default and I think those are valuable on their own.
@mmhat I've taken a look at the usage example provided in your PR. It looks like your solution provides a finer grain control, which should definitely find use in non-trivial tasks. However it also comes at a price of a more complicated API for the developer.
I think both solutions should be merged. Mine will be useful for simple cases, providing a simpler API, yours will be useful for advanced cases.
@mmhat I've taken a look at the usage example provided in your PR. It looks like your solution provides a finer grain control, which should definitely find use in non-trivial tasks. However it also comes at a price of a more complicated API for the developer.
I think both solutions should be merged. Mine will be useful for simple cases, providing a simpler API, yours will be useful for advanced cases.
@nikita-volkov Thanks for the feedback; I think its a fair assessment! I also don't see a problem to support both ways and I'd be happy to see it merged.
Trying to resolve the conflicts after the changes by @mmhat. The only issue is with the added allowSeparators global option in that change. I don't see an intuitive interpretation of that option with the changes that I've introduced here. Do we really need that option? It does complicate things and I'm failing to see the necessity of it.
Anyway, I've adapted the changes. Now setting allowSeparators to False will effectively disable the tree construction using maps as introduced in this PR. Should be mergeable now.
@nikita-volkov I had a look at your changes and added the CLI flags in https://github.com/nikita-volkov/dhall-haskell/pull/1. This is a PR against your feature branch; Please review the changes there and give me some feedback.
Regarding path separators in file names: I like the default (do not allow them) as it is now; The directory structure should primarily stem from the Dhall code and not from the content of an opaque text value.
@mmhat I don't like the idea of parameterising the compiler with various modes of interpretation of one codebase. This will likely lead to all sorts of confusion for the users and will require authors to inform the users about the compiler settings needed to compile their codebase. That's the reason I'm skeptical about the added allowSeparators flag in the first place, your PR to my fork adds even more options.
There can only be one truth and IMO it's best for build tools to exhibit the same property: there can only be one interpretation of one codebase. That's what makes builds predictable.
Pinging @Gabriella439 to draw some attention.
I don't really disagree with you on the fact that it is a good thing to have one and only one interpretation of a codebase. My point is more about the defaults here: I see all three checks as safety features guarding against programming errors (e.g. no path separators) or security risks (e.g. no ".."). The CLI flags are simply escape hatches in the case You Know What You Are Doing. So to me its more about disabling safety checks than interpreting the program in a different way.
If we disagree here on the defaults of those settings then I am happy to discuss that, but I'd like a switch for enabling/disabling anyway.