diff --git a/pkg/fs/confirmeddir.go b/pkg/fs/confirmeddir.go new file mode 100644 index 000000000..9774bb846 --- /dev/null +++ b/pkg/fs/confirmeddir.go @@ -0,0 +1,58 @@ +package fs + +import ( + "path/filepath" + "strings" +) + +// ConfirmedDir is a clean, absolute, delinkified path +// that was confirmed to point to an existing directory. +type ConfirmedDir string + +// HasPrefix returns true if the directory argument +// is a prefix of self (d) from the point of view of +// a file system. +// +// I.e., it's true if the argument equals or contains +// self (d) in a file path sense. +// +// HasPrefix emulates the semantics of strings.HasPrefix +// such that the following are true: +// +// strings.HasPrefix("foobar", "foobar") +// strings.HasPrefix("foobar", "foo") +// strings.HasPrefix("foobar", "") +// +// d := fSys.ConfirmDir("/foo/bar") +// d.HasPrefix("/foo/bar") +// d.HasPrefix("/foo") +// d.HasPrefix("/") +// +// Not contacting a file system here to check for +// actual path existence. +// +// This is tested on linux, but will have trouble +// on other operating systems. As soon as related +// work is completed in the core filepath package, +// this code should be refactored to use it. +// See: +// https://github.com/golang/go/issues/18355 +// https://github.com/golang/dep/issues/296 +// https://github.com/golang/dep/blob/master/internal/fs/fs.go#L33 +// https://codereview.appspot.com/5712045 +func (d ConfirmedDir) HasPrefix(path ConfirmedDir) bool { + if path.String() == string(filepath.Separator) || path == d { + return true + } + return strings.HasPrefix( + string(d), + string(path)+string(filepath.Separator)) +} + +func (d ConfirmedDir) Join(path string) string { + return filepath.Join(string(d), path) +} + +func (d ConfirmedDir) String() string { + return string(d) +} diff --git a/pkg/fs/confirmeddir_test.go b/pkg/fs/confirmeddir_test.go new file mode 100644 index 000000000..1f097b14e --- /dev/null +++ b/pkg/fs/confirmeddir_test.go @@ -0,0 +1,87 @@ +package fs + +import ( + "testing" +) + +func TestJoin(t *testing.T) { + fSys := MakeFakeFS() + err := fSys.Mkdir("/foo") + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + d, f, err := fSys.CleanedAbs("/foo") + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if f != "" { + t.Fatalf("unexpected file: %v", f) + } + if d.Join("bar") != "/foo/bar" { + t.Fatalf("expected join %s", d.Join("bar")) + } +} + +func TestHasPrefix_Slash(t *testing.T) { + d, f, err := MakeFakeFS().CleanedAbs("/") + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if f != "" { + t.Fatalf("unexpected file: %v", f) + } + if d.HasPrefix("/hey") { + t.Fatalf("should be false") + } + if !d.HasPrefix("/") { + t.Fatalf("/ should have the prefix /") + } +} + +func TestHasPrefix_SlashFoo(t *testing.T) { + fSys := MakeFakeFS() + err := fSys.Mkdir("/foo") + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + d, _, err := fSys.CleanedAbs("/foo") + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if d.HasPrefix("/fo") { + t.Fatalf("/foo does not have path prefix /fo") + } + if d.HasPrefix("/fod") { + t.Fatalf("/foo does not have path prefix /fod") + } + if !d.HasPrefix("/foo") { + t.Fatalf("/foo should have prefix /foo") + } +} + +func TestHasPrefix_SlashFooBar(t *testing.T) { + fSys := MakeFakeFS() + err := fSys.MkdirAll("/foo/bar") + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + d, _, err := fSys.CleanedAbs("/foo/bar") + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if d.HasPrefix("/fo") { + t.Fatalf("/foo/bar does not have path prefix /fo") + } + if d.HasPrefix("/foobar") { + t.Fatalf("/foo/bar does not have path prefix /foobar") + } + if !d.HasPrefix("/foo/bar") { + t.Fatalf("/foo/bar should have prefix /foo/bar") + } + if !d.HasPrefix("/foo") { + t.Fatalf("/foo/bar should have prefix /foo") + } + if !d.HasPrefix("/") { + t.Fatalf("/foo/bar should have prefix /") + } +} diff --git a/pkg/fs/fakefs.go b/pkg/fs/fakefs.go index 37669ed15..1a8fad667 100644 --- a/pkg/fs/fakefs.go +++ b/pkg/fs/fakefs.go @@ -102,16 +102,16 @@ func (fs *fakeFs) Open(name string) (File, error) { return fs.m[name], nil } -// CleanedAbs does nothing and cannot fail. -func (fs *fakeFs) CleanedAbs(path string) (string, string, error) { +// CleanedAbs cannot fail. +func (fs *fakeFs) CleanedAbs(path string) (ConfirmedDir, string, error) { if fs.IsDir(path) { - return path, "", nil + return ConfirmedDir(path), "", nil } d := filepath.Dir(path) if d == path { - return d, "", nil + return ConfirmedDir(d), "", nil } - return d, filepath.Base(path), nil + return ConfirmedDir(d), filepath.Base(path), nil } // Exists returns true if file is known. diff --git a/pkg/fs/fs.go b/pkg/fs/fs.go index 65f5e0148..4b47dba64 100644 --- a/pkg/fs/fs.go +++ b/pkg/fs/fs.go @@ -30,7 +30,7 @@ type FileSystem interface { RemoveAll(name string) error Open(name string) (File, error) IsDir(name string) bool - CleanedAbs(path string) (string, string, error) + CleanedAbs(path string) (ConfirmedDir, string, error) Exists(name string) bool Glob(pattern string) ([]string, error) ReadFile(name string) ([]byte, error) diff --git a/pkg/fs/realfs.go b/pkg/fs/realfs.go index 75f6f2987..11e5813b7 100644 --- a/pkg/fs/realfs.go +++ b/pkg/fs/realfs.go @@ -60,7 +60,8 @@ func (realFS) Open(name string) (File, error) { return os.Open(name) } // and file components. If the entire path is // a directory, the file component is an empty // string. -func (x realFS) CleanedAbs(path string) (string, string, error) { +func (x realFS) CleanedAbs( + path string) (ConfirmedDir, string, error) { absRoot, err := filepath.Abs(path) if err != nil { return "", "", fmt.Errorf( @@ -72,7 +73,7 @@ func (x realFS) CleanedAbs(path string) (string, string, error) { "evalsymlink failure on '%s' : %v", path, err) } if x.IsDir(deLinked) { - return deLinked, "", nil + return ConfirmedDir(deLinked), "", nil } d := filepath.Dir(deLinked) if !x.IsDir(d) { @@ -89,7 +90,7 @@ func (x realFS) CleanedAbs(path string) (string, string, error) { log.Fatalf("these should be equal: '%s', '%s'", filepath.Join(d, f), deLinked) } - return d, f, nil + return ConfirmedDir(d), f, nil } // Exists returns true if os.Stat succeeds. diff --git a/pkg/fs/realfs_test.go b/pkg/fs/realfs_test.go index 7add5fb64..74bc318cd 100644 --- a/pkg/fs/realfs_test.go +++ b/pkg/fs/realfs_test.go @@ -52,7 +52,7 @@ func TestCleanedAbs_1(t *testing.T) { if err != nil { t.Fatalf("unexpected err=%v", err) } - if d != wd { + if d.String() != wd { t.Fatalf("unexpected d=%s", d) } if f != "" { @@ -90,7 +90,7 @@ func TestCleanedAbs_3(t *testing.T) { if err != nil { t.Fatalf("unexpected err=%v", err) } - if d != testDir { + if d.String() != testDir { t.Fatalf("unexpected d=%s", d) } if f != "foo" { @@ -119,7 +119,7 @@ func TestCleanedAbs_4(t *testing.T) { if err != nil { t.Fatalf("unexpected err=%v", err) } - if d != filepath.Join(testDir, "d1", "d2") { + if d.String() != filepath.Join(testDir, "d1", "d2") { t.Fatalf("unexpected d=%s", d) } if f != "" { @@ -131,7 +131,7 @@ func TestCleanedAbs_4(t *testing.T) { if err != nil { t.Fatalf("unexpected err=%v", err) } - if d != filepath.Join(testDir, "d1", "d2") { + if d.String() != filepath.Join(testDir, "d1", "d2") { t.Fatalf("unexpected d=%s", d) } if f != "bar" { diff --git a/pkg/loader/fileloader.go b/pkg/loader/fileloader.go index 0384020f8..02864582c 100644 --- a/pkg/loader/fileloader.go +++ b/pkg/loader/fileloader.go @@ -78,10 +78,16 @@ import ( // from /etc/passwd will fail. // type fileLoader struct { - // Previously visited absolute directory paths. - // Tracked to avoid cycles. - // The last entry is the current root. - roots []string + // Loader that spawned this loader. + // Used to avoid cycles. + referrer *fileLoader + // An absolute, cleaned path to a directory. + // The Load function reads from this directory, + // or directories below it. + root fs.ConfirmedDir + // URI, if any, used for a download into root. + // TODO(monopole): use non-string type. + uri string // File system utilities. fSys fs.FileSystem // Used to clone repositories. @@ -103,12 +109,12 @@ func NewFileLoaderAtRoot(fSys fs.FileSystem) *fileLoader { // Root returns the absolute path that is prepended to any // relative paths used in Load. func (l *fileLoader) Root() string { - return l.roots[len(l.roots)-1] + return l.root.String() } func newLoaderOrDie(fSys fs.FileSystem, path string) *fileLoader { l, err := newFileLoaderAt( - path, fSys, []string{}, simpleGitCloner) + path, fSys, nil, simpleGitCloner) if err != nil { log.Fatalf("unable to make loader at '%s'; %v", path, err) } @@ -117,34 +123,33 @@ func newLoaderOrDie(fSys fs.FileSystem, path string) *fileLoader { // newFileLoaderAt returns a new fileLoader with given root. func newFileLoaderAt( - root string, fSys fs.FileSystem, - roots []string, cloner gitCloner) (*fileLoader, error) { - if root == "" { + possibleRoot string, fSys fs.FileSystem, + referrer *fileLoader, cloner gitCloner) (*fileLoader, error) { + if possibleRoot == "" { return nil, fmt.Errorf( "loader root cannot be empty") } - absRoot, f, err := fSys.CleanedAbs(root) + root, f, err := fSys.CleanedAbs(possibleRoot) if err != nil { return nil, fmt.Errorf( - "absolute path error in '%s' : %v", root, err) + "absolute path error in '%s' : %v", possibleRoot, err) } if f != "" { return nil, fmt.Errorf( "got file '%s', but '%s' must be a directory to be a root", - f, root) + f, possibleRoot) } - if err := isPathEqualToOrAbove(absRoot, roots); err != nil { - return nil, err - } - if !fSys.IsDir(absRoot) { - return nil, fmt.Errorf( - "'%s' does not exist or is not a directory", absRoot) + if referrer != nil { + if err := referrer.errIfArgEqualOrHigher(root); err != nil { + return nil, err + } } return &fileLoader{ - roots: append(roots, absRoot), - fSys: fSys, - cloner: cloner, - cleaner: func() error { return nil }, + root: root, + referrer: referrer, + fSys: fSys, + cloner: cloner, + cleaner: func() error { return nil }, }, nil } @@ -155,68 +160,82 @@ func (l *fileLoader) New(path string) (ifc.Loader, error) { return nil, fmt.Errorf("new root cannot be empty") } if isRepoUrl(path) { - // This works well enough for purpose at hand to detect - // previously visited URLs and thus avoid cycles. - if err := isPathEqualToOrAbove(path, l.roots); err != nil { + // Avoid cycles. + if err := l.errIfPreviouslySeenUri(path); err != nil { return nil, err } - return newGitLoader(path, l.fSys, l.roots, l.cloner) + return newGitLoader(path, l.fSys, l.referrer, l.cloner) } if filepath.IsAbs(path) { return nil, fmt.Errorf("new root '%s' cannot be absolute", path) } return newFileLoaderAt( - filepath.Join(l.Root(), path), l.fSys, l.roots, l.cloner) + l.root.Join(path), l.fSys, l, l.cloner) } // newGitLoader returns a new Loader pinned to a temporary // directory holding a cloned git repo. func newGitLoader( - root string, fSys fs.FileSystem, - roots []string, cloner gitCloner) (ifc.Loader, error) { - tmpDirForRepo, pathInRepo, err := cloner(root) + uri string, fSys fs.FileSystem, + referrer *fileLoader, cloner gitCloner) (ifc.Loader, error) { + tmpDirForRepo, pathInRepo, err := cloner(uri) if err != nil { return nil, err } - trueRoot := filepath.Join(tmpDirForRepo, pathInRepo) - if !fSys.IsDir(trueRoot) { + root, f, err := fSys.CleanedAbs( + filepath.Join(tmpDirForRepo, pathInRepo)) + if err != nil { + return nil, err + } + if f != "" { return nil, fmt.Errorf( - "something wrong cloning '%s'; unable to find '%s'", - root, trueRoot) + "'%s' refers to file '%s'; expecting directory", pathInRepo, f) } return &fileLoader{ - roots: append(roots, root, trueRoot), - fSys: fSys, - cloner: cloner, - cleaner: func() error { return fSys.RemoveAll(tmpDirForRepo) }, + root: root, + referrer: referrer, + uri: uri, + fSys: fSys, + cloner: cloner, + cleaner: func() error { return fSys.RemoveAll(tmpDirForRepo) }, }, nil } -// isPathEqualToOrAbove tests whether the 1st argument, -// viewed as a path to a directory, is equal to or above -// any of the paths in the 2nd argument. It's assumed -// that all paths are cleaned, delinkified and absolute. -func isPathEqualToOrAbove(path string, roots []string) error { - terminated := path + string(filepath.Separator) - for _, r := range roots { - if r == path || strings.HasPrefix(r, terminated) { - return fmt.Errorf( - "cycle detected: new root '%s' contains previous root '%s'", - path, r) - } +// errIfArgEqualOrHigher tests whether the argument, +// is equal to or above the root of any ancestor. +func (l *fileLoader) errIfArgEqualOrHigher( + candidateRoot fs.ConfirmedDir) error { + if l.root.HasPrefix(candidateRoot) { + return fmt.Errorf( + "cycle detected: candidate root '%s' contains visited root '%s'", + candidateRoot, l.root) } - return nil + if l.referrer == nil { + return nil + } + return l.referrer.errIfArgEqualOrHigher(candidateRoot) +} + +func (l *fileLoader) errIfPreviouslySeenUri(uri string) error { + if strings.HasPrefix(l.uri, uri) { + return fmt.Errorf( + "cycle detected: URI '%s' referenced by previous URI '%s'", + uri, l.uri) + } + if l.referrer == nil { + return nil + } + return l.referrer.errIfPreviouslySeenUri(uri) } // Load returns content of file at the given relative path, // else an error. The path must refer to a file in or -// below the current Root(). +// below the current root. func (l *fileLoader) Load(path string) ([]byte, error) { if filepath.IsAbs(path) { return nil, l.loadOutOfBounds(path) } - d, f, err := l.fSys.CleanedAbs( - filepath.Join(l.Root(), path)) + d, f, err := l.fSys.CleanedAbs(l.root.Join(path)) if err != nil { return nil, err } @@ -224,41 +243,16 @@ func (l *fileLoader) Load(path string) ([]byte, error) { return nil, fmt.Errorf( "'%s' must be a file (got d='%s')", path, d) } - path = filepath.Join(d, f) - if !l.isInOrBelowRoot(path) { + if !d.HasPrefix(l.root) { return nil, l.loadOutOfBounds(path) } - return l.fSys.ReadFile(path) -} - -// isInOrBelowRoot is true if the argument is in or -// below Root() from purely a path perspective (no -// check for actual file existence). For this to work, -// both the given argument "path" and l.Root() must -// be cleaned, absolute paths, and l.Root() must be -// a directory. Both conditions enforced elsewhere. -// -// This is tested on linux, but will have trouble -// on other operating systems. As soon as related -// work is completed in the core filepath package, -// this code should be refactored to use it. -// See: -// https://github.com/golang/go/issues/18355 -// https://github.com/golang/dep/issues/296 -// https://github.com/golang/dep/blob/master/internal/fs/fs.go#L33 -// https://codereview.appspot.com/5712045 -func (l *fileLoader) isInOrBelowRoot(path string) bool { - if l.Root() == string(filepath.Separator) { - return true - } - return strings.HasPrefix( - path, l.Root()+string(filepath.Separator)) + return l.fSys.ReadFile(d.Join(f)) } func (l *fileLoader) loadOutOfBounds(path string) error { return fmt.Errorf( "security; file '%s' is not in or below '%s'", - path, l.Root()) + path, l.root) } // Cleanup runs the cleaner. diff --git a/pkg/loader/fileloader_test.go b/pkg/loader/fileloader_test.go index f1e861005..82ca48d8d 100644 --- a/pkg/loader/fileloader_test.go +++ b/pkg/loader/fileloader_test.go @@ -63,15 +63,15 @@ func MakeFakeFs(td []testData) fs.FileSystem { func TestNewFileLoaderAt_DemandsDirectory(t *testing.T) { fSys := MakeFakeFs(testCases) - _, err := newFileLoaderAt("/foo", fSys, []string{}, nil) + _, err := newFileLoaderAt("/foo", fSys, nil, nil) if err != nil { t.Fatalf("Unexpected error - a directory should work.") } - _, err = newFileLoaderAt("/foo/project", fSys, []string{}, nil) + _, err = newFileLoaderAt("/foo/project", fSys, nil, nil) if err != nil { t.Fatalf("Unexpected error - a directory should work.") } - _, err = newFileLoaderAt("/foo/project/fileA.yaml", fSys, []string{}, nil) + _, err = newFileLoaderAt("/foo/project/fileA.yaml", fSys, nil, nil) if err == nil { t.Fatalf("Expected error - a file should not work.") } diff --git a/pkg/loader/gitcloner_test.go b/pkg/loader/gitcloner_test.go index 2c5df4474..a3867ca03 100644 --- a/pkg/loader/gitcloner_test.go +++ b/pkg/loader/gitcloner_test.go @@ -167,7 +167,7 @@ func TestGitLoader(t *testing.T) { whatever `)) l, err := newGitLoader( - url, fSys, []string{}, + url, fSys, nil, makeFakeGitCloner(t, fSys, coRoot)) if err != nil { t.Fatalf("unexpected err: %v\n", err) @@ -177,10 +177,10 @@ whatever coRoot+"/"+pathInRepo, l.Root()) } if _, err = l.New(url); err == nil { - t.Fatalf("expected cycle error") + t.Fatalf("expected cycle error 1") } if _, err = l.New(rootUrl + "/" + "foo"); err == nil { - t.Fatalf("expected cycle error") + t.Fatalf("expected cycle error 2") } pathInRepo = "foo/overlay" diff --git a/pkg/loader/loader.go b/pkg/loader/loader.go index 5e954095d..5e5aa8664 100644 --- a/pkg/loader/loader.go +++ b/pkg/loader/loader.go @@ -26,8 +26,8 @@ import ( func NewLoader(root string, fSys fs.FileSystem) (ifc.Loader, error) { if isRepoUrl(root) { return newGitLoader( - root, fSys, []string{}, simpleGitCloner) + root, fSys, nil, simpleGitCloner) } return newFileLoaderAt( - root, fSys, []string{}, simpleGitCloner) + root, fSys, nil, simpleGitCloner) }