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.

Sign up for GitHub

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

Jump to bottom

Osv: Add a fallback for the severity #5545

Merged
merged 5 commits into from
Jul 13, 2022
Merged

Osv: Add a fallback for the severity #5545

merged 5 commits into from
Jul 13, 2022

Conversation

fviernau
Copy link
Member

Please see the individual commits.

Part of #3599.

@fviernau fviernau requested review from a team, MarcelBochtler and oheger-bosch as code owners July 12, 2022 14:40
@fviernau fviernau force-pushed the osv-severity-fallback branch from 71d2fc0 to fdd9d5d Compare July 12, 2022 14:51
@codecov
Copy link

codecov bot commented Jul 12, 2022 •
edited
Loading

Codecov Report

Merging #5545 (1b73e2b) into main (1b73e2b) will not change coverage.
The diff coverage is n/a.

❗ Current head 1b73e2b differs from pull request most recent head 6ad8550. Consider uploading reports for the commit 6ad8550 to get more accurate results

@@            Coverage Diff            @@
##               main    #5545   +/-   ##
=========================================
  Coverage     70.42%   70.42%           
  Complexity     2121     2121           
=========================================
  Files           270      270           
  Lines         14859    14859           
  Branches       2407     2407           
=========================================
  Hits          10465    10465           
  Misses         3294     3294           
  Partials       1100     1100
Flag Coverage Δ
funTest-analyzer-docker 73.61% <0.00%> (ø)
test 32.81% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b73e2b...6ad8550. Read the comment docs.

sschuberth
sschuberth reviewed Jul 12, 2022
View reviewed changes
clients/osv/src/main/kotlin/Model.kt Outdated
@@ -25,6 +25,15 @@ import kotlinx.serialization.SerialName
import kotlinx.serialization.Serializable
import kotlinx.serialization.json.JsonObject

// FIXME: Remove all JsonElement subtypes as property types from the model in favor of raw strings holding JSON.
Copy link
Member

Choose a reason for hiding this comment

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

Can't you simply use a custom deserializer as outlined here that calls encodeToString() on the JsonElement? Performance should not be an issue here, as I do not expect these model element to be large.

Copy link
Member Author

@fviernau fviernau Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I've read this comment in the link you provided already before and have tried to write code based on it. I haven't figured it out though. As you say "simply", maybe you know how to use it to reach the goal. In particular, it was unclear what this polymorphicSerlializer variable is and how to modify BaseClass in that code snippet to make it compile. Do you have a hint?

@fviernau fviernau force-pushed the osv-severity-fallback branch from fdd9d5d to 50c94cf Compare July 13, 2022 06:49
@fviernau fviernau requested a review from sschuberth July 13, 2022 06:49
@fviernau fviernau mentioned this pull request Jul 13, 2022
Closed
@fviernau fviernau force-pushed the osv-severity-fallback branch from 50c94cf to 988a35e Compare July 13, 2022 08:41
@fviernau fviernau force-pushed the osv-severity-fallback branch from 988a35e to 1686110 Compare July 13, 2022 13:31
@fviernau fviernau requested a review from sschuberth July 13, 2022 13:31
sschuberth
sschuberth reviewed Jul 13, 2022
View reviewed changes
advisor/src/funTest/assets/retrieve-package-findings-expected-result.json
@@ -49,5 +49,32 @@
"severity" : "7.5"
} ]
} ]
} ],
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this was a misunderstanding of one of my earlier comments, but IMO this commit should be squashed with the next one, so test data and implementation are added in the same commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

My intention was to make the effect of the parsing obvious in the commits. Your proposal would defeat that idea. But I do actually like it and prefer to not squash. Ok?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, let's keep it as-is.

fviernau added 5 commits July 13, 2022 18:56
@fviernau
Model: Drop an unnecessary empty line
c42076e 
Signed-off-by: Frank Viernau <[email protected]>
@fviernau
Model: Add a TODO comment
7689570 
Signed-off-by: Frank Viernau <[email protected]>
@fviernau
OsvFunTest: Add a package with a vulnerability without severity
d557182 
The vulnerability lacks a severity in its dedicataed property, but has
a 'severity' property in the `databaseSpecific: JSONObject?` property.
This prepares for adding a fallback solution.

Signed-off-by: Frank Viernau <[email protected]>
@fviernau
Osv: Add a fallback for the severity
99a344c 
The server does return vulnerabilities which do not have a severity
value in the dedicated property. The unspecified `databaseSpecific`
property often times holds a primitive `severity` property with values
such as `[HIGH, MEDIUM, LOW]`. Make use of these values as a fallback as
these to provide more indication than a `null` value.

Note: The data model of 'osv/client' currently uses subtypes of
JsonElement to expose a couple of unspecified JSON objects as
properties. Accessing these requires the client code to add
'kotlinx.serialization' as dependency which is not nice. A solution to
that would be to use "raw" string values containing the JSON, which is
unfortunately not yet possible but may become so in the future, see
[1][2][3].

So, for now add 'kotlinx.serialization' as dependency to the advisor in
order to access the property and leave a FIXME comment as reminder.

[1] Kotlin/kotlinx.serialization#1298
[2] Kotlin/kotlinx.serialization#1405
[3] Kotlin/kotlinx.serialization#1058

Signed-off-by: Frank Viernau <[email protected]>
@fviernau
Osv: Add a TODO for parsing the input severity list
6ad8550 
Signed-off-by: Frank Viernau <[email protected]>
@fviernau fviernau force-pushed the osv-severity-fallback branch from 1686110 to 6ad8550 Compare July 13, 2022 16:59
@fviernau fviernau requested a review from sschuberth July 13, 2022 16:59
@fviernau fviernau merged commit 93ca30c into main Jul 13, 2022
@fviernau fviernau deleted the osv-severity-fallback branch July 13, 2022 19:06
@fviernau
Copy link
Member Author

Merge despite the bogus webapp build falure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sschuberth sschuberth sschuberth approved these changes

@MarcelBochtler MarcelBochtler Awaiting requested review from MarcelBochtler MarcelBochtler is a code owner

@oheger-bosch oheger-bosch Awaiting requested review from oheger-bosch oheger-bosch is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fviernau @sschuberth