diff --git a/bundle/bundle_test.go b/bundle/bundle_test.go index da89463b..b34bb763 100644 --- a/bundle/bundle_test.go +++ b/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 diff --git a/bundle/import.go b/bundle/import.go index af380cc0..bae7c5fd 100644 --- a/bundle/import.go +++ b/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 } } }