Skip to content

Commit 1df7118

Browse files
committed
fix(commands): tighten outside-root path check to match path element
Address review feedback from coderabbitai on pkg/commands/template.go: `strings.HasPrefix(relPath, "..")` also matched valid paths whose first directory component literally starts with "..", such as `..templates/controlplane.yaml`. On the template command path, that sent a legitimate input into the templates/<basename> fallback and silently loaded a different file when one happened to exist under templates/. Extract isOutsideRoot helper that matches ".\".\" + path separator" or the exact ".." element, and route all three duplicate check sites (apply.go resolveTemplatePaths, template.go generateOutput modeline branch, template.go resolveEngineTemplatePaths) through it. Add a regression test that seeds a rootDir/..templates/controlplane.yaml real target alongside a rootDir/templates/controlplane.yaml decoy and asserts the real target is returned. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
1 parent 855b13b commit 1df7118

4 files changed

Lines changed: 96 additions & 4 deletions

File tree

pkg/commands/apply.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,15 @@ func wrapWithNodeContext(f func(ctx context.Context, c *client.Client) error) fu
259259
}
260260
}
261261

262+
// isOutsideRoot reports whether a cleaned relative path escapes the
263+
// project root. A HasPrefix(".." ) test would misclassify sibling
264+
// directories whose first path element merely starts with "..", such
265+
// as "..templates/controlplane.yaml"; we match a full path element
266+
// instead.
267+
func isOutsideRoot(relPath string) bool {
268+
return relPath == ".." || strings.HasPrefix(relPath, ".."+string(filepath.Separator))
269+
}
270+
262271
// resolveTemplatePaths resolves template file paths relative to the project root,
263272
// normalizing them for the Helm engine (forward slashes).
264273
// Relative paths from the modeline are resolved against rootDir, not CWD.
@@ -300,7 +309,7 @@ func resolveTemplatePaths(templates []string, rootDir string) []string {
300309
continue
301310
}
302311
relPath = filepath.Clean(relPath)
303-
if strings.HasPrefix(relPath, "..") {
312+
if isOutsideRoot(relPath) {
304313
// Path goes outside project root — use original path as-is
305314
resolved[i] = engine.NormalizeTemplatePath(templatePath)
306315
continue

pkg/commands/apply_test.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,12 @@ func TestResolveTemplatePaths(t *testing.T) {
9595
if err := os.MkdirAll(filepath.Join(tmpRoot, "templates"), 0o755); err != nil {
9696
t.Fatalf("failed to create templates dir: %v", err)
9797
}
98+
// A sibling directory whose name literally starts with "..". A naive
99+
// HasPrefix(relPath, "..") check would misclassify it as outside-root;
100+
// the resolver must treat ".." as a full path element, not a prefix.
101+
if err := os.MkdirAll(filepath.Join(tmpRoot, "..templates"), 0o755); err != nil {
102+
t.Fatalf("failed to create ..templates dir: %v", err)
103+
}
98104

99105
// Build a platform-portable absolute path outside tmpRoot.
100106
// filepath.VolumeName is "" on POSIX (yielding e.g. "/other/...") and
@@ -142,6 +148,15 @@ func TestResolveTemplatePaths(t *testing.T) {
142148
rootDir: tmpRoot,
143149
want: []string{filepath.ToSlash(absOutside)},
144150
},
151+
{
152+
// Directory name literally starting with "..". If the
153+
// outside-root check used HasPrefix("..") it would wrongly
154+
// drop this path back to the original input.
155+
name: "sibling dir whose name starts with .. is inside rootDir",
156+
templates: []string{"..templates/controlplane.yaml"},
157+
rootDir: tmpRoot,
158+
want: []string{"..templates/controlplane.yaml"},
159+
},
145160
}
146161

147162
for _, tt := range tests {

pkg/commands/template.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020
"fmt"
2121
"os"
2222
"path/filepath"
23-
"strings"
2423

2524
"github.com/cozystack/talm/pkg/engine"
2625
"github.com/cozystack/talm/pkg/modeline"
@@ -280,7 +279,7 @@ func generateOutput(ctx context.Context, c *client.Client, args []string) (strin
280279
// Normalize the path (remove .. and .)
281280
relPath = filepath.Clean(relPath)
282281
// Check if path goes outside root
283-
if strings.HasPrefix(relPath, "..") {
282+
if isOutsideRoot(relPath) {
284283
// Path goes outside root, try to find file in templates/ relative to root
285284
// This handles cases like "../templates/controlplane.yaml" when file is actually in root/templates/
286285
templateName := filepath.Base(templatePath)
@@ -421,7 +420,7 @@ func resolveEngineTemplatePaths(templateFiles []string, rootDir string) []string
421420
continue
422421
}
423422
relPath = filepath.Clean(relPath)
424-
if strings.HasPrefix(relPath, "..") {
423+
if isOutsideRoot(relPath) {
425424
templateName := filepath.Base(templatePath)
426425
possiblePath := filepath.Join("templates", templateName)
427426
fullPath := filepath.Join(absRootDir, possiblePath)

pkg/commands/template_test.go

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
// Copyright Cozystack 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 commands
16+
17+
import (
18+
"os"
19+
"path/filepath"
20+
"testing"
21+
)
22+
23+
// TestResolveEngineTemplatePaths_DotDotPrefixedDir pins that a
24+
// sibling directory whose name literally starts with ".." (e.g.
25+
// "..templates") is not mistaken for an outside-root path and routed
26+
// through the templates/<basename> fallback — that would silently
27+
// substitute a different file when one exists under templates/.
28+
func TestResolveEngineTemplatePaths_DotDotPrefixedDir(t *testing.T) {
29+
// Resolve symlinks so the rootDir and the post-Chdir cwd share the
30+
// same canonical form — on macOS, t.TempDir lives under /var/...
31+
// but os.Getwd returns the realpath /private/var/... which throws
32+
// off the Rel computation.
33+
rootDir, err := filepath.EvalSymlinks(t.TempDir())
34+
if err != nil {
35+
t.Fatalf("eval symlinks: %v", err)
36+
}
37+
// Seed both ..templates/controlplane.yaml (the real target) and
38+
// templates/controlplane.yaml (a decoy the buggy fallback would
39+
// have picked instead).
40+
if err := os.MkdirAll(filepath.Join(rootDir, "..templates"), 0o755); err != nil {
41+
t.Fatalf("mkdir ..templates: %v", err)
42+
}
43+
if err := os.WriteFile(filepath.Join(rootDir, "..templates", "controlplane.yaml"), []byte("real"), 0o600); err != nil {
44+
t.Fatalf("seed ..templates/controlplane.yaml: %v", err)
45+
}
46+
if err := os.MkdirAll(filepath.Join(rootDir, "templates"), 0o755); err != nil {
47+
t.Fatalf("mkdir templates: %v", err)
48+
}
49+
if err := os.WriteFile(filepath.Join(rootDir, "templates", "controlplane.yaml"), []byte("decoy"), 0o600); err != nil {
50+
t.Fatalf("seed templates/controlplane.yaml: %v", err)
51+
}
52+
53+
origCwd, err := os.Getwd()
54+
if err != nil {
55+
t.Fatalf("getwd: %v", err)
56+
}
57+
if err := os.Chdir(rootDir); err != nil {
58+
t.Fatalf("chdir: %v", err)
59+
}
60+
t.Cleanup(func() { _ = os.Chdir(origCwd) })
61+
62+
got := resolveEngineTemplatePaths([]string{"..templates/controlplane.yaml"}, rootDir)
63+
if len(got) != 1 {
64+
t.Fatalf("len = %d, want 1", len(got))
65+
}
66+
if got[0] != "..templates/controlplane.yaml" {
67+
t.Errorf("got %q, want %q (the ..templates dir was misclassified as outside-root and routed through the basename fallback)", got[0], "..templates/controlplane.yaml")
68+
}
69+
}

0 commit comments

Comments
 (0)