From d2498fc786e3afd6fec2267148eab209d60995c5 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Tue, 18 Aug 2026 22:25:41 -0400 Subject: [PATCH 1/8] feat: package origin as a first-class model field An SBOM should say where each package came from, and only the component that read a manifest knows: npm's "resolved" is a tarball, cargo's "git+...#sha" is a pinned repository, uv's "editable" is a local path. Recovering that from the URL string downstream is guesswork with a per-ecosystem counterexample for every rule, so the answer has to travel with the dependency. Add PackageOrigin, carried on Dependency and Package: - ArtifactOrigin and RepositoryOrigin record what a manifest said; both return nil rather than a wrong answer when the value is not publishable. - NormalizeOriginURL is the single rule every published location satisfies: absolute http(s), host present, non-empty path, no userinfo, re-serialized from the parse. Local paths, file://, ssh and scp-style remotes, "git+" prefixes, registry and index roots, and credentialed URLs are all rejected, so filesystem layout and secrets cannot reach a published document. - Normalized() applies that rule again on read, so an origin arriving from a plugin or a hand-built graph is held to the same standard as one from a built-in component. - ReconcileOrigin settles records of one package that a graph merge folds together. Absence is not a disagreement; two different assertions cancel and stay cancelled, because publishing whichever record a merge kept first would make the answer depend on traversal order rather than on the project. Graph merging and Package.MergeFrom reconcile rather than keeping the first record, which is where an arbitrary answer used to come from. The field is optional and omitted when empty, so protocol v1 payloads from older binaries decode unchanged and payloads carrying origin are ignored by them -- the contract stays strictly additive. This gives external plugins a supported way to assert origin, which they had no access to while the rule lived in a host's internal package. Co-Authored-By: Claude Opus 5 --- container.go | 4 + dependency.go | 6 +- fuzz_test.go | 100 ++++++++++++++ origin.go | 214 +++++++++++++++++++++++++++++ origin_test.go | 357 +++++++++++++++++++++++++++++++++++++++++++++++++ package.go | 9 ++ 6 files changed, 689 insertions(+), 1 deletion(-) create mode 100644 origin.go create mode 100644 origin_test.go diff --git a/container.go b/container.go index b3fa22d..3703237 100644 --- a/container.go +++ b/container.go @@ -170,6 +170,10 @@ func addNodeIfMissing(g *Graph, node *Dependency) error { if existing, ok := g.Node(node.ID); ok && existing != nil { existing.Relationship = MergeDependencyRelationship(existing.Relationship, node.Relationship) mergeDependencyLocations(existing, clone.Locations) + // One node is one package. Where two records of it disagree about + // where it came from, the merge settles to no origin rather than + // to whichever arrived first. + existing.Origin = ReconcileOrigin(existing.Origin, clone.Origin) } return nil } diff --git a/dependency.go b/dependency.go index 34a578e..34ec3e1 100644 --- a/dependency.go +++ b/dependency.go @@ -94,7 +94,10 @@ type Dependency struct { Copyright string `json:"copyright,omitempty"` FoundBy string `json:"found_by,omitempty"` ResolvedURL string `json:"resolved_url,omitempty"` - Metadata map[string]any `json:"metadata,omitempty"` + // Origin is where this dependency was resolved from, as recorded by the + // detector that read the manifest. Read it through Origin.Normalized(). + Origin *PackageOrigin `json:"origin,omitempty"` + Metadata map[string]any `json:"metadata,omitempty"` // Matched is true when the referenced package was enriched by a matcher. Matched bool `json:"matched,omitempty"` @@ -194,6 +197,7 @@ func (d *Dependency) Clone() *Dependency { } } } + clone.Origin = d.Origin.Clone() clone.Metadata = cloneAnyMap(d.Metadata) return &clone } diff --git a/fuzz_test.go b/fuzz_test.go index 4623cca..841a967 100644 --- a/fuzz_test.go +++ b/fuzz_test.go @@ -2,6 +2,8 @@ package sdk import ( "encoding/json" + "net/url" + "strings" "testing" ) @@ -147,3 +149,101 @@ func requireFuzzGraphValid(t *testing.T, graph *Graph) { return true }) } + +// FuzzPackageOrigin drives the origin rule with arbitrary lockfile-derived +// strings: detectors pass raw manifest fields straight through, so whatever a +// repository can put in a lockfile reaches these constructors. +func FuzzPackageOrigin(f *testing.F) { + f.Add("https://registry.npmjs.org/react/-/react-18.2.0.tgz", "") + f.Add("https://github.com/owner/repo.git", "9f8e7d6c5b4a3928176554433221100ffeeddcc0") + f.Add("https://github.com/example/helper?rev=main#abc123", "v1.2.3") + f.Add("https://user:s3cret@nexus.corp/repo/pkg.tgz", "main") + f.Add("git+ssh://git@github.com/owner/repo.git#9f8e7d6", "9f8e7d6") + f.Add("file:///home/someone/wheels/pkg.whl", "") + f.Add("/Users/someone/src/project", "") + f.Add("http://0#0", "0") + f.Add("http://0/0#\x02", "\x02") + f.Add("%./0", "%") + f.Add("https://", "") + f.Add("https://:8080/pkg.tgz", "") + f.Add("https://registry.example.test/", "") + + f.Fuzz(func(t *testing.T, rawURL, revision string) { + artifact, repository := ArtifactOrigin(rawURL), RepositoryOrigin(rawURL, revision) + assertPublishableOrigin(t, artifact) + assertPublishableOrigin(t, repository) + // Reading back what was written must reach the same conclusion, and + // reconciling a record with itself must not change it. + assertPublishableOrigin(t, repository.Normalized()) + if settled, again := ReconcileOrigin(repository, repository), repository.Normalized(); !sameOrigin(settled, again) { + t.Fatalf("reconciling a record with itself changed it: %+v then %+v", again, settled) + } + // A disagreement is recorded rather than resolved, whatever the inputs. + if other := ArtifactOrigin("https://registry.example.test/other/pkg-1.0.0.tgz"); !artifact.Empty() && artifact.Normalized().ArtifactURL != other.ArtifactURL { + if settled := ReconcileOrigin(artifact, other); !settled.Empty() { + t.Fatalf("two different origins settled on %+v", settled) + } + } + }) +} + +// assertPublishableOrigin fails when an origin carries anything a published +// document must never show. +func assertPublishableOrigin(t *testing.T, origin *PackageOrigin) { + t.Helper() + normalized := origin.Normalized() + if normalized == nil { + return + } + if normalized.ArtifactURL != "" && normalized.Repository != "" { + t.Fatalf("origin names two locations at once: %+v", normalized) + } + if normalized.Revision != "" && normalized.Repository == "" { + t.Fatalf("revision %q recorded without a repository", normalized.Revision) + } + for _, raw := range []string{normalized.ArtifactURL, normalized.Repository} { + if raw == "" { + continue + } + parsed, err := url.Parse(raw) + if err != nil { + t.Fatalf("published URL %q does not parse: %v", raw, err) + } + if parsed.Scheme != "http" && parsed.Scheme != "https" { + t.Fatalf("published URL %q is not a web location", raw) + } + if parsed.Hostname() == "" { + t.Fatalf("published URL %q has no host", raw) + } + if parsed.User != nil { + t.Fatalf("published URL %q carries credentials", raw) + } + if parsed.Fragment != "" { + t.Fatalf("published URL %q carries a fragment", raw) + } + if strings.Trim(parsed.Path, "/") == "" { + t.Fatalf("published URL %q names a host root, not a package", raw) + } + } + if normalized.Repository != "" { + parsed, _ := url.Parse(normalized.Repository) + if parsed.RawQuery != "" || parsed.ForceQuery { + t.Fatalf("repository %q carries a query", normalized.Repository) + } + } + if !isValidOriginRevision(normalized.Revision) && normalized.Revision != "" { + t.Fatalf("revision %q would break a locator grammar", normalized.Revision) + } +} + +// sameOrigin compares two origins that may be nil. +func sameOrigin(left, right *PackageOrigin) bool { + switch { + case left == nil && right == nil: + return true + case left == nil || right == nil: + return false + default: + return *left == *right + } +} diff --git a/origin.go b/origin.go new file mode 100644 index 0000000..287baf0 --- /dev/null +++ b/origin.go @@ -0,0 +1,214 @@ +package sdk + +import ( + "net/url" + "strings" +) + +// maxOriginRevisionLength bounds a recorded revision. Commit hashes and tags +// are far shorter; anything longer is not a revision. +const maxOriginRevisionLength = 128 + +// PackageOrigin is where a package came from, as asserted by the component +// that resolved it. A detector reads it from a lockfile's source fields; a +// matcher may resolve one from package identity. +// +// A package has one origin: either it was downloaded as an artifact or it was +// resolved from a repository, never both. An empty origin means the component +// had nothing publishable to say, which is the normal case for a package whose +// lockfile records only a registry or index root. Consumers such as SBOM export +// should publish nothing rather than guess. +type PackageOrigin struct { + // ArtifactURL is the exact file the package was downloaded from. + ArtifactURL string `json:"artifact_url,omitempty"` + // Repository is the source repository the package was resolved from. + Repository string `json:"repository,omitempty"` + // Revision is the revision pinned in Repository, when the lockfile + // recorded one. Never set without Repository. + Revision string `json:"revision,omitempty"` + + // Disputed marks a package whose occurrences disagreed about where it came + // from -- one manifest resolving it from a private mirror and another from + // a public registry, say. A disputed origin reports no location at all: + // publishing whichever occurrence a merge happened to keep would make the + // answer depend on traversal order rather than on the project. The mark + // survives further merging so a later occurrence repeating one of the + // disputed values cannot revive it. + Disputed bool `json:"disputed,omitempty"` +} + +// ArtifactOrigin records the exact artifact a package was resolved from. +// Callers pass the lockfile field verbatim. It returns nil when the value is +// not a publishable location, since a missing origin is correct output and a +// wrong one is not. +func ArtifactOrigin(rawURL string) *PackageOrigin { + normalized, ok := NormalizeOriginURL(rawURL, false) + if !ok { + return nil + } + return &PackageOrigin{ArtifactURL: normalized} +} + +// RepositoryOrigin records the source repository a package was resolved from, +// plus the revision that was pinned. It returns nil when the URL is not a +// publishable location; an unusable revision drops only the revision, keeping +// the repository. +func RepositoryOrigin(rawURL, revision string) *PackageOrigin { + normalized, ok := NormalizeOriginURL(rawURL, true) + if !ok { + return nil + } + origin := &PackageOrigin{Repository: normalized} + if pinned := strings.TrimSpace(revision); isValidOriginRevision(pinned) { + origin.Revision = pinned + } + return origin +} + +// NormalizeOriginURL is the single rule every published origin URL satisfies. +// Apply it when recording a URL and again when reading one back, so an origin +// that arrives from a plugin or a hand-built graph is held to the same standard +// as one from a built-in component. +// +// A value passes only when it is an absolute http or https URL with a host, a +// non-empty path, and no embedded credentials; the result is re-serialized from +// the parse rather than returned as given. Everything else -- local paths, +// file://, git@host:org/repo, ssh://, git+ssh://, "git+" prefixes, registry and +// index roots, and URLs carrying userinfo -- is rejected, so filesystem layout +// and credentials cannot reach a published document. +// +// The repository argument selects the repository form: query and fragment are +// dropped, because they carry the ref that was requested rather than the one +// that was resolved, which callers pass separately. The artifact form drops the +// fragment (a checksum or anchor, never part of the location) and rejects a +// value carrying a query, which marks a signed or tokenized link rather than a +// stable location. +func NormalizeOriginURL(raw string, repository bool) (string, bool) { + trimmed := strings.TrimSpace(raw) + if trimmed == "" { + return "", false + } + parsed, err := url.Parse(trimmed) + if err != nil { + return "", false + } + switch strings.ToLower(parsed.Scheme) { + case "http", "https": + default: + return "", false + } + // Hostname also rejects a malformed host such as "https://:8080/pkg". + if parsed.Hostname() == "" || parsed.User != nil { + return "", false + } + parsed.Scheme = strings.ToLower(parsed.Scheme) + parsed.Fragment = "" + parsed.RawFragment = "" + // A host root names a server, not a package: a registry or index root on + // the artifact side, and no repository at all on the other. An empty path + // would also make a "@" locator re-parse as userinfo. + if strings.Trim(parsed.Path, "/") == "" { + return "", false + } + if repository { + parsed.RawQuery = "" + parsed.ForceQuery = false + } else if parsed.RawQuery != "" || parsed.ForceQuery { + return "", false + } + normalized := parsed.String() + if normalized == "" { + return "", false + } + return normalized, true +} + +// Empty reports whether o names no location. A disputed origin is empty: the +// disagreement is recorded, but there is nothing to publish. +func (o *PackageOrigin) Empty() bool { + if o == nil { + return true + } + return o.Disputed || (o.ArtifactURL == "" && o.Repository == "") +} + +// Normalized returns o with every value re-validated, or nil when nothing +// publishable survives. Read origin through this rather than reading the fields +// directly: it is what keeps a plugin-supplied or hand-built value from +// reaching a published document unchecked. An artifact wins over a repository +// in the case -- which the constructors never produce -- where both are set. +func (o *PackageOrigin) Normalized() *PackageOrigin { + if o == nil || o.Disputed { + return nil + } + if artifact, ok := NormalizeOriginURL(o.ArtifactURL, false); ok { + return &PackageOrigin{ArtifactURL: artifact} + } + repository, ok := NormalizeOriginURL(o.Repository, true) + if !ok { + return nil + } + normalized := &PackageOrigin{Repository: repository} + if pinned := strings.TrimSpace(o.Revision); isValidOriginRevision(pinned) { + normalized.Revision = pinned + } + return normalized +} + +// Clone returns a deep copy. +func (o *PackageOrigin) Clone() *PackageOrigin { + if o == nil { + return nil + } + clone := *o + return &clone +} + +// ReconcileOrigin settles the origins of two records of one package, which is +// what happens when a package appears in several manifests or several times in +// one dependency tree. +// +// Absence is not a disagreement: a record asserting nothing leaves an existing +// origin standing, and one asserting something fills a gap. Two records +// asserting different origins cancel, and stay cancelled -- the result is +// marked disputed, so a third record repeating one of the disputed values +// cannot revive it. +func ReconcileOrigin(existing, incoming *PackageOrigin) *PackageOrigin { + current, candidate := existing.Normalized(), incoming.Normalized() + switch { + case disputed(existing), disputed(incoming): + return &PackageOrigin{Disputed: true} + case candidate == nil: + return current + case current == nil: + return candidate + case *current != *candidate: + return &PackageOrigin{Disputed: true} + default: + return current + } +} + +// disputed reports whether an origin already records a disagreement. +func disputed(o *PackageOrigin) bool { + return o != nil && o.Disputed +} + +// isValidOriginRevision reports whether revision is safe to publish beside a +// repository. The charset keeps commit hashes, tags, and branch-style refs +// while excluding whitespace, "@", and percent escapes, which would break +// locator grammars such as SPDX's "git+@". +func isValidOriginRevision(revision string) bool { + if revision == "" || len(revision) > maxOriginRevisionLength { + return false + } + for _, r := range revision { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + case r == '.', r == '_', r == '-', r == '+', r == '/': + default: + return false + } + } + return true +} diff --git a/origin_test.go b/origin_test.go new file mode 100644 index 0000000..76755bd --- /dev/null +++ b/origin_test.go @@ -0,0 +1,357 @@ +package sdk + +import ( + "encoding/json" + "strings" + "testing" +) + +func TestArtifactOrigin(t *testing.T) { + cases := []struct { + name string + raw string + want string + }{ + {name: "registry tarball", raw: "https://registry.npmjs.org/react/-/react-18.2.0.tgz", want: "https://registry.npmjs.org/react/-/react-18.2.0.tgz"}, + {name: "checksum fragment is stripped", raw: "https://registry.npmjs.org/react/-/react-18.2.0.tgz#ceeba773e3e9d2b6f1a2b6b9f4f1cb2f9c2e1a55", want: "https://registry.npmjs.org/react/-/react-18.2.0.tgz"}, + {name: "uppercase scheme is normalized", raw: "HTTPS://files.pythonhosted.org/packages/x/django-5.0.tar.gz", want: "https://files.pythonhosted.org/packages/x/django-5.0.tar.gz"}, + {name: "signed link carrying a query", raw: "https://nexus.corp/repo/pkg.tgz?token=abc123"}, + {name: "embedded credentials", raw: "https://user:s3cret@nexus.corp/repo/pkg.tgz"}, + {name: "registry root", raw: "https://registry.npmjs.org/"}, + {name: "relative path", raw: "packages/lib"}, + {name: "absolute local path", raw: "/Users/someone/src/project"}, + {name: "file url", raw: "file:///home/someone/wheels/pkg.whl"}, + {name: "git+ssh remote", raw: "git+ssh://git@github.com/owner/repo.git#9f8e7d6"}, + {name: "git+https prefix", raw: "git+https://github.com/owner/repo.git"}, + {name: "scp-style remote", raw: "git@github.com:owner/repo.git"}, + {name: "windows path", raw: `C:\src\project`}, + {name: "malformed host", raw: "https://:8080/pkg.tgz"}, + {name: "non-web scheme", raw: "ftp://files.example.com/pkg.tgz"}, + {name: "empty", raw: " "}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + origin := ArtifactOrigin(tc.raw) + if tc.want == "" { + if origin != nil { + t.Fatalf("origin = %+v, want nil", origin) + } + return + } + if origin == nil { + t.Fatalf("origin = nil, want artifact %q", tc.want) + } + if origin.ArtifactURL != tc.want { + t.Fatalf("artifact = %q, want %q", origin.ArtifactURL, tc.want) + } + if origin.Repository != "" || origin.Revision != "" { + t.Fatalf("artifact origin carries repository data: %+v", origin) + } + }) + } +} + +func TestRepositoryOrigin(t *testing.T) { + cases := []struct { + name string + raw string + revision string + wantRepo string + wantRevision string + }{ + { + name: "repository with resolved commit", + raw: "https://github.com/owner/repo.git", + revision: "9f8e7d6c5b4a3928176554433221100ffeeddcc0", + wantRepo: "https://github.com/owner/repo.git", + wantRevision: "9f8e7d6c5b4a3928176554433221100ffeeddcc0", + }, + { + name: "requested ref is dropped for the resolved one", + raw: "https://github.com/example/helper?rev=main#abc123", + revision: "0a1b2c3d4e5f60718293a4b5c6d7e8f901234567", + wantRepo: "https://github.com/example/helper", + wantRevision: "0a1b2c3d4e5f60718293a4b5c6d7e8f901234567", + }, + {name: "tag pin", raw: "https://github.com/owner/repo", revision: "v1.2.3", wantRepo: "https://github.com/owner/repo", wantRevision: "v1.2.3"}, + {name: "branch-style ref", raw: "https://github.com/owner/repo", revision: "release/2026-08", wantRepo: "https://github.com/owner/repo", wantRevision: "release/2026-08"}, + {name: "unpinned repository", raw: "https://github.com/owner/repo", wantRepo: "https://github.com/owner/repo"}, + {name: "revision breaking a locator grammar", raw: "https://github.com/owner/repo", revision: "feature@login", wantRepo: "https://github.com/owner/repo"}, + {name: "whitespace revision", raw: "https://github.com/owner/repo", revision: "not a revision", wantRepo: "https://github.com/owner/repo"}, + {name: "overlong revision", raw: "https://github.com/owner/repo", revision: strings.Repeat("a", 129), wantRepo: "https://github.com/owner/repo"}, + {name: "bare host", raw: "https://github.com", revision: "9f8e7d6"}, + {name: "index root", raw: "https://index.crates.io/", revision: "9f8e7d6"}, + {name: "credentialed remote", raw: "https://oauth2:glpat-xxxxxxxxxxxxxxxxxxxx@gitlab.corp/team/repo.git", revision: "9f8e7d6"}, + {name: "ssh remote", raw: "ssh://github.com/owner/repo.git", revision: "9f8e7d6"}, + {name: "local checkout", raw: "/Users/someone/src/repo", revision: "9f8e7d6"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + origin := RepositoryOrigin(tc.raw, tc.revision) + if tc.wantRepo == "" { + if origin != nil { + t.Fatalf("origin = %+v, want nil", origin) + } + return + } + if origin == nil { + t.Fatalf("origin = nil, want repository %q", tc.wantRepo) + } + if origin.Repository != tc.wantRepo || origin.Revision != tc.wantRevision { + t.Fatalf("origin = %+v, want repository %q revision %q", origin, tc.wantRepo, tc.wantRevision) + } + if origin.ArtifactURL != "" { + t.Fatalf("repository origin carries an artifact URL: %q", origin.ArtifactURL) + } + }) + } +} + +// Origin can reach a consumer from a plugin or a hand-built graph that never +// went through the constructors, so reading re-validates. +func TestPackageOriginNormalized(t *testing.T) { + cases := []struct { + name string + origin *PackageOrigin + want *PackageOrigin + }{ + {name: "nil"}, + {name: "empty", origin: &PackageOrigin{}}, + {name: "credentialed artifact", origin: &PackageOrigin{ArtifactURL: "https://build:s3cret@nexus.corp/pkg.tgz"}}, + {name: "local repository", origin: &PackageOrigin{Repository: "file:///home/someone/repo"}}, + {name: "revision without a repository", origin: &PackageOrigin{Revision: "9f8e7d6"}}, + {name: "disputed reports nothing", origin: &PackageOrigin{Disputed: true, ArtifactURL: "https://registry.npmjs.org/react/-/react-18.2.0.tgz"}}, + { + name: "artifact wins over repository", + origin: &PackageOrigin{ArtifactURL: "https://registry.npmjs.org/react/-/react-18.2.0.tgz", Repository: "https://github.com/facebook/react"}, + want: &PackageOrigin{ArtifactURL: "https://registry.npmjs.org/react/-/react-18.2.0.tgz"}, + }, + { + name: "query and fragment are stripped from a hand-built repository", + origin: &PackageOrigin{Repository: "https://github.com/owner/repo?rev=main#abc", Revision: "9f8e7d6"}, + want: &PackageOrigin{Repository: "https://github.com/owner/repo", Revision: "9f8e7d6"}, + }, + { + name: "unusable revision is dropped, repository kept", + origin: &PackageOrigin{Repository: "https://github.com/owner/repo", Revision: "feature@login"}, + want: &PackageOrigin{Repository: "https://github.com/owner/repo"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := tc.origin.Normalized() + switch { + case tc.want == nil && got != nil: + t.Fatalf("normalized = %+v, want nil", got) + case tc.want != nil && got == nil: + t.Fatalf("normalized = nil, want %+v", tc.want) + case tc.want != nil && *got != *tc.want: + t.Fatalf("normalized = %+v, want %+v", got, tc.want) + } + }) + } +} + +func TestPackageOriginEmpty(t *testing.T) { + var nilOrigin *PackageOrigin + if !nilOrigin.Empty() { + t.Fatal("nil origin should be empty") + } + if !(&PackageOrigin{}).Empty() { + t.Fatal("zero origin should be empty") + } + if !(&PackageOrigin{Disputed: true}).Empty() { + t.Fatal("a disputed origin names no location, so it is empty") + } + if (&PackageOrigin{ArtifactURL: "https://example.test/pkg.tgz"}).Empty() { + t.Fatal("artifact origin should not be empty") + } +} + +func TestReconcileOrigin(t *testing.T) { + const ( + artifact = "https://registry.npmjs.org/react/-/react-18.2.0.tgz" + mirror = "https://npm.corp/mirror/react/-/react-18.2.0.tgz" + repo = "https://github.com/facebook/react" + ) + + cases := []struct { + name string + existing *PackageOrigin + incoming *PackageOrigin + want *PackageOrigin + }{ + {name: "neither records anything"}, + {name: "records agree", existing: ArtifactOrigin(artifact), incoming: ArtifactOrigin(artifact), want: &PackageOrigin{ArtifactURL: artifact}}, + {name: "absence keeps an origin", existing: ArtifactOrigin(artifact), want: &PackageOrigin{ArtifactURL: artifact}}, + {name: "absence fills a gap", incoming: ArtifactOrigin(artifact), want: &PackageOrigin{ArtifactURL: artifact}}, + {name: "records disagree", existing: ArtifactOrigin(artifact), incoming: ArtifactOrigin(mirror), want: &PackageOrigin{Disputed: true}}, + {name: "different kinds disagree", existing: ArtifactOrigin(artifact), incoming: RepositoryOrigin(repo, ""), want: &PackageOrigin{Disputed: true}}, + {name: "different pins disagree", existing: RepositoryOrigin(repo, "aaaabbbbccccddddeeeeffff0000111122223333"), incoming: RepositoryOrigin(repo, ""), want: &PackageOrigin{Disputed: true}}, + {name: "a disagreement is not lifted by absence", existing: &PackageOrigin{Disputed: true}, want: &PackageOrigin{Disputed: true}}, + {name: "a disagreement is not lifted by agreement", existing: &PackageOrigin{Disputed: true}, incoming: ArtifactOrigin(artifact), want: &PackageOrigin{Disputed: true}}, + {name: "a disputed record poisons a settled one", existing: ArtifactOrigin(artifact), incoming: &PackageOrigin{Disputed: true}, want: &PackageOrigin{Disputed: true}}, + {name: "an unpublishable record is not a disagreement", existing: ArtifactOrigin(artifact), incoming: &PackageOrigin{ArtifactURL: "/Users/someone/pkg.tgz"}, want: &PackageOrigin{ArtifactURL: artifact}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := ReconcileOrigin(tc.existing, tc.incoming) + switch { + case tc.want == nil && got != nil: + t.Fatalf("reconciled = %+v, want nil", got) + case tc.want != nil && got == nil: + t.Fatalf("reconciled = nil, want %+v", tc.want) + case tc.want != nil && *got != *tc.want: + t.Fatalf("reconciled = %+v, want %+v", got, tc.want) + } + }) + } +} + +// Three records claiming A, B, then A must not settle on A. +func TestReconcileOriginDisagreementIsFinal(t *testing.T) { + const ( + artifact = "https://registry.npmjs.org/react/-/react-18.2.0.tgz" + mirror = "https://npm.corp/mirror/react/-/react-18.2.0.tgz" + ) + settled := ReconcileOrigin(ArtifactOrigin(artifact), ArtifactOrigin(mirror)) + settled = ReconcileOrigin(settled, ArtifactOrigin(artifact)) + + if !settled.Empty() { + t.Fatalf("origin = %+v, want none: the records never agreed", settled) + } + if !settled.Disputed { + t.Fatal("the disagreement must stay recorded, or a later merge revives a disputed value") + } +} + +// A merged graph node carries the reconciled answer rather than whichever +// record was added first. +func TestMergeGraphReconcilesOrigin(t *testing.T) { + const ( + artifact = "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz" + mirror = "https://npm.corp/mirror/lodash/-/lodash-4.17.21.tgz" + ) + build := func(t *testing.T, url string) *Graph { + t.Helper() + g := New() + node := NewDependencyWithID("lodash@4.17.21", Dependency{ + Coordinates: Coordinates{Name: "lodash", Version: "4.17.21", Ecosystem: EcosystemNPM}, + Origin: ArtifactOrigin(url), + }) + if err := g.AddNode(node); err != nil { + t.Fatal(err) + } + return g + } + + t.Run("disagreement settles to nothing", func(t *testing.T) { + merged := New() + if err := MergeGraph(merged, build(t, artifact)); err != nil { + t.Fatal(err) + } + if err := MergeGraph(merged, build(t, mirror)); err != nil { + t.Fatal(err) + } + node, ok := merged.Node("lodash@4.17.21") + if !ok { + t.Fatal("expected lodash in the merged graph") + } + if got := node.Origin.Normalized(); got != nil { + t.Fatalf("merged origin = %+v, want none", got) + } + }) + + t.Run("agreement survives", func(t *testing.T) { + merged := New() + if err := MergeGraph(merged, build(t, artifact)); err != nil { + t.Fatal(err) + } + if err := MergeGraph(merged, build(t, artifact)); err != nil { + t.Fatal(err) + } + node, _ := merged.Node("lodash@4.17.21") + if got := node.Origin.Normalized(); got == nil || got.ArtifactURL != artifact { + t.Fatalf("merged origin = %+v, want %q", got, artifact) + } + }) +} + +// Cloning a dependency must not leave the copies sharing origin state. +func TestDependencyCloneCopiesOrigin(t *testing.T) { + dep := NewDependencyWithID("react@18.2.0", Dependency{ + Coordinates: Coordinates{Name: "react", Version: "18.2.0"}, + Origin: ArtifactOrigin("https://registry.npmjs.org/react/-/react-18.2.0.tgz"), + }) + clone := dep.Clone() + clone.Origin.ArtifactURL = "https://npm.corp/mirror/react/-/react-18.2.0.tgz" + + if dep.Origin.ArtifactURL != "https://registry.npmjs.org/react/-/react-18.2.0.tgz" { + t.Fatalf("mutating a clone changed the original: %+v", dep.Origin) + } +} + +// Origin travels with the package a dependency refers to. +func TestPackageFromDependencyCarriesOrigin(t *testing.T) { + dep := NewDependencyWithID("react@18.2.0", Dependency{ + Coordinates: Coordinates{Name: "react", Version: "18.2.0", Ecosystem: EcosystemNPM, PURL: "pkg:npm/react@18.2.0"}, + Origin: ArtifactOrigin("https://registry.npmjs.org/react/-/react-18.2.0.tgz"), + }) + pkg := PackageFromDependency(dep) + if pkg.Origin == nil || pkg.Origin.ArtifactURL != "https://registry.npmjs.org/react/-/react-18.2.0.tgz" { + t.Fatalf("package origin = %+v, want the dependency's", pkg.Origin) + } + pkg.Origin.ArtifactURL = "https://npm.corp/mirror/react/-/react-18.2.0.tgz" + if dep.Origin.ArtifactURL == pkg.Origin.ArtifactURL { + t.Fatal("package and dependency share origin state") + } +} + +// Registry deduplication settles two records of one package the same way. +func TestPackageMergeFromReconcilesOrigin(t *testing.T) { + const ( + artifact = "https://registry.npmjs.org/react/-/react-18.2.0.tgz" + mirror = "https://npm.corp/mirror/react/-/react-18.2.0.tgz" + ) + pkg := &Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Origin: ArtifactOrigin(artifact)} + pkg.MergeFrom(&Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Origin: ArtifactOrigin(mirror)}) + + if got := pkg.Origin.Normalized(); got != nil { + t.Fatalf("merged origin = %+v, want none", got) + } +} + +// The wire contract is additive: an origin-bearing payload round-trips, and a +// payload from a build that predates the field still decodes. +func TestPackageOriginWireRoundTrip(t *testing.T) { + dep := NewDependencyWithID("react@18.2.0", Dependency{ + Coordinates: Coordinates{Name: "react", Version: "18.2.0"}, + Origin: RepositoryOrigin("https://github.com/facebook/react", "b4c5d6e7f8091a2b3c4d5e6f708192a3b4c5d6e7"), + }) + raw, err := json.Marshal(dep) + if err != nil { + t.Fatal(err) + } + var decoded Dependency + if err := json.Unmarshal(raw, &decoded); err != nil { + t.Fatal(err) + } + if decoded.Origin == nil || *decoded.Origin != *dep.Origin { + t.Fatalf("decoded origin = %+v, want %+v", decoded.Origin, dep.Origin) + } + + var legacy Dependency + if err := json.Unmarshal([]byte(`{"id":"react@18.2.0","name":"react","version":"18.2.0"}`), &legacy); err != nil { + t.Fatal(err) + } + if legacy.Origin != nil { + t.Fatalf("origin = %+v, want nil for a payload that predates the field", legacy.Origin) + } + if raw, err := json.Marshal(legacy); err != nil || strings.Contains(string(raw), "origin") { + t.Fatalf("an absent origin must not be serialized: %s (err %v)", raw, err) + } +} diff --git a/package.go b/package.go index 5d6b32c..bba57fe 100644 --- a/package.go +++ b/package.go @@ -175,6 +175,10 @@ type Package struct { ID string `json:"id,omitempty"` Copyright string `json:"copyright,omitempty"` ResolvedURL string `json:"resolved_url,omitempty"` + // Origin is where this package came from: carried from the dependency that + // referenced it, or resolved by a matcher. Read it through + // Origin.Normalized(). + Origin *PackageOrigin `json:"origin,omitempty"` CPEs []string `json:"cpes,omitempty"` Digests []Digest `json:"digests,omitempty"` @@ -280,6 +284,7 @@ func (p *Package) Clone() *Package { clone.Vulnerabilities = append(clone.Vulnerabilities, v.Clone()) } } + clone.Origin = p.Origin.Clone() clone.Scorecard = p.Scorecard.Clone() clone.EOL = p.EOL.Clone() clone.Remediation = p.Remediation.Clone() @@ -325,6 +330,9 @@ func (p *Package) MergeFrom(src *Package) { if p.ResolvedURL == "" { p.ResolvedURL = src.ResolvedURL } + // Two records of one package that disagree about where it came from settle + // to no origin rather than to whichever was merged first. + p.Origin = ReconcileOrigin(p.Origin, src.Origin) if len(p.CPEs) == 0 { p.CPEs = cloneStrings(src.CPEs) } @@ -428,6 +436,7 @@ func PackageFromDependency(dep *Dependency) *Package { }, ID: purl, ResolvedURL: dep.ResolvedURL, + Origin: dep.Origin.Clone(), } } From 9bb219f4f46f598f6832cb49a1839d5455714f7d Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Tue, 18 Aug 2026 22:26:46 -0400 Subject: [PATCH 2/8] feat: carriers for provenance facts the model cannot express today Two additive fields, both optional, neither populated by anything yet. They exist so the facts have a typed home when a producer appears, rather than arriving as untyped metadata that every consumer has to guess at. Digest.Subject says what a hash covers. Empty means the published artifact, which is what nearly every ecosystem records, so existing producers keep their meaning unchanged. It exists because some hashes are not hashes of a file: a Go module's "h1:" value is SHA-256 over a manifest of the source tree's file hashes, not over the module zip, and a consumer that compares it against a downloaded file will always find a mismatch. Today that distinction is lost. Package.Attestations holds signed statements about how a package was built or published -- in-toto statements such as SLSA provenance. Bomly neither fetches nor verifies them; the type records what a matcher found and, importantly, whether it verified the signature. That flag is the part worth modelling: an unverified statement is weaker evidence, not proof, and the difference is the first thing lost when provenance is carried as free-form data. This commit is separable from the origin work and can be dropped if you would rather wait for a producer -- nothing depends on it. Co-Authored-By: Claude Opus 5 --- package.go | 91 +++++++++++++++++++++++++++++++++--- provenance_test.go | 112 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 196 insertions(+), 7 deletions(-) create mode 100644 provenance_test.go diff --git a/package.go b/package.go index bba57fe..3b2a198 100644 --- a/package.go +++ b/package.go @@ -67,6 +67,70 @@ type PackageLicense struct { type Digest struct { Algorithm DigestAlgorithm `json:"algorithm,omitempty"` Value string `json:"value,omitempty"` + // Subject says what the digest covers. Empty means the published artifact, + // which is what most ecosystems record and what a consumer should assume. + // It exists because some ecosystems record a hash that is not a hash of a + // file: a Go module's "h1:" value is SHA-256 over a manifest of the source + // tree's file hashes, not over the module zip, so a consumer that treats it + // as an artifact digest and compares it against a downloaded file will + // always find a mismatch. + Subject DigestSubject `json:"subject,omitempty"` +} + +// DigestSubject identifies what a digest was computed over. +type DigestSubject string + +const ( + // DigestSubjectArtifact is a digest of the published file itself. It is + // the zero value: a producer that does not say means the artifact. + DigestSubjectArtifact DigestSubject = "" + // DigestSubjectSourceTree is a digest over a source tree or over a + // manifest of its file hashes, such as a Go module "h1:" dirhash. + DigestSubjectSourceTree DigestSubject = "source-tree" + // DigestSubjectMetadata is a digest of a package's metadata document + // rather than of the package itself, such as a manifest or lockfile entry. + DigestSubjectMetadata DigestSubject = "metadata" +) + +// PackageAttestation records a signed statement about how a package was built +// or published: an in-toto statement such as SLSA provenance, or a +// publish-time signature. +// +// Bomly does not fetch or verify attestations today. The type exists so a +// matcher that does can attach what it found without a model change, and so +// consumers can tell a verified statement from one that was merely present -- +// a distinction that matters more than the statement itself, and that is +// easily lost when provenance data is carried in untyped metadata. +type PackageAttestation struct { + // PredicateType identifies what the statement asserts, using the in-toto + // predicate vocabulary (for example "https://slsa.dev/provenance/v1"). + PredicateType string `json:"predicate_type,omitempty"` + // Source names the component or service that attached the statement, in + // the same style as PackageScorecard.Source. + Source string `json:"source,omitempty"` + // URL is where the statement can be fetched. + URL string `json:"url,omitempty"` + // Digest identifies the statement itself, so two fetches of one URL can be + // told apart. + Digest *Digest `json:"digest,omitempty"` + // Issuer is the identity that signed the statement -- an OIDC identity, a + // key id, or a registry account -- as reported by whatever verified it. + Issuer string `json:"issuer,omitempty"` + // Verified records that the component attaching this statement checked its + // signature. False means the statement was found but not verified, which is + // weaker evidence rather than evidence of tampering; consumers must not + // present an unverified statement as proof of provenance. + Verified bool `json:"verified,omitempty"` +} + +// Clone returns a deep copy. +func (a PackageAttestation) Clone() PackageAttestation { + clone := a + if a.Digest != nil { + digest := *a.Digest + clone.Digest = &digest + } + return clone } // PackageEOL captures end-of-life enrichment attached by the EOL matcher. @@ -180,13 +244,14 @@ type Package struct { // Origin.Normalized(). Origin *PackageOrigin `json:"origin,omitempty"` - CPEs []string `json:"cpes,omitempty"` - Digests []Digest `json:"digests,omitempty"` - Licenses []PackageLicense `json:"licenses,omitempty"` - Vulnerabilities []Vulnerability `json:"vulnerabilities,omitempty"` - Scorecard *PackageScorecard `json:"scorecard,omitempty"` - EOL *PackageEOL `json:"eol,omitempty"` - Remediation *PackageRemediation `json:"remediation,omitempty"` + CPEs []string `json:"cpes,omitempty"` + Digests []Digest `json:"digests,omitempty"` + Licenses []PackageLicense `json:"licenses,omitempty"` + Vulnerabilities []Vulnerability `json:"vulnerabilities,omitempty"` + Attestations []PackageAttestation `json:"attestations,omitempty"` + Scorecard *PackageScorecard `json:"scorecard,omitempty"` + EOL *PackageEOL `json:"eol,omitempty"` + Remediation *PackageRemediation `json:"remediation,omitempty"` // Matched indicates that this package was successfully matched by one or // more external enrichment sources. @@ -285,6 +350,12 @@ func (p *Package) Clone() *Package { } } clone.Origin = p.Origin.Clone() + if len(p.Attestations) > 0 { + clone.Attestations = make([]PackageAttestation, 0, len(p.Attestations)) + for _, attestation := range p.Attestations { + clone.Attestations = append(clone.Attestations, attestation.Clone()) + } + } clone.Scorecard = p.Scorecard.Clone() clone.EOL = p.EOL.Clone() clone.Remediation = p.Remediation.Clone() @@ -333,6 +404,12 @@ func (p *Package) MergeFrom(src *Package) { // Two records of one package that disagree about where it came from settle // to no origin rather than to whichever was merged first. p.Origin = ReconcileOrigin(p.Origin, src.Origin) + if len(p.Attestations) == 0 && len(src.Attestations) > 0 { + p.Attestations = make([]PackageAttestation, 0, len(src.Attestations)) + for _, attestation := range src.Attestations { + p.Attestations = append(p.Attestations, attestation.Clone()) + } + } if len(p.CPEs) == 0 { p.CPEs = cloneStrings(src.CPEs) } diff --git a/provenance_test.go b/provenance_test.go new file mode 100644 index 0000000..340b306 --- /dev/null +++ b/provenance_test.go @@ -0,0 +1,112 @@ +package sdk + +import ( + "encoding/json" + "strings" + "testing" +) + +// A digest that does not say what it covers means the published artifact, so +// existing producers keep their meaning without changing anything. +func TestDigestSubjectDefaultsToArtifact(t *testing.T) { + digest := Digest{Algorithm: DigestAlgorithmSHA256, Value: "abc123"} + if digest.Subject != DigestSubjectArtifact { + t.Fatalf("subject = %q, want the artifact default", digest.Subject) + } + + raw, err := json.Marshal(digest) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "subject") { + t.Fatalf("the default subject must not be serialized: %s", raw) + } +} + +// A Go module h1 value hashes a manifest of the source tree, not the module +// zip; saying so is the point of the field. +func TestDigestSubjectSourceTreeRoundTrips(t *testing.T) { + digest := Digest{Algorithm: DigestAlgorithmSHA256, Value: "abc123", Subject: DigestSubjectSourceTree} + raw, err := json.Marshal(digest) + if err != nil { + t.Fatal(err) + } + var decoded Digest + if err := json.Unmarshal(raw, &decoded); err != nil { + t.Fatal(err) + } + if decoded != digest { + t.Fatalf("decoded = %+v, want %+v", decoded, digest) + } + + // A payload from a build that predates the field still decodes. + var legacy Digest + if err := json.Unmarshal([]byte(`{"algorithm":"sha256","value":"abc123"}`), &legacy); err != nil { + t.Fatal(err) + } + if legacy.Subject != DigestSubjectArtifact { + t.Fatalf("subject = %q, want the artifact default", legacy.Subject) + } +} + +func TestPackageAttestationCloneIsDeep(t *testing.T) { + pkg := &Package{ + Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, + Attestations: []PackageAttestation{{ + PredicateType: "https://slsa.dev/provenance/v1", + Source: "example-matcher", + URL: "https://registry.example.test/react/18.2.0/provenance", + Digest: &Digest{Algorithm: DigestAlgorithmSHA256, Value: "abc123"}, + Issuer: "https://accounts.example.test/workflow", + Verified: true, + }}, + } + + clone := pkg.Clone() + clone.Attestations[0].Verified = false + clone.Attestations[0].Digest.Value = "def456" + clone.Attestations[0].Issuer = "someone-else" + + original := pkg.Attestations[0] + if !original.Verified || original.Digest.Value != "abc123" || original.Issuer != "https://accounts.example.test/workflow" { + t.Fatalf("mutating a clone changed the original: %+v", original) + } +} + +// Attestations survive registry deduplication when the record that wins has +// none of its own. +func TestPackageMergeFromFillsAttestations(t *testing.T) { + pkg := &Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}} + pkg.MergeFrom(&Package{ + Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, + Attestations: []PackageAttestation{{PredicateType: "https://slsa.dev/provenance/v1", Verified: true}}, + }) + + if len(pkg.Attestations) != 1 || !pkg.Attestations[0].Verified { + t.Fatalf("attestations = %+v, want the merged record's", pkg.Attestations) + } + + // The merged copy must not share state with the source. + source := &Package{ + Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, + Attestations: []PackageAttestation{{PredicateType: "https://slsa.dev/provenance/v1", Digest: &Digest{Value: "abc123"}}}, + } + target := &Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}} + target.MergeFrom(source) + target.Attestations[0].Digest.Value = "def456" + if source.Attestations[0].Digest.Value != "abc123" { + t.Fatal("merged attestations share state with the source") + } +} + +// An empty attestation list is omitted, so payloads for the overwhelmingly +// common case are unchanged. +func TestPackageAttestationsOmittedWhenEmpty(t *testing.T) { + raw, err := json.Marshal(&Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}}) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "attestations") { + t.Fatalf("an empty attestation list must not be serialized: %s", raw) + } +} From 77e7b0668615e317825b4efc75dbaf901ecc421b Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Tue, 18 Aug 2026 22:48:19 -0400 Subject: [PATCH 3/8] fix: reconcile origin per occurrence, canonicalize host case, union attestations Three review findings, all real: - The registry seeding path skipped every occurrence of a package after the first, so a package resolved differently in two manifests kept whichever was walked first -- the exact order-dependence this work is meant to remove, on the path that actually feeds matchers. Every occurrence now reconciles onto the registry package; the package is still enriched once. - Hosts are case-insensitive, so "https://GitHub.com/owner/repo" and "https://github.com/owner/repo" name one place. Comparing them as strings made reconciliation read a disagreement and drop a perfectly good origin over formatting alone. The host is now lowercased; the path is left alone, being case-sensitive. - Attestations were kept first-wins, which discards statements when several components each attach one -- a provenance statement from one matcher and a signature from another. They now union, deduplicated by source, predicate type, URL, and digest, the way vulnerabilities already do. Where two records describe one statement and either verified it, the merged record is verified: verification is a fact a component established, not an opinion. Co-Authored-By: Claude Opus 5 --- matcherkit/registry.go | 15 +++++-- matcherkit/registry_origin_test.go | 72 ++++++++++++++++++++++++++++++ origin.go | 5 +++ origin_test.go | 25 +++++++++++ package.go | 53 +++++++++++++++++++--- provenance_test.go | 43 ++++++++++++++++++ 6 files changed, 203 insertions(+), 10 deletions(-) create mode 100644 matcherkit/registry_origin_test.go diff --git a/matcherkit/registry.go b/matcherkit/registry.go index 824698c..dce8548 100644 --- a/matcherkit/registry.go +++ b/matcherkit/registry.go @@ -32,15 +32,22 @@ func RegistryPackagesForGraph(g *sdk.Graph, reg *sdk.PackageRegistry, target *sd continue } dep.PackageRef = purl - if _, ok := seen[purl]; ok { - continue - } - seen[purl] = struct{}{} pkg, ok := reg.Get(purl) if !ok { pkg = reg.Add(sdk.PackageFromDependency(dep)) + } else if pkg != nil { + // A package is enriched once, but every occurrence of it still + // gets a say about where it came from: two manifests resolving + // one package from different places disagree, and the package + // must not report whichever was walked first. + pkg.Origin = sdk.ReconcileOrigin(pkg.Origin, dep.Origin) } + + if _, alreadySeen := seen[purl]; alreadySeen { + continue + } + seen[purl] = struct{}{} if pkg != nil { out = append(out, pkg) } diff --git a/matcherkit/registry_origin_test.go b/matcherkit/registry_origin_test.go new file mode 100644 index 0000000..6d13e77 --- /dev/null +++ b/matcherkit/registry_origin_test.go @@ -0,0 +1,72 @@ +package matcherkit_test + +import ( + "testing" + + sdk "github.com/bomly-dev/bomly-sdk" + "github.com/bomly-dev/bomly-sdk/matcherkit" +) + +// node builds one dependency occurrence of a package with a chosen origin. +func node(t *testing.T, id, artifactURL string) *sdk.Dependency { + t.Helper() + dep := sdk.NewDependencyWithID(id, sdk.Dependency{ + Coordinates: sdk.Coordinates{ + Name: "lodash", Version: "4.17.21", Ecosystem: sdk.EcosystemNPM, PURL: "pkg:npm/lodash@4.17.21", + }, + }) + if artifactURL != "" { + dep.Origin = sdk.ArtifactOrigin(artifactURL) + } + return dep +} + +// A package is enriched once however many dependencies reference it, but every +// occurrence still gets a say about where it came from -- otherwise the +// registry publishes whichever was walked first. +func TestRegistryPackagesReconcileOriginAcrossOccurrences(t *testing.T) { + const ( + public = "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz" + private = "https://npm.corp/mirror/lodash/-/lodash-4.17.21.tgz" + ) + + cases := []struct { + name string + left string + right string + want string + }{ + {name: "occurrences agree", left: public, right: public, want: public}, + {name: "occurrences disagree", left: public, right: private}, + {name: "one occurrence says nothing", left: public, right: "", want: public}, + {name: "a later occurrence fills the gap", left: "", right: public, want: public}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + g := sdk.New() + for _, dep := range []*sdk.Dependency{node(t, "web:lodash", tc.left), node(t, "api:lodash", tc.right)} { + if err := g.AddNode(dep); err != nil { + t.Fatal(err) + } + } + + registry := sdk.NewPackageRegistry() + packages := matcherkit.RegistryPackagesForGraph(g, registry, nil) + if len(packages) != 1 { + t.Fatalf("registry packages = %d, want 1: the package is enriched once", len(packages)) + } + + origin := packages[0].Origin.Normalized() + if tc.want == "" { + if origin != nil { + t.Fatalf("origin = %+v, want none", origin) + } + return + } + if origin == nil || origin.ArtifactURL != tc.want { + t.Fatalf("origin = %+v, want %q", origin, tc.want) + } + }) + } +} diff --git a/origin.go b/origin.go index 287baf0..bd62f89 100644 --- a/origin.go +++ b/origin.go @@ -102,6 +102,11 @@ func NormalizeOriginURL(raw string, repository bool) (string, bool) { return "", false } parsed.Scheme = strings.ToLower(parsed.Scheme) + // Hosts are case-insensitive, so two records writing one host differently + // name the same location. Without this they would compare unequal and + // reconcile to a disagreement, losing an origin to formatting alone. The + // path is left alone: it is case-sensitive. + parsed.Host = strings.ToLower(parsed.Host) parsed.Fragment = "" parsed.RawFragment = "" // A host root names a server, not a package: a registry or index root on diff --git a/origin_test.go b/origin_test.go index 76755bd..4476dfc 100644 --- a/origin_test.go +++ b/origin_test.go @@ -155,6 +155,31 @@ func TestPackageOriginNormalized(t *testing.T) { } } +// Hosts are case-insensitive. Two producers writing one host differently name +// the same place, and must not reconcile to a disagreement. +func TestPackageOriginHostCaseIsCanonical(t *testing.T) { + upper := RepositoryOrigin("https://GitHub.com/Owner/Repo", "aaaabbbbccccddddeeeeffff0000111122223333") + lower := RepositoryOrigin("https://github.com/Owner/Repo", "aaaabbbbccccddddeeeeffff0000111122223333") + + if upper == nil || lower == nil { + t.Fatal("both spellings should be publishable") + } + if upper.Repository != "https://github.com/Owner/Repo" { + t.Fatalf("repository = %q, want a lowercased host and an untouched path", upper.Repository) + } + if settled := ReconcileOrigin(upper, lower); settled.Empty() { + t.Fatal("host casing alone must not read as a disagreement") + } + + // The path is case-sensitive, so these are different locations. + if settled := ReconcileOrigin( + ArtifactOrigin("https://example.test/Pkg-1.0.0.tgz"), + ArtifactOrigin("https://example.test/pkg-1.0.0.tgz"), + ); !settled.Empty() { + t.Fatalf("origin = %+v, want a disagreement: the paths differ", settled) + } +} + func TestPackageOriginEmpty(t *testing.T) { var nilOrigin *PackageOrigin if !nilOrigin.Empty() { diff --git a/package.go b/package.go index 3b2a198..dec19fa 100644 --- a/package.go +++ b/package.go @@ -123,6 +123,52 @@ type PackageAttestation struct { Verified bool `json:"verified,omitempty"` } +// mergeAttestations folds incoming statements into p, keeping one record per +// distinct statement. Several components can attest to one package -- a build +// provenance statement from one, a publish signature from another -- so this +// unions rather than keeping whichever arrived first, the way vulnerabilities +// already do. When two records describe the same statement and either verified +// it, the merged record is verified: verification is a fact one component +// established, not an opinion. +func (p *Package) mergeAttestations(incoming []PackageAttestation) { + if len(incoming) == 0 { + return + } + index := make(map[attestationKey]int, len(p.Attestations)+len(incoming)) + for i, attestation := range p.Attestations { + index[attestation.key()] = i + } + for _, attestation := range incoming { + key := attestation.key() + existing, found := index[key] + if !found { + index[key] = len(p.Attestations) + p.Attestations = append(p.Attestations, attestation.Clone()) + continue + } + if attestation.Verified { + p.Attestations[existing].Verified = true + } + } +} + +// attestationKey identifies one statement for deduplication. +type attestationKey struct { + source string + predicateType string + url string + digest string +} + +// key returns a's deduplication identity. +func (a PackageAttestation) key() attestationKey { + key := attestationKey{source: a.Source, predicateType: a.PredicateType, url: a.URL} + if a.Digest != nil { + key.digest = string(a.Digest.Algorithm) + ":" + a.Digest.Value + } + return key +} + // Clone returns a deep copy. func (a PackageAttestation) Clone() PackageAttestation { clone := a @@ -404,12 +450,7 @@ func (p *Package) MergeFrom(src *Package) { // Two records of one package that disagree about where it came from settle // to no origin rather than to whichever was merged first. p.Origin = ReconcileOrigin(p.Origin, src.Origin) - if len(p.Attestations) == 0 && len(src.Attestations) > 0 { - p.Attestations = make([]PackageAttestation, 0, len(src.Attestations)) - for _, attestation := range src.Attestations { - p.Attestations = append(p.Attestations, attestation.Clone()) - } - } + p.mergeAttestations(src.Attestations) if len(p.CPEs) == 0 { p.CPEs = cloneStrings(src.CPEs) } diff --git a/provenance_test.go b/provenance_test.go index 340b306..5b7668b 100644 --- a/provenance_test.go +++ b/provenance_test.go @@ -110,3 +110,46 @@ func TestPackageAttestationsOmittedWhenEmpty(t *testing.T) { t.Fatalf("an empty attestation list must not be serialized: %s", raw) } } + +// Several components can attest to one package, so merging unions rather than +// keeping whichever arrived first. +func TestPackageMergeFromUnionsAttestations(t *testing.T) { + pkg := &Package{ + Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, + Attestations: []PackageAttestation{{Source: "provenance-matcher", PredicateType: "https://slsa.dev/provenance/v1", URL: "https://example.test/provenance"}}, + } + pkg.MergeFrom(&Package{ + Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, + Attestations: []PackageAttestation{ + {Source: "signature-matcher", PredicateType: "https://in-toto.io/attestation/release/v0.1", URL: "https://example.test/release"}, + // The same statement the first record already carries, now verified. + {Source: "provenance-matcher", PredicateType: "https://slsa.dev/provenance/v1", URL: "https://example.test/provenance", Verified: true}, + }, + }) + + if len(pkg.Attestations) != 2 { + t.Fatalf("attestations = %d, want both distinct statements: %+v", len(pkg.Attestations), pkg.Attestations) + } + if !pkg.Attestations[0].Verified { + t.Fatal("a statement another component verified must end up verified") + } + if pkg.Attestations[1].PredicateType != "https://in-toto.io/attestation/release/v0.1" { + t.Fatalf("second attestation = %+v, want the signature statement", pkg.Attestations[1]) + } +} + +// Statements differing only by digest are different statements. +func TestPackageMergeFromKeepsDistinctDigests(t *testing.T) { + pkg := &Package{ + Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, + Attestations: []PackageAttestation{{Source: "m", URL: "https://example.test/p", Digest: &Digest{Algorithm: DigestAlgorithmSHA256, Value: "aaa"}}}, + } + pkg.MergeFrom(&Package{ + Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, + Attestations: []PackageAttestation{{Source: "m", URL: "https://example.test/p", Digest: &Digest{Algorithm: DigestAlgorithmSHA256, Value: "bbb"}}}, + }) + + if len(pkg.Attestations) != 2 { + t.Fatalf("attestations = %d, want two: statements with different digests are different", len(pkg.Attestations)) + } +} From 5431c96b3e0a459a2496857ed23785f135a60cfe Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Tue, 18 Aug 2026 22:59:25 -0400 Subject: [PATCH 4/8] fix: bind verification to its issuer; canonicalize default ports - Attestation merging promoted Verified across records sharing a source, predicate, URL, and digest -- but not an issuer. A record naming issuer A could come out verified because issuer B verified something else, and a record with no issuer could come out verified with no identity attached, which is the one thing a verified claim must have. Records now merge only when their issuers are compatible: equal, or one unknown. Two named issuers are two statements, because they are two signers. - A digest's Subject is part of a statement's identity: the same bytes hashed over a source tree and over an artifact are different claims. - An explicit default port names the same origin as no port at all, so "https://host:443/pkg" and "https://host/pkg" no longer read as a disagreement and discard a good origin. IPv6 literals keep their brackets, and a non-default port stays part of the location. - Empty and Normalized now answer one question. Empty read the raw fields while Normalized re-validated them, so an origin carrying an unpublishable value was not Empty but normalized to nil -- and the obvious guard-then-read pattern dereferenced nil. The two synthetic credentials in the tests are labelled as such, so a lint gate added later does not trip over the fixtures that prove credentials are rejected. Co-Authored-By: Claude Opus 5 --- fuzz_test.go | 7 +++++- origin.go | 21 +++++++++++----- origin_test.go | 62 ++++++++++++++++++++++++++++++++++++++++++++-- package.go | 55 +++++++++++++++++++++++++++++----------- provenance_test.go | 56 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 177 insertions(+), 24 deletions(-) diff --git a/fuzz_test.go b/fuzz_test.go index 841a967..7a0a332 100644 --- a/fuzz_test.go +++ b/fuzz_test.go @@ -175,11 +175,16 @@ func FuzzPackageOrigin(f *testing.F) { // Reading back what was written must reach the same conclusion, and // reconciling a record with itself must not change it. assertPublishableOrigin(t, repository.Normalized()) + // Normalizing an already-normalized origin must be a fixed point. + if once, twice := repository.Normalized(), repository.Normalized().Normalized(); !sameOrigin(once, twice) { + t.Fatalf("normalizing twice changed the origin: %+v then %+v", once, twice) + } if settled, again := ReconcileOrigin(repository, repository), repository.Normalized(); !sameOrigin(settled, again) { t.Fatalf("reconciling a record with itself changed it: %+v then %+v", again, settled) } // A disagreement is recorded rather than resolved, whatever the inputs. - if other := ArtifactOrigin("https://registry.example.test/other/pkg-1.0.0.tgz"); !artifact.Empty() && artifact.Normalized().ArtifactURL != other.ArtifactURL { + normalized := artifact.Normalized() + if other := ArtifactOrigin("https://registry.example.test/other/pkg-1.0.0.tgz"); normalized != nil && normalized.ArtifactURL != other.ArtifactURL { if settled := ReconcileOrigin(artifact, other); !settled.Empty() { t.Fatalf("two different origins settled on %+v", settled) } diff --git a/origin.go b/origin.go index bd62f89..571e44c 100644 --- a/origin.go +++ b/origin.go @@ -107,6 +107,16 @@ func NormalizeOriginURL(raw string, repository bool) (string, bool) { // reconcile to a disagreement, losing an origin to formatting alone. The // path is left alone: it is case-sensitive. parsed.Host = strings.ToLower(parsed.Host) + // An explicit default port names the same origin as no port at all, so + // dropping it keeps two spellings of one location from reading as a + // disagreement. + if port := parsed.Port(); (parsed.Scheme == "https" && port == "443") || (parsed.Scheme == "http" && port == "80") { + host := parsed.Hostname() + if strings.Contains(host, ":") { + host = "[" + host + "]" // an IPv6 literal keeps its brackets + } + parsed.Host = host + } parsed.Fragment = "" parsed.RawFragment = "" // A host root names a server, not a package: a registry or index root on @@ -128,13 +138,12 @@ func NormalizeOriginURL(raw string, repository bool) (string, bool) { return normalized, true } -// Empty reports whether o names no location. A disputed origin is empty: the -// disagreement is recorded, but there is nothing to publish. +// Empty reports whether o names no publishable location. A disputed origin is +// empty -- the disagreement is recorded, but there is nothing to publish -- and +// so is one whose values do not survive validation, so a caller that checks +// Empty can read Normalized without a second nil check. func (o *PackageOrigin) Empty() bool { - if o == nil { - return true - } - return o.Disputed || (o.ArtifactURL == "" && o.Repository == "") + return o.Normalized() == nil } // Normalized returns o with every value re-validated, or nil when nothing diff --git a/origin_test.go b/origin_test.go index 4476dfc..681d001 100644 --- a/origin_test.go +++ b/origin_test.go @@ -16,7 +16,7 @@ func TestArtifactOrigin(t *testing.T) { {name: "checksum fragment is stripped", raw: "https://registry.npmjs.org/react/-/react-18.2.0.tgz#ceeba773e3e9d2b6f1a2b6b9f4f1cb2f9c2e1a55", want: "https://registry.npmjs.org/react/-/react-18.2.0.tgz"}, {name: "uppercase scheme is normalized", raw: "HTTPS://files.pythonhosted.org/packages/x/django-5.0.tar.gz", want: "https://files.pythonhosted.org/packages/x/django-5.0.tar.gz"}, {name: "signed link carrying a query", raw: "https://nexus.corp/repo/pkg.tgz?token=abc123"}, - {name: "embedded credentials", raw: "https://user:s3cret@nexus.corp/repo/pkg.tgz"}, + {name: "embedded credentials", raw: "https://user:s3cret@nexus.corp/repo/pkg.tgz"}, //nolint:gosec // synthetic credential; rejecting it is the rule under test {name: "registry root", raw: "https://registry.npmjs.org/"}, {name: "relative path", raw: "packages/lib"}, {name: "absolute local path", raw: "/Users/someone/src/project"}, @@ -82,7 +82,7 @@ func TestRepositoryOrigin(t *testing.T) { {name: "overlong revision", raw: "https://github.com/owner/repo", revision: strings.Repeat("a", 129), wantRepo: "https://github.com/owner/repo"}, {name: "bare host", raw: "https://github.com", revision: "9f8e7d6"}, {name: "index root", raw: "https://index.crates.io/", revision: "9f8e7d6"}, - {name: "credentialed remote", raw: "https://oauth2:glpat-xxxxxxxxxxxxxxxxxxxx@gitlab.corp/team/repo.git", revision: "9f8e7d6"}, + {name: "credentialed remote", raw: "https://oauth2:glpat-xxxxxxxxxxxxxxxxxxxx@gitlab.corp/team/repo.git", revision: "9f8e7d6"}, //nolint:gosec // synthetic credential; rejecting it is the rule under test {name: "ssh remote", raw: "ssh://github.com/owner/repo.git", revision: "9f8e7d6"}, {name: "local checkout", raw: "/Users/someone/src/repo", revision: "9f8e7d6"}, } @@ -180,6 +180,64 @@ func TestPackageOriginHostCaseIsCanonical(t *testing.T) { } } +// An explicit default port names the same origin as no port at all. +func TestPackageOriginDefaultPortIsCanonical(t *testing.T) { + cases := []struct { + name string + raw string + want string + }{ + {name: "https default port", raw: "https://example.test:443/pkg-1.0.0.tgz", want: "https://example.test/pkg-1.0.0.tgz"}, + {name: "http default port", raw: "http://example.test:80/pkg-1.0.0.tgz", want: "http://example.test/pkg-1.0.0.tgz"}, + {name: "a non-default port is part of the location", raw: "https://example.test:8443/pkg-1.0.0.tgz", want: "https://example.test:8443/pkg-1.0.0.tgz"}, + {name: "an IPv6 literal keeps its brackets", raw: "https://[2001:db8::1]:443/pkg-1.0.0.tgz", want: "https://[2001:db8::1]/pkg-1.0.0.tgz"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + origin := ArtifactOrigin(tc.raw) + if origin == nil { + t.Fatalf("origin = nil, want %q", tc.want) + } + if origin.ArtifactURL != tc.want { + t.Fatalf("artifact = %q, want %q", origin.ArtifactURL, tc.want) + } + }) + } + + if settled := ReconcileOrigin( + ArtifactOrigin("https://example.test:443/pkg-1.0.0.tgz"), + ArtifactOrigin("https://example.test/pkg-1.0.0.tgz"), + ); settled.Empty() { + t.Fatal("a default port alone must not read as a disagreement") + } + if settled := ReconcileOrigin( + ArtifactOrigin("https://example.test:8443/pkg-1.0.0.tgz"), + ArtifactOrigin("https://example.test/pkg-1.0.0.tgz"), + ); !settled.Empty() { + t.Fatalf("origin = %+v, want a disagreement: a non-default port is a different location", settled) + } +} + +// Empty and Normalized must answer one question, so a caller that guards on +// Empty can read Normalized without a second nil check. +func TestPackageOriginEmptyAgreesWithNormalized(t *testing.T) { + origins := []*PackageOrigin{ + nil, + {}, + {Disputed: true}, + {ArtifactURL: "https://registry.npmjs.org/react/-/react-18.2.0.tgz"}, + {ArtifactURL: "/Users/someone/pkg.tgz"}, + {Repository: "file:///home/someone/repo"}, + {Repository: "https://github.com/owner/repo", Revision: "feature@login"}, + {Revision: "9f8e7d6"}, + } + for _, origin := range origins { + if origin.Empty() != (origin.Normalized() == nil) { + t.Fatalf("Empty() and Normalized() disagree for %+v", origin) + } + } +} + func TestPackageOriginEmpty(t *testing.T) { var nilOrigin *PackageOrigin if !nilOrigin.Empty() { diff --git a/package.go b/package.go index dec19fa..3bf94d0 100644 --- a/package.go +++ b/package.go @@ -134,25 +134,48 @@ func (p *Package) mergeAttestations(incoming []PackageAttestation) { if len(incoming) == 0 { return } - index := make(map[attestationKey]int, len(p.Attestations)+len(incoming)) - for i, attestation := range p.Attestations { - index[attestation.key()] = i - } - for _, attestation := range incoming { - key := attestation.key() - existing, found := index[key] - if !found { - index[key] = len(p.Attestations) - p.Attestations = append(p.Attestations, attestation.Clone()) - continue + for _, candidate := range incoming { + merged := false + for i := range p.Attestations { + if !p.Attestations[i].describesSame(candidate) { + continue + } + p.Attestations[i].absorb(candidate) + merged = true + break } - if attestation.Verified { - p.Attestations[existing].Verified = true + if !merged { + p.Attestations = append(p.Attestations, candidate.Clone()) } } } -// attestationKey identifies one statement for deduplication. +// describesSame reports whether two records describe one statement. Their +// issuers must be compatible: equal, or one of them unknown. Two records naming +// different issuers are two statements -- different signers -- and folding them +// together would report one signer's verification under the other's name. +func (a PackageAttestation) describesSame(other PackageAttestation) bool { + if a.key() != other.key() { + return false + } + return a.Issuer == "" || other.Issuer == "" || a.Issuer == other.Issuer +} + +// absorb folds a record describing the same statement into a. An unknown +// issuer is filled from the other record, so a verified statement never ends up +// without the identity that makes the verification meaningful. +func (a *PackageAttestation) absorb(other PackageAttestation) { + if a.Issuer == "" { + a.Issuer = other.Issuer + } + if other.Verified { + a.Verified = true + } +} + +// attestationKey identifies one statement for deduplication. Issuer is +// deliberately absent: it is compared separately, because an unknown issuer is +// compatible with a known one while two known issuers are not. type attestationKey struct { source string predicateType string @@ -164,7 +187,9 @@ type attestationKey struct { func (a PackageAttestation) key() attestationKey { key := attestationKey{source: a.Source, predicateType: a.PredicateType, url: a.URL} if a.Digest != nil { - key.digest = string(a.Digest.Algorithm) + ":" + a.Digest.Value + // Subject is part of the identity: the same bytes hashed over a + // source tree and over an artifact are different claims. + key.digest = string(a.Digest.Algorithm) + ":" + a.Digest.Value + ":" + string(a.Digest.Subject) } return key } diff --git a/provenance_test.go b/provenance_test.go index 5b7668b..e53a2b3 100644 --- a/provenance_test.go +++ b/provenance_test.go @@ -153,3 +153,59 @@ func TestPackageMergeFromKeepsDistinctDigests(t *testing.T) { t.Fatalf("attestations = %d, want two: statements with different digests are different", len(pkg.Attestations)) } } + +// Verification belongs to whoever performed it. A record naming one issuer must +// never come out verified because a different issuer verified something else. +func TestPackageMergeFromKeepsVerificationWithItsIssuer(t *testing.T) { + statement := func(issuer string, verified bool) PackageAttestation { + return PackageAttestation{ + Source: "provenance-matcher", + PredicateType: "https://slsa.dev/provenance/v1", + URL: "https://example.test/provenance", + Issuer: issuer, + Verified: verified, + } + } + + t.Run("different issuers stay separate", func(t *testing.T) { + pkg := &Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("issuer-a", false)}} + pkg.MergeFrom(&Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("issuer-b", true)}}) + + if len(pkg.Attestations) != 2 { + t.Fatalf("attestations = %+v, want both issuers kept", pkg.Attestations) + } + for _, attestation := range pkg.Attestations { + if attestation.Issuer == "issuer-a" && attestation.Verified { + t.Fatal("issuer-a's statement was marked verified by issuer-b's verification") + } + } + }) + + t.Run("an unknown issuer is filled from the verified record", func(t *testing.T) { + pkg := &Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("", false)}} + pkg.MergeFrom(&Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("issuer-b", true)}}) + + if len(pkg.Attestations) != 1 { + t.Fatalf("attestations = %+v, want one record", pkg.Attestations) + } + if !pkg.Attestations[0].Verified || pkg.Attestations[0].Issuer != "issuer-b" { + t.Fatalf("attestation = %+v, want verified and attributed to issuer-b", pkg.Attestations[0]) + } + }) +} + +// Statements differing only by what their digest covers are different claims. +func TestPackageMergeFromKeepsDistinctDigestSubjects(t *testing.T) { + pkg := &Package{ + Coordinates: Coordinates{PURL: "pkg:golang/example.test/mod@1.0.0"}, + Attestations: []PackageAttestation{{Source: "m", URL: "https://example.test/p", Digest: &Digest{Algorithm: DigestAlgorithmSHA256, Value: "aaa"}}}, + } + pkg.MergeFrom(&Package{ + Coordinates: Coordinates{PURL: "pkg:golang/example.test/mod@1.0.0"}, + Attestations: []PackageAttestation{{Source: "m", URL: "https://example.test/p", Digest: &Digest{Algorithm: DigestAlgorithmSHA256, Value: "aaa", Subject: DigestSubjectSourceTree}}}, + }) + + if len(pkg.Attestations) != 2 { + t.Fatalf("attestations = %d, want two: one covers the artifact, one the source tree", len(pkg.Attestations)) + } +} From bc0e4d3627b0e130cfac0ac9465424d14277734f Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Tue, 18 Aug 2026 23:07:13 -0400 Subject: [PATCH 5/8] fix: never move a verification between issuers An issuerless verified record folded into one naming an issuer, reporting that issuer as verified on the strength of a check that recorded no signer -- and with several issuers, merge order decided which one received it. That is the mirror of the case fixed in the previous commit, and it was still reachable. Verification is a fact about a statement and a signer together, so records now fold only when they agree on the issuer, or when one of them claims nothing beyond the statement's existence -- no issuer and no verification -- in which case it is replaced wholesale rather than contributing a field. A verified record with no issuer is a real, weaker claim ("verified, signer unrecorded") and stays its own record. Tested in both merge orders, since order-independence is the property at stake. Co-Authored-By: Claude Opus 5 --- package.go | 47 +++++++++++++++++++++++++++++---------- provenance_test.go | 55 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 12 deletions(-) diff --git a/package.go b/package.go index 3bf94d0..84c743b 100644 --- a/package.go +++ b/package.go @@ -150,25 +150,48 @@ func (p *Package) mergeAttestations(incoming []PackageAttestation) { } } -// describesSame reports whether two records describe one statement. Their -// issuers must be compatible: equal, or one of them unknown. Two records naming -// different issuers are two statements -- different signers -- and folding them -// together would report one signer's verification under the other's name. +// describesSame reports whether two records can be folded into one. Verification +// is a fact about a statement *and a signer*, so records fold only when they +// agree on the issuer -- or when one of them claims nothing that could be +// misattributed. +// +// A record with no issuer and no verification says only that the statement +// exists, which any other record for it already says, so it folds into +// anything. A record with no issuer that *was* verified is a real claim ("this +// was verified, signer unrecorded") and stays separate from a record naming an +// issuer: merging them would report that issuer as verified on the strength of +// a verification that may have been of someone else's signature. func (a PackageAttestation) describesSame(other PackageAttestation) bool { if a.key() != other.key() { return false } - return a.Issuer == "" || other.Issuer == "" || a.Issuer == other.Issuer + switch { + case a.Issuer == other.Issuer: + return true + case a.claimsNothing(), other.claimsNothing(): + return true + default: + return false + } } -// absorb folds a record describing the same statement into a. An unknown -// issuer is filled from the other record, so a verified statement never ends up -// without the identity that makes the verification meaningful. +// claimsNothing reports whether a asserts anything beyond the statement's +// existence. +func (a PackageAttestation) claimsNothing() bool { + return a.Issuer == "" && !a.Verified +} + +// absorb folds a record describing the same statement into a. Verification +// never moves between issuers: it travels only when this record claims nothing, +// in which case the other record replaces it wholesale. func (a *PackageAttestation) absorb(other PackageAttestation) { - if a.Issuer == "" { - a.Issuer = other.Issuer - } - if other.Verified { + switch { + case a.claimsNothing(): + *a = other.Clone() + case other.claimsNothing(): + // Nothing to take. + case other.Verified: + // Same issuer, so the verification is this issuer's. a.Verified = true } } diff --git a/provenance_test.go b/provenance_test.go index e53a2b3..b388c27 100644 --- a/provenance_test.go +++ b/provenance_test.go @@ -181,6 +181,61 @@ func TestPackageMergeFromKeepsVerificationWithItsIssuer(t *testing.T) { } }) + // A verification recorded without a signer is its own claim. Attaching it + // to an issuer named by a different record would say that issuer's + // signature was checked, which nobody established. + t.Run("an issuerless verification is not attributed to a later issuer", func(t *testing.T) { + pkg := &Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("", true)}} + pkg.MergeFrom(&Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("issuer-a", false)}}) + + if len(pkg.Attestations) != 2 { + t.Fatalf("attestations = %+v, want the issuerless verification kept separate", pkg.Attestations) + } + for _, attestation := range pkg.Attestations { + if attestation.Issuer == "issuer-a" && attestation.Verified { + t.Fatal("issuer-a was reported verified on the strength of a verification that named no signer") + } + } + }) + + // The same, with the records arriving the other way round. + t.Run("merge order does not change the outcome", func(t *testing.T) { + pkg := &Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("issuer-a", false)}} + pkg.MergeFrom(&Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("", true)}}) + + if len(pkg.Attestations) != 2 { + t.Fatalf("attestations = %+v, want the issuerless verification kept separate", pkg.Attestations) + } + for _, attestation := range pkg.Attestations { + if attestation.Issuer == "issuer-a" && attestation.Verified { + t.Fatal("issuer-a was reported verified on the strength of a verification that named no signer") + } + } + }) + + // A record with no issuer and no verification asserts only that the + // statement exists, so it folds into one that says more. + t.Run("a record claiming nothing folds away", func(t *testing.T) { + for _, order := range []string{"weak first", "weak second"} { + t.Run(order, func(t *testing.T) { + weak, strong := statement("", false), statement("issuer-a", true) + first, second := weak, strong + if order == "weak second" { + first, second = strong, weak + } + pkg := &Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{first}} + pkg.MergeFrom(&Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{second}}) + + if len(pkg.Attestations) != 1 { + t.Fatalf("attestations = %+v, want one record", pkg.Attestations) + } + if !pkg.Attestations[0].Verified || pkg.Attestations[0].Issuer != "issuer-a" { + t.Fatalf("attestation = %+v, want verified and attributed to issuer-a", pkg.Attestations[0]) + } + }) + } + }) + t.Run("an unknown issuer is filled from the verified record", func(t *testing.T) { pkg := &Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("", false)}} pkg.MergeFrom(&Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("issuer-b", true)}}) From ffd159d57287b5121008aeee8737be5ae469f3b0 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Tue, 18 Aug 2026 23:07:38 -0400 Subject: [PATCH 6/8] test: cover verification promotion within one issuer A mutation check found the branch unguarded: removing the same-issuer verification promotion failed no test, because every existing case went through the claims-nothing path instead. Two records naming one issuer, one of them verified, now assert the merged record is verified. Co-Authored-By: Claude Opus 5 --- provenance_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/provenance_test.go b/provenance_test.go index b388c27..1b765a1 100644 --- a/provenance_test.go +++ b/provenance_test.go @@ -236,6 +236,20 @@ func TestPackageMergeFromKeepsVerificationWithItsIssuer(t *testing.T) { } }) + // One issuer, two records: verification is additive, because both records + // speak about the same signer. + t.Run("one issuer verified in a later record", func(t *testing.T) { + pkg := &Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("issuer-a", false)}} + pkg.MergeFrom(&Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("issuer-a", true)}}) + + if len(pkg.Attestations) != 1 { + t.Fatalf("attestations = %+v, want one record for one issuer", pkg.Attestations) + } + if !pkg.Attestations[0].Verified || pkg.Attestations[0].Issuer != "issuer-a" { + t.Fatalf("attestation = %+v, want issuer-a verified", pkg.Attestations[0]) + } + }) + t.Run("an unknown issuer is filled from the verified record", func(t *testing.T) { pkg := &Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("", false)}} pkg.MergeFrom(&Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{statement("issuer-b", true)}}) From 8aeb70bb7f2b65480db76e6fdc4652feda538910 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Tue, 18 Aug 2026 23:17:14 -0400 Subject: [PATCH 7/8] fix: enforce the origin rule at the wire, union digests, canonicalize escapes - The rule was enforced on read, which meant an origin built by hand or by an external component could be stored, forwarded to the next plugin, and written back out without ever passing it. It now applies on unmarshal and on marshal, so a location that would be rejected on read cannot enter or leave the process. A recorded disagreement survives the trip: it is not a location, but it is a fact worth keeping. - Digests union instead of keeping whichever record merged first. With Subject, two records can carry genuinely different claims -- a hash of the artifact from one source, a hash over the source tree from another -- and dropping a later slice would lose provenance on merge order. - Escaped unreserved characters are decoded and remaining escapes written in one hex case, because RFC 3986 makes "%7Euser" and "~user" the same path; without this they reconciled to a disagreement and a valid origin was lost to formatting. The escape work canonicalizes the *escaped* form rather than parsed.Path. Path is already decoded, where "%2F" and "/" are indistinguishable, so re-encoding from it turned an escaped slash into a path separator and silently changed the location -- caught by a test asserting an escaped slash stays escaped. Co-Authored-By: Claude Opus 5 --- origin.go | 116 +++++++++++++++++++++++++++++++++++++++++++++ origin_test.go | 93 ++++++++++++++++++++++++++++++++++++ package.go | 23 +++++++++ provenance_test.go | 22 +++++++++ 4 files changed, 254 insertions(+) diff --git a/origin.go b/origin.go index 571e44c..2d702d8 100644 --- a/origin.go +++ b/origin.go @@ -1,6 +1,7 @@ package sdk import ( + "encoding/json" "net/url" "strings" ) @@ -131,6 +132,16 @@ func NormalizeOriginURL(raw string, repository bool) (string, bool) { } else if parsed.RawQuery != "" || parsed.ForceQuery { return "", false } + // Canonicalize the escaped form, not parsed.Path: Path is already decoded, + // where "%2F" and "/" are indistinguishable, and re-encoding from it would + // turn an escaped slash into a path separator and change the location. + escaped := canonicalEscapes(parsed.EscapedPath()) + decodedPath, err := url.PathUnescape(escaped) + if err != nil { + return "", false + } + parsed.Path = decodedPath + parsed.RawPath = escaped normalized := parsed.String() if normalized == "" { return "", false @@ -138,6 +149,71 @@ func NormalizeOriginURL(raw string, repository bool) (string, bool) { return normalized, true } +// canonicalEscapes rewrites a path so two spellings of one location compare +// equal. RFC 3986 says a percent-escaped unreserved character means the same as +// the character itself, so "%7Euser" and "~user" name one path; without this +// they would reconcile to a disagreement and lose a valid origin to formatting +// alone. Reserved characters keep their escapes, since there the escape changes +// what the path means, but their hex is written one way. +func canonicalEscapes(path string) string { + if !strings.Contains(path, "%") { + return path + } + var out strings.Builder + out.Grow(len(path)) + for i := 0; i < len(path); i++ { + if path[i] != '%' || i+2 >= len(path) { + out.WriteByte(path[i]) + continue + } + decoded, ok := unhex(path[i+1], path[i+2]) + if !ok { + out.WriteByte(path[i]) + continue + } + if isUnreservedByte(decoded) { + out.WriteByte(decoded) + } else { + out.WriteString("%") + out.WriteString(strings.ToUpper(path[i+1 : i+3])) + } + i += 2 + } + return out.String() +} + +// unhex decodes one percent-escape pair. +func unhex(high, low byte) (byte, bool) { + value := 0 + for _, digit := range []byte{high, low} { + value <<= 4 + switch { + case digit >= '0' && digit <= '9': + value |= int(digit - '0') + case digit >= 'a' && digit <= 'f': + value |= int(digit-'a') + 10 + case digit >= 'A' && digit <= 'F': + value |= int(digit-'A') + 10 + default: + return 0, false + } + } + return byte(value), true +} + +// isUnreservedByte reports whether b is unreserved in RFC 3986, meaning its +// escaped and unescaped spellings are equivalent. +func isUnreservedByte(b byte) bool { + switch { + case b >= 'a' && b <= 'z', b >= 'A' && b <= 'Z', b >= '0' && b <= '9': + return true + case b == '-', b == '.', b == '_', b == '~': + return true + default: + return false + } +} + // Empty reports whether o names no publishable location. A disputed origin is // empty -- the disagreement is recorded, but there is nothing to publish -- and // so is one whose values do not survive validation, so a caller that checks @@ -169,6 +245,46 @@ func (o *PackageOrigin) Normalized() *PackageOrigin { return normalized } +// originWire carries PackageOrigin's fields without its methods, so the JSON +// hooks below can encode and decode without recursing. +type originWire PackageOrigin + +// UnmarshalJSON applies the origin rule as a value arrives, so a location that +// would be rejected on read cannot be stored, forwarded to another component, +// or written back out. A record of a disagreement survives decoding: it is not +// a location, but it is a fact worth keeping. +func (o *PackageOrigin) UnmarshalJSON(data []byte) error { + var wire originWire + if err := json.Unmarshal(data, &wire); err != nil { + return err + } + decoded := PackageOrigin(wire) + switch { + case decoded.Disputed: + *o = PackageOrigin{Disputed: true} + default: + normalized := decoded.Normalized() + if normalized == nil { + *o = PackageOrigin{} + return nil + } + *o = *normalized + } + return nil +} + +// MarshalJSON applies the same rule on the way out, so a hand-built value that +// never passed through the constructors cannot leave this process either. +func (o PackageOrigin) MarshalJSON() ([]byte, error) { + if o.Disputed { + return json.Marshal(originWire{Disputed: true}) + } + if normalized := o.Normalized(); normalized != nil { + return json.Marshal(originWire(*normalized)) + } + return json.Marshal(originWire{}) +} + // Clone returns a deep copy. func (o *PackageOrigin) Clone() *PackageOrigin { if o == nil { diff --git a/origin_test.go b/origin_test.go index 681d001..d270a6b 100644 --- a/origin_test.go +++ b/origin_test.go @@ -438,3 +438,96 @@ func TestPackageOriginWireRoundTrip(t *testing.T) { t.Fatalf("an absent origin must not be serialized: %s (err %v)", raw, err) } } + +// RFC 3986: an escaped unreserved character means the same as the character, +// so two spellings of one path must not read as a disagreement. Reserved +// characters keep their escapes, because there the escape changes the meaning. +func TestPackageOriginPercentEncodingIsCanonical(t *testing.T) { + cases := []struct{ name, raw, want string }{ + {name: "escaped tilde", raw: "https://example.test/pkg/%7Euser/a.tgz", want: "https://example.test/pkg/~user/a.tgz"}, + {name: "escaped letter", raw: "https://example.test/%70kg/a.tgz", want: "https://example.test/pkg/a.tgz"}, + {name: "lowercase hex is uppercased", raw: "https://example.test/pkg/a%2fb.tgz", want: "https://example.test/pkg/a%2Fb.tgz"}, + {name: "a reserved escape keeps its meaning", raw: "https://example.test/pkg/a%2Fb.tgz", want: "https://example.test/pkg/a%2Fb.tgz"}, + {name: "a space stays escaped", raw: "https://example.test/pkg/a%20b.tgz", want: "https://example.test/pkg/a%20b.tgz"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + origin := ArtifactOrigin(tc.raw) + if origin == nil { + t.Fatalf("origin = nil, want %q", tc.want) + } + if origin.ArtifactURL != tc.want { + t.Fatalf("artifact = %q, want %q", origin.ArtifactURL, tc.want) + } + }) + } + + // A malformed escape is not a location anything can fetch. + if origin := ArtifactOrigin("https://example.test/pkg/100%.tgz"); origin != nil { + t.Fatalf("origin = %+v, want nil for a malformed escape", origin) + } + + if settled := ReconcileOrigin( + ArtifactOrigin("https://example.test/pkg/%7Euser/a.tgz"), + ArtifactOrigin("https://example.test/pkg/~user/a.tgz"), + ); settled.Empty() { + t.Fatal("two spellings of one path must not read as a disagreement") + } + if settled := ReconcileOrigin( + ArtifactOrigin("https://example.test/pkg/a%2Fb.tgz"), + ArtifactOrigin("https://example.test/pkg/a/b.tgz"), + ); !settled.Empty() { + t.Fatalf("origin = %+v, want a disagreement: an escaped slash is a different path", settled) + } +} + +// A value that would be rejected on read must not be storable or forwardable. +func TestPackageOriginNormalizesAcrossJSON(t *testing.T) { + cases := []struct { + name string + raw string + want PackageOrigin + }{ + {name: "credentialed artifact is dropped", raw: `{"artifact_url":"https://build:s3cret@nexus.corp/pkg.tgz"}`}, + {name: "local path is dropped", raw: `{"repository":"file:///home/someone/repo"}`}, + {name: "revision without a repository is dropped", raw: `{"revision":"9f8e7d6"}`}, + { + name: "a publishable value survives", + raw: `{"artifact_url":"https://registry.npmjs.org/react/-/react-18.2.0.tgz"}`, + want: PackageOrigin{ArtifactURL: "https://registry.npmjs.org/react/-/react-18.2.0.tgz"}, + }, + { + name: "host casing is canonicalized in transit", + raw: `{"repository":"https://GitHub.com/Owner/Repo","revision":"aaaabbbbccccddddeeeeffff0000111122223333"}`, + want: PackageOrigin{Repository: "https://github.com/Owner/Repo", Revision: "aaaabbbbccccddddeeeeffff0000111122223333"}, + }, + {name: "a recorded disagreement survives", raw: `{"disputed":true}`, want: PackageOrigin{Disputed: true}}, + { + name: "a disagreement outranks any value carried with it", + raw: `{"disputed":true,"artifact_url":"https://registry.npmjs.org/react/-/react-18.2.0.tgz"}`, + want: PackageOrigin{Disputed: true}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var decoded PackageOrigin + if err := json.Unmarshal([]byte(tc.raw), &decoded); err != nil { + t.Fatalf("decode: %v", err) + } + if decoded != tc.want { + t.Fatalf("decoded = %+v, want %+v", decoded, tc.want) + } + }) + } + + // The same rule applies leaving the process, so a hand-built value cannot + // be written out either. + raw, err := json.Marshal(&PackageOrigin{ArtifactURL: "https://build:s3cret@nexus.corp/pkg.tgz"}) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "s3cret") { + t.Fatalf("marshaled %s, which carries a credential", raw) + } +} diff --git a/package.go b/package.go index 84c743b..30678fa 100644 --- a/package.go +++ b/package.go @@ -123,6 +123,28 @@ type PackageAttestation struct { Verified bool `json:"verified,omitempty"` } +// mergeDigests unions digests rather than keeping whichever record arrived +// first. Two records can carry genuinely different claims about one package -- +// a hash of the published artifact from one source and a hash over the source +// tree from another -- and Subject is what tells them apart, so dropping a +// later slice would lose provenance on merge order alone. +func (p *Package) mergeDigests(incoming []Digest) { + if len(incoming) == 0 { + return + } + seen := make(map[Digest]struct{}, len(p.Digests)+len(incoming)) + for _, digest := range p.Digests { + seen[digest] = struct{}{} + } + for _, digest := range incoming { + if _, found := seen[digest]; found { + continue + } + seen[digest] = struct{}{} + p.Digests = append(p.Digests, digest) + } +} + // mergeAttestations folds incoming statements into p, keeping one record per // distinct statement. Several components can attest to one package -- a build // provenance statement from one, a publish signature from another -- so this @@ -502,6 +524,7 @@ func (p *Package) MergeFrom(src *Package) { if len(p.CPEs) == 0 { p.CPEs = cloneStrings(src.CPEs) } + p.mergeDigests(src.Digests) if len(p.Digests) == 0 && len(src.Digests) > 0 { p.Digests = append([]Digest(nil), src.Digests...) } diff --git a/provenance_test.go b/provenance_test.go index 1b765a1..4d4cb43 100644 --- a/provenance_test.go +++ b/provenance_test.go @@ -278,3 +278,25 @@ func TestPackageMergeFromKeepsDistinctDigestSubjects(t *testing.T) { t.Fatalf("attestations = %d, want two: one covers the artifact, one the source tree", len(pkg.Attestations)) } } + +// Two sources can carry different claims about one package: a hash of the +// published artifact from one, a hash over the source tree from another. +// Keeping only the first slice would lose provenance on merge order alone. +func TestPackageMergeFromUnionsDigests(t *testing.T) { + artifact := Digest{Algorithm: DigestAlgorithmSHA256, Value: "aaa"} + sourceTree := Digest{Algorithm: DigestAlgorithmSHA256, Value: "aaa", Subject: DigestSubjectSourceTree} + + pkg := &Package{Coordinates: Coordinates{PURL: "pkg:golang/example.test/mod@1.0.0"}, Digests: []Digest{artifact}} + pkg.MergeFrom(&Package{Coordinates: Coordinates{PURL: "pkg:golang/example.test/mod@1.0.0"}, Digests: []Digest{sourceTree, artifact}}) + + if len(pkg.Digests) != 2 { + t.Fatalf("digests = %+v, want both claims kept and the repeat dropped", pkg.Digests) + } + var subjects []DigestSubject + for _, digest := range pkg.Digests { + subjects = append(subjects, digest.Subject) + } + if subjects[0] != DigestSubjectArtifact || subjects[1] != DigestSubjectSourceTree { + t.Fatalf("subjects = %v, want the artifact claim then the source-tree claim", subjects) + } +} From cac6e5cc9d8674dfa22190990a4ba8d867981f0f Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Tue, 18 Aug 2026 23:25:57 -0400 Subject: [PATCH 8/8] fix: reject unusable ports; key attestations on separate digest parts - url.Parse only checks that a port is numeric, so "https://host:99999/pkg" reached the constructors and was published as a location no client can connect to. Ports outside 1-65535 are now rejected. - The attestation key joined algorithm, value, and subject with a separator. Plugin-supplied values can contain that separator, so two different digests could produce one key and fold distinct statements together -- promoting Verified onto the wrong one. The parts are stored separately. Co-Authored-By: Claude Opus 5 --- origin.go | 9 +++++++++ origin_test.go | 12 ++++++++++++ package.go | 22 +++++++++++++++------- provenance_test.go | 17 +++++++++++++++++ 4 files changed, 53 insertions(+), 7 deletions(-) diff --git a/origin.go b/origin.go index 2d702d8..8704efe 100644 --- a/origin.go +++ b/origin.go @@ -3,6 +3,7 @@ package sdk import ( "encoding/json" "net/url" + "strconv" "strings" ) @@ -111,6 +112,14 @@ func NormalizeOriginURL(raw string, repository bool) (string, bool) { // An explicit default port names the same origin as no port at all, so // dropping it keeps two spellings of one location from reading as a // disagreement. + if port := parsed.Port(); port != "" { + // url.Parse only checks that a port is numeric, so a value no client + // could connect to still reaches here. + number, err := strconv.Atoi(port) + if err != nil || number < 1 || number > 65535 { + return "", false + } + } if port := parsed.Port(); (parsed.Scheme == "https" && port == "443") || (parsed.Scheme == "http" && port == "80") { host := parsed.Hostname() if strings.Contains(host, ":") { diff --git a/origin_test.go b/origin_test.go index d270a6b..ebc32de 100644 --- a/origin_test.go +++ b/origin_test.go @@ -190,6 +190,7 @@ func TestPackageOriginDefaultPortIsCanonical(t *testing.T) { {name: "https default port", raw: "https://example.test:443/pkg-1.0.0.tgz", want: "https://example.test/pkg-1.0.0.tgz"}, {name: "http default port", raw: "http://example.test:80/pkg-1.0.0.tgz", want: "http://example.test/pkg-1.0.0.tgz"}, {name: "a non-default port is part of the location", raw: "https://example.test:8443/pkg-1.0.0.tgz", want: "https://example.test:8443/pkg-1.0.0.tgz"}, + {name: "the highest usable port", raw: "https://example.test:65535/pkg-1.0.0.tgz", want: "https://example.test:65535/pkg-1.0.0.tgz"}, {name: "an IPv6 literal keeps its brackets", raw: "https://[2001:db8::1]:443/pkg-1.0.0.tgz", want: "https://[2001:db8::1]/pkg-1.0.0.tgz"}, } for _, tc := range cases { @@ -462,6 +463,17 @@ func TestPackageOriginPercentEncodingIsCanonical(t *testing.T) { }) } + // url.Parse accepts any numeric port, but nothing can connect to these. + for _, raw := range []string{ + "https://example.test:99999/pkg-1.0.0.tgz", + "https://example.test:0/pkg-1.0.0.tgz", + "https://example.test:65536/pkg-1.0.0.tgz", + } { + if origin := ArtifactOrigin(raw); origin != nil { + t.Errorf("origin = %+v for %q, want nil: the port is outside the usable range", origin, raw) + } + } + // A malformed escape is not a location anything can fetch. if origin := ArtifactOrigin("https://example.test/pkg/100%.tgz"); origin != nil { t.Fatalf("origin = %+v, want nil for a malformed escape", origin) diff --git a/package.go b/package.go index 30678fa..6104ac8 100644 --- a/package.go +++ b/package.go @@ -222,19 +222,27 @@ func (a *PackageAttestation) absorb(other PackageAttestation) { // deliberately absent: it is compared separately, because an unknown issuer is // compatible with a known one while two known issuers are not. type attestationKey struct { - source string - predicateType string - url string - digest string + source string + predicateType string + url string + digestAlgorithm DigestAlgorithm + digestValue string + digestSubject DigestSubject } // key returns a's deduplication identity. func (a PackageAttestation) key() attestationKey { key := attestationKey{source: a.Source, predicateType: a.PredicateType, url: a.URL} if a.Digest != nil { - // Subject is part of the identity: the same bytes hashed over a - // source tree and over an artifact are different claims. - key.digest = string(a.Digest.Algorithm) + ":" + a.Digest.Value + ":" + string(a.Digest.Subject) + // The three parts stay separate rather than joined: plugin-supplied + // values can contain the separator, and joining lets two different + // digests produce one key. + // + // Subject is part of the identity: the same bytes hashed over a source + // tree and over an artifact are different claims. + key.digestAlgorithm = a.Digest.Algorithm + key.digestValue = a.Digest.Value + key.digestSubject = a.Digest.Subject } return key } diff --git a/provenance_test.go b/provenance_test.go index 4d4cb43..d3748d1 100644 --- a/provenance_test.go +++ b/provenance_test.go @@ -300,3 +300,20 @@ func TestPackageMergeFromUnionsDigests(t *testing.T) { t.Fatalf("subjects = %v, want the artifact claim then the source-tree claim", subjects) } } + +// Plugin-supplied digest fields can contain anything, including whatever +// separator a joined key would use, so the key holds the parts separately. +func TestPackageMergeFromDistinguishesDigestsThatWouldCollide(t *testing.T) { + left := PackageAttestation{Source: "m", URL: "https://example.test/s", Digest: &Digest{Algorithm: DigestAlgorithmSHA256, Value: "a:b", Subject: "c"}} + right := PackageAttestation{Source: "m", URL: "https://example.test/s", Digest: &Digest{Algorithm: DigestAlgorithmSHA256, Value: "a", Subject: "b:c"}, Verified: true} + + pkg := &Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{left}} + pkg.MergeFrom(&Package{Coordinates: Coordinates{PURL: "pkg:npm/react@18.2.0"}, Attestations: []PackageAttestation{right}}) + + if len(pkg.Attestations) != 2 { + t.Fatalf("attestations = %+v, want two: the digests differ", pkg.Attestations) + } + if pkg.Attestations[0].Verified { + t.Fatal("verification was promoted onto a statement with a different digest") + } +}