From bf2c0f692d24877a1c5bcc3e414ec1fcbaf74c80 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 3 Jul 2026 20:54:22 +0000 Subject: [PATCH] Harden CLI lane filtering and diagnostics redaction --- internal/cli/run.go | 19 ++++--- internal/cli/run_test.go | 26 +++++++++ internal/core/config/redaction.go | 43 +++++++++++++++ internal/core/config/redaction_test.go | 64 +++++++++++++++++++++++ internal/core/diagnostics/run_dir.go | 11 +++- internal/core/diagnostics/run_dir_test.go | 28 +++++++++- 6 files changed, 182 insertions(+), 9 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index 1a3113b..20aaa22 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -98,6 +98,11 @@ func runConfigValidate(args []string, stdout, stderr io.Writer, opts Options) in fmt.Fprintln(stderr, "notarius: --only requires --pipeline") return 2 } + only, err := parseOnly(*onlyRaw) + if err != nil { + fmt.Fprintf(stderr, "notarius: %v\n", err) + return 2 + } cfg, path, err := loadConfig(*configPath, opts) if err != nil { @@ -108,7 +113,7 @@ func runConfigValidate(args []string, stdout, stderr io.Writer, opts Options) in if strings.TrimSpace(*pipelineID) != "" { if _, err := cfg.Resolve(config.ResolveInput{ PipelineID: *pipelineID, - Only: parseOnly(*onlyRaw), + Only: only, Catalog: opts.Catalog, }); err != nil { fmt.Fprintf(stderr, "notarius: %v\n", err) @@ -239,18 +244,20 @@ func requireConfigFile(path string) error { return nil } -func parseOnly(raw string) []string { +func parseOnly(raw string) ([]string, error) { if strings.TrimSpace(raw) == "" { - return nil + return nil, nil } parts := strings.Split(raw, ",") result := make([]string, 0, len(parts)) for _, part := range parts { - if trimmed := strings.TrimSpace(part); trimmed != "" { - result = append(result, trimmed) + trimmed := strings.TrimSpace(part) + if trimmed == "" { + return nil, fmt.Errorf("--only must contain comma-separated non-empty artifact lane IDs") } + result = append(result, trimmed) } - return result + return result, nil } func sortedPipelineIDs(cfg config.Config) []string { diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index c240a8a..5e1a8a4 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -168,6 +168,32 @@ func TestRunConfigValidateOnlyWithoutPipelineFails(t *testing.T) { } } +func TestRunConfigValidateRejectsMalformedOnlyValues(t *testing.T) { + configPath := writeTestConfig(t, testConfigYAML("example", "events", "notes")) + tests := []string{",", "notes,", ",notes", "events, ,notes"} + + for _, only := range tests { + t.Run(only, func(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + + code := RunWithOptions([]string{"config", "validate", "--config", configPath, "--pipeline", "example", "--only", only}, &stdout, &stderr, Options{ + Catalog: fakeCatalog(t), + }) + + if code != 2 { + t.Fatalf("RunWithOptions() code = %d, want 2", code) + } + if stdout.Len() != 0 { + t.Fatalf("stdout = %q, want empty", stdout.String()) + } + if !strings.Contains(stderr.String(), "--only") { + t.Fatalf("stderr = %q, want --only error", stderr.String()) + } + }) + } +} + func TestRunPipelinesListSortedTextOutput(t *testing.T) { configPath := writeTestConfig(t, testConfigYAMLForPipelines(map[string][]string{ "zeta": {"events"}, diff --git a/internal/core/config/redaction.go b/internal/core/config/redaction.go index ea220dc..ee41b21 100644 --- a/internal/core/config/redaction.go +++ b/internal/core/config/redaction.go @@ -1,5 +1,7 @@ package config +import "gitea.maximumdirect.net/eric/notarius/internal/framework/pipeline" + const redactedSecret = "[REDACTED]" func (c Config) Redacted() Config { @@ -12,3 +14,44 @@ func (c Config) Redacted() Config { } return redacted } + +func (c Config) RedactedDiagnosticsPayload() any { + return c.Redacted() +} + +func (e EffectiveConfig) RedactedDiagnosticsPayload() any { + return EffectiveConfig{ + Config: e.Config.Redacted(), + PipelineID: e.PipelineID, + Only: append([]string(nil), e.Only...), + ResolvedPipeline: cloneResolvedPipeline(e.ResolvedPipeline), + } +} + +func cloneResolvedPipeline(in pipeline.ResolvedPipeline) pipeline.ResolvedPipeline { + out := in + out.Input = cloneModuleBinding(in.Input) + out.Chunk = cloneModuleBinding(in.Chunk) + out.Output = cloneModuleBinding(in.Output) + if len(in.ArtifactLanes) > 0 { + out.ArtifactLanes = make([]pipeline.ResolvedArtifactLane, len(in.ArtifactLanes)) + for i, lane := range in.ArtifactLanes { + out.ArtifactLanes[i] = cloneResolvedArtifactLane(lane) + } + } + return out +} + +func cloneResolvedArtifactLane(in pipeline.ResolvedArtifactLane) pipeline.ResolvedArtifactLane { + out := in + out.Extract = cloneModuleBinding(in.Extract) + out.Merge = cloneModuleBinding(in.Merge) + out.Normalize = cloneModuleBinding(in.Normalize) + if len(in.Validators) > 0 { + out.Validators = make([]pipeline.ModuleBinding, len(in.Validators)) + for i, binding := range in.Validators { + out.Validators[i] = cloneModuleBinding(binding) + } + } + return out +} diff --git a/internal/core/config/redaction_test.go b/internal/core/config/redaction_test.go index 5e39793..178c741 100644 --- a/internal/core/config/redaction_test.go +++ b/internal/core/config/redaction_test.go @@ -35,3 +35,67 @@ func TestRedactedConfigRemovesAPIKeyValues(t *testing.T) { t.Fatalf("redaction mutated original config") } } + +func TestConfigRedactedDiagnosticsPayloadRedactsAPIKeys(t *testing.T) { + cfg := Default() + profile := cfg.LLMProfiles[pipeline.DefaultLLMProfile] + profile.APIKey = "secret" + profile.Model = "test-model" + cfg.LLMProfiles[pipeline.DefaultLLMProfile] = profile + + payload, ok := cfg.RedactedDiagnosticsPayload().(Config) + if !ok { + t.Fatalf("expected Config payload, got %T", cfg.RedactedDiagnosticsPayload()) + } + if payload.LLMProfiles[pipeline.DefaultLLMProfile].APIKey != redactedSecret { + t.Fatalf("expected API key redacted, got %+v", payload.LLMProfiles[pipeline.DefaultLLMProfile]) + } + if payload.LLMProfiles[pipeline.DefaultLLMProfile].Model != "test-model" { + t.Fatalf("expected non-secret fields preserved, got %+v", payload.LLMProfiles[pipeline.DefaultLLMProfile]) + } + if cfg.LLMProfiles[pipeline.DefaultLLMProfile].APIKey != "secret" { + t.Fatalf("redacted diagnostics payload mutated original config") + } +} + +func TestEffectiveConfigRedactedDiagnosticsPayloadRedactsAndCopies(t *testing.T) { + cfg := validConfig() + profile := cfg.LLMProfiles[pipeline.DefaultLLMProfile] + profile.APIKey = "secret" + cfg.LLMProfiles[pipeline.DefaultLLMProfile] = profile + lane := cfg.Pipelines["example"].Artifacts["events"] + lane.Extract.Options = map[string]any{"temperature": 0.2} + cfg.Pipelines["example"].Artifacts["events"] = lane + + effective, err := cfg.Resolve(ResolveInput{ + PipelineID: "example", + Only: []string{"events"}, + Catalog: fakeCatalog(t), + }) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + + payload, ok := effective.RedactedDiagnosticsPayload().(EffectiveConfig) + if !ok { + t.Fatalf("expected EffectiveConfig payload, got %T", effective.RedactedDiagnosticsPayload()) + } + if payload.Config.LLMProfiles[pipeline.DefaultLLMProfile].APIKey != redactedSecret { + t.Fatalf("expected nested API key redacted, got %+v", payload.Config.LLMProfiles[pipeline.DefaultLLMProfile]) + } + if cfg.LLMProfiles[pipeline.DefaultLLMProfile].APIKey != "secret" { + t.Fatalf("redacted diagnostics payload mutated source config") + } + if payload.PipelineID != effective.PipelineID || payload.ResolvedPipeline.Digest != effective.ResolvedPipeline.Digest { + t.Fatalf("expected pipeline metadata preserved, got %+v", payload) + } + + payload.Only[0] = "changed" + if effective.Only[0] != "events" { + t.Fatalf("expected only lanes to be copied") + } + payload.ResolvedPipeline.ArtifactLanes[0].Extract.Options["temperature"] = 1.0 + if effective.ResolvedPipeline.ArtifactLanes[0].Extract.Options["temperature"] != 0.2 { + t.Fatalf("expected resolved pipeline options to be copied") + } +} diff --git a/internal/core/diagnostics/run_dir.go b/internal/core/diagnostics/run_dir.go index 0661ca9..3616ab2 100644 --- a/internal/core/diagnostics/run_dir.go +++ b/internal/core/diagnostics/run_dir.go @@ -42,6 +42,10 @@ type RetentionDecisionInput struct { HasWarnings bool } +type RedactedEffectiveConfigPayload interface { + RedactedDiagnosticsPayload() any +} + // InvocationMetadata captures non-secret invocation details for diagnostics. type InvocationMetadata struct { Operation string `json:"operation"` @@ -134,8 +138,11 @@ func (r *RunDirectory) WriteInvocationMetadata(metadata InvocationMetadata) erro return r.WriteJSONArtifact(ArtifactInvocationMetadata, metadata) } -func (r *RunDirectory) WriteRedactedEffectiveConfig(payload any) error { - return r.WriteJSONArtifact(ArtifactEffectiveConfig, payload) +func (r *RunDirectory) WriteRedactedEffectiveConfig(payload RedactedEffectiveConfigPayload) error { + if payload == nil { + return fmt.Errorf("redacted effective config payload must not be nil") + } + return r.WriteJSONArtifact(ArtifactEffectiveConfig, payload.RedactedDiagnosticsPayload()) } func (r *RunDirectory) WriteResolvedPipeline(payload any) error { diff --git a/internal/core/diagnostics/run_dir_test.go b/internal/core/diagnostics/run_dir_test.go index 4f7ee16..e20a224 100644 --- a/internal/core/diagnostics/run_dir_test.go +++ b/internal/core/diagnostics/run_dir_test.go @@ -167,7 +167,7 @@ func TestWriteInvocationMetadataPreservesProvidedRunIDAndStartTime(t *testing.T) func TestWriteTypedArtifacts(t *testing.T) { runDir := newTestRunDirectory(t) - if err := runDir.WriteRedactedEffectiveConfig(map[string]any{"redacted": true}); err != nil { + if err := runDir.WriteRedactedEffectiveConfig(fakeRedactedEffectiveConfig{payload: map[string]any{"redacted": true}}); err != nil { t.Fatalf("WriteRedactedEffectiveConfig: %v", err) } if err := runDir.WriteResolvedPipeline(map[string]any{"pipeline": "test"}); err != nil { @@ -200,6 +200,24 @@ func TestWriteTypedArtifacts(t *testing.T) { } } +func TestWriteRedactedEffectiveConfigWritesPayloadReturnedByProvider(t *testing.T) { + runDir := newTestRunDirectory(t) + + if err := runDir.WriteRedactedEffectiveConfig(fakeRedactedEffectiveConfig{ + payload: map[string]any{ + "api_key": "[REDACTED]", + "model": "test-model", + }, + }); err != nil { + t.Fatalf("WriteRedactedEffectiveConfig: %v", err) + } + + data := string(readArtifact(t, runDir, ArtifactEffectiveConfig)) + if !strings.Contains(data, `"api_key": "[REDACTED]"`) || !strings.Contains(data, `"model": "test-model"`) { + t.Fatalf("unexpected effective config artifact: %s", data) + } +} + func TestWriteErrorLogWritesPlainTextWithTrailingNewline(t *testing.T) { runDir := newTestRunDirectory(t) @@ -320,3 +338,11 @@ func readArtifact(t *testing.T, runDir *RunDirectory, name string) []byte { } return data } + +type fakeRedactedEffectiveConfig struct { + payload any +} + +func (f fakeRedactedEffectiveConfig) RedactedDiagnosticsPayload() any { + return f.payload +}