fix(thothctl): harden workspace result and candidate publication

tools/thothctl/internal/safeio/files_linux.go

@@ -0,0 +1,266 @@
+//go:build linux
+
+package safeio
+
+import (
+	"crypto/rand"
+	"encoding/hex"
+	"io/fs"
+	"os"
+	"strings"
+
+	"golang.org/x/sys/unix"
+)
+
+// ReadCanonicalRegular opens an absolute canonical path component by component from the root
+// descriptor. O_NOFOLLOW rejects symlinks at every component, and the open directory descriptors
+// prevent later parent replacement from redirecting the final open.
+func ReadCanonicalRegular(path string, maximum int64) ([]byte, error) {
+	if err := ValidateCanonicalPath(path); err != nil {
+		return nil, err
+	}
+	components := strings.Split(strings.TrimPrefix(path, string(os.PathSeparator)), string(os.PathSeparator))
+	if len(components) == 0 || components[0] == "" {
+		return nil, ErrUnsafeFile
+	}
+
+	directory, err := unix.Open(string(os.PathSeparator), unix.O_RDONLY|unix.O_CLOEXEC|unix.O_DIRECTORY, 0)
+	if err != nil {
+		return nil, ErrUnsafeFile
+	}
+	directories := []int{directory}
+	defer func() { closeUnixDescriptors(directories) }()
+
+	for _, component := range components[:len(components)-1] {
+		nextDirectory, err := unix.Openat(directory, component, unix.O_RDONLY|unix.O_CLOEXEC|unix.O_DIRECTORY|unix.O_NOFOLLOW, 0)
+		if err != nil {
+			return nil, ErrUnsafeFile
+		}
+		directory = nextDirectory
+		directories = append(directories, directory)
+	}
+
+	descriptor, err := unix.Openat(directory, components[len(components)-1], unix.O_RDONLY|unix.O_CLOEXEC|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0)
+	if err != nil {
+		return nil, ErrUnsafeFile
+	}
+	file := os.NewFile(uintptr(descriptor), "thothctl-safeio")
+	if file == nil {
+		unix.Close(descriptor)
+		return nil, ErrUnsafeFile
+	}
+	defer file.Close()
+	var before, after unix.Stat_t
+	if err := unix.Fstat(int(file.Fd()), &before); err != nil || before.Nlink > 1 || before.Mode&unix.S_IFMT != unix.S_IFREG {
+		return nil, ErrUnsafeFile
+	}
+	contents, err := readBoundedRegularFile(file, maximum)
+	if err != nil {
+		return nil, ErrUnsafeFile
+	}
+	// Checking the retained descriptor alone misses a pathname replacement while the
+	// read is in progress.  The parent descriptor and final name must still resolve to
+	// the exact open file after reading.
+	if err := unix.Fstat(int(file.Fd()), &after); err != nil || after.Nlink > 1 || after.Mode != before.Mode || after.Ino != before.Ino || after.Dev != before.Dev || after.Size != before.Size {
+		return nil, ErrUnsafeFile
+	}
+	var named unix.Stat_t
+	if err := unix.Fstatat(directory, components[len(components)-1], &named, unix.AT_SYMLINK_NOFOLLOW); err != nil || named.Nlink > 1 || named.Mode != before.Mode || named.Ino != before.Ino || named.Dev != before.Dev {
+		return nil, ErrUnsafeFile
+	}
+	if !recheckUnixParents(components, directories) {
+		return nil, ErrUnsafeFile
+	}
+	return contents, nil
+}
+
+func closeUnixDescriptors(descriptors []int) {
+	for _, descriptor := range descriptors {
+		unix.Close(descriptor)
+	}
+}
+
+func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) error {
+	if err := ValidateCanonicalPath(path); err != nil || len(contents) > 16<<20 || mode.Perm() != 0o600 {
+		return ErrUnsafeFile
+	}
+	components := strings.Split(strings.TrimPrefix(path, string(os.PathSeparator)), string(os.PathSeparator))
+	if len(components) == 0 || components[0] == "" {
+		return ErrUnsafeFile
+	}
+	dir, err := unix.Open("/", unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
+	if err != nil {
+		return ErrUnsafeFile
+	}
+	// Every superseded descriptor is closed in the traversal. Capture the
+	// variable, rather than its initial value, so the final parent is closed too.
+	defer func() { _ = unix.Close(dir) }()
+	for _, component := range components[:len(components)-1] {
+		next, openErr := unix.Openat(dir, component, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0)
+		if openErr != nil {
+			return ErrUnsafeFile
+		}
+		_ = unix.Close(dir)
+		dir = next
+	}
+	if !recheckUnixParentPath(components[:len(components)-1], dir) {
+		return ErrUnsafeFile
+	}
+
+	// O_TMPFILE creates an inode with no directory entry. Consequently no
+	// same-identity actor can discover or hard-link candidate bytes while they
+	// are being written. AT_EMPTY_PATH then links that already-synced inode into
+	// the destination in one no-replace operation.
+	fd, err := unix.Openat(dir, ".", unix.O_RDWR|unix.O_CLOEXEC|unix.O_TMPFILE, uint32(mode.Perm()))
+	if err != nil {
+		return ErrUnsafeFile
+	}
+	file := os.NewFile(uintptr(fd), "thothctl-safeio-anonymous")
+	if file == nil {
+		_ = unix.Close(fd)
+		return ErrUnsafeFile
+	}
+	closed := false
+	closeFile := func() error {
+		if closed {
+			return nil
+		}
+		closed = true
+		return file.Close()
+	}
+	var expected unix.Stat_t
+	if err := unix.Fstat(fd, &expected); err != nil || expected.Nlink != 0 || expected.Mode&unix.S_IFMT != unix.S_IFREG {
+		_ = closeFile()
+		return ErrUnsafeFile
+	}
+	cleanup := func() {
+		// expected is immutable. Never use a post-race stat as the identity to
+		// remove: a replacement at the public name must survive our cleanup.
+		var current unix.Stat_t
+		leaf := components[len(components)-1]
+		if unix.Fstatat(dir, leaf, &current, unix.AT_SYMLINK_NOFOLLOW) == nil && current.Ino == expected.Ino && current.Dev == expected.Dev {
+			_ = unix.Unlinkat(dir, leaf, 0)
+		}
+	}
+	fail := func() error {
+		_ = closeFile()
+		cleanup()
+		return ErrUnsafeFile
+	}
+	if err := file.Chmod(mode); err != nil {
+		return fail()
+	}
+	n, err := file.Write(contents)
+	if err != nil || n != len(contents) {
+		return fail()
+	}
+	if err := file.Sync(); err != nil {
+		return fail()
+	}
+	var after unix.Stat_t
+	if err := unix.Fstat(fd, &after); err != nil || after.Nlink != 0 || after.Mode&unix.S_IFMT != unix.S_IFREG || after.Size != int64(len(contents)) {
+		return fail()
+	}
+	if !recheckUnixParentPath(components[:len(components)-1], dir) {
+		return fail()
+	}
+	leaf := components[len(components)-1]
+	if err := unix.Linkat(fd, "", dir, leaf, unix.AT_EMPTY_PATH); err != nil {
+		return fail()
+	}
+	var published unix.Stat_t
+	if err := unix.Fstatat(dir, leaf, &published, unix.AT_SYMLINK_NOFOLLOW); err != nil || published.Ino != expected.Ino || published.Dev != expected.Dev || published.Nlink != 1 {
+		return fail()
+	}
+	if err := unix.Fsync(dir); err != nil || !recheckUnixParentPath(components[:len(components)-1], dir) {
+		return fail()
+	}
+	if err := closeFile(); err != nil {
+		cleanup()
+		return ErrUnsafeFile
+	}
+	return nil
+}
+
+func privateStageName() (string, error) {
+	var random [16]byte
+	if _, err := rand.Read(random[:]); err != nil {
+		return "", err
+	}
+	return ".thothctl-candidate-" + hex.EncodeToString(random[:]), nil
+}
+
+func validateCanonicalOutputPath(path string) error {
+	if err := ValidateCanonicalPath(path); err != nil {
+		return err
+	}
+	components := strings.Split(strings.TrimPrefix(path, string(os.PathSeparator)), string(os.PathSeparator))
+	if len(components) < 2 || components[0] == "" {
+		return ErrUnsafeFile
+	}
+	dir, err := unix.Open("/", unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
+	if err != nil {
+		return ErrUnsafeFile
+	}
+	defer func() { unix.Close(dir) }()
+	for _, component := range components[:len(components)-1] {
+		next, err := unix.Openat(dir, component, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0)
+		if err != nil {
+			return ErrUnsafeFile
+		}
+		unix.Close(dir)
+		dir = next
+	}
+	var st unix.Stat_t
+	if err := unix.Fstatat(dir, components[len(components)-1], &st, unix.AT_SYMLINK_NOFOLLOW); err == nil {
+		return ErrUnsafeFile
+	} else if err != unix.ENOENT {
+		return ErrUnsafeFile
+	}
+	return nil
+}
+
+func recheckUnixParents(components []string, retained []int) bool {
+	if len(retained) != len(components) {
+		return false
+	}
+	dir, err := unix.Open("/", unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
+	if err != nil {
+		return false
+	}
+	defer func() { unix.Close(dir) }()
+	for i, component := range components[:len(components)-1] {
+		next, e := unix.Openat(dir, component, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0)
+		if e != nil {
+			return false
+		}
+		var got, want unix.Stat_t
+		if unix.Fstat(next, &got) != nil || unix.Fstat(retained[i+1], &want) != nil || got.Ino != want.Ino || got.Dev != want.Dev {
+			unix.Close(next)
+			return false
+		}
+		unix.Close(dir)
+		dir = next
+	}
+	return true
+}
+
+func recheckUnixParentPath(components []string, retained int) bool {
+	dir, err := unix.Open("/", unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
+	if err != nil {
+		return false
+	}
+	defer func() { unix.Close(dir) }()
+	for _, component := range components {
+		next, e := unix.Openat(dir, component, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0)
+		if e != nil {
+			return false
+		}
+		unix.Close(dir)
+		dir = next
+	}
+	var got, want unix.Stat_t
+	return unix.Fstat(dir, &got) == nil && unix.Fstat(retained, &want) == nil && got.Ino == want.Ino && got.Dev == want.Dev
+}
+
+func validatePlatformPathSyntax(path string) error { return nil }

tools/thothctl/internal/safeio/files_unix.go

@@ -1,4 +1,4 @@
-//go:build !windows
+//go:build !windows && !linux
 
 package safeio
 
@@ -92,7 +92,10 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err
 	if err != nil {
 		return ErrUnsafeFile
 	}
-	defer unix.Close(dir)
+	// Close the descriptor that owns the final retained parent. The traversal
+	// closes each superseded descriptor explicitly; evaluating unix.Close(dir)
+	// at defer time would close only the original root descriptor.
+	defer func() { _ = unix.Close(dir) }()
 	for _, component := range components[:len(components)-1] {
 		next, openErr := unix.Openat(dir, component, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC|unix.O_NOFOLLOW, 0)
 		if openErr != nil {
@@ -129,7 +132,8 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err
 		return ErrUnsafeFile
 	}
 	published := false
-	var publishedIdentity unix.Stat_t
+	// Keep the inode identity immutable across every check and cleanup path.
+	var publishedIdentity = staged
 	cleanup := func() {
 		if published {
 			var current unix.Stat_t
@@ -161,8 +165,12 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err
 	if err := stageFile.Sync(); err != nil {
 		return fail()
 	}
+	// On platforms without O_TMPFILE, a same-user actor can hard-link the
+	// nameable stage. Extra links are therefore not a post-write rejection:
+	// once observed, the bytes are already public and rejecting would leave the
+	// attacker's link behind. Cleanup still uses the immutable inode identity.
 	var afterWrite unix.Stat_t
-	if err := unix.Fstat(stageFD, &afterWrite); err != nil || afterWrite.Nlink != 1 || afterWrite.Mode&unix.S_IFMT != unix.S_IFREG || afterWrite.Size != int64(len(contents)) {
+	if err := unix.Fstat(stageFD, &afterWrite); err != nil || afterWrite.Nlink < 1 || afterWrite.Mode&unix.S_IFMT != unix.S_IFREG || afterWrite.Size != int64(len(contents)) {
 		return fail()
 	}
 	if err := stageFile.Close(); err != nil {
@@ -177,16 +185,17 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err
 		return fail()
 	}
 	published = true
-	publishedIdentity = staged
-	if err := unix.Fstatat(dir, components[len(components)-1], &publishedIdentity, unix.AT_SYMLINK_NOFOLLOW); err != nil ||
-		publishedIdentity.Ino != staged.Ino || publishedIdentity.Dev != staged.Dev || publishedIdentity.Nlink != 2 {
+	var linked unix.Stat_t
+	if err := unix.Fstatat(dir, components[len(components)-1], &linked, unix.AT_SYMLINK_NOFOLLOW); err != nil ||
+		linked.Ino != staged.Ino || linked.Dev != staged.Dev || linked.Nlink < 2 {
 		return fail()
 	}
 	if err := unix.Unlinkat(dir, stage, 0); err != nil {
 		return fail()
 	}
 	stageCreated = false
-	if err := unix.Fstatat(dir, components[len(components)-1], &publishedIdentity, unix.AT_SYMLINK_NOFOLLOW); err != nil || publishedIdentity.Nlink != 1 {
+	var finalIdentity unix.Stat_t
+	if err := unix.Fstatat(dir, components[len(components)-1], &finalIdentity, unix.AT_SYMLINK_NOFOLLOW); err != nil || finalIdentity.Ino != publishedIdentity.Ino || finalIdentity.Dev != publishedIdentity.Dev || finalIdentity.Nlink < 1 {
 		return fail()
 	}
 	if err := unix.Fsync(dir); err != nil || !recheckUnixParentPath(components[:len(components)-1], dir) {

tools/thothctl/internal/safeio/files_windows.go

@@ -186,8 +186,11 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err
 	if err := stageFile.Sync(); err != nil {
 		return fail()
 	}
+	// The stage name is visible on Win32. If a same-user actor hard-links it,
+	// the bytes are already public; do not turn that observation into a failure
+	// whose cleanup could not remove the attacker's link.
 	var afterWrite windows.ByHandleFileInformation
-	if err := windows.GetFileInformationByHandle(stageHandle, &afterWrite); err != nil || afterWrite.NumberOfLinks != 1 || afterWrite.FileSizeHigh != uint32(uint64(len(contents))>>32) || afterWrite.FileSizeLow != uint32(len(contents)) {
+	if err := windows.GetFileInformationByHandle(stageHandle, &afterWrite); err != nil || afterWrite.NumberOfLinks == 0 || afterWrite.FileSizeHigh != uint32(uint64(len(contents))>>32) || afterWrite.FileSizeLow != uint32(len(contents)) {
 		return fail()
 	}
 	if err := stageFile.Close(); err != nil {
@@ -203,16 +206,15 @@ func writeCanonicalExclusive(path string, contents []byte, mode fs.FileMode) err
 	published = true
 	publishedIdentity = staged
 	check, identityErr := windowsFileIdentity(finalPath)
-	if identityErr != nil || check.NumberOfLinks != 2 || !sameWindowsFile(staged, check) {
+	if identityErr != nil || check.NumberOfLinks < 2 || !sameWindowsFile(staged, check) {
 		return fail()
 	}
-	publishedIdentity = check
 	if err := windows.DeleteFile(windows.StringToUTF16Ptr(stagePath)); err != nil {
 		return fail()
 	}
 	stageCreated = false
 	finalIdentity, identityErr := windowsFileIdentity(finalPath)
-	if identityErr != nil || finalIdentity.NumberOfLinks != 1 || !sameWindowsFile(staged, finalIdentity) {
+	if identityErr != nil || finalIdentity.NumberOfLinks == 0 || !sameWindowsFile(staged, finalIdentity) {
 		return fail()
 	}
 	return nil
@@ -292,13 +294,39 @@ func validatePlatformPathSyntax(path string) error {
 	}
 	volume := filepath.VolumeName(path)
 	for _, component := range strings.Split(strings.TrimPrefix(path, volume+string(filepath.Separator)), string(filepath.Separator)) {
-		if strings.Contains(component, ":") {
+		if strings.Contains(component, ":") || strings.HasSuffix(component, " ") || strings.HasSuffix(component, ".") || isWindowsDeviceComponent(component) {
 			return ErrUnsafeFile
 		}
 	}
 	return nil
 }
 
+// Win32 aliases these names to devices, even when an extension is appended.
+// Rejecting them lexically is required because CreateFile does not promise an
+// independent regular leaf for a path containing one of these components.
+func isWindowsDeviceComponent(component string) bool {
+	base := strings.ToUpper(component)
+	if i := strings.IndexByte(base, '.'); i >= 0 {
+		base = base[:i]
+	}
+	switch base {
+	case "CON", "PRN", "AUX", "NUL", "CONIN$", "CONOUT$":
+		return true
+	}
+	if len(base) == 4 && (strings.HasPrefix(base, "COM") || strings.HasPrefix(base, "LPT")) {
+		if base[3] >= '1' && base[3] <= '9' {
+			return true
+		}
+	}
+	// Unicode superscript 1, 2 and 3 are accepted as COM/LPT suffixes by
+	// Win32's device-name compatibility rules.
+	if len([]rune(base)) == 4 && (strings.HasPrefix(base, "COM") || strings.HasPrefix(base, "LPT")) {
+		suffix := []rune(base)[3]
+		return suffix == '¹' || suffix == '²' || suffix == '³'
+	}
+	return false
+}
+
 func sameWindowsFile(a, b windows.ByHandleFileInformation) bool {
 	return a.VolumeSerialNumber == b.VolumeSerialNumber && a.FileIndexHigh == b.FileIndexHigh && a.FileIndexLow == b.FileIndexLow
 }

tools/thothctl/internal/safeio/files_windows_test.go

@@ -23,7 +23,7 @@ var _ [windowsOutputHandleShareMode - expectedWindowsOutputHandleShareMode]struc
 var _ [expectedWindowsOutputHandleShareMode - windowsOutputHandleShareMode]struct{}
 
 func TestValidateCanonicalPathRejectsWindowsNamespacesAndAlternateStreams(t *testing.T) {
-	for _, path := range []string{`C:\dir\existing.txt:candidate`, `\\?\C:\dir\candidate`, `\\.\pipe\candidate`, `\Device\HarddiskVolume1\candidate`, `\??\C:\candidate`} {
+	for _, path := range []string{`C:\dir\existing.txt:candidate`, `\\?\C:\dir\candidate`, `\\.\pipe\candidate`, `\Device\HarddiskVolume1\candidate`, `\??\C:\candidate`, `C:\dir\NUL`, `C:\dir\nul.txt`, `C:\dir\COM1`, `C:\dir\LPT9.log`, `C:\dir\CONIN$`, `C:\dir\candidate.yaml.`, `C:\dir\candidate.yaml `} {
 		if err := ValidateCanonicalPath(path); !errors.Is(err, ErrUnsafeFile) {
 			t.Errorf("ValidateCanonicalPath(%q) = %v, want ErrUnsafeFile", path, err)
 		}

tools/thothctl/internal/workspaceops/operations.go

@@ -458,7 +458,19 @@ func validateIngress(payload []byte, expected inputEnvelope) error {
 	return nil
 }
 
+// ResultProjector is an optional host-side projection applied to the validated
+// child result before any host-side candidate is published. It is deliberately
+// a callback so workspaceops does not depend on the CLI's redaction policy.
+type ResultProjector func(Result) (Result, error)
+
+// Run preserves the original API for callers that do not need a public projection.
 func Run(ctx context.Context, installation config.Installation, runner compose.Runner, command Command, stdin io.Reader) (Result, error) {
+	return RunWithProjector(ctx, installation, runner, command, stdin, nil)
+}
+
+// RunWithProjector validates the optional projected envelope before publishing
+// any host export. This ordering is part of the workspace boundary contract.
+func RunWithProjector(ctx context.Context, installation config.Installation, runner compose.Runner, command Command, stdin io.Reader, projector ResultProjector) (Result, error) {
 	env, generated, e := makeInput(command)
 	if e != nil {
 		return Result{}, e
@@ -536,6 +548,13 @@ func Run(ctx context.Context, installation config.Installation, runner compose.R
 			return Result{}, runErr
 		}
 	}
+	if projector != nil {
+		projected, projectErr := projector(result)
+		if projectErr != nil || validateResult(projected, env.WorkspaceID, operationName(command)) != nil {
+			return Result{}, errors.New("invalid workspace result")
+		}
+		result = projected
+	}
 	if hasExport {
 		if export == nil {
 			return Result{}, errors.New("invalid candidate export")

tools/thothctl/internal/workspaceops/operations_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -616,3 +617,38 @@ func TestRunRejectsTrailingOrUnknownResultEnvelope(t *testing.T) {
 		})
 	}
 }
+
+func TestRunProjectsBeforePublishingCandidate(t *testing.T) {
+	root, err := filepath.EvalSymlinks(t.TempDir())
+	if err != nil {
+		t.Fatal(err)
+	}
+	candidate := []byte("candidates: []\n")
+	digest := DigestBytes(candidate)
+	result := validWorkspaceResult("succeeded", "ok")
+	result.Operation = "suggest-fks"
+	result.RunID = strings.Repeat("b", 32)
+	result.ArtifactIdentities = []ArtifactIdentity{{Kind: "fk-candidates", Digest: digest}}
+	envelope := map[string]any{"schemaVersion": result.SchemaVersion, "status": result.Status, "code": result.Code,
+		"workspaceId": result.WorkspaceID, "workspaceRevision": result.WorkspaceRevision, "descriptorBlob": result.DescriptorBlob,
+		"operation": result.Operation, "runId": result.RunID, "completedStages": []string{"safe\nforged"},
+		"artifactIdentities": result.ArtifactIdentities,
+		"hostExport":         hostExport{MediaType: "application/yaml", SHA256: digest, ContentBase64: base64.StdEncoding.EncodeToString(candidate)}}
+	payload, err := json.Marshal(envelope)
+	if err != nil {
+		t.Fatal(err)
+	}
+	fake := filepath.Join(root, "docker")
+	if err := os.WriteFile(fake, []byte("#!/bin/sh\ncat >/dev/null\nprintf '%s' '"+string(payload)+"'\n"), 0o700); err != nil {
+		t.Fatal(err)
+	}
+	output := filepath.Join(root, "candidate.yaml")
+	_, err = RunWithProjector(context.Background(), config.Installation{ProjectDirectory: root}, compose.NewRunner(fake), SuggestFksRequest{WorkspaceID: "psd", Output: output}, nil,
+		func(result Result) (Result, error) { return Result{}, errors.New("unsafe projected result") })
+	if err == nil {
+		t.Fatal("accepted a rejected public projection")
+	}
+	if _, statErr := os.Stat(output); !os.IsNotExist(statErr) {
+		t.Fatalf("candidate was published before projection rejection: %v", statErr)
+	}
+}