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

fix: customer reported that sometimes box_parse_additional_details could raise an error when trying to json.loads #516

Merged
merged 13 commits into from
Sep 29, 2022

Conversation

edyesed
Copy link
Contributor

@edyesed edyesed commented Sep 26, 2022

Background

sometimes event['additional_details'] is typed to not-a-string when processing box events. json.loads() wanted a string-ish input.

Changes

  • This change catches the TypeError being raised by json.loads in panther_base_helpers.box_parse_additional_details and provides a feature-equivalent alternative result set, if the TypeError is for one of the panther ImmutableXXX types.

Testing

  • I added a new unit test file and make target to run unit tests in global_helpers/*_test.py ( note: it only works as long as there's a single file matching the pattern ).

ATTN: @panther-labs/detections for extra 👀

@edyesed edyesed requested review from a team September 26, 2022 23:18
calkim-panther
calkim-panther previously approved these changes Sep 27, 2022
Copy link
Contributor

@darwayne darwayne left a comment

Choose a reason for hiding this comment

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

based on @lindsey-w's latest internal feedback we should move away from introducing panther-core as a dependency


## NOTE: WE DO NOT WANT TO IMPORT panther_analysis_tool into any detection
# we need the types from panther_analysis_tool for unit testing sometimes, though
from panther_core.immutable import ImmutableCaseInsensitiveDict, ImmutableList
Copy link
Contributor

Choose a reason for hiding this comment

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

based on in internal discussion we want to avoid introducing panther_core as dependency to panther-analysis

Pipfile Outdated Show resolved Hide resolved
@edyesed
Copy link
Contributor Author

edyesed commented Sep 29, 2022

@darwayne + @lindsey-w , I believe this is what the internal discussion said we should do

@edyesed edyesed requested a review from darwayne September 29, 2022 16:57
@darwayne darwayne dismissed their stale review September 29, 2022 17:02

changes made

@darwayne darwayne requested a review from lindsey-w September 29, 2022 17:03
Copy link
Contributor

@darwayne darwayne left a comment

Choose a reason for hiding this comment

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

This looks right to me, would like to get additional verification from @lindsey-w

Copy link
Contributor

@lindsey-w lindsey-w left a comment

Choose a reason for hiding this comment

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

Thanks Ed!

@@ -9,6 +9,9 @@ deps:
deps-update:
pipenv update

global-helpers-unit-test:
pipenv run python global_helpers/*_test.py
Copy link
Contributor

Choose a reason for hiding this comment

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

++

@edyesed edyesed merged commit df70f0a into master Sep 29, 2022
@edyesed edyesed deleted the fix/edyesed/box_helpers_caseimmutable branch September 29, 2022 17:25
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.

4 participants