From 2ac2bbdf7943a73fbc0cff875e1e0601e6e8810f Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Thu, 4 Jun 2026 00:40:17 +0000 Subject: [PATCH] Align bundle path validation coverage --- internal/bundle/manifest_test.go | 18 ++++++++++++ internal/bundle/validate_test.go | 32 +++++++++++++++++++++ internal/ingest/archive_test.go | 48 ++++++++++++++++++++++++++++++++ internal/storage/path_test.go | 8 ++++++ pkg/bundle/bundle_test.go | 48 ++++++++++++++++++++++++++------ pkg/bundle/path.go | 3 +- 6 files changed, 146 insertions(+), 11 deletions(-) diff --git a/internal/bundle/manifest_test.go b/internal/bundle/manifest_test.go index 16d8fa9..5945277 100644 --- a/internal/bundle/manifest_test.go +++ b/internal/bundle/manifest_test.go @@ -70,11 +70,19 @@ func TestParseManifestRejectsInvalidDigestFormat(t *testing.T) { func TestParseManifestRejectsUnsafeFilePaths(t *testing.T) { tests := []string{ + `"path": ""`, + `"path": "."`, + `"path": "./report.md"`, `"path": "../report.md"`, `"path": "/report.md"`, `"path": "nested/../report.md"`, + `"path": "nested/./report.md"`, + `"path": "nested//report.md"`, + `"path": "nested\\report.md"`, `"path": "manifest.json"`, + `"path": "nested/manifest.json"`, `"path": "` + storage.StateFileName + `"`, + `"path": "nested/` + storage.StateFileName + `"`, } for _, replacement := range tests { t.Run(replacement, func(t *testing.T) { @@ -128,6 +136,16 @@ func TestValidateManifestRejectsInvalidManifest(t *testing.T) { manifest.Digest = BundleDigest(manifest.Files) return manifest }, + "nested manifest path": func(manifest Manifest) Manifest { + manifest.Files[0].Path = "nested/manifest.json" + manifest.Digest = BundleDigest(manifest.Files) + return manifest + }, + "nested state path": func(manifest Manifest) Manifest { + manifest.Files[0].Path = "nested/" + storage.StateFileName + manifest.Digest = BundleDigest(manifest.Files) + return manifest + }, "duplicate path": func(manifest Manifest) Manifest { manifest.Files[1].Path = manifest.Files[0].Path manifest.Digest = BundleDigest(manifest.Files) diff --git a/internal/bundle/validate_test.go b/internal/bundle/validate_test.go index 1454aec..edb98a5 100644 --- a/internal/bundle/validate_test.go +++ b/internal/bundle/validate_test.go @@ -65,6 +65,38 @@ func TestValidateRejectsSymlinkFile(t *testing.T) { assertErrorContains(t, err, "regular file") } +func TestValidateRejectsUnsafeManifestPaths(t *testing.T) { + tests := []string{ + "", + ".", + "./report.md", + "../report.md", + "/report.md", + "nested/../report.md", + "nested/./report.md", + "nested//report.md", + `nested\report.md`, + ManifestName, + storage.StateFileName, + "nested/" + ManifestName, + "nested/" + storage.StateFileName, + } + for _, path := range tests { + t.Run(path, func(t *testing.T) { + backend := validFakeBundle(t) + manifest := validFixtureManifest(t) + manifest.Files[0].Path = path + manifest.Digest = BundleDigest(manifest.Files) + writeManifest(t, backend, manifest) + + _, err := Validate(context.Background(), backend, "") + if err == nil { + t.Fatal("Validate() error = nil, want unsafe path error") + } + }) + } +} + func validFakeBundle(t *testing.T) *fake.Backend { t.Helper() backend := fake.New() diff --git a/internal/ingest/archive_test.go b/internal/ingest/archive_test.go index c827762..e771fe8 100644 --- a/internal/ingest/archive_test.go +++ b/internal/ingest/archive_test.go @@ -5,6 +5,7 @@ import ( "bytes" "compress/gzip" "context" + "encoding/json" "errors" "io/fs" "os" @@ -113,9 +114,19 @@ func TestStageArchiveRejectsUnsafeEntries(t *testing.T) { "path traversal": { fileEntry("../report.md", "report"), }, + "dot path": { + fileEntry("./report.md", "report"), + }, + "dot segment": { + fileEntry("nested/./report.md", "report"), + }, "backslash path": { fileEntry(`nested\report.md`, "report"), }, + "duplicate file": { + fileEntry("report.md", "report"), + fileEntry("report.md", "report"), + }, "symlink": { {name: "link.md", typeflag: tar.TypeSymlink, linkname: "report.md"}, }, @@ -125,6 +136,12 @@ func TestStageArchiveRejectsUnsafeEntries(t *testing.T) { "device": { {name: "device", typeflag: tar.TypeChar}, }, + "fifo": { + {name: "socket", typeflag: tar.TypeFifo}, + }, + "socket": { + {name: "socket", typeflag: 'S'}, + }, } for name, entries := range tests { t.Run(name, func(t *testing.T) { @@ -146,6 +163,14 @@ func TestStageArchiveRejectsBundleValidationFailures(t *testing.T) { fileEntry("nested/manifest.json", "{}"), fileEntry("report.md", "report"), }, + "listed nested manifest": { + fileEntry("manifest.json", uncheckedManifestJSON(t, manifestFor("reports.listed.nested", fileSpec{path: "nested/manifest.json", body: "{}"}))), + fileEntry("nested/manifest.json", "{}"), + }, + "listed state file": { + fileEntry("manifest.json", uncheckedManifestJSON(t, manifestFor("reports.listed.state", fileSpec{path: ".distributor.json", body: "{}"}))), + fileEntry(".distributor.json", "{}"), + }, "missing listed file": { fileEntry("manifest.json", manifestJSON(t, manifestFor("reports.missing", fileSpec{path: "missing.md", body: "missing"}))), }, @@ -168,6 +193,20 @@ func TestStageArchiveRejectsBundleValidationFailures(t *testing.T) { } } +func TestStageArchiveAcceptsSafeDirectories(t *testing.T) { + archive := makeArchive(t, false, + tarEntry{name: "nested", typeflag: tar.TypeDir}, + tarEntry{name: "nested/assets", typeflag: tar.TypeDir}, + fileEntry("manifest.json", manifestJSON(t, manifestFor("reports.directories", fileSpec{path: "nested/assets/report.md", body: "report"}))), + fileEntry("nested/assets/report.md", "report"), + ) + staged := stageArchive(t, archive, ContentTypeTar) + + if got := readFile(t, staged.Root, "nested/assets/report.md"); got != "report" { + t.Fatalf("report = %q", got) + } +} + func TestStageArchiveCleansUpFailedExtraction(t *testing.T) { stagingPath := filepath.Join(t.TempDir(), "staging") archive := makeArchive(t, false, fileEntry("../report.md", "report")) @@ -355,6 +394,15 @@ func manifestJSON(t *testing.T, manifest sourcebundle.Manifest) string { return string(data) } +func uncheckedManifestJSON(t *testing.T, manifest sourcebundle.Manifest) string { + t.Helper() + data, err := json.MarshalIndent(manifest, "", " ") + if err != nil { + t.Fatalf("MarshalIndent() error = %v", err) + } + return string(append(data, '\n')) +} + func writeFile(t *testing.T, root, relative, body string) { t.Helper() fullPath := filepath.Join(root, filepath.FromSlash(relative)) diff --git a/internal/storage/path_test.go b/internal/storage/path_test.go index 5c43846..4767b57 100644 --- a/internal/storage/path_test.go +++ b/internal/storage/path_test.go @@ -12,6 +12,10 @@ func TestValidatePath(t *testing.T) { "report.md", "daily/report.md", "a-b_1.2/report.html", + "manifest.json", + StateFileName, + "nested/manifest.json", + "nested/" + StateFileName, } for _, path := range valid { t.Run("valid "+path, func(t *testing.T) { @@ -23,10 +27,14 @@ func TestValidatePath(t *testing.T) { invalid := []string{ "", + ".", + "./report.md", "/absolute", "../outside", "nested/../outside", + "nested/.", "nested/./file", + "nested/", "nested//file", `nested\file`, } diff --git a/pkg/bundle/bundle_test.go b/pkg/bundle/bundle_test.go index 15b1628..eb1fca0 100644 --- a/pkg/bundle/bundle_test.go +++ b/pkg/bundle/bundle_test.go @@ -90,17 +90,33 @@ func TestBuildManifestRequiresOneFileMode(t *testing.T) { } } -func TestBuildManifestRejectsUnsafePath(t *testing.T) { +func TestBuildManifestRejectsUnsafeExplicitPaths(t *testing.T) { root := t.TempDir() writeFile(t, root, "report.txt", "report") - _, err := BuildManifest(BuildOptions{ - Root: root, - ID: "reports.unsafe", - Files: []string{"../report.txt"}, - }) - if err == nil { - t.Fatal("BuildManifest() error = nil, want unsafe path error") + tests := []string{ + "", + "../report.txt", + "/report.txt", + "nested/../report.txt", + "nested/./report.txt", + `nested\report.txt`, + ManifestName, + distributorStateName, + "nested/" + ManifestName, + "nested/" + distributorStateName, + } + for _, sourcePath := range tests { + t.Run(sourcePath, func(t *testing.T) { + _, err := BuildManifest(BuildOptions{ + Root: root, + ID: "reports.unsafe", + Files: []string{sourcePath}, + }) + if err == nil { + t.Fatal("BuildManifest() error = nil, want unsafe path error") + } + }) } } @@ -330,7 +346,21 @@ func TestValidateSourcePath(t *testing.T) { t.Fatalf("ValidateSourcePath(%q) error = %v", path, err) } } - invalid := []string{"", "../report.md", "/report.md", "nested/../report.md", `nested\report.md`, ManifestName, distributorStateName} + invalid := []string{ + "", + ".", + "./report.md", + "../report.md", + "/report.md", + "nested/../report.md", + "nested/./report.md", + "nested//report.md", + `nested\report.md`, + ManifestName, + distributorStateName, + "nested/" + ManifestName, + "nested/" + distributorStateName, + } for _, path := range invalid { if err := ValidateSourcePath(path); err == nil { t.Fatalf("ValidateSourcePath(%q) error = nil, want error", path) diff --git a/pkg/bundle/path.go b/pkg/bundle/path.go index cabe829..10630a0 100644 --- a/pkg/bundle/path.go +++ b/pkg/bundle/path.go @@ -23,8 +23,7 @@ func ValidateSourcePath(value string) error { return fmt.Errorf("source path %q must be a clean relative slash-separated path", value) } } - switch value { - case ManifestName, distributorStateName: + if path.Base(value) == ManifestName || path.Base(value) == distributorStateName { return fmt.Errorf("%q is reserved", value) } return nil