[go: up one dir, main page]
Skip to content

Commit

Permalink
Fix support for Windows
Browse files Browse the repository at this point in the history 
Some of tests were broken when running in Windows. These changes take
steps towards making the tests platform-agnostic. Conversely, the main
logic was not taking into account the Volume name, which could lead to
invalid paths being generated.

Signed-off-by: Paulo Gomes <pjbgf@linux.com>
  • Loading branch information
@pjbgf
pjbgf committed Aug 29, 2023
1 parent 05b6423 commit c121231
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 37 deletions.
12 changes: 11 additions & 1 deletion join.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,27 @@ func IsNotExist(err error) bool {
// components in the returned string are not modified (in other words are not
// replaced with symlinks on the filesystem) after this function has returned.
// Such a symlink race is necessarily out-of-scope of SecureJoin.
//
// Volume names in unsafePath are always discarded, regardless if they are
// provided via direct input or when evaluating symlinks. Therefore:
//
// "C:\Temp" + "D:\path\to\file.txt" results in "C:\Temp\path\to\file.txt"
func SecureJoinVFS(root, unsafePath string, vfs VFS) (string, error) {
// Use the os.* VFS implementation if none was specified.
if vfs == nil {
vfs = osVFS{}
}

unsafePath = filepath.FromSlash(unsafePath)
var path bytes.Buffer
n := 0
for unsafePath != "" {
if n > 255 {
return "", &os.PathError{Op: "SecureJoin", Path: root + "/" + unsafePath, Err: syscall.ELOOP}
return "", &os.PathError{Op: "SecureJoin", Path: root + string(filepath.Separator) + unsafePath, Err: syscall.ELOOP}
}

if v := filepath.VolumeName(unsafePath); v != "" {
unsafePath = unsafePath[len(v):]
}

// Next path component, p.
Expand Down
79 changes: 43 additions & 36 deletions join_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"io/ioutil"
"os"
"path/filepath"
"runtime"
"syscall"
"testing"
)
Expand All @@ -22,6 +23,11 @@ func symlink(t *testing.T, oldname, newname string) {
}
}

type input struct {
root, unsafe string
expected string
}

// Test basic handling of symlink expansion.
func TestSymlink(t *testing.T) {
dir, err := ioutil.TempDir("", "TestSymlink")
Expand All @@ -38,21 +44,26 @@ func TestSymlink(t *testing.T) {
symlink(t, "../../../../../../../../../../../../../etc", filepath.Join(dir, "etclink"))
symlink(t, "/../../../../../../../../../../../../../etc/passwd", filepath.Join(dir, "passwd"))

for _, test := range []struct {
root, unsafe string
expected string
}{
rootOrVol := string(filepath.Separator)
if vol := filepath.VolumeName(dir); vol != "" {
rootOrVol = vol + rootOrVol
}

tc := []input{
// Make sure that expansion with a root of '/' proceeds in the expected fashion.
{"/", filepath.Join(dir, "passwd"), "/etc/passwd"},
{"/", filepath.Join(dir, "etclink"), "/etc"},
{"/", filepath.Join(dir, "etc"), filepath.Join(dir, "somepath")},
{rootOrVol, filepath.Join(dir, "passwd"), filepath.Join(rootOrVol, "etc", "passwd")},
{rootOrVol, filepath.Join(dir, "etclink"), filepath.Join(rootOrVol, "etc")},

{rootOrVol, filepath.Join(dir, "etc"), filepath.Join(dir, "somepath")},
// Now test scoped expansion.
{dir, "passwd", filepath.Join(dir, "somepath", "passwd")},
{dir, "etclink", filepath.Join(dir, "somepath")},
{dir, "etc", filepath.Join(dir, "somepath")},
{dir, "etc/test", filepath.Join(dir, "somepath", "test")},
{dir, "etc/test/..", filepath.Join(dir, "somepath")},
} {
}

for _, test := range tc {
got, err := SecureJoin(test.root, test.unsafe)
if err != nil {
t.Errorf("securejoin(%q, %q): unexpected error: %v", test.root, test.unsafe, err)
Expand Down Expand Up @@ -85,29 +96,31 @@ func TestNoSymlink(t *testing.T) {
}
defer os.RemoveAll(dir)

for _, test := range []struct {
root, unsafe string
}{
// TODO: Do we need to have some conditional FromSlash handling here?
{dir, "somepath"},
{dir, "even/more/path"},
{dir, "/this/is/a/path"},
{dir, "also/a/../path/././/with/some/./.././junk"},
{dir, "yetanother/../path/././/with/some/./.././junk../../../../../../../../../../../../etc/passwd"},
{dir, "/../../../../../../../../../../../../../../../../etc/passwd"},
{dir, "../../../../../../../../../../../../../../../../somedir"},
{dir, "../../../../../../../../../../../../../../../../"},
{dir, "./../../.././././../../../../../../../../../../../../../../../../etc passwd"},
} {
expected := filepath.Join(test.root, filepath.Clean(string(filepath.Separator)+test.unsafe))
tc := []input{
{dir, "somepath", filepath.Join(dir, "somepath")},
{dir, "even/more/path", filepath.Join(dir, "even", "more", "path")},
{dir, "/this/is/a/path", filepath.Join(dir, "this", "is", "a", "path")},
{dir, "also/a/../path/././/with/some/./.././junk", filepath.Join(dir, "also", "path", "with", "junk")},
{dir, "yetanother/../path/././/with/some/./.././junk../../../../../../../../../../../../etc/passwd", filepath.Join(dir, "etc", "passwd")},
{dir, "/../../../../../../../../../../../../../../../../etc/passwd", filepath.Join(dir, "etc", "passwd")},
{dir, "../../../../../../../../../../../../../../../../somedir", filepath.Join(dir, "somedir")},
{dir, "../../../../../../../../../../../../../../../../", filepath.Join(dir)},
{dir, "./../../.././././../../../../../../../../../../../../../../../../etc passwd", filepath.Join(dir, "etc passwd")},
}

if runtime.GOOS == "windows" {
tc = append(tc, []input{
{dir, "d:\\etc\\test", filepath.Join(dir, "etc", "test")},
}...)
}

for _, test := range tc {
got, err := SecureJoin(test.root, test.unsafe)
if err != nil {
t.Errorf("securejoin(%q, %q): unexpected error: %v", test.root, test.unsafe, err)
continue
}
if got != expected {
t.Errorf("securejoin(%q, %q): expected %q, got %q", test.root, test.unsafe, expected, got)
continue
if got != test.expected {
t.Errorf("securejoin(%q, %q): expected %q, got %q", test.root, test.unsafe, test.expected, got)
}
}
}
Expand All @@ -130,10 +143,7 @@ func TestNonLexical(t *testing.T) {
symlink(t, "/../cousinparent/cousin", filepath.Join(dir, "subdir", "link2"))
symlink(t, "/../../../../../../../../../../../../../../../../cousinparent/cousin", filepath.Join(dir, "subdir", "link3"))

for _, test := range []struct {
root, unsafe string
expected string
}{
for _, test := range []input{
{dir, "subdir", filepath.Join(dir, "subdir")},
{dir, "subdir/link/test", filepath.Join(dir, "cousinparent", "cousin", "test")},
{dir, "subdir/link2/test", filepath.Join(dir, "cousinparent", "cousin", "test")},
Expand Down Expand Up @@ -188,7 +198,7 @@ func TestSymlinkLoop(t *testing.T) {
} {
got, err := SecureJoin(test.root, test.unsafe)
if !errors.Is(err, syscall.ELOOP) {
t.Errorf("securejoin(%q, %q): expected ELOOP, got %v & %q", test.root, test.unsafe, err, got)
t.Errorf("securejoin(%q, %q): expected ELOOP, got %q & %v", test.root, test.unsafe, got, err)
continue
}
}
Expand Down Expand Up @@ -275,10 +285,7 @@ func TestSecureJoinVFS(t *testing.T) {
symlink(t, "/../cousinparent/cousin", filepath.Join(dir, "subdir", "link2"))
symlink(t, "/../../../../../../../../../../../../../../../../cousinparent/cousin", filepath.Join(dir, "subdir", "link3"))

for _, test := range []struct {
root, unsafe string
expected string
}{
for _, test := range []input{
{dir, "subdir", filepath.Join(dir, "subdir")},
{dir, "subdir/link/test", filepath.Join(dir, "cousinparent", "cousin", "test")},
{dir, "subdir/link2/test", filepath.Join(dir, "cousinparent", "cousin", "test")},
Expand Down

0 comments on commit c121231

Please sign in to comment.