fix(ci): pin artifact destinations (D-1058, D-1015, D-1194, D-1129)

free/ci/internal/artifacts/records/read.go

@@ -7,7 +7,7 @@ package records
 // Constraints: "not found" and "not visible" are the same error value; a
 // record whose blob is gone is reported missing (and remembered) and Get
 // refuses it (E712 missing_blob); Get writes only inside a directory that
-// authz.ArtifactGetDest accepted, creates every file exclusively and
+// authz.OpenArtifactGetDest pinned, creates every file exclusively and
 // re-verifies size and digest of what it wrote.
 
 import (
@@ -99,29 +99,15 @@ func (r Reader) Get(ctx context.Context, v authz.Viewer, id, dest, cwd string) (
 	if err := verifyBlob(r.Blobs, rec); err != nil {
 		return "", rec, err
 	}
-	if err := os.Mkdir(dir, 0o700); err != nil && !errors.Is(err, os.ErrExist) {
-		return "", rec, failf(ReasonUnreadable, "destination: %v", err)
-	}
-	// re-check after the create: the directory must still be empty and ours
-	if dir, err = authz.ArtifactGetDest(dir, cwd); err != nil {
+	root, dir, err := authz.OpenArtifactGetDest(dir, cwd)
+	if err != nil {
 		return "", rec, err
 	}
-	return r.write(dir, rel, rec)
+	defer func() { _ = root.Close() }()
+	return r.write(root, dir, rel, rec)
 }
 
-func (r Reader) write(dir, rel string, rec Record) (string, Record, error) {
-	before, err := os.Lstat(dir)
-	if err != nil || !before.IsDir() {
-		return "", rec, failf(ReasonUnreadable, "destination %q is not a plain directory", dir)
-	}
-	root, err := os.OpenRoot(dir)
-	if err != nil {
-		return "", rec, failf(ReasonUnreadable, "destination: %v", err)
-	}
-	defer func() { _ = root.Close() }()
-	if opened, serr := root.Stat("."); serr != nil || !os.SameFile(before, opened) {
-		return "", rec, failf(ReasonUnreadable, "destination %q changed while it was opened", dir)
-	}
+func (r Reader) write(root *os.Root, dir, rel string, rec Record) (string, Record, error) {
 	if d := filepath.Dir(rel); d != "." {
 		if err := root.MkdirAll(d, 0o700); err != nil {
 			return "", rec, failf(ReasonUnreadable, "destination: %v", err)

free/ci/internal/authz/dest.go

@@ -4,11 +4,9 @@ package authz
 // directory that is not the working directory and not under a .git directory.
 // Inputs:  the requested destination and the caller's working directory.
 // Outputs: the absolute, symlink-resolved destination, or a *PathError (E683).
-// Constraints: the only I/O in this package (stat, read the directory, resolve
-// symlinks). It creates nothing: the caller creates a missing destination with
-// os.Mkdir (not MkdirAll) mode 0700, which fails if anything appeared in the
-// meantime, and passes the returned path on. A leaf that is a symlink is
-// refused, so the check and the write name the same directory.
+// Constraints: this function only validates a pathname. Extraction callers
+// must use OpenArtifactGetDest and write through its pinned os.Root; a returned
+// pathname alone cannot protect against concurrent parent replacement.
 
 import (
 	"errors"

free/ci/internal/authz/dest_open.go

@@ -0,0 +1,76 @@
+package authz
+
+import (
+	"errors"
+	"io"
+	"os"
+	"path/filepath"
+)
+
+// OpenArtifactGetDest validates and opens an empty destination for extraction.
+// A missing leaf is created with mode 0700 through its pinned parent. Callers
+// must write through the returned root and close it; the path is for display
+// only. Renaming a parent after this call cannot redirect extraction.
+func OpenArtifactGetDest(dest, cwd string) (*os.Root, string, error) {
+	if dest == "" || !filepath.IsAbs(cwd) {
+		return nil, "", reject(DestEmpty, dest)
+	}
+	requested := dest
+	if !filepath.IsAbs(requested) {
+		requested = filepath.Join(cwd, requested)
+	}
+	requested = filepath.Clean(requested)
+	// Capture identities before validation, so replacement between validation
+	// and opening cannot turn an approved pathname into a different directory.
+	parentInfo, err := os.Stat(filepath.Dir(requested))
+	if err != nil {
+		return nil, "", reject(DestNoParent, dest)
+	}
+	leafInfo, leafErr := os.Lstat(requested)
+	if leafErr != nil && !errors.Is(leafErr, os.ErrNotExist) {
+		return nil, "", reject(DestUnreachable, dest)
+	}
+	resolved, err := ArtifactGetDest(dest, cwd)
+	if err != nil {
+		return nil, "", err
+	}
+	parent, err := os.OpenRoot(filepath.Dir(resolved))
+	if err != nil {
+		return nil, "", reject(DestUnreachable, dest)
+	}
+	defer parent.Close()
+	if opened, err := parent.Stat("."); err != nil || !os.SameFile(parentInfo, opened) {
+		return nil, "", reject(DestUnreachable, dest)
+	}
+	leaf := filepath.Base(resolved)
+	if leafErr != nil {
+		if err := parent.Mkdir(leaf, 0o700); err != nil {
+			return nil, "", reject(DestUnreachable, dest)
+		}
+		leafInfo, err = parent.Lstat(leaf)
+		if err != nil {
+			return nil, "", reject(DestUnreachable, dest)
+		}
+	}
+	if !leafInfo.IsDir() {
+		return nil, "", reject(DestNotDir, dest)
+	}
+	root, err := parent.OpenRoot(leaf)
+	if err != nil {
+		return nil, "", reject(DestUnreachable, dest)
+	}
+	fail := func(reason PathReason) (*os.Root, string, error) { root.Close(); return nil, "", reject(reason, dest) }
+	if opened, err := root.Stat("."); err != nil || !os.SameFile(leafInfo, opened) {
+		return fail(DestUnreachable)
+	}
+	dir, err := root.Open(".")
+	if err != nil {
+		return fail(DestUnreachable)
+	}
+	names, err := dir.Readdirnames(1)
+	dir.Close()
+	if len(names) != 0 || (err != nil && !errors.Is(err, io.EOF)) {
+		return fail(DestNotEmpty)
+	}
+	return root, resolved, nil
+}

free/ci/internal/authz/dest_open_test.go

@@ -0,0 +1,73 @@
+package authz_test
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/nself-org/plugins/free/ci/internal/authz"
+)
+
+func TestOpenArtifactGetDestPinsParentAndLeaf(t *testing.T) {
+	for _, existing := range []bool{false, true} {
+		t.Run(map[bool]string{false: "new", true: "existing"}[existing], func(t *testing.T) {
+			base := scratch(t)
+			cwd := filepath.Join(base, "work")
+			parent := filepath.Join(base, "parent")
+			outside := filepath.Join(base, "outside")
+			for _, p := range []string{cwd, parent, outside} {
+				mustMkdir(t, p)
+			}
+			dest := filepath.Join(parent, "out")
+			if existing {
+				mustMkdir(t, dest)
+			}
+			root, _, err := authz.OpenArtifactGetDest(dest, cwd)
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer root.Close()
+			moved := filepath.Join(base, "moved")
+			if err := os.Rename(parent, moved); err != nil {
+				t.Fatal(err)
+			}
+			mustMkdir(t, filepath.Join(outside, "out"))
+			if err := os.Symlink(outside, parent); err != nil {
+				t.Fatal(err)
+			}
+			file, err := root.OpenFile("artifact", os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600)
+			if err != nil {
+				t.Fatal(err)
+			}
+			file.Close()
+			if _, err := os.Stat(filepath.Join(outside, "out", "artifact")); !os.IsNotExist(err) {
+				t.Fatalf("write escaped to replacement parent: %v", err)
+			}
+			if _, err := os.Stat(filepath.Join(moved, "out", "artifact")); err != nil {
+				t.Fatal(err)
+			}
+		})
+	}
+}
+
+func TestOpenArtifactGetDestRefusesNonemptyAndSymlink(t *testing.T) {
+	base := scratch(t)
+	cwd := filepath.Join(base, "work")
+	mustMkdir(t, cwd)
+	full := filepath.Join(base, "full")
+	mustMkdir(t, full)
+	mustWrite(t, filepath.Join(full, "artifact"))
+	link := filepath.Join(base, "link")
+	if err := os.Symlink(full, link); err != nil {
+		t.Fatal(err)
+	}
+	for _, dest := range []string{full, link, cwd, filepath.Join(base, ".git", "out")} {
+		root, _, err := authz.OpenArtifactGetDest(dest, cwd)
+		if root != nil {
+			root.Close()
+		}
+		if err == nil {
+			t.Fatalf("accepted %s", dest)
+		}
+	}
+}