Potential fix for code scanning alert no. 1: Clear-text logging of sensitive information#9
Merged
Merged
Conversation
…nsitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/client.ts (3)
166-184: ⚖️ Poor tradeoffConsider circular reference protection.
The recursive implementation could stack overflow if the request payload contains circular references. While typical API request data is JSON-serializable and won't have circular references, a developer could accidentally pass a non-serializable object with circular structures to a request method.
🛡️ Suggested protection against circular references
-private redactSensitiveData(value: unknown): unknown { +private redactSensitiveData(value: unknown, seen = new WeakSet<object>()): unknown { if (Array.isArray(value)) { + if (seen.has(value)) return '[CIRCULAR]'; + seen.add(value); - return value.map((item) => this.redactSensitiveData(item)); + return value.map((item) => this.redactSensitiveData(item, seen)); } if (value && typeof value === 'object') { + if (seen.has(value)) return '[CIRCULAR]'; + seen.add(value); const redacted: Record<string, unknown> = {}; for (const [key, nestedValue] of Object.entries( value as Record<string, unknown>, )) { redacted[key] = this.isSensitiveKey(key) ? '[REDACTED]' - : this.redactSensitiveData(nestedValue); + : this.redactSensitiveData(nestedValue, seen); } return redacted; } return value; }Alternatively, document the assumption that request data must be JSON-serializable:
+/** + * Recursively redacts sensitive fields from request data. + * `@param` value - Request payload (must be JSON-serializable, no circular refs) + * `@returns` Redacted copy with sensitive values replaced by '[REDACTED]' + */ private redactSensitiveData(value: unknown): unknown {🤖 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 `@src/client.ts` around lines 166 - 184, redactSensitiveData currently recurses without guarding against circular references; update redactSensitiveData to accept (or create internally) a WeakSet named seen and add each object/array when first visited, skip or return a placeholder like '[CIRCULAR]' if an object is already in seen, and ensure arrays and nested objects use the same seen set when recursing; reference the redactSensitiveData function and isSensitiveKey to locate where to add the WeakSet tracking and circular-return behavior.
188-188: 💤 Low valueSimplify undefined handling for consistency.
Converting
undefinedto an empty string changes the debug output when no request body is present. It would be clearer to either logundefinedas-is or not log the data parameter at all for GET/DELETE requests.♻️ Suggested simplification
Option 1: Pass through undefined (simpler)
-const safeData = data === undefined ? '' : this.redactSensitiveData(data); +const safeData = data !== undefined ? this.redactSensitiveData(data) : undefined; // eslint-disable-next-line no-console console.log(`[CheFu SDK] ${method.toUpperCase()} ${url}`, safeData);Option 2: Omit data parameter when undefined (cleaner output)
-const safeData = data === undefined ? '' : this.redactSensitiveData(data); -// eslint-disable-next-line no-console -console.log(`[CheFu SDK] ${method.toUpperCase()} ${url}`, safeData); +// eslint-disable-next-line no-console +if (data !== undefined) { + const safeData = this.redactSensitiveData(data); + console.log(`[CheFu SDK] ${method.toUpperCase()} ${url}`, safeData); +} else { + console.log(`[CheFu SDK] ${method.toUpperCase()} ${url}`); +}🤖 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 `@src/client.ts` at line 188, The current assignment for safeData converts undefined to an empty string which alters debug output; change the logic in src/client.ts so safeData preserves undefined instead of turning it into '' by only calling redactSensitiveData when data is defined (e.g., set safeData to undefined when data is undefined, otherwise to this.redactSensitiveData(data)); update any subsequent logging or usage that expects the previous '' behavior to handle undefined accordingly.
150-164: 🏗️ Heavy liftSensitive-key detection already covers current SDK payload fields; optional hardening for future naming variants
isSensitiveKey()lowercases the key name, so SDK field names likepassword,apiKey,authToken,idToken, andrefreshTokenmap to entries in the list (and/auth/loginrequest payload usespassword).- Codebase search shows no usage of snake_case / hyphenated variants like
access_token,api_key, orclient_secretin request payloads, so the initially flagged gap doesn’t appear for current code paths.- Consider expanding matching (e.g.,
_/-normalization and/or token/secret substring/pattern coverage) to avoid future false negatives if payload field naming changes.🤖 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 `@src/client.ts` around lines 150 - 164, The isSensitiveKey function currently lowercases the key but only matches exact names; update isSensitiveKey to normalize separators (replace '_' and '-' with '' or ' ') and then check both exact names and substring/patterns (e.g., contains 'token', 'secret', 'key', 'pwd', 'pass') to catch variants like access_token, api-key, client_secret or authToken; locate and modify the isSensitiveKey method to perform the normalization and expanded matching while keeping the existing explicit list for exact matches.
🤖 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.
Nitpick comments:
In `@src/client.ts`:
- Around line 166-184: redactSensitiveData currently recurses without guarding
against circular references; update redactSensitiveData to accept (or create
internally) a WeakSet named seen and add each object/array when first visited,
skip or return a placeholder like '[CIRCULAR]' if an object is already in seen,
and ensure arrays and nested objects use the same seen set when recursing;
reference the redactSensitiveData function and isSensitiveKey to locate where to
add the WeakSet tracking and circular-return behavior.
- Line 188: The current assignment for safeData converts undefined to an empty
string which alters debug output; change the logic in src/client.ts so safeData
preserves undefined instead of turning it into '' by only calling
redactSensitiveData when data is defined (e.g., set safeData to undefined when
data is undefined, otherwise to this.redactSensitiveData(data)); update any
subsequent logging or usage that expects the previous '' behavior to handle
undefined accordingly.
- Around line 150-164: The isSensitiveKey function currently lowercases the key
but only matches exact names; update isSensitiveKey to normalize separators
(replace '_' and '-' with '' or ' ') and then check both exact names and
substring/patterns (e.g., contains 'token', 'secret', 'key', 'pwd', 'pass') to
catch variants like access_token, api-key, client_secret or authToken; locate
and modify the isSensitiveKey method to perform the normalization and expanded
matching while keeping the existing explicit list for exact matches.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Potential fix for https://github.com/CheFu-code/CheFu-Academy-SDK/security/code-scanning/1
The correct fix is to stop logging raw request payloads and instead log a sanitized/redacted version. Keep functionality (debug observability) while preventing sensitive disclosure by masking known secret keys recursively (for nested objects), and avoiding unsafe serialization of unknown values.
Best single fix in
src/client.ts:password,token,authToken,apiKey,authorization,secret, etc.) with"[REDACTED]".logRequestto log the sanitized payload instead of rawdata.This preserves existing SDK behavior (still logs requests in debug mode) while removing clear-text sensitive leakage.
Suggested fixes powered by Copilot Autofix. Review carefully before merging.
Summary by CodeRabbit
Release Notes