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.

Sign up for GitHub

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

Jump to bottom

Allow use of fs.FS for $INCLUDE and wrap errors #1526

Merged
merged 2 commits into from
Jan 15, 2024
Merged

Allow use of fs.FS for $INCLUDE and wrap errors #1526

merged 2 commits into from
Jan 15, 2024

Commits on Jan 9, 2024

  1. Allow use of fs.FS for $INCLUDE and wrap errors 

    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.
    @dpifke
    dpifke committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    3a4f2f2 View commit details
    Browse the repository at this point in the history

  2. Don't duplicate SetIncludeAllowed; clarify edge cases 

    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.
    @dpifke
    dpifke committed Jan 9, 2024
    Configuration menu
    Copy the full SHA
    d59cf64 View commit details
    Browse the repository at this point in the history