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

(feat) Add masking to python middleware #955

Merged
merged 15 commits into from
Sep 18, 2024
Merged

(feat) Add masking to python middleware #955

merged 15 commits into from
Sep 18, 2024

Conversation

hoxworth
Copy link
Contributor

🚥 Resolves RM-8742

🧰 Changes

Adds sensitive data masking to the Python SDK

🧬 QA & Testing

Updated and added tests as applicable

@hoxworth hoxworth changed the title Add masking to python middleware (feat) Add masking to python middleware Jan 25, 2024
@gratcliff
Copy link
Member

@AndriiAndreiev when you have a chance, can you take over this PR from Kenny? I'm happy to chat through this live - but the general intention is to use hash any group.id and authorization headers using a sha512 algo. We use @readme/ssri in Node, but we'll need a different solution here.

@AndriiAndreiev AndriiAndreiev marked this pull request as ready for review September 5, 2024 15:39
@@ -224,6 +220,9 @@ def _build_request_payload(self, request) -> dict:
"bodySize": -1,
}

if "Authorization" in headers:
Copy link
Contributor

Choose a reason for hiding this comment

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

at this point, if there is an Authorization header, won't there already be an unmasked Authorization header in payload["headers"]?

Copy link
Member

Choose a reason for hiding this comment

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

That would be my expectation too - does .append overwrite the existing header here, or do we need to forcefully delete it?

Copy link
Member

@gratcliff gratcliff left a comment

Choose a reason for hiding this comment

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

@AndriiAndreiev aside from confirming @llimllib 's comment, this looks good to me! Thank you for picking this up and bringing it to the finish line 😊

@AndriiAndreiev
Copy link
Collaborator

@gratcliff @llimllib I made small changes on "Authorization" header masking so that there is no copy of the original and masked header

@gratcliff gratcliff merged commit b24b2d8 into main Sep 18, 2024
45 checks passed
@gratcliff gratcliff deleted the add-python-hashing branch September 18, 2024 14:59
gratcliff pushed a commit that referenced this pull request Sep 18, 2024
| 🚥 Resolves [RM-8742](https://linear.app/readme-io/issue/RM-8742) |
| :------------------- |

## 🧰 Changes

Adds sensitive data masking to the Dotnet SDK
The logic is the same as we will have in Python SDK (PR #955)
gratcliff pushed a commit that referenced this pull request Sep 23, 2024
| 🚥 Resolves [RM-8742](https://linear.app/readme-io/issue/RM-8742) |
| :------------------- |

## 🧰 Changes

Adds sensitive data masking to the PHP SDK
The logic is the same as we will have in Python SDK (PR #955)
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