diff --git a/docs/internal/fileops.md b/docs/internal/fileops.md index 09b2943..ebd1c2e 100644 --- a/docs/internal/fileops.md +++ b/docs/internal/fileops.md @@ -4,6 +4,20 @@ byte file. Callers keep ownership of serialization, validation, cancellation, and destination-directory policy. +## Destination Confinement + +Before it creates, replaces, or installs a destination file, `fileops` opens +each ancestor from the filesystem root and rejects symbolic links or components +that change during traversal. The resulting parent-directory handle is retained +for sibling temporary-file creation and rename, so a later pathname swap cannot +redirect the replacement. Existing destination symlinks are replaced as leaf +entries; their targets are never followed. + +Remote object acquisition uses a writer supplied by the storage owner. The +writer receives a `fileops`-owned, already-open sibling temporary file rather +than a mutable destination path. Callers still own remote object selection, +validation, conflict handling, and final mode. + ## Replacement Contract `ReplaceFileAtomic` requires an existing destination directory. It creates a diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 6f8703f..bf38a32 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -19,7 +19,7 @@ All stages are pending when this plan is created. | 1 | Align data classification and group workspace modes | RSK-004 | Completed | | 2 | Enforce safe identifiers and fuzz path/source contracts | COR-002, TST-013 | Completed | | 3 | Consolidate crash-durable atomic file replacement | RSK-002, DUP-001, DUP-005 | Completed | -| 4 | Add confined destination and download/install capabilities | COR-003, DUP-003, TST-003 | Pending | +| 4 | Add confined destination and download/install capabilities | COR-003, DUP-003, TST-003 | Completed | | 5 | Confine recursive cleanup and replace sentinel locks | RSK-003 | Pending | | 6 | Harden API-key file acquisition | RSK-010 | Pending | | 7 | Bound and verify external result acquisition | RSK-013, TST-007 | Pending | diff --git a/internal/adapters/storage/download_writer.go b/internal/adapters/storage/download_writer.go new file mode 100644 index 0000000..c7c2bc6 --- /dev/null +++ b/internal/adapters/storage/download_writer.go @@ -0,0 +1,23 @@ +package storage + +import ( + "context" + "fmt" + "io" +) + +// WriterDownloader is implemented by storage backends that stream an object +// into a caller-owned file handle. +type WriterDownloader interface { + DownloadTo(ctx context.Context, key string, destination io.Writer) error +} + +// DownloadTo streams one object into destination. Destination-confined callers +// require this capability rather than granting a backend a mutable pathname. +func DownloadTo(ctx context.Context, store ObjectStore, key string, destination io.Writer) error { + writer, ok := store.(WriterDownloader) + if !ok { + return fmt.Errorf("object store does not support handle-confined downloads") + } + return writer.DownloadTo(ctx, key, destination) +} diff --git a/internal/adapters/storage/fake.go b/internal/adapters/storage/fake.go index 26b0e48..30187c2 100644 --- a/internal/adapters/storage/fake.go +++ b/internal/adapters/storage/fake.go @@ -3,6 +3,7 @@ package storage import ( "context" "fmt" + "io" "os" "path/filepath" "sort" @@ -87,34 +88,43 @@ func (f *FakeBackend) List(ctx context.Context, prefix string) ([]ObjectInfo, er return out, nil } -// Download writes one object to a local path. -func (f *FakeBackend) Download(ctx context.Context, key, localPath string) error { +// DownloadTo writes one object to a caller-owned destination writer. +func (f *FakeBackend) DownloadTo(ctx context.Context, key string, destination io.Writer) error { if err := ctx.Err(); err != nil { return err } if f.DownloadErr != nil { return f.DownloadErr } - if strings.TrimSpace(localPath) == "" { - return fmt.Errorf("download object: local path is required") + if destination == nil { + return fmt.Errorf("download object: destination writer is required") } obj, ok := f.Objects[normalizeObjectKey(key)] if !ok { return fmt.Errorf("download object %q: %w", key, os.ErrNotExist) } - f.Downloads = append(f.Downloads, FakeDownloadCall{ - Key: normalizeObjectKey(key), - LocalPath: localPath, - }) + f.Downloads = append(f.Downloads, FakeDownloadCall{Key: normalizeObjectKey(key)}) + if _, err := destination.Write(obj.Data); err != nil { + return fmt.Errorf("download object %q: write destination: %w", key, err) + } + return nil +} +// Download writes one object to a local path. +func (f *FakeBackend) Download(ctx context.Context, key, localPath string) error { + if strings.TrimSpace(localPath) == "" { + return fmt.Errorf("download object: local path is required") + } if err := os.MkdirAll(filepath.Dir(localPath), 0o755); err != nil { return fmt.Errorf("download object %q: create parent directory: %w", key, err) } - if err := os.WriteFile(localPath, obj.Data, 0o644); err != nil { - return fmt.Errorf("download object %q: write local file: %w", key, err) + destination, err := os.Create(localPath) + if err != nil { + return fmt.Errorf("download object %q: create local file: %w", key, err) } - return nil + defer destination.Close() + return f.DownloadTo(ctx, key, destination) } // Upload reads a local file and stores it under key. diff --git a/internal/adapters/storage/s3_backend.go b/internal/adapters/storage/s3_backend.go index 71b0b4f..bf517a2 100644 --- a/internal/adapters/storage/s3_backend.go +++ b/internal/adapters/storage/s3_backend.go @@ -150,11 +150,11 @@ func (b *S3Backend) List(ctx context.Context, prefix string) ([]ObjectInfo, erro return out, nil } -// Download retrieves one object to localPath, creating parent directories as needed. -func (b *S3Backend) Download(ctx context.Context, key, localPath string) error { +// DownloadTo retrieves one object into the caller-owned destination writer. +func (b *S3Backend) DownloadTo(ctx context.Context, key string, destination io.Writer) error { normalizedKey := normalizeObjectKey(key) - if strings.TrimSpace(localPath) == "" { - return fmt.Errorf("download object: local path is required") + if destination == nil { + return fmt.Errorf("download object: destination writer is required") } resp, err := b.client.GetObject(ctx, &s3.GetObjectInput{ @@ -166,20 +166,30 @@ func (b *S3Backend) Download(ctx context.Context, key, localPath string) error { } defer resp.Body.Close() + if _, err := io.Copy(destination, resp.Body); err != nil { + return fmt.Errorf("download object %q: copy body: %w", normalizedKey, err) + } + return nil +} + +// Download retrieves one object to localPath, creating parent directories as needed. +func (b *S3Backend) Download(ctx context.Context, key, localPath string) error { + if strings.TrimSpace(localPath) == "" { + return fmt.Errorf("download object: local path is required") + } if err := os.MkdirAll(filepath.Dir(localPath), 0o755); err != nil { - return fmt.Errorf("download object %q: create parent directory: %w", normalizedKey, err) + return fmt.Errorf("download object %q: create parent directory: %w", key, err) } dst, err := os.Create(localPath) if err != nil { - return fmt.Errorf("download object %q: create local file: %w", normalizedKey, err) + return fmt.Errorf("download object %q: create local file: %w", key, err) } defer dst.Close() - - if _, err := io.Copy(dst, resp.Body); err != nil { - return fmt.Errorf("download object %q: copy body: %w", normalizedKey, err) + if err := b.DownloadTo(ctx, key, dst); err != nil { + return err } if err := dst.Sync(); err != nil { - return fmt.Errorf("download object %q: sync local file: %w", normalizedKey, err) + return fmt.Errorf("download object %q: sync local file: %w", key, err) } return nil } diff --git a/internal/app/restore_execute.go b/internal/app/restore_execute.go index 7560828..28ab871 100644 --- a/internal/app/restore_execute.go +++ b/internal/app/restore_execute.go @@ -3,7 +3,7 @@ package app import ( "context" "fmt" - "os" + "io" "path/filepath" "strings" @@ -98,27 +98,35 @@ func executeRestoreDownloadAction( return executeRestoreAudioAction(ctx, cfg, safeLocalPath, action, store) } - tmpPath, err := downloadObjectToSiblingTemp(ctx, store, action.RemoteKey, safeLocalPath) + if err := fileops.EnsureWorkspaceDirectory(filepath.Dir(safeLocalPath)); err != nil { + return fmt.Errorf("create destination directory: %w", err) + } + temporary, err := fileops.DownloadToSiblingTemp(safeLocalPath, func(destination io.Writer) error { + return storage.DownloadTo(ctx, store, action.RemoteKey, destination) + }) if err != nil { return fmt.Errorf("download to temp file: %w", err) } - removeTmp := true - defer func() { - if removeTmp { - _ = os.Remove(tmpPath) - } - }() + defer func() { _ = temporary.Cleanup() }() if action.LocalRelativePath == config.PathManifestFile { - if err := validateRestoredManifest(ctx, cfg, current, tmpPath); err != nil { + file, err := temporary.Open() + if err != nil { + return fmt.Errorf("open restored manifest: %w", err) + } + err = validateRestoredManifest(ctx, cfg, current, file) + closeErr := file.Close() + if err != nil { return err } + if closeErr != nil { + return fmt.Errorf("close restored manifest: %w", closeErr) + } } - if err := fileops.InstallDownloadedTempFile(tmpPath, safeLocalPath, fileops.WorkspaceFileMode); err != nil { + if err := temporary.Install(filepath.Base(safeLocalPath), fileops.WorkspaceFileMode); err != nil { return fmt.Errorf("install file atomically: %w", err) } - removeTmp = false return nil } @@ -155,36 +163,9 @@ func executeRestoreAudioAction( return nil } -func downloadObjectToSiblingTemp(ctx context.Context, store storage.ObjectStore, remoteKey, destPath string) (string, error) { - if strings.TrimSpace(destPath) == "" { - return "", fmt.Errorf("destination path is required") - } - dir := filepath.Dir(destPath) - if err := fileops.EnsureWorkspaceDirectory(dir); err != nil { - return "", fmt.Errorf("create destination directory: %w", err) - } - base := filepath.Base(destPath) - tmp, err := os.CreateTemp(dir, "."+base+".restore-*.tmp") - if err != nil { - return "", fmt.Errorf("create temp file: %w", err) - } - tmpPath := tmp.Name() - if err := tmp.Close(); err != nil { - _ = os.Remove(tmpPath) - return "", fmt.Errorf("close temp file: %w", err) - } - - if err := store.Download(ctx, remoteKey, tmpPath); err != nil { - _ = os.Remove(tmpPath) - return "", err - } - - return tmpPath, nil -} - -func validateRestoredManifest(ctx context.Context, cfg *config.Config, current *RemoteCurrentState, path string) error { +func validateRestoredManifest(ctx context.Context, cfg *config.Config, current *RemoteCurrentState, source io.Reader) error { manifestStore := &manifest.LocalStore{} - m, err := manifestStore.Load(ctx, path) + m, err := manifestStore.LoadReader(ctx, source) if err != nil { return fmt.Errorf("validate manifest decode: %w", err) } diff --git a/internal/app/restore_execution_test.go b/internal/app/restore_execution_test.go index 225aca9..dbe4cb8 100644 --- a/internal/app/restore_execution_test.go +++ b/internal/app/restore_execution_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "io" "os" "path/filepath" "strings" @@ -581,6 +582,19 @@ func (s *stagedManifestDownloadStore) Download(ctx context.Context, key, localPa return s.delegate.Download(ctx, key, localPath) } +func (s *stagedManifestDownloadStore) DownloadTo(ctx context.Context, key string, destination io.Writer) error { + if strings.TrimSpace(key) == strings.TrimSpace(s.manifestKey) { + s.manifestReads++ + payload := s.secondManifest + if s.manifestReads <= 1 { + payload = s.firstManifest + } + _, err := destination.Write(payload) + return err + } + return storage.DownloadTo(ctx, s.delegate, key, destination) +} + func (s *stagedManifestDownloadStore) Upload(ctx context.Context, localPath, key string, opts storage.UploadOptions) (storage.ObjectInfo, error) { return s.delegate.Upload(ctx, localPath, key, opts) } diff --git a/internal/audio/s3_audio.go b/internal/audio/s3_audio.go index 0a4cf63..5a7b16f 100644 --- a/internal/audio/s3_audio.go +++ b/internal/audio/s3_audio.go @@ -3,6 +3,7 @@ package audio import ( "context" "fmt" + "io" "os" "path/filepath" "strings" @@ -142,29 +143,7 @@ func downloadObjectAtomic(ctx context.Context, store storage.ObjectStore, key, d if err := fileops.EnsureWorkspaceDirectory(dir); err != nil { return fmt.Errorf("create destination directory: %w", err) } - base := filepath.Base(destPath) - tmp, err := os.CreateTemp(dir, "."+base+".download-*.tmp") - if err != nil { - return fmt.Errorf("create temp file: %w", err) - } - tmpPath := tmp.Name() - if err := tmp.Close(); err != nil { - _ = os.Remove(tmpPath) - return fmt.Errorf("close temp file: %w", err) - } - removeTmp := true - defer func() { - if removeTmp { - _ = os.Remove(tmpPath) - } - }() - - if err := store.Download(ctx, key, tmpPath); err != nil { - return err - } - if err := fileops.InstallDownloadedTempFile(tmpPath, destPath, fileops.WorkspaceFileMode); err != nil { - return err - } - removeTmp = false - return nil + return fileops.DownloadAndInstall(destPath, fileops.WorkspaceFileMode, func(destination io.Writer) error { + return storage.DownloadTo(ctx, store, key, destination) + }) } diff --git a/internal/fileops/confined.go b/internal/fileops/confined.go new file mode 100644 index 0000000..ae328f8 --- /dev/null +++ b/internal/fileops/confined.go @@ -0,0 +1,413 @@ +package fileops + +import ( + "crypto/rand" + "encoding/hex" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strings" +) + +// openConfinedParent opens dst's parent without following a symbolic-link +// component. It starts from the filesystem root, so the resulting handle stays +// valid even if a pathname ancestor is later renamed or replaced. +func openConfinedParent(dst string, create bool, mode os.FileMode) (*os.Root, string, error) { + if strings.TrimSpace(dst) == "" { + return nil, "", fmt.Errorf("destination path is required") + } + abs, err := filepath.Abs(dst) + if err != nil { + return nil, "", fmt.Errorf("resolve destination path: %w", err) + } + parts, err := absolutePathParts(abs) + if err != nil { + return nil, "", err + } + if len(parts) == 0 { + return nil, "", fmt.Errorf("destination file name is required") + } + + rootPath := filesystemRoot(abs) + root, err := os.OpenRoot(rootPath) + if err != nil { + return nil, "", fmt.Errorf("open filesystem root %q: %w", rootPath, err) + } + for _, part := range parts[:len(parts)-1] { + child, err := openConfinedChild(root, part, create, mode) + if err != nil { + _ = root.Close() + return nil, "", err + } + _ = root.Close() + root = child + } + return root, parts[len(parts)-1], nil +} + +func ensureConfinedDirectory(directory string, mode os.FileMode) error { + abs, err := filepath.Abs(directory) + if err != nil { + return fmt.Errorf("resolve workspace directory: %w", err) + } + parts, err := absolutePathParts(abs) + if err != nil { + return err + } + rootPath := filesystemRoot(abs) + root, err := os.OpenRoot(rootPath) + if err != nil { + return fmt.Errorf("open filesystem root %q: %w", rootPath, err) + } + defer func() { _ = root.Close() }() + + for _, part := range parts { + child, err := openConfinedChild(root, part, true, mode) + if err != nil { + return err + } + _ = root.Close() + root = child + } + return setOpenedDirectoryMode(root, mode) +} + +func absolutePathParts(path string) ([]string, error) { + root := filesystemRoot(path) + relative, err := filepath.Rel(root, path) + if err != nil { + return nil, fmt.Errorf("resolve path below filesystem root: %w", err) + } + if relative == "." { + return nil, nil + } + parts := strings.FieldsFunc(relative, func(r rune) bool { return r == filepath.Separator || r == '/' || r == '\\' }) + for _, part := range parts { + if part == "" || part == "." || part == ".." { + return nil, fmt.Errorf("unsafe filesystem path component %q", part) + } + } + return parts, nil +} + +func filesystemRoot(path string) string { + volume := filepath.VolumeName(path) + if volume == "" { + return string(filepath.Separator) + } + return volume + string(filepath.Separator) +} + +func openConfinedChild(parent *os.Root, name string, create bool, mode os.FileMode) (*os.Root, error) { + info, err := parent.Lstat(name) + created := false + if errors.Is(err, os.ErrNotExist) { + if !create { + return nil, fmt.Errorf("destination ancestor %q does not exist", name) + } + if err := parent.Mkdir(name, mode.Perm()); err != nil && !errors.Is(err, os.ErrExist) { + return nil, fmt.Errorf("create destination ancestor %q: %w", name, err) + } + created = true + info, err = parent.Lstat(name) + } + if err != nil { + return nil, fmt.Errorf("inspect destination ancestor %q: %w", name, err) + } + if info.Mode()&os.ModeSymlink != 0 || !info.IsDir() { + return nil, fmt.Errorf("destination ancestor %q is not a regular directory", name) + } + + child, err := parent.OpenRoot(name) + if err != nil { + return nil, fmt.Errorf("open destination ancestor %q: %w", name, err) + } + opened, err := child.Stat(".") + if err != nil { + _ = child.Close() + return nil, fmt.Errorf("inspect opened destination ancestor %q: %w", name, err) + } + current, err := parent.Lstat(name) + if err != nil || current.Mode()&os.ModeSymlink != 0 || !current.IsDir() || !os.SameFile(opened, current) { + _ = child.Close() + if err != nil { + return nil, fmt.Errorf("reinspect destination ancestor %q: %w", name, err) + } + return nil, fmt.Errorf("destination ancestor %q changed while being opened", name) + } + if created { + if err := setOpenedDirectoryMode(child, mode); err != nil { + _ = child.Close() + return nil, fmt.Errorf("set destination ancestor permissions %q: %w", name, err) + } + } + return child, nil +} + +func setOpenedDirectoryMode(root *os.Root, mode os.FileMode) error { + directory, err := root.Open(".") + if err != nil { + return err + } + defer func() { _ = directory.Close() }() + return directory.Chmod(mode) +} + +func createSiblingTemp(parent *os.Root, base string) (*os.File, string, error) { + for attempt := 0; attempt < 100; attempt++ { + var randomBytes [16]byte + if _, err := rand.Read(randomBytes[:]); err != nil { + return nil, "", fmt.Errorf("generate temporary file name: %w", err) + } + name := "." + base + ".tmp-" + hex.EncodeToString(randomBytes[:]) + file, err := parent.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o600) + if errors.Is(err, os.ErrExist) { + continue + } + if err != nil { + return nil, "", err + } + return file, name, nil + } + return nil, "", fmt.Errorf("create unique sibling temporary file") +} + +func syncOpenedDirectory(parent *os.Root) error { + directory, err := parent.Open(".") + if err != nil { + return err + } + defer func() { _ = directory.Close() }() + return syncDirectoryFile(directory, parent.Name()) +} + +// DownloadedTempFile is a completed, destination-confined temporary file. It +// keeps its parent directory open until the caller installs or cleans it up. +type DownloadedTempFile struct { + parent *os.Root + name string +} + +// DownloadToSiblingTemp writes a remote object into a sibling temporary file. +// The caller supplies transport behavior through download and retains object +// identity and validation policy. The destination parent must already exist. +func DownloadToSiblingTemp(destination string, download func(io.Writer) error) (*DownloadedTempFile, error) { + if download == nil { + return nil, fmt.Errorf("download function is required") + } + parent, name, err := openConfinedParent(destination, false, 0) + if err != nil { + return nil, err + } + file, tempName, err := createSiblingTemp(parent, name) + if err != nil { + _ = parent.Close() + return nil, fmt.Errorf("create sibling temporary file: %w", err) + } + if err := download(file); err != nil { + _ = file.Close() + _ = parent.Remove(tempName) + _ = parent.Close() + return nil, err + } + if err := file.Sync(); err != nil { + _ = file.Close() + _ = parent.Remove(tempName) + _ = parent.Close() + return nil, fmt.Errorf("sync downloaded temporary file: %w", err) + } + if err := file.Close(); err != nil { + _ = parent.Remove(tempName) + _ = parent.Close() + return nil, fmt.Errorf("close downloaded temporary file: %w", err) + } + return &DownloadedTempFile{parent: parent, name: tempName}, nil +} + +// Open returns a read handle for validation before installation. +func (f *DownloadedTempFile) Open() (*os.File, error) { + if f == nil || f.parent == nil || f.name == "" { + return nil, fmt.Errorf("downloaded temporary file is unavailable") + } + return f.parent.Open(f.name) +} + +// Install atomically replaces destinationName in the opened parent directory. +func (f *DownloadedTempFile) Install(destinationName string, mode os.FileMode) error { + if f == nil || f.parent == nil || f.name == "" { + return fmt.Errorf("downloaded temporary file is unavailable") + } + if filepath.Base(destinationName) != destinationName || destinationName == "." || destinationName == "" { + return fmt.Errorf("destination file name is required") + } + file, err := f.parent.Open(f.name) + if err != nil { + return fmt.Errorf("open downloaded temporary file: %w", err) + } + if err := file.Chmod(mode); err != nil { + _ = file.Close() + return fmt.Errorf("set downloaded temporary file permissions: %w", err) + } + if err := file.Sync(); err != nil { + _ = file.Close() + return fmt.Errorf("sync downloaded temporary file: %w", err) + } + if err := file.Close(); err != nil { + return fmt.Errorf("close downloaded temporary file: %w", err) + } + if err := f.parent.Rename(f.name, destinationName); err != nil { + return fmt.Errorf("install downloaded temporary file: %w", err) + } + f.name = "" + if err := syncOpenedDirectory(f.parent); err != nil { + return fmt.Errorf("sync destination directory after downloaded-file replacement: %w", err) + } + return f.closeParent() +} + +// Cleanup removes an uninstalled temporary file. It is safe to call repeatedly. +func (f *DownloadedTempFile) Cleanup() error { + if f == nil || f.parent == nil { + return nil + } + var err error + if f.name != "" { + err = f.parent.Remove(f.name) + if errors.Is(err, os.ErrNotExist) { + err = nil + } + f.name = "" + } + closeErr := f.closeParent() + if err != nil && closeErr != nil { + return errors.Join(err, closeErr) + } + if err != nil { + return err + } + return closeErr +} + +func (f *DownloadedTempFile) closeParent() error { + if f.parent == nil { + return nil + } + err := f.parent.Close() + f.parent = nil + return err +} + +// DownloadAndInstall obtains remote bytes through a confined sibling temporary +// file and installs them with the caller-selected final mode. +func DownloadAndInstall(destination string, mode os.FileMode, download func(io.Writer) error) error { + temporary, err := DownloadToSiblingTemp(destination, download) + if err != nil { + return err + } + defer func() { _ = temporary.Cleanup() }() + return temporary.Install(filepath.Base(destination), mode) +} + +func replaceFileFromReaderConfined(dst string, source io.Reader, options ReplaceFileOptions) (resultErr error) { + parent, name, err := openConfinedParent(dst, false, 0) + if err != nil { + return err + } + defer func() { _ = parent.Close() }() + temporary, tempName, err := createSiblingTemp(parent, name) + if err != nil { + return fmt.Errorf("create temp file: %w", err) + } + removeTemp := true + defer func() { + if removeTemp { + if err := parent.Remove(tempName); err != nil && !errors.Is(err, os.ErrNotExist) { + resultErr = errors.Join(resultErr, fmt.Errorf("remove temporary file: %w", err)) + } + } + }() + + if _, err := io.Copy(temporary, source); err != nil { + return closeTemporaryFileAfterError(temporary, fmt.Errorf("write temp file: %w", err)) + } + if err := temporary.Chmod(options.Mode); err != nil { + return closeTemporaryFileAfterError(temporary, fmt.Errorf("set temp file permissions: %w", err)) + } + if err := temporary.Sync(); err != nil { + return closeTemporaryFileAfterError(temporary, fmt.Errorf("sync temp file: %w", err)) + } + if err := temporary.Close(); err != nil { + return fmt.Errorf("close temp file: %w", err) + } + if options.BeforeRename != nil { + if err := options.BeforeRename(); err != nil { + return fmt.Errorf("check before file replacement: %w", err) + } + } + if err := parent.Rename(tempName, name); err != nil { + return fmt.Errorf("install temp file: %w", err) + } + removeTemp = false + if err := syncOpenedDirectory(parent); err != nil { + return fmt.Errorf("sync destination directory after replacement: %w", err) + } + return nil +} + +func installDownloadedTempFileConfined(tmpPath, dst string, mode os.FileMode) error { + if filepath.Clean(filepath.Dir(tmpPath)) != filepath.Clean(filepath.Dir(dst)) { + source, err := os.Open(tmpPath) + if err != nil { + return fmt.Errorf("open downloaded temporary file: %w", err) + } + err = replaceFileFromReaderConfined(dst, source, ReplaceFileOptions{Mode: mode}) + closeErr := source.Close() + if err != nil { + return fmt.Errorf("install downloaded file: %w", err) + } + if closeErr != nil { + return fmt.Errorf("close downloaded temporary file: %w", closeErr) + } + if err := os.Remove(tmpPath); err != nil { + return fmt.Errorf("remove installed temporary file: %w", err) + } + return nil + } + parent, destinationName, err := openConfinedParent(dst, false, 0) + if err != nil { + return err + } + defer func() { _ = parent.Close() }() + temporaryName := filepath.Base(tmpPath) + info, err := parent.Lstat(temporaryName) + if err != nil { + return fmt.Errorf("inspect downloaded temporary file: %w", err) + } + if info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() { + return fmt.Errorf("downloaded temporary file is not a regular file") + } + file, err := parent.Open(temporaryName) + if err != nil { + return fmt.Errorf("open downloaded temporary file: %w", err) + } + if err := file.Chmod(mode); err != nil { + _ = file.Close() + return fmt.Errorf("set temp file permissions: %w", err) + } + if err := file.Sync(); err != nil { + _ = file.Close() + return fmt.Errorf("sync downloaded temp file: %w", err) + } + if err := file.Close(); err != nil { + return fmt.Errorf("close downloaded temp file: %w", err) + } + if err := parent.Rename(temporaryName, destinationName); err != nil { + return fmt.Errorf("install downloaded file: %w", err) + } + if err := syncOpenedDirectory(parent); err != nil { + return fmt.Errorf("sync destination directory after downloaded-file replacement: %w", err) + } + return nil +} diff --git a/internal/fileops/directory.go b/internal/fileops/directory.go index e7fa3f0..857541c 100644 --- a/internal/fileops/directory.go +++ b/internal/fileops/directory.go @@ -21,6 +21,13 @@ func PromoteDirectory(src, dst string) error { if err := checkAtomicDirectoryPromotionSupport(); err != nil { return err } + parent, _, err := openConfinedParent(dst, false, 0) + if err != nil { + return fmt.Errorf("open destination parent: %w", err) + } + if err := parent.Close(); err != nil { + return fmt.Errorf("close destination parent: %w", err) + } return promoteDirectory(src, dst, renameDirectoryNoReplace) } diff --git a/internal/fileops/directory_test.go b/internal/fileops/directory_test.go index 8a4bcbf..5c88882 100644 --- a/internal/fileops/directory_test.go +++ b/internal/fileops/directory_test.go @@ -431,6 +431,29 @@ func TestPromoteDirectoryRejectsDestinationInsideSource(t *testing.T) { } } +func TestPromoteDirectoryRejectsSymlinkedDestinationAncestor(t *testing.T) { + root := t.TempDir() + source := filepath.Join(root, "source") + if err := os.Mkdir(source, 0o755); err != nil { + t.Fatalf("Mkdir(source) error = %v", err) + } + if err := os.WriteFile(filepath.Join(source, "value.txt"), []byte("source"), 0o644); err != nil { + t.Fatalf("WriteFile(source) error = %v", err) + } + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(root, "redirect")); err != nil { + t.Skipf("Symlink unavailable: %v", err) + } + + err := PromoteDirectory(source, filepath.Join(root, "redirect", "bundle")) + if err == nil { + t.Fatal("PromoteDirectory() error = nil, want symlink ancestor rejection") + } + if _, err := os.Stat(filepath.Join(outside, "bundle")); !os.IsNotExist(err) { + t.Fatalf("outside bundle exists: stat err = %v", err) + } +} + func mustWriteFile(t *testing.T, path string, data []byte, mode os.FileMode) { t.Helper() if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { diff --git a/internal/fileops/fileops.go b/internal/fileops/fileops.go index a7d0d8b..e14d315 100644 --- a/internal/fileops/fileops.go +++ b/internal/fileops/fileops.go @@ -32,7 +32,7 @@ type ReplaceFileOptions struct { // existing destination or directory syncing is unavailable, it returns an // error rather than claiming an equivalent durability guarantee. func ReplaceFileAtomic(dst string, data []byte, options ReplaceFileOptions) error { - return replaceFileAtomicWithOperations(dst, data, options, systemAtomicReplacementOperations) + return replaceFileFromReaderConfined(dst, bytes.NewReader(data), options) } // WriteFileAtomic creates the destination parent with workspace permissions @@ -72,11 +72,10 @@ func CopyFileAtomicWithChecksum(src, dst string, perm os.FileMode) (string, erro } digest := sha256.New() - err = replaceFileFromReaderWithOperations( + err = replaceFileFromReaderConfined( dst, io.TeeReader(in, digest), ReplaceFileOptions{Mode: perm}, - systemAtomicReplacementOperations, ) if err != nil { return "", err @@ -94,7 +93,7 @@ func InstallDownloadedTempFile(tmpPath, dst string, perm os.FileMode) error { if err := EnsureWorkspaceDirectory(filepath.Dir(dst)); err != nil { return fmt.Errorf("create destination directory: %w", err) } - return installDownloadedTempFileWithOperations(tmpPath, dst, perm, systemAtomicReplacementOperations) + return installDownloadedTempFileConfined(tmpPath, dst, perm) } type temporaryFile interface { diff --git a/internal/fileops/fileops_test.go b/internal/fileops/fileops_test.go index cdd796d..452e9ca 100644 --- a/internal/fileops/fileops_test.go +++ b/internal/fileops/fileops_test.go @@ -115,6 +115,82 @@ func TestInstallDownloadedTempFileSetsPermissions(t *testing.T) { } } +func TestWriteFileAtomicRejectsSymlinkedDestinationAncestor(t *testing.T) { + root := t.TempDir() + outside := t.TempDir() + sentinel := filepath.Join(outside, "sentinel.txt") + if err := os.WriteFile(sentinel, []byte("unchanged"), 0o600); err != nil { + t.Fatalf("WriteFile(sentinel) error = %v", err) + } + if err := os.Symlink(outside, filepath.Join(root, "redirect")); err != nil { + t.Skipf("Symlink unavailable: %v", err) + } + + err := WriteFileAtomic(filepath.Join(root, "redirect", "output.txt"), []byte("new"), 0o640) + if err == nil { + t.Fatal("WriteFileAtomic() error = nil, want symlink ancestor rejection") + } + data, err := os.ReadFile(sentinel) + if err != nil { + t.Fatalf("ReadFile(sentinel) error = %v", err) + } + if string(data) != "unchanged" { + t.Fatalf("outside sentinel = %q, want unchanged", data) + } + if _, err := os.Stat(filepath.Join(outside, "output.txt")); !os.IsNotExist(err) { + t.Fatalf("outside output exists: stat err = %v", err) + } +} + +func TestWriteFileAtomicReplacesLeafSymlinkWithoutFollowingIt(t *testing.T) { + root := t.TempDir() + outside := filepath.Join(t.TempDir(), "outside.txt") + if err := os.WriteFile(outside, []byte("outside"), 0o600); err != nil { + t.Fatalf("WriteFile(outside) error = %v", err) + } + destination := filepath.Join(root, "output.txt") + if err := os.Symlink(outside, destination); err != nil { + t.Skipf("Symlink unavailable: %v", err) + } + + if err := WriteFileAtomic(destination, []byte("inside"), 0o640); err != nil { + t.Fatalf("WriteFileAtomic() error = %v", err) + } + outsideData, err := os.ReadFile(outside) + if err != nil { + t.Fatalf("ReadFile(outside) error = %v", err) + } + if string(outsideData) != "outside" { + t.Fatalf("outside file = %q, want unchanged", outsideData) + } + info, err := os.Lstat(destination) + if err != nil || info.Mode()&os.ModeSymlink != 0 { + t.Fatalf("destination was not replaced with a regular file: info=%v err=%v", info, err) + } +} + +func TestDownloadAndInstallRejectsSymlinkedDestinationAncestor(t *testing.T) { + root := t.TempDir() + outside := t.TempDir() + if err := os.Symlink(outside, filepath.Join(root, "redirect")); err != nil { + t.Skipf("Symlink unavailable: %v", err) + } + + err := DownloadAndInstall(filepath.Join(root, "redirect", "output.txt"), 0o640, func(io.Writer) error { + return nil + }) + if err == nil { + t.Fatal("DownloadAndInstall() error = nil, want symlink ancestor rejection") + } + entries, err := os.ReadDir(outside) + if err != nil { + t.Fatalf("ReadDir(outside) error = %v", err) + } + if len(entries) != 0 { + t.Fatalf("outside destination received entries: %v", entries) + } +} + func TestReplaceFileAtomicOrdersDurableOperations(t *testing.T) { events := make([]string, 0, 8) tmp := &recordingTemporaryFile{name: "/work/.result.tmp-1", events: &events} diff --git a/internal/fileops/modes.go b/internal/fileops/modes.go index cabebf5..4157aa6 100644 --- a/internal/fileops/modes.go +++ b/internal/fileops/modes.go @@ -3,7 +3,6 @@ package fileops import ( "fmt" "os" - "path/filepath" "strings" ) @@ -23,35 +22,7 @@ func EnsureWorkspaceDirectory(directory string) error { return fmt.Errorf("workspace directory is required") } - directory = filepath.Clean(directory) - missing := make([]string, 0) - for current := directory; ; current = filepath.Dir(current) { - info, err := os.Lstat(current) - if err == nil { - if !info.IsDir() { - return fmt.Errorf("workspace directory %q is not a directory", current) - } - break - } - if !os.IsNotExist(err) { - return fmt.Errorf("inspect workspace directory %q: %w", current, err) - } - missing = append(missing, current) - parent := filepath.Dir(current) - if parent == current { - break - } - } - - if err := os.MkdirAll(directory, WorkspaceDirectoryMode); err != nil { - return fmt.Errorf("create workspace directory %q: %w", directory, err) - } - for index := len(missing) - 1; index >= 0; index-- { - if err := os.Chmod(missing[index], WorkspaceDirectoryMode); err != nil { - return fmt.Errorf("set workspace directory permissions %q: %w", missing[index], err) - } - } - if err := os.Chmod(directory, WorkspaceDirectoryMode); err != nil { + if err := ensureConfinedDirectory(directory, WorkspaceDirectoryMode); err != nil { return fmt.Errorf("set workspace directory permissions %q: %w", directory, err) } return nil diff --git a/internal/fileops/sync_directory_unix.go b/internal/fileops/sync_directory_unix.go index 2b00587..2001d17 100644 --- a/internal/fileops/sync_directory_unix.go +++ b/internal/fileops/sync_directory_unix.go @@ -16,7 +16,11 @@ func syncDirectory(path string) error { } defer func() { _ = directory.Close() }() - err = directory.Sync() + return syncDirectoryFile(directory, path) +} + +func syncDirectoryFile(directory *os.File, path string) error { + err := directory.Sync() // Some Unix filesystems do not implement directory syncing. Report this // explicitly so callers do not confuse visible replacement with a durable one. if errors.Is(err, syscall.EINVAL) || errors.Is(err, syscall.ENOTSUP) { diff --git a/internal/fileops/sync_directory_unsupported.go b/internal/fileops/sync_directory_unsupported.go index 300d35e..8750fdb 100644 --- a/internal/fileops/sync_directory_unsupported.go +++ b/internal/fileops/sync_directory_unsupported.go @@ -4,9 +4,14 @@ package fileops import ( "fmt" + "os" "runtime" ) func syncDirectory(path string) error { return fmt.Errorf("%w for %q on %s", ErrDirectorySyncUnsupported, path, runtime.GOOS) } + +func syncDirectoryFile(_ *os.File, path string) error { + return syncDirectory(path) +} diff --git a/internal/fileops/sync_directory_windows.go b/internal/fileops/sync_directory_windows.go index 693bbb9..f91b06d 100644 --- a/internal/fileops/sync_directory_windows.go +++ b/internal/fileops/sync_directory_windows.go @@ -5,6 +5,7 @@ package fileops import ( "errors" "fmt" + "os" "golang.org/x/sys/windows" ) @@ -39,3 +40,13 @@ func syncDirectory(path string) error { } return err } + +func syncDirectoryFile(directory *os.File, path string) error { + err := directory.Sync() + if errors.Is(err, windows.ERROR_INVALID_FUNCTION) || + errors.Is(err, windows.ERROR_INVALID_HANDLE) || + errors.Is(err, windows.ERROR_NOT_SUPPORTED) { + return fmt.Errorf("%w for %q: %w", ErrDirectorySyncUnsupported, path, err) + } + return err +} diff --git a/internal/manifest/store.go b/internal/manifest/store.go index 0b64df7..9c28752 100644 --- a/internal/manifest/store.go +++ b/internal/manifest/store.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "io" "os" "path/filepath" "strings" @@ -45,18 +46,35 @@ func (s *LocalStore) Load(ctx context.Context, path string) (*Manifest, error) { return nil, fmt.Errorf("load manifest: path is required") } - data, err := os.ReadFile(path) + file, err := os.Open(path) if err != nil { return nil, fmt.Errorf("load manifest %q: %w", path, err) } + defer file.Close() + return s.LoadReader(ctx, file) +} + +// LoadReader reads and validates a manifest from a caller-owned reader. +func (s *LocalStore) LoadReader(ctx context.Context, source io.Reader) (*Manifest, error) { + if err := checkContext(ctx); err != nil { + return nil, err + } + if source == nil { + return nil, fmt.Errorf("load manifest: source is required") + } + + data, err := io.ReadAll(source) + if err != nil { + return nil, fmt.Errorf("read manifest: %w", err) + } var m Manifest if err := json.Unmarshal(data, &m); err != nil { - return nil, fmt.Errorf("decode manifest %q: %w", path, err) + return nil, fmt.Errorf("decode manifest: %w", err) } if err := validateLoadedManifest(&m); err != nil { - return nil, fmt.Errorf("manifest %q invalid: %w", path, err) + return nil, fmt.Errorf("manifest invalid: %w", err) } normalizeManifest(&m) diff --git a/internal/stage/prepare_previous.go b/internal/stage/prepare_previous.go index c83e5c8..5705e79 100644 --- a/internal/stage/prepare_previous.go +++ b/internal/stage/prepare_previous.go @@ -3,10 +3,11 @@ package stage import ( "context" "fmt" - "os" + "io" "path/filepath" "sort" + "gitea.maximumdirect.net/eric/narratio/internal/adapters/storage" "gitea.maximumdirect.net/eric/narratio/internal/artifacts" "gitea.maximumdirect.net/eric/narratio/internal/fileops" "gitea.maximumdirect.net/eric/narratio/internal/manifest" @@ -56,33 +57,9 @@ func hydratePreviousSessionArtifacts( return nil, fmt.Errorf("create previous-session path directory for %q: %w", record.LocalPath, err) } - base := filepath.Base(record.LocalPath) - tmp, err := os.CreateTemp(filepath.Dir(record.LocalPath), "."+base+".prepare-previous-*.tmp") - if err != nil { - return nil, fmt.Errorf("create previous-session temp file for %q: %w", record.LocalPath, err) - } - tmpPath := tmp.Name() - if err := tmp.Close(); err != nil { - _ = os.Remove(tmpPath) - return nil, fmt.Errorf("close previous-session temp file for %q: %w", record.LocalPath, err) - } - if err := func() error { - removeTmp := true - defer func() { - if removeTmp { - _ = os.Remove(tmpPath) - } - }() - - if err := env.ObjectStore.Download(ctx, record.RemoteKey, tmpPath); err != nil { - return fmt.Errorf("download previous-session object %q to temp file: %w", record.RemoteKey, err) - } - if err := fileops.InstallDownloadedTempFile(tmpPath, record.LocalPath, fileops.WorkspaceFileMode); err != nil { - return fmt.Errorf("install previous-session object %q at %q: %w", record.RemoteKey, record.LocalPath, err) - } - removeTmp = false - return nil - }(); err != nil { + if err := fileops.DownloadAndInstall(record.LocalPath, fileops.WorkspaceFileMode, func(destination io.Writer) error { + return storage.DownloadTo(ctx, env.ObjectStore, record.RemoteKey, destination) + }); err != nil { return nil, err } if record.Kind == preparePreviousInputKindArtifact { diff --git a/internal/stage/prepare_previous_test.go b/internal/stage/prepare_previous_test.go index 48756c2..8deba5e 100644 --- a/internal/stage/prepare_previous_test.go +++ b/internal/stage/prepare_previous_test.go @@ -3,6 +3,7 @@ package stage import ( "context" "encoding/json" + "io" "os" "path/filepath" "strings" @@ -391,6 +392,11 @@ func (s *preparePreviousCaptureStore) Download(ctx context.Context, key, localPa return s.delegate.Download(ctx, key, localPath) } +func (s *preparePreviousCaptureStore) DownloadTo(ctx context.Context, key string, destination io.Writer) error { + s.downloadKeys = append(s.downloadKeys, key) + return storage.DownloadTo(ctx, s.delegate, key, destination) +} + func (s *preparePreviousCaptureStore) Upload(ctx context.Context, localPath, key string, opts storage.UploadOptions) (storage.ObjectInfo, error) { return s.delegate.Upload(ctx, localPath, key, opts) }