pytm icon indicating copy to clipboard operation
pytm copied to clipboard

Fix `to_serializable`

Open raphaelahrens opened this issue 9 months ago • 0 comments

In a recent PR https://github.com/OWASP/pytm/pull/263 the to_serializable function misbehaved and serialized a set of strings into the string "{'pytm/threatlib/threats.json'}". The issue is caused by the default to_serializable, which is used because there seems to be no rule to serialize the member variable threatsFile of the class TM.

to_serializable is a @singledispatch function.

https://github.com/OWASP/pytm/blob/0c8f23c4feed1970c205962cea23c8166ba7739e/pytm/pytm.py#L1999-L2002

But the potential of @singledispatch is not fully used here since the it is only used in the default variant (seen above) and the two overload variants serialize(obj, nested=True) and serialize(obj, nested=False)

https://github.com/OWASP/pytm/blob/0c8f23c4feed1970c205962cea23c8166ba7739e/pytm/pytm.py#L2005-L2016

The reason why the string "{'pytm/threatlib/threats.json'}" is created is that the default to_serializable is used. This is just the same as

json.dumps(str(set(["./a.json"])))

The issue is created by https://github.com/OWASP/pytm/blob/0c8f23c4feed1970c205962cea23c8166ba7739e/pytm/pytm.py#L2007

Because of this check for not nested in serialize().

https://github.com/OWASP/pytm/blob/0c8f23c4feed1970c205962cea23c8166ba7739e/pytm/pytm.py#L2048

https://github.com/OWASP/pytm/blob/0c8f23c4feed1970c205962cea23c8166ba7739e/pytm/pytm.py#L2019-L2054

The serialize function is overloaded with special handling of member variables and requires a rewrite. All the checks seem to be for specific classes which are all handle in the same function. Also why is there the check for nested? This is basically equivalent to checking if the class is TM.

In summary we have two types of overloading in the function to_serializable, one via @singledispatch and one via the use of the use of isinstance and the nested parameter in serialize. I propose to move to just use @singledispatch and move the isinstance functionality of serialize into to_serializable. Maybe remove serialize in the process or at least make it less complex.

The serialize function is also used in sqlDump. https://github.com/OWASP/pytm/blob/0c8f23c4feed1970c205962cea23c8166ba7739e/pytm/pytm.py#L1272 Is is unclear if it can be replaced with to_serializable.

raphaelahrens avatar Mar 22 '25 10:03 raphaelahrens