`bom.vulnerabilities` model by thepwagner · Pull Request #163 · CycloneDX/cyclonedx-javascript-library

thepwagner · 2022-08-06T17:55:21Z

Add Vulnerabilties to the Bom model, including enums as needed.

The implementation is styled after the existing Component implementation and tests.

this is part of #164

Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com>

jkowalleck · 2022-08-07T06:09:31Z

Thank you for this feature, @thepwagner

Review will take a while, did a part of it already.

Are you planning on implementing the JSON- and XML normalizer, too?

jkowalleck

partial review.

please find critical remarks with an ❌ .
other remarks are discussion items.

src/enums/vulnerabilityAffectsVersionStatus.ts

src/enums/vulnerabilityAnalysisJustification.ts

jkowalleck · 2022-08-07T05:35:42Z

src/enums/vulnerabilityAnalysisResponse.ts

+Copyright (c) OWASP Foundation. All Rights Reserved.
+*/
+
+export enum VulnerabilityAnalysisResponse {


rename to ImpactAnalysisResponsesType as the XML schema defines?

I switched to ImpactAnalysisResponse as this was the most consistent with the JSON schema, even if the JSON schema doesn't give this enum a standalone definition.

Please advise if you'd prefer a different name.

src/enums/vulnerabilityAnalysisState.ts

src/enums/vulnerabilityRatingMethod.ts

src/enums/vulnerabilityAnalysisJustification.ts

src/enums/vulnerabilityAffectsVersionStatus.ts

tests/unit/Models.Vulnerability.spec.js

Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com>

…lities

thepwagner · 2022-08-07T12:55:00Z

@jkowalleck thank you for the quick initial feedback!
I've addressed feedback and resolved the threads I think are resolved - please advise if you'd prefer to resolve after verifying the fixes.

I made all suggested renames, biased towards the naming of the JSON schema.
I've extended the compare() methods to consider all fields, including nested sets like Set<URL | string> or SortableSet<VulnerabilityReference>. This meant adding a generic SortableSet.compare()
Using the JSON schema for functional tests is cool! I extended the name-mapping helper to support snake_case since there was no feedback on the naming of enum values in the first review.

I was not intending to implement the normalizers.

jkowalleck

please find critical chaneg requests marked with ❌ '
everything else is a discussion item.

everything is just my opinionated view, lets discuss :)

jkowalleck · 2022-08-08T06:55:01Z

src/enums/affectedStatus.ts

+Copyright (c) OWASP Foundation. All Rights Reserved.
+*/
+
+export enum AffectedStatus {


re: https://github.com/CycloneDX/cyclonedx-javascript-library/pull/163/files/542767bab211a12fa71d209e88978e2b4f49c3bf#r939618319

👍 lets keep it as enum AffectedStatus.

alternative enum ImpactAnalysisAffectedStatus might get in conflict with enum ImpactAnalysisState which already has a value NotAffected.

jkowalleck · 2022-08-08T07:05:05Z

src/helpers/sortableSet.ts

+  compare (other: SortableSet<T>): number {
+    const thisSorted = this.sorted()
+    const otherSorted = other.sorted()
+    const lenDiff = thisSorted.length - otherSorted.length


❌ why sort upfront?
Could you add an in-code comment to explain?

jkowalleck · 2022-08-08T07:05:29Z

src/helpers/sortableSet.ts

+      return lenDiff
+    }
+
+    for (let i = 0; i < thisSorted.length; i++) {


❌ better: for ( const i=0 ; i < thisSorted.length ; ++i ) {}

jkowalleck · 2022-08-08T07:18:54Z

src/types/cwe.ts

+
+/**
+ * Define the format for acceptable Common Weakness Enumeration (CWE) IDs.
+ * Refer to {@link https://cwe.mitre.org/index.html} for official specification.


jkowalleck · 2022-08-08T07:31:38Z

tests/_helpers/stringFunctions.js

   */
  capitaliseFirstLetter: s => s.charAt(0).toUpperCase() + s.slice(1),
  /**
-   * UpperCamelCase a string


i liked the old description better.
I understand the need for more description.

❌ better go with:

UpperCamelCase a string ("white space example", "snake_case_example" or "kebab-case-example" )

❌ when on it, lets replace not only _ but also -(kebab-case)

jkowalleck · 2022-08-08T10:03:33Z

src/models/vulnerabilityCredits.ts

+}
+
+export class VulnerabilityCredits implements Comparable {
+  organizations?: OrganizationalEntityRepository


❌ repositories must not be optiional, but they may be empty

jkowalleck · 2022-08-08T10:03:37Z

src/models/vulnerabilityCredits.ts

+
+export class VulnerabilityCredits implements Comparable {
+  organizations?: OrganizationalEntityRepository
+  individuals?: OrganizationalContactRepository


❌ repositories must not be optiional, but they may be empty

jkowalleck · 2022-08-08T10:04:43Z

src/models/vulnerabilityRating.ts

+  }
+
+  compare (other: VulnerabilityRating): number {
+    return (this.score ?? 0) - (other.score ?? 0)


this appears to be like an insufficient comparison.
if this is enough to properly compare, then please add a in-code comment that explains why.

jkowalleck · 2022-08-08T10:06:29Z

src/models/vulnerabilityReference.ts

+  }
+
+  compare (other: VulnerabilityReference): number {
+    return (this.id ?? '').localeCompare(other.id ?? '')


❌ unnecessary null-comparisson

jkowalleck · 2022-08-08T10:07:29Z

src/models/vulnerabilitySource.ts

+import { Comparable, SortableSet } from '../helpers/sortableSet'
+
+interface OptionalProperties {
+  url?: URL | string


❌ use type refenrences ala VulnerabilitySource['url']

thepwagner · 2022-08-12T14:42:16Z

The changes remaining on this PR are far removed from my initial use case: as you can tell I'm not a strong TypeScript developer, and all I wanted was the Bom model object to have a []Vulnerability field that I could use.

I appreciate the value that your normalization brings, but the added complexity means contributing to this library is outside of my ability. If anyone else wants to pick this up for the basis of the implementation, please do!

@jkowalleck thank you for the reviews and sorry I couldn't see this through.

jkowalleck · 2022-08-12T14:55:46Z

thank you for all your work, @thepwagner

could you open a pull request with your latest things and have it target the "feature/vulnerabilities" branch ?
Plan is to merge your models in there, fine tune them a bit and merge them into master for an early release.
I would love to keep you involved, @thepwagner , so when a beta-release of the feature is published, you could check if it suites your needs.

Then, in a later feature, the normalizers and serializers can be developed and be released separately.

This way you (and others) will have the models for use soon,
and will not be blocked by normalization details.

jkowalleck · 2023-01-20T13:36:45Z

work continues in #419

Bom.Vulnerabilities model

542767b

Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com>

thepwagner requested a review from a team as a code owner August 6, 2022 17:55

jkowalleck added the enhancement New feature or request label Aug 7, 2022

jkowalleck mentioned this pull request Aug 7, 2022

complete support for Bom.Vulnerabilities #164

Closed

12 tasks

jkowalleck changed the title ~~Bom.Vulnerabilities model~~ bom.vulnerabilities model Aug 7, 2022

jkowalleck requested changes Aug 7, 2022

View reviewed changes

thepwagner added 4 commits August 7, 2022 06:48

rename enums per JSON schema

9d48635

Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com>

models: Vulnerability comparisons

1221c43

Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com>

Functional tests for vulnerability enums

9e9ff63

Signed-off-by: Peter Wagner <1559510+thepwagner@users.noreply.github.com>

Merge remote-tracking branch 'upstream/main' into model-bom-vulnerabi…

30be7c6

…lities

jkowalleck mentioned this pull request Aug 8, 2022

more sortable sets #165

Merged

jkowalleck requested changes Aug 8, 2022

View reviewed changes

thepwagner closed this Aug 12, 2022

thepwagner mentioned this pull request Aug 12, 2022

trivy BOMs and vuln diffs thepwagner/sbom-action#56

Merged

jkowalleck mentioned this pull request Jan 20, 2023

feat: bom.vulnerabilities data models and enums #419

Merged

21 tasks

jkowalleck added this to the Bom.Vulnerabilities milestone Jan 20, 2023

Uh oh!

Conversation

thepwagner commented Aug 6, 2022 • edited by jkowalleck Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkowalleck commented Aug 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkowalleck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thepwagner commented Aug 7, 2022

Uh oh!

jkowalleck left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thepwagner commented Aug 12, 2022

Uh oh!

jkowalleck commented Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkowalleck commented Jan 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thepwagner commented Aug 6, 2022 •

edited by jkowalleck

Loading

jkowalleck commented Aug 7, 2022 •

edited

Loading

jkowalleck left a comment •

edited

Loading

jkowalleck commented Aug 12, 2022 •

edited

Loading