diff --git a/docs/operations.md b/docs/operations.md index a219337..d52fe08 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -24,9 +24,17 @@ Durable logical files are written under: // ``` -Each output file is written atomically. Notarius never automatically removes -output. The [JSON output contract](integrations/json-output.md) owns the -logical file names, schemas, and media types inside a run directory. +The CLI generates one run ID in the form +`run--<32-lowercase-hex-characters>` and uses it +for output, manifests, and any requested debug bundle. It validates every +logical output name before exclusively creating the run directory. If that +directory already exists, the invocation fails without changing it. + +Each output file is written atomically. A later file-write failure leaves the +newly allocated partial run directory in place for inspection; Notarius never +automatically removes output. The +[JSON output contract](integrations/json-output.md) owns the logical file +names, schemas, and media types inside a run directory. Remove an output run directory only after its consumer data is no longer needed. This is data deletion, not cache cleanup. @@ -148,10 +156,10 @@ summary. Use exact paths for manual cleanup. Examples: ```sh -rm -rf ./notarius-output/run-1234567890 +rm -rf ./notarius-output/run-1721300000000000000-0123456789abcdef0123456789abcdef rm -rf /var/cache/notarius/chunk-plans/0123abcd rm -rf /var/cache/notarius/checkpoints/pipeline/input-0123/pipeline-4567/identity-89ab -rm -rf ./notarius-debug/run-1234567890 +rm -rf ./notarius-debug/run-1721300000000000000-0123456789abcdef0123456789abcdef ``` Avoid broad recursive cleanup against a parent root unless it is an explicit diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index c6d436b..c790ae2 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -213,7 +213,7 @@ pass. ## Stage 2: Make run identity collision-resistant and output exclusive -**Status:** Not started +**Status:** Complete ### Objective diff --git a/internal/cli/run.go b/internal/cli/run.go index 12ef0f5..6725f12 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -37,6 +37,7 @@ type Options struct { Catalog pipeline.ModuleCatalog Registries pipeline.Registries LLMClientFactory LLMClientFactory + RunIDGenerator RunIDGenerator LookupEnv func(string) (string, bool) Now func() time.Time UserCacheDir func() (string, error) @@ -91,6 +92,9 @@ func normalizeOptions(opts Options) (Options, error) { if opts.Now == nil { opts.Now = time.Now } + if opts.RunIDGenerator == nil { + opts.RunIDGenerator = defaultRunIDGenerator + } if opts.UserCacheDir == nil { opts.UserCacheDir = os.UserCacheDir } @@ -213,17 +217,25 @@ func runPipelineCommand(args []string, stdout, stderr io.Writer, opts Options) i } startedAt := opts.Now().UTC() - runID := fmt.Sprintf("run-%d", startedAt.UnixNano()) + runID, err := opts.RunIDGenerator(startedAt) + if err != nil { + fmt.Fprintf(stderr, "notarius: generate run ID: %v\n", err) + return 1 + } + if err := validateRunID(runID); err != nil { + fmt.Fprintf(stderr, "notarius: invalid generated run ID: %v\n", err) + return 1 + } var summary *debugbundle.SummaryWriter debugPath := "" debugRecorder := pipeline.NoopDebugRecorder() if *debug { - bundle, err := debugbundle.Allocate(cfg.Debug.Directory) + bundle, err := debugbundle.Allocate(cfg.Debug.Directory, runID, startedAt) if err != nil { fmt.Fprintf(stderr, "notarius: %v\n", err) return 1 } - runID, debugPath, summary = bundle.RunID(), bundle.Path(), bundle.Summary() + debugPath, summary = bundle.Path(), bundle.Summary() debugRecorder, err = opts.DebugRecorderFactory(bundle.TraceRoot()) if err != nil { return failPipelineCommand(stderr, summary, debugPath, fmt.Errorf("create debug recorder: %w", err), true) @@ -515,8 +527,15 @@ func writeOutputFiles(runOutputDir string, files []contracts.OutputFile) error { targets = append(targets, outputTarget{path: targetPath, file: file}) } - if err := os.MkdirAll(runOutputDir, 0o755); err != nil { - return fmt.Errorf("create output directory %q: %w", runOutputDir, err) + outputParent := filepath.Dir(runOutputDir) + if err := os.MkdirAll(outputParent, 0o755); err != nil { + return fmt.Errorf("create output parent %q: %w", outputParent, err) + } + if err := os.Mkdir(runOutputDir, 0o755); err != nil { + if os.IsExist(err) { + return fmt.Errorf("output run directory %q already exists", runOutputDir) + } + return fmt.Errorf("create output run directory %q: %w", runOutputDir, err) } for _, target := range targets { if err := os.MkdirAll(filepath.Dir(target.path), 0o755); err != nil { diff --git a/internal/cli/run_id.go b/internal/cli/run_id.go new file mode 100644 index 0000000..d5b8139 --- /dev/null +++ b/internal/cli/run_id.go @@ -0,0 +1,34 @@ +package cli + +import ( + "crypto/rand" + "encoding/hex" + "fmt" + "io" + "path/filepath" + "strings" + "time" +) + +type RunIDGenerator func(time.Time) (string, error) + +func defaultRunIDGenerator(startedAt time.Time) (string, error) { + var suffix [16]byte + if _, err := io.ReadFull(rand.Reader, suffix[:]); err != nil { + return "", fmt.Errorf("read random run ID suffix: %w", err) + } + return fmt.Sprintf("run-%d-%s", startedAt.UnixNano(), hex.EncodeToString(suffix[:])), nil +} + +func validateRunID(runID string) error { + if runID == "" { + return fmt.Errorf("run ID must not be empty") + } + if runID != strings.TrimSpace(runID) { + return fmt.Errorf("run ID %q must not have surrounding whitespace", runID) + } + if strings.ContainsAny(runID, `/\\`) || filepath.IsAbs(runID) || filepath.Clean(runID) != runID || runID == "." || runID == ".." { + return fmt.Errorf("run ID %q must be one safe path component", runID) + } + return nil +} diff --git a/internal/cli/run_id_test.go b/internal/cli/run_id_test.go new file mode 100644 index 0000000..a4c08bb --- /dev/null +++ b/internal/cli/run_id_test.go @@ -0,0 +1,87 @@ +package cli + +import ( + "os" + "path/filepath" + "regexp" + "strings" + "testing" + "time" + + "gitea.maximumdirect.net/eric/notarius/internal/framework/contracts" +) + +func TestDefaultRunIDGeneratorProducesUniqueSafeIDs(t *testing.T) { + startedAt := time.Unix(0, 123456789).UTC() + pattern := regexp.MustCompile(`^run-123456789-[0-9a-f]{32}$`) + seen := make(map[string]struct{}, 256) + for i := 0; i < 256; i++ { + runID, err := defaultRunIDGenerator(startedAt) + if err != nil { + t.Fatal(err) + } + if !pattern.MatchString(runID) { + t.Fatalf("run ID %q does not match production format", runID) + } + if err := validateRunID(runID); err != nil { + t.Fatalf("run ID %q is not path-safe: %v", runID, err) + } + if _, exists := seen[runID]; exists { + t.Fatalf("duplicate run ID %q", runID) + } + seen[runID] = struct{}{} + } +} + +func TestWriteOutputFilesSupportsNestedLogicalPaths(t *testing.T) { + runPath := filepath.Join(t.TempDir(), "output", "run-safe") + if err := writeOutputFiles(runPath, []contracts.OutputFile{{Name: "nested/result.json", Bytes: []byte("result")}}); err != nil { + t.Fatal(err) + } + data, err := os.ReadFile(filepath.Join(runPath, "nested", "result.json")) + if err != nil || string(data) != "result" { + t.Fatalf("nested output = %q, %v", data, err) + } +} + +func TestWriteOutputFilesRejectsUnsafeNamesBeforeAllocatingRunDirectory(t *testing.T) { + outputRoot := filepath.Join(t.TempDir(), "output") + runPath := filepath.Join(outputRoot, "run-safe") + for _, name := range []string{"", "../outside", "/absolute", `nested\\outside`, "nested/../outside"} { + t.Run(name, func(t *testing.T) { + if err := writeOutputFiles(runPath, []contracts.OutputFile{{Name: "safe.json"}, {Name: name}}); err == nil { + t.Fatalf("writeOutputFiles accepted %q", name) + } + if _, err := os.Stat(outputRoot); !os.IsNotExist(err) { + t.Fatalf("output root exists or stat failed after %q: %v", name, err) + } + }) + } +} + +func TestWriteOutputFilesRetainsNewPartialDirectoryAndPreservesSibling(t *testing.T) { + outputRoot := filepath.Join(t.TempDir(), "output") + siblingPath := filepath.Join(outputRoot, "sibling") + if err := os.MkdirAll(siblingPath, 0o755); err != nil { + t.Fatal(err) + } + sentinelPath := filepath.Join(siblingPath, "sentinel") + if err := os.WriteFile(sentinelPath, []byte("preserve sibling"), 0o644); err != nil { + t.Fatal(err) + } + + runPath := filepath.Join(outputRoot, "run-safe") + err := writeOutputFiles(runPath, []contracts.OutputFile{ + {Name: "blocked", Bytes: []byte("partial output")}, + {Name: "blocked/nested.json", Bytes: []byte("unreachable")}, + }) + if err == nil || !strings.Contains(err.Error(), "create output directory") { + t.Fatalf("writeOutputFiles() error = %v, want later directory failure", err) + } + if got, err := os.ReadFile(filepath.Join(runPath, "blocked")); err != nil || string(got) != "partial output" { + t.Fatalf("partial output = %q, %v", got, err) + } + if got, err := os.ReadFile(sentinelPath); err != nil || string(got) != "preserve sibling" { + t.Fatalf("sibling sentinel = %q, %v", got, err) + } +} diff --git a/internal/cli/state_hardening_test.go b/internal/cli/state_hardening_test.go index 1c8227f..838aea4 100644 --- a/internal/cli/state_hardening_test.go +++ b/internal/cli/state_hardening_test.go @@ -3,6 +3,7 @@ package cli import ( "bytes" "context" + "encoding/json" "errors" "fmt" "os" @@ -166,7 +167,7 @@ func TestRunRetainsDebugBundlesAcrossFailures(t *testing.T) { h.extractErr = errors.New("synthetic extraction failure") return h.options() }}, - {"output", "create output directory", func(t *testing.T, roots stateTestRoots, h *stateTestHarness) Options { + {"output", "create output parent", func(t *testing.T, roots stateTestRoots, h *stateTestHarness) Options { if err := os.WriteFile(roots.output, []byte("not a directory"), 0o600); err != nil { t.Fatal(err) } @@ -291,6 +292,135 @@ func TestRunRedactsSensitiveModuleOptionsFromConfigAndPipelineSummaries(t *testi } } +func TestRunUsesOneInjectedIdentityForDebugOutputAndManifest(t *testing.T) { + roots := newStateTestRoots(t) + harness := newStateTestHarness() + opts := harness.options() + const runID = "run-1000000000-11111111111111111111111111111111" + opts.RunIDGenerator = func(time.Time) (string, error) { return runID, nil } + + result := runStateTest(t, roots, opts, true, false, "bypass") + if result.code != 0 { + t.Fatalf("code=%d stderr=%q", result.code, result.stderr) + } + outputPath := filepath.Join(roots.output, runID) + debugPath := filepath.Join(roots.debug, runID) + assertFile(t, filepath.Join(outputPath, "result.json")) + assertFile(t, filepath.Join(debugPath, "summary", "run-manifest.json")) + if !strings.Contains(result.stdout, "output="+outputPath) || !strings.Contains(result.stdout, "debug="+debugPath) { + t.Fatalf("stdout=%q, want shared run identity", result.stdout) + } + data, err := os.ReadFile(filepath.Join(debugPath, "summary", "run-manifest.json")) + if err != nil { + t.Fatal(err) + } + var manifest artifacts.RunManifest + if err := json.Unmarshal(data, &manifest); err != nil { + t.Fatal(err) + } + if manifest.RunID != runID { + t.Fatalf("manifest run ID = %q, want %q", manifest.RunID, runID) + } +} + +func TestRunRefusesExistingOutputDirectoryWithoutChangingIt(t *testing.T) { + roots := newStateTestRoots(t) + const runID = "run-1000000000-22222222222222222222222222222222" + runPath := filepath.Join(roots.output, runID) + if err := os.MkdirAll(filepath.Join(runPath, "nested"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(runPath, "sentinel"), []byte("existing output"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(runPath, "nested", "data"), []byte("preserve me"), 0o644); err != nil { + t.Fatal(err) + } + before := readTree(t, runPath) + opts := newStateTestHarness().options() + opts.RunIDGenerator = func(time.Time) (string, error) { return runID, nil } + + result := runStateTest(t, roots, opts, false, false, "bypass") + if result.code != 1 || !strings.Contains(result.stderr, "output run directory") || !strings.Contains(result.stderr, "already exists") { + t.Fatalf("code=%d stderr=%q", result.code, result.stderr) + } + if after := readTree(t, runPath); !sameFiles(after, before) { + t.Fatalf("existing output changed: before=%v after=%v", before, after) + } +} + +func TestRepeatedRunIdentityCannotOverwriteFirstOutput(t *testing.T) { + roots := newStateTestRoots(t) + const runID = "run-1000000000-33333333333333333333333333333333" + harness := newStateTestHarness() + opts := harness.options() + opts.RunIDGenerator = func(time.Time) (string, error) { return runID, nil } + + first := runStateTest(t, roots, opts, false, false, "bypass") + if first.code != 0 { + t.Fatalf("first code=%d stderr=%q", first.code, first.stderr) + } + runPath := filepath.Join(roots.output, runID) + before := readTree(t, runPath) + second := runStateTest(t, roots, opts, false, false, "bypass") + if second.code != 1 || !strings.Contains(second.stderr, "already exists") { + t.Fatalf("second code=%d stderr=%q", second.code, second.stderr) + } + if after := readTree(t, runPath); !sameFiles(after, before) { + t.Fatalf("first output changed: before=%v after=%v", before, after) + } +} + +func TestRunRefusesExistingDebugBundleWithoutChangingIt(t *testing.T) { + roots := newStateTestRoots(t) + const runID = "run-1000000000-44444444444444444444444444444444" + bundlePath := filepath.Join(roots.debug, runID) + if err := os.MkdirAll(bundlePath, 0o700); err != nil { + t.Fatal(err) + } + sentinelPath := filepath.Join(bundlePath, "sentinel") + if err := os.WriteFile(sentinelPath, []byte("existing debug"), 0o600); err != nil { + t.Fatal(err) + } + opts := newStateTestHarness().options() + opts.RunIDGenerator = func(time.Time) (string, error) { return runID, nil } + + result := runStateTest(t, roots, opts, true, false, "bypass") + if result.code != 1 || !strings.Contains(result.stderr, "debug bundle") || !strings.Contains(result.stderr, "already exists") { + t.Fatalf("code=%d stderr=%q", result.code, result.stderr) + } + if got, err := os.ReadFile(sentinelPath); err != nil || string(got) != "existing debug" { + t.Fatalf("sentinel = %q, %v", got, err) + } + assertAbsent(t, roots.output) +} + +func TestRunIDGenerationFailurePrecedesDebugAllocation(t *testing.T) { + roots := newStateTestRoots(t) + opts := newStateTestHarness().options() + opts.RunIDGenerator = func(time.Time) (string, error) { return "", errors.New("random source unavailable") } + + result := runStateTest(t, roots, opts, true, false, "bypass") + if result.code != 1 || !strings.Contains(result.stderr, "generate run ID: random source unavailable") { + t.Fatalf("code=%d stderr=%q", result.code, result.stderr) + } + assertAbsent(t, roots.debug) + assertAbsent(t, roots.output) +} + +func TestRunRejectsUnsafeGeneratedIdentityBeforePathUse(t *testing.T) { + roots := newStateTestRoots(t) + opts := newStateTestHarness().options() + opts.RunIDGenerator = func(time.Time) (string, error) { return "../outside", nil } + + result := runStateTest(t, roots, opts, true, false, "bypass") + if result.code != 1 || !strings.Contains(result.stderr, "invalid generated run ID") || !strings.Contains(result.stderr, "one safe path component") { + t.Fatalf("code=%d stderr=%q", result.code, result.stderr) + } + assertAbsent(t, roots.debug) + assertAbsent(t, roots.output) +} + type stateTestRoots struct{ config, input, output, plans, checkpoints, debug string } func newStateTestRoots(t *testing.T) stateTestRoots { @@ -456,6 +586,7 @@ func sameFiles(left, right map[string][]byte) bool { type stateTestHarness struct { mu sync.Mutex chunkCalls, extractCalls int + runIDCalls uint64 extractErr error } @@ -483,7 +614,12 @@ func (h *stateTestHarness) options() Options { if err := registries.Outputs.RegisterWithSpec(pipeline.ModuleSpec{Key: "test/output", Stage: pipeline.StageOutput, Requires: []string{"normalized"}, Provides: []string{"output"}}, func() (contracts.OutputEncoder, error) { return stateTestOutput{}, nil }); err != nil { panic(err) } - return Options{Catalog: catalogFromRegistries(registries), Registries: registries, LookupEnv: emptyLookup, Now: func() time.Time { return time.Unix(1, 0) }, UserCacheDir: func() (string, error) { return "", errors.New("unexpected user cache lookup") }, LLMClientFactory: func(context.Context, config.Config, string) (contracts.StructuredLLMClient, []artifacts.LLMProfileManifest, error) { + return Options{Catalog: catalogFromRegistries(registries), Registries: registries, LookupEnv: emptyLookup, Now: func() time.Time { return time.Unix(1, 0) }, RunIDGenerator: func(startedAt time.Time) (string, error) { + h.mu.Lock() + defer h.mu.Unlock() + h.runIDCalls++ + return fmt.Sprintf("run-%d-%032x", startedAt.UnixNano(), h.runIDCalls), nil + }, UserCacheDir: func() (string, error) { return "", errors.New("unexpected user cache lookup") }, LLMClientFactory: func(context.Context, config.Config, string) (contracts.StructuredLLMClient, []artifacts.LLMProfileManifest, error) { return nil, nil, nil }} } diff --git a/internal/cli/state_surfaces_test.go b/internal/cli/state_surfaces_test.go index f424b52..785edbe 100644 --- a/internal/cli/state_surfaces_test.go +++ b/internal/cli/state_surfaces_test.go @@ -6,8 +6,13 @@ import ( "path/filepath" "strings" "testing" + "time" ) +const stateSurfaceRunID = "run-1000000000-55555555555555555555555555555555" + +func stateSurfaceRunIDGenerator(time.Time) (string, error) { return stateSurfaceRunID, nil } + func TestRunRejectsDebugDirectoryWithoutDebug(t *testing.T) { var stdout, stderr bytes.Buffer code := RunWithOptions([]string{"run", "example", "--input", "source.json", "--debug-dir", t.TempDir()}, &stdout, &stderr, Options{}) @@ -20,7 +25,7 @@ func TestRunDebugAllocatesBeforePipelineResolution(t *testing.T) { root := t.TempDir() configPath := writeV3Config(t, "") var stdout, stderr bytes.Buffer - code := RunWithOptions([]string{"run", "missing", "--config", configPath, "--input", "source.json", "--debug", "--debug-dir", root, "--chunk_cache", "bypass"}, &stdout, &stderr, Options{LookupEnv: emptyLookup}) + code := RunWithOptions([]string{"run", "missing", "--config", configPath, "--input", "source.json", "--debug", "--debug-dir", root, "--chunk_cache", "bypass"}, &stdout, &stderr, Options{LookupEnv: emptyLookup, RunIDGenerator: stateSurfaceRunIDGenerator}) if code != 1 { t.Fatalf("code=%d stderr=%q", code, stderr.String()) } @@ -28,7 +33,10 @@ func TestRunDebugAllocatesBeforePipelineResolution(t *testing.T) { if err != nil || len(entries) != 1 { t.Fatalf("debug bundles: %v, %v", entries, err) } - bundle := filepath.Join(root, entries[0].Name()) + if entries[0].Name() != stateSurfaceRunID { + t.Fatalf("debug bundle name = %q, want %q", entries[0].Name(), stateSurfaceRunID) + } + bundle := filepath.Join(root, stateSurfaceRunID) for _, name := range []string{"summary", "trace"} { if info, err := os.Stat(filepath.Join(bundle, name)); err != nil || !info.IsDir() { t.Fatalf("%s: %v", name, err) @@ -49,7 +57,7 @@ func TestRunWithoutDebugDoesNotAllocateDebugRoot(t *testing.T) { } return "", false } - code := RunWithOptions([]string{"run", "missing", "--config", configPath, "--input", "source.json", "--chunk_cache", "bypass"}, &stdout, &stderr, Options{LookupEnv: lookup}) + code := RunWithOptions([]string{"run", "missing", "--config", configPath, "--input", "source.json", "--chunk_cache", "bypass"}, &stdout, &stderr, Options{LookupEnv: lookup, RunIDGenerator: stateSurfaceRunIDGenerator}) if code != 1 { t.Fatalf("code=%d stderr=%q", code, stderr.String()) } diff --git a/internal/core/debugbundle/bundle.go b/internal/core/debugbundle/bundle.go index 3859bfa..3184f8f 100644 --- a/internal/core/debugbundle/bundle.go +++ b/internal/core/debugbundle/bundle.go @@ -9,47 +9,49 @@ import ( "time" ) -const maxCreateAttempts = 16 - -var utcNow = func() time.Time { return time.Now().UTC() } - type Bundle struct { path, summaryRoot, traceRoot string createdAt time.Time } -func Allocate(parent string) (*Bundle, error) { +func Allocate(parent, runID string, startedAt time.Time) (*Bundle, error) { parent = strings.TrimSpace(parent) if parent == "" { return nil, fmt.Errorf("debug parent must not be empty") } + if err := validateRunID(runID); err != nil { + return nil, err + } if err := os.MkdirAll(parent, 0o700); err != nil { return nil, fmt.Errorf("create debug parent %q: %w", parent, err) } - var last string - for attempt := 0; attempt < maxCreateAttempts; attempt++ { - createdAt := utcNow() - runID := fmt.Sprintf("run-%d", createdAt.UnixNano()) - path := filepath.Join(parent, runID) - last = path - if err := os.Mkdir(path, 0o700); err != nil { - if os.IsExist(err) { - continue - } - return nil, fmt.Errorf("create debug bundle %q: %w", path, err) + path := filepath.Join(parent, runID) + if err := os.Mkdir(path, 0o700); err != nil { + if os.IsExist(err) { + return nil, fmt.Errorf("debug bundle %q already exists", path) } - summary, trace := filepath.Join(path, "summary"), filepath.Join(path, "trace") - if err := os.Mkdir(summary, 0o700); err != nil { - _ = os.Remove(path) - return nil, fmt.Errorf("create debug summary %q: %w", summary, err) - } - if err := os.Mkdir(trace, 0o700); err != nil { - _ = os.RemoveAll(path) - return nil, fmt.Errorf("create debug trace %q: %w", trace, err) - } - return &Bundle{path: path, summaryRoot: summary, traceRoot: trace, createdAt: createdAt}, nil + return nil, fmt.Errorf("create debug bundle %q: %w", path, err) } - return nil, fmt.Errorf("create debug bundle %q: exhausted unique run ID attempts", last) + summary, trace := filepath.Join(path, "summary"), filepath.Join(path, "trace") + if err := os.Mkdir(summary, 0o700); err != nil { + _ = os.Remove(path) + return nil, fmt.Errorf("create debug summary %q: %w", summary, err) + } + if err := os.Mkdir(trace, 0o700); err != nil { + _ = os.RemoveAll(path) + return nil, fmt.Errorf("create debug trace %q: %w", trace, err) + } + return &Bundle{path: path, summaryRoot: summary, traceRoot: trace, createdAt: startedAt}, nil +} + +func validateRunID(runID string) error { + if runID == "" { + return fmt.Errorf("debug run ID must not be empty") + } + if runID != strings.TrimSpace(runID) || strings.ContainsAny(runID, `/\\`) || filepath.IsAbs(runID) || filepath.Clean(runID) != runID || runID == "." || runID == ".." { + return fmt.Errorf("debug run ID %q must be one safe path component", runID) + } + return nil } func (b *Bundle) Path() string { if b == nil { diff --git a/internal/core/debugbundle/bundle_test.go b/internal/core/debugbundle/bundle_test.go index 46498af..ab65946 100644 --- a/internal/core/debugbundle/bundle_test.go +++ b/internal/core/debugbundle/bundle_test.go @@ -1,8 +1,10 @@ package debugbundle import ( + "bytes" "os" "path/filepath" + "strings" "testing" "time" @@ -11,17 +13,16 @@ import ( "gitea.maximumdirect.net/eric/notarius/internal/framework/pipeline" ) +const testBundleRunID = "run-42-00000000000000000000000000000001" + func TestAllocateCreatesRestrictiveSummaryAndTrace(t *testing.T) { parent := t.TempDir() fixed := time.Unix(0, 42).UTC() - previous := utcNow - utcNow = func() time.Time { return fixed } - defer func() { utcNow = previous }() - bundle, err := Allocate(parent) + bundle, err := Allocate(parent, testBundleRunID, fixed) if err != nil { t.Fatal(err) } - if bundle.RunID() != "run-42" || bundle.SummaryRoot() != filepath.Join(bundle.Path(), "summary") || bundle.TraceRoot() != filepath.Join(bundle.Path(), "trace") { + if bundle.RunID() != testBundleRunID || bundle.CreatedAt() != fixed || bundle.SummaryRoot() != filepath.Join(bundle.Path(), "summary") || bundle.TraceRoot() != filepath.Join(bundle.Path(), "trace") { t.Fatalf("bundle=%#v", bundle) } for _, path := range []string{bundle.Path(), bundle.SummaryRoot(), bundle.TraceRoot()} { @@ -44,45 +45,42 @@ func TestAllocateCreatesRestrictiveSummaryAndTrace(t *testing.T) { t.Fatalf("file mode=%#o", info.Mode().Perm()) } } -func TestAllocateRetriesAndDoesNotDeleteBundle(t *testing.T) { +func TestAllocateRejectsExistingBundleWithoutChangingIt(t *testing.T) { parent := t.TempDir() - fixed := time.Unix(0, 9).UTC() - previous := utcNow - defer func() { utcNow = previous }() - calls := 0 - utcNow = func() time.Time { calls++; return fixed.Add(time.Duration(calls-1) * time.Nanosecond) } - if err := os.Mkdir(filepath.Join(parent, "run-9"), 0o700); err != nil { + bundlePath := filepath.Join(parent, testBundleRunID) + if err := os.Mkdir(bundlePath, 0o700); err != nil { t.Fatal(err) } - bundle, err := Allocate(parent) - if err != nil { + sentinelPath := filepath.Join(bundlePath, "sentinel") + sentinel := []byte("existing bundle") + if err := os.WriteFile(sentinelPath, sentinel, 0o600); err != nil { t.Fatal(err) } - if bundle.RunID() != "run-10" { - t.Fatalf("run id=%q", bundle.RunID()) + + if _, err := Allocate(parent, testBundleRunID, time.Unix(0, 42)); err == nil || !strings.Contains(err.Error(), "already exists") { + t.Fatalf("Allocate() error = %v, want collision", err) } - if _, err := os.Stat(bundle.Path()); err != nil { - t.Fatal(err) + if got, err := os.ReadFile(sentinelPath); err != nil || !bytes.Equal(got, sentinel) { + t.Fatalf("sentinel = %q, %v", got, err) } } -func TestAllocateReturnsErrorAfterRunIDCollisionsAreExhausted(t *testing.T) { - parent := t.TempDir() - fixed := time.Unix(0, 99).UTC() - if err := os.Mkdir(filepath.Join(parent, "run-99"), 0o700); err != nil { - t.Fatal(err) - } - previous := utcNow - utcNow = func() time.Time { return fixed } - defer func() { utcNow = previous }() - - if _, err := Allocate(parent); err == nil { - t.Fatal("Allocate succeeded after exhausting run ID collisions") +func TestAllocateRejectsUnsafeRunIDsBeforeCreatingParent(t *testing.T) { + for _, runID := range []string{"", ".", "..", "../escape", `..\\escape`, "/absolute", " trailing "} { + t.Run(runID, func(t *testing.T) { + parent := filepath.Join(t.TempDir(), "debug") + if _, err := Allocate(parent, runID, time.Time{}); err == nil { + t.Fatalf("Allocate(%q) succeeded", runID) + } + if _, err := os.Stat(parent); !os.IsNotExist(err) { + t.Fatalf("debug parent exists or stat failed: %v", err) + } + }) } } func TestSummaryWriterWritesEverySummaryArtifact(t *testing.T) { - bundle, err := Allocate(t.TempDir()) + bundle, err := Allocate(t.TempDir(), testBundleRunID, time.Unix(0, 42)) if err != nil { t.Fatal(err) } @@ -140,7 +138,7 @@ func TestSummaryWriterWritesEverySummaryArtifact(t *testing.T) { } } func TestSummaryWriterConfinesArtifacts(t *testing.T) { - bundle, err := Allocate(t.TempDir()) + bundle, err := Allocate(t.TempDir(), testBundleRunID, time.Unix(0, 42)) if err != nil { t.Fatal(err) }