From 2821ce744bae6727a945939f90f001290b5c92b9 Mon Sep 17 00:00:00 2001 From: Sueun Cho Date: Tue, 4 Aug 2026 11:10:55 +0900 Subject: [PATCH] Fix wrong SPDX license expression when a package has multiple licenses 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 --- internal/spdxlicense/license.go | 27 +++++++++++++ internal/spdxlicense/license_test.go | 25 ++++++++++++ .../cyclonedxutil/helpers/licenses.go | 7 +++- .../cyclonedxutil/helpers/licenses_test.go | 37 +++++++++++++++++ .../internal/spdxutil/helpers/license.go | 29 +++++++++----- .../internal/spdxutil/helpers/license_test.go | 40 +++++++++++++++++++ 6 files changed, 154 insertions(+), 11 deletions(-) diff --git a/internal/spdxlicense/license.go b/internal/spdxlicense/license.go index ff8bd1265cd..b126c0e7fe9 100644 --- a/internal/spdxlicense/license.go +++ b/internal/spdxlicense/license.go @@ -62,3 +62,30 @@ func stripScheme(url string) string { url = strings.TrimPrefix(url, "http://") return url } + +// HasTopLevelOr reports whether the expression has an OR operator outside of every parenthesised +// group. +// +// It matters when an expression is joined onto others with AND, because SPDX applies AND before +// OR: joining "MIT OR (Apache-2.0 AND BSD-3-Clause)" unwrapped gives +// "MIT OR (Apache-2.0 AND BSD-3-Clause) AND GPL-3.0-only", which reads as +// "MIT OR ((Apache-2.0 AND BSD-3-Clause) AND GPL-3.0-only)" and drops GPL-3.0-only from the first +// choice. Testing only the first and last character misses that, and misses +// "(MIT OR Apache-2.0) OR (GPL-3.0-only AND MIT)" as well. +func HasTopLevelOr(expression string) bool { + depth := 0 + for i, c := range expression { + switch c { + case '(': + depth++ + case ')': + depth-- + default: + if depth <= 0 && strings.HasPrefix(expression[i:], " OR ") { + return true + } + } + } + + return false +} diff --git a/internal/spdxlicense/license_test.go b/internal/spdxlicense/license_test.go index e1444a62c6c..4685ab6ab11 100644 --- a/internal/spdxlicense/license_test.go +++ b/internal/spdxlicense/license_test.go @@ -83,3 +83,28 @@ func TestSPDXIDRecognition(t *testing.T) { }) } } + +func TestHasTopLevelOr(t *testing.T) { + tests := []struct { + expression string + want bool + }{ + {"", false}, + {"MIT", false}, + {"MIT AND Apache-2.0", false}, + {"Apache-2.0 WITH LLVM-exception", false}, + {"MIT OR Apache-2.0", true}, + {"(MIT OR Apache-2.0)", false}, + {"ISC AND (BSD-3-Clause OR MIT)", false}, + {"MIT OR (Apache-2.0 AND BSD-3-Clause)", true}, + {"(GPL-2.0-only WITH Linux-syscall-note) OR MIT", true}, + {"(MIT OR Apache-2.0) OR (GPL-3.0-only AND LGPL-2.1-only)", true}, + {"(MIT AND (Apache-2.0 OR BSD-3-Clause))", false}, + {"LicenseRef-one-thing-first", false}, + } + for _, tt := range tests { + t.Run(tt.expression, func(t *testing.T) { + assert.Equal(t, tt.want, HasTopLevelOr(tt.expression)) + }) + } +} diff --git a/syft/format/internal/cyclonedxutil/helpers/licenses.go b/syft/format/internal/cyclonedxutil/helpers/licenses.go index 3330c86a7f4..45598817796 100644 --- a/syft/format/internal/cyclonedxutil/helpers/licenses.go +++ b/syft/format/internal/cyclonedxutil/helpers/licenses.go @@ -194,8 +194,11 @@ func processLicenseURLs(l pkg.License, spdxID string, populate *cyclonedx.Licens func mergeSPDX(ex []string) string { var candidate []string for _, e := range ex { - // if the expression does not have balanced parens add them - if !strings.HasPrefix(e, "(") && !strings.HasSuffix(e, ")") { + // wrap when the expression does not have balanced parens, and also when it carries an OR + // outside of any parens: SPDX applies AND before OR, so joining + // "MIT OR (Apache-2.0 AND BSD-3-Clause)" as-is changes what it means, and the + // prefix/suffix check on its own does not catch that + if spdxlicense.HasTopLevelOr(e) || (!strings.HasPrefix(e, "(") && !strings.HasSuffix(e, ")")) { e = "(" + e + ")" } candidate = append(candidate, e) diff --git a/syft/format/internal/cyclonedxutil/helpers/licenses_test.go b/syft/format/internal/cyclonedxutil/helpers/licenses_test.go index 257e96acb47..5232579a717 100644 --- a/syft/format/internal/cyclonedxutil/helpers/licenses_test.go +++ b/syft/format/internal/cyclonedxutil/helpers/licenses_test.go @@ -314,3 +314,40 @@ func TestDecodeLicenses(t *testing.T) { }) } } + +func Test_mergeSPDX(t *testing.T) { + tests := []struct { + name string + input []string + expected string + }{ + { + // without the parens this reads as + // "((GPL-3.0-only AND MIT-0) AND MIT) OR (Apache-2.0 AND BSD-3-Clause)", + // so Apache-2.0 plus BSD-3-Clause alone would satisfy it + name: "expression with a top-level OR is wrapped", + input: []string{"MIT OR (Apache-2.0 AND BSD-3-Clause)", "GPL-3.0-only AND MIT-0"}, + expected: "(MIT OR (Apache-2.0 AND BSD-3-Clause)) AND (GPL-3.0-only AND MIT-0)", + }, + { + name: "top-level AND keeps the text it has today", + input: []string{"ISC AND (BSD-3-Clause OR MIT)", "GPL-3.0-only AND MIT-0"}, + expected: "ISC AND (BSD-3-Clause OR MIT) AND (GPL-3.0-only AND MIT-0)", + }, + { + name: "single expression is unwrapped by reduceOuter", + input: []string{"MIT OR Apache-2.0"}, + expected: "MIT OR Apache-2.0", + }, + { + name: "plain ids", + input: []string{"MIT", "Apache-2.0"}, + expected: "(MIT) AND (Apache-2.0)", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.expected, mergeSPDX(test.input)) + }) + } +} diff --git a/syft/format/internal/spdxutil/helpers/license.go b/syft/format/internal/spdxutil/helpers/license.go index 8ba477a6af0..27d80a58b9d 100644 --- a/syft/format/internal/spdxutil/helpers/license.go +++ b/syft/format/internal/spdxutil/helpers/license.go @@ -42,15 +42,8 @@ func joinLicenses(licenses []SPDXLicense) string { for _, l := range licenses { v := l.ID - // check if license does not start or end with parens - if !strings.HasPrefix(v, "(") && !strings.HasSuffix(v, ")") { - // if license contains AND, OR, or WITH, then wrap in parens - if strings.Contains(v, " AND ") || - strings.Contains(v, " OR ") || - strings.Contains(v, " WITH ") { - newLicenses = append(newLicenses, "("+v+")") - continue - } + if needsParens(v) { + v = "(" + v + ")" } newLicenses = append(newLicenses, v) } @@ -58,6 +51,24 @@ func joinLicenses(licenses []SPDXLicense) string { return strings.Join(newLicenses, " AND ") } +// needsParens reports whether an expression has to be wrapped before it is joined onto the others +// with AND. An expression carrying a top-level OR has to be, or the join changes what it means. +// The rest is the rule this used before, so expressions that are already emitted correctly keep +// the text they have today. +func needsParens(expression string) bool { + if spdxlicense.HasTopLevelOr(expression) { + return true + } + + if strings.HasPrefix(expression, "(") || strings.HasSuffix(expression, ")") { + return false + } + + return strings.Contains(expression, " AND ") || + strings.Contains(expression, " OR ") || + strings.Contains(expression, " WITH ") +} + type SPDXLicense struct { // Valid SPDX ID OR License Value (should have LicenseRef- prefix and be sanitized) // OR combination of the above as a valid SPDX License Expression as defined in Annex D. diff --git a/syft/format/internal/spdxutil/helpers/license_test.go b/syft/format/internal/spdxutil/helpers/license_test.go index 4dd262ec3d8..99e0a98d4d1 100644 --- a/syft/format/internal/spdxutil/helpers/license_test.go +++ b/syft/format/internal/spdxutil/helpers/license_test.go @@ -170,6 +170,46 @@ func Test_joinLicenses(t *testing.T) { args: []SPDXLicense{{ID: "MIT AND Apache"}, {ID: "GPL-3.0-only"}}, want: "(MIT AND Apache) AND GPL-3.0-only", }, + { + // without the parens this reads as "MIT OR (Apache-2.0 AND BSD-3-Clause AND GPL-3.0-only)", + // which allows MIT on its own and drops the GPL-3.0-only obligation + name: "expression ending in a paren is still wrapped", + args: []SPDXLicense{{ID: "MIT OR (Apache-2.0 AND BSD-3-Clause)"}, {ID: "GPL-3.0-only"}}, + want: "(MIT OR (Apache-2.0 AND BSD-3-Clause)) AND GPL-3.0-only", + }, + { + name: "expression starting with a paren is still wrapped", + args: []SPDXLicense{{ID: "(MIT OR Apache-2.0) OR GPL-3.0-only"}, {ID: "LGPL-2.1-only"}}, + want: "((MIT OR Apache-2.0) OR GPL-3.0-only) AND LGPL-2.1-only", + }, + { + name: "separate groups joined at the top level are wrapped", + args: []SPDXLicense{{ID: "(MIT OR Apache-2.0) OR (GPL-3.0-only AND LGPL-2.1-only)"}, {ID: "GPL-3.0-only"}}, + want: "((MIT OR Apache-2.0) OR (GPL-3.0-only AND LGPL-2.1-only)) AND GPL-3.0-only", + }, + { + name: "a single parenthesised group is not wrapped again", + args: []SPDXLicense{{ID: "(MIT OR Apache-2.0)"}, {ID: "GPL-3.0-only"}}, + want: "(MIT OR Apache-2.0) AND GPL-3.0-only", + }, + { + // top-level operator is AND, so joining is already safe and the text must not move + name: "expression whose top-level operator is AND keeps its text", + args: []SPDXLicense{{ID: "ISC AND (BSD-3-Clause OR MIT)"}, {ID: "MIT"}}, + want: "ISC AND (BSD-3-Clause OR MIT) AND MIT", + }, + { + // nothing is joined onto it, so the parens are redundant, but a lone compound + // expression is already wrapped today and this keeps the two shapes consistent + name: "single expression with a top-level OR is wrapped", + args: []SPDXLicense{{ID: "(GPL-2.0-only WITH Linux-syscall-note) OR MIT"}}, + want: "((GPL-2.0-only WITH Linux-syscall-note) OR MIT)", + }, + { + name: "single expression without parens keeps the wrapping it has today", + args: []SPDXLicense{{ID: "MIT OR Apache-2.0"}}, + want: "(MIT OR Apache-2.0)", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {