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

Add optional severity field override for affected packages. #106

Merged
merged 8 commits into from
Jan 11, 2023

Conversation

oswalpalash
Copy link
Contributor

Based on #40 (comment)

ossf#40

Signed-off-by: Palash Oswal <oswalpalash@gmail.com>
ossf#40

Signed-off-by: Palash Oswal <oswalpalash@gmail.com>
@captn3m0
Copy link
Contributor

captn3m0 commented Jan 5, 2023

Looks good.

Copy link
Contributor

@oliverchang oliverchang left a comment

Choose a reason for hiding this comment

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

Thanks!

I wonder if we need to call out any semantics here. i.e. What does it mean for there to be both a top level severity and per-package severity?

Does it make sense to have both? Or should we disallow specifying both?

@joshbressers @kurtseifried @chrisbloom7 @rsc

docs/schema.md Outdated
@@ -356,6 +364,11 @@ The `purl` field is a string following the
[Package URL specification](https://github.com/package-url/purl-spec) that
identifies the package. This field is optional but recommended.

The `severity` field is an optional JSON array that allows generating systems to
describe the severity of a vulnerability using one or more quantitative scoring
methods. Each `severity` item is a JSON object specifying a `type` and `score`
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of "Each severity item is a JSON object specifying a type and score",
can we just say that this array is identical to the top level severity field (ideally with an anchored link?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oliverchang I've made the changes.

Signed-off-by: Palash Oswal <oswalpalash@gmail.com>
docs/schema.md Outdated Show resolved Hide resolved
Co-authored-by: Chris Bloom <chrisbloom7@github.com>
Signed-off-by: Palash Oswal <oswalpalash@gmail.com>
Copy link
Contributor

@oliverchang oliverchang left a comment

Choose a reason for hiding this comment

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

thanks again for this PR. a few more comments.

docs/schema.md Outdated Show resolved Hide resolved
docs/schema.md Outdated
@@ -356,6 +364,12 @@ The `purl` field is a string following the
[Package URL specification](https://github.com/package-url/purl-spec) that
identifies the package. This field is optional but recommended.

The `severity` field is an optional element [defined here](#severity-field).
Per package severity override is optional for different packages. It is only
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking more I'd suggest enforcing that either the top level severity or the per-package severites are set, but not both.

It doesn't make much sense to me, and it enforces a bit more consistency. We should also validate this with our JSON schema, but that can come later.

Maybe we can replace everything from the second paragraph onwards with:

This `severity` field applies to a specific package, in cases where affected packages have
differing severities for the same vulnerability. If any package level `severity` fields are set,
the top level `severity` (add hyperlink) must not be set. 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Ready for another review

Signed-off-by: Palash Oswal <oswalpalash@gmail.com>
Signed-off-by: Palash Oswal <oswalpalash@gmail.com>
Signed-off-by: Palash Oswal <oswalpalash@gmail.com>
Copy link
Contributor

@oliverchang oliverchang left a comment

Choose a reason for hiding this comment

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

thanks!

docs/schema.md Outdated
@@ -324,6 +332,12 @@ platform information that doesn't apply equally to all listed versions and
ranges, a separate entry with the same `package` in the `affected` array may be
needed.

The `severity` field is an optional element [defined here](#severity-field).
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry one more comment: Can you just move this text down to under the heading on line 411? That makes this text a bit more easily discoverable since it can be found from the ToC.

The other text here around the relevant fields (e.g. "versions", "ranges") live here because they offer some context relevant to both of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense @oliverchang ! All set

Signed-off-by: Palash Oswal <oswalpalash@gmail.com>
Copy link
Contributor

@oliverchang oliverchang left a comment

Choose a reason for hiding this comment

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

thank you! will also let @rsc and @chrisbloom7 sign off on this.

Copy link

@rsc rsc left a comment

Choose a reason for hiding this comment

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

LGTM. The motivation in #40 convinces me.

@oliverchang oliverchang merged commit e6ba0d9 into ossf:main Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants