OWASP / threat-dragon

An open source threat modeling tool from OWASP
https://owasp.org/www-project-threat-dragon/
Apache License 2.0
902 stars 244 forks source link

Severity Should Not be a Required Field #197

Closed lreading closed 2 years ago

lreading commented 3 years ago

Describe the bug Severity is a required field, and defaults to "medium". When threat modeling, the severity may be unknown at the time of recording the threat. Additionally, some users may use a more complex severity system.

Expected behavior Severity is not required. After being selected, severity should be able to be un-set

Environment N/A

To Reproduce

Any additional context, screenshots, etc Thanks to Simon S for bringing this to our attention

jgadsden commented 3 years ago

This is a difficult one, and a very good point raised by Simon S. We already have a block of 'High', 'Medium' and 'Low'. We could add a fourth 'NA' but with the possibility of adding a score as well #196 we may end up with a block of five ... which will look too complex?

Could we start using drop downs instead of radio buttons?

lreading commented 3 years ago

I think using drop-downs would be the simplest solution, as opposed to adding anything. Using a dropdown, you could un-set the value after it is set, or leave it blank to begin with.

Gavitron commented 2 years ago

I was doing a threat modelling exercise with an SME, using Threat Dragon for the first time, and we were both confused by 'severity'. For a given threat, we wanted to articulate things like

and we weren't sure how to map all of that into the tristate of 'severity'.

I suspect that's what this open issue is about too: https://github.com/OWASP/threat-dragon/issues/149 but am not certain of that author's intent.

for the time being, we will export our threats to a spreadsheet, and break out the dimensions there, but it would be great to see these dimensions pulled into TD 2.x

thanks!

jgadsden commented 2 years ago

Hello @Gavitron - you make a very good point. Severity is not meaningful - or at least can not be quantified - in most threat analysis. Would it be better to regard 'Severity' as 'Priority' ? Then we could rename this field, while keeping backward compatible, and the values become useful

Open to suggestions @Gavitron , @lreading and @mike-goodwin

We would also have to look at the 'traffic light' for the threats listed in the left-hand pane, probably move from Red Amber Green

Gavitron commented 2 years ago

hello @jgadsden - I think that's a great suggestion to regard 'Severity' as 'Priority'; it's a metric we can determine quickly, while collaborating on the models, and helps inform many subsequent actions, including potentially even helping to decide which parts of the model to refine first. as far as traffic lights, there is precedent elsewhere that LMH can map to Green, Yellow, Red, so you may not have to change much for that as well, but tbh I haven't looked at the code to say with any certainty. :)

jgadsden commented 2 years ago

OK, happy to rename Severity to Priority. The .json files need to be backward compatible, so we have to to stick with threat.severity in the .json .

Will do this for version 1.x if @lreading you are happy to go ahead and do this for version 2.0?

lreading commented 2 years ago

Will make the change in v2 now so as not to forget later. :sweat_smile: Great points made here, and I agree that priority is more meaningful.

There's been discussion regarding the use of color as a meaningful indicator. The traffic light style indicators could be difficult to distinguish for someone who is colorblind. Very loosely related to #150 We don't have a full solution for that yet, but it's definitely something we'd like to consider updating in the future!

jgadsden commented 2 years ago

OK, thanks @lreading . Version 1.x has been updated and will see in future if we can make the code work with either .json attributes severity or priority. Might get messy :-)

lreading commented 2 years ago

v2 now has the threat-edit modal and is labeled as priority, but the backing json is still severity. Additionally, it is now a dropdown that also contains an empty value as an option. Should we close this as done, or did we want to make further changes in v1?

jgadsden commented 2 years ago

Yes, this caused me a lot of thinking. We need to remain backwardly compatible with existing threat models and also the JIRA + TMT utilities, so in the end I left it as 'severity' in the json.

There must be better solutions though, maybe have both 'severity' and 'priority' that are synonymous and displayed on the edit box? I am very open to suggestions :)

But until we decide then yes, we can close this as done