From fbcb43a74bbf163fea507d0c0a87d6879f8ad4c6 Mon Sep 17 00:00:00 2001 From: "Jeromy Statia (from Dev Box)" Date: Mon, 21 Jul 2025 16:13:39 -0700 Subject: [PATCH 1/3] Add filename annotation post-processing to LoadSBOMFromFile and LoadArtifactFromFile - LoadSBOMFromFile and LoadArtifactFromFile now check if the descriptor has a title annotation - If no title annotation exists, they add one using filepath.Base(filename) - This ensures only the filename (not the full path) is used in the annotation - LoadSBOMFromReader and LoadArtifactFromReader remain unchanged (no annotations added) - Added comprehensive tests to verify the behavior - Tests ensure annotations are only added by *FromFile functions, not *FromReader functions - Maintains backward compatibility and doesn't overwrite existing annotations --- pkg/artifact.go | 17 ++++++++- pkg/artifact_test.go | 84 ++++++++++++++++++++++++++++++++++++++++++++ pkg/sbom.go | 18 +++++++++- pkg/sbom_test.go | 77 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 194 insertions(+), 2 deletions(-) diff --git a/pkg/artifact.go b/pkg/artifact.go index 7316d0a..8f29b45 100644 --- a/pkg/artifact.go +++ b/pkg/artifact.go @@ -4,6 +4,7 @@ import ( "fmt" "io" "os" + "path/filepath" ocispec "github.com/opencontainers/image-spec/specs-go/v1" "oras.land/oras-go/v2/content" @@ -15,7 +16,21 @@ func LoadArtifactFromFile(filename string, mediaType string) (*ocispec.Descripto return nil, nil, fmt.Errorf("error loading artifact from file: %w", err) } - return LoadArtifactFromReader(file, mediaType) + desc, artifactBytes, err := LoadArtifactFromReader(file, mediaType) + if err != nil { + return nil, nil, err + } + + // Check if the descriptor already has a title annotation, if not, add it + if desc.Annotations == nil || desc.Annotations[ocispec.AnnotationTitle] == "" { + if desc.Annotations == nil { + desc.Annotations = make(map[string]string) + } + // Use only the base filename, not the full path + desc.Annotations[ocispec.AnnotationTitle] = filepath.Base(filename) + } + + return desc, artifactBytes, nil } func LoadArtifactFromReader(reader io.ReadCloser, mediaType string) (*ocispec.Descriptor, []byte, error) { diff --git a/pkg/artifact_test.go b/pkg/artifact_test.go index 649c8a0..fecc50a 100644 --- a/pkg/artifact_test.go +++ b/pkg/artifact_test.go @@ -4,6 +4,8 @@ import ( "bytes" "io" "testing" + + ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) func TestLoadArtifactFromFile(t *testing.T) { @@ -60,3 +62,85 @@ func TestLoadArtifactFromReader(t *testing.T) { t.Errorf("expected desc.Digest to be 'sha256:40b61fe1b15af0a4d5402735b26343e8cf8a045f4d81710e6108a21d91eaf366', got: %s", desc.Digest.String()) } } + +func TestLoadArtifactFromFile_AddsFilenameAnnotation(t *testing.T) { + // Define the path to the test file + filePath := "../examples/artifact.example.json" + mediaType := "application/json" + expectedFilename := "artifact.example.json" // Only the base filename, not the full path + + // Call the function with the test file path and media type + desc, _, err := LoadArtifactFromFile(filePath, mediaType) + + // Check that there was no error + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + // Check that the artifact filename annotation is set with just the filename + if desc.Annotations == nil { + t.Fatalf("expected annotations to be set, got nil") + } + + title, exists := desc.Annotations[ocispec.AnnotationTitle] + if !exists { + t.Errorf("expected annotation %s to exist", ocispec.AnnotationTitle) + } + if title != expectedFilename { + t.Errorf("expected annotation %s to be '%s', got: %s", ocispec.AnnotationTitle, expectedFilename, title) + } +} + +func TestLoadArtifactFromReader_NoAnnotations(t *testing.T) { + // Test that LoadArtifactFromReader doesn't add annotations + testData := []byte(`{"test": "data"}`) + mediaType := "application/json" + + // Create a test reader with the test data + reader := io.NopCloser(bytes.NewReader(testData)) + + // Call the function with the test reader and media type + desc, _, err := LoadArtifactFromReader(reader, mediaType) + + // Check that there was no error + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + // Check that no annotations are set since LoadArtifactFromReader doesn't add title annotations + if desc.Annotations != nil { + if _, exists := desc.Annotations[ocispec.AnnotationTitle]; exists { + t.Errorf("expected no %s annotation from LoadArtifactFromReader, but it was set", ocispec.AnnotationTitle) + } + } +} + +func TestLoadArtifactFromFile_FilenameExtractionFromPath(t *testing.T) { + // Test that the filename is correctly extracted from various path formats + // Since we can't easily test different paths to the same file, we just verify + // that the existing test file produces the expected filename + filePath := "../examples/artifact.example.json" + expectedFilename := "artifact.example.json" + mediaType := "application/json" + + // Call the function + desc, _, err := LoadArtifactFromFile(filePath, mediaType) + + // Check that there was no error + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + // Verify that only the filename (not the path) is in the annotation + if desc.Annotations == nil { + t.Fatalf("expected annotations to be set, got nil") + } + + title, exists := desc.Annotations[ocispec.AnnotationTitle] + if !exists { + t.Errorf("expected annotation %s to exist", ocispec.AnnotationTitle) + } + if title != expectedFilename { + t.Errorf("expected annotation %s to be '%s', got: %s", ocispec.AnnotationTitle, expectedFilename, title) + } +} diff --git a/pkg/sbom.go b/pkg/sbom.go index d25f89d..ac5e760 100644 --- a/pkg/sbom.go +++ b/pkg/sbom.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "os" + "path/filepath" "strings" ocispec "github.com/opencontainers/image-spec/specs-go/v1" @@ -33,6 +34,7 @@ type SPDXDocument struct { // LoadSBOMFromFile opens a file given by filename, reads its contents, and loads it into an SPDX document. // It also calculates the file size and generates an OCI descriptor for the file. // It returns the loaded SPDX document, the OCI descriptor, and any error encountered. +// If the descriptor doesn't have a title annotation, it will be added using the base filename. func LoadSBOMFromFile(filename string, strict bool) (*SPDXDocument, *ocispec.Descriptor, []byte, error) { file, err := os.Open(filename) if err != nil { @@ -40,7 +42,21 @@ func LoadSBOMFromFile(filename string, strict bool) (*SPDXDocument, *ocispec.Des } defer file.Close() - return LoadSBOMFromReader(file, strict) + doc, desc, sbomBytes, err := LoadSBOMFromReader(file, strict) + if err != nil { + return nil, nil, nil, err + } + + // Check if the descriptor already has a title annotation, if not, add it + if desc.Annotations == nil || desc.Annotations[ocispec.AnnotationTitle] == "" { + if desc.Annotations == nil { + desc.Annotations = make(map[string]string) + } + // Use only the base filename, not the full path + desc.Annotations[ocispec.AnnotationTitle] = filepath.Base(filename) + } + + return doc, desc, sbomBytes, nil } // LoadSBOMFromReader reads an SPDX document from an io.ReadCloser, generates an OCI descriptor for the document, diff --git a/pkg/sbom_test.go b/pkg/sbom_test.go index d20e6c2..6d15163 100644 --- a/pkg/sbom_test.go +++ b/pkg/sbom_test.go @@ -5,6 +5,8 @@ import ( "io" "strings" "testing" + + ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) const spdxStr string = `{ @@ -167,3 +169,78 @@ func TestGetAnnotations(t *testing.T) { t.Errorf("expected creators annotation to be 'Tool: SPDX-Java-Tools-v2.1.20, Organization: Source Auditor Inc.', got: %v", annotations[OCI_ANNOTATION_CREATORS]) } } + +func TestLoadSBOMFromFile_AddsFilenameAnnotation(t *testing.T) { + // Define the path to the test file + filePath := "../examples/SPDXJSONExample-v2.3.spdx.json" + expectedFilename := "SPDXJSONExample-v2.3.spdx.json" // Only the base filename, not the full path + + // Call the function with the test file path + _, desc, _, err := LoadSBOMFromFile(filePath, true) + + // Check that there was no error + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + // Check that the artifact filename annotation is set with just the filename + if desc.Annotations == nil { + t.Fatalf("expected annotations to be set, got nil") + } + + title, exists := desc.Annotations[ocispec.AnnotationTitle] + if !exists { + t.Errorf("expected annotation %s to exist", ocispec.AnnotationTitle) + } + if title != expectedFilename { + t.Errorf("expected annotation %s to be '%s', got: %s", ocispec.AnnotationTitle, expectedFilename, title) + } +} + +func TestLoadSBOMFromReader_NoAnnotations(t *testing.T) { + // Test that LoadSBOMFromReader doesn't add annotations + // Create a test reader with the SPDX JSON data + reader := io.NopCloser(strings.NewReader(spdxStr)) + + // Call the function with the test reader + _, desc, _, err := LoadSBOMFromReader(reader, true) + + // Check that there was no error + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + // Check that no title annotation is set since LoadSBOMFromReader doesn't add title annotations + if desc.Annotations != nil { + if _, exists := desc.Annotations[ocispec.AnnotationTitle]; exists { + t.Errorf("expected no %s annotation from LoadSBOMFromReader, but it was set", ocispec.AnnotationTitle) + } + } +} + +func TestLoadSBOMFromFile_PreservesExistingAnnotation(t *testing.T) { + // This test simulates the case where a descriptor already has a title annotation + // Since we can't easily mock this with the current implementation, this serves as documentation + // of the intended behavior: if an annotation already exists, it should not be overwritten + + // For now, we just test that the annotation is added when it doesn't exist + filePath := "../examples/SPDXJSONExample-v2.3.spdx.json" + expectedFilename := "SPDXJSONExample-v2.3.spdx.json" + + // Call the function + _, desc, _, err := LoadSBOMFromFile(filePath, true) + + // Check that there was no error + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + // Verify the title annotation is set + title, exists := desc.Annotations[ocispec.AnnotationTitle] + if !exists { + t.Errorf("expected annotation %s to exist", ocispec.AnnotationTitle) + } + if title != expectedFilename { + t.Errorf("expected annotation %s to be '%s', got: %s", ocispec.AnnotationTitle, expectedFilename, title) + } +} From 970cee526bacb5b9379132d295a4a12a396380a5 Mon Sep 17 00:00:00 2001 From: "Jeromy Statia (from Dev Box)" Date: Tue, 22 Jul 2025 08:23:16 -0700 Subject: [PATCH 2/3] Refactor filename annotation handling in LoadArtifactFromFile and LoadSBOMFromFile; add tests for AddFilenameAnnotationIfMissing --- pkg/artifact.go | 22 ++++-- pkg/artifact_test.go | 174 +++++++++++++++++++++++++++++++++++++++++++ pkg/sbom.go | 11 +-- pkg/sbom_test.go | 27 ------- 4 files changed, 190 insertions(+), 44 deletions(-) diff --git a/pkg/artifact.go b/pkg/artifact.go index 8f29b45..8794cba 100644 --- a/pkg/artifact.go +++ b/pkg/artifact.go @@ -21,14 +21,8 @@ func LoadArtifactFromFile(filename string, mediaType string) (*ocispec.Descripto return nil, nil, err } - // Check if the descriptor already has a title annotation, if not, add it - if desc.Annotations == nil || desc.Annotations[ocispec.AnnotationTitle] == "" { - if desc.Annotations == nil { - desc.Annotations = make(map[string]string) - } - // Use only the base filename, not the full path - desc.Annotations[ocispec.AnnotationTitle] = filepath.Base(filename) - } + // Add filename annotation if missing + AddFilenameAnnotationIfMissing(desc, filename) return desc, artifactBytes, nil } @@ -46,3 +40,15 @@ func LoadArtifactFromReader(reader io.ReadCloser, mediaType string) (*ocispec.De return &desc, artifactBytes, nil } + +// AddFilenameAnnotationIfMissing adds a title annotation to the descriptor using the base filename +// if the annotation doesn't already exist or is empty. This function modifies the descriptor in-place. +func AddFilenameAnnotationIfMissing(desc *ocispec.Descriptor, filename string) { + if desc.Annotations == nil || desc.Annotations[ocispec.AnnotationTitle] == "" { + if desc.Annotations == nil { + desc.Annotations = make(map[string]string) + } + // Use only the base filename, not the full path + desc.Annotations[ocispec.AnnotationTitle] = filepath.Base(filename) + } +} diff --git a/pkg/artifact_test.go b/pkg/artifact_test.go index fecc50a..ffd6593 100644 --- a/pkg/artifact_test.go +++ b/pkg/artifact_test.go @@ -144,3 +144,177 @@ func TestLoadArtifactFromFile_FilenameExtractionFromPath(t *testing.T) { t.Errorf("expected annotation %s to be '%s', got: %s", ocispec.AnnotationTitle, expectedFilename, title) } } + +func TestAddFilenameAnnotationIfMissing_AddsAnnotationWhenMissing(t *testing.T) { + // Test adding annotation when descriptor has no annotations + desc := &ocispec.Descriptor{ + MediaType: "application/json", + Size: 100, + Digest: "sha256:abc123", + } + filename := "../path/to/test-file.json" + expectedFilename := "test-file.json" + + AddFilenameAnnotationIfMissing(desc, filename) + + // Check that annotations were created and title was set + if desc.Annotations == nil { + t.Fatalf("expected annotations to be created, got nil") + } + + title, exists := desc.Annotations[ocispec.AnnotationTitle] + if !exists { + t.Errorf("expected annotation %s to exist", ocispec.AnnotationTitle) + } + if title != expectedFilename { + t.Errorf("expected annotation %s to be '%s', got: %s", ocispec.AnnotationTitle, expectedFilename, title) + } +} + +func TestAddFilenameAnnotationIfMissing_AddsAnnotationWhenEmpty(t *testing.T) { + // Test adding annotation when descriptor has empty annotations map + desc := &ocispec.Descriptor{ + MediaType: "application/json", + Size: 100, + Digest: "sha256:abc123", + Annotations: make(map[string]string), + } + filename := "test-file.json" + expectedFilename := "test-file.json" + + AddFilenameAnnotationIfMissing(desc, filename) + + // Check that title annotation was added + title, exists := desc.Annotations[ocispec.AnnotationTitle] + if !exists { + t.Errorf("expected annotation %s to exist", ocispec.AnnotationTitle) + } + if title != expectedFilename { + t.Errorf("expected annotation %s to be '%s', got: %s", ocispec.AnnotationTitle, expectedFilename, title) + } +} + +func TestAddFilenameAnnotationIfMissing_AddsAnnotationWhenTitleEmpty(t *testing.T) { + // Test adding annotation when descriptor has annotations but empty title + desc := &ocispec.Descriptor{ + MediaType: "application/json", + Size: 100, + Digest: "sha256:abc123", + Annotations: map[string]string{ + "other.annotation": "some-value", + ocispec.AnnotationTitle: "", // Empty title + }, + } + filename := "C:\\Windows\\System32\\test-file.json" + expectedFilename := "test-file.json" + + AddFilenameAnnotationIfMissing(desc, filename) + + // Check that title annotation was set + title, exists := desc.Annotations[ocispec.AnnotationTitle] + if !exists { + t.Errorf("expected annotation %s to exist", ocispec.AnnotationTitle) + } + if title != expectedFilename { + t.Errorf("expected annotation %s to be '%s', got: %s", ocispec.AnnotationTitle, expectedFilename, title) + } + + // Check that other annotations are preserved + other, exists := desc.Annotations["other.annotation"] + if !exists || other != "some-value" { + t.Errorf("expected other annotations to be preserved") + } +} + +func TestAddFilenameAnnotationIfMissing_PreservesExistingAnnotation(t *testing.T) { + // Test that existing non-empty title annotation is preserved + existingTitle := "existing-title.json" + desc := &ocispec.Descriptor{ + MediaType: "application/json", + Size: 100, + Digest: "sha256:abc123", + Annotations: map[string]string{ + ocispec.AnnotationTitle: existingTitle, + "other.annotation": "some-value", + }, + } + filename := "new-filename.json" + + AddFilenameAnnotationIfMissing(desc, filename) + + // Check that existing title annotation was preserved + title, exists := desc.Annotations[ocispec.AnnotationTitle] + if !exists { + t.Errorf("expected annotation %s to exist", ocispec.AnnotationTitle) + } + if title != existingTitle { + t.Errorf("expected annotation %s to be preserved as '%s', got: %s", ocispec.AnnotationTitle, existingTitle, title) + } + + // Check that other annotations are preserved + other, exists := desc.Annotations["other.annotation"] + if !exists || other != "some-value" { + t.Errorf("expected other annotations to be preserved") + } +} + +func TestAddFilenameAnnotationIfMissing_HandlesVariousPathFormats(t *testing.T) { + // Test that the function correctly extracts filenames from various path formats + testCases := []struct { + name string + inputPath string + expectedName string + }{ + { + name: "relative path with slash", + inputPath: "../examples/test.json", + expectedName: "test.json", + }, + { + name: "simple filename", + inputPath: "test.json", + expectedName: "test.json", + }, + { + name: "absolute unix path", + inputPath: "/path/to/test.json", + expectedName: "test.json", + }, + { + name: "absolute windows path", + inputPath: "C:\\Windows\\System32\\test.json", + expectedName: "test.json", + }, + { + name: "current directory", + inputPath: "./test.json", + expectedName: "test.json", + }, + { + name: "windows style backslash", + inputPath: "examples\\test.json", + expectedName: "test.json", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + desc := &ocispec.Descriptor{ + MediaType: "application/json", + Size: 100, + Digest: "sha256:abc123", + } + + AddFilenameAnnotationIfMissing(desc, tc.inputPath) + + // Check that the correct filename was extracted + title, exists := desc.Annotations[ocispec.AnnotationTitle] + if !exists { + t.Errorf("expected annotation %s to exist", ocispec.AnnotationTitle) + } + if title != tc.expectedName { + t.Errorf("for path '%s', expected annotation %s to be '%s', got: %s", tc.inputPath, ocispec.AnnotationTitle, tc.expectedName, title) + } + }) + } +} diff --git a/pkg/sbom.go b/pkg/sbom.go index ac5e760..eb8cce9 100644 --- a/pkg/sbom.go +++ b/pkg/sbom.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "os" - "path/filepath" "strings" ocispec "github.com/opencontainers/image-spec/specs-go/v1" @@ -47,14 +46,8 @@ func LoadSBOMFromFile(filename string, strict bool) (*SPDXDocument, *ocispec.Des return nil, nil, nil, err } - // Check if the descriptor already has a title annotation, if not, add it - if desc.Annotations == nil || desc.Annotations[ocispec.AnnotationTitle] == "" { - if desc.Annotations == nil { - desc.Annotations = make(map[string]string) - } - // Use only the base filename, not the full path - desc.Annotations[ocispec.AnnotationTitle] = filepath.Base(filename) - } + // Add filename annotation if missing + AddFilenameAnnotationIfMissing(desc, filename) return doc, desc, sbomBytes, nil } diff --git a/pkg/sbom_test.go b/pkg/sbom_test.go index 6d15163..1109bb3 100644 --- a/pkg/sbom_test.go +++ b/pkg/sbom_test.go @@ -217,30 +217,3 @@ func TestLoadSBOMFromReader_NoAnnotations(t *testing.T) { } } } - -func TestLoadSBOMFromFile_PreservesExistingAnnotation(t *testing.T) { - // This test simulates the case where a descriptor already has a title annotation - // Since we can't easily mock this with the current implementation, this serves as documentation - // of the intended behavior: if an annotation already exists, it should not be overwritten - - // For now, we just test that the annotation is added when it doesn't exist - filePath := "../examples/SPDXJSONExample-v2.3.spdx.json" - expectedFilename := "SPDXJSONExample-v2.3.spdx.json" - - // Call the function - _, desc, _, err := LoadSBOMFromFile(filePath, true) - - // Check that there was no error - if err != nil { - t.Fatalf("expected no error, got: %v", err) - } - - // Verify the title annotation is set - title, exists := desc.Annotations[ocispec.AnnotationTitle] - if !exists { - t.Errorf("expected annotation %s to exist", ocispec.AnnotationTitle) - } - if title != expectedFilename { - t.Errorf("expected annotation %s to be '%s', got: %s", ocispec.AnnotationTitle, expectedFilename, title) - } -} From 85058408b3d6591979131e82af00d1ff7bcbc118 Mon Sep 17 00:00:00 2001 From: "Jeromy Statia (from Dev Box)" Date: Tue, 22 Jul 2025 08:30:21 -0700 Subject: [PATCH 3/3] Improve filename annotation handling in AddFilenameAnnotationIfMissing to support both Unix and Windows path separators --- pkg/artifact.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/artifact.go b/pkg/artifact.go index 8794cba..e2e1885 100644 --- a/pkg/artifact.go +++ b/pkg/artifact.go @@ -5,6 +5,7 @@ import ( "io" "os" "path/filepath" + "strings" ocispec "github.com/opencontainers/image-spec/specs-go/v1" "oras.land/oras-go/v2/content" @@ -49,6 +50,12 @@ func AddFilenameAnnotationIfMissing(desc *ocispec.Descriptor, filename string) { desc.Annotations = make(map[string]string) } // Use only the base filename, not the full path - desc.Annotations[ocispec.AnnotationTitle] = filepath.Base(filename) + // Handle both Unix and Windows path separators regardless of platform + basename := filepath.Base(filename) + // If filepath.Base didn't extract properly (e.g., Windows paths on Unix), try manual extraction + if lastSlash := strings.LastIndexByte(basename, '\\'); lastSlash >= 0 { + basename = basename[lastSlash+1:] + } + desc.Annotations[ocispec.AnnotationTitle] = basename } }