fix: redact PII in user-data error logging#489
Conversation
- Replace bare catch blocks with proper error capture and structured logging - Log errors with function name and query parameters for observability - Rethrow errors so callers can differentiate DB failures from not-found results - Follow existing structured logging pattern from lib/api-utils.ts Addresses CodeRabbit review on PR piyushdotcomm#488
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
WalkthroughAdds recursive sensitive-key redaction and a structured logError helper to lib/user-data.ts, and updates getUserById, getUserByEmail, and getAccountByUserId to call logError(...) in their catch blocks while still returning null on failure. ChangesCentralized error logging for user data operations
Sequence Diagram(s)sequenceDiagram
participant getUserFunc as getUserById/getUserByEmail/getAccountByUserId
participant Redactor as redactParams
participant Logger as logError
participant Console as console.error
getUserFunc->>Redactor: redact input params
Redactor->>Logger: return redacted params
getUserFunc->>Logger: call logError(functionName, redactedParams, error)
Logger->>Console: write JSON log (timestamp, context, params, message/stack)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
👋 Thanks for opening a PR, @narutamaaurum!Your PR has entered the 🚦 PR Review Pipeline.
What happens next
A pipeline status comment will appear below and update automatically as your PR progresses. While you wait
This comment is posted only once. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/user-data.ts (1)
3-4: 💤 Low valueConsider narrowing the "key" pattern to reduce false positives.
The substring
"key"will match legitimate non-sensitive fields likeprimaryKey,foreignKey,cacheKey,indexKey. Consider using more specific patterns like"apikey","api_key", or"secretkey"instead of the bare"key".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/user-data.ts` around lines 3 - 4, The SENSITIVE_KEYS array currently includes the overly-broad token "key" which causes false positives; update SENSITIVE_KEYS (the constant in lib/user-data.ts) to replace "key" with narrower patterns such as "apikey", "api_key", "secretkey" and optionally "private_key" or "access_key" so that legitimate fields like primaryKey/foreignKey aren't matched; ensure any code that uses SENSITIVE_KEYS for detection still works with the new entries (adjust normalization/lookup if it assumed substring matching).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/user-data.ts`:
- Around line 5-16: redactParams only inspects top-level keys so nested
objects/arrays with sensitive fields (e.g., user.email) remain unredacted;
update redactParams to perform a recursive traversal: for objects iterate
entries and call the same redaction logic on nested values, for arrays map each
element through the redactor, and when a key (use the lowercased key check
against SENSITIVE_KEYS) matches redact the entire value to "[REDACTED]"; ensure
you return a new structure (do not mutate inputs) and reference the existing
redactParams function and SENSITIVE_KEYS constant when implementing the
recursive helper.
- Around line 40-43: The DB helper functions getUserById, getUserByEmail, and
getAccountByUserId currently log and rethrow errors which causes the auth.ts jwt
callback (which calls getUserById(token.sub) without try/catch) to fail on
transient DB errors; change those functions to catch/log the error and return
null (or undefined) instead of rethrowing so callers like the jwt callback treat
DB failures as “no user” and continue, or alternately add a try/catch around the
getUserById call in the jwt callback to handle errors and degrade to returning
the token; choose one approach and apply consistently to the named
functions/call site so the jwt flow never throws on DB errors.
---
Nitpick comments:
In `@lib/user-data.ts`:
- Around line 3-4: The SENSITIVE_KEYS array currently includes the overly-broad
token "key" which causes false positives; update SENSITIVE_KEYS (the constant in
lib/user-data.ts) to replace "key" with narrower patterns such as "apikey",
"api_key", "secretkey" and optionally "private_key" or "access_key" so that
legitimate fields like primaryKey/foreignKey aren't matched; ensure any code
that uses SENSITIVE_KEYS for detection still works with the new entries (adjust
normalization/lookup if it assumed substring matching).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 34b7708d-20c2-4d9a-851a-925857cdea9f
📒 Files selected for processing (1)
lib/user-data.ts
- Narrow SENSITIVE_KEYS: remove overly broad 'key' pattern, add specific patterns (apikey, api_key, secretkey, private_key, access_key) - Make redactParams recursive to handle nested objects and arrays - Change DB helper functions (getUserById, getUserByEmail, getAccountByUserId) to return null on error instead of rethrowing, so auth JWT callback treats DB failures as 'no user' Signed-off-by: Narutama Aurum <narutamaaurum@gmail.com>
|
Addressed CodeRabbit review comments:
All lint and tests pass. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/user-data.ts (1)
53-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact or suppress error text fields before emitting logs.
Line 53 and Line 54 currently log raw
error.message/error.stack. Those fields can still contain emails or other identifiers, which bypasses key-based param redaction.Proposed minimal hardening
+const redactText = (value: string): string => + value + // email + .replace(/[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}/gi, "[REDACTED_EMAIL]") + // broad token-like blobs (JWT/API keys/secrets) + .replace(/\b[A-Za-z0-9_-]{20,}\b/g, "[REDACTED_TOKEN]"); + const logError = ( functionName: string, params: Record<string, unknown>, error: unknown ) => { console.error( JSON.stringify({ timestamp: new Date().toISOString(), level: "error", context: functionName, params: redactParams(params), - error: error instanceof Error ? error.message : "Unknown error", - stack: error instanceof Error ? error.stack : undefined, + error: error instanceof Error ? redactText(error.message) : "Unknown error", + stack: error instanceof Error && error.stack ? redactText(error.stack) : undefined, }) ); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/user-data.ts` around lines 53 - 54, The current serialization in lib/user-data.ts emits raw error.message and error.stack which can leak PII; replace those with sanitized values before logging by mapping error.message and error.stack to either a fixed placeholder (e.g. "[REDACTED_ERROR]") or a redacted string produced by a simple sanitizer that strips emails/identifiers (apply to the error variable used in the object where error: error instanceof Error ? error.message : "Unknown error" and stack: error instanceof Error ? error.stack : undefined). Update the code paths that produce these fields so logs only contain the sanitized value (or undefined) and ensure the sanitizer is applied consistently wherever error.message or error.stack are emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/user-data.ts`:
- Around line 53-54: The current serialization in lib/user-data.ts emits raw
error.message and error.stack which can leak PII; replace those with sanitized
values before logging by mapping error.message and error.stack to either a fixed
placeholder (e.g. "[REDACTED_ERROR]") or a redacted string produced by a simple
sanitizer that strips emails/identifiers (apply to the error variable used in
the object where error: error instanceof Error ? error.message : "Unknown error"
and stack: error instanceof Error ? error.stack : undefined). Update the code
paths that produce these fields so logs only contain the sanitized value (or
undefined) and ensure the sanitizer is applied consistently wherever
error.message or error.stack are emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0de5311d-d8b9-47a5-8d50-44cd5e63f89a
📒 Files selected for processing (1)
lib/user-data.ts
1 participant
Closes #274
This PR addresses CodeRabbit review comments on #488 by adding PII redaction to error logging in lib/user-data.ts.
Changes:
This ensures emails and other sensitive data are not logged in error messages.
Summary by CodeRabbit