Skip to content

Commit 17134de

Browse files
Copilotintel352
andauthored
bundle.Import: skip dot entries, use filepath.Abs for destDir containment check (#345)
* Initial plan * Fix bundle.Import: skip dot entries, use filepath.Abs for destDir Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
1 parent 1275356 commit 17134de

2 files changed

Lines changed: 93 additions & 3 deletions

File tree

bundle/bundle_test.go

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,92 @@ func TestImportPathTraversal(t *testing.T) {
180180
}
181181
}
182182

183+
func TestImportDotDirectoryEntry(t *testing.T) {
184+
// Tarballs created with "tar -czf bundle.tar.gz ." include a top-level "."
185+
// directory entry. Import should skip it gracefully instead of failing.
186+
var buf bytes.Buffer
187+
gw := gzip.NewWriter(&buf)
188+
tw := tar.NewWriter(gw)
189+
190+
// Write a "." directory entry (as created by tar when archiving a directory itself)
191+
_ = tw.WriteHeader(&tar.Header{
192+
Typeflag: tar.TypeDir,
193+
Name: ".",
194+
Mode: 0755,
195+
})
196+
197+
manifestData, _ := json.Marshal(Manifest{
198+
Version: "1.0",
199+
Name: "dot-test",
200+
Files: []string{"workflow.yaml"},
201+
})
202+
_ = writeToTar(tw, "manifest.json", manifestData)
203+
_ = writeToTar(tw, "workflow.yaml", []byte("modules: []\nworkflows: {}\ntriggers: {}"))
204+
205+
tw.Close()
206+
gw.Close()
207+
208+
destDir := t.TempDir()
209+
manifest, workflowPath, err := Import(&buf, destDir)
210+
if err != nil {
211+
t.Fatalf("Import failed on tarball with dot entry: %v", err)
212+
}
213+
if manifest.Name != "dot-test" {
214+
t.Errorf("manifest name = %q, want %q", manifest.Name, "dot-test")
215+
}
216+
if workflowPath == "" {
217+
t.Error("workflowPath is empty")
218+
}
219+
}
220+
221+
func TestImportRelativeDestDir(t *testing.T) {
222+
// Import must work correctly even when destDir is a relative path.
223+
var buf bytes.Buffer
224+
gw := gzip.NewWriter(&buf)
225+
tw := tar.NewWriter(gw)
226+
227+
manifestData, _ := json.Marshal(Manifest{
228+
Version: "1.0",
229+
Name: "rel-test",
230+
Files: []string{"workflow.yaml"},
231+
})
232+
_ = writeToTar(tw, "manifest.json", manifestData)
233+
_ = writeToTar(tw, "workflow.yaml", []byte("modules: []\nworkflows: {}\ntriggers: {}"))
234+
235+
tw.Close()
236+
gw.Close()
237+
238+
// Use a temp dir but pass it as a relative path from its parent.
239+
abs := t.TempDir()
240+
parent := filepath.Dir(abs)
241+
rel := filepath.Join(filepath.Base(parent), filepath.Base(abs))
242+
243+
// Change working directory temporarily to the grandparent so rel is valid.
244+
orig, err := os.Getwd()
245+
if err != nil {
246+
t.Fatal(err)
247+
}
248+
if err := os.Chdir(filepath.Dir(parent)); err != nil {
249+
t.Fatal(err)
250+
}
251+
defer func() { _ = os.Chdir(orig) }()
252+
253+
manifest, workflowPath, err := Import(&buf, rel)
254+
if err != nil {
255+
t.Fatalf("Import failed with relative destDir: %v", err)
256+
}
257+
if manifest.Name != "rel-test" {
258+
t.Errorf("manifest name = %q, want %q", manifest.Name, "rel-test")
259+
}
260+
if workflowPath == "" {
261+
t.Error("workflowPath is empty")
262+
}
263+
// workflowPath must be absolute
264+
if !filepath.IsAbs(workflowPath) {
265+
t.Errorf("workflowPath is not absolute: %s", workflowPath)
266+
}
267+
}
268+
183269
func TestImportFileSizeLimit(t *testing.T) {
184270
// Build a tar.gz with a file exceeding MaxFileSize
185271
var buf bytes.Buffer

bundle/import.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,11 @@ func Import(r io.Reader, destDir string) (*Manifest, string, error) {
2424
if err := os.MkdirAll(destDir, 0750); err != nil {
2525
return nil, "", fmt.Errorf("create dest dir: %w", err)
2626
}
27-
// Normalise the destination directory once for safe prefix checks.
28-
absDestDir := filepath.Clean(destDir)
27+
// Resolve the destination directory to an absolute path once for safe prefix checks.
28+
absDestDir, err := filepath.Abs(destDir)
29+
if err != nil {
30+
return nil, "", fmt.Errorf("resolve dest dir: %w", err)
31+
}
2932

3033
gr, err := gzip.NewReader(r)
3134
if err != nil {
@@ -50,7 +53,8 @@ func Import(r io.Reader, destDir string) (*Manifest, string, error) {
5053
// Path traversal protection
5154
clean := filepath.Clean(hdr.Name)
5255
if clean == "." {
53-
return nil, "", fmt.Errorf("invalid path in bundle: %s", hdr.Name)
56+
// Top-level "." entries are harmless (common in tarballs created with "tar -czf . ..."); skip them.
57+
continue
5458
}
5559
if strings.HasPrefix(clean, "/") || strings.HasPrefix(clean, `\`) {
5660
return nil, "", fmt.Errorf("invalid absolute path in bundle: %s", hdr.Name)

0 commit comments

Comments
 (0)