feat(logger): add PII masking and safe object log formatting#35
feat(logger): add PII masking and safe object log formatting#35tr-emp-260 wants to merge 7 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a recursive PII masking utility and integrates it into the logger to mask info.message; includes unit tests, an example, README docs, package.json test tooling update, and a .gitignore addition. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Client/App
participant Logger as Logger
participant Mask as maskData
participant Transport as Console/File
App->>Logger: log(info)
activate Logger
Logger->>Mask: maskData(info.message)
activate Mask
Mask-->>Logger: maskedMessage
deactivate Mask
Logger->>Logger: format log line with maskedMessage + metadata
Logger->>Transport: write(formattedLog)
deactivate Logger
Transport-->>App: ack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/logger/logger.js (1)
37-37:⚠️ Potential issue | 🔴 CriticalBug:
userTypeis undeclared —uservariable is never updated.
userTypeis not declared anywhere in this scope. In non-strict mode this silently creates a global variable, while in strict mode it would throw aReferenceError. Either way, theuservariable (declared on Line 8, used on Line 47) is never updated byupdateLogData, so logs will always showUSER: sys_defaultregardless of what's passed in.Should be
user = data["user"].🐛 Proposed fix
const updateLogData = (data) => { if (data["requestId"]) requestId = data["requestId"]; if (data["userId"]) userId = data["userId"]; - if (data["user"]) userType = data["user"]; + if (data["user"]) user = data["user"]; };
🤖 Fix all issues with AI agents
In `@src/logger/mask.js`:
- Around line 6-9: The phone regex in the mask config (the object with the regex
and replacer) is too broad and matches any 10+ digit sequence; replace it with a
stricter approach: either narrow the regex to require a leading '+' or
parenthesized country code and limit total length (e.g., allow country code +
7–12 digits) or remove the free-form regex and add key-based masking for object
properties named phone/phoneNumber/mobile similar to how name is handled; update
the replacer to remain "**********" and ensure the change targets the same regex
property/replacer pair so other numeric sequences (order IDs, timestamps) are no
longer masked.
🧹 Nitpick comments (1)
src/logger/mask.js (1)
37-46: Region masking doesn't apply regex sanitization to un-listed string fields.Fields spread from
valuethat aren't explicitly listed (e.g., any future additions to the region object) will pass through unmasked. More importantly, if someone adds a field likelandmarkcontaining PII, it won't be caught. This is a minor extensibility concern — just worth noting for future maintenance.Also, string values inside the explicitly masked fields (address, city, etc.) are replaced entirely with
MASK, so embedded emails/phones in those fields are handled implicitly. That part is fine.
| { | ||
| regex: /\+?\d[\d\s\-]{8,}\d/g, | ||
| replacer: () => "**********" | ||
| } |
There was a problem hiding this comment.
Phone regex is overly broad — will mask non-phone numeric sequences.
The pattern \+?\d[\d\s\-]{8,}\d matches any 10+ digit sequence, which means order IDs, timestamps, transaction amounts, and other numeric identifiers will be incorrectly masked. For example, "Order 1234567890 confirmed" would have the order ID replaced with "**********".
Consider tightening the regex (e.g., requiring a leading + or parenthesized country code, or restricting match length) or switching to a key-based approach for phone masking in objects, similar to how name is handled.
🤖 Prompt for AI Agents
In `@src/logger/mask.js` around lines 6 - 9, The phone regex in the mask config
(the object with the regex and replacer) is too broad and matches any 10+ digit
sequence; replace it with a stricter approach: either narrow the regex to
require a leading '+' or parenthesized country code and limit total length
(e.g., allow country code + 7–12 digits) or remove the free-form regex and add
key-based masking for object properties named phone/phoneNumber/mobile similar
to how name is handled; update the replacer to remain "**********" and ensure
the change targets the same regex property/replacer pair so other numeric
sequences (order IDs, timestamps) are no longer masked.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/__tests__/logger.test.js`:
- Around line 55-60: The test name is misleading: update the jest test
description for the test using maskData that currently reads "should fully mask
phone <=3 digits" to reflect that inputs of length <=3 should not be changed
(e.g., "should not mask phone <=3 digits" or similar); locate the test block
containing the maskData call and the input "123" and change only the test name
string to match the inline comment and expected assertion.
🧹 Nitpick comments (2)
src/__tests__/logger.test.js (2)
145-161: Toggle test doesn't fully restore module state after mutation.Setting
process.env.MASK_LOGS = "true"on line 160 doesn't re-load the module, soENABLE_MASKINGinside the cachedmask.jsfrom line 153 remainsfalse. This is harmless today since no subsequent tests use that reference, but it's fragile. Consider usingafterEach/afterAllwithjest.resetModules()and restoring the env to avoid ordering issues as the test suite grows.Suggested approach
describe("Masking Toggle Tests", () => { + const originalEnv = process.env.MASK_LOGS; + + afterEach(() => { + process.env.MASK_LOGS = originalEnv; + jest.resetModules(); + }); test("should return original value when MASK_LOGS=false", () => { process.env.MASK_LOGS = "false"; - jest.resetModules(); const { maskData } = require('../logger/mask'); const input = "test@gmail.com"; const output = maskData(input); expect(output).toBe("test@gmail.com"); - - process.env.MASK_LOGS = "true"; // restore for other tests }); });
9-14: Consider stronger assertions for email masking.The test only checks that the output contains
"@"and isn't the original. It doesn't verify the masking format (e.g., partial redaction pattern). A snapshot or regex match on the expected masked format would catch regressions more reliably.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/logger/mask.js`:
- Around line 88-124: maskObject currently recurses without guarding against
circular references; update maskObject to accept an optional seen WeakSet (e.g.,
seen = new WeakSet()) and optionally a depth counter with a MAX_DEPTH constant,
then before recursing check if seen.has(obj) or depth >= MAX_DEPTH and return
the object (or a safe masked placeholder) to avoid infinite recursion; when
first visiting an object call seen.add(obj) and pass seen (and depth+1) into
recursive maskObject calls for nested objects so circular references and deep
nesting are safely handled.
- Around line 102-111: The region handling block in src/logger/mask.js fails
when region is null because typeof null === "object"; update the guard in the
region branch (the if that checks key === "region") to ensure value is not null
(e.g., check value !== null or value) before accessing
value.address/value.city/etc., then keep the cloned[key] assignment that masks
address/city/state/zipcode only when value is a non-null object.
- Around line 65-72: The masking logic in maskName produces a string one
character too long for parts with length > 3; update the repeat count so the
masked part preserves the same total length by changing the repeated-star count
from part.length - 2 to part.length - 3 in the map callback (the code that
builds the masked string using part[0], "*".repeat(...), and part.slice(-2)
inside the maskName logic).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/logger/mask.js`:
- Around line 104-113: The region branch in src/logger/mask.js currently spreads
...value and only masks address/city/state/zipcode, allowing any other fields to
bypass masking; change the logic in the key === "region" branch to first apply
the existing recursive maskObject (or the module's recursive masking routine) to
value, then assign cloned[key] to that masked result and overwrite
address/city/state/zipcode with MASK where present (using the constant MASK) so
unknown or nested PII in the region object is also masked while preserving the
explicit overrides for the four known fields.
| if (key === "region" && value!==null && typeof value === "object") { | ||
| cloned[key] = { | ||
| ...value, | ||
| address: value.address ? MASK : value.address, | ||
| city: value.city ? MASK : value.city, | ||
| state: value.state ? MASK : value.state, | ||
| zipcode: value.zipcode ? MASK : value.zipcode | ||
| }; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Region sub-object fields beyond the four listed ones skip masking entirely.
The region branch spreads ...value and only masks address, city, state, and zipcode. Any other fields present in the region object (e.g., email, phone, country, or nested sub-objects) are preserved verbatim — they bypass both regex masking and recursive maskObject. If a region object ever carries additional PII fields, they will leak into logs.
Consider recursively masking the region object first, then overwriting the four known fields:
Proposed fix
if (key === "region" && value!==null && typeof value === "object") {
+ const maskedRegion = maskObject(value, seen);
cloned[key] = {
- ...value,
- address: value.address ? MASK : value.address,
- city: value.city ? MASK : value.city,
- state: value.state ? MASK : value.state,
- zipcode: value.zipcode ? MASK : value.zipcode
+ ...maskedRegion,
+ address: value.address ? MASK : value.address,
+ city: value.city ? MASK : value.city,
+ state: value.state ? MASK : value.state,
+ zipcode: value.zipcode ? MASK : value.zipcode
};
return;
}🤖 Prompt for AI Agents
In `@src/logger/mask.js` around lines 104 - 113, The region branch in
src/logger/mask.js currently spreads ...value and only masks
address/city/state/zipcode, allowing any other fields to bypass masking; change
the logic in the key === "region" branch to first apply the existing recursive
maskObject (or the module's recursive masking routine) to value, then assign
cloned[key] to that masked result and overwrite address/city/state/zipcode with
MASK where present (using the constant MASK) so unknown or nested PII in the
region object is also masked while preserving the explicit overrides for the
four known fields.
1 participant
This PR introduces log masking to prevent sensitive data exposure in application logs.
**Masking rules cover:-
Email addresses (regex-based)
Phone numbers (regex-based)
name field (key-based)
region address components (key-based)
Key Changes
Added masking utility (mask.js)
Updated logger formatter to support object-aware masking
Ensured masking is applied before serialization
Preserved backward compatibility with existing logger API
Behavior
String logs → Regex masking applied
Object logs → Key-aware masking + recursive handling
Non-sensitive fields → Preserved
Motivation
Prevents accidental PII leakage through console/file logs while maintaining log readability and debugging value.
Summary by CodeRabbit
New Features
Documentation
Examples
Tests
Chores
Bug