From a2a144dffa34b38acdd40bb60102e3b9fbce157d Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 11 Aug 2026 12:28:55 +0000 Subject: [PATCH] Close diagnostic and restore coverage gaps --- docs/roadmap/implementation.md | 2 +- internal/app/restore_execution_test.go | 83 ++++++++++++++++- internal/fileops/confined.go | 124 +++++++++++++++++++++++-- internal/fileops/confined_open_test.go | 92 ++++++++++++++++++ 4 files changed, 289 insertions(+), 12 deletions(-) diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 8c6a82e..87055c5 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -56,7 +56,7 @@ the full audit or any audit line range. | 32 | Reconciled lifecycle/analyze documentation and closed the original audit traceability inventory. | COM-002, COM-005 | Completed | | 33 | Confine diagnostic log destinations and eliminate pathname-based tail reads. | Follow-up review | Completed | | 34 | Dispose of owned subprocess descendants after natural leader exit. | Follow-up review | Completed | -| 35 | Bound remote current-state and lock control-plane reads. | Follow-up review | Pending | +| 35 | Bound remote current-state and lock control-plane reads. | Follow-up review | Completed | Completed stages must not be reimplemented wholesale. A pending stage may adjust their code only where its stated remediation requires it. diff --git a/internal/app/restore_execution_test.go b/internal/app/restore_execution_test.go index c03d2d7..6e9f86c 100644 --- a/internal/app/restore_execution_test.go +++ b/internal/app/restore_execution_test.go @@ -357,7 +357,7 @@ func TestExecuteRestoreLockConflictFailsAndWritesNothing(t *testing.T) { } } -func TestExecuteRestoreInvalidManifestDoesNotCorruptExistingManifest(t *testing.T) { +func TestExecuteRestorePlanInvalidManifestDoesNotCorruptExistingManifest(t *testing.T) { cfg := restorePlanConfig(t) store := &storage.FakeBackend{} current := seedCommittedRestoreSnapshot(t, cfg, store, "20260519T010203Z-a1b2c3d4", map[string][]byte{ @@ -417,6 +417,87 @@ func TestExecuteRestoreInvalidManifestDoesNotCorruptExistingManifest(t *testing. } } +func TestExecuteRestoreInvalidManifestReportsFailureAndRetainsRecoveryMarker(t *testing.T) { + workspaceRoot := t.TempDir() + pipelinePath, campaignPath, sessionPath := writeValidConfigFiles(t, workspaceRoot) + cfg, err := config.LoadWithSessionOptions(pipelinePath, campaignPath, sessionPath, config.SessionLoadOptions{}) + if err != nil { + t.Fatalf("LoadWithSessionOptions() error = %v", err) + } + store := &storage.FakeBackend{} + seedCommittedRestoreSnapshot(t, cfg, store, "20260519T010203Z-a1b2c3d4", map[string][]byte{ + "transcripts/full.json": []byte("remote-transcript"), + }) + + sessionRoot := artifacts.SessionWorkDirForCampaign(workspaceRoot, cfg.Session.Campaign, cfg.Session.SessionID) + existing := manifest.New(cfg.Session.SessionID, nowUTC()) + existing.Campaign = cfg.Session.Campaign + existingPath := filepath.Join(sessionRoot, "manifest.json") + manifestStore := &manifest.LocalStore{} + if err := manifestStore.Save(context.Background(), existingPath, existing); err != nil { + t.Fatalf("save existing local manifest: %v", err) + } + existingData, err := os.ReadFile(existingPath) + if err != nil { + t.Fatalf("read existing local manifest: %v", err) + } + + restoreWithStoreAndRealPhases(t, store) + realBuildRestorePlan := buildRestorePlanFn + buildRestorePlanFn = func(ctx context.Context, cfg *config.Config, current *RemoteCurrentState, objectStore storage.ObjectStore, opts RestorePlanOptions) (*RestorePlan, error) { + plan, err := realBuildRestorePlan(ctx, cfg, current, objectStore, opts) + if err != nil { + return nil, err + } + invalidManifest := []byte("{invalid json") + for index := range plan.Actions { + if plan.Actions[index].LocalRelativePath != config.PathManifestFile { + continue + } + plan.Actions[index].VerifiedContent = invalidManifest + plan.Actions[index].SHA256 = restoreCommitSHA256(invalidManifest) + plan.Actions[index].Size = int64(len(invalidManifest)) + store.SeedObject(storage.FakeObject{ + Key: plan.Actions[index].RemoteKey, + Data: invalidManifest, + ETag: plan.Actions[index].Generation, + }) + return plan, nil + } + t.Fatal("restore plan has no manifest action") + return nil, nil + } + + var stdout bytes.Buffer + var stderr bytes.Buffer + code := Execute([]string{"session", "restore", cfg.Session.SessionID, "--config", pipelinePath, "--campaign-file", campaignPath, "--session", sessionPath, "--force"}, &stdout, &stderr) + if code == 0 { + t.Fatal("exit code = 0, want non-zero") + } + if !strings.Contains(stderr.String(), "validate manifest decode") { + t.Fatalf("stderr = %q, want manifest validation failure", stderr.String()) + } + mustReadEquals(t, filepath.Join(sessionRoot, "transcripts", "full.json"), "remote-transcript") + report := mustReadRestoreReport(t, filepath.Join(sessionRoot, "reports", "restore-latest.json")) + if report.Status != "failed" { + t.Fatalf("report status = %q, want failed", report.Status) + } + if strings.TrimSpace(report.Error) == "" { + t.Fatal("report error is empty, want failure context") + } + afterData, err := os.ReadFile(existingPath) + if err != nil { + t.Fatalf("read local manifest after failure: %v", err) + } + if string(afterData) != string(existingData) { + t.Fatalf("local manifest changed after failed restore; before=%q after=%q", string(existingData), string(afterData)) + } + markerPath := artifacts.SessionRestoreMarkerPathForCampaign(workspaceRoot, cfg.Session.Campaign, cfg.Session.SessionID) + if _, err := os.Stat(markerPath); err != nil { + t.Fatalf("incomplete restore marker should remain after failed forced restore: %v", err) + } +} + func TestExecuteRestorePlanPathMismatchFails(t *testing.T) { cfg := restorePlanConfig(t) current := restorePlanCurrentState(t, cfg) diff --git a/internal/fileops/confined.go b/internal/fileops/confined.go index c3ba75d..7815a92 100644 --- a/internal/fileops/confined.go +++ b/internal/fileops/confined.go @@ -156,13 +156,17 @@ func setOpenedDirectoryMode(root *os.Root, mode os.FileMode) error { } func createSiblingTemp(parent *os.Root, base string) (*os.File, string, error) { + return createSiblingTempWithFlags(parent, base, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o600) +} + +func createSiblingTempWithFlags(parent *os.Root, base string, flags int, mode os.FileMode) (*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) + file, err := parent.OpenFile(name, flags|os.O_CREATE|os.O_EXCL, mode) if errors.Is(err, os.ErrExist) { continue } @@ -184,28 +188,128 @@ func syncOpenedDirectory(parent *os.Root) error { } // OpenFileConfined opens a file after verifying its parent hierarchy without -// following symbolic links. Existing symbolic-link and non-regular leaves are +// mutating through symbolic links. Truncating opens atomically replace the leaf +// with a newly created sibling, while other creating opens never carry O_CREATE +// into an existing-leaf open. Existing symbolic-link and non-regular leaves are // rejected. func OpenFileConfined(path string, flags int, mode os.FileMode) (*os.File, error) { + return openFileConfined(path, flags, mode, nil) +} + +func openFileConfined(path string, flags int, mode os.FileMode, beforeOpen func() error) (*os.File, error) { parent, name, err := openConfinedParent(path, false, 0) if err != nil { return nil, err } defer func() { _ = parent.Close() }() - if info, err := parent.Lstat(name); err == nil { - if info.Mode()&os.ModeSymlink != 0 { - return nil, fmt.Errorf("destination file %q is a symbolic link", name) + + if flags&(os.O_CREATE|os.O_EXCL) == os.O_CREATE|os.O_EXCL { + if err := runBeforeConfinedOpen(beforeOpen); err != nil { + return nil, err } - if !info.Mode().IsRegular() { - return nil, fmt.Errorf("destination file %q is not a regular file", name) + file, err := parent.OpenFile(name, flags, mode) + if err != nil { + return nil, err } - } else if err != nil && !errors.Is(err, os.ErrNotExist) { - return nil, fmt.Errorf("inspect destination file %q: %w", name, err) + return validateOpenedConfinedFile(parent, name, file) } - file, err := parent.OpenFile(name, flags, mode) + + exists, err := validateConfinedLeaf(parent, name) if err != nil { return nil, err } + if flags&os.O_TRUNC != 0 { + if !exists && flags&os.O_CREATE == 0 { + return nil, fmt.Errorf("open destination file %q: %w", name, os.ErrNotExist) + } + return replaceAndOpenConfinedFile(parent, name, flags, mode, beforeOpen) + } + + if err := runBeforeConfinedOpen(beforeOpen); err != nil { + return nil, err + } + if !exists && flags&os.O_CREATE != 0 { + file, err := parent.OpenFile(name, flags|os.O_EXCL, mode) + if err == nil { + return validateOpenedConfinedFile(parent, name, file) + } + if !errors.Is(err, os.ErrExist) { + return nil, err + } + if _, validationErr := validateConfinedLeaf(parent, name); validationErr != nil { + return nil, validationErr + } + } + + // The leaf now exists, so removing O_CREATE prevents a raced dangling + // symlink from creating its target. Opening an existing symlink can expose a + // handle, but no caller can mutate through it before the identity check below. + file, err := parent.OpenFile(name, flags&^os.O_CREATE, mode) + if err != nil { + return nil, err + } + return validateOpenedConfinedFile(parent, name, file) +} + +func validateConfinedLeaf(parent *os.Root, name string) (bool, error) { + info, err := parent.Lstat(name) + if errors.Is(err, os.ErrNotExist) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("inspect destination file %q: %w", name, err) + } + if info.Mode()&os.ModeSymlink != 0 { + return false, fmt.Errorf("destination file %q is a symbolic link", name) + } + if !info.Mode().IsRegular() { + return false, fmt.Errorf("destination file %q is not a regular file", name) + } + return true, nil +} + +func replaceAndOpenConfinedFile(parent *os.Root, name string, flags int, mode os.FileMode, beforeOpen func() error) (_ *os.File, resultErr error) { + temporary, temporaryName, err := createSiblingTempWithFlags(parent, name, flags&^os.O_TRUNC, mode) + if err != nil { + return nil, fmt.Errorf("create replacement for destination file %q: %w", name, err) + } + installed := false + defer func() { + if installed { + return + } + closeErr := temporary.Close() + removeErr := parent.Remove(temporaryName) + if errors.Is(removeErr, os.ErrNotExist) { + removeErr = nil + } + resultErr = errors.Join(resultErr, closeErr, removeErr) + }() + + if err := runBeforeConfinedOpen(beforeOpen); err != nil { + return nil, err + } + // Rename replaces the directory entry itself rather than following it. A + // leaf changed to a symlink after validation therefore cannot redirect the + // truncation to its target. + if err := parent.Rename(temporaryName, name); err != nil { + return nil, fmt.Errorf("install replacement for destination file %q: %w", name, err) + } + installed = true + return validateOpenedConfinedFile(parent, name, temporary) +} + +func runBeforeConfinedOpen(beforeOpen func() error) error { + if beforeOpen == nil { + return nil + } + if err := beforeOpen(); err != nil { + return fmt.Errorf("check before confined file open: %w", err) + } + return nil +} + +func validateOpenedConfinedFile(parent *os.Root, name string, file *os.File) (*os.File, error) { opened, err := file.Stat() if err != nil { _ = file.Close() diff --git a/internal/fileops/confined_open_test.go b/internal/fileops/confined_open_test.go index c17b35d..2bc1c1b 100644 --- a/internal/fileops/confined_open_test.go +++ b/internal/fileops/confined_open_test.go @@ -54,3 +54,95 @@ func TestOpenFileConfinedRejectsSymlinkWithoutTruncatingTarget(t *testing.T) { t.Fatalf("target content = %q, want %q", data, original) } } + +func TestOpenFileConfinedDoesNotFollowLeafReplacedBeforeTruncate(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("creating symlinks requires privileges that are not available on every Windows runner") + } + dir := t.TempDir() + target := filepath.Join(dir, "target.log") + const original = "unrelated content" + if err := os.WriteFile(target, []byte(original), 0o600); err != nil { + t.Fatalf("WriteFile(target) error = %v", err) + } + leaf := filepath.Join(dir, "diagnostic.log") + if err := os.WriteFile(leaf, []byte("old diagnostic"), 0o600); err != nil { + t.Fatalf("WriteFile(leaf) error = %v", err) + } + + file, err := openFileConfined(leaf, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, WorkspaceFileMode, func() error { + if err := os.Remove(leaf); err != nil { + return err + } + return os.Symlink(filepath.Base(target), leaf) + }) + if err != nil { + t.Fatalf("openFileConfined() error = %v", err) + } + if _, err := file.WriteString("new diagnostic"); err != nil { + _ = file.Close() + t.Fatalf("WriteString() error = %v", err) + } + if err := file.Close(); err != nil { + t.Fatalf("Close() error = %v", err) + } + + targetData, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile(target) error = %v", err) + } + if string(targetData) != original { + t.Fatalf("target content = %q, want %q", targetData, original) + } + leafData, err := os.ReadFile(leaf) + if err != nil { + t.Fatalf("ReadFile(leaf) error = %v", err) + } + if string(leafData) != "new diagnostic" { + t.Fatalf("leaf content = %q, want replacement diagnostic", leafData) + } + info, err := os.Lstat(leaf) + if err != nil { + t.Fatalf("Lstat(leaf) error = %v", err) + } + if info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() { + t.Fatalf("leaf mode = %v, want regular file", info.Mode()) + } +} + +func TestOpenFileConfinedRejectsLeafReplacedBeforeExistingFileOpen(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("creating symlinks requires privileges that are not available on every Windows runner") + } + dir := t.TempDir() + target := filepath.Join(dir, "target.lock") + const original = "unrelated lock content" + if err := os.WriteFile(target, []byte(original), 0o600); err != nil { + t.Fatalf("WriteFile(target) error = %v", err) + } + leaf := filepath.Join(dir, "session.lock") + if err := os.WriteFile(leaf, []byte("old lock"), 0o600); err != nil { + t.Fatalf("WriteFile(leaf) error = %v", err) + } + + file, err := openFileConfined(leaf, os.O_RDWR|os.O_CREATE, WorkspaceFileMode, func() error { + if err := os.Remove(leaf); err != nil { + return err + } + return os.Symlink(filepath.Base(target), leaf) + }) + if file != nil { + _ = file.Close() + t.Fatal("openFileConfined() returned a raced symbolic-link target") + } + if err == nil || !strings.Contains(err.Error(), "changed while being opened") { + t.Fatalf("openFileConfined() error = %v, want identity-change rejection", err) + } + targetData, readErr := os.ReadFile(target) + if readErr != nil { + t.Fatalf("ReadFile(target) error = %v", readErr) + } + if string(targetData) != original { + t.Fatalf("target content = %q, want %q", targetData, original) + } +}