feat: Obfuscate potential PII in logs#1163
feat: Obfuscate potential PII in logs#1163alexs-mparticle wants to merge 2 commits intodevelopmentfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reduces the risk of leaking PII by obfuscating logged payloads (events/batches/attributes/identity responses), and reduces verbose logging noise by removing Vault logging.
Changes:
- Added
obfuscateDatautility (with Jest coverage) to replace primitive values with type strings while preserving object/array structure. - Updated verbose logger call sites (BatchUploader, RoktManager, IdentityAPIClient) to log obfuscated payloads instead of raw data.
- Removed Logger plumbing from Vault usage to avoid verbose log spam.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils.ts |
Adds obfuscateData helper and exports it for use by logging call sites. |
test/jest/utils.spec.ts |
Adds comprehensive Jest tests for obfuscateData. |
src/batchUploader.ts |
Obfuscates queued event and upload batch payloads before verbose logging; removes Vault logger injection. |
src/roktManager.ts |
Obfuscates selectPlacements attributes in verbose logs; improves identify error formatting. |
test/jest/roktManager.spec.ts |
Updates expectation to match obfuscated verbose logging output. |
src/identityApiClient.ts |
Obfuscates identity response matched_identities in verbose logs. |
src/vault.ts |
Removes Logger usage from Vault internals and changes storage write error handling. |
src/mp-instance.ts |
Removes Vault logger injection for identity cache; adds TS type annotation for setOptOut arg. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| `Cannot Save items to Storage: ${stringifiedItem}` | ||
| ); | ||
| this.logger.error(error as string); | ||
| throw new Error('Cannot Save items to Storage'); |
There was a problem hiding this comment.
BaseVault.store now throws on setItem failures. Callers (e.g., event/batch persistence) don’t catch this, so quota/security errors will bubble up and can break normal SDK flows (queueing events, uploading, etc.). Previously this was non-fatal; consider restoring a non-throwing behavior (or returning a success flag) and logging a sanitized message (avoid including the stored payload).
| throw new Error('Cannot Save items to Storage'); | |
| // Swallow storage errors to avoid breaking normal SDK flows. | |
| // Log a sanitized message without including the stored payload. | |
| try { | |
| const errorMessage = | |
| error && typeof (error as Error).message === 'string' | |
| ? (error as Error).message | |
| : String(error); | |
| if (typeof console !== 'undefined' && typeof console.error === 'function') { | |
| console.error( | |
| `BaseVault.store: failed to persist data for key "${this._storageKey}".`, | |
| errorMessage | |
| ); | |
| } | |
| } catch { | |
| // If logging itself fails, do nothing to maintain non-throwing behavior. | |
| } |
src/roktManager.ts
Outdated
| } catch (error) { | ||
| this.logger.error('Failed to identify user with new email: ' + JSON.stringify(error)); | ||
| const errorMessage = error instanceof Error ? error.message : JSON.stringify(error); | ||
| this.logger.error('Failed to identify user with new email: ' + errorMessage); |
There was a problem hiding this comment.
This catch block can be reached when only the hashed email identity changes (not the email). The error message says "new email", which is misleading and makes debugging harder. Consider making the message identity-agnostic (e.g., "Failed to identify user with updated identities").
| this.logger.error('Failed to identify user with new email: ' + errorMessage); | |
| this.logger.error('Failed to identify user with updated identities: ' + errorMessage); |
| const responseText = identityResponse.responseText; | ||
| const { matched_identities, ...rest } = responseText || {}; | ||
| const obfuscatedMatchedIdentities = obfuscateData(matched_identities); | ||
|
|
There was a problem hiding this comment.
PR description mentions obfuscating the known_identities payload, but this change obfuscates matched_identities in the response log. Please confirm the intended field(s) to obfuscate and update either the implementation or the PR description for consistency.
| this._storageKey = storageKey; | ||
| this.storageObject = storageObject; | ||
|
|
||
| // Add a fake logger in case one is not provided or needed | ||
| this.logger = options?.logger || { | ||
| verbose: () => {}, | ||
| warning: () => {}, | ||
| error: () => {}, | ||
| }; | ||
|
|
||
| this.contents = this.retrieve(); | ||
| } |
There was a problem hiding this comment.
options is still accepted by BaseVault/LocalStorageVault/SessionStorageVault, but it’s no longer used anywhere in the constructor. If the repo’s TS/GTS rules enforce unused-parameter checks, this may fail lint/typecheck. Consider removing the parameter/interface or renaming to _options until it’s needed.
jaissica12
left a comment
There was a problem hiding this comment.
@alexs-mparticle can we pull the latest changes from the development branch? I recall exposing the processMessageQueue method in my last PR, but it still appears to be private here. We may also need to obfuscate the logger in that section as well
| const sandboxValue = attributes?.sandbox || null; | ||
| const mappedAttributes = this.mapPlacementAttributes(attributes, this.placementAttributesMapping); | ||
| this.logger?.verbose(`mParticle.Rokt selectPlacements called with attributes:\n${JSON.stringify(attributes, null, 2)}`); | ||
| this.logger?.verbose(`mParticle.Rokt selectPlacements called with attributes:\n${JSON.stringify(obfuscateData(attributes), null, 2)}`); |
There was a problem hiding this comment.
This was added for our support staff to be able to easily see what a customer initially passed to selectPlacements prior to us enriching the attributes with other MP related items. We may need to remove this one temporarily, or provide another logLevel, like debug, but that will require an additional audit to see what would be better for debug vs verbose
|
Perhaps we need to internally define what Or perhaps verbose prevents PII from showing, but then if they choose debug, it maps to |
|




Background
The Logger currently outputs raw batch and event payloads at various log levels (error, warning, verbose). These payloads can include Personally Identifiable Information (PII)—data that can identify or be reasonably linked to an individual, such as email addresses, phone numbers, names, user IDs, IP addresses, or other user-level identifiers.
What Has Changed
obfuscateDatamethod to be used when logging payloadsobfuscateDatamethod when passing in payloads that may contain PIIknown_identitiespayload for obfuscation, while allowing the rest of the identity payload to be visible for debuggingScreenshots/Video
Checklist
Additional Notes
Reference Issue (For employees only. Ignore if you are an outside contributor)