Skip to content

fix: reject unknown names in threat conditions - #362

Open
rajanpanth wants to merge 3 commits into
OWASP:masterfrom
rajanpanth:fix/reject-unknown-threat-condition-names
Open

fix: reject unknown names in threat conditions#362
rajanpanth wants to merge 3 commits into
OWASP:masterfrom
rajanpanth:fix/reject-unknown-threat-condition-names

Conversation

@rajanpanth

Copy link
Copy Markdown

Summary

  • reject unknown names while validating threat condition expressions
  • preserve names introduced by list/generator comprehensions
  • add regression coverage for unknown names and comprehension variables

Testing

  • py -3.12 -m compileall pytm\threat.py tests\test_pydantic_models.py
  • git diff --check
  • py -3.12 -m pytest tests\test_pydantic_models.py::TestConditionValidator -q in a temporary local venv

@rajanpanth
rajanpanth requested a review from izar as a code owner August 15, 2026 16:13
@NoodlesNZ

Copy link
Copy Markdown
Contributor

Looks fine to me, although should include some additional changes:

_build_eval_globals should include an entry for LLM. While this doesn't break anything right now, if someone was writing threats using this element then it would error out.

We should also include a mention in CHANGELOG when we next release as this is a breaking change and may catch out legacy users.

@rajanpanth

Copy link
Copy Markdown
Author

Both points addressed.

LLM is now in _build_eval_globals (2e40cf1). It was the only element class missing from the map, so conditions referencing it raised NameError even though eight shipped threats target the element.

The CHANGELOG entry is reworked in 97a4fbe. I'd originally filed the LLM addition itself under BREAKING, which was wrong — the breaking part is the strict rejection of unknown names. That's now its own entry describing what legacy users will actually hit (fail-fast when the Threat is constructed, instead of NameError later during evaluation) and how to resolve it, with the LLM fix moved under Fixed.

On the environment — fair. I have the suite running locally now. pytest tests/ gives 243 passed, 4 failed: test_dfd, test_dfd_colormap, test_dfd_duplicates_ignore and test_multilevel_dfd, all Windows path-separator diffs (pytm/images/... vs pytm\images\...). They fail identically on master, so they're not from this PR.

One thing I hit that may be worth its own issue: tests/test_pytmfunc.py can't be collected at all on Windows without PYTHONUTF8=1, because line 38 opens threats.json without an explicit encoding and cp1252 fails on it.

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.

2 participants