From 39876262e03b476711f4da333d3ffef6e9354897 Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Tue, 28 Jul 2026 09:12:59 +0300 Subject: [PATCH 1/2] Confine tarball extract to artifact root Signed-off-by: Stefan Prodan (cherry picked from commit ff07ecdb08442a31cb9691b1c441b487f17f631a) --- api/v1beta1/artifactgenerator_types.go | 2 +- ...tensions.fluxcd.io_artifactgenerators.yaml | 2 +- internal/builder/builder.go | 25 ++- internal/builder/builder_internal_test.go | 136 +++++++++++++ internal/builder/builder_test.go | 178 ++++++++++++++++++ internal/builder/extract.go | 20 +- internal/builder/extract_test.go | 26 +++ .../artifactgenerator_pathpattern_test.go | 18 +- 8 files changed, 380 insertions(+), 27 deletions(-) create mode 100644 internal/builder/builder_internal_test.go diff --git a/api/v1beta1/artifactgenerator_types.go b/api/v1beta1/artifactgenerator_types.go index 02e28eb1..4fe5feab 100644 --- a/api/v1beta1/artifactgenerator_types.go +++ b/api/v1beta1/artifactgenerator_types.go @@ -166,7 +166,7 @@ type CopyOperation struct { // The format is "@artifact/path", the alias "artifact" // refers to the root path of the generated artifact. When pathPattern // is set, the path may use capture placeholders such as "{app}". - // +kubebuilder:validation:Pattern="^@(artifact)/(.*)$" + // +kubebuilder:validation:Pattern="^@artifact/([^/]{0,1}|[^./][^/]|[.][^./]|[^/]{3,})(/([^/]{0,1}|[^./][^/]|[.][^./]|[^/]{3,}))*$" // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=1024 // +required diff --git a/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml b/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml index 9118a795..4edbd490 100644 --- a/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml +++ b/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml @@ -112,7 +112,7 @@ spec: is set, the path may use capture placeholders such as "{app}". maxLength: 1024 minLength: 1 - pattern: ^@(artifact)/(.*)$ + pattern: ^@artifact/([^/]{0,1}|[^./][^/]|[.][^./]|[^/]{3,})(/([^/]{0,1}|[^./][^/]|[.][^./]|[^/]{3,}))*$ type: string required: - from diff --git a/internal/builder/builder.go b/internal/builder/builder.go index 8e636fed..30f11f5d 100644 --- a/internal/builder/builder.go +++ b/internal/builder/builder.go @@ -187,7 +187,7 @@ func applyCopyOperation(ctx context.Context, if !isGlobPattern { // Direct path reference - check what it actually is first (cp-like behavior) - return applySingleSourceCopy(ctx, op, srcRoot, srcPattern, stagingRoot, stagingDir, destRelPath, destEndsWithSlash) + return applySingleSourceCopy(ctx, op, srcRoot, srcPattern, stagingRoot, destRelPath, destEndsWithSlash) } matches, err := getGlobMatchingEntries(op, srcRoot, srcPattern) @@ -233,7 +233,7 @@ func applyCopyOperation(ctx context.Context, // Ignore files that are not tarball archives and directories continue } - if err := extractTarball(ctx, srcRoot, match, stagingDir, destRelPath); err != nil { + if err := extractTarball(ctx, srcRoot, match, stagingRoot, destRelPath); err != nil { return fmt.Errorf("failed to extract tarball '%s' to '%s': %w", match, destRelPath, err) } } else { @@ -256,7 +256,6 @@ func applySingleSourceCopy(ctx context.Context, srcRoot *os.Root, srcPath string, stagingRoot *os.Root, - stagingDir string, destPath string, destEndsWithSlash bool) error { // Clean the source path to handle trailing slashes @@ -279,7 +278,7 @@ func applySingleSourceCopy(ctx context.Context, return applySingleDirectoryCopy(ctx, op, srcRoot, srcPath, stagingRoot, destPath) } - return applySingleFileCopy(ctx, op, srcRoot, srcPath, stagingRoot, stagingDir, destPath, destEndsWithSlash) + return applySingleFileCopy(ctx, op, srcRoot, srcPath, stagingRoot, destPath, destEndsWithSlash) } // applySingleFileCopy handles copying a single file using cp-like semantics: @@ -290,7 +289,6 @@ func applySingleFileCopy(ctx context.Context, srcRoot *os.Root, srcPath string, stagingRoot *os.Root, - stagingDir string, destPath string, destEndsWithSlash bool) error { // Check if the file should be excluded @@ -303,7 +301,7 @@ func applySingleFileCopy(ctx context.Context, if !isTarball(srcPath) { return fmt.Errorf("extract strategy requires tarball file (.tar.gz or .tgz), got '%s'", srcPath) } - return extractTarball(ctx, srcRoot, srcPath, stagingDir, destPath) + return extractTarball(ctx, srcRoot, srcPath, stagingRoot, destPath) } var finalDestPath string @@ -407,7 +405,20 @@ func parseCopyDestinationRelative(to string) (string, error) { return "", fmt.Errorf("destination must start with '@artifact/'") } - return strings.TrimPrefix(to, "@artifact/"), nil + destPath := strings.TrimPrefix(to, "@artifact/") + if destPath == "" { + return ".", nil + } + if !filepath.IsLocal(destPath) { + return "", fmt.Errorf("destination path must stay within the artifact root") + } + for _, part := range strings.Split(filepath.ToSlash(destPath), "/") { + if part == ".." { + return "", fmt.Errorf("destination path must not contain '..'") + } + } + + return destPath, nil } // copyFileWithRoots copies a file from srcRoot to stagingRoot os.Root, diff --git a/internal/builder/builder_internal_test.go b/internal/builder/builder_internal_test.go new file mode 100644 index 00000000..e708a57e --- /dev/null +++ b/internal/builder/builder_internal_test.go @@ -0,0 +1,136 @@ +/* +Copyright 2026 The Flux authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// This is the only white-box test file in the package. It is reserved for +// defensive guards that no black-box test can reach, because an earlier layer +// rejects the input before the guard runs. Anything reachable through Build +// belongs in builder_test.go instead. + +package builder + +import ( + "context" + "os" + "path/filepath" + "testing" + + . "github.com/onsi/gomega" +) + +// TestRelativeToBase covers the guard rejecting paths that escape basePath. +// Build never produces such a pair: excludeBasePath is always either the copy +// source itself or the literal prefix of its glob, so every walked path is +// underneath it. The guard only matters if a future caller passes an +// unrelated base. +func TestRelativeToBase(t *testing.T) { + tests := []struct { + name string + basePath string + filePath string + want string + wantOK bool + }{ + { + name: "empty base returns the path unchanged", + basePath: "", + filePath: filepath.Join("a", "b"), + want: filepath.Join("a", "b"), + wantOK: true, + }, + { + name: "current directory base returns the path unchanged", + basePath: ".", + filePath: filepath.Join("a", "b"), + want: filepath.Join("a", "b"), + wantOK: true, + }, + { + name: "path under base is made relative", + basePath: "a", + filePath: filepath.Join("a", "b", "c"), + want: filepath.Join("b", "c"), + wantOK: true, + }, + { + name: "trailing slash on base is cleaned", + basePath: "a" + string(filepath.Separator), + filePath: filepath.Join("a", "b"), + want: "b", + wantOK: true, + }, + { + name: "path equal to base", + basePath: filepath.Join("a", "b"), + filePath: filepath.Join("a", "b"), + want: ".", + wantOK: true, + }, + { + name: "leading dots in a name are not treated as traversal", + basePath: "a", + filePath: filepath.Join("a", "..b"), + want: "..b", + wantOK: true, + }, + { + name: "sibling of base escapes", + basePath: filepath.Join("a", "b"), + filePath: filepath.Join("a", "c"), + wantOK: false, + }, + { + name: "parent of base escapes", + basePath: filepath.Join("a", "b"), + filePath: "a", + wantOK: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + got, ok := relativeToBase(tt.basePath, tt.filePath) + g.Expect(ok).To(Equal(tt.wantOK)) + if !tt.wantOK { + g.Expect(got).To(BeEmpty()) + return + } + g.Expect(got).To(Equal(tt.want)) + }) + } +} + +func TestExtractTarballConfinesDestination(t *testing.T) { + g := NewWithT(t) + tmpDir := t.TempDir() + sourceDir := filepath.Join(tmpDir, "source") + stagingDir := filepath.Join(tmpDir, "staging") + g.Expect(os.MkdirAll(sourceDir, 0o755)).To(Succeed()) + g.Expect(os.MkdirAll(stagingDir, 0o755)).To(Succeed()) + g.Expect(os.WriteFile(filepath.Join(sourceDir, "manifests.tgz"), []byte("invalid"), 0o644)).To(Succeed()) + + srcRoot, err := os.OpenRoot(sourceDir) + g.Expect(err).ToNot(HaveOccurred()) + defer srcRoot.Close() + stagingRoot, err := os.OpenRoot(stagingDir) + g.Expect(err).ToNot(HaveOccurred()) + defer stagingRoot.Close() + + err = extractTarball(context.Background(), srcRoot, "manifests.tgz", stagingRoot, "../escape") + g.Expect(err).To(MatchError(ContainSubstring("failed to create destination directory"))) + g.Expect(filepath.Join(tmpDir, "escape")).ToNot(BeADirectory()) + g.Expect(filepath.Join(tmpDir, "escape", "config.yaml")).ToNot(BeAnExistingFile()) +} diff --git a/internal/builder/builder_test.go b/internal/builder/builder_test.go index 5caf5eae..6a01f4fd 100644 --- a/internal/builder/builder_test.go +++ b/internal/builder/builder_test.go @@ -28,6 +28,7 @@ import ( gotkmeta "github.com/fluxcd/pkg/apis/meta" swapi "github.com/fluxcd/source-watcher/api/v2/v1beta1" + "github.com/fluxcd/source-watcher/v2/internal/builder" ) func TestBuild(t *testing.T) { @@ -690,6 +691,183 @@ func TestBuildErrors(t *testing.T) { } } +func TestBuildCopySources(t *testing.T) { + tests := []struct { + name string + from string + expectedError string + }{ + { + name: "missing alias prefix", + from: "source/config.yaml", + expectedError: "source must start with '@'", + }, + { + name: "alias without a pattern", + from: "@source", + expectedError: "source format must be '@alias/pattern'", + }, + { + name: "unknown alias", + from: "@missing/config.yaml", + expectedError: "source alias 'missing' not found", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + srcDir := filepath.Join(tmpDir, "source") + workspaceDir := filepath.Join(tmpDir, "workspace") + + setupDirs(t, srcDir, workspaceDir) + createFile(t, srcDir, "config.yaml", "apiVersion: v1") + + spec := &swapi.OutputArtifact{ + Name: "cp-source", + Copy: []swapi.CopyOperation{ + { + From: tt.from, + To: "@artifact/", + }, + }, + } + sources := map[string]string{"source": srcDir} + + _, err := testBuilder.Build(context.Background(), spec, sources, "test-namespace", workspaceDir) + g.Expect(err).To(MatchError(ContainSubstring(tt.expectedError))) + }) + } +} + +func TestMkdirTempAbs(t *testing.T) { + t.Run("returns a fully resolved absolute path", func(t *testing.T) { + g := NewWithT(t) + + tmpDir, err := builder.MkdirTempAbs("", "ag-") + g.Expect(err).ToNot(HaveOccurred()) + defer os.RemoveAll(tmpDir) + + g.Expect(tmpDir).To(BeADirectory()) + g.Expect(filepath.IsAbs(tmpDir)).To(BeTrue()) + + // The returned path must contain no symlink components, otherwise + // paths derived from it would not compare equal to their resolved + // form. On macOS os.MkdirTemp returns /var/..., a symlink to + // /private/var. + resolved, err := filepath.EvalSymlinks(tmpDir) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(tmpDir).To(Equal(resolved)) + }) + + t.Run("error when the parent directory does not exist", func(t *testing.T) { + g := NewWithT(t) + + _, err := builder.MkdirTempAbs(filepath.Join(t.TempDir(), "nonexistent"), "ag-") + g.Expect(err).To(HaveOccurred()) + }) +} + +func TestBuildCopyDestinations(t *testing.T) { + tests := []struct { + name string + to string + expectedFile string + expectedError string + }{ + { + name: "artifact root", + to: "@artifact/", + expectedFile: "config.yaml", + }, + { + name: "subdirectory with trailing slash", + to: "@artifact/sub/dir/", + expectedFile: filepath.Join("sub", "dir", "config.yaml"), + }, + { + name: "subdirectory without trailing slash renames the file", + to: "@artifact/sub/dir", + expectedFile: filepath.Join("sub", "dir"), + }, + { + name: "current directory component", + to: "@artifact/./sub/", + expectedFile: filepath.Join("sub", "config.yaml"), + }, + { + name: "dots within a directory name", + to: "@artifact/sub..dir/", + expectedFile: filepath.Join("sub..dir", "config.yaml"), + }, + { + name: "parent directory", + to: "@artifact/..", + expectedError: "destination path must stay within the artifact root", + }, + { + name: "parent traversal", + to: "@artifact/../../escape", + expectedError: "destination path must stay within the artifact root", + }, + { + name: "nested parent component", + to: "@artifact/sub/../escape", + expectedError: "destination path must not contain '..'", + }, + { + name: "absolute path", + to: "@artifact//tmp/escape", + expectedError: "destination path must stay within the artifact root", + }, + { + name: "unknown destination alias", + to: "@source/sub", + expectedError: "destination must start with '@artifact/'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + srcDir := filepath.Join(tmpDir, "source") + workspaceDir := filepath.Join(tmpDir, "workspace") + + setupDirs(t, srcDir, workspaceDir) + createFile(t, srcDir, "config.yaml", "apiVersion: v1") + + spec := &swapi.OutputArtifact{ + Name: "cp-destination", + Copy: []swapi.CopyOperation{ + { + From: "@source/config.yaml", + To: tt.to, + }, + }, + } + sources := map[string]string{"source": srcDir} + + _, err := testBuilder.Build(context.Background(), spec, sources, "test-namespace", workspaceDir) + stagingDir := filepath.Join(workspaceDir, spec.Name) + + if tt.expectedError != "" { + g.Expect(err).To(MatchError(ContainSubstring(tt.expectedError))) + // Nothing must be written outside the staging directory. + g.Expect(filepath.Join(tmpDir, "escape")).ToNot(BeAnExistingFile()) + g.Expect(filepath.Join(workspaceDir, "escape")).ToNot(BeAnExistingFile()) + return + } + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(filepath.Join(stagingDir, tt.expectedFile)).To(BeAnExistingFile()) + }) + } +} + func TestBuildWithExcludes(t *testing.T) { tests := []struct { name string diff --git a/internal/builder/extract.go b/internal/builder/extract.go index 8e54189a..73778151 100644 --- a/internal/builder/extract.go +++ b/internal/builder/extract.go @@ -58,7 +58,7 @@ func isTarball(path string) bool { func extractTarball(ctx context.Context, srcRoot *os.Root, srcPath string, - stagingDir string, + stagingRoot *os.Root, destPath string) error { if err := ctx.Err(); err != nil { return err @@ -71,15 +71,19 @@ func extractTarball(ctx context.Context, } defer srcFile.Close() - // Create the full destination path - fullDestPath := filepath.Join(stagingDir, destPath) - if err := os.MkdirAll(fullDestPath, 0o755); err != nil { - return fmt.Errorf("failed to create destination directory %q: %w", fullDestPath, err) + cleanDestPath := filepath.Clean(destPath) + if err := stagingRoot.MkdirAll(cleanDestPath, 0o755); err != nil { + return fmt.Errorf("failed to create destination directory %q: %w", destPath, err) } - // Use fluxcd/pkg/tar.Untar for secure extraction - if err := tar.Untar(srcFile, fullDestPath); err != nil { - return fmt.Errorf("failed to extract tarball %q to %q: %w", srcPath, fullDestPath, err) + destRoot, err := stagingRoot.OpenRoot(cleanDestPath) + if err != nil { + return fmt.Errorf("failed to open destination directory %q: %w", destPath, err) + } + defer destRoot.Close() + + if err := tar.Untar(srcFile, destRoot.Name()); err != nil { + return fmt.Errorf("failed to extract tarball %q to %q: %w", srcPath, destPath, err) } return nil diff --git a/internal/builder/extract_test.go b/internal/builder/extract_test.go index 549ff514..6c0c2a0b 100644 --- a/internal/builder/extract_test.go +++ b/internal/builder/extract_test.go @@ -616,6 +616,32 @@ env: second } +func TestBuild_ExtractStrategyRejectsDestinationTraversal(t *testing.T) { + g := NewWithT(t) + tmpDir := t.TempDir() + sourceDir := filepath.Join(tmpDir, "source") + workspaceDir := filepath.Join(tmpDir, "workspace") + setupDirs(t, sourceDir, workspaceDir) + g.Expect(createTestTarball(filepath.Join(sourceDir, "manifests.tgz"))).To(Succeed()) + + spec := &swapi.OutputArtifact{ + Name: "extract-traversal", + Copy: []swapi.CopyOperation{ + { + From: "@source/manifests.tgz", + To: "@artifact/../../escape", + Strategy: swapi.ExtractStrategy, + }, + }, + } + + artifact, err := testBuilder.Build(context.Background(), spec, + map[string]string{"source": sourceDir}, "test-extract", workspaceDir) + g.Expect(err).To(MatchError(ContainSubstring("destination path must stay within the artifact root"))) + g.Expect(artifact).To(BeNil()) + g.Expect(filepath.Join(tmpDir, "escape", "config.yaml")).ToNot(BeAnExistingFile()) +} + // createTestTarball creates a test tarball with sample files func createTestTarball(path string) error { file, err := os.Create(path) diff --git a/internal/controller/artifactgenerator_pathpattern_test.go b/internal/controller/artifactgenerator_pathpattern_test.go index ce1e3b5e..9027b6e1 100644 --- a/internal/controller/artifactgenerator_pathpattern_test.go +++ b/internal/controller/artifactgenerator_pathpattern_test.go @@ -419,26 +419,24 @@ func TestBuildArtifactRequests(t *testing.T) { t.Run("duplicate names after lowercasing", func(t *testing.T) { g := gomega.NewWithT(t) - dupDir, err := os.MkdirTemp("", "dup-test") - g.Expect(err).ToNot(gomega.HaveOccurred()) - defer os.RemoveAll(dupDir) - - dupAliasDir := filepath.Join(dupDir, "repo") - // Both will lowercase to "auth" - os.MkdirAll(filepath.Join(dupAliasDir, "apps", "Auth"), 0o755) - os.MkdirAll(filepath.Join(dupAliasDir, "apps", "auth"), 0o755) + dupAliasDir := filepath.Join(t.TempDir(), "repo") + // "Auth" and "auth" both lowercase to "auth", so the rendered names + // collide. They are kept under different parents so the two paths stay + // distinct on case-insensitive filesystems such as macOS APFS. + g.Expect(os.MkdirAll(filepath.Join(dupAliasDir, "apps", "dev", "Auth"), 0o755)).To(gomega.Succeed()) + g.Expect(os.MkdirAll(filepath.Join(dupAliasDir, "apps", "prod", "auth"), 0o755)).To(gomega.Succeed()) dupSources := map[string]string{"repo": dupAliasDir} obj := &swapi.ArtifactGenerator{ Spec: swapi.ArtifactGeneratorSpec{ - PathPattern: "@repo/apps/{app}", + PathPattern: "@repo/apps/{env}/{app}", OutputArtifacts: []swapi.OutputArtifact{ {Name: "app-{app}"}, }, }, } - _, err = buildArtifactRequests(obj, dupSources) + _, err := buildArtifactRequests(obj, dupSources) g.Expect(err).To(gomega.HaveOccurred()) g.Expect(err.Error()).To(gomega.ContainSubstring("pathPattern")) g.Expect(err.Error()).To(gomega.ContainSubstring("both resolve to artifact name")) From b129c433c39e41c2931bf0683eed10d2f79f6420 Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Tue, 28 Jul 2026 10:05:31 +0300 Subject: [PATCH 2/2] Bound glob alternation expansion Signed-off-by: Stefan Prodan (cherry picked from commit e69070777d7afa385310e487d99d3c6e190686bc) --- api/v1beta1/artifactgenerator_types.go | 1 + ...tensions.fluxcd.io_artifactgenerators.yaml | 1 + docs/spec/v1beta1/artifactgenerators.md | 5 +- internal/builder/builder.go | 49 +++++++- internal/builder/builder_test.go | 116 ++++++++++++++++++ 5 files changed, 167 insertions(+), 5 deletions(-) diff --git a/api/v1beta1/artifactgenerator_types.go b/api/v1beta1/artifactgenerator_types.go index 4fe5feab..df148eaa 100644 --- a/api/v1beta1/artifactgenerator_types.go +++ b/api/v1beta1/artifactgenerator_types.go @@ -178,6 +178,7 @@ type CopyOperation struct { // prefix of 'From'. Patterns without a separator (e.g. "*.md") match // the file name at any depth. // +kubebuilder:validation:MaxItems=100 + // +kubebuilder:validation:items:MaxLength=1024 // +optional Exclude []string `json:"exclude,omitempty"` diff --git a/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml b/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml index 4edbd490..35d9e1e3 100644 --- a/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml +++ b/config/crd/bases/source.extensions.fluxcd.io_artifactgenerators.yaml @@ -78,6 +78,7 @@ spec: prefix of 'From'. Patterns without a separator (e.g. "*.md") match the file name at any depth. items: + maxLength: 1024 type: string maxItems: 100 type: array diff --git a/docs/spec/v1beta1/artifactgenerators.md b/docs/spec/v1beta1/artifactgenerators.md index dc5992e5..26fe5db8 100644 --- a/docs/spec/v1beta1/artifactgenerators.md +++ b/docs/spec/v1beta1/artifactgenerators.md @@ -319,10 +319,13 @@ Each copy operation specifies how to copy files from sources into the generated Any file matched by `from` that also matches an exclude pattern will be ignored. Patterns are matched against paths relative to the source alias root or to the non-glob prefix of `from`. Patterns without a separator (e.g. `*.md`) match the - file name at any depth. + file name at any depth. Each pattern is limited to 1024 characters. - `strategy` (optional): Defines how to handle files during copy operations: `Overwrite` (default), `Merge` (for YAML files), or `Extract` (for tarball archives). +The `from` and `exclude` glob patterns may each contain at most 20 commas +inside `{}` alternation groups; patterns over the limit are rejected. + Copy operations use `cp`-like semantics: - Operations are executed in order; later operations can overwrite files from earlier ones diff --git a/internal/builder/builder.go b/internal/builder/builder.go index 30f11f5d..b5429f90 100644 --- a/internal/builder/builder.go +++ b/internal/builder/builder.go @@ -22,6 +22,7 @@ import ( "io/fs" "os" "path/filepath" + "slices" "strings" "github.com/bmatcuk/doublestar/v4" @@ -152,6 +153,10 @@ func applyCopyOperation(ctx context.Context, return fmt.Errorf("invalid copy source '%s': %w", op.From, err) } + if err := validateGlobPattern(srcPattern); err != nil { + return fmt.Errorf("invalid copy source '%s': %w", op.From, err) + } + destRelPath, err := parseCopyDestinationRelative(op.To) if err != nil { return fmt.Errorf("invalid copy destination '%s': %w", op.To, err) @@ -163,6 +168,9 @@ func applyCopyOperation(ctx context.Context, } for _, pattern := range op.Exclude { + if err := validateGlobPattern(pattern); err != nil { + return fmt.Errorf("invalid exclude pattern '%s': %w", pattern, err) + } if _, err := doublestar.Match(pattern, "."); err != nil { return fmt.Errorf("invalid exclude pattern '%s'", pattern) } @@ -344,6 +352,41 @@ func containsGlobChars(path string) bool { return strings.ContainsAny(path, "*?[]") } +// maxGlobAlternationCommas bounds glob alternation expansion. doublestar +// re-matches the whole pattern once per alternative and cannot be interrupted, +// so cost is the product of the group widths; commas bound that product at 2^n. +// 20 is ~780ms worst case, 22 is ~3s. +const maxGlobAlternationCommas = 20 + +// validateGlobPattern bounds a pattern's alternation expansion. Only commas +// inside a '{}' group count; a backslash escapes the next byte. Must run before +// any matcher, including doublestar.Match. +func validateGlobPattern(pattern string) error { + var commas, depth int + for i := 0; i < len(pattern); i++ { + switch pattern[i] { + case '\\': + i++ // escaped byte is a literal + case '{': + depth++ + case '}': + if depth > 0 { + depth-- + } + case ',': + if depth > 0 { + commas++ + } + } + } + + if commas > maxGlobAlternationCommas { + return fmt.Errorf("pattern has %d alternatives in '{}' groups, at most %d are allowed", + commas, maxGlobAlternationCommas) + } + return nil +} + // calculateGlobDestination determines the correct destination path for a glob match // to match cp-like behavior for different glob patterns: // - dir/** patterns strip the directory prefix (like cp -r dir/** dest/) @@ -412,10 +455,8 @@ func parseCopyDestinationRelative(to string) (string, error) { if !filepath.IsLocal(destPath) { return "", fmt.Errorf("destination path must stay within the artifact root") } - for _, part := range strings.Split(filepath.ToSlash(destPath), "/") { - if part == ".." { - return "", fmt.Errorf("destination path must not contain '..'") - } + if slices.Contains(strings.Split(filepath.ToSlash(destPath), "/"), "..") { + return "", fmt.Errorf("destination path must not contain '..'") } return destPath, nil diff --git a/internal/builder/builder_test.go b/internal/builder/builder_test.go index 6a01f4fd..3a4b76d8 100644 --- a/internal/builder/builder_test.go +++ b/internal/builder/builder_test.go @@ -21,7 +21,9 @@ import ( "fmt" "os" "path/filepath" + "strings" "testing" + "time" . "github.com/onsi/gomega" @@ -742,6 +744,120 @@ func TestBuildCopySources(t *testing.T) { } } +// TestBuildGlobAlternationLimit checks that patterns too expensive to match are +// rejected before reaching a matcher. Every rejected pattern below takes seconds +// or more to match, so the elapsed assertion catches validation running late. +func TestBuildGlobAlternationLimit(t *testing.T) { + manyGroups := strings.Repeat("{a,a}", 30) + "z" // ~50s + wideGroups := strings.Repeat("{a,a,a,a,a,a,a,a}", 10) + "z" // ~30s; passes a group-count bound + stallsPreValidation := "{.,.}" + strings.Repeat("{,}", 29) + "z" // stalls doublestar.Match(p, ".") + atLimit := strings.Repeat("{a,a}", 20) + "zzz*" // accepted, matches nothing + + tests := []struct { + name string + from string + strategy string + exclude []string + expectedError string + }{ + { + name: "many narrow groups in from", + from: "@source/" + manyGroups + "*", + expectedError: "at most 20 are allowed", + }, + { + name: "many narrow groups in exclude", + from: "@source/**", + exclude: []string{manyGroups}, + expectedError: "at most 20 are allowed", + }, + { + name: "many narrow groups in a later exclude entry", + from: "@source/**", + exclude: []string{"*.md", manyGroups}, + expectedError: "at most 20 are allowed", + }, + { + name: "few wide groups in exclude", + from: "@source/**", + exclude: []string{wideGroups}, + expectedError: "at most 20 are allowed", + }, + { + name: "few wide groups in from", + from: "@source/" + wideGroups + "*", + expectedError: "at most 20 are allowed", + }, + { + name: "pattern that stalls the exclude syntax check", + from: "@source/**", + exclude: []string{stallsPreValidation}, + expectedError: "at most 20 are allowed", + }, + { + // Extract takes the doublestar.Glob branch, not fs.Glob. + name: "many narrow groups in from with Extract strategy", + from: "@source/" + manyGroups + "*.tgz", + strategy: swapi.ExtractStrategy, + expectedError: "at most 20 are allowed", + }, + { + name: "at the limit is accepted", + from: "@source/**", + exclude: []string{atLimit}, + }, + { + name: "escaped braces are literals and do not count", + from: "@source/**", + exclude: []string{strings.Repeat(`\{a\}`, 12) + "/**"}, + }, + { + name: "realistic extension alternation is accepted", + from: "@source/**", + exclude: []string{"**/*.{yaml,yml,json,toml}"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + srcDir := filepath.Join(tmpDir, "source") + workspaceDir := filepath.Join(tmpDir, "workspace") + + setupDirs(t, srcDir, workspaceDir) + // Long enough for the alternation recursion to bite. + createFile(t, srcDir, strings.Repeat("a", 40), "content") + + spec := &swapi.OutputArtifact{ + Name: "glob-alternations", + Copy: []swapi.CopyOperation{ + { + From: tt.from, + To: "@artifact/", + Exclude: tt.exclude, + Strategy: tt.strategy, + }, + }, + } + sources := map[string]string{"source": srcDir} + + start := time.Now() + _, err := testBuilder.Build(context.Background(), spec, sources, "test-namespace", workspaceDir) + elapsed := time.Since(start) + + if tt.expectedError != "" { + g.Expect(err).To(MatchError(ContainSubstring(tt.expectedError))) + // Rejected before matching, not after. + g.Expect(elapsed).To(BeNumerically("<", 2*time.Second)) + return + } + g.Expect(err).ToNot(HaveOccurred()) + }) + } +} + func TestMkdirTempAbs(t *testing.T) { t.Run("returns a fully resolved absolute path", func(t *testing.T) { g := NewWithT(t)