Skip to content

fix: avoid UnboundLocalError in SigmaCorrelationRule.from_dict with collect_errors=True - #545

Open
cristianchiriac wants to merge 4 commits into
SigmaHQ:mainfrom
cristianchiriac:fix/sigmacorrelation-collect-errors-unboundlocal
Open

cristianchiriac wants to merge 4 commits into
SigmaHQ:mainfrom
cristianchiriac:fix/sigmacorrelation-collect-errors-unboundlocal

Conversation

@cristianchiriac

@cristianchiriac cristianchiriac commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Two related crashes in SigmaCorrelationRule.from_dict(..., collect_errors=True), which is meant to collect parsing errors into .errors instead of raising:

1. condition UnboundLocalError. When an extended (string) condition is used with a non-temporal correlation type — or condition parsing otherwise fails — the local variable condition is never assigned before being passed to cls(...):

from sigma.correlations import SigmaCorrelationRule
SigmaCorrelationRule.from_dict(
    {
        "title": "Test",
        "correlation": {
            "type": "event_count",
            "rules": ["a"],
            "timespan": "5m",
            "condition": "count() > 5",  # extended condition, non-temporal type
        },
    },
    collect_errors=True,
)
# UnboundLocalError: cannot access local variable 'condition' ...

2. Non-dict correlation field. A non-dict correlation value (None, a string, an int, a list, ...) makes .get("type") raise an unhandled AttributeError, bypassing collect_errors entirely:

SigmaCorrelationRule.from_dict({"title": "Test", "correlation": None}, collect_errors=True)
# AttributeError: 'NoneType' object has no attribute 'get'

Fix

  • Initialize condition to the same default already used for the dataclass field (SigmaCorrelationCondition(GTE, 1)) before the branch that parses it, so the object can still be constructed on the collect_errors=True path while the real error is recorded in .errors.
  • Validate that correlation is a dict up front; if not, record a SigmaCorrelationRuleError (raising it immediately when collect_errors=False, matching the rest of the function) and fall back to an empty dict so the remaining field-level checks still run and report every problem at once.

Test plan

  • Added regression tests in tests/test_correlations.py for both cases (raising and collect_errors=True paths).
  • pytest tests/test_filters.py tests/test_correlations.py passes (125 passed, 1 skipped).
  • Full suite: 1547 passed, 1 skipped, 2 pre-existing failures unrelated to this change (network/pip access in test_plugins.py).

…ollect_errors=True

When an extended (string) condition is used with a non-temporal
correlation type, or condition parsing otherwise fails, and
collect_errors=True, 'condition' was never assigned, so cls() raised
an UnboundLocalError instead of returning the collected errors.
…Rule.from_dict

A non-dict 'correlation' value (None, string, int, list, ...) made
.get("type") raise an unhandled AttributeError, bypassing
collect_errors entirely. Validate the type up front and record it as
a SigmaCorrelationRuleError instead.
@thomaspatzke

Copy link
Copy Markdown
Member

Please reformat with black, one file is missing. Wasn't able to push to the PR.

@cristianchiriac

Copy link
Copy Markdown
Contributor Author

Thanks for flagging this. I ran black 26.5.1 (the version pinned in the pre-commit config) on tests/test_correlations.py and pushed the result in 5cfc620. black --check . now passes on all 100 files, so CI should be able to run the full test matrix again.

@thomaspatzke

Copy link
Copy Markdown
Member

Now there are mypy type validation issues.

@cristianchiriac

Copy link
Copy Markdown
Contributor Author

Fixed in ee2d3ad. The new isinstance check made mypy see correlation_rule as a dict, so .get() became optional and it complained about type and timespan. I typed it as Any like it was before this PR. mypy passes locally now.

This branch has not been deployed

No deployments
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