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(masking): fixes multi-level nested fields, and hides non-JSON entries #2004

Merged
merged 2 commits into from
Nov 23, 2024

Conversation

jamfor352
Copy link
Contributor

@jamfor352 jamfor352 commented Nov 20, 2024

Hi! Follow up to previous PR as I realised there were a few small issues after deploying to another environment:

  1. Returning the original values if it isn't JSON-formattable is actually dangerous, as it means if the schema registry is down or if the wrong schema registry is selected in AKHQ in a multi-schema-registry-setup, the binary format data will still show as a String which in most cases still displays some of the sensitive data. To be safe, this means json_mask_by_default will now ONLY show structured data and anything else will display a placeholder message. This prevents any unwanted data leakage.

  2. Fixes multi-level nested objects for the 'mask by default' option. Before we weren't prepending currentKey. So for example given a request to expose one.two.three, one.two.three would still be masked - the filter would have needed to specify just two.three (which is incorrect and would also mean another.two.three would also be unmasked). This fixes it to work as expected, so one.two.three unmasks one.two.three and also isn't applied to another.two.three

… rather than shows them anyway, and fixes multi-nested fields
@jamfor352 jamfor352 changed the title fix(masking): fixes 2 issues fix(masking): fixes multi-level nested fields, and hides non-JSON entries Nov 20, 2024
@jamfor352
Copy link
Contributor Author

Hey @AlexisSouquiere could I get a review on this one please? A much smaller changeset which only fixes the issues described so hopefully should be a lot faster to get in than the previous PR!

Copy link
Collaborator

@AlexisSouquiere AlexisSouquiere left a comment

Choose a reason for hiding this comment

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

Good for me. Thanks for the clarification

@jamfor352
Copy link
Contributor Author

Good for me. Thanks for the clarification

Thank you!
cc: @tchiotludo I won't be making more changes to this branch so feel free to merge whenever suits :)

@tchiotludo tchiotludo merged commit 7a4c389 into tchiotludo:dev Nov 23, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants