Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update deep_walk and unit test #1447

Closed
wants to merge 1 commit into from

Conversation

biancafu-panther
Copy link
Contributor

Background

Discovered that deep_walk only returns unique values found instead of all values.

Changes

  • updated deep_walk function to return all values

Testing

  • updated deep_walk unit tests for the function change

@biancafu-panther biancafu-panther requested a review from a team as a code owner December 9, 2024 22:30
Copy link
Contributor

@arielkr256 arielkr256 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@biancafu-panther what prompted this change? I'm worried that updating a core helper like deep_walk could have unintended consequences for custom rules using this function. We could either add a new deep_walk_all helper, or add a named argument to deep_walk like show_all which defaults to False so that the default behavior of the function is unchanged. We would also need to update the deep_walk function in panther_core to match.

@geoffg-sentry
Copy link
Contributor

As a customer with a bunch of custom rules, I've only ever expected unique values to be returned from a deep_walk given the OrderedDict. I can imagine other folks might not notice it though since it's not explicitly called out in the docs today https://docs.panther.com/detections/rules/python#deep_walk

Use deep_walk() to return values associated with keys that are deeply nested in Python dictionaries, which may contain any number of dictionaries or lists. If it matches multiple event fields, an array of matches will be returned; if only one match is made, the value of that match will be returned.

@biancafu-panther
Copy link
Contributor Author

Hey @arielkr256! The initial motivation for creating this PR was a customer case where deep_walk was used with the expectation that it would return all values, not just unique ones. However, I think what you said makes total sense. I do agree with what @geoffg-sentry mentioned above that maybe we can add a note in the documentation to clarify this behaviour.

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.

3 participants