🔒 Secure Logger Implementation to Prevent Sensitive Data Exposure#18
Conversation
- Added `app/lib/logger.ts` with `logError` and `sanitizeValue` functions.
- `sanitizeValue` redacts API keys, JWTs, and sensitive object keys (e.g., password, secret, token).
- Replaced `console.error` with `logError` in:
- `app/api/chat/route.ts`
- `app/api/quote/route.ts`
- `app/services/ragService.ts`
- Added unit tests in `app/lib/logger.test.ts`.
This fixes the vulnerability where sensitive information could be logged to the console on error.
Co-authored-by: codewithaman07 <135147451+codewithaman07@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a secure logging utility to prevent sensitive data exposure in server logs. The implementation adds a logError function that sanitizes error objects by redacting API keys, tokens, passwords, and other sensitive information before logging. The changes replace vulnerable console.error calls in server-side API routes and services with the new secure logger, and include unit tests to verify the sanitization logic.
Changes:
- Added
app/lib/logger.tswithsanitizeValueandlogErrorfunctions to sanitize and safely log errors - Added
app/lib/logger.test.tswith unit tests covering API key redaction, JWT redaction, sensitive key handling, circular references, and Error object sanitization - Replaced
console.errorcalls withlogErrorin server-side files:app/api/chat/route.ts,app/api/quote/route.ts, andapp/services/ragService.ts
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| app/lib/logger.ts | New secure logger utility with sanitization logic for sensitive data (API keys, tokens, passwords, etc.) and circular reference handling |
| app/lib/logger.test.ts | Unit tests for the sanitization logic covering various scenarios including API keys, JWTs, sensitive object keys, nested objects, circular references, and Error objects |
| app/services/ragService.ts | Replaced console.error with logError for RAG service error handling |
| app/api/chat/route.ts | Replaced console.error with logError for chat API error handling |
| app/api/quote/route.ts | Replaced console.error with logError in four locations (main error handler, daily quote fetch, batch quotes fetch, and random quote fetch) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Basic heuristic to redact likely API keys | ||
| // Google API keys start with AIza | ||
| if (value.startsWith('AIza')) { | ||
| return '[REDACTED_API_KEY]'; | ||
| } | ||
| // Redact JWTs (start with eyJ and are long) | ||
| if (value.startsWith('eyJ') && value.length > 50 && !value.includes(' ')) { | ||
| return '[REDACTED_JWT]'; | ||
| } |
There was a problem hiding this comment.
The sanitization logic for strings only checks for Google API keys (starting with 'AIza') and JWTs. However, there are many other API key formats that could be logged. Consider expanding the pattern matching to include other common API key formats such as: AWS keys (AKIA...), generic alphanumeric keys of certain lengths (e.g., 32+ characters without spaces), keys with specific prefixes (sk-, pk-, etc.), or keys matching patterns like hex strings. Alternatively, consider using a more comprehensive regex pattern to catch various API key formats.
| // Basic heuristic to redact likely API keys | |
| // Google API keys start with AIza | |
| if (value.startsWith('AIza')) { | |
| return '[REDACTED_API_KEY]'; | |
| } | |
| // Redact JWTs (start with eyJ and are long) | |
| if (value.startsWith('eyJ') && value.length > 50 && !value.includes(' ')) { | |
| return '[REDACTED_JWT]'; | |
| } | |
| // Basic heuristic to redact likely API keys and tokens | |
| // Google API keys start with AIza | |
| if (value.startsWith('AIza')) { | |
| return '[REDACTED_API_KEY]'; | |
| } | |
| // Redact JWTs (start with eyJ and are long) | |
| if (value.startsWith('eyJ') && value.length > 50 && !value.includes(' ')) { | |
| return '[REDACTED_JWT]'; | |
| } | |
| // Additional common API key / secret patterns | |
| const apiKeyPatterns: { regex: RegExp; replacement: string }[] = [ | |
| // AWS access keys: AKIA / ASIA followed by 16 uppercase alphanumerics | |
| { regex: /^(AKIA|ASIA)[0-9A-Z]{16}$/, replacement: '[REDACTED_API_KEY]' }, | |
| // Stripe-style and similar keys: sk_*, sk-*, pk_*, pk-* with long random suffixes | |
| { regex: /\b(sk|pk)[_-][0-9a-zA-Z]{16,}\b/, replacement: '[REDACTED_API_KEY]' }, | |
| // Generic long hex strings (32+ hex chars) | |
| { regex: /^[0-9a-fA-F]{32,}$/, replacement: '[REDACTED_SECRET]' }, | |
| // Generic long opaque tokens: URL-safe / base62-ish, 32+ chars, no spaces | |
| { regex: /^[A-Za-z0-9_\-]{32,}$/, replacement: '[REDACTED_SECRET]' }, | |
| ]; | |
| for (const { regex, replacement } of apiKeyPatterns) { | |
| if (regex.test(value)) { | |
| return replacement; | |
| } | |
| } |
| const sanitized = sanitizeValue(error) as any; | ||
| assert.strictEqual(sanitized.message, 'Something went wrong'); | ||
| assert.strictEqual(sanitized.apiKey, '[REDACTED]'); | ||
| }); |
There was a problem hiding this comment.
The test cases are missing coverage for several important scenarios: (1) Testing that error messages and stack traces containing sensitive data are properly sanitized, (2) Testing arrays containing sensitive values, (3) Testing that normal strings that happen to contain keywords like "key" or "token" in other contexts (e.g., "the key to success", "token of appreciation") are not incorrectly redacted, and (4) Testing deeply nested objects with sensitive data at multiple levels. Consider adding these test cases to ensure comprehensive sanitization coverage.
| }); | |
| }); | |
| test('sanitizeValue should sanitize sensitive data in error message and stack', () => { | |
| const apiKey = 'AIzaSyD...'; | |
| const error = new Error(`Failed with key ${apiKey}`); | |
| // Simulate a stack that also contains the API key | |
| (error as any).stack = `Error: Failed with key ${apiKey}\n at someFunction (file.ts:10:5)`; | |
| const sanitized = sanitizeValue(error) as any; | |
| // The raw API key should not appear in message or stack | |
| assert.ok(!sanitized.message.includes(apiKey)); | |
| assert.ok(!sanitized.stack.includes(apiKey)); | |
| // The API key should be redacted consistently with string sanitization behavior | |
| assert.ok(sanitized.message.includes('[REDACTED_API_KEY]') || sanitized.message === '[REDACTED_API_KEY]'); | |
| assert.ok( | |
| typeof sanitized.stack === 'string' | |
| ? sanitized.stack.includes('[REDACTED_API_KEY]') || !sanitized.stack.includes(apiKey) | |
| : true | |
| ); | |
| }); | |
| test('sanitizeValue should redact sensitive values inside arrays', () => { | |
| const data = [ | |
| 'public-value', | |
| 'AIzaSyD...', | |
| 'another-public', | |
| { password: 'super-secret' }, | |
| ]; | |
| const sanitized = sanitizeValue(data) as any; | |
| assert.strictEqual(sanitized[0], 'public-value'); | |
| assert.strictEqual(sanitized[1], '[REDACTED_API_KEY]'); | |
| assert.strictEqual(sanitized[2], 'another-public'); | |
| assert.strictEqual(sanitized[3].password, '[REDACTED]'); | |
| }); | |
| test('sanitizeValue should not redact non-sensitive uses of keywords', () => { | |
| const input = { | |
| message1: 'the key to success', | |
| message2: 'a token of appreciation', | |
| description: 'The config field is called apiKey but this sentence is not a secret.', | |
| }; | |
| const sanitized = sanitizeValue(input) as any; | |
| assert.strictEqual(sanitized.message1, input.message1); | |
| assert.strictEqual(sanitized.message2, input.message2); | |
| assert.strictEqual(sanitized.description, input.description); | |
| }); | |
| test('sanitizeValue should redact sensitive data in deeply nested objects', () => { | |
| const nested = { | |
| level1: { | |
| apiKey: 'top-level-key', | |
| level2: { | |
| token: 'very-secret-token', | |
| info: 'public-info', | |
| items: [ | |
| { password: 'password-one' }, | |
| { nested: { secretToken: 'token-two' } }, | |
| ], | |
| }, | |
| }, | |
| other: 'unchanged', | |
| }; | |
| const sanitized = sanitizeValue(nested) as any; | |
| // Top-level sensitive field | |
| assert.strictEqual(sanitized.level1.apiKey, '[REDACTED]'); | |
| // Nested sensitive token | |
| assert.strictEqual(sanitized.level1.level2.token, '[REDACTED]'); | |
| // Public info should remain the same | |
| assert.strictEqual(sanitized.level1.level2.info, 'public-info'); | |
| // Sensitive data inside arrays at deeper levels | |
| assert.strictEqual(sanitized.level1.level2.items[0].password, '[REDACTED]'); | |
| assert.strictEqual(sanitized.level1.level2.items[1].nested.secretToken, '[REDACTED]'); | |
| // Non-sensitive top-level field should remain unchanged | |
| assert.strictEqual(sanitized.other, 'unchanged'); | |
| }); |
| export function logError(context: string, error: unknown) { | ||
| try { | ||
| const sanitized = sanitizeValue(error); | ||
| console.error(`${context}:`, JSON.stringify(sanitized, null, 2)); |
There was a problem hiding this comment.
Using JSON.stringify for logging may not handle all cases gracefully. When sanitizeValue returns a value that JSON.stringify cannot serialize (such as certain native objects, functions, or symbols), this will throw an error. While there's a try-catch wrapper, consider documenting this limitation or using a more robust serialization approach that can handle edge cases like BigInt, undefined in arrays, or other non-JSON-serializable values.
| console.error(`${context}:`, JSON.stringify(sanitized, null, 2)); | |
| const serialized = JSON.stringify( | |
| sanitized, | |
| (_key, value) => (typeof value === 'bigint' ? value.toString() : value), | |
| 2, | |
| ); | |
| console.error(`${context}:`, serialized); |
| return ( | ||
| lowerKey.includes('key') || | ||
| lowerKey.includes('secret') || | ||
| lowerKey.includes('token') || | ||
| lowerKey.includes('password') || | ||
| lowerKey.includes('authorization') || | ||
| lowerKey.includes('cookie') || | ||
| lowerKey.includes('credentials') | ||
| ); |
There was a problem hiding this comment.
The isSensitiveKey function may have false positives. Keys like "publicKey", "tokenize", "keyframe", "keywords", "keynote", or "passkey" contain the sensitive keywords but may not actually be sensitive in all contexts. Consider making the pattern matching more specific (e.g., exact matches for common patterns like "apiKey", "api_key", "API_KEY") or using a whitelist/blacklist approach to avoid over-redaction of legitimate data that happens to contain these substrings.
| return ( | |
| lowerKey.includes('key') || | |
| lowerKey.includes('secret') || | |
| lowerKey.includes('token') || | |
| lowerKey.includes('password') || | |
| lowerKey.includes('authorization') || | |
| lowerKey.includes('cookie') || | |
| lowerKey.includes('credentials') | |
| ); | |
| // Common exact sensitive keys (case-insensitive) | |
| const sensitiveExactKeys = [ | |
| 'key', | |
| 'api_key', | |
| 'apikey', | |
| 'api-key', | |
| 'access_token', | |
| 'accesstoken', | |
| 'access-token', | |
| 'refresh_token', | |
| 'refreshtoken', | |
| 'refresh-token', | |
| 'id_token', | |
| 'idtoken', | |
| 'id-token', | |
| 'token', | |
| 'secret', | |
| 'password', | |
| 'pass', | |
| 'auth', | |
| 'authorization', | |
| 'cookie', | |
| 'cookies', | |
| 'session', | |
| 'sessionid', | |
| 'session_id', | |
| 'session-token', | |
| 'sessiontoken', | |
| 'credential', | |
| 'credentials', | |
| ]; | |
| if (sensitiveExactKeys.includes(lowerKey)) { | |
| return true; | |
| } | |
| // Match sensitive terms when they appear as separate words or joined by | |
| // common separators (underscore, dash, space). This avoids flagging | |
| // benign keys like "publicKey" or "tokenize" while still catching | |
| // patterns such as "my_api_key" or "user-access_token". | |
| const sensitivePattern = | |
| /(^|[_\-\s])(api|access|refresh|id)?(key|token|secret|password|authorization|cookie|session|credential)s?($|[_\-\s])/i; | |
| return sensitivePattern.test(lowerKey); |
| * Sanitizes the error object to remove sensitive information. | ||
| * @param error The error object. | ||
| * @returns A sanitized error object or string. |
There was a problem hiding this comment.
The JSDoc comment incorrectly states "@param error The error object" when the actual parameter name is "value" and accepts any type (unknown), not just error objects. Update the documentation to accurately reflect that this function sanitizes any value, not specifically error objects. For example: "@param value The value to sanitize (can be any type)".
| * Sanitizes the error object to remove sensitive information. | |
| * @param error The error object. | |
| * @returns A sanitized error object or string. | |
| * Sanitizes a value to remove sensitive information. | |
| * @param value The value to sanitize (can be any type). | |
| * @returns A sanitized value or string. |
| @@ -0,0 +1,63 @@ | |||
| import { test } from 'node:test'; | |||
| import assert from 'node:assert'; | |||
| import { sanitizeValue } from './logger.ts'; | |||
There was a problem hiding this comment.
The import path should be './logger' instead of './logger.ts'. TypeScript import statements should not include file extensions when importing other TypeScript files in the same project. This could cause issues with module resolution.
| import { sanitizeValue } from './logger.ts'; | |
| import { sanitizeValue } from './logger'; |
| message: err.message, | ||
| stack: err.stack, |
There was a problem hiding this comment.
The error message and stack trace should also be sanitized to prevent sensitive data leakage. Error messages and stack traces can contain sensitive information like file paths, environment variables, API keys embedded in URLs, or other data that was being processed when the error occurred. Consider applying sanitizeValue recursively to the message and stack properties of Error objects.
| message: err.message, | |
| stack: err.stack, | |
| message: sanitizeValue(err.message, seen), | |
| stack: sanitizeValue(err.stack, seen), |
This PR introduces a secure logging utility
logErrorinapp/lib/logger.tsto prevent sensitive data exposure in server logs. It sanitizes error objects by redacting API keys, tokens, and other sensitive information. The vulnerableconsole.errorcalls inapp/api/chat/route.ts,app/api/quote/route.ts, andapp/services/ragService.tshave been replaced withlogError. Unit tests have been added to verify the sanitization logic.PR created automatically by Jules for task 16823562910971665413 started by @codewithaman07