diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index e563db22e..91b8d245b 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -549,6 +549,10 @@ func (cad *cadEngine) UpdatePackageResourcesWithoutRender(ctx context.Context, r return nil, fmt.Errorf("cannot update a package revision with lifecycle value %q; package must be Draft", lifecycle) } + if err := util.ValidateResourcePaths(newRes.Spec.Resources); err != nil { + return nil, err + } + repo, err := cad.cache.OpenRepository(ctx, repositoryObj) if err != nil { return nil, err diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 8819049d4..7f562cfdf 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -894,13 +894,15 @@ func TestUpdatePackageResourcesRenderFailure(t *testing.T) { func TestUpdatePackageResourcesWithoutRender(t *testing.T) { tests := []struct { - name string - lifecycle porchapi.PackageRevisionLifecycle - oldRV string - newRV string - closeErr error - expectError bool - errorContains string + name string + lifecycle porchapi.PackageRevisionLifecycle + oldRV string + newRV string + resources map[string]string + closeErr error + skipWriteClose bool + expectError bool + errorContains string }{ { name: "success - draft lifecycle", @@ -941,6 +943,16 @@ func TestUpdatePackageResourcesWithoutRender(t *testing.T) { expectError: true, errorContains: "git push failed", }, + { + name: "failure - path traversal rejected", + lifecycle: porchapi.PackageRevisionLifecycleDraft, + oldRV: "1", + newRV: "1", + resources: map[string]string{"../../etc/config": "content"}, + skipWriteClose: true, + expectError: true, + errorContains: "invalid resource path", + }, } for _, tt := range tests { @@ -963,21 +975,26 @@ func TestUpdatePackageResourcesWithoutRender(t *testing.T) { ResourceVersion: tt.oldRV, }, } + resources := tt.resources + if resources == nil { + resources = map[string]string{"Kptfile": "test"} + } newRes := &porchapi.PackageRevisionResources{ ObjectMeta: metav1.ObjectMeta{ Name: "test-pkg", ResourceVersion: tt.newRV, }, Spec: porchapi.PackageRevisionResourcesSpec{ - Resources: map[string]string{"Kptfile": "test"}, + Resources: resources, }, } mockPkgRev.On("Lifecycle", mock.Anything).Return(tt.lifecycle).Maybe() mockPkgRev.On("Key").Return(repository.PackageRevisionKey{}).Maybe() - // Only expect repo open + draft flow when we pass validation - needsDraft := tt.newRV != "" && tt.oldRV == tt.newRV && tt.lifecycle == porchapi.PackageRevisionLifecycleDraft + // Only expect repo open + draft flow when we pass all pre-draft validation + needsDraft := !tt.skipWriteClose && tt.newRV != "" && tt.oldRV == tt.newRV && + tt.lifecycle == porchapi.PackageRevisionLifecycleDraft if needsDraft { mockCache.On("OpenRepository", mock.Anything, repositoryObj).Return(mockRepo, nil) mockRepo.On("UpdatePackageRevision", mock.Anything, mockPkgRev).Return(mockDraft, nil) diff --git a/pkg/engine/safejoin.go b/pkg/engine/safejoin.go deleted file mode 100644 index 9896da586..000000000 --- a/pkg/engine/safejoin.go +++ /dev/null @@ -1,37 +0,0 @@ -// Copyright 2022 The kpt 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. - -package engine - -import ( - "fmt" - "path/filepath" - "strings" -) - -// Relevant: https://github.com/golang/go/issues/20126 - -func filepathSafeJoin(dir string, relative string) (string, error) { - p := filepath.Join(dir, relative) - p = filepath.Clean(p) - - rel, err := filepath.Rel(dir, p) - if err != nil { - return "", fmt.Errorf("invalid relative path %q", relative) - } - if rel != relative || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || strings.HasPrefix(rel, "."+string(filepath.Separator)) { - return "", fmt.Errorf("invalid relative path %q", relative) - } - return p, nil -} diff --git a/pkg/repository/update.go b/pkg/repository/update.go index 3d33b1fdc..ec575cb4b 100644 --- a/pkg/repository/update.go +++ b/pkg/repository/update.go @@ -1,4 +1,4 @@ -// Copyright 2022-2025 The kpt Authors +// Copyright 2022-2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -23,6 +23,7 @@ import ( "github.com/kptdev/kpt/pkg/lib/update" updatetypes "github.com/kptdev/kpt/pkg/lib/update/updatetypes" + "github.com/kptdev/porch/pkg/util" ) const LocalUpdateDir = "kpt-pkg-update-*" @@ -99,13 +100,16 @@ func (m *DefaultPackageUpdater) do(_ context.Context, localPkgDir, originalPkgDi func writeResourcesToDirectory(dir string, resources PackageResources) error { for k, v := range resources.Contents { - p := filepath.Join(dir, k) - dir := filepath.Dir(p) - if err := os.MkdirAll(dir, 0750); err != nil { - return fmt.Errorf("failed to create directory %q: %w", dir, err) + p, err := util.FilepathSafeJoin(dir, k) + if err != nil { + return fmt.Errorf("invalid resource path %q: %w", k, err) + } + d := filepath.Dir(p) + if err := os.MkdirAll(d, 0750); err != nil { + return fmt.Errorf("failed to create directory %q: %w", d, err) } if err := os.WriteFile(p, []byte(v), 0600); err != nil { - return fmt.Errorf("failed to write file %q: %w", dir, err) + return fmt.Errorf("failed to write file %q: %w", p, err) } } return nil diff --git a/pkg/repository/update_test.go b/pkg/repository/update_test.go index 4c96ecd8e..0e2a8e04d 100644 --- a/pkg/repository/update_test.go +++ b/pkg/repository/update_test.go @@ -1,4 +1,4 @@ -// Copyright 2022, 2024 The kpt Authors +// Copyright 2022, 2024, 2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -149,3 +149,29 @@ func TestDefaultPackageUpdaterdo(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "upstream content", updatedResources.Contents["file1.txt"]) } + +func TestWriteResourcesToDirectoryRejectsInvalidPaths(t *testing.T) { + dir := t.TempDir() + + tests := []struct { + name string + key string + }{ + {"relative path escapes base", "../escape.txt"}, + {"nested relative path escapes base", "a/../../escape.txt"}, + {"absolute path", "/etc/file"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resources := PackageResources{ + Contents: map[string]string{ + tt.key: "content", + }, + } + err := writeResourcesToDirectory(dir, resources) + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid resource path") + }) + } +} diff --git a/pkg/task/replace_test.go b/pkg/task/replace_test.go index 9bb937994..a50cfa9fe 100644 --- a/pkg/task/replace_test.go +++ b/pkg/task/replace_test.go @@ -1,4 +1,4 @@ -// Copyright 2022, 2024 The kpt Authors +// Copyright 2022, 2024, 2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -91,3 +91,36 @@ func removeCommentsFromFile(t *testing.T, name, contents string) string { return nocomment.String() } + +func TestReplaceResourcesRejectsInvalidPaths(t *testing.T) { + ctx := context.Background() + + replace := &replaceResourcesMutation{ + newResources: &porchapi.PackageRevisionResources{ + Spec: porchapi.PackageRevisionResourcesSpec{ + Resources: map[string]string{ + "../../../etc/config": "content", + }, + }, + }, + oldResources: &porchapi.PackageRevisionResources{ + Spec: porchapi.PackageRevisionResourcesSpec{ + Resources: map[string]string{ + "Kptfile": "existing", + }, + }, + }, + } + + input := repository.PackageResources{ + Contents: map[string]string{"Kptfile": "existing"}, + } + + _, _, err := replace.apply(ctx, input) + if err == nil { + t.Fatal("expected error for invalid path, got nil") + } + if !bytes.Contains([]byte(err.Error()), []byte("invalid resource path")) { + t.Errorf("unexpected error message: %v", err) + } +} diff --git a/pkg/task/replaceresources.go b/pkg/task/replaceresources.go index b59013727..0cd5b6af8 100644 --- a/pkg/task/replaceresources.go +++ b/pkg/task/replaceresources.go @@ -1,4 +1,4 @@ -// Copyright 2024 The kpt Authors +// Copyright 2024, 2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -20,6 +20,7 @@ import ( porchapi "github.com/kptdev/porch/api/porch/v1alpha1" "github.com/kptdev/porch/pkg/repository" + "github.com/kptdev/porch/pkg/util" "go.opentelemetry.io/otel/trace" ) @@ -34,6 +35,10 @@ func (m *replaceResourcesMutation) apply(ctx context.Context, resources reposito _, span := tracer.Start(ctx, "mutationReplaceResources::apply", trace.WithAttributes()) defer span.End() + if err := util.ValidateResourcePaths(m.newResources.Spec.Resources); err != nil { + return repository.PackageResources{}, nil, err + } + old := resources.Contents newRes, err := healConfig(old, m.newResources.Spec.Resources) if err != nil { diff --git a/pkg/util/safejoin.go b/pkg/util/safejoin.go new file mode 100644 index 000000000..5d4b1521c --- /dev/null +++ b/pkg/util/safejoin.go @@ -0,0 +1,55 @@ +// Copyright 2022, 2026 The kpt 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. + +package util + +import ( + "fmt" + "path/filepath" + "strings" +) + +// Relevant: https://github.com/golang/go/issues/20126 + +// FilepathSafeJoin joins dir and relative, returning an error if relative is +// not a clean, canonical relative path within dir. It rejects path traversal +// (.. sequences), absolute paths, the bare "." and ".." entries, and any path +// that would be altered by filepath.Clean (e.g. leading "./", redundant +// separators, or internal "a/../b" segments). +func FilepathSafeJoin(dir, relative string) (string, error) { + p := filepath.Join(dir, relative) + p = filepath.Clean(p) + + rel, err := filepath.Rel(dir, p) + if err != nil { + return "", fmt.Errorf("invalid relative path %q", relative) + } + if rel == "." || rel == ".." || rel != relative || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || strings.HasPrefix(rel, "."+string(filepath.Separator)) { + return "", fmt.Errorf("invalid relative path %q", relative) + } + return p, nil +} + +// ValidateResourcePaths checks that all keys in a resource map are valid +// relative file paths within a package. It rejects path traversal sequences, +// absolute paths, and non-canonical paths (e.g. leading "./", bare "." or ".."). +// Returns an error on the first invalid key. +func ValidateResourcePaths(resources map[string]string) error { + for k := range resources { + if _, err := FilepathSafeJoin(".", k); err != nil { + return fmt.Errorf("invalid resource path %q: %w", k, err) + } + } + return nil +} diff --git a/pkg/engine/safejoin_test.go b/pkg/util/safejoin_test.go similarity index 53% rename from pkg/engine/safejoin_test.go rename to pkg/util/safejoin_test.go index 3c2e711fb..9c2abd8fd 100644 --- a/pkg/engine/safejoin_test.go +++ b/pkg/util/safejoin_test.go @@ -1,4 +1,4 @@ -// Copyright 2022 The kpt Authors +// Copyright 2022, 2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package engine +package util import ( "fmt" @@ -21,7 +21,7 @@ import ( // Relevant: https://github.com/golang/go/issues/20126 -func TestSafeJoin(t *testing.T) { +func TestFilepathSafeJoin(t *testing.T) { grid := []struct { base string relative string @@ -63,6 +63,16 @@ func TestSafeJoin(t *testing.T) { relative: "../foo", wantError: true, }, + { + base: "/tmp", + relative: "..", + wantError: true, + }, + { + base: "/tmp", + relative: ".", + wantError: true, + }, { base: "tmp/", relative: "a/../foo", @@ -82,7 +92,7 @@ func TestSafeJoin(t *testing.T) { for _, g := range grid { t.Run(fmt.Sprintf("%#v", g), func(t *testing.T) { - got, err := filepathSafeJoin(g.base, g.relative) + got, err := FilepathSafeJoin(g.base, g.relative) if g.wantError { if err == nil { t.Errorf("got %q and nil error, want error", got) @@ -95,3 +105,59 @@ func TestSafeJoin(t *testing.T) { }) } } + +func TestValidateResourcePaths(t *testing.T) { + tests := []struct { + name string + resources map[string]string + wantError bool + }{ + { + name: "valid simple path", + resources: map[string]string{"Kptfile": "content", "subdir/file.yaml": "content"}, + wantError: false, + }, + { + name: "path traversal with ..", + resources: map[string]string{"../etc/config": "content"}, + wantError: true, + }, + { + name: "bare .. without trailing path", + resources: map[string]string{"..": "content"}, + wantError: true, + }, + { + name: "dot refers to directory not a file", + resources: map[string]string{".": "content"}, + wantError: true, + }, + { + name: "path traversal nested", + resources: map[string]string{"a/../../etc/file": "content"}, + wantError: true, + }, + { + name: "absolute path", + resources: map[string]string{"/etc/cron.d/job": "content"}, + wantError: true, + }, + { + name: "empty map is valid", + resources: map[string]string{}, + wantError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateResourcePaths(tt.resources) + if tt.wantError && err == nil { + t.Error("expected error, got nil") + } + if !tt.wantError && err != nil { + t.Errorf("unexpected error: %v", err) + } + }) + } +} diff --git a/test/e2e/api/rpkg_edit_test.go b/test/e2e/api/rpkg_edit_test.go index 1997483cf..47d1afd1f 100644 --- a/test/e2e/api/rpkg_edit_test.go +++ b/test/e2e/api/rpkg_edit_test.go @@ -1,4 +1,4 @@ -// Copyright 2025 The kpt Authors +// Copyright 2025-2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -216,6 +216,30 @@ metadata: t.Logf("ConfigMap content matches expected result after KRM function processing") } +func (t *PorchSuite) TestUpdateResourcesRejectsInvalidPaths() { + const ( + repository = "invalid-path-test" + packageName = "invalid-path-pkg" + workspace = defaultWorkspace + ) + + t.RegisterGitRepositoryF(t.GetPorchTestRepoURL(), repository, "", suiteutils.GiteaUser, suiteutils.GiteaPassword) + pr := t.CreatePackageDraftF(repository, packageName, workspace) + + var prResources porchapi.PackageRevisionResources + t.GetF(client.ObjectKey{ + Namespace: t.Namespace, + Name: pr.Name, + }, &prResources) + + // Attempt to add a resource with a relative path that escapes the package directory + prResources.Spec.Resources["../../etc/config"] = "content" + + err := t.Client.Update(t.GetContext(), &prResources) + t.Require().Error(err, "update with invalid resource path should be rejected") + t.Require().ErrorContains(err, "invalid resource path") +} + func (t *PorchSuite) TestUpdateResourcesEmptyPatch() { const ( repository = "empty-patch-test"