WIP: Include in Firehose models multipe CWEs#33
WIP: Include in Firehose models multipe CWEs#33davidCarlos wants to merge 2 commits intofedora-static-analysis:masterfrom
Conversation
|
Exemple with multiple CWEs.
|
| <results> | ||
| <issue cwe="401" test-id="refcount-too-high"> | ||
| <issue cwe="401,200" test-id="refcount-too-high"> | ||
|
|
There was a problem hiding this comment.
Thanks for posting this.
Could you please add a fresh example e.g. taken from the flawfinder example you posted a screenshot of?
I'm not keen on having comma-separated values in the XML; I think it would be cleaner to introduce elements for these IDs, so that client code doesn't have to parse the attributes. Instead we should add a new zero-or-more child element to the XML schema.
We might want to support other categorization schemes - the readme.rst talks about:
potentially with other IDs and URLs, e.g. the ID "SIG30-C" with
URL https://www.securecoding.cert.org/confluence/display/seccode/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
so maybe we want to generalize things accordingly.
| <attribute name="cwe"> | ||
| <data type="integer"/> | ||
| <data type="string"/> | ||
| </attribute> |
There was a problem hiding this comment.
As noted above, this probably should be something like:
<zeroOrMore>
<choice>
<element name="cwe">
<attribute name="cwe">
<data type="integer"/>
</attribute>
</element>
<element name="something-else"/>
</choice>
<zeroOrMore>
or somesuch.
| class Issue(Result): | ||
| attrs = [Attribute('cwe', int, nullable=True), | ||
| attrs = [Attribute('cwe', list, nullable=True), | ||
| Attribute('testid', _string_type, nullable=True), |
There was a problem hiding this comment.
"cwe" should probably be replaced by "externalids" or somesuch, for external IDs. (Not sure of the name). It would be a list of instances of some base class, with CWE being a subclass.
|
|
||
| def get_cwe_str(self): | ||
| cwe_list_str = [] | ||
| if self.cwe is not None: |
There was a problem hiding this comment.
This would become a method of a new CWE class.
|
|
||
|
|
||
| def get_cwe_url(self): | ||
| cwe_list_str = [] |
|
Note that also I've recently added documentation, so any big change like this would need changes to the docs. |
|
Thanks for the review. I will keep working on this PR based on your comments. In the case of PR #31, could we use only one CWE in the final xml for a while (This issue is bigger than i thought)? When we finish this PR we include this new feature in the parsers. |
|
Thanks. Clearly this needs more thought, let's come up with a design for how to handle this (maybe mention the issue on the mailing list?). |
davidCarlos
changed the title
|
Hey @davidmalcolm, i will work again on this PR. I will try to add a new model to support multiples cwes. Probably we will have to fix all the parsers that collects cwes. |
2 participants
This PR allows include in Firehose's xml multiple CWEs related to a same warning. Now the cwe field can have more than one CWE, separated by comma. The Issue model expects a list of CWEs, but to keep compatibility with the existent parsers, pass a integer value still is possible.