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

Resolves #380: Added severity in the view at http://127.0.0.1:8000/vulnerabilities/<… #390

Merged
merged 20 commits into from
Apr 29, 2021

Conversation

Pushpit07
Copy link
Contributor

Added severity in the view at http://127.0.0.1:8000/vulnerabilities/<vulnerability_id> by calling the scores() function for the particular reference and getting its value and scoring_system

Signed-off-by: Pushpit [email protected]

…vulnerability_id> by calling the scores() function for the particular reference and getting its value and scoring_system

Signed-off-by: Pushpit <[email protected]>
@Pushpit07 Pushpit07 changed the title Added severity in the view at http://127.0.0.1:8000/vulnerabilities/<… Resolves #380: Added severity in the view at http://127.0.0.1:8000/vulnerabilities/<… Mar 19, 2021
@Pushpit07
Copy link
Contributor Author

Now all the severity values and scoring_systems related to the particular reference is being shown

Screenshot 2021-03-19 at 16 25 30G
Screenshot 2021-03-19 at 16 26 01
Screenshot 2021-03-19 at 16 26 27

Screenshot 2021-03-19 at 16 24 53

@Hritik14
Copy link
Collaborator

Hritik14 commented Mar 19, 2021

I guess if we could have a generalized version of severity (for ex, the one like purl), it would be much better. What do you think @sbs2001 ?

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 19, 2021

@Hritik14 nope unifying them is the last thing we want to do. Severity changes according to the vendor. A score provided by redhat, would be given more importance when the found vulnerability is affecting a builtin redhat package but archlinux would rate it low if it is a external package.

See the bottom section at https://access.redhat.com/security/updates/classification/

@Hritik14
Copy link
Collaborator

Hritik14 commented Mar 19, 2021

By generalisation, I didn’t actually mean generalised to one number/score. I meant that we can use a general format that could be applicable everywhere.
A very rudimentary example would be ‘ASA:7.6’ or ‘RHSA:3.5’.
EDIT: My bad. It is already present in severity_systems.py. I guess it didn't occur to me because the severity is present as a list of the scoring systems here.

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 19, 2021

@Pushpit07 I think continuing to use tables leaves us with lot of unused whitespace and the scores look too crammed up.

I think we should switch using a list instead, with nested severities ?

Something like
References:-

  • Reference ID value | Reference URL
    • Severity scoring system {scoring_system} | {scoring_value}

A practical example:-

References:-
  *AVG-853| https://security.archlinux.org/AVG-853
    *CVSS_3 | 8.5
     *CVSS_3_vector| CVSS:3/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:H   

@Pushpit07
Copy link
Contributor Author

@sbs2001 I feel that tables represent data in a much more organised and readable format. Implementing the items as a list would actually make the experience cumbersome.
Instead, what I suggest is that we can provide a list of severities and scoring_system inside the table itself. Something like-

AVG-853 https://security.archlinux.org/AVG-853 * CVSS_3
*8.5
* cvssv3
* cvssv3_vector
* cvssv2_vector
AVG-652 https://security.archlinux.org/AVG-652 * CVSS_2
*7.0
* cvssv2
* cvssv2_vector

What say?

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 20, 2021

@Pushpit07 that's a interesting approach. The example you provided doesn't help though, why am I seeing CVSS_3 in two columns ? How can I differentiate between what's the severity system and the value ? Please provide a clearer example :)

Implementing the items as a list would actually make the experience cumbersome.

Other than duplication of metadata, I don't see any other problem with this approach, maybe I'm missing something here ?

@Pushpit07
Copy link
Contributor Author

Pushpit07 commented Mar 20, 2021

@Pushpit07 that's a interesting approach. The example you provided doesn't help though, why am I seeing CVSS_3 in two columns ? How can I differentiate between what's the severity system and the value ? Please provide a clearer example :)

Table headers would be there to differentiate though

Reference ID URL Severity Value Scoring system
- https://security.archlinux.org/AVG-853
AVG-652 https://security.archlinux.org/AVG-652 * AV:N/AC:M/Au:N/C:C/I:C/A:C
* 7.0
* cvssv2
* cvssv2_vector

@Pushpit07
Copy link
Contributor Author

Other than duplication of metadata, I don't see any other problem with this approach, maybe I'm missing something here ?

Yeah, duplication is the main reason due to which I'm suggesting to avoid it

@Pushpit07
Copy link
Contributor Author

@sbs2001 Any updates on this? What to do?

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 24, 2021

@Pushpit07 sorry for the late reply. IMHO tables is the way to go, but we need some way to denote the informational hierarchy. In simple terms we want to denote that severity_system -> severity_value pairs are child of the reference.

The current approach brings them to the same level as that of the reference's main attributes which is kinda misleading.

Sorry for taking this long,I think this is not a good first issue, but you've been doing a good job. We need further design to fix the hierarchy thing and this will be done. I'm open to ideas here :)

@Pushpit07
Copy link
Contributor Author

If I understand correctly, is this right?

Screenshot 2021-03-24 at 16 43 53

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 24, 2021

@Pushpit07 the severity value should be the child/sibling of the each individual of each scoring system. See the example at #390 (comment).

@Pushpit07
Copy link
Contributor Author

Oh, okay. This is what you mean?

Screenshot 2021-03-25 at 08 26 56

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 25, 2021

@Pushpit07 yep that's what I mean't. I realized that we would want to nest them further.
Something like

References:-
  *AVG-853| https://security.archlinux.org/AVG-853
    *Severities
       System    |     Value
       *CVSS_3 | 8.5
       *CVSS_3_vector| CVSS:3/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:H   

That'd be easy to be implement right ?

@Pushpit07
Copy link
Contributor Author

Yup. Piece of cake

@AmitGupta7580
Copy link
Contributor

Can we use this type of nested rows for severity represntation ?

image

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 29, 2021

@AmitGupta7580 that would work in case most of the references have a nested severity. But in reality that is not the case. Most of references don't provide a score. So we would end up with lots of wasted whitespace, and the references would get cramped unnecessarily

@AmitGupta7580
Copy link
Contributor

image
image

What about this Tree structure View where user can expand particular reference and view its severity extracted from that reference.

@sbs2001
Copy link
Collaborator

sbs2001 commented Mar 30, 2021

@AmitGupta7580 why are there multiple references nodes ?

@AmitGupta7580
Copy link
Contributor

AmitGupta7580 commented Mar 30, 2021

@AmitGupta7580 why are there multiple references nodes ?

@sbs2001 No these 2 images are same, I just expanded the 2nd reference. It is the tree view of #390 (comment) this information.

@Pushpit07
Copy link
Contributor Author

@sbs2001 Apologies for the delay. Been ill for the last 4 days and running behind schedule. Will give updates in a few hours

@AmitGupta7580
Copy link
Contributor

@Pushpit07 Can you please brief me what are you trying to implement.

@Pushpit07
Copy link
Contributor Author

@sbs2001 Updated design

Screenshot 2021-04-01 at 15 58 04

Screenshot 2021-04-01 at 15 58 31

Copy link
Collaborator

@sbs2001 sbs2001 left a comment

Choose a reason for hiding this comment

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

@Pushpit07 I like your design.

Just don't use bootstrap, we already use bulma, using 2 css frameworks is a bad idea especially for a small feature.

@@ -6,6 +6,7 @@
<link rel="stylesheet" href="{% static 'css/bulma.css' %}"/>
<link rel="stylesheet" href="{% static 'css/custom.css' %}"/>
<link rel="stylesheet" href="{% static 'css/font-awesome.css' %}"/>
<link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-eOJMYsd53ii+scO/bJGFsiCZc+5NDVN2yr8+0RDqr0Ql0h+rP48ckxlpbzKgwra6" crossorigin="anonymous">
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't want to use bootstrap. We already use bulma. See https://bulma.io/documentation/elements/table/

@@ -6,4 +6,8 @@

.Site-content {
flex: 1;
}

table {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm guessing this won't be needed if we use bulma ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was necessary. I removed the is-bordered class from the table so that cells are not divided by a line. That was causing difficulty in understanding the structure of severity

@Pushpit07
Copy link
Contributor Author

It looks good though. And isn't causing any confusion now

Signed-off-by: Pushpit <[email protected]>
@Pushpit07
Copy link
Contributor Author

Screenshot 2021-04-07 at 18 56 19

@Pushpit07
Copy link
Contributor Author

Removed table structure

Screenshot 2021-04-08 at 20 41 16

@Pushpit07
Copy link
Contributor Author

@sbs2001 ping

@sbs2001
Copy link
Collaborator

sbs2001 commented Apr 14, 2021

@Pushpit07 This looks weird when there's no reference id.

I think, we should make this simpler.

Let's keep the references as they are, not touch them.

Instead we'll create another section called scores.

This section would have a table(same like the references one) with 3 columns
system | value | reference

the reference would have value equal to the parent url of the score.

Because really, going lists is making things complicated wrt css and presentation. I especially don't want to maintain the custom css.

Again sorry for wasting your time, design is tough.

@Pushpit07
Copy link
Contributor Author

This looks weird when there's no reference id.

Yes, it's hard to understand this way

Instead we'll create another section called scores.

This section would have a table(same like the references one) with 3 columns
system | value | reference

the reference would have value equal to the parent url of the score.

Can you explain this by an example?

@sbs2001
Copy link
Collaborator

sbs2001 commented Apr 19, 2021

@sbs2001
Copy link
Collaborator

sbs2001 commented Apr 26, 2021

@Pushpit07 ping

@Pushpit07
Copy link
Contributor Author

oh, probably missed your comment. Will change it to how it is in the jsfiddle. Give me a day's time

@Pushpit07
Copy link
Contributor Author

@sbs2001

Screenshot 2021-04-28 at 21 07 46

Screenshot 2021-04-28 at 21 07 36

Copy link
Collaborator

@sbs2001 sbs2001 left a comment

Choose a reason for hiding this comment

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

Thanks @Pushpit07 , see my suggestions inline, and this will be good to merge then.

vulnerabilities/templates/vulnerability.html Outdated Show resolved Hide resolved
vulnerabilities/templates/vulnerability.html Outdated Show resolved Hide resolved
vulnerabilities/templates/vulnerability.html Outdated Show resolved Hide resolved
vulnerabilities/templates/vulnerability.html Outdated Show resolved Hide resolved
vulnerabilities/templates/vulnerability.html Outdated Show resolved Hide resolved
@sbs2001
Copy link
Collaborator

sbs2001 commented Apr 29, 2021

@Pushpit07 This branch is stale, could you rebase this with latest main

@Pushpit07 Pushpit07 requested a review from sbs2001 April 29, 2021 16:29
Copy link
Collaborator

@sbs2001 sbs2001 left a comment

Choose a reason for hiding this comment

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

Thanks merging, this

@sbs2001 sbs2001 merged commit d7ba181 into aboutcode-org:main Apr 29, 2021
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.

4 participants