Potential fix for code scanning alert no. 65: Uncontrolled data used in path expression

bundle/bundle_test.go

@@ -180,6 +180,92 @@ func TestImportPathTraversal(t *testing.T) {
 	}
 }
 
+func TestImportDotDirectoryEntry(t *testing.T) {
+	// Tarballs created with "tar -czf bundle.tar.gz ." include a top-level "."
+	// directory entry. Import should skip it gracefully instead of failing.
+	var buf bytes.Buffer
+	gw := gzip.NewWriter(&buf)
+	tw := tar.NewWriter(gw)
+
+	// Write a "." directory entry (as created by tar when archiving a directory itself)
+	_ = tw.WriteHeader(&tar.Header{
+		Typeflag: tar.TypeDir,
+		Name:     ".",
+		Mode:     0755,
+	})
+
+	manifestData, _ := json.Marshal(Manifest{
+		Version: "1.0",
+		Name:    "dot-test",
+		Files:   []string{"workflow.yaml"},
+	})
+	_ = writeToTar(tw, "manifest.json", manifestData)
+	_ = writeToTar(tw, "workflow.yaml", []byte("modules: []\nworkflows: {}\ntriggers: {}"))
+
+	tw.Close()
+	gw.Close()
+
+	destDir := t.TempDir()
+	manifest, workflowPath, err := Import(&buf, destDir)
+	if err != nil {
+		t.Fatalf("Import failed on tarball with dot entry: %v", err)
+	}
+	if manifest.Name != "dot-test" {
+		t.Errorf("manifest name = %q, want %q", manifest.Name, "dot-test")
+	}
+	if workflowPath == "" {
+		t.Error("workflowPath is empty")
+	}
+}
+
+func TestImportRelativeDestDir(t *testing.T) {
+	// Import must work correctly even when destDir is a relative path.
+	var buf bytes.Buffer
+	gw := gzip.NewWriter(&buf)
+	tw := tar.NewWriter(gw)
+
+	manifestData, _ := json.Marshal(Manifest{
+		Version: "1.0",
+		Name:    "rel-test",
+		Files:   []string{"workflow.yaml"},
+	})
+	_ = writeToTar(tw, "manifest.json", manifestData)
+	_ = writeToTar(tw, "workflow.yaml", []byte("modules: []\nworkflows: {}\ntriggers: {}"))
+
+	tw.Close()
+	gw.Close()
+
+	// Use a temp dir but pass it as a relative path from its parent.
+	abs := t.TempDir()
+	parent := filepath.Dir(abs)
+	rel := filepath.Join(filepath.Base(parent), filepath.Base(abs))
+
+	// Change working directory temporarily to the grandparent so rel is valid.
+	orig, err := os.Getwd()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Chdir(filepath.Dir(parent)); err != nil {
+		t.Fatal(err)
+	}
+	defer func() { _ = os.Chdir(orig) }()
+
+	manifest, workflowPath, err := Import(&buf, rel)
+	if err != nil {
+		t.Fatalf("Import failed with relative destDir: %v", err)
+	}
+	if manifest.Name != "rel-test" {
+		t.Errorf("manifest name = %q, want %q", manifest.Name, "rel-test")
+	}
+	if workflowPath == "" {
+		t.Error("workflowPath is empty")
+	}
+	// workflowPath must be absolute
+	if !filepath.IsAbs(workflowPath) {
+		t.Errorf("workflowPath is not absolute: %s", workflowPath)
+	}
+}
+
 func TestImportFileSizeLimit(t *testing.T) {
 	// Build a tar.gz with a file exceeding MaxFileSize
 	var buf bytes.Buffer

bundle/import.go

@@ -24,6 +24,11 @@ func Import(r io.Reader, destDir string) (*Manifest, string, error) {
 	if err := os.MkdirAll(destDir, 0750); err != nil {
 		return nil, "", fmt.Errorf("create dest dir: %w", err)
 	}
+	// Resolve the destination directory to an absolute path once for safe prefix checks.
+	absDestDir, err := filepath.Abs(destDir)
+	if err != nil {
+		return nil, "", fmt.Errorf("resolve dest dir: %w", err)
+	}
 
 	gr, err := gzip.NewReader(r)
 	if err != nil {
@@ -47,8 +52,16 @@ func Import(r io.Reader, destDir string) (*Manifest, string, error) {
 
 		// Path traversal protection
 		clean := filepath.Clean(hdr.Name)
-		if strings.HasPrefix(clean, "..") || strings.HasPrefix(clean, "/") {
-			return nil, "", fmt.Errorf("invalid path in bundle: %s", hdr.Name)
+		if clean == "." {
+			// Top-level "." entries are harmless (common in tarballs created with "tar -czf . ..."); skip them.
+			continue
+		}
+		if strings.HasPrefix(clean, "/") || strings.HasPrefix(clean, `\`) {
+			return nil, "", fmt.Errorf("invalid absolute path in bundle: %s", hdr.Name)
+		}
+		// Reject any parent directory components after cleaning.
+		if strings.Contains(clean, ".."+string(os.PathSeparator)) || clean == ".." {
+			return nil, "", fmt.Errorf("invalid path in bundle (parent directory reference): %s", hdr.Name)
 		}
 
 		// Size checks
@@ -60,16 +73,21 @@ func Import(r io.Reader, destDir string) (*Manifest, string, error) {
 			return nil, "", fmt.Errorf("bundle exceeds max total size (%d)", MaxBundleSize)
 		}
 
-		destPath := filepath.Join(destDir, clean)
+		destPath := filepath.Join(absDestDir, clean)
+		absDestPath := filepath.Clean(destPath)
+		// Ensure the final destination path is still within the destination directory.
+		if absDestPath != absDestDir && !strings.HasPrefix(absDestPath, absDestDir+string(os.PathSeparator)) {
+			return nil, "", fmt.Errorf("invalid path in bundle (outside destination): %s", hdr.Name)
+		}
 
 		switch hdr.Typeflag {
 		case tar.TypeDir:
-			if err := os.MkdirAll(destPath, 0750); err != nil {
+			if err := os.MkdirAll(absDestPath, 0750); err != nil {
 				return nil, "", fmt.Errorf("create dir %s: %w", clean, err)
 			}
 		case tar.TypeReg:
 			// Ensure parent directory exists
-			if err := os.MkdirAll(filepath.Dir(destPath), 0750); err != nil {
+			if err := os.MkdirAll(filepath.Dir(absDestPath), 0750); err != nil {
 				return nil, "", fmt.Errorf("create parent dir for %s: %w", clean, err)
 			}
 
@@ -81,7 +99,7 @@ func Import(r io.Reader, destDir string) (*Manifest, string, error) {
 				return nil, "", fmt.Errorf("file %s exceeds max size", clean)
 			}
 
-			if err := os.WriteFile(destPath, data, 0600); err != nil {
+			if err := os.WriteFile(absDestPath, data, 0600); err != nil {
 				return nil, "", fmt.Errorf("write %s: %w", clean, err)
 			}
 
@@ -95,7 +113,7 @@ func Import(r io.Reader, destDir string) (*Manifest, string, error) {
 			}
 
 			if clean == "workflow.yaml" {
-				workflowPath = destPath
+				workflowPath = absDestPath
 			}
 		}
 	}