Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where empty email headers (like Cc, Bcc) were incorrectly encoded as empty arrays instead of empty strings when processed by the proxy.
Key Changes:
- Updated
SanitizerUtils.getPseudonymizeEmailHeaderToJsonto return an empty string for blank email header values instead of an empty array - Added test coverage for the empty header scenario with a new test case
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
java/core/src/main/java/co/worklytics/psoxy/impl/SanitizerUtils.java |
Changed return value from new ArrayList<>() to "" for blank email headers; added TODO comment about future v0.6 improvements |
java/core/src/test/java/co/worklytics/psoxy/rules/google/GMailTests.java |
Added new test case for messages with empty CC headers |
docs/sources/google-workspace/gmail/example-api-responses/original/message_emptyCC.json |
Added example original API response with empty Cc header |
docs/sources/google-workspace/gmail/example-api-responses/sanitized/message_emptyCC.json |
Added expected sanitized output showing empty Cc as empty string |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "value":"" | ||
| }, | ||
| { | ||
| "name":"bcC", |
There was a problem hiding this comment.
Corrected capitalization of 'bcC' to 'Bcc' to match standard email header naming conventions.
| "name":"bcC", | |
| "name":"Bcc", |
| "value": "" | ||
| }, | ||
| { | ||
| "name": "bcC", |
There was a problem hiding this comment.
Corrected capitalization of 'bcC' to 'Bcc' to match standard email header naming conventions.
| "name": "bcC", | |
| "name": "Bcc", |
|
|
||
| if (StringUtils.isBlank((String) value)) { | ||
| return new ArrayList<>(); | ||
| return ""; // empty string |
There was a problem hiding this comment.
so this had nothing do do with pseudonym format directly, but maybe bc of poor code structure where we ran urlsafe encoding here (see L350), but in JSON format case we run outside, this when to "[]" in that case, where as in URLSAFE case it's left as []???
There was a problem hiding this comment.
ok; questions here. isBlank supports null+whitespaced+empty.
If the header is null, with this are we going to return an empty string instead of null.
Does it make sense to handle null case too ?
There was a problem hiding this comment.
yeah, that's fair point. should preserve if we can.
There was a problem hiding this comment.
nm, null is checked up at L325; so it DOES preserve null if header was null
|
|
||
| if (StringUtils.isBlank((String) value)) { | ||
| return new ArrayList<>(); | ||
| return ""; // empty string |
There was a problem hiding this comment.
ok; questions here. isBlank supports null+whitespaced+empty.
If the header is null, with this are we going to return an empty string instead of null.
Does it make sense to handle null case too ?
* Bump org.apache.httpcomponents.client5:httpclient5 in /java/gateway-core Bumps [org.apache.httpcomponents.client5:httpclient5](https://github.com/apache/httpcomponents-client) from 5.4.1 to 5.4.3. - [Changelog](https://github.com/apache/httpcomponents-client/blob/rel/v5.4.3/RELEASE_NOTES.txt) - [Commits](apache/httpcomponents-client@rel/v5.4.1...rel/v5.4.3) --- updated-dependencies: - dependency-name: org.apache.httpcomponents.client5:httpclient5 dependency-version: 5.4.3 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> * prep v0.5.12 * Salesforce rules refresh (#1039) * Update rules * Support example with POST * bump many dependencies (#1038) * apache commons codec to 1.19.0 * more dep updates * webhook collector auth perf (#1036) * add logic to keep lambdas warm * fix style * init public keys in mem for webhook collectors on start-up * aws replay tool (#1034) * aws replace tool * add documentation * webhook collector output prefix support (#1035) * improve webhook collector mode configuration * support webhook output prefix * fix style * fix path stuff * fix missed rename * fix various test issues * gmail empty header fix (#1040) * deal with empty header case more correctly; that's quite clearly a bug * add test of empty CC case * GCP VPC doc improvements (#1042) * improve GCP vpc docs + conditions * fix style, unrelated * improve error feedback on network connectivity issues * doc that GCP VPC needs external connectivity via router/nat for non-google sources * move validation up to dop * try to skip compile in tests * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix some references to role constants (#1043) * config to deploy packages as mvn libs via GitHub Packages (#1041) * improve OAuth token refresh (#1037) * interface to secret store to get multiple versions of things * refactor how token refreshin works, so potentially more proactive / better re-use * pick most recent token based on expirationDate * cr feedback * fix refactor * remove stray bracket * Update java/core/src/main/java/co/worklytics/psoxy/gateway/impl/oauth/OAuthRefreshTokenSourceAuthStrategy.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update java/core/src/main/java/co/worklytics/psoxy/gateway/impl/CompositeConfigService.java Co-authored-by: aperez-worklytics <75276364+aperez-worklytics@users.noreply.github.com> * drop sort on lastAccessDate; only date-level granularity, so why bother --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: aperez-worklytics <75276364+aperez-worklytics@users.noreply.github.com> * more psoxy constants (#1044) * expose more outputs relevant to gcp-hosted deployments * gcp min perms to host * expose gcp network roles, for completenes * doc gcp vpc roles * doc that Project IAM Admin is required * fix docs * Update infra/modules/psoxy-constants/main.tf Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update infra/modules/psoxy-constants/outputs.tf Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update docs/gcp/vpc.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * prep v0.5.12 (#1046) * fix bad comment * try to make sbom stuff more robust * drop mvn plugins that are problematic, see to cause build errors * use reactor build * sboms * fix sbom generation * update package-lock in psoxy-test * fix gcp release artifact script * prompt user if gcp artifact has already been published * prompt user if aws artifact has already been published * warn if we're NOT running in the expected place --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: aperez-worklytics <75276364+aperez-worklytics@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fixes
Change implications