Implementing 6.2.24 Usage of Non-Latest CWE Version#699
Conversation
|
Closes #310 |
konradweiss
changed the title
|
Is this ready to be reviewed? |
Yes, and happy to get some advice for improvements :) |
|
|
||
| #[test] | ||
| fn test_test_6_2_24() { | ||
| let case_01 = Err(vec
There was a problem hiding this comment.
| let case_01 = Err(vec![create_non_latest_cwe_error( | |
| let case_01_cwe_version_before_latest = Err(vec![create_non_latest_cwe_error( |
We try to go for speaking variable names for the failing cases. For these, it's kind of trivial to see whats wrong, but we found this helpful for more complex cases.
| let case_02 = Err(vec![create_non_latest_cwe_error( | ||
| "CWE-143", | ||
| "4.15", | ||
| "4.13", | ||
| "/vulnerabilities/0/cwes/0", | ||
| )]); |
There was a problem hiding this comment.
Same as above, I think a speaking var name would help. Also, I would like to point out the error message being generated here:
Weakness 'CWE-143' uses non-latest CWE version 4.15 (latest: 4.13).
This might be confusing to users, as the message does not indicate that the latest CWE version at the time of release is evaluated, and 4.15 is clearly later than 4.13. We could for example a) include a mention of the release date of latest or b) provide a different error message for when latest is "earlier" than the actual version of the CWE.
| create_non_latest_cwe_error("CWE-318", "4.14", "4.13", "/vulnerabilities/1/cwes/0"), | ||
| create_non_latest_cwe_error("CWE-61", "4.15", "4.13", "/vulnerabilities/2/cwes/0"), | ||
| ]); | ||
|
|
There was a problem hiding this comment.
We try to provide a small comment on what the passing test cases do, to allow for a shared unterstanding and discussion of test coverage.
For example:
// Case 11: 1 vuln, 1 correct cwe version, correction of case 01
// Case 12: 1 vuln, 1 correct cwe version, corrrection of case 02
// Case 13: 1 vuln, 3 correct cwe versions, correction of case 03
// Case 14: multiple vulns, multiple correct cwe versions, correction of case 04
Did you find any code paths / gaps during implementation? If so, feel free to open an issue so we can add a supplemental test for it :)
| &cwe_item.id, | ||
| version, | ||
| latest, | ||
| format!("/vulnerabilities/{i_r}/cwes/{i_cwe}").as_str(), |
There was a problem hiding this comment.
I think we can safe a few string allocations by passing the indices and only having one format! call in the create_non_latest_cwe_error() function here :)
| // skip the check (no external CWE data available). | ||
| let Some(latest) = latest_version else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
This check can live outside this loop I think, if the latest version is None, this test can not run.
There are ongoing discussions how to handle this (test not being able to run, being skipped for various reasons, ...) upstream (one of them: oasis-tcs/csaf#1231). We had this discussion as #409 in our repo.
Please add a comment on which under which coniditions latest_version can be None, which will help us choose the correct return type during that coming refactor.
| if !date.is_valid() { | ||
| return None; | ||
| } | ||
|
|
||
| // Convert to a date (UTC) and compare against the release dates stored in the CWE assets. | ||
| let doc_date: NaiveDate = match date { | ||
| CsafDateTime::Valid(v) => v.get_as_utc().date_naive(), | ||
| _ => return None, | ||
| }; |
There was a problem hiding this comment.
These two checks are duplicate, as date.is_valid() is just matches!(self, CsafDateTime::Valid(_)), which you do in the second statement.
Also, lets discuss if a) this check should be done in the main function, and this function should accept only ValidCsafDatetime structs, and also b if this function should be co-located with CWE_ENTRIES
| if let Some(cwes) = vulnerability.get_cwe() { | ||
| for (i_cwe, cwe_item) in cwes.iter().enumerate() { | ||
| // Require the CWE item to include a version. If missing, the parser | ||
| // should have rejected the document; skip checking here. |
There was a problem hiding this comment.
This comment is slightly incorrect: CWE version is required for CSAF 2.1 and noneexistent for CSAF 2.0. So the CSAF 2.1 deserialization would have broken before this test could be executed, making this codepath practically unreachable.
This code path should be revisited if we implement #159 (although this test simply does not make sense for CSAF 2.0 and would probably be skipped altogether) and #571, where the strict requirements introduced by our generated type deserialization will not exist and this codepath could be visited.
|
Lastly: A big thank you for your first contribution(s) to your project, if there are any further questions we are happy to help! I've written a bit more lengthy comments to show you around some of the currently still rough edges of our codebase, my comments might also find their way into #700 or similar so others don't stumble over the same things. Thanks again, and I look forward to the discussions and your potential future contributions 👍 |
Thanks, for you review comments :) I will look to address them ASAP, which for me should be on Friday |
2 participants
Note: Some of the test cases that are expected to fail use a newer version than the current one. For now I have implemented them with the same Error message, which could be changed.