Fix wrong SPDX license expression when a package has multiple licenses - #5150
Open
sueun-dev wants to merge 1 commit into
Open
Fix wrong SPDX license expression when a package has multiple licenses#5150sueun-dev wants to merge 1 commit into
sueun-dev wants to merge 1 commit into
Conversation
Both the SPDX and the CycloneDX encoder parenthesise an expression before joining
it onto the others with AND, and both decide with the same check: does it start
with "(" or end with ")". An expression can do either and still carry an OR at
the top level, and SPDX applies AND before OR, so those get joined unwrapped and
the result no longer means what was declared.
"MIT OR (Apache-2.0 AND BSD-3-Clause)" joined onto "GPL-3.0-only" reads as a
choice that no longer requires GPL-3.0-only at all.
Wrap when the expression has an OR outside of any parentheses. Expressions whose
top-level operator is AND or WITH keep the text they are emitted with today.
Signed-off-by: Sueun Cho <sueun.dev@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Both the SPDX and the CycloneDX encoder parenthesise a license expression before joining it onto the other licenses with
AND, and both lean on the same check — does it start with(or end with):An expression can do either and still carry an
ORat the top level. SPDX appliesANDbeforeOR, so those get joined unwrapped and the result stops meaning what was declared. Both checks came in together in 42fa9e4.SPDX side. Syft's Arch cataloger splits pacman's
%LICENSE%block line by line (syft/pkg/cataloger/arch/package.go), so an Arch package can carry many licenses.linux-lts-headers6.18.41 ships 42 of them, 8 skip the wrapping and 7 of those carry a top-levelOR—(GPL-2.0-only WITH Linux-syscall-note) OR BSD-2-Clause,... OR MIT, and so on. Scanning a pacman root holding that real license block:What that costs, checked with
spdxexp.Satisfies(already in go.mod) on the same shape as the AURandroid-*-liburingpackages, whose%LICENSE%is["(GPL-2.0-only WITH Linux-syscall-note) OR MIT", "LGPL-2.0-or-later", "MIT"]:GPL-2.0-only WITH Linux-syscall-noteSo the SBOM says the package can be taken under the kernel syscall-note GPL on its own, dropping the LGPL-2.0-or-later and MIT declared next to it.
Being straight about how often that fires: I walked all 10045 Core+Extra x86_64 packages on archlinux.org and 3 hit it —
core/linux-lts-headers,extra/linux-rt-headers,extra/linux-rt-lts-headers. Other ecosystems mostly carry one license string per package, so there is nothing to join.CycloneDX side.
mergeSPDXhas the same defect, reached when every license on a package is a compound expression.syft converton a CycloneDX SBOM whose component carries two expressions:Satisfieson the emitted one withApache-2.0plusBSD-3-Clausealone is true today and false after — theGPL-3.0-onlydrops out the same way. Any SBOM in that shape is corrupted on re-encode, so this half is not tied to one distro.The change adds
spdxlicense.HasTopLevelOr— both packages already importinternal/spdxlicense, no new imports — and wraps when it reports true. The rest of each condition is untouched, so expressions whose top-level operator isANDkeep the text they have today:ISC AND (BSD-3-Clause OR MIT), the real alpinelibretlsvalue insyft/format/testdata/alpine-syft.json, does not move. Scanning this repo with a binary built before and after gives identical license fields for all 2113 SPDX packages and all 2649 CycloneDX components.One output change beyond the corruption, so it is not a surprise in review: a package with a single license whose expression has a top-level
ORand a paren at an edge now gains a redundant outer paren ((GPL-2.0-only WITH Linux-syscall-note) OR MIT→((GPL-2.0-only WITH Linux-syscall-note) OR MIT)). Nothing is joined onto it so the parens are redundant, but a loneMIT OR Apache-2.0is already wrapped today, so this makes the two shapes consistent rather than splitting on whether a paren happens to sit at the edge. Both are pinned by tests. Happy to special-caselen == 1instead if you would rather that output stayed exactly as-is.Not addressed:
spdxexp.ValidateLicensesaccepts operators written without surrounding spaces, likeMIT OR(Apache-2.0), and neither the old check nor this one wraps those. Pre-existing, left alone.Type of change
Checklist
Eleven cases added across the two tables: five fail on main, six pin behaviour that must not change.
mergeSPDXhad no test naming it — it was covered incidentally throughTest_encodeLicense— soTest_mergeSPDXis its first direct one.go build ./...,go test ./syft/format/... ./internal/spdxlicense/ ./syft/pkg/ ./syft/license/...andgolangci-lint run --timeout 15m ./...are clean.