Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SpdxDocumentFile: Report issues from subprojects that cannot be mapped to a package ID. #8851

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,17 @@ class PackageManagerDependencyHandler(
packageManager: String,
definitionFile: String,
scope: String,
linkage: PackageLinkage
linkage: PackageLinkage,
issues: List<Issue> = emptyList()
): PackageReference =
PackageReference(
id = Identifier(
type = TYPE,
namespace = packageManager,
name = definitionFile.encodeColon(),
version = "$linkage@$scope"
)
),
issues = issues
)

private fun getPackageManagerDependency(node: DependencyNode): PackageManagerDependency? =
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
SPDXID: "SPDXRef-DOCUMENT"
spdxVersion: "SPDX-2.2"
creationInfo:
created: "2022-06-29T00:00:00Z"
creators:
- "Organization: OSS Review Toolkit"
name: "illegal_chars"
dataLicense: "CC0-1.0"
documentNamespace: "http://spdx.org/spdxdocs/example"
documentDescribes:
- "SPDXRef-Package-asio_dtls"
packages:
- SPDXID: "SPDXRef-Package-illegal_chars" # This SPDX ID contains '_' which is not allowed.
description: "A SPDX document that is in a directory characters not allowed for SPDX IDs"
copyrightText: "NONE"
filesAnalyzed: false
homepage: "https://example.com/"
licenseConcluded: "NOASSERTION"
licenseDeclared: "NOASSERTION"
originator: "Organization: Robert Bosch GmbH"
name: "illegal_chars"
downloadLocation: "https://example.com"
versionInfo: "1.0.0"
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
SPDXID: "SPDXRef-DOCUMENT"
spdxVersion: "SPDX-2.2"
creationInfo:
created: "2024-07-08T18:30:22Z"
creators:
- "OSS Review Toolkit"
name: "External Ref with illegal chars"
dataLicense: "CC0-1.0"
documentNamespace: "https://spdx.org/spdxdocs/example"
documentDescribes:
- "SPDXRef-Package-illegal-chars"

externalDocumentRefs:
- externalDocumentId: "DocumentRef-illegal_chars" # This SPDX ID contains '_' which is not allowed.
spdxDocument: "illegal_chars/package.spdx.yml"
checksum:
algorithm: "SHA1"
checksumValue: "3e3edec9f5073a7b17a9fd066e5f49ed3e41f0a2"

packages:
- SPDXID: "SPDXRef-Package-test"
copyrightText: "NONE"
downloadLocation: "https://example.com"
filesAnalyzed: false
homepage: "NONE"
licenseConcluded: "Apache-2.0"
licenseDeclared: "Apache-2.0"
name: "Illegal Chars"
originator: "Organization: OSS Review Toolkit"
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ externalDocumentRefs:
spdxDocument: "./subproject/subproject-with-conan-reference.spdx.yml"
checksum:
algorithm: "SHA1"
checksumValue: "53840294281f7d1bf401b746162da11dcf9306eb"
checksumValue: "032a087b0da8f353c03671a72179b807b4ec7eb3"
Copy link
Member

@sschuberth sschuberth Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we have tests for SpdxResolvedDocument in place that verify the checksum, so please explain in the commit message how this could have been wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to explain it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks. Let me try to rephrase that for my understanding: The checksum was always wrong, but that fact did not result in an issue or test failure until the previous commit. Now that issues are properly propagated, it would result in a test failure of SpdxDocumentFileFunTest whose expected result files contain no issues. Correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

packages:
- SPDXID: "SPDXRef-Package-xyz"
description: "Awesome product created by Example Inc."
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import io.kotest.core.spec.style.WordSpec
import io.kotest.matchers.collections.containExactly
import io.kotest.matchers.collections.containExactlyInAnyOrder
import io.kotest.matchers.collections.shouldContainExactlyInAnyOrder
import io.kotest.matchers.collections.shouldHaveSingleElement
import io.kotest.matchers.collections.shouldHaveSize
import io.kotest.matchers.maps.haveSize
import io.kotest.matchers.nulls.shouldNotBeNull
Expand Down Expand Up @@ -255,6 +256,29 @@ class SpdxDocumentFileFunTest : WordSpec({
Identifier("SpdxDocumentFile:OpenSSL Development Team:openssl:1.1.1g")
)
}

"collect issues for subprojects using illegal SPDX identifiers" {
val projectFile = projectDir.resolve("illegal-chars-external-refs/project-xyz.spdx.yml")
val subProjectFile = projectDir.resolve("illegal-chars-external-refs/illegal_chars/package.spdx.yml")
val definitionFiles = listOf(projectFile, subProjectFile)

val result = create("SpdxDocumentFile").resolveDependencies(definitionFiles, emptyMap())

val rootProject = result.projectResults[projectFile.absoluteFile]?.first()

rootProject shouldNotBeNull {
issues shouldHaveSize 1
issues.shouldHaveSingleElement {
val expectedMessage = Regex(
"""
.*SPDX ID 'SPDXRef-Package-illegal_chars' is only allowed to contain letters, numbers, '\.', and '-'.*
""".trimIndent()
)

expectedMessage.containsMatchIn(it.message)
}
}
}
}
})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,8 @@ class SpdxDocumentFile(
}

internal fun getPackageManagerDependency(pkgId: String, doc: SpdxResolvedDocument): PackageReference? {
val spdxPackage = doc.getSpdxPackageForId(pkgId, mutableListOf()) ?: return null
val issues = mutableListOf<Issue>()
val spdxPackage = doc.getSpdxPackageForId(pkgId, issues) ?: return null
val definitionFile = doc.getDefinitionFile(pkgId) ?: return null

if (spdxPackage.packageFilename.isBlank()) return null
Expand All @@ -376,7 +377,8 @@ class SpdxDocumentFile(
packageManager = factory.type,
definitionFile = VersionControlSystem.getPathInfo(packageFile).path,
scope = scope,
linkage = PackageLinkage.PROJECT_STATIC // TODO: Set linkage based on SPDX reference type.
linkage = PackageLinkage.PROJECT_STATIC, // TODO: Set linkage based on SPDX reference type.
issues = issues
)
}
}
Expand Down Expand Up @@ -523,7 +525,7 @@ class SpdxDocumentFile(
scopeDependencies = scopes
)

return listOf(ProjectAnalyzerResult(project, packages))
return listOf(ProjectAnalyzerResult(project, packages, transitiveDocument.getIssuesWithoutSpdxPackage()))
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,15 @@
return pkg
}

/**
* Retrieve the issues from [issuesByReferenceId] that are not associated with [any package][packagesById]. These
* issues can be related to general issues within the SPDX document.
*/
fun getIssuesWithoutSpdxPackage() =
issuesByReferenceId.mapNotNull { (id, issue) ->

Check warning on line 141 in plugins/package-managers/spdx/src/main/kotlin/utils/SpdxResolvedDocument.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/spdx/src/main/kotlin/utils/SpdxResolvedDocument.kt#L141

Added line #L141 was not covered by tests
if (packagesById[id] == null) issue else null
}

/**
* Return the local definition file in which the package with the given [identifier] is declared. If the package
* cannot be resolved or if it has not been declared in a local file, return *null*.
Expand Down
Loading