#1628: Support for OS specific CVEs - #2370
Conversation
Coverage Report for CI Build 33406931848Coverage increased (+0.02%) to 73.632%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions48 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
c5309e2 to
e394226
Compare
laert-ll
added
enhancement
Ali-Shariati-Najafabadi
left a comment
Ali-Shariati-Najafabadi
left a comment
There was a problem hiding this comment.
Pulled the branch and ran the full test suite locally plus checkstyle. Build is green and the core idea (isAffected/merge/JSON round-trip) works and is tested. Found one actual crash bug and a few smaller things worth looking at before merging, left inline.
| jgen.writeStartObject(); | ||
| for (Map.Entry<String, List<VersionRange>> condition : conditions.entrySet()) { | ||
| jgen.writeFieldName(condition.getKey()); | ||
| writeArray(condition.getValue(), jgen); |
There was a problem hiding this comment.
If an OS entry in conditions is explicitly null (e.g. "windows": null in security.json), it loads fine but blows up here with an NPE the moment the file gets re-saved/normalized, since writeArray iterates the list without a null check. Tested it locally, confirmed the crash. Worth guarding against in readConditions (either reject or default to an empty list) rather than passing the null through.
|
|
||
| private Map<String, List<VersionRange>> readConditions(JsonParser p) throws IOException { | ||
|
|
||
| if (p.getCurrentToken() != JsonToken.START_OBJECT) { |
There was a problem hiding this comment.
Everywhere else in this deserializer (readArray, readValue) an unexpected token throws IllegalStateException. Here we just silently return null if conditions isn't an object. Feels inconsistent - a malformed conditions block would just get dropped without any error, which seems risky for security data.
| assertThat(cve.isAffected(VersionIdentifier.of("2.0.6"), null)).isFalse(); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Could we get a test for merge() actually combining conditions from both sides? Right now mergeConditions() is only hit indirectly through the round-trip test, which never calls merge(). Given how fiddly the version-range merging already is, I'd want that path covered directly (overlapping ranges, one-sided keys, etc.).
| if (contains(this.versions, version)) { | ||
| return true; | ||
| } | ||
| return (os != null) && contains(this.conditions.get(os.toString()), version); |
There was a problem hiding this comment.
Minor: the OS key lookup is case-sensitive (os.toString() is always lowercase). A typo like "Windows" in a hand-written security.json would just silently never match, no warning. Not blocking, just easy to miss.
| @@ -17,20 +20,62 @@ | |||
| * @param severity the severity in the range from (0,10.0] where 10.0 is most critical. | |||
There was a problem hiding this comment.
Nice implementation of the conditions map from #1628. One thing worth a doc note: the issue mentioned arch-specific keys like windows_arm64 as a possible future extension - right now isAffected only checks os.toString() so those wouldn't actually be evaluated even though they'd parse fine. Probably fine to punt on for now, just flagging so nobody assumes it works.
1fbec9a to
832df30
Compare
3 participants
This PR fixes #1628
Implemented changes:
security.jsonCVE model extended so that a single CVE can express affected version ranges that only apply on specific operating systems.conditionsmap to theCvemodel.CveJsonDeserializer/CveJsonSerializerread and writeconditions. It is only written when present, so existingsecurity.jsonfiles remain unchanged.Cve.isAffected(version, os)combines the globalversionswith the current OS's conditional ranges;Cve.merge(...)also merges the per-OS conditions.Testing instructions
Run
mvn clean testManual (single machine):
<IDE_ROOT>/urls/<tool>/<edition>/security.json(create if absent) and add a CVE whose globalversionsdo NOT match the version you'll install, but whoseconditionsfor your current OS (windows/linux/mac) do:{ "issues": [ { "id": "CVE-TEST-OS", "severity": 9.0, "versions": ["(0,0.0.1)"], "conditions": { "linux": ["[<version>]"] } } ] }Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.batand notfeature/921 fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internalpom.xmlfiles or otherwise if runtime dependencies changed, you have updated our LICENSE.asciidoc