From 38bf4c51ef88335ce679cec439dbc8dc9af8f3c9 Mon Sep 17 00:00:00 2001 From: Oleks Date: Tue, 4 Aug 2026 01:00:23 +0300 Subject: [PATCH] analyze: widen unescaped-${VAR} detector to scan the whole raw pipeline file findUnescapedVars only walked parsed Commands strings, so a literal ${VAR} in a step name, top-level comment, or environment: value - all outside any commands: string - passed the check clean while Woodpecker's own ${VAR} substitution pass (which runs over the entire raw pipeline YAML text, not just command bodies) hard-failed the pipeline at compile time. Add findUnescapedVarsInFile as a whole-file pass alongside the existing per-command scan, deduped against it so nothing inside a commands: string gets reported twice. Fixes oleks/pipetree#13, reproduced by a fixture matching the actual incident: an explanatory comment above a step's commands: block containing unescaped ${VAR} example text (oleks/deals pipelines #12/#13, oleks/element-web-patched #5/#6). --- internal/analyze/footguns.go | 51 ++++++++++++++++++- internal/analyze/footguns_test.go | 24 +++++++++ .../deals-comment-above-commands.yaml | 18 +++++++ internal/render/analyze.go | 8 ++- 4 files changed, 98 insertions(+), 3 deletions(-) create mode 100644 internal/analyze/testdata/deals-comment-above-commands.yaml diff --git a/internal/analyze/footguns.go b/internal/analyze/footguns.go index 8bd4de8..afa6344 100644 --- a/internal/analyze/footguns.go +++ b/internal/analyze/footguns.go @@ -35,6 +35,7 @@ package analyze import ( + "os" "regexp" "sort" "strings" @@ -151,13 +152,41 @@ func findUnescapedVars(cmd string, matrixVars map[string]bool) []struct { Line int Var string Context string +} { + return scanLinesForUnescapedVars(strings.Split(cmd, "\n"), matrixVars) +} + +// findUnescapedVarsInFile scans the FULL raw pipeline YAML text - not just +// parsed `commands:` strings - for the same `${VAR}` footgun. This closes a +// false-negative gap: Woodpecker's `${VAR}` substitution pass runs over the +// entire raw pipeline file text before parsing, including step names, +// top-level/inline comments and `environment:` values that live outside any +// commands string, so a file can pass findUnescapedVars clean and still get +// rejected by Woodpecker at compile time (oleks/pipetree#13 - a real repro: +// a `#` comment ABOVE a step's commands: block containing literal `${VAR}` +// example text compiled fine per the old command-only scan, but Woodpecker +// itself hard-failed with "unable to parse variable name" on push). +func findUnescapedVarsInFile(rawText string, matrixVars map[string]bool) []struct { + Line int + Var string + Context string +} { + return scanLinesForUnescapedVars(strings.Split(rawText, "\n"), matrixVars) +} + +// scanLinesForUnescapedVars is the line-level detector shared by the +// per-command scan (findUnescapedVars) and the whole-file scan +// (findUnescapedVarsInFile) - only the text they're handed differs. +func scanLinesForUnescapedVars(lines []string, matrixVars map[string]bool) []struct { + Line int + Var string + Context string } { var hits []struct { Line int Var string Context string } - lines := strings.Split(cmd, "\n") for i, line := range lines { trimmed := strings.TrimSpace(line) for _, loc := range dollarBraceRe.FindAllStringSubmatchIndex(line, -1) { @@ -190,6 +219,12 @@ func runFootguns(proj *model.Project, apos *[]StrayApostrophe, vars *[]Unescaped matrixVars[axis] = true } } + // seen dedups the whole-file pass below against hits the + // per-command pass already attributed to a step, keyed on the + // (trimmed) source line and var name - both passes see the same + // physical line for anything inside a commands: string, and + // without this a hit there would be reported twice. + seen := make(map[string]bool) for _, s := range pl.Steps { for _, cmd := range s.Commands { for _, h := range findStrayApostrophes(cmd) { @@ -203,9 +238,23 @@ func runFootguns(proj *model.Project, apos *[]StrayApostrophe, vars *[]Unescaped Project: proj.Name, Pipeline: pl.Name, Step: s.Name, Line: h.Line, Var: h.Var, Context: h.Context, }) + seen[h.Var+"\x00"+h.Context] = true } } } + if raw, err := os.ReadFile(pl.File); err == nil { + for _, h := range findUnescapedVarsInFile(string(raw), matrixVars) { + key := h.Var + "\x00" + h.Context + if seen[key] { + continue + } + seen[key] = true + *vars = append(*vars, UnescapedWoodpeckerVar{ + Project: proj.Name, Pipeline: pl.Name, + Line: h.Line, Var: h.Var, Context: h.Context, + }) + } + } } } diff --git a/internal/analyze/footguns_test.go b/internal/analyze/footguns_test.go index b73e331..03e3a35 100644 --- a/internal/analyze/footguns_test.go +++ b/internal/analyze/footguns_test.go @@ -115,6 +115,30 @@ func TestFootguns_NegativeCase_MatrixVar(t *testing.T) { } } +// TestUnescapedVar_RawFilePass_PositiveCase reproduces oleks/pipetree#13: a +// literal, unescaped ${VAR} in a YAML comment ABOVE a step's commands: +// block. The per-command scan alone would miss it (the comment line is +// never part of any parsed Commands string), but Woodpecker's own +// substitution pass runs over the whole raw file, so pipetree must catch it +// too via the whole-file pass (findUnescapedVarsInFile). +func TestUnescapedVar_RawFilePass_PositiveCase(t *testing.T) { + proj := loadProject(t, "deals", "testdata/deals-comment-above-commands.yaml") + report := Run([]*model.Project{proj}) + + found := false + for _, u := range report.UnescapedVars { + if u.Var == "VAR" { + found = true + if u.Step != "" { + t.Errorf("expected no step attribution for a comment-line hit outside any commands: block, got Step=%q", u.Step) + } + } + } + if !found { + t.Errorf("expected the whole-file pass to flag the unescaped ${VAR} in the comment above the step, got: %+v", report.UnescapedVars) + } +} + // TestFindStrayApostrophes_Unit exercises the line-level detector directly // against small synthetic commands, independent of YAML parsing. func TestFindStrayApostrophes_Unit(t *testing.T) { diff --git a/internal/analyze/testdata/deals-comment-above-commands.yaml b/internal/analyze/testdata/deals-comment-above-commands.yaml new file mode 100644 index 0000000..ac0c0fe --- /dev/null +++ b/internal/analyze/testdata/deals-comment-above-commands.yaml @@ -0,0 +1,18 @@ +# Repro for oleks/pipetree#13: an explanatory comment ABOVE a step's +# commands: block containing literal, unescaped ${VAR} example text (the +# genuine mistake made while documenting this exact footgun on +# oleks/deals). The old command-only scan (findUnescapedVars) never saw +# this line because a comment line isn't part of any parsed Commands +# string - but Woodpecker's own ${VAR} substitution pass runs over the +# ENTIRE raw pipeline file, so it hard-failed at compile time ("unable to +# parse variable name") on oleks/deals pipelines #12/#13 and +# oleks/element-web-patched #5/#6. Below: this comment escapes it as +# ${VAR} unescaped on purpose, to reproduce the false negative. +when: + - event: [push, pull_request] + +steps: + - name: build + image: git.oleks.space/oleks/nix-ci:latest + commands: + - echo "no footgun down here" diff --git a/internal/render/analyze.go b/internal/render/analyze.go index bd6c987..b901944 100644 --- a/internal/render/analyze.go +++ b/internal/render/analyze.go @@ -32,8 +32,12 @@ func Analyze(w io.Writer, report analyze.Report) { _, _ = fmt.Fprintf(w, " %s / %s / step %s, line %d: %s\n", a.Project, a.Pipeline, a.Step, a.Line, a.Context) } - _, _ = fmt.Fprintf(w, "\n%d unescaped ${VAR} in step commands (Woodpecker substitutes these at parse time, before the shell runs - escape as $${VAR} to hand it to the shell):\n", len(report.UnescapedVars)) + _, _ = fmt.Fprintf(w, "\n%d unescaped ${VAR} in the pipeline file (Woodpecker substitutes these at parse time, before the shell runs - escape as $${VAR} to hand it to the shell):\n", len(report.UnescapedVars)) for _, u := range report.UnescapedVars { - _, _ = fmt.Fprintf(w, " %s / %s / step %s, line %d: ${%s} in %s\n", u.Project, u.Pipeline, u.Step, u.Line, u.Var, u.Context) + if u.Step != "" { + _, _ = fmt.Fprintf(w, " %s / %s / step %s, line %d: ${%s} in %s\n", u.Project, u.Pipeline, u.Step, u.Line, u.Var, u.Context) + } else { + _, _ = fmt.Fprintf(w, " %s / %s, line %d (outside any step's commands): ${%s} in %s\n", u.Project, u.Pipeline, u.Line, u.Var, u.Context) + } } } -- 2.54.0