fix(attachments): contain file creation and cleanup with directory handles

Problem: CodeQL reports uncontrolled attachment paths in creation, interrupted
uploads and stale desktop-owner cleanup. A deterministic directory replacement
can make cleanup delete an external same-name file and leave the owned partial.

Root cause: validation and later absolute-path operations resolve the attachment
directory independently, so they do not retain one filesystem owner during I/O.

Fix: anchor creation to the workspace with os.Root, retain the attachment
directory handle through byte/stream write cleanup, and use rooted removal for
stale desktop writes and failed image previews. Preserve missing workspace
creation, extension validation, exclusive file creation and existing limits.

Verification: controller suite, attachment desktop regressions, deterministic
read-error/cancellation races, focused race detector, vet, pinned linter and
repolint passed. Native Windows 11 ARM64 controller/desktop regressions passed,
including junction escape rejection, protected cleanup and 80 MiB streaming.
The independent recovery catalog failure also reproduces with the PR's Go
changes removed through a base-source overlay; it is not changed here.

desktop/attachment_cleanup_security_test.go

@@ -0,0 +1,56 @@
+package main
+
+import (
+	"encoding/base64"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"runtime"
+	"testing"
+)
+
+func TestStaleAttachmentCleanupDoesNotFollowSwappedDirectory(t *testing.T) {
+	a, root, _ := uploadFixture(t)
+	outside := t.TempDir()
+	attachments := filepath.Join(root, ".reasonix", "attachments")
+	want := []byte("external victim must survive stale attachment cleanup")
+	var victim string
+	a.attachmentIOHook = func() {
+		entries, err := os.ReadDir(attachments)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if len(entries) != 1 || entries[0].IsDir() {
+			t.Fatalf("saved attachment entries = %v", entries)
+		}
+		victim = filepath.Join(outside, entries[0].Name())
+		if err := os.WriteFile(victim, want, 0o600); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.Rename(attachments, filepath.Join(root, ".reasonix", "original-attachments")); err != nil {
+			t.Fatal(err)
+		}
+		if runtime.GOOS == "windows" {
+			if out, err := exec.Command("cmd", "/c", "mklink", "/J", attachments, outside).CombinedOutput(); err != nil {
+				t.Fatalf("create attachment junction: %v: %s", err, out)
+			}
+		} else if err := os.Symlink(outside, attachments); err != nil {
+			t.Fatal(err)
+		}
+		a.mu.Lock()
+		a.tabs["a"].SessionGeneration++
+		a.mu.Unlock()
+	}
+
+	data := "data:application/octet-stream;base64," + base64.StdEncoding.EncodeToString([]byte("saved APK fixture"))
+	if ref, err := a.SavePastedFileForTab("a", "app.apk", data); err == nil || ref != "" {
+		t.Fatalf("stale write returned ref %q, err %v", ref, err)
+	}
+	got, err := os.ReadFile(victim)
+	if err != nil {
+		t.Fatalf("stale-owner cleanup deleted the external victim through the swapped attachment directory: %v", err)
+	}
+	if string(got) != string(want) {
+		t.Fatalf("external victim changed: %q", got)
+	}
+}

desktop/attachments_app.go

@@ -168,7 +168,7 @@ func (a *App) finishAttachmentWrite(target attachmentTarget, rel string, err err
 		return rel, nil
 	}
 	if isAttachmentRefPath(rel) {
-		_ = os.Remove(filepath.Join(target.root, filepath.FromSlash(rel)))
+		_ = control.RemoveStoredAttachment(target.root, rel)
 	}
 	return "", fmt.Errorf("attachment workspace changed while saving; please retry")
 }
@@ -442,7 +442,7 @@ func (a *App) attachDroppedForTarget(target attachmentTarget, path string) (Drop
 			}
 			preview, previewErr := a.attachmentDataURLForTarget(target, rel)
 			if previewErr != nil {
-				_ = os.Remove(filepath.Join(target.root, filepath.FromSlash(rel)))
+				_ = control.RemoveStoredAttachment(target.root, rel)
 				return DroppedItem{}, previewErr
 			}
 			return DroppedItem{Kind: "attachment", Path: rel, PreviewURL: preview}, nil

internal/control/attachment_storage.go

@@ -0,0 +1,107 @@
+package control
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+)
+
+type ownedAttachmentFile struct {
+	*os.File
+	directory *os.Root
+	name      string
+	rel       string
+	complete  bool
+}
+
+// Hold the directory handle through cleanup: re-resolving its path after a
+// rename or symlink replacement could remove a different file.
+func createOwnedAttachmentFile(base, ext string) (*ownedAttachmentFile, error) {
+	directory, err := openAttachmentRootIn(base)
+	if err != nil {
+		return nil, err
+	}
+
+	if !safeAttachmentExt.MatchString(ext) {
+		ext = ".bin"
+	}
+	for range maxAttachmentCreateAttempts {
+		name := filepath.Base(attachmentPath(ext))
+		f, err := directory.OpenFile(name, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644)
+		if os.IsExist(err) {
+			continue
+		}
+		if err != nil {
+			_ = directory.Close()
+			return nil, err
+		}
+		return &ownedAttachmentFile{File: f, directory: directory, name: name,
+			rel: filepath.Join(".reasonix", "attachments", name)}, nil
+	}
+	_ = directory.Close()
+	return nil, fmt.Errorf("create unique attachment path")
+}
+
+func (f *ownedAttachmentFile) finish() error {
+	if err := f.Close(); err != nil {
+		return err
+	}
+	f.complete = true
+	return nil
+}
+
+func (f *ownedAttachmentFile) cleanup() {
+	_ = f.Close()
+	if !f.complete {
+		_ = f.directory.Remove(f.name)
+	}
+	_ = f.directory.Close()
+}
+
+func openAttachmentRootIn(base string) (*os.Root, error) {
+	if strings.TrimSpace(base) == "" {
+		base = "."
+	}
+	if err := os.MkdirAll(base, 0o755); err != nil {
+		return nil, err
+	}
+	workspace, err := os.OpenRoot(base)
+	if err != nil {
+		return nil, err
+	}
+	defer workspace.Close()
+	for _, name := range []string{".reasonix", filepath.Join(".reasonix", "attachments")} {
+		if err := workspace.Mkdir(name, 0o755); err != nil && !os.IsExist(err) {
+			return nil, err
+		}
+		info, err := workspace.Lstat(name)
+		if err != nil {
+			return nil, err
+		}
+		if info.Mode()&os.ModeSymlink != 0 || !info.IsDir() {
+			return nil, fmt.Errorf("attachment directory must be a directory without symlinks")
+		}
+	}
+	return workspace.OpenRoot(filepath.Join(".reasonix", "attachments"))
+}
+
+// RemoveStoredAttachment removes a direct attachment reference without following
+// a replaced workspace path outside its root.
+func RemoveStoredAttachment(base, ref string) error {
+	clean := filepath.Clean(filepath.FromSlash(ref))
+	if filepath.Dir(clean) != filepath.Join(".reasonix", "attachments") {
+		return fmt.Errorf("attachment path is outside .reasonix/attachments")
+	}
+	workspace, err := os.OpenRoot(base)
+	if err != nil {
+		return err
+	}
+	defer workspace.Close()
+	directory, err := workspace.OpenRoot(filepath.Join(".reasonix", "attachments"))
+	if err != nil {
+		return err
+	}
+	defer directory.Close()
+	return directory.Remove(filepath.Base(clean))
+}

internal/control/attachment_storage_security_test.go

@@ -0,0 +1,141 @@
+package control
+
+import (
+	"context"
+	"errors"
+	"io"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+)
+
+type attachmentSecurityReader func([]byte) (int, error)
+
+func (read attachmentSecurityReader) Read(p []byte) (int, error) { return read(p) }
+
+func attachmentStorageDirectoryLink(t *testing.T, target, link string) {
+	t.Helper()
+	if runtime.GOOS == "windows" {
+		if out, err := exec.Command("cmd", "/c", "mklink", "/J", link, target).CombinedOutput(); err != nil {
+			t.Fatalf("create directory junction: %v: %s", err, out)
+		}
+		return
+	}
+	if err := os.Symlink(target, link); err != nil {
+		if errors.Is(err, errors.ErrUnsupported) {
+			t.Skipf("symlinks unavailable: %v", err)
+		}
+		t.Fatalf("create directory symlink: %v", err)
+	}
+}
+
+func TestAttachmentStorageRejectsSymlinkedReasonixParent(t *testing.T) {
+	attachmentStorageDirectoryLink(t, t.TempDir(), filepath.Join(t.TempDir(), "probe"))
+	for _, transport := range []string{"stream", "legacy-bytes"} {
+		t.Run(transport, func(t *testing.T) {
+			root, outside := t.TempDir(), t.TempDir()
+			attachmentStorageDirectoryLink(t, outside, filepath.Join(root, ".reasonix"))
+			var path string
+			var err error
+			if transport == "stream" {
+				path, err = SaveAttachmentStreamInRoot(context.Background(), root, "app.apk", strings.NewReader("apk bytes"))
+			} else {
+				path, err = SaveAttachmentBytesInRoot(root, "app.apk", []byte("apk bytes"))
+			}
+			if err == nil || path != "" {
+				t.Errorf("symlinked workspace storage accepted: path=%q error=%v", path, err)
+			}
+			entries, readErr := os.ReadDir(outside)
+			if readErr != nil {
+				t.Fatal(readErr)
+			}
+			if len(entries) != 0 {
+				t.Errorf("attachment creation escaped the workspace: outside entries=%v", entries)
+			}
+		})
+	}
+}
+
+func TestAttachmentStorageCleanupKeepsOriginalDirectoryOwner(t *testing.T) {
+	attachmentStorageDirectoryLink(t, t.TempDir(), filepath.Join(t.TempDir(), "probe"))
+	for _, failure := range []string{"read-error", "cancel"} {
+		t.Run(failure, func(t *testing.T) {
+			root, outside := t.TempDir(), t.TempDir()
+			directory := filepath.Join(root, ".reasonix", "attachments")
+			retained := filepath.Join(root, ".reasonix", "retained-attachments")
+			cleanupDirectory := retained
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+			readFailure := errors.New("attachment source failed")
+			wantErr := readFailure
+			if failure == "cancel" {
+				wantErr = context.Canceled
+			}
+			const victimContent = "outside file must survive upload cleanup"
+			var basename string
+			reads := 0
+			reader := attachmentSecurityReader(func(p []byte) (int, error) {
+				reads++
+				if reads == 1 {
+					return copy(p, "partially uploaded apk"), nil
+				}
+				if reads > 2 {
+					return 0, io.EOF
+				}
+				entries, err := os.ReadDir(directory)
+				if err != nil {
+					t.Fatal(err)
+				}
+				var partial []os.DirEntry
+				for _, entry := range entries {
+					if !entry.IsDir() && filepath.Ext(entry.Name()) == ".apk" {
+						partial = append(partial, entry)
+					}
+				}
+				if len(partial) != 1 {
+					t.Fatalf("expected one opened partial APK, got %v", entries)
+				}
+				basename = partial[0].Name()
+				partialBytes, err := os.ReadFile(filepath.Join(directory, basename))
+				if err != nil || string(partialBytes) != "partially uploaded apk" {
+					t.Fatalf("partial bytes must be saved before replacement: bytes=%q error=%v", partialBytes, err)
+				}
+				if err := os.WriteFile(filepath.Join(outside, basename), []byte(victimContent), 0o600); err != nil {
+					t.Fatal(err)
+				}
+				if err := os.Rename(directory, retained); err != nil {
+					if runtime.GOOS != "windows" || !errors.Is(err, os.ErrPermission) {
+						t.Fatal(err)
+					}
+					// Windows may prevent replacement while the upload handle is open.
+					cleanupDirectory = directory
+					t.Log("open upload handle refused directory replacement")
+				} else {
+					attachmentStorageDirectoryLink(t, outside, directory)
+				}
+				if failure == "cancel" {
+					cancel()
+				}
+				return 0, wantErr
+			})
+
+			path, err := SaveAttachmentStreamInRoot(ctx, root, "app.apk", reader)
+			if path != "" || !errors.Is(err, wantErr) {
+				t.Errorf("failed upload result: path=%q error=%v; want %v", path, err, wantErr)
+			}
+			if reads != 2 || basename == "" {
+				t.Fatalf("replacement did not run after the file opened: reads=%d name=%q", reads, basename)
+			}
+			victim, err := os.ReadFile(filepath.Join(outside, basename))
+			if err != nil || string(victim) != victimContent {
+				t.Errorf("cleanup touched an outside file: bytes=%q error=%v", victim, err)
+			}
+			if _, err := os.Lstat(filepath.Join(cleanupDirectory, basename)); !errors.Is(err, os.ErrNotExist) {
+				t.Errorf("cleanup did not remove the owned partial file: %v", err)
+			}
+		})
+	}
+}

internal/control/attachment_stream.go

@@ -4,38 +4,26 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"os"
 	"path/filepath"
 	"strings"
 )
 
 // SaveAttachmentStreamInRoot stores ordinary file bytes without the legacy
 // data-URL size cap. A 64 KiB buffer bounds memory, not total stored bytes.
-func SaveAttachmentStreamInRoot(ctx context.Context, root, name string, source io.Reader) (path string, err error) {
+func SaveAttachmentStreamInRoot(ctx context.Context, root, name string, source io.Reader) (string, error) {
 	if err := ctx.Err(); err != nil {
 		return "", err
 	}
-	abs, err := filepath.Abs(root)
-	if err != nil {
-		return "", err
-	}
-	if err := ensureAttachmentRootIn(abs); err != nil {
-		return "", err
-	}
 	ext := strings.ToLower(filepath.Ext(name))
 	if !safeAttachmentExt.MatchString(ext) {
 		ext = ".bin"
 	}
-	rel, f, err := createAttachmentFileIn(abs, ext)
+	owned, err := createOwnedAttachmentFile(root, ext)
 	if err != nil {
 		return "", err
 	}
-	defer func() {
-		_ = f.Close()
-		if err != nil {
-			_ = os.Remove(filepath.Join(abs, rel))
-		}
-	}()
+	defer owned.cleanup()
+
 	buf := make([]byte, 64<<10)
 	var total int64
 	for {
@@ -47,7 +35,7 @@ func SaveAttachmentStreamInRoot(ctx context.Context, root, name string, source i
 			if err := ctx.Err(); err != nil {
 				return "", err
 			}
-			if written, err := f.Write(buf[:n]); err != nil {
+			if written, err := owned.Write(buf[:n]); err != nil {
 				return "", err
 			} else if written != n {
 				return "", io.ErrShortWrite
@@ -67,8 +55,8 @@ func SaveAttachmentStreamInRoot(ctx context.Context, root, name string, source i
 	if total == 0 {
 		return "", fmt.Errorf("attachment is empty")
 	}
-	if err := f.Close(); err != nil {
+	if err := owned.finish(); err != nil {
 		return "", err
 	}
-	return filepath.ToSlash(rel), nil
+	return filepath.ToSlash(owned.rel), nil
 }

internal/control/attachments.go

@@ -165,34 +165,20 @@ func SaveImageBytesInRoot(root, declaredMime string, raw []byte) (string, error)
 }
 
 func saveAttachmentBytesInRoot(root, ext string, raw []byte) (string, error) {
-	if strings.TrimSpace(root) == "" {
-		root = "."
-	}
-	absRoot, err := filepath.Abs(root)
+	owned, err := createOwnedAttachmentFile(root, ext)
 	if err != nil {
 		return "", err
 	}
-	if err := ensureAttachmentRootIn(absRoot); err != nil {
-		return "", err
-	}
-	rel, f, err := createAttachmentFileIn(absRoot, ext)
-	if err != nil {
-		return "", err
-	}
-	if n, err := f.Write(raw); err != nil {
-		_ = f.Close()
-		_ = os.Remove(filepath.Join(absRoot, rel))
+	defer owned.cleanup()
+	if n, err := owned.Write(raw); err != nil {
 		return "", err
 	} else if n != len(raw) {
-		_ = f.Close()
-		_ = os.Remove(filepath.Join(absRoot, rel))
 		return "", io.ErrShortWrite
 	}
-	if err := f.Close(); err != nil {
-		_ = os.Remove(filepath.Join(absRoot, rel))
+	if err := owned.finish(); err != nil {
 		return "", err
 	}
-	return filepath.ToSlash(rel), nil
+	return filepath.ToSlash(owned.rel), nil
 }
 
 func SaveImageFile(path string) (string, error) {
@@ -588,39 +574,11 @@ func ensureAttachmentRoot() error {
 }
 
 func ensureAttachmentRootIn(base string) error {
-	if info, err := os.Lstat(filepath.Join(base, ".reasonix")); err == nil {
-		if info.Mode()&os.ModeSymlink != 0 {
-			return fmt.Errorf("attachment path must not contain symlinks")
-		}
-		if !info.IsDir() {
-			return fmt.Errorf("attachment path exists but is not a directory")
-		}
-	} else if !os.IsNotExist(err) {
-		return err
-	}
-	root := filepath.Join(base, ".reasonix", "attachments")
-	if info, err := os.Lstat(root); err == nil {
-		if info.Mode()&os.ModeSymlink != 0 {
-			return fmt.Errorf("attachment directory must not be a symlink")
-		}
-		if !info.IsDir() {
-			return fmt.Errorf("attachment path exists but is not a directory")
-		}
-		return nil
-	} else if !os.IsNotExist(err) {
-		return err
-	}
-	if err := os.MkdirAll(root, 0o755); err != nil {
-		return err
-	}
-	info, err := os.Lstat(root)
+	directory, err := openAttachmentRootIn(base)
 	if err != nil {
 		return err
 	}
-	if info.Mode()&os.ModeSymlink != 0 || !info.IsDir() {
-		return fmt.Errorf("attachment directory is invalid")
-	}
-	return nil
+	return directory.Close()
 }
 
 func validateAttachmentDirectories(base string) error {
@@ -739,18 +697,12 @@ func createAttachmentFile(ext string) (string, *os.File, error) {
 }
 
 func createAttachmentFileIn(base, ext string) (string, *os.File, error) {
-	for range maxAttachmentCreateAttempts {
-		rel := attachmentPath(ext)
-		f, err := os.OpenFile(filepath.Join(base, rel), os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644)
-		if os.IsExist(err) {
-			continue
-		}
-		if err != nil {
-			return "", nil, err
-		}
-		return rel, f, nil
+	owned, err := createOwnedAttachmentFile(base, ext)
+	if err != nil {
+		return "", nil, err
 	}
-	return "", nil, fmt.Errorf("create unique attachment path")
+	_ = owned.directory.Close()
+	return owned.rel, owned.File, nil
 }
 
 func attachmentPath(ext string) string {