diff --git a/internal/safepaths/absolute.go b/internal/safepaths/absolute.go index db0551b32d4..c3039f19f99 100644 --- a/internal/safepaths/absolute.go +++ b/internal/safepaths/absolute.go @@ -70,5 +70,12 @@ type PathTraversalError struct { } func (e PathTraversalError) Error() string { + if e.Base.path == "" { + path := filepath.Join(e.Elems...) + if len(e.Elems) == 1 { + path = e.Elems[0] + } + return fmt.Sprintf("path %q is not a local child path", path) + } return fmt.Sprintf("joining %s and %s would be a traversal", e.Base, filepath.Join(e.Elems...)) } diff --git a/internal/safepaths/link_other.go b/internal/safepaths/link_other.go new file mode 100644 index 00000000000..ca5f624390f --- /dev/null +++ b/internal/safepaths/link_other.go @@ -0,0 +1,9 @@ +//go:build !windows + +package safepaths + +import "os" + +func isPathRedirect(_ *Root, _ *os.Root, _ string, info os.FileInfo) (bool, error) { + return info.Mode()&os.ModeSymlink != 0, nil +} diff --git a/internal/safepaths/link_windows.go b/internal/safepaths/link_windows.go new file mode 100644 index 00000000000..0e5725b91fb --- /dev/null +++ b/internal/safepaths/link_windows.go @@ -0,0 +1,37 @@ +package safepaths + +import ( + "errors" + "os" + "syscall" +) + +// os.Root intentionally refuses every final reparse point. Windows does not +// expose a supported handle-relative open that both follows non-link reparse +// data and atomically refuses a race to a name-surrogate redirect, so fail +// closed rather than re-resolving the pinned parent by path. +var errUnsupportedNonLinkReparse = errors.New("non-link reparse points cannot be safely opened beneath a rooted directory on Windows") + +func isPathRedirect(owner *Root, root *os.Root, name string, info os.FileInfo) (bool, error) { + return classifyWindowsPathRedirect(info, func() error { + _, err := owner.readlink(root, name) + return err + }) +} + +func classifyWindowsPathRedirect(info os.FileInfo, readlink func() error) (bool, error) { + if info.Mode()&os.ModeSymlink != 0 { + return true, nil + } + data, ok := info.Sys().(*syscall.Win32FileAttributeData) + if !ok || data.FileAttributes&syscall.FILE_ATTRIBUTE_REPARSE_POINT == 0 { + return false, nil + } + if err := readlink(); err != nil { + if os.IsNotExist(err) { + return false, errUnsupportedNonLinkReparse + } + return false, err + } + return true, nil +} diff --git a/internal/safepaths/link_windows_test.go b/internal/safepaths/link_windows_test.go new file mode 100644 index 00000000000..484d26a1d1d --- /dev/null +++ b/internal/safepaths/link_windows_test.go @@ -0,0 +1,172 @@ +package safepaths + +import ( + "errors" + "io/fs" + "os" + "os/exec" + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestReparsePointClassification(t *testing.T) { + t.Run("name surrogate redirects", func(t *testing.T) { + info := reparsePointInfo{mode: fs.ModeSymlink} + redirect, err := classifyWindowsPathRedirect(info, func() error { + return errors.New("should not be called") + }) + require.NoError(t, err) + assert.True(t, redirect) + }) + t.Run("mount point redirects", func(t *testing.T) { + info := reparsePointInfo{ + mode: fs.ModeIrregular, + attributes: syscall.FILE_ATTRIBUTE_REPARSE_POINT, + } + redirect, err := classifyWindowsPathRedirect(info, func() error { return nil }) + require.NoError(t, err) + assert.True(t, redirect) + }) + t.Run("non-link reparse point fails closed", func(t *testing.T) { + info := reparsePointInfo{ + mode: fs.ModeDir, + attributes: syscall.FILE_ATTRIBUTE_REPARSE_POINT, + } + redirect, err := classifyWindowsPathRedirect(info, func() error { return os.ErrNotExist }) + assert.False(t, redirect) + require.ErrorIs(t, err, errUnsupportedNonLinkReparse) + }) + t.Run("unexpected readlink error is returned", func(t *testing.T) { + info := reparsePointInfo{ + mode: fs.ModeIrregular, + attributes: syscall.FILE_ATTRIBUTE_REPARSE_POINT, + } + + redirect, err := classifyWindowsPathRedirect(info, func() error { + return errors.New("readlink failed") + }) + assert.False(t, redirect) + require.ErrorContains(t, err, "readlink failed") + }) +} + +func TestCreateReplaceFailsClosedForNonLinkReparse(t *testing.T) { + rootDir := t.TempDir() + path := filepath.Join(rootDir, "entry") + require.NoError(t, os.WriteFile(path, []byte("original"), 0o644)) + before, err := os.Stat(path) + require.NoError(t, err) + + root, err := OpenRoot(rootDir) + require.NoError(t, err) + defer root.Close() + root.lstat = func(*os.Root, string) (os.FileInfo, error) { + return reparsePointInfo{ + mode: fs.ModeIrregular, + attributes: syscall.FILE_ATTRIBUTE_REPARSE_POINT, + }, nil + } + root.readlink = func(*os.Root, string) (string, error) { + return "", os.ErrNotExist + } + + file, err := root.Create("entry", 0o644, 0o755, true) + assert.Nil(t, file) + require.ErrorIs(t, err, errUnsupportedNonLinkReparse) + after, err := os.Stat(path) + require.NoError(t, err) + assert.True(t, os.SameFile(before, after)) + content, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "original", string(content)) +} + +func TestRootRejectsJunction(t *testing.T) { + rootDir := t.TempDir() + target := t.TempDir() + junction := filepath.Join(rootDir, "junction") + if output, err := exec.Command("cmd", "/c", "mklink", "/J", junction, target).CombinedOutput(); err != nil { + t.Skipf("junctions unavailable: %v: %s", err, output) + } + + root, err := OpenRoot(rootDir) + require.NoError(t, err) + defer root.Close() + + err = root.WriteFile(filepath.Join("junction", "file.txt"), []byte("content"), 0o644, 0o755, false) + require.ErrorContains(t, err, "symbolic link") + assert.NoFileExists(t, filepath.Join(target, "file.txt")) + + err = root.WriteFile("junction", []byte("content"), 0o644, 0o755, true) + require.Error(t, err) + info, err := os.Lstat(junction) + require.NoError(t, err) + assert.NotZero(t, info.Sys().(*syscall.Win32FileAttributeData).FileAttributes&syscall.FILE_ATTRIBUTE_REPARSE_POINT) +} + +func TestOpenFileRejectsWindowsTrailingSeparator(t *testing.T) { + tests := []struct { + name string + separator string + }{ + {name: "backslash", separator: `\`}, + {name: "slash", separator: `/`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + root := t.TempDir() + path := filepath.Join(root, "output") + tt.separator + + file, err := OpenFile(path, 0o644, 0o755, false) + if file != nil { + _ = file.Close() + } + require.ErrorContains(t, err, "ends in a separator") + _, statErr := os.Lstat(filepath.Join(root, "output")) + require.True(t, os.IsNotExist(statErr)) + }) + } +} + +func TestRootCreateRejectsWindowsTrailingSeparator(t *testing.T) { + tests := []struct { + name string + separator string + }{ + {name: "backslash", separator: `\`}, + {name: "slash", separator: `/`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rootDir := t.TempDir() + root, err := OpenRoot(rootDir) + require.NoError(t, err) + defer root.Close() + + err = root.WriteFile("output"+tt.separator, []byte("content"), 0o644, 0o755, false) + require.ErrorContains(t, err, "ends in a separator") + _, statErr := os.Lstat(filepath.Join(rootDir, "output")) + require.True(t, os.IsNotExist(statErr)) + assert.NoFileExists(t, filepath.Join(rootDir, "output", "output")) + }) + } +} + +type reparsePointInfo struct { + mode fs.FileMode + attributes uint32 +} + +func (reparsePointInfo) Name() string { return "reparse" } +func (reparsePointInfo) Size() int64 { return 0 } +func (i reparsePointInfo) Mode() fs.FileMode { return i.mode } +func (reparsePointInfo) ModTime() time.Time { return time.Time{} } +func (i reparsePointInfo) IsDir() bool { return i.mode.IsDir() } +func (i reparsePointInfo) Sys() any { + return &syscall.Win32FileAttributeData{FileAttributes: i.attributes} +} diff --git a/internal/safepaths/open_nonblock_other.go b/internal/safepaths/open_nonblock_other.go new file mode 100644 index 00000000000..811e11af76c --- /dev/null +++ b/internal/safepaths/open_nonblock_other.go @@ -0,0 +1,13 @@ +//go:build !unix + +package safepaths + +import "os" + +func nonBlockingOpenFlag() int { + return 0 +} + +func clearNonblocking(*os.File) error { + return nil +} diff --git a/internal/safepaths/open_nonblock_unix.go b/internal/safepaths/open_nonblock_unix.go new file mode 100644 index 00000000000..4bd24868da4 --- /dev/null +++ b/internal/safepaths/open_nonblock_unix.go @@ -0,0 +1,16 @@ +//go:build unix + +package safepaths + +import ( + "os" + "syscall" +) + +func nonBlockingOpenFlag() int { + return syscall.O_NONBLOCK +} + +func clearNonblocking(file *os.File) error { + return syscall.SetNonblock(int(file.Fd()), false) +} diff --git a/internal/safepaths/root.go b/internal/safepaths/root.go new file mode 100644 index 00000000000..215aa210dad --- /dev/null +++ b/internal/safepaths/root.go @@ -0,0 +1,488 @@ +package safepaths + +import ( + "fmt" + "io" + "os" + "path/filepath" + "strings" + "syscall" +) + +// Root restricts filesystem operations to a directory tree and refuses to +// traverse symbolic links below that directory. +// +// The caller owns each returned Root and must call Close when it is +// no longer needed. +type Root struct { + root *os.Root + openRoot func(*os.Root, string) (*os.Root, error) + openFile func(*os.Root, string, int, os.FileMode) (*os.File, error) + lstat func(*os.Root, string) (os.FileInfo, error) + readlink func(*os.Root, string) (string, error) + stat func(*os.Root, string) (os.FileInfo, error) +} + +// OpenRoot opens an existing directory as a rooted filesystem. +func OpenRoot(path string) (*Root, error) { + absolute, err := ParseAbsolute(path) + if err != nil { + return nil, err + } + + root, err := os.OpenRoot(absolute.String()) + if err != nil { + return nil, err + } + + return &Root{ + root: root, + openRoot: func(root *os.Root, name string) (*os.Root, error) { + return root.OpenRoot(name) + }, + openFile: func(root *os.Root, name string, flag int, perm os.FileMode) (*os.File, error) { + return root.OpenFile(name, flag, perm) + }, + lstat: func(root *os.Root, name string) (os.FileInfo, error) { + return root.Lstat(name) + }, + readlink: func(root *os.Root, name string) (string, error) { + return root.Readlink(name) + }, + stat: func(root *os.Root, name string) (os.FileInfo, error) { + return root.Stat(name) + }, + }, nil +} + +// OpenRootDir creates and opens a user-selected directory as a rooted filesystem. +// The selected path may contain symbolic links; child paths beneath the opened +// root may not. +func OpenRootDir(path string, perm os.FileMode) (*Root, error) { + if path == "" { + path = "." + } + if err := os.MkdirAll(path, perm); err != nil { + return nil, err + } + return OpenRoot(path) +} + +// OpenFile creates a file without traversing symbolic links below its rooted +// parent. Creation is always exclusive, including when replace is false. +// filePerm and dirPerm specify creation modes before applying the umask. +func OpenFile(path string, filePerm, dirPerm os.FileMode, replace bool) (*os.File, error) { + if hasTrailingPathSeparator(path) { + return nil, trailingPathSeparatorError(path) + } + parent := filepath.Dir(path) + if err := os.MkdirAll(parent, dirPerm); err != nil { + return nil, err + } + root, err := OpenRoot(parent) + if err != nil { + return nil, err + } + file, createErr := root.Create(filepath.Base(path), filePerm, dirPerm, replace) + closeErr := root.Close() + if createErr != nil { + return nil, createErr + } + if closeErr != nil { + _ = file.Close() + return nil, closeErr + } + return file, nil +} + +// WriteFile writes a complete file without traversing symbolic links. +// Creation permissions follow [OpenFile]. +func WriteFile(path string, data []byte, filePerm, dirPerm os.FileMode, replace bool) (err error) { + file, err := OpenFile(path, filePerm, dirPerm, replace) + if err != nil { + return err + } + defer func() { + if closeErr := file.Close(); err == nil && closeErr != nil { + err = closeErr + } + }() + + _, err = file.Write(data) + return err +} + +// Close closes the rooted filesystem. +func (r *Root) Close() error { + return r.root.Close() +} + +// Sub creates and opens a rooted subdirectory. +func (r *Root) Sub(name string, perm os.FileMode) (*Root, error) { + local, err := r.localPath(name) + if err != nil { + return nil, err + } + root, err := r.openDirectories(local, perm) + if err != nil { + return nil, err + } + + return &Root{ + root: root, + openRoot: r.openRoot, + openFile: r.openFile, + lstat: r.lstat, + readlink: r.readlink, + stat: r.stat, + }, nil +} + +// MkdirAll creates a directory path without traversing symbolic links. +func (r *Root) MkdirAll(name string, perm os.FileMode) error { + local, err := r.localPath(name) + if err != nil { + return err + } + if local == "." { + return nil + } + root, err := r.openDirectories(local, perm) + if err != nil { + return err + } + return root.Close() +} + +// Validate checks name using the same lexical rules as [ValidateChild]. +func (r *Root) Validate(name string) error { + _, err := r.localPath(name) + return err +} + +// Create creates a new file beneath the root. Creation is exclusive unless an +// existing regular file is safely opened and truncated for replacement. +// filePerm and dirPerm specify creation modes before applying the umask. +// Existing regular files and directories retain their permissions. +func (r *Root) Create(name string, filePerm, dirPerm os.FileMode, replace bool) (*os.File, error) { + if hasTrailingPathSeparator(name) { + return nil, trailingPathSeparatorError(name) + } + local, err := r.localPath(name) + if err != nil { + return nil, err + } + if local == "." { + return nil, fmt.Errorf("cannot create root directory as a file") + } + parent, err := r.openDirectories(filepath.Dir(local), dirPerm) + if err != nil { + return nil, err + } + defer parent.Close() + base := filepath.Base(local) + + if replace { + info, err := r.lstat(parent, base) + if err == nil { + redirect, err := isPathRedirect(r, parent, base, info) + if err != nil { + return nil, err + } + if redirect { + targetIsDir, err := r.redirectTargetIsDir(parent, base) + if err != nil { + return nil, err + } + if targetIsDir { + return nil, directoryWriteError(name) + } + if err := parent.Remove(base); err != nil { + return nil, err + } + } else { + if info.IsDir() { + return nil, directoryWriteError(name) + } + if info.Mode()&os.ModeNamedPipe != 0 { + return nil, namedPipeWriteError(name) + } + file, err := r.openVerifiedEntry(parent, base, local) + if err != nil { + return nil, err + } + if info.Mode().IsRegular() || info.Mode()&os.ModeIrregular != 0 { + if err := file.Truncate(0); err != nil { + _ = file.Close() + return nil, err + } + if _, err := file.Seek(0, io.SeekStart); err != nil { + _ = file.Close() + return nil, err + } + } + return file, nil + } + } else if !os.IsNotExist(err) { + return nil, err + } + } + + return parent.OpenFile(base, os.O_WRONLY|os.O_CREATE|os.O_EXCL, filePerm) +} + +// WriteFile writes a complete file beneath the root. +// Creation permissions follow [Root.Create]. +func (r *Root) WriteFile(name string, data []byte, filePerm, dirPerm os.FileMode, replace bool) (err error) { + file, err := r.Create(name, filePerm, dirPerm, replace) + if err != nil { + return err + } + defer func() { + if closeErr := file.Close(); err == nil && closeErr != nil { + err = closeErr + } + }() + + _, err = file.Write(data) + return err +} + +// CopyFile copies content into a file beneath the root. +// Creation permissions follow [Root.Create]. +func (r *Root) CopyFile(name string, src io.Reader, filePerm, dirPerm os.FileMode, replace bool) (err error) { + file, err := r.Create(name, filePerm, dirPerm, replace) + if err != nil { + return err + } + defer func() { + if closeErr := file.Close(); err == nil && closeErr != nil { + err = closeErr + } + }() + + _, err = io.Copy(file, src) + return err +} + +func (r *Root) localPath(name string) (string, error) { + return validateChildPath(name) +} + +// ValidateChild reports whether the cleaned name is a lexically contained path, +// using the host operating system's path rules. Joining it to a root stays +// lexically within that root. Nested paths and names that clean to "." are allowed. +// +// This check does not consult the filesystem or the current working directory, +// and does not account for symbolic links. +func ValidateChild(name string) error { + _, err := validateChildPath(name) + return err +} + +func validateChildPath(name string) (string, error) { + local := filepath.Clean(name) + if !filepath.IsLocal(local) { + return "", PathTraversalError{Elems: []string{name}} + } + return local, nil +} + +func (r *Root) openDirectories(name string, perm os.FileMode) (*os.Root, error) { + var current string + parent := r.root + parentOwned := false + closeParent := func() { + if parentOwned { + _ = parent.Close() + } + } + + if name == "." { + root, err := r.openVerifiedDirectory(parent, ".", ".") + if err != nil { + return nil, err + } + return root, nil + } + + for component := range strings.SplitSeq(name, string(filepath.Separator)) { + if component == "" || component == "." { + continue + } + current = filepath.Join(current, component) + + info, err := parent.Lstat(component) + if os.IsNotExist(err) { + if err := parent.Mkdir(component, perm); err != nil && !os.IsExist(err) { + closeParent() + return nil, err + } + info, err = parent.Lstat(component) + } + if err != nil { + closeParent() + return nil, err + } + redirect, redirectErr := isPathRedirect(r, parent, component, info) + if redirectErr != nil { + closeParent() + return nil, redirectErr + } + if redirect { + closeParent() + return nil, SymlinkTraversalError{Path: current} + } + if !info.IsDir() { + closeParent() + return nil, &os.PathError{Op: "open", Path: current, Err: fmt.Errorf("not a directory")} + } + + next, err := r.openVerifiedDirectory(parent, component, current) + if err != nil { + closeParent() + return nil, err + } + if parentOwned { + if err := parent.Close(); err != nil { + _ = next.Close() + return nil, err + } + } + parent = next + parentOwned = true + } + return parent, nil +} + +func (r *Root) openVerifiedDirectory(parent *os.Root, name, displayPath string) (*os.Root, error) { + before, err := parent.Lstat(name) + if err != nil { + return nil, err + } + redirect, err := isPathRedirect(r, parent, name, before) + if err != nil { + return nil, err + } + if redirect { + return nil, SymlinkTraversalError{Path: displayPath} + } + + root, err := r.openRoot(parent, name) + if err != nil { + return nil, err + } + opened, err := root.Stat(".") + if err != nil { + _ = root.Close() + return nil, err + } + after, err := parent.Lstat(name) + if err != nil { + _ = root.Close() + return nil, err + } + redirect, err = isPathRedirect(r, parent, name, after) + if err != nil { + _ = root.Close() + return nil, err + } + if redirect { + _ = root.Close() + return nil, SymlinkTraversalError{Path: displayPath} + } + if !os.SameFile(before, opened) || !os.SameFile(before, after) { + _ = root.Close() + return nil, fmt.Errorf("directory %q changed while opening", displayPath) + } + return root, nil +} + +func (r *Root) openVerifiedEntry(parent *os.Root, name, displayPath string) (*os.File, error) { + before, err := parent.Lstat(name) + if err != nil { + return nil, err + } + redirect, err := isPathRedirect(r, parent, name, before) + if err != nil { + return nil, err + } + if redirect { + return nil, SymlinkTraversalError{Path: displayPath} + } + if before.Mode()&os.ModeNamedPipe != 0 { + return nil, namedPipeWriteError(displayPath) + } + + openFlag := os.O_WRONLY | nonBlockingOpenFlag() + file, err := r.openFile(parent, name, openFlag, 0) + if err != nil { + return nil, err + } + opened, err := file.Stat() + if err != nil { + _ = file.Close() + return nil, err + } + after, err := parent.Lstat(name) + if err != nil { + _ = file.Close() + return nil, err + } + redirect, err = isPathRedirect(r, parent, name, after) + if err != nil { + _ = file.Close() + return nil, err + } + if redirect { + _ = file.Close() + return nil, SymlinkTraversalError{Path: displayPath} + } + if !os.SameFile(before, opened) || !os.SameFile(before, after) { + _ = file.Close() + return nil, fmt.Errorf("file %q changed while opening", displayPath) + } + if openFlag != os.O_WRONLY { + if err := clearNonblocking(file); err != nil { + _ = file.Close() + return nil, err + } + } + return file, nil +} + +func (r *Root) redirectTargetIsDir(parent *os.Root, name string) (bool, error) { + info, err := r.stat(parent, name) + if os.IsNotExist(err) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("could not safely classify redirect target %q: %w", name, err) + } + return info.IsDir(), nil +} + +func directoryWriteError(path string) error { + return &os.PathError{Op: "open", Path: path, Err: syscall.EISDIR} +} + +func namedPipeWriteError(path string) error { + return &os.PathError{Op: "open", Path: path, Err: fmt.Errorf("named pipes cannot be overwritten safely")} +} + +// SymlinkTraversalError reports a symbolic link in a rooted path. +type SymlinkTraversalError struct { + Path string +} + +func (e SymlinkTraversalError) Error() string { + return fmt.Sprintf("refusing to traverse symbolic link %q", e.Path) +} + +func hasTrailingPathSeparator(path string) bool { + return len(path) > 0 && os.IsPathSeparator(path[len(path)-1]) +} + +func trailingPathSeparatorError(path string) error { + return &os.PathError{Op: "open", Path: path, Err: fmt.Errorf("file path ends in a separator")} +} diff --git a/internal/safepaths/root_mode_unix_test.go b/internal/safepaths/root_mode_unix_test.go new file mode 100644 index 00000000000..0ac081914b4 --- /dev/null +++ b/internal/safepaths/root_mode_unix_test.go @@ -0,0 +1,102 @@ +//go:build unix + +package safepaths_test + +import ( + "os" + "path/filepath" + "strings" + "syscall" + "testing" + + "github.com/cli/cli/v2/internal/safepaths" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFileCreationModes(t *testing.T) { + tests := []struct { + name string + filePerm os.FileMode + dirPerm os.FileMode + umask int + replace bool + wantFilePerm os.FileMode + wantDirPerm os.FileMode + }{ + { + name: "separate file and directory modes", + filePerm: 0o600, + dirPerm: 0o700, + wantFilePerm: 0o600, + wantDirPerm: 0o700, + }, + { + name: "creation respects umask", + filePerm: 0o666, + dirPerm: 0o777, + umask: 0o027, + wantFilePerm: 0o640, + wantDirPerm: 0o750, + }, + { + name: "replacement preserves existing modes", + filePerm: 0o666, + dirPerm: 0o777, + replace: true, + wantFilePerm: 0o640, + wantDirPerm: 0o750, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Umask is process-wide, so these tests must stay sequential. + oldUmask := syscall.Umask(tt.umask) + t.Cleanup(func() { syscall.Umask(oldUmask) }) + + for _, operation := range []string{"OpenFile", "WriteFile", "Root.Create", "Root.WriteFile", "Root.CopyFile"} { + t.Run(operation, func(t *testing.T) { + rootDir := t.TempDir() + name := filepath.Join("one", "two", "file.txt") + path := filepath.Join(rootDir, name) + if tt.replace { + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o750)) + require.NoError(t, os.WriteFile(path, []byte("old"), 0o640)) + } + + root, err := safepaths.OpenRoot(rootDir) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, root.Close()) }) + + var file *os.File + switch operation { + case "OpenFile": + file, err = safepaths.OpenFile(path, tt.filePerm, tt.dirPerm, tt.replace) + case "WriteFile": + err = safepaths.WriteFile(path, []byte("new"), tt.filePerm, tt.dirPerm, tt.replace) + case "Root.Create": + file, err = root.Create(name, tt.filePerm, tt.dirPerm, tt.replace) + case "Root.WriteFile": + err = root.WriteFile(name, []byte("new"), tt.filePerm, tt.dirPerm, tt.replace) + case "Root.CopyFile": + err = root.CopyFile(name, strings.NewReader("new"), tt.filePerm, tt.dirPerm, tt.replace) + } + require.NoError(t, err) + if file != nil { + require.NoError(t, file.Close()) + } + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, tt.wantFilePerm, info.Mode().Perm()) + for _, dir := range []string{"one", filepath.Join("one", "two")} { + info, err := os.Stat(filepath.Join(rootDir, dir)) + require.NoError(t, err) + assert.Equal(t, tt.wantDirPerm, info.Mode().Perm(), dir) + } + }) + } + }) + } +} diff --git a/internal/safepaths/root_race_test.go b/internal/safepaths/root_race_test.go new file mode 100644 index 00000000000..7c79d2a39d0 --- /dev/null +++ b/internal/safepaths/root_race_test.go @@ -0,0 +1,41 @@ +package safepaths + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRootRejectsDirectorySwapDuringOpen(t *testing.T) { + rootDir := t.TempDir() + originalDir := filepath.Join(rootDir, "nested") + require.NoError(t, os.Mkdir(originalDir, 0o755)) + redirectedDir := filepath.Join(rootDir, "redirected") + require.NoError(t, os.Mkdir(redirectedDir, 0o755)) + + root, err := OpenRoot(rootDir) + require.NoError(t, err) + defer root.Close() + + defaultOpenRoot := root.openRoot + swapped := false + root.openRoot = func(parent *os.Root, name string) (*os.Root, error) { + if name == "nested" && !swapped { + swapped = true + moved := filepath.Join(rootDir, "moved") + require.NoError(t, os.Rename(originalDir, moved)) + if err := os.Symlink("redirected", originalDir); err != nil { + t.Skipf("directory symlinks unavailable: %v", err) + } + } + return defaultOpenRoot(parent, name) + } + + err = root.WriteFile(filepath.Join("nested", "file.txt"), []byte("content"), 0o644, 0o755, false) + require.Error(t, err) + assert.NoFileExists(t, filepath.Join(redirectedDir, "file.txt")) + assert.NoFileExists(t, filepath.Join(rootDir, "moved", "file.txt")) +} diff --git a/internal/safepaths/root_special_unix_test.go b/internal/safepaths/root_special_unix_test.go new file mode 100644 index 00000000000..d6a7437733d --- /dev/null +++ b/internal/safepaths/root_special_unix_test.go @@ -0,0 +1,96 @@ +//go:build unix + +package safepaths + +import ( + "os" + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCreateReplacePreservesDevice(t *testing.T) { + info, err := os.Lstat("/dev/null") + if err != nil || info.Mode()&os.ModeDevice == 0 { + t.Skip("/dev/null is unavailable") + } + + root, err := OpenRoot("/dev") + require.NoError(t, err) + defer root.Close() + file, err := root.Create("null", 0o644, 0o755, true) + require.NoError(t, err) + _, err = file.Write([]byte("content")) + require.NoError(t, err) + require.NoError(t, file.Close()) + + after, err := os.Lstat("/dev/null") + require.NoError(t, err) + assert.NotZero(t, after.Mode()&os.ModeDevice) + assert.True(t, os.SameFile(info, after)) +} + +func TestCreateReplaceDoesNotUnlinkFIFO(t *testing.T) { + rootDir := t.TempDir() + fifo := filepath.Join(rootDir, "fifo") + require.NoError(t, syscall.Mkfifo(fifo, 0o600)) + before, err := os.Lstat(fifo) + require.NoError(t, err) + + root, err := OpenRoot(rootDir) + require.NoError(t, err) + defer root.Close() + + result := make(chan error, 1) + go func() { + file, err := root.Create("fifo", 0o644, 0o755, true) + if file != nil { + _ = file.Close() + } + result <- err + }() + select { + case err := <-result: + require.ErrorContains(t, err, "named pipes cannot be overwritten safely") + case <-time.After(time.Second): + reader, openErr := os.OpenFile(fifo, os.O_RDONLY|syscall.O_NONBLOCK, 0) + if openErr == nil { + _ = reader.Close() + } + t.Fatal("FIFO replacement blocked without a reader") + } + + after, err := os.Lstat(fifo) + require.NoError(t, err) + assert.NotZero(t, after.Mode()&os.ModeNamedPipe) + assert.True(t, os.SameFile(before, after)) +} + +func TestCreateReplaceDoesNotBlockOnFIFOSwap(t *testing.T) { + rootDir := t.TempDir() + path := filepath.Join(rootDir, "target") + require.NoError(t, os.WriteFile(path, []byte("regular"), 0o600)) + + root, err := OpenRoot(rootDir) + require.NoError(t, err) + defer root.Close() + defaultOpenFile := root.openFile + root.openFile = func(parent *os.Root, name string, flag int, perm os.FileMode) (*os.File, error) { + require.NoError(t, os.Remove(path)) + require.NoError(t, syscall.Mkfifo(path, 0o600)) + return defaultOpenFile(parent, name, flag, perm) + } + + start := time.Now() + file, err := root.Create("target", 0o644, 0o755, true) + assert.Nil(t, file) + require.Error(t, err) + assert.Less(t, time.Since(start), time.Second) + info, err := os.Lstat(path) + require.NoError(t, err) + assert.NotZero(t, info.Mode()&os.ModeNamedPipe) +} diff --git a/internal/safepaths/root_test.go b/internal/safepaths/root_test.go new file mode 100644 index 00000000000..d086ed39a22 --- /dev/null +++ b/internal/safepaths/root_test.go @@ -0,0 +1,392 @@ +package safepaths_test + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/cli/cli/v2/internal/safepaths" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRootCreate(t *testing.T) { + var preservedMode os.FileMode + + tests := []struct { + name string + path string + replace bool + setup func(t *testing.T, root, outside string) + wantErr string + verify func(t *testing.T, root, outside string) + }{ + { + name: "creates nested file", + path: "nested/file.txt", + verify: func(t *testing.T, root, _ string) { + t.Helper() + content, err := os.ReadFile(filepath.Join(root, "nested", "file.txt")) + require.NoError(t, err) + assert.Equal(t, "new", string(content)) + }, + }, + { + name: "rejects root directory as file", + path: ".", + wantErr: "root directory", + }, + { + name: "rejects lexical escape", + path: "../file.txt", + wantErr: "not a local child path", + }, + { + name: "rejects trailing separator", + path: "nested" + string(os.PathSeparator), + wantErr: "ends in a separator", + verify: func(t *testing.T, root, _ string) { + t.Helper() + _, err := os.Lstat(filepath.Join(root, "nested")) + require.True(t, os.IsNotExist(err)) + assert.NoFileExists(t, filepath.Join(root, "nested", "nested")) + }, + }, + { + name: "rejects ancestor symlink", + path: "nested/file.txt", + setup: func(t *testing.T, root, outside string) { + t.Helper() + require.NoError(t, os.Symlink(outside, filepath.Join(root, "nested"))) + }, + wantErr: "symbolic link", + verify: func(t *testing.T, _, outside string) { + t.Helper() + assert.NoFileExists(t, filepath.Join(outside, "file.txt")) + }, + }, + { + name: "no clobber rejects dangling final symlink", + path: "file.txt", + setup: func(t *testing.T, root, outside string) { + t.Helper() + require.NoError(t, os.Symlink(filepath.Join(outside, "missing.txt"), filepath.Join(root, "file.txt"))) + }, + wantErr: "file exists", + verify: func(t *testing.T, _, outside string) { + t.Helper() + assert.NoFileExists(t, filepath.Join(outside, "missing.txt")) + }, + }, + { + name: "clobber replaces final symlink", + path: "file.txt", + replace: true, + setup: func(t *testing.T, root, outside string) { + t.Helper() + require.NoError(t, os.WriteFile(filepath.Join(root, "target.txt"), []byte("old"), 0o644)) + require.NoError(t, os.Symlink("target.txt", filepath.Join(root, "file.txt"))) + }, + verify: func(t *testing.T, root, outside string) { + t.Helper() + info, err := os.Lstat(filepath.Join(root, "file.txt")) + require.NoError(t, err) + assert.Zero(t, info.Mode()&os.ModeSymlink) + content, err := os.ReadFile(filepath.Join(root, "file.txt")) + require.NoError(t, err) + assert.Equal(t, "new", string(content)) + content, err = os.ReadFile(filepath.Join(root, "target.txt")) + require.NoError(t, err) + assert.Equal(t, "old", string(content)) + }, + }, + { + name: "clobber refuses symlink to directory", + path: "file.txt", + replace: true, + setup: func(t *testing.T, root, outside string) { + t.Helper() + require.NoError(t, os.Mkdir(filepath.Join(root, "target"), 0o755)) + require.NoError(t, os.Symlink("target", filepath.Join(root, "file.txt"))) + }, + wantErr: "is a directory", + verify: func(t *testing.T, root, _ string) { + t.Helper() + info, err := os.Lstat(filepath.Join(root, "file.txt")) + require.NoError(t, err) + assert.NotZero(t, info.Mode()&os.ModeSymlink) + }, + }, + { + name: "clobber refuses redirect outside root", + path: "file.txt", + replace: true, + setup: func(t *testing.T, root, outside string) { + t.Helper() + require.NoError(t, os.WriteFile(filepath.Join(outside, "target.txt"), []byte("old"), 0o644)) + require.NoError(t, os.Symlink(filepath.Join(outside, "target.txt"), filepath.Join(root, "file.txt"))) + }, + wantErr: "could not safely classify redirect target", + verify: func(t *testing.T, root, outside string) { + t.Helper() + info, err := os.Lstat(filepath.Join(root, "file.txt")) + require.NoError(t, err) + assert.NotZero(t, info.Mode()&os.ModeSymlink) + content, err := os.ReadFile(filepath.Join(outside, "target.txt")) + require.NoError(t, err) + assert.Equal(t, "old", string(content)) + }, + }, + { + name: "clobber replaces dangling redirect", + path: "file.txt", + replace: true, + setup: func(t *testing.T, root, _ string) { + t.Helper() + require.NoError(t, os.Symlink("missing.txt", filepath.Join(root, "file.txt"))) + }, + verify: func(t *testing.T, root, _ string) { + t.Helper() + info, err := os.Lstat(filepath.Join(root, "file.txt")) + require.NoError(t, err) + assert.Zero(t, info.Mode()&os.ModeSymlink) + assert.NoFileExists(t, filepath.Join(root, "missing.txt")) + }, + }, + { + name: "clobber preserves regular file mode", + path: "file.txt", + replace: true, + setup: func(t *testing.T, root, _ string) { + t.Helper() + require.NoError(t, os.WriteFile(filepath.Join(root, "file.txt"), []byte("old"), 0o600)) + info, err := os.Stat(filepath.Join(root, "file.txt")) + require.NoError(t, err) + preservedMode = info.Mode().Perm() + }, + verify: func(t *testing.T, root, _ string) { + t.Helper() + info, err := os.Stat(filepath.Join(root, "file.txt")) + require.NoError(t, err) + assert.Equal(t, preservedMode, info.Mode().Perm()) + content, err := os.ReadFile(filepath.Join(root, "file.txt")) + require.NoError(t, err) + assert.Equal(t, "new", string(content)) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rootDir := t.TempDir() + outside := t.TempDir() + if tt.setup != nil { + tt.setup(t, rootDir, outside) + } + + root, err := safepaths.OpenRoot(rootDir) + require.NoError(t, err) + defer root.Close() + + err = root.WriteFile(tt.path, []byte("new"), 0o644, 0o755, tt.replace) + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + } else { + require.NoError(t, err) + } + if tt.verify != nil { + tt.verify(t, rootDir, outside) + } + }) + } +} + +func TestValidateChild(t *testing.T) { + root, err := safepaths.OpenRoot(t.TempDir()) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, root.Close()) }) + + validators := []struct { + name string + validate func(string) error + }{ + {name: "ValidateChild", validate: safepaths.ValidateChild}, + {name: "Root.Validate", validate: root.Validate}, + } + tests := []struct { + name string + path string + wantErr bool + }{ + {name: "empty path cleans to root", path: ""}, + {name: "root", path: "."}, + {name: "local child", path: "nested/file.txt"}, + {name: "contained parent component", path: "nested/../file.txt"}, + {name: "trailing separator", path: "nested/"}, + {name: "parent escape", path: "../file.txt", wantErr: true}, + {name: "nested parent escape", path: "nested/../../file.txt", wantErr: true}, + {name: "absolute", path: filepath.Join(string(os.PathSeparator), "file.txt"), wantErr: true}, + {name: "backslash child", path: `nested\file.txt`}, + {name: "backslash parent escape", path: `..\..\file.txt`, wantErr: runtime.GOOS == "windows"}, + {name: "mixed separator escape", path: `nested/..\..\file.txt`, wantErr: runtime.GOOS == "windows"}, + {name: "drive relative path", path: `C:file.txt`, wantErr: runtime.GOOS == "windows"}, + {name: "drive absolute path", path: `C:\file.txt`, wantErr: runtime.GOOS == "windows"}, + {name: "UNC path", path: `\\server\share\file.txt`, wantErr: runtime.GOOS == "windows"}, + {name: "reserved Windows name", path: "NUL", wantErr: runtime.GOOS == "windows"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for _, validator := range validators { + t.Run(validator.name, func(t *testing.T) { + err := validator.validate(tt.path) + if tt.wantErr { + var traversalErr safepaths.PathTraversalError + require.ErrorAs(t, err, &traversalErr) + assert.Equal(t, []string{tt.path}, traversalErr.Elems) + } else { + require.NoError(t, err) + } + }) + } + }) + } +} + +func TestOpenFileRejectsTrailingSeparator(t *testing.T) { + tests := []struct { + name string + path func(root string) string + }{ + { + name: "relative path", + path: func(string) string { + return "output" + string(os.PathSeparator) + }, + }, + { + name: "absolute path", + path: func(root string) string { + return filepath.Join(root, "output") + string(os.PathSeparator) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + root := t.TempDir() + t.Chdir(root) + path := tt.path(root) + + file, err := safepaths.OpenFile(path, 0o644, 0o755, false) + if file != nil { + _ = file.Close() + } + require.ErrorContains(t, err, "ends in a separator") + _, statErr := os.Lstat(filepath.Join(root, "output")) + require.True(t, os.IsNotExist(statErr)) + assert.NoFileExists(t, filepath.Join(root, "output", "output")) + }) + } +} + +func TestOpenFileAllowsTrustedParentSymlinks(t *testing.T) { + tests := []struct { + name string + path func(sandbox string) string + }{ + { + name: "relative", + path: func(string) string { + return filepath.Join("link", "file.txt") + }, + }, + { + name: "absolute", + path: func(sandbox string) string { + return filepath.Join(sandbox, "link", "file.txt") + }, + }, + { + name: "parent relative", + path: func(string) string { + return filepath.Join("..", "link", "file.txt") + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sandbox := t.TempDir() + target := filepath.Join(sandbox, "target") + require.NoError(t, os.Mkdir(target, 0o755)) + require.NoError(t, os.Symlink(target, filepath.Join(sandbox, "link"))) + if tt.name == "parent relative" { + work := filepath.Join(sandbox, "work") + require.NoError(t, os.Mkdir(work, 0o755)) + t.Chdir(work) + } else { + t.Chdir(sandbox) + } + + file, err := safepaths.OpenFile(tt.path(sandbox), 0o644, 0o755, false) + require.NoError(t, err) + _, err = file.Write([]byte("content")) + require.NoError(t, err) + require.NoError(t, file.Close()) + content, err := os.ReadFile(filepath.Join(target, "file.txt")) + require.NoError(t, err) + assert.Equal(t, "content", string(content)) + }) + } +} + +func TestOpenRootDirAllowsTrustedPathSymlinks(t *testing.T) { + tests := []struct { + name string + path func(sandbox string) string + }{ + { + name: "relative", + path: func(string) string { + return filepath.Join("link", "selected") + }, + }, + { + name: "absolute", + path: func(sandbox string) string { + return filepath.Join(sandbox, "link", "selected") + }, + }, + { + name: "parent relative", + path: func(string) string { + return filepath.Join("..", "link", "selected") + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sandbox := t.TempDir() + target := filepath.Join(sandbox, "target") + require.NoError(t, os.Mkdir(target, 0o755)) + require.NoError(t, os.Symlink(target, filepath.Join(sandbox, "link"))) + if tt.name == "parent relative" { + work := filepath.Join(sandbox, "work") + require.NoError(t, os.Mkdir(work, 0o755)) + t.Chdir(work) + } else { + t.Chdir(sandbox) + } + + root, err := safepaths.OpenRootDir(tt.path(sandbox), 0o755) + require.NoError(t, err) + require.NoError(t, root.WriteFile("file.txt", []byte("content"), 0o644, 0o755, false)) + require.NoError(t, root.Close()) + content, err := os.ReadFile(filepath.Join(target, "selected", "file.txt")) + require.NoError(t, err) + assert.Equal(t, "content", string(content)) + }) + } +} diff --git a/internal/safepaths/root_validation_test.go b/internal/safepaths/root_validation_test.go new file mode 100644 index 00000000000..3705e386ee5 --- /dev/null +++ b/internal/safepaths/root_validation_test.go @@ -0,0 +1,76 @@ +package safepaths + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDirectoryIdentityMismatch(t *testing.T) { + rootDir := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(rootDir, "expected"), 0o755)) + require.NoError(t, os.Mkdir(filepath.Join(rootDir, "other"), 0o755)) + + root, err := OpenRoot(rootDir) + require.NoError(t, err) + defer root.Close() + defaultOpenRoot := root.openRoot + root.openRoot = func(parent *os.Root, name string) (*os.Root, error) { + if name == "expected" { + return defaultOpenRoot(parent, "other") + } + return defaultOpenRoot(parent, name) + } + + child, err := root.Sub("expected", 0o755) + assert.Nil(t, child) + require.ErrorContains(t, err, "changed while opening") +} + +func TestFileIdentityMismatch(t *testing.T) { + rootDir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(rootDir, "expected"), []byte("expected"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(rootDir, "other"), []byte("other"), 0o644)) + + root, err := OpenRoot(rootDir) + require.NoError(t, err) + defer root.Close() + defaultOpenFile := root.openFile + root.openFile = func(parent *os.Root, name string, flag int, perm os.FileMode) (*os.File, error) { + if name == "expected" { + return defaultOpenFile(parent, "other", flag, perm) + } + return defaultOpenFile(parent, name, flag, perm) + } + + file, err := root.Create("expected", 0o644, 0o755, true) + assert.Nil(t, file) + require.ErrorContains(t, err, "changed while opening") + content, err := os.ReadFile(filepath.Join(rootDir, "other")) + require.NoError(t, err) + assert.Equal(t, "other", string(content)) +} + +func TestRedirectTargetClassificationError(t *testing.T) { + rootDir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(rootDir, "target"), []byte("target"), 0o644)) + require.NoError(t, os.Symlink("target", filepath.Join(rootDir, "link"))) + + root, err := OpenRoot(rootDir) + require.NoError(t, err) + defer root.Close() + root.stat = func(*os.Root, string) (os.FileInfo, error) { + return nil, errors.New("stat failed") + } + + file, err := root.Create("link", 0o644, 0o755, true) + assert.Nil(t, file) + require.ErrorContains(t, err, "could not safely classify redirect target") + info, err := os.Lstat(filepath.Join(rootDir, "link")) + require.NoError(t, err) + assert.NotZero(t, info.Mode()&os.ModeSymlink) +} diff --git a/internal/zip/fixtures/myproject.zip b/internal/zip/fixtures/myproject.zip deleted file mode 100644 index 2fdf3f90c6e..00000000000 Binary files a/internal/zip/fixtures/myproject.zip and /dev/null differ diff --git a/internal/zip/zip.go b/internal/zip/zip.go index a97f13d186c..8e9657a05fb 100644 --- a/internal/zip/zip.go +++ b/internal/zip/zip.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "os" - "path/filepath" "github.com/cli/cli/v2/internal/safepaths" ) @@ -18,30 +17,23 @@ const ( ) // ExtractZip extracts the contents of a zip archive to destDir. -// Files that would result in path traversal are silently skipped. -// Files that would produce any other error cause the extraction to be aborted, -// and the error is returned. -func ExtractZip(zr *zip.Reader, destDir safepaths.Absolute) error { +// Entries that would escape destDir lexically are skipped. +func ExtractZip(zr *zip.Reader, destDir *safepaths.Root) error { for _, zf := range zr.File { - fpath, err := destDir.Join(zf.Name) - if err != nil { + if err := extractZipFile(zf, destDir); err != nil { if _, ok := errors.AsType[safepaths.PathTraversalError](err); ok { continue } - return err - } - - if err := extractZipFile(zf, fpath); err != nil { return fmt.Errorf("error extracting %q: %w", zf.Name, err) } } return nil } -func extractZipFile(zf *zip.File, dest safepaths.Absolute) (extractErr error) { +func extractZipFile(zf *zip.File, dest *safepaths.Root) (extractErr error) { zm := zf.Mode() if zm.IsDir() { - extractErr = os.MkdirAll(dest.String(), dirMode) + extractErr = dest.MkdirAll(zf.Name, dirMode) return } @@ -52,12 +44,8 @@ func extractZipFile(zf *zip.File, dest safepaths.Absolute) (extractErr error) { } defer f.Close() - if extractErr = os.MkdirAll(filepath.Dir(dest.String()), dirMode); extractErr != nil { - return - } - var df *os.File - if df, extractErr = os.OpenFile(dest.String(), os.O_WRONLY|os.O_CREATE|os.O_EXCL, getPerm(zm)); extractErr != nil { + if df, extractErr = dest.Create(zf.Name, getPerm(zm), dirMode, false); extractErr != nil { return } diff --git a/internal/zip/zip_test.go b/internal/zip/zip_test.go index 37e83661cf0..6e58f4c4036 100644 --- a/internal/zip/zip_test.go +++ b/internal/zip/zip_test.go @@ -2,26 +2,114 @@ package zip import ( "archive/zip" + "bytes" "os" "path/filepath" "testing" "github.com/cli/cli/v2/internal/safepaths" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func Test_extractZip(t *testing.T) { - tmpDir := t.TempDir() - extractPath, err := safepaths.ParseAbsolute(filepath.Join(tmpDir, "artifact")) - require.NoError(t, err) + tests := []struct { + name string + directories []string + entries []string + setup func(t *testing.T, extractPath, outside string) + wantExtract []string + wantDirs []string + wantErr string + }{ + { + name: "extracts nested file", + entries: []string{"src/main.go"}, + wantExtract: []string{"src/main.go"}, + }, + { + name: "skips lexical escape", + entries: []string{"../outside.txt"}, + }, + { + name: "skips escapes among safe entries", + entries: []string{ + "../outside-before.txt", + "safe-one.txt", + "../../outside-between.txt", + "nested/safe-two.txt", + }, + wantExtract: []string{"safe-one.txt", "nested/safe-two.txt"}, + }, + { + name: "extracts explicit directory", + directories: []string{"nested/"}, + wantDirs: []string{"nested"}, + }, + { + name: "explicit directory rejects planted link", + directories: []string{"nested/"}, + setup: func(t *testing.T, extractPath, outside string) { + t.Helper() + require.NoError(t, os.Symlink(outside, filepath.Join(extractPath, "nested"))) + }, + wantErr: "symbolic link", + }, + } - zipFile, err := zip.OpenReader("./fixtures/myproject.zip") - require.NoError(t, err) - defer zipFile.Close() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var archive bytes.Buffer + writer := zip.NewWriter(&archive) + for _, name := range tt.directories { + header := &zip.FileHeader{Name: name} + header.SetMode(os.ModeDir | 0o755) + _, err := writer.CreateHeader(header) + require.NoError(t, err) + } + for _, name := range tt.entries { + file, err := writer.Create(name) + require.NoError(t, err) + _, err = file.Write([]byte("content")) + require.NoError(t, err) + } + require.NoError(t, writer.Close()) - err = ExtractZip(&zipFile.Reader, extractPath) - require.NoError(t, err) + reader, err := zip.NewReader(bytes.NewReader(archive.Bytes()), int64(archive.Len())) + require.NoError(t, err) + extractPath := t.TempDir() + outside := t.TempDir() + if tt.setup != nil { + tt.setup(t, extractPath, outside) + } + root, err := safepaths.OpenRoot(extractPath) + require.NoError(t, err) + defer root.Close() + + err = ExtractZip(reader, root) + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + assert.Empty(t, mustReadDir(t, outside)) + return + } + require.NoError(t, err) + for _, name := range tt.wantExtract { + content, err := os.ReadFile(filepath.Join(extractPath, filepath.FromSlash(name))) + require.NoError(t, err) + assert.Equal(t, "content", string(content)) + } + for _, name := range tt.wantDirs { + require.DirExists(t, filepath.Join(extractPath, filepath.FromSlash(name))) + } + assert.NoFileExists(t, filepath.Join(extractPath, "..", "outside-before.txt")) + assert.NoFileExists(t, filepath.Join(extractPath, "..", "..", "outside-between.txt")) + }) + } +} - _, err = os.Stat(filepath.Join(extractPath.String(), "src", "main.go")) +func mustReadDir(t *testing.T, path string) []os.DirEntry { + t.Helper() + entries, err := os.ReadDir(path) require.NoError(t, err) + return entries } diff --git a/pkg/cmd/attestation/download/download_test.go b/pkg/cmd/attestation/download/download_test.go index d470c7afbce..f9491f70560 100644 --- a/pkg/cmd/attestation/download/download_test.go +++ b/pkg/cmd/attestation/download/download_test.go @@ -4,6 +4,8 @@ import ( "bytes" "fmt" "net/http" + "os" + "path/filepath" "runtime" "strings" "testing" @@ -325,4 +327,26 @@ func TestRunDownload(t *testing.T) { require.Error(t, err) require.ErrorAs(t, err, &ErrAttestationFileCreation) }) + + t.Run("replaces destination symlink without following it", func(t *testing.T) { + opts := baseOpts + outputDir := t.TempDir() + opts.Store = NewLiveStore(outputDir) + + digested, err := artifact.NewDigestedArtifact(opts.OCIClient, opts.ArtifactPath, opts.DigestAlgorithm) + require.NoError(t, err) + metadataPath := opts.Store.(*LiveStore).createJSONLinesFilePath(digested.DigestWithAlg()) + targetPath := filepath.Join(outputDir, "target.jsonl") + require.NoError(t, os.WriteFile(targetPath, []byte("unchanged"), 0o644)) + require.NoError(t, os.Symlink(filepath.Base(targetPath), metadataPath)) + + require.NoError(t, runDownload(&opts)) + + info, err := os.Lstat(metadataPath) + require.NoError(t, err) + assert.Zero(t, info.Mode()&os.ModeSymlink) + target, err := os.ReadFile(targetPath) + require.NoError(t, err) + assert.Equal(t, "unchanged", string(target)) + }) } diff --git a/pkg/cmd/attestation/download/metadata.go b/pkg/cmd/attestation/download/metadata.go index 4bc353a96fd..4e900f92388 100644 --- a/pkg/cmd/attestation/download/metadata.go +++ b/pkg/cmd/attestation/download/metadata.go @@ -4,10 +4,10 @@ import ( "encoding/json" "errors" "fmt" - "os" "runtime" "strings" + "github.com/cli/cli/v2/internal/safepaths" "github.com/cli/cli/v2/pkg/cmd/attestation/api" ) @@ -22,23 +22,37 @@ type LiveStore struct { } func (s *LiveStore) createJSONLinesFilePath(artifact string) string { + path := s.createJSONLinesFileName(artifact) + if s.outputPath != "" { + return fmt.Sprintf("%s/%s", s.outputPath, path) + } + return path +} + +func (s *LiveStore) createJSONLinesFileName(artifact string) string { if runtime.GOOS == "windows" { // Colons are special characters in Windows and cannot be used in file names. // Replace them with dashes to avoid issues. artifact = strings.ReplaceAll(artifact, ":", "-") } - path := fmt.Sprintf("%s.jsonl", artifact) - if s.outputPath != "" { - return fmt.Sprintf("%s/%s", s.outputPath, path) - } - return path + return fmt.Sprintf("%s.jsonl", artifact) } func (s *LiveStore) createMetadataFile(artifactDigest string, attestationsResp []*api.Attestation) (string, error) { metadataFilePath := s.createJSONLinesFilePath(artifactDigest) + outputPath := s.outputPath + if outputPath == "" { + outputPath = "." + } + + root, err := safepaths.OpenRootDir(outputPath, 0o755) + if err != nil { + return "", errors.Join(ErrAttestationFileCreation, fmt.Errorf("failed to open output directory: %v", err)) + } + defer root.Close() - f, err := os.Create(metadataFilePath) + f, err := root.Create(s.createJSONLinesFileName(artifactDigest), 0o666, 0o755, true) if err != nil { return "", errors.Join(ErrAttestationFileCreation, fmt.Errorf("failed to create file: %v", err)) } diff --git a/pkg/cmd/attestation/download/metadata_mode_unix_test.go b/pkg/cmd/attestation/download/metadata_mode_unix_test.go new file mode 100644 index 00000000000..a43a63dea5e --- /dev/null +++ b/pkg/cmd/attestation/download/metadata_mode_unix_test.go @@ -0,0 +1,32 @@ +//go:build unix + +package download + +import ( + "os" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCreateMetadataFileMode(t *testing.T) { + oldUmask := syscall.Umask(0) + defer syscall.Umask(oldUmask) + + store := NewLiveStore(t.TempDir()) + path, err := store.createMetadataFile("sha256:value", nil) + require.NoError(t, err) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o666), info.Mode().Perm()) + + require.NoError(t, os.Chmod(path, 0o600)) + _, err = store.createMetadataFile("sha256:value", nil) + require.NoError(t, err) + info, err = os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) +} diff --git a/pkg/cmd/attestation/verification/extensions.go b/pkg/cmd/attestation/verification/extensions.go index 3ac9ac0a02b..2708e1f4ae2 100644 --- a/pkg/cmd/attestation/verification/extensions.go +++ b/pkg/cmd/attestation/verification/extensions.go @@ -65,7 +65,8 @@ func verifyCertExtensions(given, expected certificate.Summary) error { if expected.SourceRepositoryDigest != "" && !strings.EqualFold(expected.SourceRepositoryDigest, given.SourceRepositoryDigest) { return fmt.Errorf("expected SourceRepositoryDigest to be %s, got %s", expected.SourceRepositoryDigest, given.SourceRepositoryDigest) } - if expected.SourceRepositoryRef != "" && !strings.EqualFold(expected.SourceRepositoryRef, given.SourceRepositoryRef) { + // SourceRepositoryRef must be a case-sensitive comparison + if expected.SourceRepositoryRef != "" && expected.SourceRepositoryRef != given.SourceRepositoryRef { return fmt.Errorf("expected SourceRepositoryRef to be %s, got %s", expected.SourceRepositoryRef, given.SourceRepositoryRef) } diff --git a/pkg/cmd/attestation/verification/extensions_test.go b/pkg/cmd/attestation/verification/extensions_test.go index de00abcef04..af2e50c7f0d 100644 --- a/pkg/cmd/attestation/verification/extensions_test.go +++ b/pkg/cmd/attestation/verification/extensions_test.go @@ -16,6 +16,7 @@ func createSampleResult() *AttestationProcessingResult { SourceRepositoryOwnerURI: "https://github.com/owner", SourceRepositoryURI: "https://github.com/owner/repo", Issuer: "https://token.actions.githubusercontent.com", + SourceRepositoryRef: "refs/heads/main", }, }, }, @@ -28,7 +29,9 @@ func TestVerifyCertExtensions(t *testing.T) { certSummary := certificate.Summary{ SourceRepositoryOwnerURI: "https://github.com/owner", SourceRepositoryURI: "https://github.com/owner/repo", - Issuer: GitHubOIDCIssuer} + Issuer: GitHubOIDCIssuer, + SourceRepositoryRef: "refs/heads/main", + } c := EnforcementCriteria{ Certificate: certSummary, @@ -86,10 +89,18 @@ func TestVerifyCertExtensions(t *testing.T) { }) t.Run("with partial OIDCIssuer match", func(t *testing.T) { - expectedResults := results + expectedResults := []*AttestationProcessingResult{createSampleResult()} expectedResults[0].VerificationResult.Signature.Certificate.Extensions.Issuer = "https://token.actions.githubusercontent.com/foo-bar" verified, err := VerifyCertExtensions(expectedResults, c) require.ErrorContains(t, err, "expected Issuer to be https://token.actions.githubusercontent.com, got https://token.actions.githubusercontent.com/foo-bar -- if you have a custom OIDC issuer") require.Nil(t, verified) }) + + t.Run("with wrong case-sensitive SourceRepositoryRef", func(t *testing.T) { + expectedResults := []*AttestationProcessingResult{createSampleResult()} + expectedResults[0].VerificationResult.Signature.Certificate.Extensions.SourceRepositoryRef = "refs/heads/MAIN" + verified, err := VerifyCertExtensions(expectedResults, c) + require.ErrorContains(t, err, "expected SourceRepositoryRef to be refs/heads/main, got refs/heads/MAIN") + require.Nil(t, verified) + }) } diff --git a/pkg/cmd/attestation/verify/policy.go b/pkg/cmd/attestation/verify/policy.go index 9f15653686e..5829b39c6b1 100644 --- a/pkg/cmd/attestation/verify/policy.go +++ b/pkg/cmd/attestation/verify/policy.go @@ -15,6 +15,42 @@ import ( const hostRegex = `^[a-zA-Z0-9-]+\.[a-zA-Z0-9-]+.*$` +// signerWorkflowRefPattern matches the "@" that terminates the +// SubjectAlternativeName of a certificate issued to a GitHub Actions workflow. +// +// The separator cannot be located by splitting on "@", because both sides may contain +// one. A git ref may contain "@", as in the refs/tags/pkg@1.2.3 convention used by +// JavaScript monorepos, and so may a workflow file name. What separates them is shape: +// a ref is either a "refs/"-prefixed path or a bare object ID, and a workflow file name +// can be neither. It cannot contain "/", since Actions only loads workflows stored +// directly in .github/workflows, and it cannot be a bare object ID, since it must carry +// a .yml or .yaml extension. +const signerWorkflowRefPattern = `@(refs/.*|[0-9a-fA-F]+)` + +// signerWorkflowHasRef reports whether a --signer-workflow value already pins a ref. +var signerWorkflowHasRef = regexp.MustCompile(signerWorkflowRefPattern + `$`) + +// signerWorkflowSANPattern builds the SAN pattern enforced for --signer-workflow. +// +// sigstore-go matches SAN patterns with regexp.MatchString, which is unanchored, so a +// pattern anchored only at the start is a prefix match: a workflow named +// release.yml.attacker.yml satisfies a pin on release.yml. Anchoring the end makes the +// pin an identity match. +// +// The documented flag format carries no ref, and such a value has to keep matching a +// certificate from any ref, so the ref is optional. Once the value pins a ref of its +// own it must match the end of the SAN exactly. Leaving the ref optional there would +// let a second ref follow the pinned one, and since a branch may be named +// "main@refs/heads/attacker", a pin on refs/heads/main would still admit a certificate +// built from a different branch. +func signerWorkflowSANPattern(workflowURL string) string { + pattern := "^" + regexp.QuoteMeta(workflowURL) + if signerWorkflowHasRef.MatchString(workflowURL) { + return pattern + "$" + } + return pattern + "(" + signerWorkflowRefPattern + ")?$" +} + func expandToGitHubURL(tenant, ownerOrRepo string) string { if tenant == "" { return fmt.Sprintf("https://github.com/%s", ownerOrRepo) @@ -155,7 +191,7 @@ func validateSignerWorkflow(hostname, signerWorkflow string) (string, error) { } if match { - return "^" + regexp.QuoteMeta(fmt.Sprintf("https://%s", signerWorkflow)), nil + return signerWorkflowSANPattern(fmt.Sprintf("https://%s", signerWorkflow)), nil } // if the provided workflow did not match the expect format @@ -164,5 +200,5 @@ func validateSignerWorkflow(hostname, signerWorkflow string) (string, error) { return "", errors.New("unknown signer workflow host") } - return "^" + regexp.QuoteMeta(fmt.Sprintf("https://%s/%s", hostname, signerWorkflow)), nil + return signerWorkflowSANPattern(fmt.Sprintf("https://%s/%s", hostname, signerWorkflow)), nil } diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index ae6e022f05f..ccabf4bd0f8 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -6,6 +6,8 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/verification" + "github.com/sigstore/sigstore-go/pkg/fulcio/certificate" + "github.com/sigstore/sigstore-go/pkg/verify" "github.com/stretchr/testify/require" ) @@ -71,7 +73,7 @@ func TestNewEnforcementCriteria(t *testing.T) { c, err := newEnforcementCriteria(opts) require.NoError(t, err) - require.Equal(t, `^https://github\.com/foo/bar/\.github/workflows/attest\.yml`, c.SANRegex) + require.Equal(t, `^https://github\.com/foo/bar/\.github/workflows/attest\.yml(@(refs/.*|[0-9a-fA-F]+))?$`, c.SANRegex) require.Zero(t, c.SAN) }) @@ -297,25 +299,25 @@ func TestValidateSignerWorkflow(t *testing.T) { { name: "workflow with default host", providedSignerWorkflow: "github/artifact-attestations-workflows/.github/workflows/attest.yml", - expectedWorkflowRegex: `^https://github\.com/github/artifact-attestations-workflows/\.github/workflows/attest\.yml`, + expectedWorkflowRegex: `^https://github\.com/github/artifact-attestations-workflows/\.github/workflows/attest\.yml(@(refs/.*|[0-9a-fA-F]+))?$`, host: "github.com", }, { name: "workflow with workflow URL included", providedSignerWorkflow: "github.com/github/artifact-attestations-workflows/.github/workflows/attest.yml", - expectedWorkflowRegex: `^https://github\.com/github/artifact-attestations-workflows/\.github/workflows/attest\.yml`, + expectedWorkflowRegex: `^https://github\.com/github/artifact-attestations-workflows/\.github/workflows/attest\.yml(@(refs/.*|[0-9a-fA-F]+))?$`, host: "github.com", }, { name: "workflow with GH_HOST set", providedSignerWorkflow: "github/artifact-attestations-workflows/.github/workflows/attest.yml", - expectedWorkflowRegex: `^https://myhost\.github\.com/github/artifact-attestations-workflows/\.github/workflows/attest\.yml`, + expectedWorkflowRegex: `^https://myhost\.github\.com/github/artifact-attestations-workflows/\.github/workflows/attest\.yml(@(refs/.*|[0-9a-fA-F]+))?$`, host: "myhost.github.com", }, { name: "workflow with authenticated host", providedSignerWorkflow: "github/artifact-attestations-workflows/.github/workflows/attest.yml", - expectedWorkflowRegex: `^https://authedhost\.github\.com/github/artifact-attestations-workflows/\.github/workflows/attest\.yml`, + expectedWorkflowRegex: `^https://authedhost\.github\.com/github/artifact-attestations-workflows/\.github/workflows/attest\.yml(@(refs/.*|[0-9a-fA-F]+))?$`, host: "authedhost.github.com", }, } @@ -334,3 +336,184 @@ func TestValidateSignerWorkflow(t *testing.T) { } } } + +// TestSignerWorkflowSANMatching drives the pattern built by validateSignerWorkflow +// through the sigstore-go matcher that consumes it, because that matcher applies the +// pattern with regexp.MatchString and so does not anchor it on the caller's behalf. +func TestSignerWorkflowSANMatching(t *testing.T) { + const pinnedWorkflow = "owner/builder/.github/workflows/release.yml" + const workflowDir = "https://github.com/owner/builder/.github/workflows/" + + testcases := []struct { + name string + san string + expectMatch bool + }{ + { + name: "pinned workflow at a branch ref", + san: workflowDir + "release.yml@refs/heads/main", + expectMatch: true, + }, + { + name: "pinned workflow at a commit SHA", + san: workflowDir + "release.yml@09b495c3f12c7881b3cc17209a327792065c1a1d", + expectMatch: true, + }, + { + name: "pinned workflow at a tag containing @", + san: workflowDir + "release.yml@refs/tags/pkg@1.2.3", + expectMatch: true, + }, + { + name: "pinned workflow at a branch containing @", + san: workflowDir + "release.yml@refs/heads/feature@v2", + expectMatch: true, + }, + { + name: "pinned workflow with no ref", + san: workflowDir + "release.yml", + expectMatch: true, + }, + { + name: "workflow whose file name extends the pinned name", + san: workflowDir + "release.yml.attacker.yml@refs/heads/attacker", + expectMatch: false, + }, + { + name: "workflow whose file name contains the ref separator", + san: workflowDir + "release.yml@attacker.yml@refs/heads/attacker", + expectMatch: false, + }, + { + name: "different workflow in the pinned repository", + san: workflowDir + "other.yml@refs/heads/main", + expectMatch: false, + }, + { + name: "pinned workflow in a lookalike repository", + san: "https://github.com/owner/builder-attacker/.github/workflows/release.yml@refs/heads/main", + expectMatch: false, + }, + } + + sanRegex, err := validateSignerWorkflow("github.com", pinnedWorkflow) + require.NoError(t, err) + + matcher, err := verify.NewSANMatcher("", sanRegex) + require.NoError(t, err) + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + err := matcher.Verify(certificate.Summary{SubjectAlternativeName: tc.san}) + if tc.expectMatch { + require.NoError(t, err) + } else { + require.Error(t, err) + } + }) + } +} + +// TestSignerWorkflowSANMatchingObservedCertificate uses the SubjectAlternativeName from +// a certificate Fulcio actually issued for a workflow stored at +// .github/workflows/provenance-image.yml@evil.yml. Actions loads a workflow file whose +// name contains "@", and neither the OIDC claim nor Fulcio escapes it, so the separator +// really can appear more than once in a SAN. +func TestSignerWorkflowSANMatchingObservedCertificate(t *testing.T) { + const observedSAN = "https://github.com/bdehamer/attest-demo/.github/workflows/provenance-image.yml@evil.yml@refs/heads/main" + + sanRegex, err := validateSignerWorkflow("github.com", "bdehamer/attest-demo/.github/workflows/provenance-image.yml") + require.NoError(t, err) + + matcher, err := verify.NewSANMatcher("", sanRegex) + require.NoError(t, err) + + require.Error(t, matcher.Verify(certificate.Summary{SubjectAlternativeName: observedSAN})) +} + +// TestSignerWorkflowSANMatchingPinnedRef covers a --signer-workflow value that pins a +// ref as well as a workflow. Such a value has to match the end of the SAN exactly, +// because a git ref may itself be named so that it begins with the pinned ref and is +// followed by something ref-shaped. +func TestSignerWorkflowSANMatchingPinnedRef(t *testing.T) { + const pinnedWorkflow = "owner/builder/.github/workflows/release.yml@refs/heads/main" + const workflowURL = "https://github.com/owner/builder/.github/workflows/release.yml" + + testcases := []struct { + name string + san string + expectMatch bool + }{ + { + name: "the pinned ref", + san: workflowURL + "@refs/heads/main", + expectMatch: true, + }, + { + name: "a branch whose name extends the pinned ref", + san: workflowURL + "@refs/heads/mainattacker", + expectMatch: false, + }, + { + name: "a branch named so that a second ref follows the pinned ref", + san: workflowURL + "@refs/heads/main@refs/heads/attacker", + expectMatch: false, + }, + { + name: "a branch named so that an object ID follows the pinned ref", + san: workflowURL + "@refs/heads/main@09b495c3f12c7881b3cc17209a327792065c1a1d", + expectMatch: false, + }, + { + name: "a different ref", + san: workflowURL + "@refs/heads/attacker", + expectMatch: false, + }, + } + + sanRegex, err := validateSignerWorkflow("github.com", pinnedWorkflow) + require.NoError(t, err) + + matcher, err := verify.NewSANMatcher("", sanRegex) + require.NoError(t, err) + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + err := matcher.Verify(certificate.Summary{SubjectAlternativeName: tc.san}) + if tc.expectMatch { + require.NoError(t, err) + } else { + require.Error(t, err) + } + }) + } +} + +// TestSignerWorkflowSANMatchingObservedNestedRef uses the job_workflow_ref claim +// observed from a workflow run on a branch named "main@refs/heads/evil". Actions runs a +// workflow from such a branch, so a pin on refs/heads/main must not admit it. +func TestSignerWorkflowSANMatchingObservedNestedRef(t *testing.T) { + const observedSAN = "https://github.com/bdehamer/attest-demo/.github/workflows/control-probe.yml@refs/heads/main@refs/heads/evil" + + sanRegex, err := validateSignerWorkflow("github.com", "bdehamer/attest-demo/.github/workflows/control-probe.yml@refs/heads/main") + require.NoError(t, err) + + matcher, err := verify.NewSANMatcher("", sanRegex) + require.NoError(t, err) + + require.Error(t, matcher.Verify(certificate.Summary{SubjectAlternativeName: observedSAN})) +} + +// TestSignerWorkflowSANMatchingRepositoryValue covers a --signer-workflow value that +// names a repository instead of a workflow. The value is still matched as a complete +// identity, so it cannot match a repository whose name merely starts with it. +func TestSignerWorkflowSANMatchingRepositoryValue(t *testing.T) { + sanRegex, err := validateSignerWorkflow("github.com", "owner/builder") + require.NoError(t, err) + + matcher, err := verify.NewSANMatcher("", sanRegex) + require.NoError(t, err) + + san := "https://github.com/owner/builder-attacker/.github/workflows/release.yml@refs/heads/main" + require.Error(t, matcher.Verify(certificate.Summary{SubjectAlternativeName: san})) +} diff --git a/pkg/cmd/copilot/copilot.go b/pkg/cmd/copilot/copilot.go index e3c610de3c8..09cfd64293b 100644 --- a/pkg/cmd/copilot/copilot.go +++ b/pkg/cmd/copilot/copilot.go @@ -382,10 +382,11 @@ func extractZip(path, destDir string) error { } defer zipReader.Close() - absPath, err := safepaths.ParseAbsolute(destDir) + root, err := safepaths.OpenRoot(destDir) if err != nil { return err } + defer root.Close() // As of the time of writing, ghzip.ExtractZip will safely skip files that // would result in path traversal. This is an issue for our use-case because @@ -393,14 +394,13 @@ func extractZip(path, destDir string) error { // To avoid breaking the shared ghzip.ExtractZip code that expects unsafe // paths to be ignored and no error produced, we pre-validate here, // producing an error if any such file is found. - for _, f := range zipReader.File { - _, err := absPath.Join(f.Name) - if err != nil { + for _, file := range zipReader.File { + if err := root.Validate(file.Name); err != nil { return err } } - if err := ghzip.ExtractZip(&zipReader.Reader, absPath); err != nil { + if err := ghzip.ExtractZip(&zipReader.Reader, root); err != nil { return err } @@ -417,10 +417,11 @@ func extractTarGz(r io.Reader, destDir string) error { } defer gzr.Close() - absDestDirPath, err := safepaths.ParseAbsolute(destDir) + root, err := safepaths.OpenRoot(destDir) if err != nil { return err } + defer root.Close() tr := tar.NewReader(gzr) for { @@ -431,38 +432,15 @@ func extractTarGz(r io.Reader, destDir string) error { if err != nil { return fmt.Errorf("failed to read tar: %w", err) } - - absFilePath, err := absDestDirPath.Join(header.Name) - if err != nil { + if err := safepaths.ValidateChild(header.Name); err != nil { return err } - target := absFilePath.String() if header.Typeflag == tar.TypeReg { - if err := os.MkdirAll(filepath.Dir(target), 0755); err != nil { - return fmt.Errorf("failed to create parent directory: %w", err) + if err := root.CopyFile(header.Name, tr, os.FileMode(header.Mode)&0o777, 0o755, true); err != nil { + return fmt.Errorf("failed to extract file: %w", err) } - if err := extractFile(target, os.FileMode(header.Mode)&0777, tr); err != nil { - return err - } - } - } - return nil -} - -// extractFile creates a file at target with the given mode and copies content from r. -func extractFile(target string, mode os.FileMode, r io.Reader) (err error) { - out, err := os.OpenFile(target, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode) - if err != nil { - return fmt.Errorf("failed to create file: %w", err) - } - defer func() { - if cerr := out.Close(); err == nil && cerr != nil { - err = fmt.Errorf("failed to close file: %w", cerr) } - }() - if _, err := io.Copy(out, r); err != nil { - return fmt.Errorf("failed to write file: %w", err) } return nil } diff --git a/pkg/cmd/copilot/copilot_test.go b/pkg/cmd/copilot/copilot_test.go index 570ab6c45de..83c691124b8 100644 --- a/pkg/cmd/copilot/copilot_test.go +++ b/pkg/cmd/copilot/copilot_test.go @@ -271,6 +271,76 @@ func TestExtractTarGz(t *testing.T) { err := extractTarGz(bytes.NewReader([]byte("not valid gzip")), destDir) require.Error(t, err, "expected error for invalid gzip, got nil") }) + + t.Run("rejects invalid nonregular entry before extraction", func(t *testing.T) { + var archive bytes.Buffer + gzipWriter := gzip.NewWriter(&archive) + tarWriter := tar.NewWriter(gzipWriter) + require.NoError(t, tarWriter.WriteHeader(&tar.Header{ + Name: "../outside", + Typeflag: tar.TypeDir, + Mode: 0o755, + })) + content := []byte("content") + require.NoError(t, tarWriter.WriteHeader(&tar.Header{ + Name: "copilot", + Mode: 0o755, + Size: int64(len(content)), + })) + _, err := tarWriter.Write(content) + require.NoError(t, err) + require.NoError(t, tarWriter.Close()) + require.NoError(t, gzipWriter.Close()) + + destDir := t.TempDir() + err = extractTarGz(bytes.NewReader(archive.Bytes()), destDir) + require.ErrorContains(t, err, "not a local child path") + assert.NoFileExists(t, filepath.Join(destDir, "copilot")) + }) + + t.Run("rejects ancestor symlink", func(t *testing.T) { + content := []byte("nested content") + archive := createTarGzBuffer(t, map[string][]byte{ + "subdir/file.txt": content, + }) + destDir := t.TempDir() + outside := t.TempDir() + require.NoError(t, os.Symlink(outside, filepath.Join(destDir, "subdir"))) + + err := extractTarGz(bytes.NewReader(archive), destDir) + require.ErrorContains(t, err, "symbolic link") + assert.NoFileExists(t, filepath.Join(outside, "file.txt")) + }) + + t.Run("replaces dangling final symlink", func(t *testing.T) { + archive := createTarGzBuffer(t, map[string][]byte{ + "copilot": []byte("content"), + }) + destDir := t.TempDir() + missing := filepath.Join(destDir, "missing") + require.NoError(t, os.Symlink("missing", filepath.Join(destDir, "copilot"))) + + err := extractTarGz(bytes.NewReader(archive), destDir) + require.NoError(t, err) + assert.NoFileExists(t, missing) + info, err := os.Lstat(filepath.Join(destDir, "copilot")) + require.NoError(t, err) + assert.Zero(t, info.Mode()&os.ModeSymlink) + }) + + t.Run("replaces leftover file on retry", func(t *testing.T) { + archive := createTarGzBuffer(t, map[string][]byte{ + "copilot": []byte("complete"), + }) + destDir := t.TempDir() + target := filepath.Join(destDir, "copilot") + require.NoError(t, os.WriteFile(target, []byte("partial"), 0o644)) + + require.NoError(t, extractTarGz(bytes.NewReader(archive), destDir)) + content, err := os.ReadFile(target) + require.NoError(t, err) + assert.Equal(t, "complete", string(content)) + }) } func TestExtractZip(t *testing.T) { @@ -333,6 +403,46 @@ func TestExtractZip(t *testing.T) { err := extractZip(zipPath, destDir) require.Error(t, err, "expected error for path traversal, got nil") }) + + t.Run("validates all entries before extraction", func(t *testing.T) { + zipDir := t.TempDir() + zipPath := filepath.Join(zipDir, "archive.zip") + + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + valid, err := zw.Create("copilot.exe") + require.NoError(t, err) + _, err = valid.Write([]byte("content")) + require.NoError(t, err) + invalid, err := zw.Create("../outside.txt") + require.NoError(t, err) + _, err = invalid.Write([]byte("content")) + require.NoError(t, err) + require.NoError(t, zw.Close()) + require.NoError(t, os.WriteFile(zipPath, buf.Bytes(), 0o755)) + + destDir := t.TempDir() + err = extractZip(zipPath, destDir) + require.ErrorContains(t, err, "not a local child path") + assert.NoFileExists(t, filepath.Join(destDir, "copilot.exe")) + }) + + t.Run("rejects ancestor symlink", func(t *testing.T) { + zipDir := t.TempDir() + zipPath := filepath.Join(zipDir, "archive.zip") + archive := createZipBuffer(t, map[string][]byte{ + "subdir/file.txt": []byte("content"), + }) + require.NoError(t, os.WriteFile(zipPath, archive, 0o755)) + + destDir := t.TempDir() + outside := t.TempDir() + require.NoError(t, os.Symlink(outside, filepath.Join(destDir, "subdir"))) + + err := extractZip(zipPath, destDir) + require.ErrorContains(t, err, "symbolic link") + assert.NoFileExists(t, filepath.Join(outside, "file.txt")) + }) } func TestFetchExpectedChecksum(t *testing.T) { diff --git a/pkg/cmd/release/download/download.go b/pkg/cmd/release/download/download.go index 22309094e87..0669111cf36 100644 --- a/pkg/cmd/release/download/download.go +++ b/pkg/cmd/release/download/download.go @@ -13,10 +13,12 @@ import ( "runtime" "slices" "strings" + "sync" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/safepaths" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" @@ -224,7 +226,7 @@ func downloadRun(opts *DownloadOptions) error { // is made per copy below. Writing to a file keeps the raw bytes. opts.IO.SetContentSanitization(false) - dest := destinationWriter{ + dest := &destinationWriter{ file: opts.OutputFile, dir: opts.Destination, skipExisting: opts.SkipExisting, @@ -233,6 +235,7 @@ func downloadRun(opts *DownloadOptions) error { allowEscapes: opts.AllowEscapeSequences, isTTY: opts.IO.IsStdoutTTY(), } + defer dest.Close() targets := make([]downloadTarget, len(toDownload)) for i, a := range toDownload { @@ -242,7 +245,7 @@ func downloadRun(opts *DownloadOptions) error { } } - return downloadAssets(&dest, httpClient, targets, opts.Concurrency, isArchive, opts.IO) + return downloadAssets(dest, httpClient, targets, opts.Concurrency, isArchive, opts.IO) } func matchAny(patterns []string, name string) bool { @@ -380,9 +383,29 @@ type destinationWriter struct { stdout io.Writer allowEscapes bool isTTY bool + root *safepaths.Root + rootOnce sync.Once + rootErr error } -func (w destinationWriter) makePath(name string) string { +func (w *destinationWriter) ensureRoot() error { + if w.file != "" { + return nil + } + w.rootOnce.Do(func() { + w.root, w.rootErr = safepaths.OpenRootDir(w.dir, 0o755) + }) + return w.rootErr +} + +func (w *destinationWriter) Close() error { + if w.root == nil { + return nil + } + return w.root.Close() +} + +func (w *destinationWriter) makePath(name string) string { if w.file == "" { return filepath.Join(w.dir, name) } @@ -390,7 +413,7 @@ func (w destinationWriter) makePath(name string) string { } // Check returns an error if a file already exists at destination -func (w destinationWriter) Check(name string) error { +func (w *destinationWriter) Check(name string) error { if name == "" { // skip check as file name will only be known after the API request return nil @@ -400,11 +423,22 @@ func (w destinationWriter) Check(name string) error { // writing to stdout should always proceed return nil } + if err := w.validateName(name); err != nil { + return err + } return w.check(fp) } -func (w destinationWriter) check(fp string) error { - if _, err := os.Stat(fp); err == nil { +func (w *destinationWriter) validateName(name string) error { + if w.file != "" { + return nil + } + return safepaths.ValidateChild(name) +} + +func (w *destinationWriter) check(fp string) error { + _, err := os.Lstat(fp) + if err == nil { if w.skipExisting { return errSkipped } @@ -414,12 +448,14 @@ func (w destinationWriter) check(fp string) error { fp, ) } + } else if !os.IsNotExist(err) { + return err } return nil } // Copy writes the data from r into a file specified by name. -func (w destinationWriter) Copy(name string, r io.Reader) (copyErr error) { +func (w *destinationWriter) Copy(name string, r io.Reader) (copyErr error) { fp := w.makePath(name) if fp == "-" { if w.allowEscapes { @@ -434,18 +470,34 @@ func (w destinationWriter) Copy(name string, r io.Reader) (copyErr error) { } return } + if copyErr = w.validateName(name); copyErr != nil { + return + } if copyErr = w.check(fp); copyErr != nil { return } - if dir := filepath.Dir(fp); dir != "." { - if copyErr = os.MkdirAll(dir, 0755); copyErr != nil { + var f *os.File + if w.file == "" { + if copyErr = w.ensureRoot(); copyErr != nil { return } + f, copyErr = w.root.Create(name, 0o644, 0o755, w.overwrite) + } else { + f, copyErr = safepaths.OpenFile(fp, 0o644, 0o755, w.overwrite) } - - var f *os.File - if f, copyErr = os.OpenFile(fp, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644); copyErr != nil { + if copyErr != nil { + if os.IsExist(copyErr) { + if w.skipExisting { + return errSkipped + } + if !w.overwrite { + return fmt.Errorf( + "%s already exists (use `--clobber` to overwrite file or `--skip-existing` to skip file)", + fp, + ) + } + } return } diff --git a/pkg/cmd/release/download/download_test.go b/pkg/cmd/release/download/download_test.go index cc5ac482b76..8b440c08ca0 100644 --- a/pkg/cmd/release/download/download_test.go +++ b/pkg/cmd/release/download/download_test.go @@ -178,15 +178,22 @@ func Test_NewCmdDownload(t *testing.T) { } func Test_downloadRun(t *testing.T) { + var clobberTarget string + var ancestorTarget string + var directoryTarget string + tests := []struct { - name string - isTTY bool - opts DownloadOptions - httpStubs func(*httpmock.Registry) - wantErr string - wantStdout string - wantStderr string - wantFiles []string + name string + isTTY bool + opts DownloadOptions + httpStubs func(*httpmock.Registry) + setup func(t *testing.T) + verify func(t *testing.T) + wantErr string + wantErrContains string + wantStdout string + wantStderr string + wantFiles []string }{ { name: "download all assets", @@ -222,6 +229,24 @@ func Test_downloadRun(t *testing.T) { "windows-64bit.zip", }, }, + { + name: "empty destination uses current directory", + isTTY: true, + opts: DownloadOptions{ + TagName: "v1.2.3", + Destination: "", + Concurrency: 1, + }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "asset.bin", "size": 3, "url": "https://api.github.com/assets/1234" } + ] + }`) + reg.Register(httpmock.REST("GET", "assets/1234"), httpmock.StringResponse("new")) + }, + wantFiles: []string{"asset.bin"}, + }, { name: "downloads published release when the draft lookup is unauthorized", isTTY: true, @@ -286,6 +311,149 @@ func Test_downloadRun(t *testing.T) { "tmp/assets/windows-64bit.zip", }, }, + { + name: "HTTP failure does not create destination directory", + isTTY: true, + opts: DownloadOptions{ + TagName: "v1.2.3", + Destination: "missing", + Concurrency: 1, + }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "asset.bin", "size": 3, "url": "https://api.github.com/assets/1234" } + ] + }`) + reg.Register(httpmock.REST("GET", "assets/1234"), httpmock.StatusStringResponse(500, "server error")) + }, + verify: func(t *testing.T) { + t.Helper() + assert.NoDirExists(t, "missing") + }, + wantErrContains: "HTTP 500", + }, + { + name: "clobber replaces final symlink", + isTTY: true, + opts: DownloadOptions{ + TagName: "v1.2.3", + Destination: ".", + Concurrency: 1, + OverwriteExisting: true, + }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "asset.bin", "size": 3, "url": "https://api.github.com/assets/1234" } + ] + }`) + reg.Register(httpmock.REST("GET", "assets/1234"), httpmock.StringResponse("new")) + }, + setup: func(t *testing.T) { + t.Helper() + clobberTarget = "target.bin" + require.NoError(t, os.WriteFile(clobberTarget, []byte("old"), 0o644)) + require.NoError(t, os.Symlink(clobberTarget, "asset.bin")) + }, + verify: func(t *testing.T) { + t.Helper() + info, err := os.Lstat("asset.bin") + require.NoError(t, err) + assert.Zero(t, info.Mode()&os.ModeSymlink) + content, err := os.ReadFile("asset.bin") + require.NoError(t, err) + assert.Equal(t, "new", string(content)) + content, err = os.ReadFile(clobberTarget) + require.NoError(t, err) + assert.Equal(t, "old", string(content)) + }, + wantFiles: []string{"asset.bin", "target.bin"}, + }, + { + name: "clobber refuses symlink to directory", + isTTY: true, + opts: DownloadOptions{ + TagName: "v1.2.3", + Destination: ".", + Concurrency: 1, + OverwriteExisting: true, + }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "asset.bin", "size": 3, "url": "https://api.github.com/assets/1234" } + ] + }`) + reg.Register(httpmock.REST("GET", "assets/1234"), httpmock.StringResponse("new")) + }, + setup: func(t *testing.T) { + t.Helper() + directoryTarget = "directory-target" + require.NoError(t, os.Mkdir(directoryTarget, 0o755)) + require.NoError(t, os.Symlink(directoryTarget, "asset.bin")) + }, + verify: func(t *testing.T) { + t.Helper() + info, err := os.Lstat("asset.bin") + require.NoError(t, err) + assert.NotZero(t, info.Mode()&os.ModeSymlink) + entries, err := os.ReadDir(directoryTarget) + require.NoError(t, err) + assert.Empty(t, entries) + }, + wantErrContains: "is a directory", + wantFiles: []string{"asset.bin"}, + }, + { + name: "rejects ancestor symlink", + isTTY: true, + opts: DownloadOptions{ + TagName: "v1.2.3", + Destination: ".", + Concurrency: 1, + }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "nested/asset.bin", "size": 3, "url": "https://api.github.com/assets/1234" } + ] + }`) + reg.Register(httpmock.REST("GET", "assets/1234"), httpmock.StringResponse("new")) + }, + setup: func(t *testing.T) { + t.Helper() + ancestorTarget = t.TempDir() + require.NoError(t, os.Symlink(ancestorTarget, "nested")) + }, + verify: func(t *testing.T) { + t.Helper() + assert.NoFileExists(t, filepath.Join(ancestorTarget, "asset.bin")) + }, + wantErr: `refusing to traverse symbolic link "nested"`, + wantFiles: []string{"nested"}, + }, + { + name: "rejects nonlocal asset name before request", + isTTY: true, + opts: DownloadOptions{ + TagName: "v1.2.3", + Destination: ".", + Concurrency: 1, + }, + httpStubs: func(reg *httpmock.Registry) { + shared.StubFetchRelease(t, reg, "OWNER", "REPO", "v1.2.3", `{ + "assets": [ + { "name": "/asset.bin", "size": 3, "url": "https://api.github.com/assets/1234" } + ] + }`) + }, + verify: func(t *testing.T) { + t.Helper() + assert.NoFileExists(t, "asset.bin") + }, + wantErr: `path "/asset.bin" is not a local child path`, + }, { name: "no match for pattern", isTTY: true, @@ -680,6 +848,9 @@ func Test_downloadRun(t *testing.T) { t.Run(tt.name, func(t *testing.T) { tempDir := t.TempDir() t.Chdir(tempDir) + if tt.setup != nil { + tt.setup(t) + } ios, _, stdout, stderr := iostreams.Test() ios.SetStdoutTTY(tt.isTTY) @@ -702,8 +873,17 @@ func Test_downloadRun(t *testing.T) { } err := downloadRun(&tt.opts) - if tt.wantErr != "" { - assert.EqualError(t, err, tt.wantErr) + if tt.wantErr != "" || tt.wantErrContains != "" { + require.Error(t, err) + if tt.wantErr != "" { + assert.EqualError(t, err, tt.wantErr) + } + if tt.wantErrContains != "" { + assert.ErrorContains(t, err, tt.wantErrContains) + } + if tt.verify != nil { + tt.verify(t) + } return } assert.NoError(t, err) @@ -727,6 +907,9 @@ func Test_downloadRun(t *testing.T) { downloadedFiles, err := listFiles(".") assert.NoError(t, err) assert.Equal(t, tt.wantFiles, downloadedFiles) + if tt.verify != nil { + tt.verify(t) + } }) } } @@ -838,11 +1021,17 @@ func Test_downloadRun_cloberAndSkip(t *testing.T) { _, err = f1.WriteString(oldAssetContents) assert.NoError(t, err) f1.Close() + require.NoError(t, os.Chmod(file, 0o600)) f2, err := os.Create(archive) assert.NoError(t, err) _, err = f2.WriteString(oldZipballContents) assert.NoError(t, err) f2.Close() + require.NoError(t, os.Chmod(archive, 0o600)) + fileBefore, err := os.Stat(file) + require.NoError(t, err) + archiveBefore, err := os.Stat(archive) + require.NoError(t, err) tt.opts.Destination = dest @@ -883,6 +1072,8 @@ func Test_downloadRun_cloberAndSkip(t *testing.T) { assert.NoError(t, err) assert.Equal(t, tt.wantFileSize, fs.Size()) assert.Equal(t, tt.wantArchiveSize, as.Size()) + assert.Equal(t, fileBefore.Mode().Perm(), fs.Mode().Perm()) + assert.Equal(t, archiveBefore.Mode().Perm(), as.Mode().Perm()) }) } } @@ -1010,6 +1201,7 @@ func Test_downloadAsset_archiveAvoidsLegacyCodeload(t *testing.T) { tempDir := t.TempDir() dest := destinationWriter{dir: tempDir} + defer dest.Close() err := downloadAsset(&dest, ts.Client(), safeurl.NewImmutableSafeURL(ts.URL+"/asset"), "", true) require.NoError(t, err) diff --git a/pkg/cmd/repo/read-file/read_file.go b/pkg/cmd/repo/read-file/read_file.go index 5940236cf72..3b9e6451cd6 100644 --- a/pkg/cmd/repo/read-file/read_file.go +++ b/pkg/cmd/repo/read-file/read_file.go @@ -11,6 +11,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/safepaths" "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" @@ -250,21 +251,22 @@ func lstat(path string) (*lstatResult, error) { // These indirect the file system operations used when writing output, so tests can // substitute them without touching the real file system. var ( - lstatF = lstat - mkdirAllF = os.MkdirAll - writeFileF = os.WriteFile + lstatF = lstat + writeOutputFile = rootedWriteOutputFile ) // writeToOutput writes file content to a local path and returns the final destination. -// A symlink target is refused, a directory target receives the file under its remote basename, -// and missing parent directories are created. Existing files are only overwritten when clobber is true. +// Symlink output paths are refused. A directory target receives the file under +// its remote basename using the local operating system's path rules, and existing +// regular files are only replaced when clobber is true. func writeToOutput(file *repoFile, output string, clobber bool) (string, error) { dest := output + outputDir := "" // A trailing separator signals the user intends dest to be a directory even if it does not exist yet. asDir := strings.HasSuffix(dest, "/") || strings.HasSuffix(dest, string(os.PathSeparator)) - if lr, err := lstatF(dest); err == nil { + if lr, err := lstatF(trimTrailingPathSeparators(dest)); err == nil { if lr.isSymlink { return "", fmt.Errorf("output path is a symlink") } @@ -275,8 +277,14 @@ func writeToOutput(file *repoFile, output string, clobber bool) (string, error) return "", err } + name := dest if asDir { - dest = filepath.Join(dest, file.Name) + name = filepath.Base(file.Name) + if name == "." || !filepath.IsLocal(name) { + return "", fmt.Errorf("invalid output filename %q", file.Name) + } + outputDir = dest + dest = filepath.Join(dest, name) } if lr, err := lstatF(dest); err == nil { @@ -290,15 +298,36 @@ func writeToOutput(file *repoFile, output string, clobber bool) (string, error) return "", err } - if dir := filepath.Dir(dest); dir != "" && dir != "." { - if err := mkdirAllF(dir, 0755); err != nil { - return "", err + if err := writeOutputFile(outputDir, name, file.Content, 0o644, clobber); err != nil { + if os.IsExist(err) && !clobber { + return "", fmt.Errorf("output path already exists: %q (use --clobber to overwrite)", dest) } - } - - if err := writeFileF(dest, file.Content, 0644); err != nil { return "", err } return dest, nil } + +func trimTrailingPathSeparators(path string) string { + end := len(path) + for end > 0 && os.IsPathSeparator(path[end-1]) { + end-- + } + if end == 0 || end <= len(filepath.VolumeName(path)) { + return path + } + return path[:end] +} + +func rootedWriteOutputFile(dir, name string, data []byte, perm os.FileMode, replace bool) error { + if dir == "" { + return safepaths.WriteFile(name, data, perm, 0o755, replace) + } + + root, err := safepaths.OpenRootDir(dir, 0o755) + if err != nil { + return err + } + defer root.Close() + return root.WriteFile(name, data, perm, 0o755, replace) +} diff --git a/pkg/cmd/repo/read-file/read_file_test.go b/pkg/cmd/repo/read-file/read_file_test.go index 83716319015..db82d393f99 100644 --- a/pkg/cmd/repo/read-file/read_file_test.go +++ b/pkg/cmd/repo/read-file/read_file_test.go @@ -8,7 +8,9 @@ import ( "io" "io/fs" "net/http" + "os" "path/filepath" + "runtime" "strings" "testing" @@ -167,6 +169,8 @@ func Test_readFileRun(t *testing.T) { opts ReadFileOptions httpStubs func(*httpmock.Registry) jsonFields []string + setup func(t *testing.T) + verify func(t *testing.T) wantOut string wantStderr string wantErrMsg string @@ -196,6 +200,41 @@ func Test_readFileRun(t *testing.T) { }, wantOut: "hello world\n", }, + { + name: "directory output saves only the remote basename", + opts: ReadFileOptions{Output: "out/"}, + setup: func(t *testing.T) { + t.Helper() + t.Chdir(t.TempDir()) + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/contents/README.md"), + httpmock.JSONResponse(fileContentResponse(filepath.Join("nested", "notes.txt"), "hello world\n")), + ) + }, + verify: func(t *testing.T) { + t.Helper() + entries, err := os.ReadDir("out") + require.NoError(t, err) + require.Len(t, entries, 1) + assert.Equal(t, "notes.txt", entries[0].Name()) + content, err := os.ReadFile(filepath.Join("out", "notes.txt")) + require.NoError(t, err) + assert.Equal(t, "hello world\n", string(content)) + }, + }, + { + name: "json preserves the original remote name", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/contents/README.md"), + httpmock.JSONResponse(fileContentResponse(`nested\notes.txt`, "hello world\n")), + ) + }, + jsonFields: []string{"name"}, + wantOut: "{\"name\":\"nested\\\\notes.txt\"}\n", + }, { name: "writes file content through pager (tty)", tty: true, @@ -522,6 +561,9 @@ func Test_readFileRun(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if tt.setup != nil { + tt.setup(t) + } reg := &httpmock.Registry{} defer reg.Verify(t) if tt.httpStubs != nil { @@ -560,21 +602,40 @@ func Test_readFileRun(t *testing.T) { require.NoError(t, err) assert.Equal(t, tt.wantOut, stdout.String()) assert.Equal(t, tt.wantStderr, stderr.String()) + if tt.verify != nil { + tt.verify(t) + } }) } } func Test_writeToOutput(t *testing.T) { + var clobberTarget string + var danglingTarget string + var ancestorTarget string + var outputDirTarget string + var regularMode os.FileMode + backslashBasename := `nested\README.md` + backslashTraversalBasename := `..\..\README.md` + if runtime.GOOS == "windows" { + backslashBasename = "README.md" + backslashTraversalBasename = "README.md" + } + tests := []struct { - name string - file *repoFile - output string - clobber bool - lstat func(path string) (*lstatResult, error) - wantDest string - wantWrite string - wantDir string - wantErr string + name string + file *repoFile + output string + clobber bool + lstat func(path string) (*lstatResult, error) + setup func(t *testing.T) + useRealFS bool + verify func(t *testing.T) + wantDest string + wantWrite string + wantDir string + wantLstatPaths []string + wantErr string }{ { name: "writes to a new file path", @@ -587,17 +648,109 @@ func Test_writeToOutput(t *testing.T) { }, { name: "directory target uses remote basename", - file: &repoFile{Name: "README.md", Content: []byte("hi")}, + file: &repoFile{Name: "nested/README.md", Content: []byte("hi")}, output: "out/", lstat: func(path string) (*lstatResult, error) { - if path == "out/" { + if path == "out" { + return &lstatResult{isDir: true}, nil + } + return nil, fs.ErrNotExist + }, + wantDest: filepath.Join("out", "README.md"), + wantWrite: "hi", + wantDir: "out", + wantLstatPaths: []string{"out", filepath.Join("out", "README.md")}, + }, + { + name: "existing directory without separator uses remote basename", + file: &repoFile{Name: "nested/README.md", Content: []byte("hi")}, + output: "out", + lstat: func(path string) (*lstatResult, error) { + if path == "out" { return &lstatResult{isDir: true}, nil } return nil, fs.ErrNotExist }, - wantDest: filepath.Join("out", "README.md"), + wantDest: filepath.Join("out", "README.md"), + wantWrite: "hi", + wantDir: "out", + wantLstatPaths: []string{"out", filepath.Join("out", "README.md")}, + }, + { + name: "directory target handles native backslash separators", + file: &repoFile{Name: `nested\README.md`, Content: []byte("hi")}, + output: "out/", + lstat: func(string) (*lstatResult, error) { return nil, fs.ErrNotExist }, + wantDest: filepath.Join("out", backslashBasename), + wantWrite: "hi", + wantDir: "out", + wantLstatPaths: []string{"out", filepath.Join("out", backslashBasename)}, + }, + { + name: "directory target handles native backslash parent components", + file: &repoFile{Name: `..\..\README.md`, Content: []byte("hi")}, + output: "out/", + lstat: func(string) (*lstatResult, error) { return nil, fs.ErrNotExist }, + wantDest: filepath.Join("out", backslashTraversalBasename), + wantWrite: "hi", + wantDir: "out", + wantLstatPaths: []string{"out", filepath.Join("out", backslashTraversalBasename)}, + }, + { + name: "directory target uses basename of absolute remote name", + file: &repoFile{Name: filepath.Join(string(os.PathSeparator), "nested", "README.md"), Content: []byte("hi")}, + output: "out/", + lstat: func(string) (*lstatResult, error) { return nil, fs.ErrNotExist }, + wantDest: filepath.Join("out", "README.md"), + wantWrite: "hi", + wantDir: "out", + wantLstatPaths: []string{"out", filepath.Join("out", "README.md")}, + }, + { + name: "explicit output retains user selected path", + file: &repoFile{Name: "../remote.md", Content: []byte("hi")}, + output: filepath.Join("out", "nested", "local.md"), + lstat: func(string) (*lstatResult, error) { return nil, fs.ErrNotExist }, + wantDest: filepath.Join("out", "nested", "local.md"), wantWrite: "hi", - wantDir: "out", + wantDir: filepath.Join("out", "nested"), + }, + { + name: "basename collision without clobber errors", + file: &repoFile{Name: "nested/README.md", Content: []byte("hi")}, + output: "out/", + lstat: func(path string) (*lstatResult, error) { + if path == "out" { + return &lstatResult{isDir: true}, nil + } + return &lstatResult{}, nil + }, + wantLstatPaths: []string{"out", filepath.Join("out", "README.md")}, + wantErr: fmt.Sprintf("output path already exists: %q (use --clobber to overwrite)", filepath.Join("out", "README.md")), + }, + { + name: "directory target rejects empty remote name", + file: &repoFile{Name: "", Content: []byte("hi")}, + output: "out/", + lstat: func(string) (*lstatResult, error) { return nil, fs.ErrNotExist }, + wantLstatPaths: []string{"out"}, + wantErr: `invalid output filename ""`, + }, + { + name: "directory target rejects parent-only remote name", + file: &repoFile{Name: "..", Content: []byte("hi")}, + output: "out/", + lstat: func(string) (*lstatResult, error) { return nil, fs.ErrNotExist }, + wantLstatPaths: []string{"out"}, + wantErr: `invalid output filename ".."`, + }, + { + name: "directory target rejects root-only remote name", + file: &repoFile{Name: string(os.PathSeparator), Content: []byte("hi")}, + output: "out/", + lstat: func(string) (*lstatResult, error) { return nil, fs.ErrNotExist }, + wantLstatPaths: []string{"out"}, + wantErr: fmt.Sprintf("invalid output filename %q", string(os.PathSeparator)), }, { name: "existing file without clobber errors", @@ -641,41 +794,237 @@ func Test_writeToOutput(t *testing.T) { }, wantErr: "something went wrong", }, + { + name: "clobber rejects output symlink", + file: &repoFile{Name: "README.md", Content: []byte("hi")}, + output: "README.md", + clobber: true, + lstat: lstat, + useRealFS: true, + setup: func(t *testing.T) { + t.Helper() + clobberTarget = filepath.Join(t.TempDir(), "target.md") + require.NoError(t, os.WriteFile(clobberTarget, []byte("old"), 0o644)) + require.NoError(t, os.Symlink(clobberTarget, "README.md")) + }, + verify: func(t *testing.T) { + t.Helper() + info, err := os.Lstat("README.md") + require.NoError(t, err) + assert.NotZero(t, info.Mode()&os.ModeSymlink) + content, err := os.ReadFile(clobberTarget) + require.NoError(t, err) + assert.Equal(t, "old", string(content)) + }, + wantErr: "output path is a symlink", + }, + { + name: "no clobber rejects dangling final symlink", + file: &repoFile{Name: "README.md", Content: []byte("hi")}, + output: "README.md", + lstat: lstat, + useRealFS: true, + setup: func(t *testing.T) { + t.Helper() + danglingTarget = filepath.Join(t.TempDir(), "missing.md") + require.NoError(t, os.Symlink(danglingTarget, "README.md")) + }, + verify: func(t *testing.T) { + t.Helper() + assert.NoFileExists(t, danglingTarget) + }, + wantErr: "output path is a symlink", + }, + { + name: "trailing separator rejects output symlink", + file: &repoFile{Name: "README.md", Content: []byte("hi")}, + output: "linked/", + lstat: lstat, + useRealFS: true, + setup: func(t *testing.T) { + t.Helper() + outputDirTarget = t.TempDir() + require.NoError(t, os.Symlink(outputDirTarget, "linked")) + }, + verify: func(t *testing.T) { + t.Helper() + assert.NoFileExists(t, filepath.Join(outputDirTarget, "README.md")) + }, + wantErr: "output path is a symlink", + }, + { + name: "trailing separator clobber rejects output symlink", + file: &repoFile{Name: "README.md", Content: []byte("hi")}, + output: "linked/", + clobber: true, + lstat: lstat, + useRealFS: true, + setup: func(t *testing.T) { + t.Helper() + outputDirTarget = t.TempDir() + require.NoError(t, os.Symlink(outputDirTarget, "linked")) + }, + verify: func(t *testing.T) { + t.Helper() + info, err := os.Lstat("linked") + require.NoError(t, err) + assert.NotZero(t, info.Mode()&os.ModeSymlink) + assert.NoFileExists(t, filepath.Join(outputDirTarget, "README.md")) + }, + wantErr: "output path is a symlink", + }, + { + name: "directory output clobber rejects basename symlink", + file: &repoFile{Name: "nested/README.md", Content: []byte("hi")}, + output: "out/", + clobber: true, + lstat: lstat, + useRealFS: true, + setup: func(t *testing.T) { + t.Helper() + require.NoError(t, os.Mkdir("out", 0o755)) + clobberTarget = filepath.Join(t.TempDir(), "target.md") + require.NoError(t, os.WriteFile(clobberTarget, []byte("old"), 0o644)) + require.NoError(t, os.Symlink(clobberTarget, filepath.Join("out", "README.md"))) + }, + verify: func(t *testing.T) { + t.Helper() + info, err := os.Lstat(filepath.Join("out", "README.md")) + require.NoError(t, err) + assert.NotZero(t, info.Mode()&os.ModeSymlink) + content, err := os.ReadFile(clobberTarget) + require.NoError(t, err) + assert.Equal(t, "old", string(content)) + }, + wantErr: "output path is a symlink", + }, + { + name: "clobber preserves regular file mode", + file: &repoFile{Name: "README.md", Content: []byte("hi")}, + output: "README.md", + clobber: true, + lstat: lstat, + useRealFS: true, + setup: func(t *testing.T) { + t.Helper() + require.NoError(t, os.WriteFile("README.md", []byte("old"), 0o600)) + info, err := os.Stat("README.md") + require.NoError(t, err) + regularMode = info.Mode().Perm() + }, + verify: func(t *testing.T) { + t.Helper() + info, err := os.Stat("README.md") + require.NoError(t, err) + assert.Equal(t, regularMode, info.Mode().Perm()) + content, err := os.ReadFile("README.md") + require.NoError(t, err) + assert.Equal(t, "hi", string(content)) + }, + wantDest: "README.md", + }, + { + name: "allows ancestor symlink in explicit output parent", + file: &repoFile{Name: "README.md", Content: []byte("hi")}, + output: filepath.Join("out", "nested", "README.md"), + lstat: lstat, + useRealFS: true, + setup: func(t *testing.T) { + t.Helper() + ancestorTarget = t.TempDir() + require.NoError(t, os.Symlink(ancestorTarget, "out")) + }, + verify: func(t *testing.T) { + t.Helper() + content, err := os.ReadFile(filepath.Join(ancestorTarget, "nested", "README.md")) + require.NoError(t, err) + assert.Equal(t, "hi", string(content)) + }, + wantDest: filepath.Join("out", "nested", "README.md"), + }, + { + name: "directory target flattens remote parent components", + file: &repoFile{Name: "../README.md", Content: []byte("hi")}, + output: "out/", + lstat: lstat, + useRealFS: true, + setup: func(t *testing.T) { + t.Helper() + require.NoError(t, os.Mkdir("out", 0o755)) + }, + verify: func(t *testing.T) { + t.Helper() + assert.NoFileExists(t, "README.md") + content, err := os.ReadFile(filepath.Join("out", "README.md")) + require.NoError(t, err) + assert.Equal(t, "hi", string(content)) + }, + wantDest: filepath.Join("out", "README.md"), + wantLstatPaths: []string{"out", filepath.Join("out", "README.md")}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Chdir(t.TempDir()) + if tt.setup != nil { + tt.setup(t) + } + var gotDir string var gotWritePath string var gotWriteContent []byte + var gotLstatPaths []string - origLstat, origMkdir, origWrite := lstatF, mkdirAllF, writeFileF + origLstat, origWrite := lstatF, writeOutputFile defer func() { - lstatF, mkdirAllF, writeFileF = origLstat, origMkdir, origWrite + lstatF, writeOutputFile = origLstat, origWrite }() - lstatF = tt.lstat - mkdirAllF = func(path string, _ fs.FileMode) error { - gotDir = path - return nil + lstatF = func(path string) (*lstatResult, error) { + gotLstatPaths = append(gotLstatPaths, path) + return tt.lstat(path) } - writeFileF = func(path string, data []byte, _ fs.FileMode) error { - gotWritePath = path - gotWriteContent = data - return nil + if !tt.useRealFS { + writeOutputFile = func(dir, name string, data []byte, _ fs.FileMode, _ bool) error { + gotDir = dir + if dir == "" { + gotDir = filepath.Dir(name) + gotWritePath = name + } else { + gotDir = filepath.Clean(dir) + gotWritePath = filepath.Join(dir, name) + } + gotWriteContent = data + return nil + } } + originalName := tt.file.Name dest, err := writeToOutput(tt.file, tt.output, tt.clobber) + assert.Equal(t, originalName, tt.file.Name) + if tt.wantLstatPaths != nil { + assert.Equal(t, tt.wantLstatPaths, gotLstatPaths) + } if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) + assert.Empty(t, gotWritePath) + if tt.verify != nil { + tt.verify(t) + } return } require.NoError(t, err) assert.Equal(t, tt.wantDest, dest) - assert.Equal(t, tt.wantDest, gotWritePath) - assert.Equal(t, tt.wantWrite, string(gotWriteContent)) - if tt.wantDir != "" { - assert.Equal(t, tt.wantDir, gotDir) + if !tt.useRealFS { + assert.Equal(t, tt.wantDest, gotWritePath) + assert.Equal(t, tt.wantWrite, string(gotWriteContent)) + if tt.wantDir != "" { + assert.Equal(t, tt.wantDir, gotDir) + } + } + if tt.verify != nil { + tt.verify(t) } }) } diff --git a/pkg/cmd/run/download/download.go b/pkg/cmd/run/download/download.go index 760d1b4bdc2..ca7733bda01 100644 --- a/pkg/cmd/run/download/download.go +++ b/pkg/cmd/run/download/download.go @@ -30,7 +30,7 @@ type DownloadOptions struct { type platform interface { List(runID string) ([]shared.Artifact, error) - Download(url safeurl.SafeURL, dir safepaths.Absolute) error + Download(url safeurl.SafeURL, openDestination func() (*safepaths.Root, error)) error } type iprompter interface { @@ -152,23 +152,11 @@ func runDownload(opts *DownloadOptions) error { } } - opts.IO.StartProgressIndicator() - defer opts.IO.StopProgressIndicator() - - // track downloaded artifacts and avoid re-downloading any of the same name, isolate if multiple artifacts - downloaded := set.NewStringSet() - isolateArtifacts := isolateArtifacts(wantNames, wantPatterns) - - absoluteDestinationDir, err := safepaths.ParseAbsolute(opts.DestinationDir) - if err != nil { - return fmt.Errorf("error parsing destination directory: %w", err) - } - + // Select matching artifacts before creating the destination directory. + selectedNames := set.NewStringSet() + var selected []shared.Artifact for _, a := range artifacts { - if a.Expired { - continue - } - if downloaded.Contains(a.Name) { + if a.Expired || selectedNames.Contains(a.Name) { continue } if len(wantNames) > 0 || len(wantPatterns) > 0 { @@ -176,27 +164,49 @@ func runDownload(opts *DownloadOptions) error { continue } } + selected = append(selected, a) + selectedNames.Add(a.Name) + } + if len(selected) == 0 { + return errors.New("no artifact matches any of the names or patterns provided") + } + + opts.IO.StartProgressIndicator() + defer opts.IO.StopProgressIndicator() + + isolateArtifacts := isolateArtifacts(wantNames, wantPatterns) - destDir := absoluteDestinationDir + for _, a := range selected { if isolateArtifacts { - destDir, err = absoluteDestinationDir.Join(a.Name) + if err := safepaths.ValidateChild(a.Name); err != nil { + return fmt.Errorf("error downloading %s: would result in path traversal", a.Name) + } + } + + openDestination := func() (*safepaths.Root, error) { + root, err := safepaths.OpenRootDir(opts.DestinationDir, 0o755) if err != nil { - if _, ok := errors.AsType[safepaths.PathTraversalError](err); ok { - return fmt.Errorf("error downloading %s: would result in path traversal", a.Name) - } - return err + return nil, fmt.Errorf("error opening destination directory: %w", err) } + if !isolateArtifacts { + return root, nil + } + destDir, err := root.Sub(a.Name, 0o755) + closeErr := root.Close() + if err != nil { + return nil, err + } + if closeErr != nil { + _ = destDir.Close() + return nil, closeErr + } + return destDir, nil } - err := opts.Platform.Download(safeurl.NewImmutableSafeURL(a.DownloadURL), destDir) + err := opts.Platform.Download(safeurl.NewImmutableSafeURL(a.DownloadURL), openDestination) if err != nil { return fmt.Errorf("error downloading %s: %w", a.Name, err) } - downloaded.Add(a.Name) - } - - if downloaded.Len() == 0 { - return errors.New("no artifact matches any of the names or patterns provided") } return nil diff --git a/pkg/cmd/run/download/download_test.go b/pkg/cmd/run/download/download_test.go index a001cd78111..7fc88b2d217 100644 --- a/pkg/cmd/run/download/download_test.go +++ b/pkg/cmd/run/download/download_test.go @@ -158,7 +158,8 @@ type testArtifact struct { } type fakePlatform struct { - runs []run + runs []run + downloadErr error } func (f *fakePlatform) List(runID string) ([]shared.Artifact, error) { @@ -187,10 +188,16 @@ func (f *fakePlatform) List(runID string) ([]shared.Artifact, error) { return artifacts, nil } -func (f *fakePlatform) Download(url safeurl.SafeURL, dir safepaths.Absolute) error { - if err := os.MkdirAll(dir.String(), 0755); err != nil { +func (f *fakePlatform) Download(url safeurl.SafeURL, openDestination func() (*safepaths.Root, error)) error { + if f.downloadErr != nil { + return f.downloadErr + } + dir, err := openDestination() + if err != nil { return err } + defer dir.Close() + // Now to be consistent, we find the artifact with the provided URL. // It's a bit janky to iterate the runs, to find the right artifact // rather than keying directly to it, but it allows the setup of the @@ -200,8 +207,7 @@ func (f *fakePlatform) Download(url safeurl.SafeURL, dir safepaths.Absolute) err for _, testArtifact := range run.testArtifacts { if testArtifact.artifact.DownloadURL == url.String() { for _, file := range testArtifact.files { - path := filepath.Join(dir.String(), file) - return os.WriteFile(path, []byte{}, 0600) + return dir.WriteFile(file, []byte{}, 0o600, 0o755, false) } } } @@ -212,12 +218,14 @@ func (f *fakePlatform) Download(url safeurl.SafeURL, dir safepaths.Absolute) err func Test_runDownload(t *testing.T) { tests := []struct { - name string - opts DownloadOptions - platform *fakePlatform - promptStubs func(*prompter.MockPrompter) - expectedFiles []string - wantErr string + name string + opts DownloadOptions + platform *fakePlatform + promptStubs func(*prompter.MockPrompter) + expectedFiles []string + wantErr string + wantDestinationMissing bool + useEmptyDestination bool }{ { name: "download non-expired to relative directory", @@ -269,6 +277,31 @@ func Test_runDownload(t *testing.T) { filepath.Join("artifact-2", "artifact-2-file"), }, }, + { + name: "empty destination uses current directory", + opts: DownloadOptions{ + RunID: "2345", + Names: []string{"artifact-1"}, + }, + platform: &fakePlatform{ + runs: []run{ + { + id: "2345", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + }, + files: []string{"artifact-file"}, + }, + }, + }, + }, + }, + expectedFiles: []string{"artifact-file"}, + useEmptyDestination: true, + }, { name: "download non-expired to absolute directory", opts: DownloadOptions{ @@ -359,8 +392,9 @@ func Test_runDownload(t *testing.T) { { name: "no name matches", opts: DownloadOptions{ - RunID: "2345", - Names: []string{"artifact-3"}, + RunID: "2345", + Names: []string{"artifact-3"}, + DestinationDir: "missing", }, platform: &fakePlatform{ runs: []run{ @@ -391,8 +425,34 @@ func Test_runDownload(t *testing.T) { }, }, }, - expectedFiles: []string{}, - wantErr: "no artifact matches any of the names or patterns provided", + expectedFiles: []string{}, + wantErr: "no artifact matches any of the names or patterns provided", + wantDestinationMissing: true, + }, + { + name: "download error does not create destination", + opts: DownloadOptions{ + RunID: "2345", + DestinationDir: "missing", + }, + platform: &fakePlatform{ + downloadErr: errors.New("transfer failed"), + runs: []run{ + { + id: "2345", + testArtifacts: []testArtifact{ + { + artifact: shared.Artifact{ + Name: "artifact-1", + DownloadURL: "http://download.com/artifact1.zip", + }, + }, + }, + }, + }, + }, + wantErr: "error downloading artifact-1: transfer failed", + wantDestinationMissing: true, }, { name: "pattern matches", @@ -716,10 +776,16 @@ func Test_runDownload(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { opts := &tt.opts - if opts.DestinationDir == "" { + assertionDir := "" + if tt.useEmptyDestination { + t.Chdir(t.TempDir()) + assertionDir = "." + } else if opts.DestinationDir == "" { opts.DestinationDir = t.TempDir() + assertionDir = opts.DestinationDir } else { opts.DestinationDir = filepath.Join(t.TempDir(), opts.DestinationDir) + assertionDir = opts.DestinationDir } ios, _, stdout, stderr := iostreams.Test() @@ -738,13 +804,17 @@ func Test_runDownload(t *testing.T) { } else { require.NoError(t, err) } + if tt.wantDestinationMissing { + assert.NoDirExists(t, opts.DestinationDir) + return + } // Check that the exact number of files exist - require.Equal(t, len(tt.expectedFiles), countFilesInDirRecursively(t, opts.DestinationDir)) + require.Equal(t, len(tt.expectedFiles), countFilesInDirRecursively(t, assertionDir)) // Then check that the exact files are correct for _, name := range tt.expectedFiles { - require.FileExists(t, filepath.Join(opts.DestinationDir, name)) + require.FileExists(t, filepath.Join(assertionDir, name)) } assert.Equal(t, "", stdout.String()) diff --git a/pkg/cmd/run/download/fixtures/myproject.zip b/pkg/cmd/run/download/fixtures/myproject.zip deleted file mode 100644 index 2fdf3f90c6e..00000000000 Binary files a/pkg/cmd/run/download/fixtures/myproject.zip and /dev/null differ diff --git a/pkg/cmd/run/download/http.go b/pkg/cmd/run/download/http.go index ad15984c67c..7217be573ee 100644 --- a/pkg/cmd/run/download/http.go +++ b/pkg/cmd/run/download/http.go @@ -24,11 +24,11 @@ func (p *apiPlatform) List(runID string) ([]shared.Artifact, error) { return shared.ListArtifacts(p.client, p.repo, runID) } -func (p *apiPlatform) Download(url safeurl.SafeURL, dir safepaths.Absolute) error { - return downloadArtifact(p.client, p.repo.RepoHost(), url, dir) +func (p *apiPlatform) Download(url safeurl.SafeURL, openDestination func() (*safepaths.Root, error)) error { + return downloadArtifact(p.client, p.repo.RepoHost(), url, openDestination) } -func downloadArtifact(httpClient *http.Client, hostname string, url safeurl.SafeURL, destDir safepaths.Absolute) error { +func downloadArtifact(httpClient *http.Client, hostname string, url safeurl.SafeURL, openDestination func() (*safepaths.Root, error)) (downloadErr error) { // The server rejects this :( //api.WithHeader("Accept", "application/zip") // @@ -61,6 +61,15 @@ func downloadArtifact(httpClient *http.Client, hostname string, url safeurl.Safe if err != nil { return fmt.Errorf("error extracting zip archive: %w", err) } + destDir, err := openDestination() + if err != nil { + return err + } + defer func() { + if err := destDir.Close(); downloadErr == nil && err != nil { + downloadErr = err + } + }() if err := ghzip.ExtractZip(zipfile, destDir); err != nil { return fmt.Errorf("error extracting zip archive: %w", err) } diff --git a/pkg/cmd/run/download/http_test.go b/pkg/cmd/run/download/http_test.go index 33874388621..cc7911eaf2c 100644 --- a/pkg/cmd/run/download/http_test.go +++ b/pkg/cmd/run/download/http_test.go @@ -1,6 +1,8 @@ package download import ( + "archive/zip" + "bytes" "net/http" "os" "path/filepath" @@ -59,51 +61,128 @@ func Test_List_perRepository(t *testing.T) { } func Test_Download(t *testing.T) { - tmpDir := t.TempDir() - destDir, err := safepaths.ParseAbsolute(filepath.Join(tmpDir, "artifact")) - require.NoError(t, err) + tests := []struct { + name string + setup func(t *testing.T, destDir, outside string) + wantErr string + verify func(t *testing.T, tmpDir, outside string) + }{ + { + name: "extracts artifact", + verify: func(t *testing.T, tmpDir, _ string) { + t.Helper() + var paths []string + parentPrefix := tmpDir + string(filepath.Separator) + err := filepath.Walk(tmpDir, func(p string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if p == tmpDir { + return nil + } + entry := strings.TrimPrefix(p, parentPrefix) + if info.IsDir() { + entry += "/" + } else if info.Mode()&0o111 != 0 { + entry += "(X)" + } + paths = append(paths, entry) + return nil + }) + require.NoError(t, err) - reg := &httpmock.Registry{} - defer reg.Verify(t) + sort.Strings(paths) + assert.Equal(t, []string{ + "artifact/", + filepath.Join("artifact", "bin") + "/", + filepath.Join("artifact", "bin", "myexe"), + filepath.Join("artifact", "empty") + "/", + filepath.Join("artifact", "readme.md"), + filepath.Join("artifact", "src") + "/", + filepath.Join("artifact", "src", "main.go"), + filepath.Join("artifact", "src", "util.go"), + }, paths) + }, + }, + { + name: "rejects artifact ancestor symlink", + setup: func(t *testing.T, destDir, outside string) { + t.Helper() + require.NoError(t, os.Symlink(outside, filepath.Join(destDir, "src"))) + }, + wantErr: "symbolic link", + verify: func(t *testing.T, _, outside string) { + t.Helper() + assert.NoFileExists(t, filepath.Join(outside, "main.go")) + assert.NoFileExists(t, filepath.Join(outside, "util.go")) + }, + }, + } - reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/actions/artifacts/12345/zip"), - httpmock.FileResponse("./fixtures/myproject.zip")) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := t.TempDir() + destDir := filepath.Join(tmpDir, "artifact") + require.NoError(t, os.MkdirAll(destDir, 0o755)) + outside := t.TempDir() + if tt.setup != nil { + tt.setup(t, destDir, outside) + } - api := &apiPlatform{ - client: &http.Client{Transport: reg}, - repo: ghrepo.New("OWNER", "REPO"), + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/actions/artifacts/12345/zip"), + httpmock.BinaryResponse(testArtifactZip(t))) + + api := &apiPlatform{ + client: &http.Client{Transport: reg}, + repo: ghrepo.New("OWNER", "REPO"), + } + err := api.Download( + safeurl.NewImmutableSafeURL("https://api.github.com/repos/OWNER/REPO/actions/artifacts/12345/zip"), + func() (*safepaths.Root, error) { + return safepaths.OpenRoot(destDir) + }, + ) + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + } else { + require.NoError(t, err) + } + if tt.verify != nil { + tt.verify(t, tmpDir, outside) + } + }) } - require.NoError(t, api.Download(safeurl.NewImmutableSafeURL("https://api.github.com/repos/OWNER/REPO/actions/artifacts/12345/zip"), destDir)) +} - var paths []string - parentPrefix := tmpDir + string(filepath.Separator) - err = filepath.Walk(tmpDir, func(p string, info os.FileInfo, err error) error { - if err != nil { - return err - } - if p == tmpDir { - return nil - } - entry := strings.TrimPrefix(p, parentPrefix) - if info.IsDir() { - entry += "/" - } else if info.Mode()&0111 != 0 { - entry += "(X)" - } - paths = append(paths, entry) - return nil - }) - require.NoError(t, err) +func testArtifactZip(t *testing.T) []byte { + t.Helper() - sort.Strings(paths) - assert.Equal(t, []string{ - "artifact/", - filepath.Join("artifact", "bin") + "/", - filepath.Join("artifact", "bin", "myexe"), - filepath.Join("artifact", "readme.md"), - filepath.Join("artifact", "src") + "/", - filepath.Join("artifact", "src", "main.go"), - filepath.Join("artifact", "src", "util.go"), - }, paths) + var archive bytes.Buffer + writer := zip.NewWriter(&archive) + files := []struct { + name string + mode os.FileMode + }{ + {name: "bin/myexe", mode: 0o644}, + {name: "empty/", mode: os.ModeDir | 0o755}, + {name: "../outside.txt", mode: 0o644}, + {name: "readme.md", mode: 0o644}, + {name: "src/main.go", mode: 0o644}, + {name: "src/util.go", mode: 0o644}, + } + for _, file := range files { + header := &zip.FileHeader{Name: file.name, Method: zip.Store} + header.SetMode(file.mode) + entry, err := writer.CreateHeader(header) + require.NoError(t, err) + if !file.mode.IsDir() { + _, err = entry.Write([]byte("content")) + require.NoError(t, err) + } + } + require.NoError(t, writer.Close()) + return archive.Bytes() } diff --git a/pkg/cmd/skills/search/search.go b/pkg/cmd/skills/search/search.go index aeb025ec521..ae32dd45a84 100644 --- a/pkg/cmd/skills/search/search.go +++ b/pkg/cmd/skills/search/search.go @@ -569,17 +569,8 @@ func promptInstall(opts *SearchOptions, skills []skillResult) error { fmt.Fprintf(opts.IO.ErrOut, "\n%s Installing %s from %s...\n", cs.Blue("::"), displayName, s.Repo) - // Use the repo-relative directory path (e.g. "skills/author/name") - // for disambiguation when installing namespaced skills, so the - // install command can resolve the exact skill without ambiguity. - installArg := s.SkillName - if s.Namespace != "" { - installArg = strings.TrimSuffix(s.Path, "/SKILL.md") - } - - //nolint:gosec // arguments are from user-selected search results, not arbitrary input - cmd := exec.Command(opts.ExecutablePath, "skills", "install", s.Repo, installArg, - "--agent", host.ID, "--scope", scope) + //nolint:gosec // executable is the current gh binary + cmd := exec.Command(opts.ExecutablePath, installCommandArgs(s, host.ID, scope)...) cmd.Stdin = os.Stdin cmd.Stdout = opts.IO.Out cmd.Stderr = opts.IO.ErrOut @@ -592,6 +583,26 @@ func promptInstall(opts *SearchOptions, skills []skillResult) error { return nil } +func installCommandArgs(skill skillResult, agent, scope string) []string { + // Use the repo-relative directory path (e.g. "skills/author/name") + // for disambiguation when installing namespaced skills, so the + // install command can resolve the exact skill without ambiguity. + installArg := skill.SkillName + if skill.Namespace != "" { + installArg = strings.TrimSuffix(skill.Path, "/SKILL.md") + } + + return []string{ + "skills", "install", + "--agent", agent, + "--scope", scope, + // Keep untrusted repository and skill-selector values after -- so + // they cannot be parsed as flags. + "--", + skill.Repo, installArg, + } +} + // relevanceScore computes a numeric ranking score for a search result. // Higher scores rank first. Signals (in priority order): // - Exact skill name match (3 000 points) diff --git a/pkg/cmd/skills/search/search_test.go b/pkg/cmd/skills/search/search_test.go index d2983f5da7a..8f2fe51f059 100644 --- a/pkg/cmd/skills/search/search_test.go +++ b/pkg/cmd/skills/search/search_test.go @@ -10,6 +10,7 @@ import ( "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/telemetry" + "github.com/cli/cli/v2/pkg/cmd/skills/install" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -463,6 +464,70 @@ func TestExtractSkillInfo(t *testing.T) { } } +func TestInstallCommandArgs(t *testing.T) { + tests := []struct { + name string + resultPath string + wantSkill string + want []string + }{ + { + name: "namespaced skill", + resultPath: "skills/monalisa/code-review/SKILL.md", + wantSkill: "skills/monalisa/code-review", + want: []string{ + "skills", "install", + "--agent", "github-copilot", + "--scope", "project", + "--", + "org/repo", "skills/monalisa/code-review", + }, + }, + { + name: "flag-like result path", + resultPath: "--option/skills/monalisa/code-review/SKILL.md", + wantSkill: "--option/skills/monalisa/code-review", + want: []string{ + "skills", "install", + "--agent", "github-copilot", + "--scope", "project", + "--", + "org/repo", "--option/skills/monalisa/code-review", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + results := deduplicateResults([]codeSearchItem{{ + Path: tt.resultPath, + Repository: codeSearchRepository{FullName: "org/repo"}, + }}) + require.Len(t, results, 1) + + args := installCommandArgs(results[0], "github-copilot", "project") + assert.Equal(t, tt.want, args) + + var installOpts *install.InstallOptions + cmd := install.NewCmdInstall(&cmdutil.Factory{}, &telemetry.NoOpService{}, func(opts *install.InstallOptions) error { + installOpts = opts + return nil + }) + cmd.SetArgs(args[2:]) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + _, err := cmd.ExecuteC() + require.NoError(t, err) + require.NotNil(t, installOpts) + assert.Equal(t, "org/repo", installOpts.SkillSource) + assert.Equal(t, tt.wantSkill, installOpts.SkillName) + assert.Equal(t, "github-copilot", installOpts.Agent) + assert.Equal(t, "project", installOpts.Scope) + }) + } +} + func TestFilterByRelevance(t *testing.T) { skills := []skillResult{ {Repo: "org/repo1", Owner: "org", RepoName: "repo1", SkillName: "terraform"},