From 3dc06188523afd464abba7bf374300629ca9260b Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Fri, 21 Aug 2026 01:01:59 -0400 Subject: [PATCH 1/2] =?UTF-8?q?refactor:=20DependencyOrigin=20=E2=80=94=20?= =?UTF-8?q?one=20concept,=20no=20merging,=20clear=20siblings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A dependency origin (asserted by a manifest, distilled by a detector) is not a package source (resolved from a registry during enrichment). The model now says so structurally rather than by convention: - PackageOrigin is renamed DependencyOrigin and lives only on Dependency. The name follows the two standards that record this concept as a structured value: Go modules' Origin (URL, ref, hash) and PEP 610's "Direct URL Origin". Manifests variously say "resolved", "resolution", or "source"; "source" is taken by the DependencySource kind enum, and "resolution" collides with version-solving vocabulary. - Package.Origin is gone. A registry package is one occurrence per PURL and position-free; the only enrichment-derived source repository remains PackageScorecard.Repository, which was always distinctly named. The registry seeding helper no longer touches origin. - ReconcileOrigin and the Disputed flag are gone. Merging origins was machinery for a case the model does not represent: occurrences that genuinely differ are different graph positions, not a conflict to resolve. Graph merging now fills gaps -- locations union, relationship merges, and an origin fills in from whichever record has one, so it survives regardless of manifest order -- and on a genuine conflict the existing record's origin stays, deterministically. Same fill-if-empty convention Package.MergeFrom has always used. - ResolvedURL stays a sibling field on both types, documented for what it is: the manifest's resolution field verbatim on Dependency (raw evidence the origin is distilled from, never published), and detection-time transport for matchers on Package. - Digest/DigestSubject and PackageAttestation move to their own files (digest.go, attestation.go), joining Coordinates as shared types with a file each. Pure moves. Wire compatibility: the origin JSON key is unchanged; a payload carrying the removed "disputed" field decodes as its remaining values (unknown fields are ignored), and old binaries never emitted values alongside it. The nine plugin repos pin SDK v0.3.0 and reference none of this API. Co-Authored-By: Claude Fable 5 --- attestation.go | 166 +++++++++++++++ container.go | 12 +- dependency.go | 16 +- digest.go | 30 +++ fuzz_test.go | 21 +- matcherkit/registry.go | 16 +- matcherkit/registry_origin_test.go | 72 ------- origin.go | 119 ++++------- origin_test.go | 326 +++++++++++------------------ package.go | 210 +------------------ 10 files changed, 391 insertions(+), 597 deletions(-) create mode 100644 attestation.go create mode 100644 digest.go delete mode 100644 matcherkit/registry_origin_test.go diff --git a/attestation.go b/attestation.go new file mode 100644 index 0000000..4b98f27 --- /dev/null +++ b/attestation.go @@ -0,0 +1,166 @@ +package sdk + +// 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"` +} + +// 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 +// 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 + } + 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 !merged { + p.Attestations = append(p.Attestations, candidate.Clone()) + } + } +} + +// 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 + } + switch { + case a.Issuer == other.Issuer: + return true + case a.claimsNothing(), other.claimsNothing(): + return true + default: + return false + } +} + +// 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) { + 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 + } +} + +// 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 + 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 { + // 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 +} + +// Clone returns a deep copy. +func (a PackageAttestation) Clone() PackageAttestation { + clone := a + if a.Digest != nil { + digest := *a.Digest + clone.Digest = &digest + } + return clone +} diff --git a/container.go b/container.go index 3703237..c2b431d 100644 --- a/container.go +++ b/container.go @@ -170,10 +170,14 @@ 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) + // Merging fills gaps rather than resolving conflicts: locations + // union, and an origin fills in from whichever record has one, so + // it survives regardless of manifest order. Where both records + // assert different origins -- rare -- the existing one stays, + // deterministically. + if existing.Origin == nil { + existing.Origin = clone.Origin + } } return nil } diff --git a/dependency.go b/dependency.go index 34ec3e1..8c7f828 100644 --- a/dependency.go +++ b/dependency.go @@ -93,11 +93,17 @@ type Dependency struct { Digests []Digest `json:"digests,omitempty"` Copyright string `json:"copyright,omitempty"` FoundBy string `json:"found_by,omitempty"` - ResolvedURL string `json:"resolved_url,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"` + // ResolvedURL is the manifest's resolution field verbatim -- it may be a + // pseudo-URL, a registry or index root, or a local path, and is never + // published. It is raw evidence: Origin is the validated assertion + // distilled from it and from the manifest's other source fields. + ResolvedURL string `json:"resolved_url,omitempty"` + // Origin is where this dependency was resolved from, distilled by the + // detector from the manifest's structured source fields. Read it through + // Origin.Normalized(): ResolvedURL is the raw evidence, Origin the + // validated assertion, Normalized() the view consumers publish. + Origin *DependencyOrigin `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"` diff --git a/digest.go b/digest.go new file mode 100644 index 0000000..f916316 --- /dev/null +++ b/digest.go @@ -0,0 +1,30 @@ +package sdk + +// Digest captures integrity information for a package artifact. +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" +) diff --git a/fuzz_test.go b/fuzz_test.go index 7a0a332..f7ebb9c 100644 --- a/fuzz_test.go +++ b/fuzz_test.go @@ -150,10 +150,10 @@ func requireFuzzGraphValid(t *testing.T, graph *Graph) { }) } -// FuzzPackageOrigin drives the origin rule with arbitrary lockfile-derived +// FuzzDependencyOrigin 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) { +func FuzzDependencyOrigin(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") @@ -172,29 +172,18 @@ func FuzzPackageOrigin(f *testing.F) { 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. + // Reading back what was written must reach the same conclusion. 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. - 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) - } - } }) } // assertPublishableOrigin fails when an origin carries anything a published // document must never show. -func assertPublishableOrigin(t *testing.T, origin *PackageOrigin) { +func assertPublishableOrigin(t *testing.T, origin *DependencyOrigin) { t.Helper() normalized := origin.Normalized() if normalized == nil { @@ -242,7 +231,7 @@ func assertPublishableOrigin(t *testing.T, origin *PackageOrigin) { } // sameOrigin compares two origins that may be nil. -func sameOrigin(left, right *PackageOrigin) bool { +func sameOrigin(left, right *DependencyOrigin) bool { switch { case left == nil && right == nil: return true diff --git a/matcherkit/registry.go b/matcherkit/registry.go index dce8548..6f7df2c 100644 --- a/matcherkit/registry.go +++ b/matcherkit/registry.go @@ -33,21 +33,15 @@ func RegistryPackagesForGraph(g *sdk.Graph, reg *sdk.PackageRegistry, target *sd } dep.PackageRef = purl - 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{}{} + + pkg, ok := reg.Get(purl) + if !ok { + pkg = reg.Add(sdk.PackageFromDependency(dep)) + } if pkg != nil { out = append(out, pkg) } diff --git a/matcherkit/registry_origin_test.go b/matcherkit/registry_origin_test.go deleted file mode 100644 index 6d13e77..0000000 --- a/matcherkit/registry_origin_test.go +++ /dev/null @@ -1,72 +0,0 @@ -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 f5f4500..78d8fcc 100644 --- a/origin.go +++ b/origin.go @@ -11,16 +11,23 @@ import ( // 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. +// DependencyOrigin is where a dependency was resolved from, as asserted by the +// manifest the detector read. It is distilled at detection time from the +// manifest's structured source fields -- not derivable later from the raw +// ResolvedURL, which merges several fields and loses their meaning. The name +// follows the two standards that record this concept as a structured value: +// Go modules' Origin (URL, ref, hash) and PEP 610's "Direct URL Origin". // -// 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 { +// A dependency has one origin: either it was downloaded as an artifact or it +// was resolved from a repository, never both. An empty origin means the +// manifest had nothing publishable to say, which is the normal case for a +// dependency whose lockfile records only a registry or index root. Consumers +// such as SBOM export should publish nothing rather than guess. +// +// This is detection data. Registry-side enrichment that resolves a source +// repository from package identity is a different, weaker claim and lives on +// its own fields (for example PackageScorecard.Repository), never here. +type DependencyOrigin 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. @@ -28,39 +35,30 @@ type PackageOrigin struct { // 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 { +func ArtifactOrigin(rawURL string) *DependencyOrigin { normalized, ok := NormalizeOriginURL(rawURL, false) if !ok { return nil } - return &PackageOrigin{ArtifactURL: normalized} + return &DependencyOrigin{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 { +func RepositoryOrigin(rawURL, revision string) *DependencyOrigin { normalized, ok := NormalizeOriginURL(rawURL, true) if !ok { return nil } - origin := &PackageOrigin{Repository: normalized} + origin := &DependencyOrigin{Repository: normalized} if pinned := strings.TrimSpace(revision); isValidOriginRevision(pinned) { origin.Revision = pinned } @@ -233,11 +231,10 @@ func isUnreservedByte(b byte) bool { } } -// 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 { +// Empty reports whether o names no publishable location -- including an origin +// whose values do not survive validation, so a caller that checks Empty can +// read Normalized without a second nil check. +func (o *DependencyOrigin) Empty() bool { return o.Normalized() == nil } @@ -246,58 +243,50 @@ func (o *PackageOrigin) Empty() bool { // 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 { +func (o *DependencyOrigin) Normalized() *DependencyOrigin { + if o == nil { return nil } if artifact, ok := NormalizeOriginURL(o.ArtifactURL, false); ok { - return &PackageOrigin{ArtifactURL: artifact} + return &DependencyOrigin{ArtifactURL: artifact} } repository, ok := NormalizeOriginURL(o.Repository, true) if !ok { return nil } - normalized := &PackageOrigin{Repository: repository} + normalized := &DependencyOrigin{Repository: repository} if pinned := strings.TrimSpace(o.Revision); isValidOriginRevision(pinned) { normalized.Revision = pinned } return normalized } -// originWire carries PackageOrigin's fields without its methods, so the JSON +// originWire carries DependencyOrigin's fields without its methods, so the JSON // hooks below can encode and decode without recursing. -type originWire PackageOrigin +type originWire DependencyOrigin // 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 { +func (o *DependencyOrigin) 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 + decoded := DependencyOrigin(wire) + normalized := decoded.Normalized() + if normalized == nil { + *o = DependencyOrigin{} + 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}) - } +func (o DependencyOrigin) MarshalJSON() ([]byte, error) { if normalized := o.Normalized(); normalized != nil { return json.Marshal(originWire(*normalized)) } @@ -305,7 +294,7 @@ func (o PackageOrigin) MarshalJSON() ([]byte, error) { } // Clone returns a deep copy. -func (o *PackageOrigin) Clone() *PackageOrigin { +func (o *DependencyOrigin) Clone() *DependencyOrigin { if o == nil { return nil } @@ -313,36 +302,6 @@ func (o *PackageOrigin) Clone() *PackageOrigin { 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 diff --git a/origin_test.go b/origin_test.go index b5869a6..f41a904 100644 --- a/origin_test.go +++ b/origin_test.go @@ -6,6 +6,17 @@ import ( "testing" ) +// sameLocation reports whether two origins normalize to one location. With no +// merge logic in the model, "two spellings of one place" is expressed as +// canonical-form equality rather than as a reconciliation outcome. +func sameLocation(left, right *DependencyOrigin) bool { + l, r := left.Normalized(), right.Normalized() + if l == nil || r == nil { + return l == r + } + return *l == *r +} + func TestArtifactOrigin(t *testing.T) { cases := []struct { name string @@ -111,32 +122,31 @@ func TestRepositoryOrigin(t *testing.T) { // 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) { +func TestDependencyOriginNormalized(t *testing.T) { cases := []struct { name string - origin *PackageOrigin - want *PackageOrigin + origin *DependencyOrigin + want *DependencyOrigin }{ {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: "empty", origin: &DependencyOrigin{}}, + {name: "credentialed artifact", origin: &DependencyOrigin{ArtifactURL: "https://build:s3cret@nexus.corp/pkg.tgz"}}, + {name: "local repository", origin: &DependencyOrigin{Repository: "file:///home/someone/repo"}}, + {name: "revision without a repository", origin: &DependencyOrigin{Revision: "9f8e7d6"}}, { 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"}, + origin: &DependencyOrigin{ArtifactURL: "https://registry.npmjs.org/react/-/react-18.2.0.tgz", Repository: "https://github.com/facebook/react"}, + want: &DependencyOrigin{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"}, + origin: &DependencyOrigin{Repository: "https://github.com/owner/repo?rev=main#abc", Revision: "9f8e7d6"}, + want: &DependencyOrigin{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"}, + origin: &DependencyOrigin{Repository: "https://github.com/owner/repo", Revision: "feature@login"}, + want: &DependencyOrigin{Repository: "https://github.com/owner/repo"}, }, } @@ -157,7 +167,7 @@ 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) { +func TestDependencyOriginHostCaseIsCanonical(t *testing.T) { upper := RepositoryOrigin("https://GitHub.com/Owner/Repo", "aaaabbbbccccddddeeeeffff0000111122223333") lower := RepositoryOrigin("https://github.com/Owner/Repo", "aaaabbbbccccddddeeeeffff0000111122223333") @@ -167,21 +177,21 @@ func TestPackageOriginHostCaseIsCanonical(t *testing.T) { 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") + if !sameLocation(upper, lower) { + t.Fatal("two spellings of one host must normalize to one location") } - // The path is case-sensitive, so these are different locations. - if settled := ReconcileOrigin( + // The path is case-sensitive, so these stay different locations. + if sameLocation( 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) + ) { + t.Fatal("paths differing in case are different locations") } } // An explicit default port names the same origin as no port at all. -func TestPackageOriginDefaultPortIsCanonical(t *testing.T) { +func TestDependencyOriginDefaultPortIsCanonical(t *testing.T) { cases := []struct { name string raw string @@ -210,39 +220,38 @@ func TestPackageOriginDefaultPortIsCanonical(t *testing.T) { }) } - if settled := ReconcileOrigin( + if !sameLocation( 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") + ) { + t.Fatal("a default port must normalize away") } - if settled := ReconcileOrigin( + if !sameLocation( ArtifactOrigin("https://example.test:0443/pkg-1.0.0.tgz"), ArtifactOrigin("https://example.test/pkg-1.0.0.tgz"), - ); settled.Empty() { - t.Fatal("a default port written with leading zeros must not read as a disagreement") + ) { + t.Fatal("a default port written with leading zeros must normalize away") } - if settled := ReconcileOrigin( + if !sameLocation( ArtifactOrigin("https://example.test:08443/pkg-1.0.0.tgz"), ArtifactOrigin("https://example.test:8443/pkg-1.0.0.tgz"), - ); settled.Empty() { - t.Fatal("one port written two ways must not read as a disagreement") + ) { + t.Fatal("one port written two ways must normalize to one location") } - if settled := ReconcileOrigin( + if sameLocation( 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) + ) { + t.Fatal("a non-default port is a different location") } } // 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{ +func TestDependencyOriginEmptyAgreesWithNormalized(t *testing.T) { + origins := []*DependencyOrigin{ nil, {}, - {Disputed: true}, {ArtifactURL: "https://registry.npmjs.org/react/-/react-18.2.0.tgz"}, {ArtifactURL: "/Users/someone/pkg.tgz"}, {Repository: "file:///home/someone/repo"}, @@ -256,132 +265,19 @@ func TestPackageOriginEmptyAgreesWithNormalized(t *testing.T) { } } -func TestPackageOriginEmpty(t *testing.T) { - var nilOrigin *PackageOrigin +func TestDependencyOriginEmpty(t *testing.T) { + var nilOrigin *DependencyOrigin if !nilOrigin.Empty() { t.Fatal("nil origin should be empty") } - if !(&PackageOrigin{}).Empty() { + if !(&DependencyOrigin{}).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() { + if (&DependencyOrigin{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{ @@ -396,39 +292,9 @@ func TestDependencyCloneCopiesOrigin(t *testing.T) { } } -// 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) { +func TestDependencyOriginWireRoundTrip(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"), @@ -460,7 +326,7 @@ func TestPackageOriginWireRoundTrip(t *testing.T) { // 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) { +func TestDependencyOriginPercentEncodingIsCanonical(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"}, @@ -496,26 +362,26 @@ func TestPackageOriginPercentEncodingIsCanonical(t *testing.T) { t.Fatalf("origin = %+v, want nil for a malformed escape", origin) } - if settled := ReconcileOrigin( + if !sameLocation( 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") + ) { + t.Fatal("two spellings of one path must normalize to one location") } - if settled := ReconcileOrigin( + if sameLocation( 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) + ) { + t.Fatal("an escaped slash is a different path") } } // A value that would be rejected on read must not be storable or forwardable. -func TestPackageOriginNormalizesAcrossJSON(t *testing.T) { +func TestDependencyOriginNormalizesAcrossJSON(t *testing.T) { cases := []struct { name string raw string - want PackageOrigin + want DependencyOrigin }{ {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"}`}, @@ -523,24 +389,18 @@ func TestPackageOriginNormalizesAcrossJSON(t *testing.T) { { 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"}, + want: DependencyOrigin{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}, + want: DependencyOrigin{Repository: "https://github.com/Owner/Repo", Revision: "aaaabbbbccccddddeeeeffff0000111122223333"}, }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - var decoded PackageOrigin + var decoded DependencyOrigin if err := json.Unmarshal([]byte(tc.raw), &decoded); err != nil { t.Fatalf("decode: %v", err) } @@ -552,7 +412,7 @@ func TestPackageOriginNormalizesAcrossJSON(t *testing.T) { // 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"}) + raw, err := json.Marshal(&DependencyOrigin{ArtifactURL: "https://build:s3cret@nexus.corp/pkg.tgz"}) if err != nil { t.Fatal(err) } @@ -560,3 +420,61 @@ func TestPackageOriginNormalizesAcrossJSON(t *testing.T) { t.Fatalf("marshaled %s, which carries a credential", raw) } } + +// Graph merging is a merger of both in the fill-gaps sense: an origin fills in +// from whichever record has one, so it survives regardless of manifest order, +// and on a genuine conflict the existing record's origin stays. +func TestMergeGraphFillsOriginGaps(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}, + }) + if url != "" { + node.Origin = ArtifactOrigin(url) + } + if err := g.AddNode(node); err != nil { + t.Fatal(err) + } + return g + } + originOf := func(t *testing.T, g *Graph) *DependencyOrigin { + t.Helper() + node, ok := g.Node("lodash@4.17.21") + if !ok { + t.Fatal("expected lodash in the merged graph") + } + return node.Origin.Normalized() + } + + t.Run("a later record fills the gap", func(t *testing.T) { + merged := New() + if err := MergeGraph(merged, build(t, "")); err != nil { + t.Fatal(err) + } + if err := MergeGraph(merged, build(t, artifact)); err != nil { + t.Fatal(err) + } + if got := originOf(t, merged); got == nil || got.ArtifactURL != artifact { + t.Fatalf("merged origin = %+v, want %q regardless of manifest order", got, artifact) + } + }) + + t.Run("an existing origin stays on conflict", 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) + } + if got := originOf(t, merged); got == nil || got.ArtifactURL != artifact { + t.Fatalf("merged origin = %+v, want the existing record's %q", got, artifact) + } + }) +} diff --git a/package.go b/package.go index 6104ac8..f9d6fa1 100644 --- a/package.go +++ b/package.go @@ -63,200 +63,6 @@ type PackageLicense struct { Type LicenseType `json:"type,omitempty"` } -// Digest captures integrity information for a package artifact. -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"` -} - -// 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 -// 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 - } - 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 !merged { - p.Attestations = append(p.Attestations, candidate.Clone()) - } - } -} - -// 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 - } - switch { - case a.Issuer == other.Issuer: - return true - case a.claimsNothing(), other.claimsNothing(): - return true - default: - return false - } -} - -// 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) { - 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 - } -} - -// 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 - 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 { - // 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 -} - -// 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. type PackageEOL struct { Source string `json:"source,omitempty"` @@ -360,13 +166,12 @@ type Package struct { Coordinates // ID is the package registry identifier. It may be a database ID, PURL, or // another stable key chosen by the package registry. - ID string `json:"id,omitempty"` - Copyright string `json:"copyright,omitempty"` + ID string `json:"id,omitempty"` + Copyright string `json:"copyright,omitempty"` + // ResolvedURL is detection-time evidence carried onto the registry package + // for matchers (repository resolution reads it). It is raw and never + // published; the dependency's validated Origin stays on the graph node. 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"` @@ -473,7 +278,6 @@ func (p *Package) Clone() *Package { clone.Vulnerabilities = append(clone.Vulnerabilities, v.Clone()) } } - clone.Origin = p.Origin.Clone() if len(p.Attestations) > 0 { clone.Attestations = make([]PackageAttestation, 0, len(p.Attestations)) for _, attestation := range p.Attestations { @@ -525,9 +329,6 @@ 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) p.mergeAttestations(src.Attestations) if len(p.CPEs) == 0 { p.CPEs = cloneStrings(src.CPEs) @@ -633,7 +434,6 @@ func PackageFromDependency(dep *Dependency) *Package { }, ID: purl, ResolvedURL: dep.ResolvedURL, - Origin: dep.Origin.Clone(), } } From 4fdcae4afe91540d64a1319f1b805a4e03514799 Mon Sep 17 00:00:00 2001 From: Ahmed ElMallah Date: Fri, 21 Aug 2026 01:18:55 -0400 Subject: [PATCH 2/2] fix: a gap is an origin that publishes nothing, not a nil pointer UnmarshalJSON leaves a non-nil zero Origin behind when a recorded value fails validation, so a graph decoded from JSON carries non-nil origins that name no location. The merge's nil check read those as "present" and refused the fill, dropping the only publishable origin and making the outcome depend on which graph decoded first. The gap check is now Empty(), which is nil-safe and asks the question the rule actually means. Also from review: the dead first-nonempty digest fallback that mergeDigests already subsumes is removed; mergeDigests moves to digest.go with its type; the UnmarshalJSON doc no longer describes the deleted disagreement record; and the two synthetic-credential fixtures carry the nolint justification the rest of the file uses. Co-Authored-By: Claude Fable 5 --- attestation.go | 22 ---------------------- container.go | 2 +- digest.go | 22 ++++++++++++++++++++++ origin.go | 5 +++-- origin_test.go | 23 +++++++++++++++++++++-- package.go | 3 --- 6 files changed, 47 insertions(+), 30 deletions(-) diff --git a/attestation.go b/attestation.go index 4b98f27..dd46e6f 100644 --- a/attestation.go +++ b/attestation.go @@ -31,28 +31,6 @@ 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 diff --git a/container.go b/container.go index c2b431d..52b5abb 100644 --- a/container.go +++ b/container.go @@ -175,7 +175,7 @@ func addNodeIfMissing(g *Graph, node *Dependency) error { // it survives regardless of manifest order. Where both records // assert different origins -- rare -- the existing one stays, // deterministically. - if existing.Origin == nil { + if existing.Origin.Empty() { existing.Origin = clone.Origin } } diff --git a/digest.go b/digest.go index f916316..71532de 100644 --- a/digest.go +++ b/digest.go @@ -28,3 +28,25 @@ const ( // rather than of the package itself, such as a manifest or lockfile entry. DigestSubjectMetadata DigestSubject = "metadata" ) + +// 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) + } +} diff --git a/origin.go b/origin.go index 78d8fcc..243949e 100644 --- a/origin.go +++ b/origin.go @@ -267,8 +267,9 @@ type originWire DependencyOrigin // 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. +// or written back out. A value that fails validation decodes to an empty +// origin -- including a payload from an older build that still carries the +// removed "disputed" field, whose remaining values stand on their own. func (o *DependencyOrigin) UnmarshalJSON(data []byte) error { var wire originWire if err := json.Unmarshal(data, &wire); err != nil { diff --git a/origin_test.go b/origin_test.go index f41a904..22cda8e 100644 --- a/origin_test.go +++ b/origin_test.go @@ -130,7 +130,7 @@ func TestDependencyOriginNormalized(t *testing.T) { }{ {name: "nil"}, {name: "empty", origin: &DependencyOrigin{}}, - {name: "credentialed artifact", origin: &DependencyOrigin{ArtifactURL: "https://build:s3cret@nexus.corp/pkg.tgz"}}, + {name: "credentialed artifact", origin: &DependencyOrigin{ArtifactURL: "https://build:s3cret@nexus.corp/pkg.tgz"}}, //nolint:gosec // synthetic credential; rejecting it is the rule under test {name: "local repository", origin: &DependencyOrigin{Repository: "file:///home/someone/repo"}}, {name: "revision without a repository", origin: &DependencyOrigin{Revision: "9f8e7d6"}}, { @@ -383,7 +383,7 @@ func TestDependencyOriginNormalizesAcrossJSON(t *testing.T) { raw string want DependencyOrigin }{ - {name: "credentialed artifact is dropped", raw: `{"artifact_url":"https://build:s3cret@nexus.corp/pkg.tgz"}`}, + {name: "credentialed artifact is dropped", raw: `{"artifact_url":"https://build:s3cret@nexus.corp/pkg.tgz"}`}, //nolint:gosec // synthetic credential; rejecting it is the rule under test {name: "local path is dropped", raw: `{"repository":"file:///home/someone/repo"}`}, {name: "revision without a repository is dropped", raw: `{"revision":"9f8e7d6"}`}, { @@ -465,6 +465,25 @@ func TestMergeGraphFillsOriginGaps(t *testing.T) { } }) + // A graph decoded from JSON holds a non-nil zero Origin when the recorded + // value was unpublishable, so a gap is "publishes nothing", not "nil". + t.Run("a decoded-empty origin is a gap", func(t *testing.T) { + merged := New() + unpublishable := build(t, "") + if node, ok := unpublishable.Node("lodash@4.17.21"); ok { + node.Origin = &DependencyOrigin{} // what UnmarshalJSON leaves behind + } + if err := MergeGraph(merged, unpublishable); err != nil { + t.Fatal(err) + } + if err := MergeGraph(merged, build(t, artifact)); err != nil { + t.Fatal(err) + } + if got := originOf(t, merged); got == nil || got.ArtifactURL != artifact { + t.Fatalf("merged origin = %+v, want %q: an empty origin must not block the fill", got, artifact) + } + }) + t.Run("an existing origin stays on conflict", func(t *testing.T) { merged := New() if err := MergeGraph(merged, build(t, artifact)); err != nil { diff --git a/package.go b/package.go index f9d6fa1..5efb6bc 100644 --- a/package.go +++ b/package.go @@ -334,9 +334,6 @@ func (p *Package) MergeFrom(src *Package) { 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...) - } if len(p.Licenses) == 0 && len(src.Licenses) > 0 { p.Licenses = append([]PackageLicense(nil), src.Licenses...) }