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

Introducing EmptyElement to avoid showing "invalid" elements on diagrams #232

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oleh-mykytyuk
Copy link

In the issue #222 was mentioned creation of the spurious "invalid" elements. Also, in the PR #223 tried to override the name of auto-generated elements.

This PR goal is to skip using "invalid" elements on DFD and SEQ diagrams. Such elements can be a problem when a lot of Finding used for overriding findings (add additional information to the threats). This approach is mentioned in the official documentation (README file), but leads to problems on the diagrams.

What is done:

  • added EmptyElement (just to be used as non-optional property for Finding)
  • skipped generation EmptyElement on DFD and SEQ diagrams (skip include EmptyElement in diagram generation)

@oleh-mykytyuk oleh-mykytyuk requested a review from izar as a code owner March 4, 2024 15:03
Copy link
Contributor

@raphaelahrens raphaelahrens left a comment

Choose a reason for hiding this comment

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

I like the commit, since it clarifies the intend.

pytm/pytm.py Outdated

def __init__(self):
super().__init__("AutoGenerated", description="Autogenerated element for Finding")
self._is_drawn = True # Prevent drawing on a DFD diagram
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems backwards. I have not checked yet, but if this is correct it should be refactored to represent the intention better.

Copy link
Author

Choose a reason for hiding this comment

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

Added description

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh after reading the code and your comment self._is_drawn is an indicator to mark if the element has already been drawn.
I read it as "is it supposed to be drawn" . So I was just confused by the name.

But now I wonder if it is necessary as a member variable at all and if it could be moved into a drawing function. But this would require a mayor change I guess, since the drawing is split between all the dfd methods. I mean they are very similar and have duplicated code.

Copy link
Author

Choose a reason for hiding this comment

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

I can exclude this element from drawing procedure using if clause (like if not isinstance(e, EmptyElement) )...
Or introduce a variable and appropriate function/property like def is_visible()
Just tell me what is better to do in your opinion!

pytm/pytm.py Outdated
@@ -1016,7 +1016,7 @@ def seq(self):
participants.append(
'database {0} as "{1}"'.format(e._uniq_name(), e.display_name())
)
elif not isinstance(e, Dataflow) and not isinstance(e, Boundary):
elif not any((isinstance(e, Dataflow), isinstance(e, Boundary), isinstance(e, EmptyElement))):
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use isinstance with a tuple of types.

instance(e, (A,B,C))

https://docs.python.org/3/library/functions.html#isinstance

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@oleh-mykytyuk
Copy link
Author

Included this situation in tests

 - added EmptyElement
 - skipped generation EmptyElement on DFD and SEQ diagrams
@izar
Copy link
Collaborator

izar commented Mar 5, 2024

hey, thanks for the PR. I am a bit full this week but will be taking a look asap.

@oleh-mykytyuk
Copy link
Author

hey, thanks for the PR. I am a bit full this week but will be taking a look asap.

Greetings! Sorry for disturbing, but how about this PR?

@izar
Copy link
Collaborator

izar commented May 9, 2024

sorry, still prioritizing work, but i have not forgotten this!

@neilpvirtualitics
Copy link

Hey, thanks for working on this!
I am using pytm, and just noticed this 'element_invalid_" issue on my diagram, when using findings.

I've tried a couple of workarounds, but I still have two problems (with 1.3.1), the element_invalid_ issue, and the text of my "response" member that I add to my Findings, not appearing in the report. I added a response field to my template like this:

<details>
  <summary>   {{{{item.id}}}}  --  {{{{item.threat_id}}}}   --   {{{{item.description}}}}</summary>
  <h6> Targeted Element </h6>
  <p> {{{{item.target}}}} </p>
  <h6> Severity </h6>
  <p>{{{{item.severity}}}}</p>
  <h6>Example Instances</h6>
  <p>{{{{item.example}}}}</p>
  <h6>Mitigations</h6>
  <p>{{{{item.mitigations}}}}</p>
  <h6>References</h6>
  <p>{{{{item.references}}}}</p>
  <h6>Response</h6>
  <p>{{{{item.response}}}}</p>
  &emsp;
</details>

but in the html, that item.response is empty, even though I supply a response="""To Mitigate: foo""", in my Finding().

One of the workarounds I thought I'd try for the invalid element issue, is to redirect the model.py --dfd-colormap output to jq to try to use jq to edit-out the element_invalid_ elements, but this output is not json. It's close to json, but jq won't deal with it.

@izar
Copy link
Collaborator

izar commented Jun 20, 2024

ho @oleh-mykytiuk will you be able to address @neilpvirtualitics comment or should i review/merge as is ?

@oleh-mykytiuk
Copy link

Hey, thanks for working on this! I am using pytm, and just noticed this 'element_invalid_" issue on my diagram, when using findings.

I've tried a couple of workarounds, but I still have two problems (with 1.3.1), the element_invalid_ issue, and the text of my "response" member that I add to my Findings, not appearing in the report. I added a response field to my template like this:

<details>
  <summary>   {{{{item.id}}}}  --  {{{{item.threat_id}}}}   --   {{{{item.description}}}}</summary>
  <h6> Targeted Element </h6>
  <p> {{{{item.target}}}} </p>
  <h6> Severity </h6>
  <p>{{{{item.severity}}}}</p>
  <h6>Example Instances</h6>
  <p>{{{{item.example}}}}</p>
  <h6>Mitigations</h6>
  <p>{{{{item.mitigations}}}}</p>
  <h6>References</h6>
  <p>{{{{item.references}}}}</p>
  <h6>Response</h6>
  <p>{{{{item.response}}}}</p>
  &emsp;
</details>

but in the html, that item.response is empty, even though I supply a response="""To Mitigate: foo""", in my Finding().

One of the workarounds I thought I'd try for the invalid element issue, is to redirect the model.py --dfd-colormap output to jq to try to use jq to edit-out the element_invalid_ elements, but this output is not json. It's close to json, but jq won't deal with it.

Hi! This is another issue. I saw the same. It can be overridden by using the property example in the template and Finding. this is because not all fields are used during report generation.
e.g.:

fundings.py

finding_DO01_api_gw_used = Finding(
    threat_id="DO01",
    example="Optional API Gateway can be used to check and limit requests",
)

template.md:

## Findings

{elements:repeat:{{item.findings:if:

### {{item.name}}
{{item.findings:repeat:
---

{{{{item.description}}}}

| # | ThreatID | Severity               | Description              |Status                |
|:-:|:--------:|------------------------|:-------------------------|:----------------------|
| {{{{item.id}}}} | {{{{item.threat_id}}}} | {{{{item.severity}}}} | {{{{item.description}}}} | {{{{item.example}}}} |

**Mitigations**: {{{{item.mitigations}}}}

**References**: {{{{item.references}}}}

}}}}}

I may open another PR to fix this. This PR should not affect other issues or problems.

@oleh-mykytyuk
Copy link
Author

ho @oleh-mykytiuk will you be able to address @neilpvirtualitics comment or should i review/merge as is ?

Hi @izar ! I replied ;) I think that this is another issue and should be fixed by another PR.

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.

5 participants