izar / pytm

A Pythonic framework for threat modeling
Other
876 stars 165 forks source link

Fix: Excluded threat IDs are ignored when using --exclude argument #174

Closed jnk22 closed 2 years ago

jnk22 commented 2 years ago

Hi,

the --exclude argument does not have any effect on the output/reports. I could not find any related issue/pull request so far.

Example: python tm.py --exclude DR01,DS06 --report template.md > report.md

(This should remove the threats Unprotected Sensitive Data and Data Leak from the list of threats at the sample TM)

The attribute _threatsExcluded are set when parsing arguments, but were not used after that. This was fixed with a simple if-statement when adding threats to the output/report.

However, this also required the argument parsing of excluded threats to be moved further up. Otherwise, reports/output would have been processed before threats are even added to the list of excluded threats as it seems.

izar commented 2 years ago

Thanks!! Funny, I could swear it was working as advertised! Any chance you might add tests for the functionality so we can detect future breaks?

jnk22 commented 2 years ago

Yes, I'll add some tests :)

izar commented 2 years ago

Thanks!

On Thu, Sep 23, 2021, 10:03 Jannik Schäfer @.***> wrote:

Yes, I'll add some tests :)

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/izar/pytm/pull/174#issuecomment-925845843, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC2BAPOLXODYQV47SH5TI3UDMXRXANCNFSM5EHXYK7Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

jnk22 commented 2 years ago

I have added a test case for excluded threat IDs.

This verifies that already excluded threats (i.e. threats that have been properly parsed with TM.process()) will not be in the findings.

This does not verify that the argument parsing of --exclude [ID1],[ID2] works though. I couldn't find any other test where you explicitly test argument parsing. If this is also required, I'll happily add some test for that as well!

If there is anything else about my test that you want me to rework, feel free to request some more changes :)

izar commented 2 years ago

Awesome! Will you be participating in Hacktoberfest ? If so, we can wait to merge until next week so this counts towards your contributions. Let me know.

jnk22 commented 2 years ago

I have signed up for Hacktoberfest now and linked my GitHub account, thanks for reminding me!

I don't know if there is anything else I need to set up first, but you can just merge this now if you want. If it counts towards my contributions that's cool, if not, that is totally okay as well :)