Fix wrong SPDX license expression when a package has multiple licenses

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
+}

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))
+		})
+	}
+}

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)

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))
+		})
+	}
+}

syft/format/internal/spdxutil/helpers/license.go

@@ -42,22 +42,33 @@ 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)
 	}
 
 	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.

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) {