From 3ce198197d58ea30bd76ed5531991b8988bd7b4c Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 7 Sep 2026 12:14:10 +0000 Subject: [PATCH] Report secrets bootstrap outcomes safely --- docs/roadmap/remediation.md | 10 +++- internal/app/command_dependencies.go | 5 ++ internal/app/commands.go | 3 ++ internal/app/config_commands_test.go | 17 +++++- internal/app/config_loader.go | 2 +- internal/app/secrets_bootstrap.go | 8 ++- internal/app/secrets_bootstrap_test.go | 72 +++++++++++++++++++++++++- 7 files changed, 110 insertions(+), 7 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 1a48246..decf27a 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -206,7 +206,15 @@ retain their relevant contributors. Verify catalog and sources agree after expan ## 6. Surface successful secrets bootstrap reports, including skipped empty variables -Status: Open — P3 +Status: Resolved — P3 + +Resolution: Successful bootstrap reports now flow once through the command +diagnostics channel, while direct internal command calls default that channel to +discard. ConfigKit's value-free report makes skipped existing empty variables +explicit and remains available before later typed-load failures; structured output +stays isolated on stdout. Command-level coverage verifies successful reporting, +skipped-empty reporting, value redaction, clean config-show output, and one report +for a two-profile diff. **Evidence:** `internal/app/secrets_bootstrap.go:36` discards the report whenever `LoadSecretsDir` returns no error. Reports are only rendered when directory diff --git a/internal/app/command_dependencies.go b/internal/app/command_dependencies.go index 77ef66e..48b0ee6 100644 --- a/internal/app/command_dependencies.go +++ b/internal/app/command_dependencies.go @@ -2,6 +2,7 @@ package app import ( "context" + "io" configkit "gitea.maximumdirect.net/eric/ConfigKit" "gitea.maximumdirect.net/eric/narratio/internal/adapters/storage" @@ -9,6 +10,7 @@ import ( ) type commandDependencies struct { + diagnostics io.Writer loadPipelineConfig func(string, config.PipelineLoadOptions) (*config.PipelineConfig, error) downloadObjectToTemp func(context.Context, storage.ObjectStore, string, string) (string, error) loadSecretsDir func(string) (*configkit.SecretsReport, error) @@ -20,6 +22,9 @@ type commandDependencies struct { } func normalizeCommandDependencies(deps commandDependencies) commandDependencies { + if deps.diagnostics == nil { + deps.diagnostics = io.Discard + } if deps.loadPipelineConfig == nil { deps.loadPipelineConfig = config.LoadPipelineWithOptions } diff --git a/internal/app/commands.go b/internal/app/commands.go index 4f93d02..2d172d1 100644 --- a/internal/app/commands.go +++ b/internal/app/commands.go @@ -19,6 +19,9 @@ func executeWithPipelineExecutor(args []string, stdout, stderr io.Writer, execut } func executeWithDependencies(args []string, stdout, stderr io.Writer, deps commandDependencies, executor pipelineExecutor) int { + if deps.diagnostics == nil { + deps.diagnostics = stderr + } deps = normalizeCommandDependencies(deps) if len(args) == 0 { printUsage(stderr) diff --git a/internal/app/config_commands_test.go b/internal/app/config_commands_test.go index 83e5e53..92b4185 100644 --- a/internal/app/config_commands_test.go +++ b/internal/app/config_commands_test.go @@ -331,14 +331,21 @@ func TestConfigDiffBootstrapsSecretsOnceForBothProfiles(t *testing.T) { pipelinePath, _, _ := writeValidConfigFiles(t, workspaceRoot) writeInspectionProfiles(t, pipelinePath, "whisperx:\n language: en\n", "whisperx:\n language: fr\n") secretsDir := t.TempDir() + const secretName = "NARRATIO_DIFF_BOOTSTRAP_REPORT" + restoreEnvAfterTest(t, secretName) + if err := os.Unsetenv(secretName); err != nil { + t.Fatal(err) + } + mustWriteSecretFile(t, filepath.Join(secretsDir, secretName), "diff-secret-must-not-appear\n") bootstrapCalls := 0 + var diagnostics bytes.Buffer deps := commandDependencies{loadSecretsDir: func(got string) (*configkit.SecretsReport, error) { bootstrapCalls++ if got != secretsDir { t.Fatalf("secrets directory = %q, want %q", got, secretsDir) } - return nil, nil - }} + return configkit.LoadSecretsDir(got) + }, diagnostics: &diagnostics} if err := configDiffWithDependencies(context.Background(), []string{ "production", "testing", "--config", pipelinePath, "--secrets-dir", secretsDir, @@ -348,6 +355,12 @@ func TestConfigDiffBootstrapsSecretsOnceForBothProfiles(t *testing.T) { if bootstrapCalls != 1 { t.Fatalf("bootstrap calls = %d, want 1", bootstrapCalls) } + if got := diagnostics.String(); strings.Count(got, `name="`+secretName+`"`) != 1 { + t.Fatalf("bootstrap report count = %d, want 1; diagnostics = %q", strings.Count(got, `name="`+secretName+`"`), got) + } + if strings.Contains(diagnostics.String(), "diff-secret-must-not-appear") { + t.Fatalf("bootstrap diagnostics exposed secret value: %q", diagnostics.String()) + } } func TestConfigInspectionOutputsNeverExposeResolvedCredentialValues(t *testing.T) { diff --git a/internal/app/config_loader.go b/internal/app/config_loader.go index 94644c5..ded80bc 100644 --- a/internal/app/config_loader.go +++ b/internal/app/config_loader.go @@ -171,7 +171,7 @@ func loadPipelineConfigWithDependencies(pipelineFlag string, opts config.Pipelin deps.secretsBootstrap.selection = secretsDir deps.secretsBootstrap.selectionSet = true } - if err := deps.secretsBootstrap.load(resolvedPipelinePath, deps.loadSecretsDir); err != nil { + if err := deps.secretsBootstrap.load(resolvedPipelinePath, deps.loadSecretsDir, deps.diagnostics); err != nil { return "", nil, err } pipelineCfg, err := deps.loadPipelineConfig(resolvedPipelinePath, opts) diff --git a/internal/app/secrets_bootstrap.go b/internal/app/secrets_bootstrap.go index 64fe182..91884b3 100644 --- a/internal/app/secrets_bootstrap.go +++ b/internal/app/secrets_bootstrap.go @@ -4,6 +4,7 @@ import ( "bytes" "errors" "fmt" + "io" "os" "path/filepath" "strings" @@ -17,7 +18,7 @@ type secretsBootstrap struct { called bool } -func (b *secretsBootstrap) load(pipelinePath string, loader func(string) (*configkit.SecretsReport, error)) error { +func (b *secretsBootstrap) load(pipelinePath string, loader func(string) (*configkit.SecretsReport, error), diagnostics io.Writer) error { if b == nil || b.called { return nil } @@ -35,6 +36,11 @@ func (b *secretsBootstrap) load(pipelinePath string, loader func(string) (*confi } report, loadErr := loader(directory) if loadErr == nil { + if report != nil { + if _, err := report.WriteTo(diagnostics); err != nil { + return fmt.Errorf("write secrets bootstrap report: %w", err) + } + } return nil } var rendered bytes.Buffer diff --git a/internal/app/secrets_bootstrap_test.go b/internal/app/secrets_bootstrap_test.go index 7197915..e4501a3 100644 --- a/internal/app/secrets_bootstrap_test.go +++ b/internal/app/secrets_bootstrap_test.go @@ -1,6 +1,8 @@ package app import ( + "bytes" + "io" "os" "path/filepath" "strings" @@ -99,9 +101,13 @@ func TestSecretsBootstrapPreservesExistingEnvironmentValues(t *testing.T) { } bootstrap := &secretsBootstrap{selection: &directory, selectionSet: true} - if err := bootstrap.load(filepath.Join(t.TempDir(), "pipeline.yml"), configkit.LoadSecretsDir); err != nil { + var diagnostics bytes.Buffer + if err := bootstrap.load(filepath.Join(t.TempDir(), "pipeline.yml"), configkit.LoadSecretsDir, &diagnostics); err != nil { t.Fatalf("bootstrap: %v", err) } + if got := diagnostics.String(); !strings.Contains(got, `status="skipped_existing"`) || !strings.Contains(got, `existing_empty=true`) { + t.Fatalf("diagnostics lack explicit skipped-empty result: %q", got) + } if got := os.Getenv(emptyName); got != "" { t.Fatalf("existing empty value = %q, want empty", got) } @@ -117,7 +123,7 @@ func TestSecretsBootstrapPreservesSafePartialReportOnFailure(t *testing.T) { directory := t.TempDir() mustWriteSecretFile(t, filepath.Join(directory, "INVALID-NAME"), "must-not-appear\n") bootstrap := &secretsBootstrap{selection: &directory, selectionSet: true} - err := bootstrap.load(filepath.Join(t.TempDir(), "pipeline.yml"), configkit.LoadSecretsDir) + err := bootstrap.load(filepath.Join(t.TempDir(), "pipeline.yml"), configkit.LoadSecretsDir, io.Discard) if err == nil { t.Fatal("bootstrap succeeded, want error") } @@ -129,4 +135,66 @@ func TestSecretsBootstrapPreservesSafePartialReportOnFailure(t *testing.T) { } } +func TestExecuteConfigShowReportsSuccessfulSecretsBootstrapOnStderr(t *testing.T) { + workspaceRoot := t.TempDir() + pipelinePath, _, _ := writeValidConfigFiles(t, workspaceRoot) + secretsDir := t.TempDir() + const name = "NARRATIO_BOOTSTRAP_REPORT_SECRET" + const canary = "synthetic-secret-must-not-appear" + restoreEnvAfterTest(t, name) + if err := os.Unsetenv(name); err != nil { + t.Fatal(err) + } + mustWriteSecretFile(t, filepath.Join(secretsDir, name), canary+"\n") + + var stdout bytes.Buffer + var stderr bytes.Buffer + if code := Execute([]string{"config", "show", "--config", pipelinePath, "--secrets-dir", secretsDir}, &stdout, &stderr); code != 0 { + t.Fatalf("config show exit = %d, stderr = %q", code, stderr.String()) + } + if got := stderr.String(); !strings.Contains(got, `status="loaded"`) || !strings.Contains(got, `name="`+name+`"`) { + t.Fatalf("stderr lacks safe bootstrap report: %q", got) + } + if strings.Contains(stderr.String(), canary) || strings.Contains(stdout.String(), canary) { + t.Fatal("command output exposed secret value") + } + if strings.Contains(stdout.String(), `status="loaded"`) || strings.Contains(stdout.String(), name) { + t.Fatalf("show stdout was polluted by bootstrap report: %q", stdout.String()) + } +} + +func TestExecuteConfigValidateReportsSkippedEmptyBeforeTypedLoadFailure(t *testing.T) { + workspaceRoot := t.TempDir() + pipelinePath, _, _ := writeValidConfigFiles(t, workspaceRoot) + const name = "NARRATIO_REVIEW_KEY" + const canary = "synthetic-skipped-secret-must-not-appear" + t.Setenv(name, "") + pipelineYAML, err := os.ReadFile(pipelinePath) + if err != nil { + t.Fatal(err) + } + pipelineYAML = []byte(strings.Replace(string(pipelineYAML), "audita:\n binary:", "audita:\n llm_api_key: "+name+"\n binary:", 1)) + if err := os.WriteFile(pipelinePath, pipelineYAML, 0o644); err != nil { + t.Fatal(err) + } + secretsDir := t.TempDir() + mustWriteSecretFile(t, filepath.Join(secretsDir, name), canary+"\n") + + var stdout bytes.Buffer + var stderr bytes.Buffer + if code := Execute([]string{"config", "validate", "--config", pipelinePath, "--secrets-dir", secretsDir}, &stdout, &stderr); code == 0 { + t.Fatal("config validate succeeded, want empty-secret failure") + } + got := stderr.String() + if !strings.Contains(got, `status="skipped_existing"`) || !strings.Contains(got, `existing_empty=true`) { + t.Fatalf("stderr lacks useful skipped-empty diagnostic: %q", got) + } + if !strings.Contains(got, "secret.empty") { + t.Fatalf("stderr lacks typed load failure: %q", got) + } + if strings.Contains(got, canary) || strings.Contains(stdout.String(), canary) { + t.Fatal("command output exposed skipped secret value") + } +} + func stringPointer(value string) *string { return &value }