OWASP / pytm

A Pythonic framework for threat modeling
Other
923 stars 175 forks source link

Fixed #221 Got an error "AttributeError: 'str' ... #242

Closed raphaelahrens closed 7 months ago

raphaelahrens commented 7 months ago

When pytm was run with the --sqldump flag with the example tm.py from the repository the execution failed with

AttributeError: 'str' object has no attribute 'name'"

This was caused by the assumptions attribute https://github.com/izar/pytm/blob/6ca9f75ddaa5bda3503a6b8cbce5e6700e03e644/tm.py#L20-L22

When dumping the model into the database all attributes of the TM class are turned into strings, by first turning the obj into a dictionary, where specific attributes are removed and some are converted, and then each value in the dictionary are turned into strings.

This filtering and conversion is done by the serilaize(obj, nested=False) function. sqlDump transforms the values into strings.

The problem in #221 was that when nested is false the default behavior of serialize() is to assume that any list of values holds objects which have either a .name or are an instance of Finding. Since assumptions is a list of strings this fails.

The fix was to add assumptions to an already existing check for similar attributes. Also the check was changed from i == x or ... to an in check.

But to be honest this code is very complex and holds many assumptions, which are not true for all classes and is constantly checking the type of the class. Maybe it would be best to write specific serialize functions for some classes, and only have a genral serialize function which takes in an object and a blacklist of attributes. The to_serializable singledispatch function already crates special functions for each class for the JSON conversion, maybe this can be extended.

izar commented 7 months ago

Thanks, now I am wondering why this passed the tests so far.

raphaelahrens commented 7 months ago

Good question, because the tests don't have an assumption in them. Should have checked the test.

raphaelahrens commented 7 months ago

Another reason could be because there is no test for sql.