fix: file selection within archives, spdx / cyclonedx file representation

Signed-off-by: Keith Zantow <kzantow@gmail.com>

internal/archive/resolver.go

@@ -273,8 +273,8 @@ func (r *Resolver) fullPath(rooted string) string {
 }
 
 // rootedPath is the tree path a query names, which is either form a path takes: a full path is trimmed to
-// the entry path within this archive, and anything else (a LocalPath) is taken as archive-relative
-// already. The archive's full path with a trailing "!" (what path.Dir gives for an entry at the root)
+// the entry path within this archive, and anything else (a LocalPath, with or without its leading slash)
+// is taken as archive-relative already, since the tree looks a relative path up from its root. The archive's full path with a trailing "!" (what path.Dir gives for an entry at the root)
 // names the root. A full path into another archive in the scan names nothing here and reports false.
 func (r *Resolver) rootedPath(userPath string) (string, bool) {
 	if rest, ok := strings.CutPrefix(userPath, r.archiveCoordinates.FullNestedPath()); ok {
@@ -289,9 +289,6 @@ func (r *Resolver) rootedPath(userPath string) (string, bool) {
 	if r.outside(userPath) {
 		return "", false
 	}
-	if !strings.HasPrefix(userPath, "/") {
-		userPath = "/" + userPath
-	}
 	return path.Clean(userPath), true
 }
 
@@ -420,10 +417,10 @@ func (r *Resolver) FilesByGlob(patterns ...string) ([]file.Location, error) {
 }
 
 // searchByGlob is the index search, except that stereoscope answers "<dir>/*" by looking the directory
-// up by name and the root has none, so "/*" walks the tree instead.
+// up by name and the root has none, so "/*" (or "*") walks the tree instead.
 func (r *Resolver) searchByGlob(pattern string) ([]stereoscopeFile.Resolution, error) {
-	if pattern == "/*" {
-		return r.tree.FilesByGlob(pattern, filetree.FollowBasenameLinks)
+	if strings.TrimPrefix(pattern, "/") == "*" {
+		return r.tree.FilesByGlob("/*", filetree.FollowBasenameLinks)
 	}
 	return r.search.SearchByGlob(pattern, filetree.FollowBasenameLinks)
 }

internal/archive/traversal.go

@@ -38,11 +38,8 @@ func FullPathIn(containerPath, entryPath string) string {
 	if containerPath == "" {
 		return entryPath
 	}
-	if !strings.HasPrefix(entryPath, "/") {
-		entryPath = "/" + entryPath
-	}
 	// cleaning an already clean path allocates nothing
-	return containerPath + file.ArchivePathSeparator + path.Clean(entryPath)[1:]
+	return containerPath + file.ArchivePathSeparator + strings.TrimPrefix(path.Clean(entryPath), "/")
 }
 
 // OwnerContainers are the containers a package owns files within: the container of each of its
@@ -100,6 +97,20 @@ func ContainerPath(c file.Coordinates) string {
 	return c.RealPath
 }
 
+// ContainingArchive returns the coordinates of the archive directly holding the file at c, reporting false
+// for a file on the scanned filesystem.
+func ContainingArchive(c file.Coordinates) (file.Coordinates, bool) {
+	containerPath := ContainerPath(c)
+	if containerPath == "" {
+		return file.Coordinates{}, false
+	}
+	container := file.Coordinates{RealPath: c.RealPath, FileSystemID: c.FileSystemID}
+	if containerPath != c.RealPath {
+		container.FullPath = containerPath
+	}
+	return container, true
+}
+
 // archiveEntries is the "!/"-delimited chain of entry paths after RealPath, reporting false for a file
 // not inside an archive. RealPath is split off first, so a "!/" in it is not taken for a boundary.
 func archiveEntries(c file.Coordinates) (string, bool) {

internal/task/archive_nesting_test.go

@@ -141,7 +141,7 @@ func Test_mixedFamilyNesting_containsEdgesChainAtEveryLevel(t *testing.T) {
 			scanDir := t.TempDir()
 			nested := writeNestedArchive(t, scanDir, nestPlan{families: row.families, leaf: markerLeaf()})
 
-			s := runNesting(t, scanDir, 3, defaultLimits(), markerTask())
+			s := withArchiveContains(t, runNesting(t, scanDir, 3, defaultLimits(), markerTask()))
 
 			// each archive's coordinates as seen in its parent; the outermost sits in the scanned
 			// directory, so it carries no filesystem id of its own
@@ -178,7 +178,7 @@ func Test_mixedFamilyNesting_containsEdgesChainAtEveryLevel(t *testing.T) {
 				"expected a CONTAINS edge from %s to the leaf package", nested.paths[len(nested.paths)-1])
 
 			// with file cataloging on, files inside an archive get an edge from it too
-			s = runNesting(t, scanDir, 3, defaultLimits(), markerTask(), fileMetadataTask())
+			s = withArchiveContains(t, runNesting(t, scanDir, 3, defaultLimits(), markerTask(), fileMetadataTask()))
 			var leafEdge bool
 			for _, rel := range s.Relationships {
 				from, _ := rel.From.(file.Coordinates)
@@ -379,6 +379,13 @@ func runNesting(t *testing.T, scanDir string, depth int, bounds archive.Limits,
 	}
 }
 
+// withArchiveContains runs the task that follows every scan with archive cataloging on.
+func withArchiveContains(t *testing.T, s *sbom.SBOM) *sbom.SBOM {
+	t.Helper()
+	require.NoError(t, NewArchiveContainsTask().Execute(context.Background(), nil, sbomsync.NewBuilder(s)))
+	return s
+}
+
 // markerTask emits one package per "marker.txt" found, so the matrix asserts behavior without
 // depending on any real cataloger's fixture format.
 func markerTask() Task {

internal/task/archive_tasks.go

@@ -32,7 +32,7 @@ const ArchiveCatalogerTaskName = "archive-cataloger"
 
 // NewArchiveCatalogerTask returns a task that extracts every archive it finds, runs the same tasks
 // against the archive's contents as a standalone filesystem, and records
-// CONTAINS relationships from the archive to what was found inside it. It recurses into the archives
+// CONTAINS relationships from the archive to the packages found directly inside it. It recurses into the archives
 // it extracts up to cfg.MaxDepth; the task drops itself from the tasks so that is the only recursion.
 //
 // Each archive is handled as the scan root is: pkgTasks, then the archives nested in it, then fileTasks,
@@ -267,12 +267,14 @@ func (c *archiveCataloger) processArchive(ctx context.Context, parentResolver fi
 
 	// as at the scan root, nested archives are cataloged before this archive's files, so the packages
 	// describing them (located in this archive) select their files here
-	children := c.discoverArchives(extracted)
-	errs := c.catalogCandidates(ctx, extracted, children, depth+1, foundBuilder)
+	errs := c.catalogCandidates(ctx, extracted, c.discoverArchives(extracted), depth+1, foundBuilder)
 
 	started = time.Now()
 	runArchiveTasks(ctx, c.fileTasks, extracted, location.Coordinates, foundBuilder)
-	addContains(location.Coordinates, archivePath, direct, children, found)
+	// the files within it are related once the scan has decided which it describes (see NewArchiveContainsTask)
+	for _, p := range direct {
+		found.Relationships = append(found.Relationships, artifact.Relationship{From: location.Coordinates, To: p, Type: artifact.ContainsRelationship})
+	}
 	mergeArchiveResults(found, builder)
 	c.progress.AtomicStage.Set(humanize.Comma(c.progress.Current()) + " archives (" + archivePath + ")")
 
@@ -440,64 +442,41 @@ func mergeArchiveResults(found *sbom.SBOM, builder sbomsync.Builder) {
 	}
 }
 
-// addContains adds a CONTAINS relationship from the archive to each package found directly in it, each
-// file directly in it that the SBOM describes, and each nested archive, so the hierarchy is navigable
-// whatever file selection is configured.
-//
-// Files directly in the archive are those whose container is the archive's own full path, which leaves
-// out the archive itself (a java package or an unknown recorded at its coordinates would otherwise
-// become a self-loop) and the files of nested archives, contained by those.
-func addContains(archiveCoordinates file.Coordinates, archivePath string, direct []pkg.Package, children []archiveCandidate, found *sbom.SBOM) {
-	contained := file.NewCoordinateSet()
-	for _, child := range children {
-		contained.Add(child.location.Coordinates)
+// NewArchiveContainsTask relates each archive to every file the SBOM describes directly within it. It must
+// run after every other task, so the files within archives are those the scan root's configuration
+// selects (file selection, unknowns, package evidence) and no others.
+func NewArchiveContainsTask() Task {
+	fn := func(_ context.Context, _ file.Resolver, builder sbomsync.Builder) error {
+		var rels []artifact.Relationship
+		builder.(sbomsync.Accessor).ReadFromSBOM(func(s *sbom.SBOM) {
+			rels = archiveContains(s.AllCoordinates())
+		})
+		builder.AddRelationships(rels...)
+		return nil
 	}
-	addInside := func(coordinates file.Coordinates) {
-		if archive.ContainerPath(coordinates) == archivePath {
-			contained.Add(coordinates)
-		}
-	}
-	eachFileArtifactCoordinates(&found.Artifacts, addInside)
-	for _, r := range found.Relationships {
-		if c, ok := r.From.(file.Coordinates); ok {
-			addInside(c)
-		}
-		if c, ok := r.To.(file.Coordinates); ok {
-			addInside(c)
-		}
-	}
-
-	var rels []artifact.Relationship
-	for _, p := range direct {
-		rels = append(rels, artifact.Relationship{From: archiveCoordinates, To: p, Type: artifact.ContainsRelationship})
-	}
-	for _, coordinates := range contained.ToSlice() {
-		rels = append(rels, artifact.Relationship{From: archiveCoordinates, To: coordinates, Type: artifact.ContainsRelationship})
-	}
-	found.Relationships = append(found.Relationships, rels...)
+	return NewTask("archive-contains", fn)
 }
 
-// eachFileArtifactCoordinates calls fn with the coordinates of every file artifact, once per artifact
-// kind describing it.
-func eachFileArtifactCoordinates(a *sbom.Artifacts, fn func(file.Coordinates)) {
-	for c := range a.FileMetadata {
-		fn(c)
-	}
-	for c := range a.FileDigests {
-		fn(c)
-	}
-	for c := range a.FileContents {
-		fn(c)
-	}
-	for c := range a.FileLicenses {
-		fn(c)
-	}
-	for c := range a.Executables {
-		fn(c)
-	}
-	for c := range a.Unknowns {
-		fn(c)
+// archiveContains relates the archive holding each file within one to that file, and each such archive to
+// the archive holding it, so an archive the SBOM does not otherwise describe still links what is within it.
+func archiveContains(coordinates []file.Coordinates) []artifact.Relationship {
+	related := file.NewCoordinateSet(coordinates...)
+	var rels []artifact.Relationship
+	for _, c := range coordinates {
+		for {
+			container, ok := archive.ContainingArchive(c)
+			if !ok {
+				break
+			}
+			rels = append(rels, artifact.Relationship{From: container, To: c, Type: artifact.ContainsRelationship})
+			if related.Contains(container) {
+				break
+			}
+			related.Add(container)
+			c = container
+		}
 	}
+	return rels
 }
 
 func mergeFileArtifacts(dst, src *sbom.Artifacts) {

internal/task/archive_tasks_test.go

@@ -337,6 +337,7 @@ func Test_archiveCataloger_noSelfContainsEdge(t *testing.T) {
 	s := newTestSBOM()
 	s.Artifacts.FileMetadata = map[file.Coordinates]file.Metadata{}
 	_ = tsk.Execute(context.Background(), dirTestResolver{dir: rootDir}, sbomsync.NewBuilder(s))
+	withArchiveContains(t, s)
 
 	require.Contains(t, s.Artifacts.Unknowns, file.NewCoordinates("/x.jar", ""), "the fixture must reproduce the unknown")
 	var contains int
@@ -350,6 +351,37 @@ func Test_archiveCataloger_noSelfContainsEdge(t *testing.T) {
 	assert.Positive(t, contains, "the archive still contains its files")
 }
 
+func Test_NewArchiveContainsTask(t *testing.T) {
+	// a file within an archive is contained only when the SBOM describes it, as a file at the scan root is
+	// listed only then; an executable no package owns is recorded whatever the file selection
+	in := func(fullPath string) file.Coordinates {
+		return file.Coordinates{RealPath: "/outer.tar", FileSystemID: "layer", FullPath: fullPath}
+	}
+	outer := file.NewCoordinates("/outer.tar", "layer")
+	jar := in("/outer.tar!/lib/app.jar")
+	manifest := in("/outer.tar!/lib/app.jar!/META-INF/MANIFEST.MF")
+	unknownExe := in("/outer.tar!/bin/setup.exe")
+
+	s := newTestSBOM()
+	s.Artifacts.FileMetadata = map[file.Coordinates]file.Metadata{manifest: {}, file.NewCoordinates("/loose.txt", "layer"): {}}
+	s.Artifacts.Executables = map[file.Coordinates]file.Executable{in("/outer.tar!/bin/tool"): {}}
+	s.Artifacts.Unknowns = map[file.Coordinates][]string{unknownExe: {"no package identified in executable file"}}
+
+	withArchiveContains(t, s)
+
+	type edge struct{ from, to file.Coordinates }
+	var edges []edge
+	for _, r := range s.Relationships {
+		assert.Equal(t, artifact.ContainsRelationship, r.Type)
+		edges = append(edges, edge{r.From.(file.Coordinates), r.To.(file.Coordinates)})
+	}
+	assert.ElementsMatch(t, []edge{
+		{jar, manifest},
+		{outer, jar}, // the jar is not described itself, but is on the way to what is
+		{outer, unknownExe},
+	}, edges)
+}
+
 func Test_NewArchiveCatalogerTask_dropsItselfFromSubPipeline(t *testing.T) {
 	// a copy of itself in the sub-pipeline would process every nesting level twice
 	cfg := cataloging.DefaultArchiveSearchConfig().WithMaxDepth(2)

internal/task/package_task_factory.go

@@ -3,9 +3,11 @@ package task
 import (
 	"context"
 	"fmt"
+	"path"
 	"strings"
 	"unicode"
 
+	"github.com/anchore/syft/internal/archive"
 	"github.com/anchore/syft/internal/bus"
 	"github.com/anchore/syft/internal/log"
 	"github.com/anchore/syft/internal/relationship"
@@ -236,10 +238,10 @@ func packageFileOwnershipRelationships(p pkg.Package, resolver file.PathResolver
 
 	locations := map[artifact.ID]file.Location{}
 
-	for _, path := range fileOwner.OwnedFiles() {
-		pathRefs, err := resolver.FilesByPath(path)
+	for _, ownedPath := range fileOwner.OwnedFiles() {
+		pathRefs, err := resolver.FilesByPath(ownedPath)
 		if err != nil {
-			return nil, fmt.Errorf("unable to find path for path=%q: %w", path, err)
+			return nil, fmt.Errorf("unable to find path for path=%q: %w", ownedPath, err)
 		}
 
 		if len(pathRefs) == 0 {
@@ -250,6 +252,11 @@ func packageFileOwnershipRelationships(p pkg.Package, resolver file.PathResolver
 		}
 
 		for _, ref := range pathRefs {
+			// an archive looks a relative path up from its root, but an owned path may be relative to
+			// anything (a python RECORD is relative to site-packages), so only an absolute one owns a file in one
+			if archive.ContainerPath(ref.Coordinates) != "" && !path.IsAbs(ownedPath) {
+				continue
+			}
 			if oldRef, ok := locations[ref.ID()]; ok {
 				log.Debugf("found path duplicate of %s", oldRef.FullNestedPath())
 			}

internal/task/package_task_factory_test.go

@@ -1,11 +1,15 @@
 package task
 
 import (
+	"bytes"
+	"context"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/anchore/syft/internal/archive"
+	"github.com/anchore/syft/internal/testutils"
 	"github.com/anchore/syft/syft/artifact"
 	"github.com/anchore/syft/syft/cataloging"
 	"github.com/anchore/syft/syft/cpe"
@@ -281,3 +285,28 @@ func TestApplyComplianceRules_DropAndStub(t *testing.T) {
 	assert.False(t, isCompliant)
 	assert.Equal(t, cataloging.UnknownStubValue, p.Version)
 }
+
+func Test_packageFileOwnershipRelationships_exactPathsWithinAnArchive(t *testing.T) {
+	// a wheel's RECORD is relative to site-packages: looked up from the wheel's root it finds the file, but
+	// only an exact path owns one, as at the scan root
+	data := testutils.Zip(t, map[string]string{"pip/__init__.py": "", "bin/tool": ""})
+	whl, err := archive.Extract(context.Background(), bytes.NewReader(data), file.NewCoordinates("/pip.whl", ""),
+		archive.NewLimiter(archive.Limits{MaxMemoryBytes: 1 << 20}), archive.NewExclusions(nil))
+	require.NoError(t, err)
+	t.Cleanup(whl.Cleanup)
+
+	found, err := whl.FilesByPath("pip/__init__.py")
+	require.NoError(t, err)
+	require.Len(t, found, 1, "a relative path still finds the file")
+
+	p := pkg.Package{Name: "pip", Metadata: pkg.PythonPackage{Files: []pkg.PythonFileRecord{{Path: "pip/__init__.py"}, {Path: "/bin/tool"}}}}
+	p.SetID()
+	rels, err := packageFileOwnershipRelationships(p, whl)
+	require.NoError(t, err)
+
+	var owned []string
+	for _, r := range rels {
+		owned = append(owned, r.To.(file.Coordinates).FullNestedPath())
+	}
+	assert.Equal(t, []string{"/pip.whl!/bin/tool"}, owned)
+}

internal/task/unknowns_tasks.go

@@ -2,6 +2,7 @@ package task
 
 import (
 	"context"
+	"path"
 	"slices"
 	"strings"
 
@@ -106,9 +107,12 @@ func coordinateReferenceLookup(resolver file.Resolver, s *sbom.SBOM) func(coords
 				}
 			}
 			for _, ownedFilePath := range f.OwnedFiles() {
-				for _, container := range archives {
-					// an archive's contents are not in this resolver to follow links in
-					allPackageCoords.Add(archive.OwnedCoordinates(container, ownedFilePath))
+				// an archive's contents are not in this resolver to follow links in, and only an absolute owned
+				// path names a file in one (see packageFileOwnershipRelationships)
+				if path.IsAbs(ownedFilePath) {
+					for _, container := range archives {
+						allPackageCoords.Add(archive.OwnedCoordinates(container, ownedFilePath))
+					}
 				}
 				if !atRoot {
 					continue

internal/task/unknowns_tasks_test.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 
+	"github.com/anchore/syft/syft/artifact"
 	"github.com/anchore/syft/syft/cataloging"
 	"github.com/anchore/syft/syft/file"
 	"github.com/anchore/syft/syft/pkg"
@@ -29,6 +30,57 @@ func Test_unknownsLabeler_archiveReasonsSurvivePackages(t *testing.T) {
 		"a package describing the archive explains a parse failure, not contents left out")
 }
 
+func Test_unknownsLabeler_archiveContentsAreReportedByFullPath(t *testing.T) {
+	// a package describing an archive does not account for the files within it
+	war := file.NewCoordinates("/app.war", "layer")
+	in := func(fullPath string) file.Coordinates {
+		return file.Coordinates{RealPath: "/app.war", FileSystemID: "layer", FullPath: fullPath}
+	}
+	nativeLib := in("/app.war!/WEB-INF/lib/jna.jar!/linux-x86-64/libjnidispatch.so")
+	unidentifiedJar := in("/app.war!/WEB-INF/lib/javax.inject-1.jar")
+	p := pkg.Package{Name: "app", Locations: file.NewLocationSet(file.NewLocationFromCoordinates(war))}
+	p.SetID()
+	noPackage := formatUnknown("no package identified in archive", "java-archive-cataloger")
+
+	s := &sbom.SBOM{
+		Artifacts: sbom.Artifacts{
+			Packages:    pkg.NewCollection(p),
+			Executables: map[file.Coordinates]file.Executable{nativeLib: {}},
+			Unknowns:    map[file.Coordinates][]string{unidentifiedJar: {noPackage}},
+		},
+		Relationships: []artifact.Relationship{{From: war, To: unidentifiedJar, Type: artifact.ContainsRelationship}},
+	}
+
+	unknownsLabelerTask{cataloging.UnknownsConfig{RemoveWhenPackagesDefined: true, IncludeExecutablesWithoutPackages: true}}.finalize(file.NewMockResolverForPaths(), s)
+
+	assert.Equal(t, map[file.Coordinates][]string{
+		nativeLib:       {formatUnknown("no package identified in executable file", unknownsLabelerTaskName)},
+		unidentifiedJar: {noPackage},
+	}, s.Artifacts.Unknowns)
+}
+
+func Test_unknownsLabeler_relativeOwnedPathsNameNothingInAnArchive(t *testing.T) {
+	// a python RECORD lists paths relative to site-packages, which resolve nowhere at the scan root either
+	whl := func(fullPath string) file.Coordinates {
+		return file.Coordinates{RealPath: "/pip.whl", FullPath: fullPath}
+	}
+	tool := whl("/pip.whl!/pip/tool")
+	p := pkg.Package{
+		Name:      "pip",
+		Locations: file.NewLocationSet(file.NewLocationFromCoordinates(whl("/pip.whl!/pip.dist-info/METADATA"))),
+		Metadata:  pkg.PythonPackage{Files: []pkg.PythonFileRecord{{Path: "pip/tool"}}},
+	}
+	p.SetID()
+	s := &sbom.SBOM{Artifacts: sbom.Artifacts{
+		Packages:    pkg.NewCollection(p),
+		Executables: map[file.Coordinates]file.Executable{tool: {}},
+	}}
+
+	unknownsLabelerTask{cataloging.UnknownsConfig{IncludeExecutablesWithoutPackages: true}}.finalize(file.NewMockResolverForPaths(), s)
+
+	assert.NotEmpty(t, s.Artifacts.Unknowns[tool])
+}
+
 func Test_unknownsLabeler_ownedFilesAreWithinTheOwnersContainer(t *testing.T) {
 	atRoot := file.NewCoordinates("/usr/bin/foo", "")
 	inArchive := file.Coordinates{RealPath: "/rootfs.tar", FullPath: "/rootfs.tar!/usr/bin/foo"}