fix(redact): reset redaction store between command runs (#5386)

* fix(redact): reset redaction store between command runs

cli.Command() may be invoked more than once in the same process (e.g.
when syft is embedded as a library). The clio initializer calls
internal/redact.Set on every command execution, but the redact store is
process-global and Set panics when a store already exists, so the second
invocation dies with "replace existing redaction store (probably
unintentional)".

Add redact.Reset() to clear the previous run's store and call it in the
initializer before Set. The double-Set guard in Set is kept: an
unexpected second Set within a single run still panics.

Add TestAppClioSetupConfigInitializerCanRunMultipleTimes which runs the
initializer twice; it panics with the exact reported message on the old
code and passes with the fix.

Fixes #2285

Signed-off-by: JasonMetal <935216773@qq.com>

* fix(redact): release the redact store at the end of each run

Clear the global redact store in the post-run hook instead of resetting it
before every `Set`. The double-`Set` panic keeps its meaning: a store is only
present while a run is in flight, so a second run starting mid-run still
panics rather than splitting secrets across two stores and leaking the ones
in the dropped store. Sequential runs in the same process work since the
previous run releases its store on the way out.

A run only releases the store it set, so it never clears a store owned by
someone else.

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

---------

Signed-off-by: JasonMetal <935216773@qq.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Co-authored-by: Alex Goodman <wagoodman@users.noreply.github.com>

cmd/syft/cli/cli_test.go

@@ -0,0 +1,25 @@
+package cli
+
+import (
+	"io"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/anchore/clio"
+	"github.com/anchore/syft/internal/redact"
+)
+
+func TestCommandCanRunMultipleTimes(t *testing.T) {
+	// https://github.com/anchore/syft/issues/2285
+	t.Cleanup(redact.Reset)
+
+	for i := 0; i < 2; i++ {
+		_, cmd := create(clio.Identification{Name: "syft"}, io.Discard)
+		cmd.SetArgs([]string{"cataloger", "list", "-o", "json"})
+		require.NoError(t, cmd.Execute())
+
+		// the store is released at the end of each run so the next one can set its own
+		require.Nil(t, redact.Get())
+	}
+}

cmd/syft/internal/clio_setup_config.go

@@ -41,6 +41,8 @@ func AppClioSetupConfig(id clio.Identification, out io.Writer) *clio.SetupConfig
 				stereoscope.SetBus(state.Bus)
 				bus.Set(state.Bus)
 
+				// the redact store is process-global and only cleared when a run ends (see the post-run below), so
+				// setting it while another run is still in flight panics rather than splitting secrets across stores.
 				redact.Set(state.RedactStore)
 
 				log.Set(state.Logger)
@@ -48,8 +50,14 @@ func AppClioSetupConfig(id clio.Identification, out io.Writer) *clio.SetupConfig
 				return nil
 			},
 		).
-		WithPostRuns(func(_ *clio.State, _ error) {
+		WithPostRuns(func(state *clio.State, _ error) {
 			stereoscope.Cleanup() //nolint:staticcheck // we don't have access to the image object here
+
+			// release the redact store now that nothing in this run can report anymore, so a later run in the same
+			// process (e.g. syft embedded as a library) can set its own. Only release the store this run set.
+			if redact.Get() == state.RedactStore {
+				redact.Reset()
+			}
 		})
 	return clioCfg
 }

cmd/syft/internal/clio_setup_config_test.go

@@ -0,0 +1,32 @@
+package internal
+
+import (
+	"io"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/anchore/clio"
+	"github.com/anchore/go-logger/adapter/discard"
+	gologgerredact "github.com/anchore/go-logger/adapter/redact"
+	"github.com/anchore/syft/internal/redact"
+)
+
+func TestAppClioSetupConfigInitializerPanicsWhileRunInFlight(t *testing.T) {
+	t.Cleanup(redact.Reset)
+
+	cfg := AppClioSetupConfig(clio.Identification{Name: "syft"}, io.Discard)
+	require.Len(t, cfg.Initializers, 1)
+
+	newState := func() *clio.State {
+		return &clio.State{Logger: discard.New(), RedactStore: gologgerredact.NewStore()}
+	}
+
+	require.NoError(t, cfg.Initializers[0](newState()))
+
+	// a second run starting before the first has finished must not silently replace the store, otherwise
+	// secrets added to the first store would no longer be redacted
+	require.PanicsWithValue(t, "replace existing redaction store (probably unintentional)", func() {
+		_ = cfg.Initializers[0](newState())
+	})
+}

internal/redact/redact.go

@@ -13,6 +13,14 @@ func Set(s redact.Store) {
 	store = s
 }
 
+// Reset clears the global redaction store. It must only be called once a command
+// run has fully finished (nothing left to Add or Apply), so that a later run can
+// Set its own store. Calling it while a run is in flight would let a second Set
+// split secrets across two stores and leak the ones in the dropped store.
+func Reset() {
+	store = nil
+}
+
 func Get() redact.Store {
 	return store
 }