Skip to content

Commit 51e8126

Browse files
authored
Validate resource file paths in package operations (#1065)
* validate resource file paths in package operations Signed-off-by: Aravindhan Ayyanathan <aravindhan.a@est.tech> * Address review comments Signed-off-by: Aravindhan Ayyanathan <aravindhan.a@est.tech> --------- Signed-off-by: Aravindhan Ayyanathan <aravindhan.a@est.tech>
1 parent d71f6d8 commit 51e8126

10 files changed

Lines changed: 258 additions & 61 deletions

File tree

pkg/engine/engine.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -549,6 +549,10 @@ func (cad *cadEngine) UpdatePackageResourcesWithoutRender(ctx context.Context, r
549549
return nil, fmt.Errorf("cannot update a package revision with lifecycle value %q; package must be Draft", lifecycle)
550550
}
551551

552+
if err := util.ValidateResourcePaths(newRes.Spec.Resources); err != nil {
553+
return nil, err
554+
}
555+
552556
repo, err := cad.cache.OpenRepository(ctx, repositoryObj)
553557
if err != nil {
554558
return nil, err

pkg/engine/engine_test.go

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -894,13 +894,15 @@ func TestUpdatePackageResourcesRenderFailure(t *testing.T) {
894894

895895
func TestUpdatePackageResourcesWithoutRender(t *testing.T) {
896896
tests := []struct {
897-
name string
898-
lifecycle porchapi.PackageRevisionLifecycle
899-
oldRV string
900-
newRV string
901-
closeErr error
902-
expectError bool
903-
errorContains string
897+
name string
898+
lifecycle porchapi.PackageRevisionLifecycle
899+
oldRV string
900+
newRV string
901+
resources map[string]string
902+
closeErr error
903+
skipWriteClose bool
904+
expectError bool
905+
errorContains string
904906
}{
905907
{
906908
name: "success - draft lifecycle",
@@ -941,6 +943,16 @@ func TestUpdatePackageResourcesWithoutRender(t *testing.T) {
941943
expectError: true,
942944
errorContains: "git push failed",
943945
},
946+
{
947+
name: "failure - path traversal rejected",
948+
lifecycle: porchapi.PackageRevisionLifecycleDraft,
949+
oldRV: "1",
950+
newRV: "1",
951+
resources: map[string]string{"../../etc/config": "content"},
952+
skipWriteClose: true,
953+
expectError: true,
954+
errorContains: "invalid resource path",
955+
},
944956
}
945957

946958
for _, tt := range tests {
@@ -963,21 +975,26 @@ func TestUpdatePackageResourcesWithoutRender(t *testing.T) {
963975
ResourceVersion: tt.oldRV,
964976
},
965977
}
978+
resources := tt.resources
979+
if resources == nil {
980+
resources = map[string]string{"Kptfile": "test"}
981+
}
966982
newRes := &porchapi.PackageRevisionResources{
967983
ObjectMeta: metav1.ObjectMeta{
968984
Name: "test-pkg",
969985
ResourceVersion: tt.newRV,
970986
},
971987
Spec: porchapi.PackageRevisionResourcesSpec{
972-
Resources: map[string]string{"Kptfile": "test"},
988+
Resources: resources,
973989
},
974990
}
975991

976992
mockPkgRev.On("Lifecycle", mock.Anything).Return(tt.lifecycle).Maybe()
977993
mockPkgRev.On("Key").Return(repository.PackageRevisionKey{}).Maybe()
978994

979-
// Only expect repo open + draft flow when we pass validation
980-
needsDraft := tt.newRV != "" && tt.oldRV == tt.newRV && tt.lifecycle == porchapi.PackageRevisionLifecycleDraft
995+
// Only expect repo open + draft flow when we pass all pre-draft validation
996+
needsDraft := !tt.skipWriteClose && tt.newRV != "" && tt.oldRV == tt.newRV &&
997+
tt.lifecycle == porchapi.PackageRevisionLifecycleDraft
981998
if needsDraft {
982999
mockCache.On("OpenRepository", mock.Anything, repositoryObj).Return(mockRepo, nil)
9831000
mockRepo.On("UpdatePackageRevision", mock.Anything, mockPkgRev).Return(mockDraft, nil)

pkg/engine/safejoin.go

Lines changed: 0 additions & 37 deletions
This file was deleted.

pkg/repository/update.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright 2022-2025 The kpt Authors
1+
// Copyright 2022-2026 The kpt Authors
22
//
33
// Licensed under the Apache License, Version 2.0 (the "License");
44
// you may not use this file except in compliance with the License.
@@ -23,6 +23,7 @@ import (
2323

2424
"github.com/kptdev/kpt/pkg/lib/update"
2525
updatetypes "github.com/kptdev/kpt/pkg/lib/update/updatetypes"
26+
"github.com/kptdev/porch/pkg/util"
2627
)
2728

2829
const LocalUpdateDir = "kpt-pkg-update-*"
@@ -99,13 +100,16 @@ func (m *DefaultPackageUpdater) do(_ context.Context, localPkgDir, originalPkgDi
99100

100101
func writeResourcesToDirectory(dir string, resources PackageResources) error {
101102
for k, v := range resources.Contents {
102-
p := filepath.Join(dir, k)
103-
dir := filepath.Dir(p)
104-
if err := os.MkdirAll(dir, 0750); err != nil {
105-
return fmt.Errorf("failed to create directory %q: %w", dir, err)
103+
p, err := util.FilepathSafeJoin(dir, k)
104+
if err != nil {
105+
return fmt.Errorf("invalid resource path %q: %w", k, err)
106+
}
107+
d := filepath.Dir(p)
108+
if err := os.MkdirAll(d, 0750); err != nil {
109+
return fmt.Errorf("failed to create directory %q: %w", d, err)
106110
}
107111
if err := os.WriteFile(p, []byte(v), 0600); err != nil {
108-
return fmt.Errorf("failed to write file %q: %w", dir, err)
112+
return fmt.Errorf("failed to write file %q: %w", p, err)
109113
}
110114
}
111115
return nil

pkg/repository/update_test.go

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright 2022, 2024 The kpt Authors
1+
// Copyright 2022, 2024, 2026 The kpt Authors
22
//
33
// Licensed under the Apache License, Version 2.0 (the "License");
44
// you may not use this file except in compliance with the License.
@@ -149,3 +149,29 @@ func TestDefaultPackageUpdaterdo(t *testing.T) {
149149
assert.NoError(t, err)
150150
assert.Equal(t, "upstream content", updatedResources.Contents["file1.txt"])
151151
}
152+
153+
func TestWriteResourcesToDirectoryRejectsInvalidPaths(t *testing.T) {
154+
dir := t.TempDir()
155+
156+
tests := []struct {
157+
name string
158+
key string
159+
}{
160+
{"relative path escapes base", "../escape.txt"},
161+
{"nested relative path escapes base", "a/../../escape.txt"},
162+
{"absolute path", "/etc/file"},
163+
}
164+
165+
for _, tt := range tests {
166+
t.Run(tt.name, func(t *testing.T) {
167+
resources := PackageResources{
168+
Contents: map[string]string{
169+
tt.key: "content",
170+
},
171+
}
172+
err := writeResourcesToDirectory(dir, resources)
173+
assert.Error(t, err)
174+
assert.Contains(t, err.Error(), "invalid resource path")
175+
})
176+
}
177+
}

pkg/task/replace_test.go

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright 2022, 2024 The kpt Authors
1+
// Copyright 2022, 2024, 2026 The kpt Authors
22
//
33
// Licensed under the Apache License, Version 2.0 (the "License");
44
// 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 {
9191

9292
return nocomment.String()
9393
}
94+
95+
func TestReplaceResourcesRejectsInvalidPaths(t *testing.T) {
96+
ctx := context.Background()
97+
98+
replace := &replaceResourcesMutation{
99+
newResources: &porchapi.PackageRevisionResources{
100+
Spec: porchapi.PackageRevisionResourcesSpec{
101+
Resources: map[string]string{
102+
"../../../etc/config": "content",
103+
},
104+
},
105+
},
106+
oldResources: &porchapi.PackageRevisionResources{
107+
Spec: porchapi.PackageRevisionResourcesSpec{
108+
Resources: map[string]string{
109+
"Kptfile": "existing",
110+
},
111+
},
112+
},
113+
}
114+
115+
input := repository.PackageResources{
116+
Contents: map[string]string{"Kptfile": "existing"},
117+
}
118+
119+
_, _, err := replace.apply(ctx, input)
120+
if err == nil {
121+
t.Fatal("expected error for invalid path, got nil")
122+
}
123+
if !bytes.Contains([]byte(err.Error()), []byte("invalid resource path")) {
124+
t.Errorf("unexpected error message: %v", err)
125+
}
126+
}

pkg/task/replaceresources.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright 2024 The kpt Authors
1+
// Copyright 2024, 2026 The kpt Authors
22
//
33
// Licensed under the Apache License, Version 2.0 (the "License");
44
// you may not use this file except in compliance with the License.
@@ -20,6 +20,7 @@ import (
2020

2121
porchapi "github.com/kptdev/porch/api/porch/v1alpha1"
2222
"github.com/kptdev/porch/pkg/repository"
23+
"github.com/kptdev/porch/pkg/util"
2324
"go.opentelemetry.io/otel/trace"
2425
)
2526

@@ -34,6 +35,10 @@ func (m *replaceResourcesMutation) apply(ctx context.Context, resources reposito
3435
_, span := tracer.Start(ctx, "mutationReplaceResources::apply", trace.WithAttributes())
3536
defer span.End()
3637

38+
if err := util.ValidateResourcePaths(m.newResources.Spec.Resources); err != nil {
39+
return repository.PackageResources{}, nil, err
40+
}
41+
3742
old := resources.Contents
3843
newRes, err := healConfig(old, m.newResources.Spec.Resources)
3944
if err != nil {

pkg/util/safejoin.go

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
// Copyright 2022, 2026 The kpt Authors
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package util
16+
17+
import (
18+
"fmt"
19+
"path/filepath"
20+
"strings"
21+
)
22+
23+
// Relevant: https://github.com/golang/go/issues/20126
24+
25+
// FilepathSafeJoin joins dir and relative, returning an error if relative is
26+
// not a clean, canonical relative path within dir. It rejects path traversal
27+
// (.. sequences), absolute paths, the bare "." and ".." entries, and any path
28+
// that would be altered by filepath.Clean (e.g. leading "./", redundant
29+
// separators, or internal "a/../b" segments).
30+
func FilepathSafeJoin(dir, relative string) (string, error) {
31+
p := filepath.Join(dir, relative)
32+
p = filepath.Clean(p)
33+
34+
rel, err := filepath.Rel(dir, p)
35+
if err != nil {
36+
return "", fmt.Errorf("invalid relative path %q", relative)
37+
}
38+
if rel == "." || rel == ".." || rel != relative || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || strings.HasPrefix(rel, "."+string(filepath.Separator)) {
39+
return "", fmt.Errorf("invalid relative path %q", relative)
40+
}
41+
return p, nil
42+
}
43+
44+
// ValidateResourcePaths checks that all keys in a resource map are valid
45+
// relative file paths within a package. It rejects path traversal sequences,
46+
// absolute paths, and non-canonical paths (e.g. leading "./", bare "." or "..").
47+
// Returns an error on the first invalid key.
48+
func ValidateResourcePaths(resources map[string]string) error {
49+
for k := range resources {
50+
if _, err := FilepathSafeJoin(".", k); err != nil {
51+
return fmt.Errorf("invalid resource path %q: %w", k, err)
52+
}
53+
}
54+
return nil
55+
}

0 commit comments

Comments
 (0)