-
Notifications
You must be signed in to change notification settings - Fork 612
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
Add downloadLocation URI validation #3697
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Stef Graces <[email protected]>
Attempted same command with changes from issue #3696, which sets it to NOASSERTION because of the validation issue {
"name": "@isaacs/cliui",
"SPDXID": "SPDXRef-Package-npm--isaacs-cliui-7026ea92955de2ad",
"versionInfo": "8.0.2",
"supplier": "Person: Ben Coe ([email protected])",
"originator": "Person: Ben Coe ([email protected])",
"downloadLocation": "NOASSERTION",
"filesAnalyzed": false,
"sourceInfo": "acquired package info from installed node module manifest file: /usr/local/lib/node_modules/npm/node_modules/@isaacs/cliui/package.json",
"licenseConcluded": "NOASSERTION",
"licenseDeclared": "ISC",
"copyrightText": "NOASSERTION",
"description": "easily create complex multi-column command-line-interfaces",
"externalRefs": [
{
"referenceCategory": "SECURITY",
"referenceType": "cpe23Type",
"referenceLocator": "cpe:2.3:a:\\@isaacs\\/cliui:\\@isaacs\\/cliui:8.0.2:*:*:*:*:*:*:*"
},
{
"referenceCategory": "PACKAGE-MANAGER",
"referenceType": "purl",
"referenceLocator": "pkg:npm/%40isaacs/[email protected]"
}
]
} |
Signed-off-by: Stef Graces <[email protected]>
546541f
to
d411a32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks very much @stgrace !
Argh, I spoke too soon; it looks like we need some snapshots and other tests updated, but I think the core change here is the right thing. I can help with this, also fine if you'd like to do it.
|
I may have been hasty to think about the nuance between NONE and NOASSERTION
} | ||
|
||
func UriValue(uri string) string { | ||
if NoneIfEmpty(uri) != NONE { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using NOASSERTION
here actually ends up being the right thing to do. The reason is, we're not claiming that there is no download location, we're only claiming that we cannot provide one. As such, I think we should update this to: if isURIValid(uri) { return uri } return NOASSERTION
, or instead get rid of this function altogether and just update the last bit of DownloadLocation
to have this, and ensure isURIValid
returns false for empty string. I'm happy to make these updates.
Very sorry for not giving this sufficient thought originally
Description
Please include a summary of the changes along with any relevant motivation and context,
or link to an issue where this is explained.
Type of change
Checklist: