diff --git a/loading/loading.go b/loading/loading.go index 0b38ac1..b06450c 100644 --- a/loading/loading.go +++ b/loading/loading.go @@ -84,7 +84,9 @@ func LoadStrategy(pth string, local, remote func(string) ([]byte, error), opts . if isFSBacked { // other fs.FS (e.g. os.DirFS) and os.Root loaders also use "/" on every platform. - // Escaping paths (absolute, "..", escaping symlinks) are rejected by the loader, not rewritten here. + // Path confinement is enforced by the loader, not here: the os.Root loader rebases + // absolute in-root paths and rejects escaping paths ("..", out-of-root absolute, + // escaping symlinks); an fs.FS loader rejects what its file system does not allow. return local(filepath.ToSlash(cpth)) } diff --git a/loading/options.go b/loading/options.go index 2c12823..2bd60a1 100644 --- a/loading/options.go +++ b/loading/options.go @@ -8,6 +8,7 @@ import ( "io/fs" "net/http" "os" + "path/filepath" "time" ) @@ -39,13 +40,21 @@ func (fo fileOptions) ReadFileFunc() func(string) ([]byte, error) { root := fo.root return func(name string) ([]byte, error) { + // os.Root only accepts paths relative to the root, but callers (and this package's + // own file:// handling) routinely produce absolute paths. Rebase an absolute path + // onto the root before handing it to os.Root. + rel, err := rootRelative(root, name) + if err != nil { + return nil, errors.Join(err, ErrLoader) + } + r, err := os.OpenRoot(root) if err != nil { return nil, errors.Join(err, ErrLoader) } defer func() { _ = r.Close() }() - return r.ReadFile(name) + return r.ReadFile(rel) } } @@ -56,6 +65,30 @@ func (fo fileOptions) ReadFileFunc() func(string) ([]byte, error) { return fo.fs.ReadFile } +// rootRelative expresses name as a path relative to root, so that it can be resolved by os.Root. +// +// A relative name is returned unchanged: os.Root confines it directly (including "../" traversal +// and symlink escapes, which it rejects at open time). +// +// An absolute name is rebased onto root. If it cannot be expressed relative to root — for +// example because it lives on a different volume on Windows — filepath.Rel returns an error, +// which is propagated so the read is rejected rather than silently escaping the root. An +// absolute path that lexically escapes root yields a "../" prefix here and is then rejected by +// os.Root. +func rootRelative(root, name string) (string, error) { + osName := filepath.FromSlash(name) + if !filepath.IsAbs(osName) { + return name, nil + } + + absRoot, err := filepath.Abs(filepath.FromSlash(root)) + if err != nil { + return "", err + } + + return filepath.Rel(absRoot, osName) +} + // WithTimeout sets a timeout for the remote file loader. // // The default timeout is 30s. @@ -123,10 +156,12 @@ func WithFS(filesystem fs.FS) Option { // WithRoot confines local file loading to dir. // -// Every requested path is resolved relative to dir, and any path that would escape dir — -// whether through an absolute path, ".." traversal, or a symlink pointing outside dir — is -// rejected. This is built on [os.Root] and is therefore resistant to the symlink escapes -// that a plain [os.DirFS] does not prevent. +// Every requested path is resolved within dir. A relative path is resolved against dir; an +// absolute path is rebased onto dir (so a caller that normalizes references to absolute paths, +// such as github.com/go-openapi/spec, still resolves correctly). Any path that would escape dir +// — through ".." traversal, an absolute path pointing outside dir, or a symlink pointing outside +// dir — is rejected. This is built on [os.Root] and is therefore resistant to the symlink +// escapes that a plain [os.DirFS] does not prevent. // // WithRoot is the recommended option when loading specs from a location derived from // untrusted input. It applies to local loading only and has no effect on remote diff --git a/loading/withroot_test.go b/loading/withroot_test.go index 232786d..cabf4ab 100644 --- a/loading/withroot_test.go +++ b/loading/withroot_test.go @@ -7,6 +7,7 @@ import ( "io/fs" "os" "path/filepath" + "strings" "testing" "testing/fstest" @@ -53,6 +54,29 @@ func TestWithRoot(t *testing.T) { }) }) + t.Run("should load absolute paths confined to the root", func(t *testing.T) { + // Callers such as go-openapi/spec normalize $ref targets to absolute paths before + // loading. These must resolve when they point within the root (they are rebased onto it), + // not be rejected merely for being absolute. + abs := filepath.Join(root, "api.yaml") + for _, pth := range []string{ + abs, // absolute path within the root + "file://" + filepath.ToSlash(abs), // absolute path within the root, via a file:// URI + } { + t.Run(pth, func(t *testing.T) { + b, err := LoadFromFileOrHTTP(pth, WithRoot(root)) + require.NoError(t, err) + assert.EqualT(t, inside, string(b)) + }) + } + + t.Run("nested absolute path", func(t *testing.T) { + b, err := LoadFromFileOrHTTP(filepath.Join(root, "sub", "api.yaml"), WithRoot(root)) + require.NoError(t, err) + assert.EqualT(t, nested, string(b)) + }) + }) + t.Run("should reject paths escaping the root", func(t *testing.T) { for _, pth := range []string{ "file:///etc/passwd", // absolute via file:// URI @@ -112,3 +136,29 @@ func TestWithRoot(t *testing.T) { assert.EqualT(t, secret, string(b)) }) } + +func TestRootRelative(t *testing.T) { + root := filepath.Join(t.TempDir(), "root") + + t.Run("relative names are returned unchanged", func(t *testing.T) { + // os.Root confines relative names directly, so they are passed through verbatim. + for _, name := range []string{"api.yaml", "sub/api.yaml", "sub/../api.yaml", "../escape.yaml"} { + got, err := rootRelative(root, name) + require.NoError(t, err) + assert.EqualT(t, name, got) + } + }) + + t.Run("absolute in-root names are rebased onto the root", func(t *testing.T) { + got, err := rootRelative(root, filepath.Join(root, "sub", "api.yaml")) + require.NoError(t, err) + assert.EqualT(t, filepath.Join("sub", "api.yaml"), got) + }) + + t.Run("absolute escaping names yield a traversal path (rejected later by os.Root)", func(t *testing.T) { + got, err := rootRelative(root, filepath.Join(filepath.Dir(root), "secret.txt")) + require.NoError(t, err) + assert.TrueT(t, strings.HasPrefix(got, ".."+string(filepath.Separator)), + "expected a traversal path, got %q", got) + }) +} diff --git a/loading/withroot_windows_test.go b/loading/withroot_windows_test.go new file mode 100644 index 0000000..89cfe10 --- /dev/null +++ b/loading/withroot_windows_test.go @@ -0,0 +1,20 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +//go:build windows + +package loading + +import ( + "testing" + + "github.com/go-openapi/testify/v2/require" +) + +func TestRootRelativeVolumeMismatch(t *testing.T) { + // A path on a different volume cannot be expressed relative to the root. filepath.Rel + // returns an error, which rootRelative must propagate so the read is rejected rather than + // silently escaping the root. + _, err := rootRelative(`C:\root`, `D:\secret.txt`) + require.Error(t, err) +}