From da90ecd083280c33ae8acf497730a30f48acc2d0 Mon Sep 17 00:00:00 2001 From: Valentin Maerten Date: Mon, 29 Jun 2026 12:36:56 +0200 Subject: [PATCH] fix: prevent secret variable leaks in summary, verbose and key ordering - mask secret values in `task --summary` (commands and vars listing) - mask resolved value of dynamic (sh) secrets in verbose logs - use masked command for platform-skipped verbose log - allow `secret` key in any position in a var definition (not only first) - add `value` to the JSON schema var definition - skip masking pass when no secret is present and dedup mask helpers - document that the `secret` flag is not propagated to derived variables --- compiler.go | 7 ++- executor_test.go | 23 +++++++ internal/summary/summary.go | 12 +++- internal/templater/secrets.go | 51 +++++++-------- task.go | 2 +- taskfile/ast/var.go | 62 +++++++++++-------- testdata/secrets/Taskfile.yml | 18 ++++++ ...rs-dynamic_secret_masked_in_verbose.golden | 5 ++ ...etVars-secret_key_order_independent.golden | 2 + ...s-secret_vars_are_masked_in_summary.golden | 15 +++++ website/src/docs/guide.md | 35 +++++++++++ website/src/public/schema.json | 3 + 12 files changed, 175 insertions(+), 60 deletions(-) create mode 100644 testdata/secrets/testdata/TestSecretVars-dynamic_secret_masked_in_verbose.golden create mode 100644 testdata/secrets/testdata/TestSecretVars-secret_key_order_independent.golden create mode 100644 testdata/secrets/testdata/TestSecretVars-secret_vars_are_masked_in_summary.golden diff --git a/compiler.go b/compiler.go index f80e79af..9c4f8a07 100644 --- a/compiler.go +++ b/compiler.go @@ -184,7 +184,12 @@ func (c *Compiler) HandleDynamicVar(v ast.Var, dir string, e []string) (string, result = strings.TrimSuffix(result, "\n") c.dynamicCache[*v.Sh] = result - c.Logger.VerboseErrf(logger.Magenta, "task: dynamic variable: %q result: %q\n", *v.Sh, result) + // Never print the resolved value of a secret variable, even in verbose mode + logResult := result + if v.Secret { + logResult = "*****" + } + c.Logger.VerboseErrf(logger.Magenta, "task: dynamic variable: %q result: %q\n", *v.Sh, logResult) return result, nil } diff --git a/executor_test.go b/executor_test.go index 82999f0a..60172b87 100644 --- a/executor_test.go +++ b/executor_test.go @@ -320,6 +320,29 @@ func TestSecretVars(t *testing.T) { ), WithTask("test-env-secret-limitation"), ) + NewExecutorTest(t, + WithName("secret vars are masked in summary"), + WithExecutorOptions( + task.WithDir("testdata/secrets"), + task.WithSummary(true), + ), + WithTask("test-secret-masking"), + ) + NewExecutorTest(t, + WithName("dynamic secret masked in verbose"), + WithExecutorOptions( + task.WithDir("testdata/secrets"), + task.WithVerbose(true), + ), + WithTask("test-dynamic-secret-verbose"), + ) + NewExecutorTest(t, + WithName("secret key order independent"), + WithExecutorOptions( + task.WithDir("testdata/secrets"), + ), + WithTask("test-secret-key-order"), + ) } func TestRequires(t *testing.T) { diff --git a/internal/summary/summary.go b/internal/summary/summary.go index 9edd9511..573e874a 100644 --- a/internal/summary/summary.go +++ b/internal/summary/summary.go @@ -117,7 +117,12 @@ func printTaskCommands(l *logger.Logger, t *ast.Task) { isCommand := c.Cmd != "" l.Outf(logger.Default, " - ") if isCommand { - l.Outf(logger.Yellow, "%s\n", c.Cmd) + // Use the masked command so secret values are not leaked in summaries + logCmd := c.LogCmd + if logCmd == "" { + logCmd = c.Cmd + } + l.Outf(logger.Yellow, "%s\n", logCmd) } else { l.Outf(logger.Green, "Task: %s\n", c.Task) } @@ -196,6 +201,11 @@ func printTaskEnv(l *logger.Logger, t *ast.Task) { // formatVarValue formats a variable value based on its type. // Handles static values, shell commands (sh:), references (ref:), and maps. func formatVarValue(v ast.Var) string { + // Never expose secret variables in the summary, whatever their type + if v.Secret { + return "*****" + } + // Shell command - check this first before Value // because dynamic vars may have both Sh and an empty Value if v.Sh != nil { diff --git a/internal/templater/secrets.go b/internal/templater/secrets.go index 59e37fff..2338254c 100644 --- a/internal/templater/secrets.go +++ b/internal/templater/secrets.go @@ -8,39 +8,19 @@ import ( // This function uses the Go templater to resolve all variables ({{.VAR}}) while // masking secret ones as "*****". func MaskSecrets(cmdTemplate string, vars *ast.Vars) string { - if vars == nil || vars.Len() == 0 { - return cmdTemplate - } - - // Create a cache map with secrets masked - maskedVars := vars.DeepCopy() - for name, v := range maskedVars.All() { - if v.Secret { - // Replace secret value with mask - maskedVars.Set(name, ast.Var{ - Value: "*****", - Secret: true, - }) - } - } - - // Use the templater to resolve the template with masked secrets - cache := &Cache{Vars: maskedVars} - result := Replace(cmdTemplate, cache) - - // If there was an error, return the original template - if cache.Err() != nil { - return cmdTemplate - } - - return result + return MaskSecretsWithExtra(cmdTemplate, vars, nil) } // MaskSecretsWithExtra is like MaskSecrets but also resolves extra variables (e.g., loop vars). func MaskSecretsWithExtra(cmdTemplate string, vars *ast.Vars, extra map[string]any) string { - if vars == nil || vars.Len() == 0 { - // Still need to resolve extra vars even if no vars - cache := &Cache{Vars: ast.NewVars()} + if vars == nil { + vars = ast.NewVars() + } + + // Fast path: if there are no secrets to mask, resolve the template directly + // without the extra DeepCopy + masking pass. + if !hasSecrets(vars) { + cache := &Cache{Vars: vars} result := ReplaceWithExtra(cmdTemplate, cache, extra) if cache.Err() != nil { return cmdTemplate @@ -48,7 +28,7 @@ func MaskSecretsWithExtra(cmdTemplate string, vars *ast.Vars, extra map[string]a return result } - // Create a cache map with secrets masked + // Create a copy with secret values masked, leaving the originals untouched. maskedVars := vars.DeepCopy() for name, v := range maskedVars.All() { if v.Secret { @@ -62,9 +42,20 @@ func MaskSecretsWithExtra(cmdTemplate string, vars *ast.Vars, extra map[string]a cache := &Cache{Vars: maskedVars} result := ReplaceWithExtra(cmdTemplate, cache, extra) + // If there was an error, return the original template if cache.Err() != nil { return cmdTemplate } return result } + +// hasSecrets reports whether any variable is marked as secret. +func hasSecrets(vars *ast.Vars) bool { + for _, v := range vars.All() { + if v.Secret { + return true + } + } + return false +} diff --git a/task.go b/task.go index 47d3245b..98d340c9 100644 --- a/task.go +++ b/task.go @@ -390,7 +390,7 @@ func (e *Executor) runCommand(ctx context.Context, t *ast.Task, call *Call, i in return err case cmd.Cmd != "": if !shouldRunOnCurrentPlatform(cmd.Platforms) { - e.Logger.VerboseOutf(logger.Yellow, "task: [%s] %s not for current platform - ignored\n", t.Name(), cmd.Cmd) + e.Logger.VerboseOutf(logger.Yellow, "task: [%s] %s not for current platform - ignored\n", t.Name(), cmd.LogCmd) return nil } diff --git a/taskfile/ast/var.go b/taskfile/ast/var.go index aa1c4e89..f02bda64 100644 --- a/taskfile/ast/var.go +++ b/taskfile/ast/var.go @@ -19,35 +19,43 @@ type Var struct { func (v *Var) UnmarshalYAML(node *yaml.Node) error { switch node.Kind { case yaml.MappingNode: - key := "" - if len(node.Content) > 0 { - key = node.Content[0].Value + var m struct { + Sh *string + Ref string + Map any + Value any + Secret bool } - switch key { - case "sh", "ref", "map", "value": - var m struct { - Sh *string - Ref string - Map any - Value any - Secret bool - } - if err := node.Decode(&m); err != nil { - return errors.NewTaskfileDecodeError(err, node) - } - v.Sh = m.Sh - v.Ref = m.Ref - v.Secret = m.Secret - // Handle both "map" and "value" keys - if m.Map != nil { - v.Value = m.Map - } else if m.Value != nil { - v.Value = m.Value - } - return nil - default: - return errors.NewTaskfileDecodeError(nil, node).WithMessage(`%q is not a valid variable type. Try "sh", "ref", "map", "value" or using a scalar value`, key) + if err := node.Decode(&m); err != nil { + return errors.NewTaskfileDecodeError(err, node) } + // Validate the keys regardless of their order: every key must be known + // and at least one type-defining key must be present. "secret" is a + // modifier, not a type, so it can appear in any position. + hasType := false + for i := 0; i+1 < len(node.Content); i += 2 { + switch node.Content[i].Value { + case "sh", "ref", "map", "value": + hasType = true + case "secret": + // modifier, not a type + default: + return errors.NewTaskfileDecodeError(nil, node).WithMessage(`%q is not a valid variable type. Try "sh", "ref", "map", "value" or using a scalar value`, node.Content[i].Value) + } + } + if !hasType { + return errors.NewTaskfileDecodeError(nil, node).WithMessage(`a variable must define one of "sh", "ref", "map", "value" or be a scalar value`) + } + v.Sh = m.Sh + v.Ref = m.Ref + v.Secret = m.Secret + // Handle both "map" and "value" keys + if m.Map != nil { + v.Value = m.Map + } else if m.Value != nil { + v.Value = m.Value + } + return nil default: var value any if err := node.Decode(&value); err != nil { diff --git a/testdata/secrets/Taskfile.yml b/testdata/secrets/Taskfile.yml index 115ec4df..27abae21 100644 --- a/testdata/secrets/Taskfile.yml +++ b/testdata/secrets/Taskfile.yml @@ -51,11 +51,29 @@ tasks: - defer: echo "Cleanup with secret={{.DEFERRED_SECRET}} and app={{.APP_NAME}}" - echo "Main command executed" + test-dynamic-secret-verbose: + desc: Test that dynamic (sh) secrets are masked even in verbose logs + cmds: + - echo "Password is {{.PASSWORD}}" + + test-secret-key-order: + desc: Test that "secret" may be declared before the value/sh key + vars: + SECRET_FIRST: + secret: true + value: "order-independent-secret" + SH_SECRET_FIRST: + secret: true + sh: "echo 'sh-order-independent-secret'" + cmds: + - echo "Value={{.SECRET_FIRST}} Sh={{.SH_SECRET_FIRST}}" + test-env-secret-limitation: desc: Test showing that env vars with secret flag are NOT masked (limitation) env: SECRET_TOKEN: value: "env-secret-token-123" + secret: true PUBLIC_ENV: "public-value" cmds: # Templates {{.VAR}} don't work with env - they're empty diff --git a/testdata/secrets/testdata/TestSecretVars-dynamic_secret_masked_in_verbose.golden b/testdata/secrets/testdata/TestSecretVars-dynamic_secret_masked_in_verbose.golden new file mode 100644 index 00000000..c49253e7 --- /dev/null +++ b/testdata/secrets/testdata/TestSecretVars-dynamic_secret_masked_in_verbose.golden @@ -0,0 +1,5 @@ +task: dynamic variable: "echo 'my-super-secret-password'" result: "*****" +task: "test-dynamic-secret-verbose" started +task: [test-dynamic-secret-verbose] echo "Password is *****" +Password is my-super-secret-password +task: "test-dynamic-secret-verbose" finished diff --git a/testdata/secrets/testdata/TestSecretVars-secret_key_order_independent.golden b/testdata/secrets/testdata/TestSecretVars-secret_key_order_independent.golden new file mode 100644 index 00000000..86770630 --- /dev/null +++ b/testdata/secrets/testdata/TestSecretVars-secret_key_order_independent.golden @@ -0,0 +1,2 @@ +task: [test-secret-key-order] echo "Value=***** Sh=*****" +Value=order-independent-secret Sh=sh-order-independent-secret diff --git a/testdata/secrets/testdata/TestSecretVars-secret_vars_are_masked_in_summary.golden b/testdata/secrets/testdata/TestSecretVars-secret_vars_are_masked_in_summary.golden new file mode 100644 index 00000000..8ab12636 --- /dev/null +++ b/testdata/secrets/testdata/TestSecretVars-secret_vars_are_masked_in_summary.golden @@ -0,0 +1,15 @@ +task: test-secret-masking + +Test that secret variables are masked in logs + +vars: + APP_NAME: "myapp" + API_KEY: ***** + PASSWORD: ***** + PUBLIC_URL: "https://example.com" + +commands: + - echo "Deploying myapp to https://example.com" + - echo "Using API key *****" + - echo "Password is *****" + - echo "Public app name is myapp" diff --git a/website/src/docs/guide.md b/website/src/docs/guide.md index 617df35a..8a06d38a 100644 --- a/website/src/docs/guide.md +++ b/website/src/docs/guide.md @@ -1636,6 +1636,7 @@ in logs, but is **not a substitute** for proper secret management practices. - ❌ Secrets visible in process inspection (e.g., `ps aux`) - ❌ Secrets in shell history - ❌ Secrets in command output (stdout/stderr) +- ❌ Secret values copied into derived (non-secret) variables Always use proper secret management tools (HashiCorp Vault, AWS Secrets Manager, etc.) for production environments. @@ -1771,6 +1772,40 @@ tasks: ::: +::: warning + +**Secrets are not propagated to derived variables.** The `secret` flag only +masks the variable it is set on. A non-secret variable that references a secret +will expose the resolved value in logs: + +```yaml +version: '3' + +vars: + API_KEY: + value: 'secret-api-key-123' + secret: true + HEADER: + value: 'Bearer {{.API_KEY}}' # ❌ not marked as secret + +tasks: + call: + cmds: + - curl -H "{{.HEADER}}" api.example.com + # Logged as: curl -H "Bearer secret-api-key-123" api.example.com (LEAK) +``` + +Mark every variable that carries a secret value as `secret: true`: + +```yaml +vars: + HEADER: + value: 'Bearer {{.API_KEY}}' + secret: true # ✅ masked +``` + +::: + ## Looping over values Task allows you to loop over certain values and execute a command for each. diff --git a/website/src/public/schema.json b/website/src/public/schema.json index 79469b96..71e5b3b2 100644 --- a/website/src/public/schema.json +++ b/website/src/public/schema.json @@ -319,6 +319,9 @@ "type": "object", "description": "The value will be treated as a literal map type and stored in the variable" }, + "value": { + "description": "A literal value assigned to the variable. Useful together with other keys such as 'secret'" + }, "secret": { "type": "boolean", "description": "Marks the variable as secret. Secret values will be masked as ***** in command logs to prevent accidental exposure of sensitive information."