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

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

Merged
merged 3 commits into from
Oct 4, 2021
Merged

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

merged 3 commits into from
Oct 4, 2021

Conversation

jnk22
Copy link
Contributor

@jnk22 jnk22 commented Sep 17, 2021

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.

@jnk22 jnk22 requested a review from izar as a code owner September 17, 2021 15:48
@izar
Copy link
Collaborator

izar commented Sep 23, 2021

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
Copy link
Contributor Author

jnk22 commented Sep 23, 2021

Yes, I'll add some tests :)

@izar
Copy link
Collaborator

izar commented Sep 23, 2021 via email

@jnk22
Copy link
Contributor Author

jnk22 commented Sep 28, 2021

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
Copy link
Collaborator

izar commented Sep 28, 2021

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
Copy link
Contributor Author

jnk22 commented Oct 4, 2021

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 :)

@izar izar merged commit 572b165 into OWASP:master Oct 4, 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.

2 participants