If RootDir is, for example, "/foo/bar/", won't this add an extra slash blindly? In which case we'd have the same bug, but for non-root cases?

Also, this is using string(filepath.Separator), when everything else above is using "/". This is ContentValue type, receiving strings from scriptlets, so these strings aren't supposed to be portable.

Also² :), I don't understand the logic that is being used here, to be honest. We seem to have comprehensive logic right above that ensures path is absolute, cleaning it up properly and storing in cpath. Then, we validate we really want cpath by running it through the checkers... finally, we drop it, and merge the original path with RootDir, and then play games with that variable to make sure it's actually inside RootDir? No... with a second RootDir, that has a slash. Feels surprisingly convoluted when we literally have just done all of those checks for the path, and we are the ones joining it.

If RootDir is, for example, "/foo/bar/", won't this add an extra slash blindly? In which case we'd have the same bug, but for non-root cases?

We can assume that the root directory has been cleaned by filepath.Clean. We could enforce it by cleaning the directory again which IMO is better for correctness.

Also, this is using string(filepath.Separator), when everything else above is using "/". This is ContentValue type, receiving strings from scriptlets, so these strings aren't supposed to be portable.

That is true and I thought about removing it because we are only being generic in this module while the rest of the code uses / directly. However, I wanted to not do that as part of this bug fix and instead respect consistency with the module.

Also² :), I don't understand the logic that is being used here, to be honest. We seem to have comprehensive logic right above that ensures path is absolute, cleaning it up properly and storing in cpath. Then, we validate we really want cpath by running it through the checkers... finally, we drop it, and merge the original path with RootDir, and then play games with that variable to make sure it's actually inside RootDir? No... with a second RootDir, that has a slash. Feels surprisingly convoluted when we literally have just done all of those checks for the path, and we are the ones joining it.

Maybe I am missing something but I see that the original code does the following:

1. Check if path and c.RootDir are absolute which doesn't actually check that they cannot be used to escape the target directory. Example: fmt.Println(filepath.IsAbs("/foo/../bar")) // prints true.
2. Call CheckRead and CheckWrite which is defined by the user of the API to validate paths. For example, slicer checks that the paths are declared and marked as mutable.
3. In the previous code we were also checking that path does not escape target dir. The only way to do that is to append both and clean it later (if we appended the cleaned version of the path cpath we would NOT produce an error, see [1]). Once the final path is cleaned we check that root is a prefix and for that to work, the path used for prefix needs to end in / or else strings.HasPrefix("/foo", "/foobar") == true.

Probably I am missing something in the sequence above that will make it easier to write, but I fail to see what, do you have any ideas?

The ultimate problem here is that we are dealing with several kinds of paths:

• The ones coming from the script which are "absolute" but will actually be translated into paths inside targetDir. We need to run checkRead and checkWrite validation with these paths and not with the final ones, application logic only cares about this.
• The paths used in the actual filesystem to create the final files which are a concatenation of the targetDir plus the "absolute" paths in the SDFs. These ones are validated as a final measure to guarantee they don't escape the targetDir.

[1]: For example if path = /../bar, target = /etc, then cpath = /bar/ and the final path is /etc/bar/ instead of an error.

[1]: For example if path = /../bar, target = /etc, then cpath = /bar/ and the final path is /etc/bar/ instead of an error.

That's the only thing that we gain by having this convoluted logic that already led to bugs, and I'm not even sure if it's an actual feature (the root hasn't been broken out of).

Otherwise, that's a lot of text to what seems like a simple issue: isn't cpath validated and clean? What else do we need to do about it other than making it relative to root?

Do you prefer to simplify it and remove the checks? The only downside as you mention is that we don't have an explicit error in the case above.

How can this be true when we validated that both RootDir and path are Abs, per checks earlier above?

Removed in #230. Just a minor correction, we have validated that the path is absolute true, but here we are relying on the fact that cleaning an absolute path returns an absolute path (note we are appending cpath not path).