Allow use of fs.FS for $INCLUDE and wrap errors #1526
Conversation
This adds ZoneParser.SetIncludeAllowedFS, to specify an fs.FS when enabling support for $INCLUDE, for reading included files from somewhere other than the local filesystem. I've also modified ParseError to support wrapping another error, such as errors encountered while opening the $INCLUDE target. This allows for much more robust handling, using errors.Is() instead of testing for particular strings (which may not be identical between fs.FS implementations). ParseError was being constructed in a lot of places using positional instead of named members. Updating ParseError initialization after the new member field was added makes this change seem a lot larger than it actually is. The changes here should be completely backwards compatible. The ParseError change should be invisible to anyone not trying to unwrap it, and ZoneParser will continue to use os.Open if the existing SetIncludeAllowed method is called instead of the new SetIncludeAllowedFS method.
|
(Deleted previous comment where I completely got wrong when fs.FS was introduced. Sorry for the noise.) fs.FS was introduced in Go 1.16: https://go.dev/doc/go1.16#fs Error wrapping was introduced in Go 1.13: https://go.dev/doc/go1.13#error_wrapping |
|
brilliant idea! Code looks good, bit messy indeed due to the error changes. Some comments incomding |
scan.go
Outdated
| @@ -220,6 +228,12 @@ func (zp *ZoneParser) SetIncludeAllowed(v bool) { | |||
| zp.includeAllowed = v | |||
| } | |||
|
|
|||
| // SetIncludeAllowedFS controls whether $INCLUDE directives are allowed, and | |||
| // specifies an [fs.FS] from which to open and read files. | |||
| func (zp *ZoneParser) SetIncludeAllowedFS(v bool, fsys fs.FS) { | |||
There was a problem hiding this comment.
On the fence if we need a whole new method for this. Should we give SetIncludeAllowed an optional fs.FS parameter instead?
There was a problem hiding this comment.
I added a new method because I didn't want to break the method signature for existing library users.
We could change SetIncludeAllowed(v bool) to SetIncludeAllowed(v bool, fs ...fs.FS), which should be backwards compatible, but would need to decide how to handle multiple fs.FS being provided. (Try each one in sequence until $INCLUDE is found?)
There was a problem hiding this comment.
Shipping separate methods as a non-breaking change, and then consolidating them if/when you release a v2 is an option.
There was a problem hiding this comment.
ack. Think my main objection is the overlap in functionality. Maybe a better idea is to have a seperate method to set the underlying FS? SetIncludeFS that just sets the FS
Rather than duplicate functionality between SetIncludeAllowed and SetIncludeAllowedFS, have a method SetIncludeFS, which only sets the fs.FS. I've improved the documentation to point out some considerations for users hoping to use fs.FS as a security boundary. Per the fs.ValidPath documentation, fs.FS implementations must use path (not filepath) semantics, with slash as a separator (even on Windows). Some, like os.DirFS, also require all paths to be relative. I've clarified this in the documentation, made the includePath manipulation more robust to edge cases, and added some additional tests for relative and absolute paths.
This adds ZoneParser.SetIncludeAllowedFS, to specify an fs.FS when enabling support for $INCLUDE, for reading included files from somewhere other than the local filesystem.
I've also modified ParseError to support wrapping another error, such as errors encountered while opening the $INCLUDE target. This allows for much more robust handling, using errors.Is() instead of testing for particular strings (which may not be identical between fs.FS implementations).
ParseError was being constructed in a lot of places using positional instead of named members. Updating ParseError initialization after the new member field was added makes this change seem a lot larger than it actually is.
The changes here should be completely backwards compatible. The ParseError change should be invisible to anyone not trying to unwrap it, and ZoneParser will continue to use os.Open if the existing SetIncludeAllowed method is called instead of the new SetIncludeAllowedFS method.