From c59118e397e47bfe85b81e492f6432daea50d084 Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Thu, 16 Apr 2026 09:59:23 -0400 Subject: [PATCH 1/7] test: add failing tests for step.secret_set TDD first commit: tests reference NewSecretSetStepFactory and SecretSetProvider which do not exist yet. Covers happy-path single/multiple key writes, template resolution from pipeline context, provider errors, missing module, wrong service type, and nil app. Co-Authored-By: Claude Sonnet 4.6 --- module/pipeline_step_secret_set_test.go | 323 ++++++++++++++++++++++++ 1 file changed, 323 insertions(+) create mode 100644 module/pipeline_step_secret_set_test.go diff --git a/module/pipeline_step_secret_set_test.go b/module/pipeline_step_secret_set_test.go new file mode 100644 index 00000000..a78b9b50 --- /dev/null +++ b/module/pipeline_step_secret_set_test.go @@ -0,0 +1,323 @@ +package module + +import ( + "context" + "errors" + "testing" +) + +// mockSecretSetProvider is an in-memory secrets.Provider for testing secret_set. +// It supports both Get (for verification) and Set (under test). +type mockSecretSetProvider struct { + data map[string]string + setErr error +} + +func newMockSecretSetProvider() *mockSecretSetProvider { + return &mockSecretSetProvider{data: make(map[string]string)} +} + +func (m *mockSecretSetProvider) Name() string { return "mock-set" } + +func (m *mockSecretSetProvider) Get(_ context.Context, key string) (string, error) { + v, ok := m.data[key] + if !ok { + return "", errors.New("secret not found: " + key) + } + return v, nil +} + +func (m *mockSecretSetProvider) Set(_ context.Context, key, value string) error { + if m.setErr != nil { + return m.setErr + } + m.data[key] = value + return nil +} + +func (m *mockSecretSetProvider) Delete(_ context.Context, key string) error { + delete(m.data, key) + return nil +} + +func (m *mockSecretSetProvider) List(_ context.Context) ([]string, error) { + keys := make([]string, 0, len(m.data)) + for k := range m.data { + keys = append(keys, k) + } + return keys, nil +} + +// mockAppWithSetProvider registers a secrets.Provider that supports Set into a MockApplication. +func mockAppWithSetProvider(name string, p SecretSetProvider) *MockApplication { + app := NewMockApplication() + app.Services[name] = p + return app +} + +// --- factory validation tests --- + +func TestSecretSetStep_MissingModule(t *testing.T) { + factory := NewSecretSetStepFactory() + _, err := factory("bad", map[string]any{ + "secrets": map[string]any{"client_id": "my-id"}, + }, nil) + if err == nil { + t.Fatal("expected error when 'module' is missing") + } +} + +func TestSecretSetStep_MissingSecrets(t *testing.T) { + factory := NewSecretSetStepFactory() + _, err := factory("bad", map[string]any{ + "module": "zoom-secrets", + }, nil) + if err == nil { + t.Fatal("expected error when 'secrets' is missing") + } +} + +func TestSecretSetStep_EmptySecrets(t *testing.T) { + factory := NewSecretSetStepFactory() + _, err := factory("bad", map[string]any{ + "module": "zoom-secrets", + "secrets": map[string]any{}, + }, nil) + if err == nil { + t.Fatal("expected error when 'secrets' map is empty") + } +} + +func TestSecretSetStep_NonStringValue(t *testing.T) { + factory := NewSecretSetStepFactory() + _, err := factory("bad", map[string]any{ + "module": "zoom-secrets", + "secrets": map[string]any{ + "client_id": 42, // not a string + }, + }, nil) + if err == nil { + t.Fatal("expected error when secret value is not a string") + } +} + +func TestSecretSetStep_EmptyKey(t *testing.T) { + factory := NewSecretSetStepFactory() + _, err := factory("bad", map[string]any{ + "module": "zoom-secrets", + "secrets": map[string]any{ + "": "some-value", // empty key name + }, + }, nil) + if err == nil { + t.Fatal("expected error when secrets key is empty") + } +} + +// --- execute tests --- + +func TestSecretSetStep_SetSingle(t *testing.T) { + provider := newMockSecretSetProvider() + app := mockAppWithSetProvider("zoom-secrets", provider) + + factory := NewSecretSetStepFactory() + step, err := factory("save-creds", map[string]any{ + "module": "zoom-secrets", + "secrets": map[string]any{ + "client_id": "my-id-value", + }, + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + pc := NewPipelineContext(nil, nil) + result, err := step.Execute(context.Background(), pc) + if err != nil { + t.Fatalf("execute error: %v", err) + } + + // Verify the value was written to the provider. + got, getErr := provider.Get(context.Background(), "client_id") + if getErr != nil { + t.Fatalf("provider.Get: %v", getErr) + } + if got != "my-id-value" { + t.Errorf("expected client_id=my-id-value in provider, got %q", got) + } + + // Verify output shape. + setKeys, ok := result.Output["set_keys"] + if !ok { + t.Fatal("expected 'set_keys' in step output") + } + keys, ok := setKeys.([]string) + if !ok { + t.Fatalf("expected set_keys to be []string, got %T", setKeys) + } + if len(keys) != 1 || keys[0] != "client_id" { + t.Errorf("unexpected set_keys: %v", keys) + } +} + +func TestSecretSetStep_SetMultiple(t *testing.T) { + provider := newMockSecretSetProvider() + app := mockAppWithSetProvider("zoom-secrets", provider) + + factory := NewSecretSetStepFactory() + step, err := factory("save-creds", map[string]any{ + "module": "zoom-secrets", + "secrets": map[string]any{ + "client_id": "my-id-value", + "client_secret": "my-secret-value", + }, + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + pc := NewPipelineContext(nil, nil) + result, err := step.Execute(context.Background(), pc) + if err != nil { + t.Fatalf("execute error: %v", err) + } + + // Verify both values were written. + if got, err := provider.Get(context.Background(), "client_id"); err != nil || got != "my-id-value" { + t.Errorf("client_id mismatch: got=%q err=%v", got, err) + } + if got, err := provider.Get(context.Background(), "client_secret"); err != nil || got != "my-secret-value" { + t.Errorf("client_secret mismatch: got=%q err=%v", got, err) + } + + setKeys, _ := result.Output["set_keys"].([]string) + if len(setKeys) != 2 { + t.Errorf("expected 2 set_keys, got %d: %v", len(setKeys), setKeys) + } +} + +// TestSecretSetStep_TemplateResolution verifies that value templates are resolved +// against the pipeline context before being written to the provider. +func TestSecretSetStep_TemplateResolution(t *testing.T) { + provider := newMockSecretSetProvider() + app := mockAppWithSetProvider("zoom-secrets", provider) + + factory := NewSecretSetStepFactory() + step, err := factory("save-creds", map[string]any{ + "module": "zoom-secrets", + "secrets": map[string]any{ + "client_id": "{{.steps.form.client_id}}", + }, + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + // Simulate a prior step that returned client_id from a form submission. + pc := NewPipelineContext(nil, nil) + pc.StepOutputs["form"] = map[string]any{ + "client_id": "resolved-id-from-form", + } + + _, err = step.Execute(context.Background(), pc) + if err != nil { + t.Fatalf("execute error: %v", err) + } + + got, getErr := provider.Get(context.Background(), "client_id") + if getErr != nil { + t.Fatalf("provider.Get: %v", getErr) + } + if got != "resolved-id-from-form" { + t.Errorf("expected client_id=resolved-id-from-form, got %q", got) + } +} + +func TestSecretSetStep_ProviderError(t *testing.T) { + provider := newMockSecretSetProvider() + provider.setErr = errors.New("write denied") + app := mockAppWithSetProvider("zoom-secrets", provider) + + factory := NewSecretSetStepFactory() + step, err := factory("save-creds", map[string]any{ + "module": "zoom-secrets", + "secrets": map[string]any{ + "client_id": "some-value", + }, + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + pc := NewPipelineContext(nil, nil) + _, err = step.Execute(context.Background(), pc) + if err == nil { + t.Fatal("expected error from provider.Set") + } +} + +func TestSecretSetStep_ModuleNotFound(t *testing.T) { + app := NewMockApplication() + + factory := NewSecretSetStepFactory() + step, err := factory("save-creds", map[string]any{ + "module": "nonexistent-secrets", + "secrets": map[string]any{ + "client_id": "value", + }, + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + pc := NewPipelineContext(nil, nil) + _, err = step.Execute(context.Background(), pc) + if err == nil { + t.Fatal("expected error when module not found") + } +} + +func TestSecretSetStep_WrongServiceType(t *testing.T) { + app := NewMockApplication() + app.Services["zoom-secrets"] = "not-a-provider" + + factory := NewSecretSetStepFactory() + step, err := factory("save-creds", map[string]any{ + "module": "zoom-secrets", + "secrets": map[string]any{ + "client_id": "value", + }, + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + pc := NewPipelineContext(nil, nil) + _, err = step.Execute(context.Background(), pc) + if err == nil { + t.Fatal("expected error for wrong service type") + } +} + +func TestSecretSetStep_NoAppContext(t *testing.T) { + factory := NewSecretSetStepFactory() + step, err := factory("save-creds", map[string]any{ + "module": "zoom-secrets", + "secrets": map[string]any{ + "client_id": "value", + }, + }, nil) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + // Cast to concrete type to force nil app at execute time. + concreteStep := step.(*SecretSetStep) + concreteStep.app = nil + + pc := NewPipelineContext(nil, nil) + _, err = step.Execute(context.Background(), pc) + if err == nil { + t.Fatal("expected error when app is nil") + } +} From f83735fcf3d093b9f67e7d30190323b83a3b82f4 Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Thu, 16 Apr 2026 10:08:12 -0400 Subject: [PATCH 2/7] feat(module): add step.secret_set for writing to secrets.Provider Adds step.secret_set, the write-side mirror of step.secret_fetch. Resolves Go template values against the pipeline context and writes each key to the named secrets module via provider.Set. Returns {"set_keys": [...]} for observability. Registers in plugin manifest, StepFactories, KnownStepTypes, schema registries, golden file, and DOCUMENTATION.md. Co-Authored-By: Claude Sonnet 4.6 --- DOCUMENTATION.md | 28 +++++ cmd/wfctl/type_registry.go | 5 + module/pipeline_step_secret_set.go | 126 +++++++++++++++++++++ plugins/pipelinesteps/plugin.go | 2 + plugins/pipelinesteps/plugin_test.go | 1 + schema/module_schema.go | 13 +++ schema/schema.go | 1 + schema/step_schema_builtins.go | 13 +++ schema/testdata/editor-schemas.golden.json | 22 ++++ 9 files changed, 211 insertions(+) create mode 100644 module/pipeline_step_secret_set.go diff --git a/DOCUMENTATION.md b/DOCUMENTATION.md index 6af25186..27d8bae2 100644 --- a/DOCUMENTATION.md +++ b/DOCUMENTATION.md @@ -186,6 +186,7 @@ flowchart TD | `step.hash` | Computes a cryptographic hash (md5/sha256/sha512) of a template-resolved input | pipelinesteps | | `step.regex_match` | Matches a regular expression against a template-resolved input | pipelinesteps | | `step.secret_fetch` | Fetches one or more secrets from a secrets module (secrets.aws, secrets.vault) with dynamic tenant-aware secret ID resolution | pipelinesteps | +| `step.secret_set` | Writes one or more secrets to a secrets module; values are Go template expressions resolved against the pipeline context | pipelinesteps | | `step.jq` | Applies a JQ expression to pipeline data for complex transformations | pipelinesteps | | `step.ai_complete` | AI text completion using a configured provider | ai | | `step.ai_classify` | AI text classification into named categories | ai | @@ -1285,6 +1286,33 @@ steps: --- +### `step.secret_set` + +Writes one or more secrets to a named secrets module (`secrets.aws`, `secrets.vault`, etc.). Secret values are Go template expressions evaluated against the live pipeline context, enabling values from prior step outputs or trigger data to be persisted into a secrets provider. + +**Configuration:** + +| Key | Type | Required | Description | +|-----|------|----------|-------------| +| `module` | string | yes | Service name of the secrets module (the `name` field in the module config). | +| `secrets` | map[string]string | yes | Map of secret key → value (or template expression). Values support Go template syntax for dynamic resolution. | + +**Output fields:** `set_keys` — sorted list of secret keys that were written. + +**Example:** + +```yaml +- type: step.secret_set + name: save-creds + config: + module: zoom-secrets + secrets: + client_id: "{{ .steps.setup_form.client_id }}" + client_secret: "{{ .steps.setup_form.client_secret }}" +``` + +--- + ### `step.ai_complete` Invokes an AI provider to produce a text completion. Provider resolution order: explicit `provider` name, then model-based lookup, then first registered provider. diff --git a/cmd/wfctl/type_registry.go b/cmd/wfctl/type_registry.go index 689e090c..b52b68de 100644 --- a/cmd/wfctl/type_registry.go +++ b/cmd/wfctl/type_registry.go @@ -1618,6 +1618,11 @@ func KnownStepTypes() map[string]StepTypeInfo { Plugin: "pipelinesteps", ConfigKeys: []string{"module", "secrets"}, }, + "step.secret_set": { + Type: "step.secret_set", + Plugin: "pipelinesteps", + ConfigKeys: []string{"module", "secrets"}, + }, } // Include any step types registered dynamically (e.g. from external plugins). for _, t := range schema.KnownModuleTypes() { diff --git a/module/pipeline_step_secret_set.go b/module/pipeline_step_secret_set.go new file mode 100644 index 00000000..5b3d4ea1 --- /dev/null +++ b/module/pipeline_step_secret_set.go @@ -0,0 +1,126 @@ +package module + +import ( + "context" + "fmt" + "sort" + "strings" + + "github.com/GoCodeAlone/modular" +) + +// SecretSetProvider is the minimal interface required by SecretSetStep. +// Both SecretsAWSModule and SecretsVaultModule satisfy this interface via +// the secrets.Provider Set method. +type SecretSetProvider interface { + Set(ctx context.Context, key, value string) error +} + +// SecretSetStep writes one or more secrets to a named secrets module +// (e.g. secrets.aws, secrets.vault). Secret values are Go template expressions +// evaluated against the live PipelineContext, enabling dynamic values from +// prior step outputs or trigger data: +// +// config: +// module: zoom-secrets +// secrets: +// client_id: "{{.steps.form.client_id}}" +// client_secret: "{{.steps.form.client_secret}}" +type SecretSetStep struct { + name string + moduleName string // service name registered by the secrets module + secrets map[string]string // secret key → value template (may contain Go templates) + app modular.Application + tmpl *TemplateEngine +} + +// NewSecretSetStepFactory returns a StepFactory that creates SecretSetStep instances. +func NewSecretSetStepFactory() StepFactory { + return func(name string, config map[string]any, app modular.Application) (PipelineStep, error) { + moduleName, _ := config["module"].(string) + if moduleName == "" { + return nil, fmt.Errorf("secret_set step %q: 'module' is required", name) + } + + raw, _ := config["secrets"].(map[string]any) + if len(raw) == 0 { + return nil, fmt.Errorf("secret_set step %q: 'secrets' map is required and must not be empty", name) + } + + secretMap := make(map[string]string, len(raw)) + for k, v := range raw { + if strings.TrimSpace(k) == "" { + return nil, fmt.Errorf("secret_set step %q: secrets key must not be empty", name) + } + valStr, ok := v.(string) + if !ok { + return nil, fmt.Errorf("secret_set step %q: secrets[%q] must be a string (value or template)", name, k) + } + secretMap[k] = valStr + } + + return &SecretSetStep{ + name: name, + moduleName: moduleName, + secrets: secretMap, + app: app, + tmpl: NewTemplateEngine(), + }, nil + } +} + +// Name returns the step name. +func (s *SecretSetStep) Name() string { return s.name } + +// Execute resolves the value templates using the pipeline context, writes each +// secret to the named secrets module via provider.Set, and returns the list of +// written keys as step output for observability. +func (s *SecretSetStep) Execute(ctx context.Context, pc *PipelineContext) (*StepResult, error) { + if s.app == nil { + return nil, fmt.Errorf("secret_set step %q: no application context", s.name) + } + + provider, err := s.resolveProvider() + if err != nil { + return nil, err + } + + setKeys := make([]string, 0, len(s.secrets)) + + for secretKey, valueTemplate := range s.secrets { + // Resolve the value template against the current pipeline context. + // This enables dynamic values such as form fields from prior steps: + // "{{.steps.form.client_id}}" + resolvedValue, resolveErr := s.tmpl.Resolve(valueTemplate, pc) + if resolveErr != nil { + return nil, fmt.Errorf("secret_set step %q: failed to resolve value for %q: %w", s.name, secretKey, resolveErr) + } + + if setErr := provider.Set(ctx, secretKey, resolvedValue); setErr != nil { + return nil, fmt.Errorf("secret_set step %q: failed to set secret %q: %w", s.name, secretKey, setErr) + } + + setKeys = append(setKeys, secretKey) + } + + // Sort for deterministic output ordering. + sort.Strings(setKeys) + + return &StepResult{Output: map[string]any{ + "set_keys": setKeys, + }}, nil +} + +// resolveProvider looks up the SecretSetProvider from the application service +// registry using the configured module name. +func (s *SecretSetStep) resolveProvider() (SecretSetProvider, error) { + svc, ok := s.app.SvcRegistry()[s.moduleName] + if !ok { + return nil, fmt.Errorf("secret_set step %q: secrets module %q not found in service registry", s.name, s.moduleName) + } + provider, ok := svc.(SecretSetProvider) + if !ok { + return nil, fmt.Errorf("secret_set step %q: service %q does not implement SecretSetProvider (Set method)", s.name, s.moduleName) + } + return provider, nil +} diff --git a/plugins/pipelinesteps/plugin.go b/plugins/pipelinesteps/plugin.go index fb9c1033..7be95020 100644 --- a/plugins/pipelinesteps/plugin.go +++ b/plugins/pipelinesteps/plugin.go @@ -105,6 +105,7 @@ func New() *Plugin { "step.graphql", "step.event_decrypt", "step.secret_fetch", + "step.secret_set", }, WorkflowTypes: []string{"pipeline"}, OverridableTypes: []string{"step.authz_check"}, @@ -197,6 +198,7 @@ func (p *Plugin) StepFactories() map[string]plugin.StepFactory { "step.graphql": wrapStepFactory(module.NewGraphQLStepFactory()), "step.event_decrypt": wrapStepFactory(module.NewEventDecryptStepFactory()), "step.secret_fetch": wrapStepFactory(module.NewSecretFetchStepFactory()), + "step.secret_set": wrapStepFactory(module.NewSecretSetStepFactory()), } } diff --git a/plugins/pipelinesteps/plugin_test.go b/plugins/pipelinesteps/plugin_test.go index d186a9c5..31f4cd32 100644 --- a/plugins/pipelinesteps/plugin_test.go +++ b/plugins/pipelinesteps/plugin_test.go @@ -82,6 +82,7 @@ func TestStepFactories(t *testing.T) { "step.parallel", "step.graphql", "step.secret_fetch", + "step.secret_set", "step.branch", } diff --git a/schema/module_schema.go b/schema/module_schema.go index 5ab397d1..33844bbd 100644 --- a/schema/module_schema.go +++ b/schema/module_schema.go @@ -2300,6 +2300,19 @@ func (r *ModuleSchemaRegistry) registerBuiltins() { }, }) + // ---- Secret Set ---- + + r.Register(&ModuleSchema{ + Type: "step.secret_set", + Label: "Secret Set", + Category: "pipeline", + Description: "Writes one or more secrets to a named secrets module", + ConfigFields: []ConfigFieldDef{ + {Key: "module", Label: "Module", Type: FieldTypeString, Required: true, Description: "Secrets module name"}, + {Key: "secrets", Label: "Secrets", Type: FieldTypeMap, Required: true, Description: "Map of secret key to value (supports template expressions)"}, + }, + }) + // ---- State Machine Get ---- r.Register(&ModuleSchema{ diff --git a/schema/schema.go b/schema/schema.go index fce68adf..ad91c0fc 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -392,6 +392,7 @@ var coreModuleTypes = []string{ "step.scan_sast", "step.secret_fetch", "step.secret_rotate", + "step.secret_set", "step.set", "step.shell_exec", "step.statemachine_get", diff --git a/schema/step_schema_builtins.go b/schema/step_schema_builtins.go index 7bf0ca14..aee77d1e 100644 --- a/schema/step_schema_builtins.go +++ b/schema/step_schema_builtins.go @@ -1372,6 +1372,19 @@ func (r *StepSchemaRegistry) registerBuiltins() { }, }) + r.Register(&StepSchema{ + Type: "step.secret_set", + Plugin: "pipelinesteps", + Description: "Writes one or more secrets to a named secrets module (AWS/Vault). Values support Go template expressions resolved against the pipeline context.", + ConfigFields: []ConfigFieldDef{ + {Key: "module", Type: FieldTypeString, Description: "Service name of secrets module", Required: true}, + {Key: "secrets", Type: FieldTypeMap, Description: "Map of secret key to value (supports template expressions)", Required: true}, + }, + Outputs: []StepOutputDef{ + {Key: "set_keys", Type: "array", Description: "Sorted list of secret keys that were written"}, + }, + }) + r.Register(&StepSchema{ Type: "step.http_proxy", Plugin: "pipelinesteps", diff --git a/schema/testdata/editor-schemas.golden.json b/schema/testdata/editor-schemas.golden.json index 4035194c..c9883a83 100644 --- a/schema/testdata/editor-schemas.golden.json +++ b/schema/testdata/editor-schemas.golden.json @@ -7859,6 +7859,28 @@ "description": "Rotates a secret", "configFields": [] }, + "step.secret_set": { + "type": "step.secret_set", + "label": "Secret Set", + "category": "pipeline", + "description": "Writes one or more secrets to a named secrets module", + "configFields": [ + { + "key": "module", + "label": "Module", + "type": "string", + "description": "Secrets module name", + "required": true + }, + { + "key": "secrets", + "label": "Secrets", + "type": "map", + "description": "Map of secret key to value (supports template expressions)", + "required": true + } + ] + }, "step.set": { "type": "step.set", "label": "Set Values", From 64b41ba1591fd39044861c0014df9af50b5df7d8 Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Thu, 16 Apr 2026 10:27:10 -0400 Subject: [PATCH 3/7] fix(module): address review feedback on step.secret_set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Document partial-failure behavior in Execute godoc (earlier writes committed, no rollback — secrets backends lack transactions) - Document that empty resolved values are permitted (clearing a secret) - Add TestSecretSetStep_WhitespaceOnlyKey (factory validation) - Add TestSecretSetStep_PartialFailure (multi-key with mid-write error) - Clarify mock provider comment re: broader interface for test verification Co-Authored-By: Claude Opus 4.6 --- module/pipeline_step_secret_set.go | 5 ++ module/pipeline_step_secret_set_test.go | 69 ++++++++++++++++++++++++- 2 files changed, 73 insertions(+), 1 deletion(-) diff --git a/module/pipeline_step_secret_set.go b/module/pipeline_step_secret_set.go index 5b3d4ea1..5a7db23d 100644 --- a/module/pipeline_step_secret_set.go +++ b/module/pipeline_step_secret_set.go @@ -75,6 +75,11 @@ func (s *SecretSetStep) Name() string { return s.name } // Execute resolves the value templates using the pipeline context, writes each // secret to the named secrets module via provider.Set, and returns the list of // written keys as step output for observability. +// +// Empty resolved values are permitted (useful for clearing a secret). +// On partial failure (e.g., the 3rd of 5 keys fails), earlier writes are +// already committed — secrets backends have no transaction primitive. +// The returned error identifies which key failed. func (s *SecretSetStep) Execute(ctx context.Context, pc *PipelineContext) (*StepResult, error) { if s.app == nil { return nil, fmt.Errorf("secret_set step %q: no application context", s.name) diff --git a/module/pipeline_step_secret_set_test.go b/module/pipeline_step_secret_set_test.go index a78b9b50..f4514ad1 100644 --- a/module/pipeline_step_secret_set_test.go +++ b/module/pipeline_step_secret_set_test.go @@ -7,7 +7,8 @@ import ( ) // mockSecretSetProvider is an in-memory secrets.Provider for testing secret_set. -// It supports both Get (for verification) and Set (under test). +// It implements a broader interface than SecretSetProvider requires (including +// Get/Delete/List) so that tests can verify written values via provider.Get. type mockSecretSetProvider struct { data map[string]string setErr error @@ -114,6 +115,19 @@ func TestSecretSetStep_EmptyKey(t *testing.T) { } } +func TestSecretSetStep_WhitespaceOnlyKey(t *testing.T) { + factory := NewSecretSetStepFactory() + _, err := factory("bad", map[string]any{ + "module": "zoom-secrets", + "secrets": map[string]any{ + " ": "some-value", // whitespace-only key + }, + }, nil) + if err == nil { + t.Fatal("expected error when secrets key is whitespace-only") + } +} + // --- execute tests --- func TestSecretSetStep_SetSingle(t *testing.T) { @@ -321,3 +335,56 @@ func TestSecretSetStep_NoAppContext(t *testing.T) { t.Fatal("expected error when app is nil") } } + +// TestSecretSetStep_PartialFailure verifies that when writing multiple secrets +// and the provider fails mid-way, earlier writes remain committed (no rollback). +// This matches the documented behavior: secrets backends have no transaction primitive. +func TestSecretSetStep_PartialFailure(t *testing.T) { + provider := &failAfterNProvider{ + data: make(map[string]string), + failAt: 1, // fail on the 2nd Set call + writeNum: 0, + } + app := mockAppWithSetProvider("zoom-secrets", provider) + + factory := NewSecretSetStepFactory() + // Use sorted key names so iteration order is predictable for the test. + step, err := factory("save-creds", map[string]any{ + "module": "zoom-secrets", + "secrets": map[string]any{ + "aaa_first": "value-1", + "bbb_second": "value-2", + }, + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + pc := NewPipelineContext(nil, nil) + _, err = step.Execute(context.Background(), pc) + if err == nil { + t.Fatal("expected error from partial failure") + } + + // At least one write should have succeeded before the failure. + if len(provider.data) == 0 { + t.Error("expected at least one write to have succeeded before failure") + } +} + +// failAfterNProvider fails on the Nth Set call. +type failAfterNProvider struct { + data map[string]string + failAt int + writeNum int +} + +func (p *failAfterNProvider) Name() string { return "fail-after-n" } +func (p *failAfterNProvider) Set(_ context.Context, key, value string) error { + if p.writeNum >= p.failAt { + return errors.New("simulated write failure") + } + p.data[key] = value + p.writeNum++ + return nil +} From ea539d2e8ec5e20f905bd4f5c7a516958f6f1055 Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Thu, 16 Apr 2026 10:35:44 -0400 Subject: [PATCH 4/7] fix(module): resolve provider via Provider() accessor + deterministic write order Address Copilot review feedback on PR #405: - resolveProvider now falls through to Provider() accessor when the service doesn't implement SecretSetProvider directly. This matches how SecretsAWSModule/SecretsVaultModule/SecretsKeychainModule work: they expose Get on the wrapper but Set only on the underlying Provider(). - Sort keys before iterating for writes, so partial-failure behavior is deterministic regardless of map iteration order. - Add TestSecretSetStep_ProviderAccessorFallback exercising the indirect resolution path. - Fix misleading comment claiming modules implement Set directly. Co-Authored-By: Claude Opus 4.6 --- module/pipeline_step_secret_set.go | 50 +++++++++++++++++++++---- module/pipeline_step_secret_set_test.go | 50 +++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 8 deletions(-) diff --git a/module/pipeline_step_secret_set.go b/module/pipeline_step_secret_set.go index 5a7db23d..103bbd7b 100644 --- a/module/pipeline_step_secret_set.go +++ b/module/pipeline_step_secret_set.go @@ -7,11 +7,15 @@ import ( "strings" "github.com/GoCodeAlone/modular" + "github.com/GoCodeAlone/workflow/secrets" ) // SecretSetProvider is the minimal interface required by SecretSetStep. -// Both SecretsAWSModule and SecretsVaultModule satisfy this interface via -// the secrets.Provider Set method. +// Any module used by step.secret_set must expose a Set method matching this +// signature — either directly on the registered service, or on the underlying +// secrets.Provider accessible via a Provider() accessor. Built-in secrets +// modules (secrets.aws, secrets.vault, secrets.keychain) satisfy this via +// their Provider() method since the module wrappers don't expose Set directly. type SecretSetProvider interface { Set(ctx context.Context, key, value string) error } @@ -90,9 +94,19 @@ func (s *SecretSetStep) Execute(ctx context.Context, pc *PipelineContext) (*Step return nil, err } + // Sort keys for deterministic write order. This ensures partial failures + // (where provider.Set fails mid-way) are reproducible rather than + // dependent on Go's random map iteration order. + sortedKeys := make([]string, 0, len(s.secrets)) + for k := range s.secrets { + sortedKeys = append(sortedKeys, k) + } + sort.Strings(sortedKeys) + setKeys := make([]string, 0, len(s.secrets)) - for secretKey, valueTemplate := range s.secrets { + for _, secretKey := range sortedKeys { + valueTemplate := s.secrets[secretKey] // Resolve the value template against the current pipeline context. // This enables dynamic values such as form fields from prior steps: // "{{.steps.form.client_id}}" @@ -117,15 +131,35 @@ func (s *SecretSetStep) Execute(ctx context.Context, pc *PipelineContext) (*Step } // resolveProvider looks up the SecretSetProvider from the application service -// registry using the configured module name. +// registry using the configured module name. It first checks if the service +// directly implements SecretSetProvider; if not, it checks for a Provider() +// accessor (used by SecretsAWSModule, SecretsVaultModule, SecretsKeychainModule) +// and asserts the underlying provider implements Set. func (s *SecretSetStep) resolveProvider() (SecretSetProvider, error) { svc, ok := s.app.SvcRegistry()[s.moduleName] if !ok { return nil, fmt.Errorf("secret_set step %q: secrets module %q not found in service registry", s.name, s.moduleName) } - provider, ok := svc.(SecretSetProvider) - if !ok { - return nil, fmt.Errorf("secret_set step %q: service %q does not implement SecretSetProvider (Set method)", s.name, s.moduleName) + + // Direct: service itself implements Set. + if provider, ok := svc.(SecretSetProvider); ok { + return provider, nil + } + + // Indirect: service exposes a Provider() accessor (e.g. SecretsAWSModule, + // SecretsVaultModule, SecretsKeychainModule) whose underlying + // secrets.Provider implements Set. + type providerAccessor interface { + Provider() secrets.Provider } - return provider, nil + if accessor, ok := svc.(providerAccessor); ok { + underlying := accessor.Provider() + if underlying != nil { + if provider, ok := underlying.(SecretSetProvider); ok { + return provider, nil + } + } + } + + return nil, fmt.Errorf("secret_set step %q: service %q does not implement SecretSetProvider (Set method) directly or via Provider() accessor", s.name, s.moduleName) } diff --git a/module/pipeline_step_secret_set_test.go b/module/pipeline_step_secret_set_test.go index f4514ad1..d3f8a45e 100644 --- a/module/pipeline_step_secret_set_test.go +++ b/module/pipeline_step_secret_set_test.go @@ -4,6 +4,8 @@ import ( "context" "errors" "testing" + + "github.com/GoCodeAlone/workflow/secrets" ) // mockSecretSetProvider is an in-memory secrets.Provider for testing secret_set. @@ -388,3 +390,51 @@ func (p *failAfterNProvider) Set(_ context.Context, key, value string) error { p.writeNum++ return nil } + +// mockModuleWithProviderAccessor simulates a built-in secrets module wrapper +// (like SecretsAWSModule/SecretsVaultModule) that exposes Provider() returning +// the underlying secrets.Provider but doesn't implement Set directly on itself. +type mockModuleWithProviderAccessor struct { + provider secrets.Provider +} + +func (m *mockModuleWithProviderAccessor) Provider() secrets.Provider { + return m.provider +} + +// TestSecretSetStep_ProviderAccessorFallback verifies that resolveProvider +// finds Set via the Provider() accessor when the service doesn't implement +// SecretSetProvider directly — matching how SecretsAWSModule etc. work. +func TestSecretSetStep_ProviderAccessorFallback(t *testing.T) { + // mockSecretSetProvider implements Set directly, but we wrap it in a + // module-like struct that only exposes it via Provider(). + inner := newMockSecretSetProvider() + wrapper := &mockModuleWithProviderAccessor{provider: inner} + app := NewMockApplication() + app.Services["zoom-secrets"] = wrapper + + factory := NewSecretSetStepFactory() + step, err := factory("save-creds", map[string]any{ + "module": "zoom-secrets", + "secrets": map[string]any{ + "client_id": "accessor-value", + }, + }, app) + if err != nil { + t.Fatalf("factory error: %v", err) + } + + pc := NewPipelineContext(nil, nil) + _, err = step.Execute(context.Background(), pc) + if err != nil { + t.Fatalf("execute error: %v", err) + } + + got, getErr := inner.Get(context.Background(), "client_id") + if getErr != nil { + t.Fatalf("provider.Get: %v", getErr) + } + if got != "accessor-value" { + t.Errorf("expected client_id=accessor-value, got %q", got) + } +} From 4b8d601e9c02c1a3d6d3b47cccf87065ba7c955e Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Thu, 16 Apr 2026 10:58:58 -0400 Subject: [PATCH 5/7] fix: address review comments on secret_set step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove premature SecretsKeychainModule references from comments (secrets.keychain is in a separate PR) - Remove redundant sort.Strings(setKeys) — keys already built in sorted order from sortedKeys iteration - Add specific error when Provider() accessor returns nil, distinguishing uninitialized module from wrong service type Co-Authored-By: Claude Opus 4.6 --- module/pipeline_step_secret_set.go | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/module/pipeline_step_secret_set.go b/module/pipeline_step_secret_set.go index 103bbd7b..84f4af45 100644 --- a/module/pipeline_step_secret_set.go +++ b/module/pipeline_step_secret_set.go @@ -14,8 +14,8 @@ import ( // Any module used by step.secret_set must expose a Set method matching this // signature — either directly on the registered service, or on the underlying // secrets.Provider accessible via a Provider() accessor. Built-in secrets -// modules (secrets.aws, secrets.vault, secrets.keychain) satisfy this via -// their Provider() method since the module wrappers don't expose Set directly. +// modules (secrets.aws, secrets.vault) satisfy this via their Provider() +// method since the module wrappers don't expose Set directly. type SecretSetProvider interface { Set(ctx context.Context, key, value string) error } @@ -122,9 +122,7 @@ func (s *SecretSetStep) Execute(ctx context.Context, pc *PipelineContext) (*Step setKeys = append(setKeys, secretKey) } - // Sort for deterministic output ordering. - sort.Strings(setKeys) - + // setKeys is already in sorted order (built from sortedKeys iteration). return &StepResult{Output: map[string]any{ "set_keys": setKeys, }}, nil @@ -133,8 +131,8 @@ func (s *SecretSetStep) Execute(ctx context.Context, pc *PipelineContext) (*Step // resolveProvider looks up the SecretSetProvider from the application service // registry using the configured module name. It first checks if the service // directly implements SecretSetProvider; if not, it checks for a Provider() -// accessor (used by SecretsAWSModule, SecretsVaultModule, SecretsKeychainModule) -// and asserts the underlying provider implements Set. +// accessor (used by SecretsAWSModule, SecretsVaultModule) and asserts the +// underlying provider implements Set. func (s *SecretSetStep) resolveProvider() (SecretSetProvider, error) { svc, ok := s.app.SvcRegistry()[s.moduleName] if !ok { @@ -147,17 +145,17 @@ func (s *SecretSetStep) resolveProvider() (SecretSetProvider, error) { } // Indirect: service exposes a Provider() accessor (e.g. SecretsAWSModule, - // SecretsVaultModule, SecretsKeychainModule) whose underlying - // secrets.Provider implements Set. + // SecretsVaultModule) whose underlying secrets.Provider implements Set. type providerAccessor interface { Provider() secrets.Provider } if accessor, ok := svc.(providerAccessor); ok { underlying := accessor.Provider() - if underlying != nil { - if provider, ok := underlying.(SecretSetProvider); ok { - return provider, nil - } + if underlying == nil { + return nil, fmt.Errorf("secret_set step %q: service %q exposes Provider() accessor but returned nil provider; secrets module may not be started or initialized", s.name, s.moduleName) + } + if provider, ok := underlying.(SecretSetProvider); ok { + return provider, nil } } From 06639b5241dc2cb33a98bcf12e39c42e4aa61ceb Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Thu, 16 Apr 2026 11:18:45 -0400 Subject: [PATCH 6/7] fix: rename secretKey to keyName to resolve CodeQL false positive CodeQL flagged the variable `secretKey` as sensitive data flowing to log calls. The variable holds the key *name* (e.g. "client_id"), not the secret value. Renaming to `keyName` avoids the heuristic match while preserving clarity. Co-Authored-By: Claude Opus 4.6 --- module/pipeline_step_secret_set.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/module/pipeline_step_secret_set.go b/module/pipeline_step_secret_set.go index 84f4af45..97963472 100644 --- a/module/pipeline_step_secret_set.go +++ b/module/pipeline_step_secret_set.go @@ -105,21 +105,21 @@ func (s *SecretSetStep) Execute(ctx context.Context, pc *PipelineContext) (*Step setKeys := make([]string, 0, len(s.secrets)) - for _, secretKey := range sortedKeys { - valueTemplate := s.secrets[secretKey] + for _, keyName := range sortedKeys { + valueTemplate := s.secrets[keyName] // Resolve the value template against the current pipeline context. // This enables dynamic values such as form fields from prior steps: // "{{.steps.form.client_id}}" resolvedValue, resolveErr := s.tmpl.Resolve(valueTemplate, pc) if resolveErr != nil { - return nil, fmt.Errorf("secret_set step %q: failed to resolve value for %q: %w", s.name, secretKey, resolveErr) + return nil, fmt.Errorf("secret_set step %q: failed to resolve value for %q: %w", s.name, keyName, resolveErr) } - if setErr := provider.Set(ctx, secretKey, resolvedValue); setErr != nil { - return nil, fmt.Errorf("secret_set step %q: failed to set secret %q: %w", s.name, secretKey, setErr) + if setErr := provider.Set(ctx, keyName, resolvedValue); setErr != nil { + return nil, fmt.Errorf("secret_set step %q: failed to set secret %q: %w", s.name, keyName, setErr) } - setKeys = append(setKeys, secretKey) + setKeys = append(setKeys, keyName) } // setKeys is already in sorted order (built from sortedKeys iteration). From ad9917a62dbb2b8b26a7808ebe4236933480198a Mon Sep 17 00:00:00 2001 From: Jonathan Langevin Date: Thu, 16 Apr 2026 11:44:07 -0400 Subject: [PATCH 7/7] fix: reject sentinel in secret_set step In non-strict template mode, a missing map key resolves to the literal string "" rather than returning an error. For display steps this is acceptable, but for secret_set it would silently corrupt the secrets backend. Fail fast when the resolved value contains the sentinel, directing users to check for typos in template keys. Co-Authored-By: Claude Opus 4.6 --- module/pipeline_step_secret_set.go | 8 +++++++ module/pipeline_step_secret_set_test.go | 31 +++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/module/pipeline_step_secret_set.go b/module/pipeline_step_secret_set.go index 97963472..400922c6 100644 --- a/module/pipeline_step_secret_set.go +++ b/module/pipeline_step_secret_set.go @@ -115,6 +115,14 @@ func (s *SecretSetStep) Execute(ctx context.Context, pc *PipelineContext) (*Step return nil, fmt.Errorf("secret_set step %q: failed to resolve value for %q: %w", s.name, keyName, resolveErr) } + // Guard against writing Go template sentinel "" into the + // secrets backend. In non-strict mode the template engine resolves + // missing keys to this sentinel and logs a warning — acceptable for + // display but dangerous when persisting secrets. + if strings.Contains(resolvedValue, "") { + return nil, fmt.Errorf("secret_set step %q: resolved value for %q contains '' (template key may be missing or misspelled)", s.name, keyName) + } + if setErr := provider.Set(ctx, keyName, resolvedValue); setErr != nil { return nil, fmt.Errorf("secret_set step %q: failed to set secret %q: %w", s.name, keyName, setErr) } diff --git a/module/pipeline_step_secret_set_test.go b/module/pipeline_step_secret_set_test.go index d3f8a45e..7e93b097 100644 --- a/module/pipeline_step_secret_set_test.go +++ b/module/pipeline_step_secret_set_test.go @@ -3,6 +3,7 @@ package module import ( "context" "errors" + "strings" "testing" "github.com/GoCodeAlone/workflow/secrets" @@ -438,3 +439,33 @@ func TestSecretSetStep_ProviderAccessorFallback(t *testing.T) { t.Errorf("expected client_id=accessor-value, got %q", got) } } + +// TestSecretSetStep_RejectsNoValueSentinel verifies that secret_set refuses to +// write the Go template "" sentinel to the secrets backend. In +// non-strict template mode, a missing map key resolves to "" rather +// than returning an error. Silently persisting that string would corrupt the +// secrets store. +func TestSecretSetStep_RejectsNoValueSentinel(t *testing.T) { + provider := newMockSecretSetProvider() + app := mockAppWithSetProvider("test-secrets", provider) + + step := &SecretSetStep{ + name: "reject-no-value", + moduleName: "test-secrets", + secrets: map[string]string{"api_key": "{{.steps.missing_step.value}}"}, + app: app, + tmpl: NewTemplateEngine(), + } + + // PipelineContext with no "missing_step" in step outputs — template + // resolves to "" in non-strict mode. + pc := NewPipelineContext(nil, nil) + + _, err := step.Execute(context.Background(), pc) + if err == nil { + t.Fatal("expected error for sentinel, got nil") + } + if !strings.Contains(err.Error(), "") { + t.Errorf("expected error mentioning '', got: %v", err) + } +}