Multi threat libs by izar · Pull Request #263 · OWASP/pytm

izar · 2025-02-20T20:01:23Z

Adds capability to select which threat lib to use on the command line:

nothing in the command line: uses the default library
--threat-files foo.json : uses the threats in foo.json only
--threat-files foo.json default : uses the threats in foo.json and the default ones

…files

raphaelahrens · 2025-02-21T10:14:25Z

tests/output.json

    "onDuplicates": "Action.NO_ACTION",
    "threatsExcluded": [],
-    "threatsFile": "pytm/threatlib/threats.json"
+    "threatsFile": "{'pytm/threatlib/threats.json'}"


Why is the output a string and not a list of strings?

that's what varStrings with only one value put out...

Is this a bug?

in the immortal words of just about everybody..."works for me"!

Ok I had a quick look and the issue is that the default to_serializable is used.

pytm/pytm/pytm.py

Lines 2015 to 2018 in 9dc0f1f

@singledispatch

def to_serializable(val):

"""Used by default."""

return str(val)

This is just the same as

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

The issue is created by

pytm/pytm/pytm.py

Line 2023 in 9dc0f1f

return serialize(obj, nested=True)

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

pytm/pytm/pytm.py

Line 2064 in 9dc0f1f

not nested

pytm/pytm/pytm.py

Lines 2035 to 2070 in 9dc0f1f

def serialize(obj, nested=False):

"""Used if *obj* is an instance of TM, Element, Threat or Finding."""

klass = obj.__class__

result = {}

if isinstance(obj, (Actor, Asset)):

result["__class__"] = klass.__name__

for i in dir(obj):

if (

i.startswith("__")

or callable(getattr(klass, i, {}))

or (

isinstance(obj, TM)

and i in ("_sf", "_duplicate_ignored_attrs", "_threats")

)

or (isinstance(obj, Element) and i in ("_is_drawn", "uuid"))

or (isinstance(obj, Finding) and i == "element")

):

continue

value = getattr(obj, i)

if isinstance(obj, TM) and i == "_elements":

value = [e for e in value if isinstance(e, (Actor, Asset))]

if value is not None:

if isinstance(value, (Element, Data)):

value = value.name

elif isinstance(obj, Threat) and i == "target":

value = [v.__name__ for v in value]

elif i in ("levels", "sourceFiles", "assumptions"):

value = list(value)

elif (

not nested

and not isinstance(value, str)

and isinstance(value, Iterable)

):

value = [v.id if isinstance(v, Finding) else v.name for v in value]

result[i.lstrip("_")] = value

return result

This whole serialize function is a bit overloaded with special handling of member variables and might require 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.

A quick fix would be something like if instance(obj, TM) and i == "threatsFile" but oh boy that is not nice.

Should this be a new issue?

sounds like something to be addressed. @nineinchnick , you there?

There appears to be only 1 check for nested. If nested is true, the behavior seems to be potentially undefined (if the code reaches the not nested elif).

Yes. I started to rewrite the code see #268, but it is difficult to understand what the intention here was.
It seems that the default is result[i.lstrip("_")] = value and the cases above only change the value of value.
So setting nested = True means that value will not be touched, except it is a Element, Data, or Threat. Or the name of the member is in ("levels", "sourceFiles", "assumptions").

The last one seems to be a fix for a specific class.

pytm/pytm.py

.gitignore

colesmj · 2025-03-28T17:04:55Z

README.md

-             [--exclude EXCLUDE] [--seq] [--list] [--describe DESCRIBE]
-             [--list-elements] [--json JSON] [--levels LEVELS [LEVELS ...]]
-             [--stale_days STALE_DAYS]
+usage: tm.py [-h] [--sqldump SQLDUMP] [--debug] [--dfd] [--report REPORT] [--exclude EXCLUDE] [--seq] [--list] [--colormap]


Are you adding --colormap in this PR also?

it should be there already, it is an old one. It appears on the currently checked code on the master branch.

oh this is the readme! let me fix that

pytm/pytm.py

colesmj · 2025-03-28T17:29:38Z

pytm/pytm.py


+        # delaying loading of threats to accomodate multiple threat files in the
+        # command line
+        if result.threat_files:


Why do extra work on line 791 to load the default when you can just do it here? You will always reach this line at least once, correct?

i don't remember the details but there is a timing issue there. Something around things being used before loaded.

colesmj · 2025-03-28T17:33:40Z

tests/output.json

    "onDuplicates": "Action.NO_ACTION",
    "threatsExcluded": [],
-    "threatsFile": "pytm/threatlib/threats.json"
+    "threatsFile": "{'pytm/threatlib/threats.json'}"


There appears to be only 1 check for nested. If nested is true, the behavior seems to be potentially undefined (if the code reaches the not nested elif).

colesmj · 2025-03-28T17:39:12Z

tests/test_pytmfunc.py

+        foo_file = f"{dir_path}/1.json"
+        bar_file = f"{dir_path}/2.json"
+        threat_files = [os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))
+    + "/pytm/threatlib/threats.json", foo_file, bar_file]


Do you have a test for when the default is not included in threatsfile?

Hm. No. Gotta do one, you're right.

adaFPF · 2025-08-20T17:43:40Z

Hi there! I'd love to see this feature get accepted and ready for use downstream. My understanding is that the following is what's holding it up:

Refactor of the serialize and to_serializable function #270 , which is in need of review, i.e. something I'm not in a position help with
Amending the test to handle the "no inclusion of default file" case

Anything else?

I can at least push an update to the test. Would y'all prefer it directly pushed to this branch, or submitted as a separate PR to this branch?

izar · 2025-10-09T19:10:06Z

@adaFPF sorry for the long silence - work overrode life. Pushing to this branch should be ok.

colesmj · 2025-10-19T16:37:27Z

@izar I resolved the merge conflict on gitignore so this change can be merged.

colesmj · 2025-10-19T16:44:03Z

@izar , @raphaelahrens is resolution for #268 + #270 a dependency for this (263)?

raphaelahrens · 2025-10-19T17:48:16Z

@izar , @raphaelahrens is resolution for #268 + #270 a dependency for this (263)?

There is the issue of the wrong serialisation.
#263 (comment)

But I would say #268 that this is not blocking this PR.

izar tarandach added 5 commits February 12, 2025 14:55

feat: Support multiple threat files in threat management system

e0b5c1a

fix: Correct threatsFile initialization to accept a list of strings

7be52ea

fix: Correct file opening in _add_threats to iterate over threat files

370ccb6

test: Update threat count assertion and add test for multiple threat …

2ec73a6

…files

Adding support for multi threat libraries following issue #261

544fa6f

izar requested a review from colesmj February 20, 2025 20:01

oops, these are needed for the tests

6da8601

raphaelahrens reviewed Feb 21, 2025

View reviewed changes

pytm/pytm.py Outdated Show resolved Hide resolved

izar tarandach and others added 2 commits February 21, 2025 08:55

better have some documentation as well...

b546efb

fixed wrong file in error message (thanks @raphaelahrens!)

9dc0f1f

raphaelahrens mentioned this pull request Mar 22, 2025

Fix to_serializable #268

Open

colesmj reviewed Mar 28, 2025

View reviewed changes

Merge branch 'master' into multi-threat-libs

f84c8f4

izar requested review from colesmj and raphaelahrens October 26, 2025 15:36

	@singledispatch
	def to_serializable(val):
	"""Used by default."""
	return str(val)

	def serialize(obj, nested=False):
	"""Used if obj is an instance of TM, Element, Threat or Finding."""
	klass = obj.__class__
	result = {}
	if isinstance(obj, (Actor, Asset)):
	result["__class__"] = klass.__name__
	for i in dir(obj):
	if (
	i.startswith("__")
	or callable(getattr(klass, i, {}))
	or (
	isinstance(obj, TM)
	and i in ("_sf", "_duplicate_ignored_attrs", "_threats")
	)
	or (isinstance(obj, Element) and i in ("_is_drawn", "uuid"))
	or (isinstance(obj, Finding) and i == "element")
	):
	continue
	value = getattr(obj, i)
	if isinstance(obj, TM) and i == "_elements":
	value = [e for e in value if isinstance(e, (Actor, Asset))]
	if value is not None:
	if isinstance(value, (Element, Data)):
	value = value.name
	elif isinstance(obj, Threat) and i == "target":
	value = [v.__name__ for v in value]
	elif i in ("levels", "sourceFiles", "assumptions"):
	value = list(value)
	elif (
	not nested
	and not isinstance(value, str)
	and isinstance(value, Iterable)
	):
	value = [v.id if isinstance(v, Finding) else v.name for v in value]
	result[i.lstrip("_")] = value
	return result

Conversation

izar commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adaFPF commented Aug 20, 2025

Uh oh!

izar commented Oct 9, 2025

Uh oh!

colesmj commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colesmj commented Oct 19, 2025

Uh oh!

raphaelahrens commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

colesmj commented Oct 19, 2025 •

edited

Loading

raphaelahrens commented Oct 19, 2025 •

edited

Loading