Skip to content

Commit 0ed0bfe

Browse files
committed
fix(config,sbom): address multi-review risks
Apply fixes for 7 risks surfaced by the multi-role review: - #1 (HIGH) buildResolvers() sorts by inputType before returning — deterministic resolver order across process restarts. Prevents flaky tests and non-reproducible SBOM output ordering. + regression tests: 'deterministic order across invocations' and 'orders by inputType alphabetically'. - #2 (HIGH) Ecosystems() returns maps.Clone(ecosystems) instead of the package-level map — defensive copy prevents accidental global mutation by callers. - #3 (MEDIUM) pip install --no-cache-dir -r <spec> — standard Docker practice, prevents pip cache layer bloat in build images. - #4 (MEDIUM) e2e pip fixture now uses short alias 'type: pip' (matches the exact user path from Kaiten 65682576). Previously covered only by unit tests; now e2e validates alias → canonical → install → SBOM. - #5 (MEDIUM) fillFileBasedSpec rejects 'lock:' for ecosystems without lock semantics (pip) with 'lock is not supported for type %q'. Silent accept would confuse users about reproducibility guarantees. + failure test entry. - #8 (MEDIUM) InstallCmd uses %q instead of %s for workdir/spec — shell- quotes values against injection. Defensive coding: values currently come from controlled werf.yaml, but template rendering ({{ env }}) widens the attack surface. - #11 (LOW) Drop redundant 'eco := eco' loop capture (Go 1.24 uses per- iteration binding natively). Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
1 parent 785a2ed commit 0ed0bfe

7 files changed

Lines changed: 99 additions & 39 deletions

File tree

pkg/build/stage/packages_test.go

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ var _ = Describe("PackagesStage", func() {
2424
)
2525

2626
It("returns stage when packages commands present", func(ctx context.Context) {
27-
stage := generateTestPackagesStage(ctx, "cd /app && go mod download")
27+
stage := generateTestPackagesStage(ctx, "cd \"/app\" && go mod download")
2828
Expect(stage).NotTo(BeNil())
2929
})
3030
})
@@ -42,7 +42,7 @@ var _ = Describe("PackagesStage", func() {
4242
)
4343

4444
It("Name returns packages", func(ctx context.Context) {
45-
stage := generateTestPackagesStage(ctx, "cd /app && go mod download")
45+
stage := generateTestPackagesStage(ctx, "cd \"/app\" && go mod download")
4646
Expect(stage.Name()).To(Equal(Packages))
4747
})
4848

@@ -67,21 +67,21 @@ var _ = Describe("PackagesStage", func() {
6767
},
6868

6969
Entry("same commands produce same hash",
70-
[]string{"cd /app && go mod download"},
71-
[]string{"cd /app && go mod download"},
70+
[]string{"cd \"/app\" && go mod download"},
71+
[]string{"cd \"/app\" && go mod download"},
7272
true),
7373
Entry("different commands produce different hash",
74-
[]string{"cd /app && go mod download"},
75-
[]string{"cd /lib && go mod download"},
74+
[]string{"cd \"/app\" && go mod download"},
75+
[]string{"cd \"/lib\" && go mod download"},
7676
false),
7777
Entry("different number of commands produce different hash",
78-
[]string{"cd /app && go mod download"},
79-
[]string{"cd /app && go mod download", "cd /lib && go mod download"},
78+
[]string{"cd \"/app\" && go mod download"},
79+
[]string{"cd \"/app\" && go mod download", "cd \"/lib\" && go mod download"},
8080
false),
8181
)
8282

8383
It("returns non-empty hash", func(ctx context.Context) {
84-
s := generateTestPackagesStage(ctx, "cd /app && go mod download")
84+
s := generateTestPackagesStage(ctx, "cd \"/app\" && go mod download")
8585
digest, err := s.GetDependencies(ctx, testConveyor(), nil, nil, nil, nil)
8686
Expect(err).NotTo(HaveOccurred())
8787
Expect(digest).NotTo(BeEmpty())
@@ -92,7 +92,7 @@ var _ = Describe("PackagesStage", func() {
9292
It("does not affect install stage creation", func(ctx context.Context) {
9393
imageBaseConfig := &config.StapelImageBase{
9494
Shell: &config.Shell{
95-
Packages: []string{"cd /app && go mod download"},
95+
Packages: []string{"cd \"/app\" && go mod download"},
9696
Install: []string{"go build ./..."},
9797
},
9898
}
@@ -158,20 +158,20 @@ var _ = Describe("GeneratePackagesCommands", func() {
158158

159159
Entry("go-mod /app", []*config.PackagesDirective{
160160
{Type: config.PackagesDirectiveTypeGoMod, FileBased: config.FileBasedSpec{Workdir: "/app"}},
161-
}, []string{"cd /app && go mod download"}),
161+
}, []string{"cd \"/app\" && go mod download"}),
162162

163163
Entry("go-mod /src/backend", []*config.PackagesDirective{
164164
{Type: config.PackagesDirectiveTypeGoMod, FileBased: config.FileBasedSpec{Workdir: "/src/backend"}},
165-
}, []string{"cd /src/backend && go mod download"}),
165+
}, []string{"cd \"/src/backend\" && go mod download"}),
166166

167167
Entry("multiple go-mod entries", []*config.PackagesDirective{
168168
{Type: config.PackagesDirectiveTypeGoMod, FileBased: config.FileBasedSpec{Workdir: "/app"}},
169169
{Type: config.PackagesDirectiveTypeGoMod, FileBased: config.FileBasedSpec{Workdir: "/lib"}},
170-
}, []string{"cd /app && go mod download", "cd /lib && go mod download"}),
170+
}, []string{"cd \"/app\" && go mod download", "cd \"/lib\" && go mod download"}),
171171

172172
Entry("root workdir", []*config.PackagesDirective{
173173
{Type: config.PackagesDirectiveTypeGoMod, FileBased: config.FileBasedSpec{Workdir: "/"}},
174-
}, []string{"cd / && go mod download"}),
174+
}, []string{"cd \"/\" && go mod download"}),
175175

176176
Entry("unsupported type", []*config.PackagesDirective{
177177
{Type: config.PackagesDirectiveType("unknown")},
@@ -191,28 +191,28 @@ var _ = Describe("GeneratePackagesCommands", func() {
191191
{Type: config.PackagesDirectiveTypeOSPM, Spec: config.PackagesSpec{Packages: []string{"curl"}}},
192192
{Type: config.PackagesDirectiveTypeGoMod, FileBased: config.FileBasedSpec{Workdir: "/app"}},
193193
{Type: config.PackagesDirectiveType("cargo")},
194-
}, []string{"pm install curl", "cd /app && go mod download"}),
194+
}, []string{"pm install curl", "cd \"/app\" && go mod download"}),
195195

196196
Entry("python-pip /app requirements.txt", []*config.PackagesDirective{
197197
{
198198
Type: config.PackagesDirectiveTypePythonPip,
199199
FileBased: config.FileBasedSpec{Workdir: "/app", Spec: "requirements.txt"},
200200
},
201-
}, []string{"cd /app && pip install -r requirements.txt"}),
201+
}, []string{"cd \"/app\" && pip install --no-cache-dir -r \"requirements.txt\""}),
202202

203203
Entry("python-poetry /svc", []*config.PackagesDirective{
204204
{
205205
Type: config.PackagesDirectiveTypePythonPoetry,
206206
FileBased: config.FileBasedSpec{Workdir: "/svc"},
207207
},
208-
}, []string{"cd /svc && poetry install --no-root"}),
208+
}, []string{"cd \"/svc\" && poetry install --no-root"}),
209209

210210
Entry("python-uv /api", []*config.PackagesDirective{
211211
{
212212
Type: config.PackagesDirectiveTypePythonUV,
213213
FileBased: config.FileBasedSpec{Workdir: "/api"},
214214
},
215-
}, []string{"cd /api && uv sync --frozen"}),
215+
}, []string{"cd \"/api\" && uv sync --frozen"}),
216216

217217
Entry("mixed: go-mod + python-uv + os-pm all produce commands", []*config.PackagesDirective{
218218
{Type: config.PackagesDirectiveTypeGoMod, FileBased: config.FileBasedSpec{Workdir: "/app"}},
@@ -221,7 +221,7 @@ var _ = Describe("GeneratePackagesCommands", func() {
221221
FileBased: config.FileBasedSpec{Workdir: "/lib"},
222222
},
223223
{Type: config.PackagesDirectiveTypeOSPM, Spec: config.PackagesSpec{Packages: []string{"curl"}}},
224-
}, []string{"cd /app && go mod download", "cd /lib && uv sync --frozen", "pm install curl"}),
224+
}, []string{"cd \"/app\" && go mod download", "cd \"/lib\" && uv sync --frozen", "pm install curl"}),
225225
)
226226
})
227227

@@ -258,7 +258,7 @@ var _ = Describe("Shell.Packages config field", func() {
258258
}
259259
},
260260

261-
Entry("populated", &config.Shell{Packages: []string{"cd /app && go mod download"}}, 1, "cd /app && go mod download"),
261+
Entry("populated", &config.Shell{Packages: []string{"cd \"/app\" && go mod download"}}, 1, "cd \"/app\" && go mod download"),
262262
Entry("empty", &config.Shell{}, 0, ""),
263263
)
264264

@@ -290,12 +290,12 @@ var _ = Describe("Builder interface Packages methods", func() {
290290
},
291291

292292
Entry("same commands — same checksum",
293-
[]string{"cd /app && go mod download"},
294-
[]string{"cd /app && go mod download"},
293+
[]string{"cd \"/app\" && go mod download"},
294+
[]string{"cd \"/app\" && go mod download"},
295295
true),
296296
Entry("different commands — different checksum",
297-
[]string{"cd /app && go mod download"},
298-
[]string{"cd /lib && go mod download"},
297+
[]string{"cd \"/app\" && go mod download"},
298+
[]string{"cd \"/lib\" && go mod download"},
299299
false),
300300
)
301301

pkg/config/packages_directive.go

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package config
22

33
import (
44
"fmt"
5+
"maps"
56
"sort"
67
)
78

@@ -39,31 +40,33 @@ var ecosystems = map[PackagesDirectiveType]PackageEcosystem{
3940
Type: PackagesDirectiveTypeGoMod,
4041
DefaultSpec: "go.mod",
4142
DefaultLock: "go.sum",
42-
InstallCmd: func(workdir, _ string) string { return fmt.Sprintf("cd %s && go mod download", workdir) },
43+
InstallCmd: func(workdir, _ string) string { return fmt.Sprintf("cd %q && go mod download", workdir) },
4344
CatalogerName: "go-module-file-cataloger",
4445
},
4546
PackagesDirectiveTypePythonUV: {
4647
Type: PackagesDirectiveTypePythonUV,
4748
Aliases: []string{"uv"},
4849
DefaultSpec: "pyproject.toml",
4950
DefaultLock: "uv.lock",
50-
InstallCmd: func(workdir, _ string) string { return fmt.Sprintf("cd %s && uv sync --frozen", workdir) },
51+
InstallCmd: func(workdir, _ string) string { return fmt.Sprintf("cd %q && uv sync --frozen", workdir) },
5152
CatalogerName: "python-package-cataloger",
5253
},
5354
PackagesDirectiveTypePythonPip: {
54-
Type: PackagesDirectiveTypePythonPip,
55-
Aliases: []string{"pip"},
56-
DefaultSpec: "requirements.txt",
57-
DefaultLock: "",
58-
InstallCmd: func(workdir, spec string) string { return fmt.Sprintf("cd %s && pip install -r %s", workdir, spec) },
55+
Type: PackagesDirectiveTypePythonPip,
56+
Aliases: []string{"pip"},
57+
DefaultSpec: "requirements.txt",
58+
DefaultLock: "",
59+
InstallCmd: func(workdir, spec string) string {
60+
return fmt.Sprintf("cd %q && pip install --no-cache-dir -r %q", workdir, spec)
61+
},
5962
CatalogerName: "python-package-cataloger",
6063
},
6164
PackagesDirectiveTypePythonPoetry: {
6265
Type: PackagesDirectiveTypePythonPoetry,
6366
Aliases: []string{"poetry"},
6467
DefaultSpec: "pyproject.toml",
6568
DefaultLock: "poetry.lock",
66-
InstallCmd: func(workdir, _ string) string { return fmt.Sprintf("cd %s && poetry install --no-root", workdir) },
69+
InstallCmd: func(workdir, _ string) string { return fmt.Sprintf("cd %q && poetry install --no-root", workdir) },
6770
CatalogerName: "python-package-cataloger",
6871
},
6972
}
@@ -78,10 +81,10 @@ var aliasToType = func() map[string]PackagesDirectiveType {
7881
return idx
7982
}()
8083

81-
// Ecosystems returns the registry of file-based package ecosystems.
82-
// The map is intended read-only; callers must not mutate entries.
84+
// Ecosystems returns a defensive copy of the file-based package ecosystems registry.
85+
// The returned map is safe to iterate; mutating it does not affect the internal registry.
8386
func Ecosystems() map[PackagesDirectiveType]PackageEcosystem {
84-
return ecosystems
87+
return maps.Clone(ecosystems)
8588
}
8689

8790
type PackagesDirective struct {

pkg/config/packages_directive_python_test.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -214,5 +214,19 @@ var _ = Describe("rawPackagesDirective python", func() {
214214
},
215215
},
216216
),
217+
218+
Entry("python-pip with lock field is rejected (pip has no lock semantics)",
219+
map[string]interface{}{
220+
"image": "image1",
221+
"from": "python:3.12",
222+
"packages": []map[string]interface{}{
223+
{
224+
"type": "python-pip",
225+
"workdir": "/app",
226+
"lock": "requirements.lock",
227+
},
228+
},
229+
},
230+
),
217231
)
218232
})

pkg/config/raw_packages_directive.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,9 @@ func (r *rawPackagesDirective) fillFileBasedSpec(d *PackagesDirective) error {
107107

108108
d.FileBased.Lock = eco.DefaultLock
109109
if r.Lock != "" {
110+
if eco.DefaultLock == "" {
111+
return fmt.Errorf("lock is not supported for type %q", d.Type)
112+
}
110113
d.FileBased.Lock = r.Lock
111114
}
112115

pkg/sbom/managedinput/managedinput.go

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package managedinput
22

33
import (
44
"path"
5+
"slices"
56
"strings"
67

78
cdx "github.com/CycloneDX/cyclonedx-go"
@@ -20,9 +21,16 @@ type inputResolver struct {
2021
var resolvers = buildResolvers()
2122

2223
func buildResolvers() []inputResolver {
23-
built := make([]inputResolver, 0, len(config.Ecosystems()))
24-
for _, eco := range config.Ecosystems() {
25-
eco := eco
24+
ecosystems := config.Ecosystems()
25+
types := make([]config.PackagesDirectiveType, 0, len(ecosystems))
26+
for t := range ecosystems {
27+
types = append(types, t)
28+
}
29+
slices.Sort(types)
30+
31+
built := make([]inputResolver, 0, len(types))
32+
for _, t := range types {
33+
eco := ecosystems[t]
2634
built = append(built, inputResolver{
2735
inputType: eco.Type,
2836
catalogerName: eco.CatalogerName,

pkg/sbom/managedinput/managedinput_test.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package managedinput
22

33
import (
44
"fmt"
5+
"sort"
56

67
cdx "github.com/CycloneDX/cyclonedx-go"
78
. "github.com/onsi/ginkgo/v2"
@@ -11,6 +12,37 @@ import (
1112
"github.com/werf/werf/v2/pkg/sbom/scanner"
1213
)
1314

15+
var _ = Describe("buildResolvers", func() {
16+
It("returns resolvers in deterministic order across invocations", func() {
17+
first := buildResolvers()
18+
firstOrder := make([]config.PackagesDirectiveType, len(first))
19+
for i, r := range first {
20+
firstOrder[i] = r.inputType
21+
}
22+
23+
for i := 0; i < 20; i++ {
24+
next := buildResolvers()
25+
nextOrder := make([]config.PackagesDirectiveType, len(next))
26+
for j, r := range next {
27+
nextOrder[j] = r.inputType
28+
}
29+
Expect(nextOrder).To(Equal(firstOrder))
30+
}
31+
})
32+
33+
It("orders resolvers by inputType alphabetically", func() {
34+
built := buildResolvers()
35+
order := make([]string, len(built))
36+
for i, r := range built {
37+
order[i] = string(r.inputType)
38+
}
39+
sorted := make([]string, len(order))
40+
copy(sorted, order)
41+
sort.Strings(sorted)
42+
Expect(order).To(Equal(sorted))
43+
})
44+
})
45+
1446
var _ = Describe("ToCatalogers", func() {
1547
DescribeTable("maps packages directives to syft catalogers",
1648
func(packages []*config.PackagesDirective, expected []scanner.Cataloger) {

test/e2e/sbom/_fixtures/inject/pip_simple/werf.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,5 +14,5 @@ shell:
1414
install:
1515
- cd /app && pip install -r requirements.txt
1616
packages:
17-
- type: python-pip
17+
- type: pip
1818
workdir: /app

0 commit comments

Comments
 (0)