-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(vulnerability): Log Injection (High) #11131
chore(vulnerability): Log Injection (High) #11131
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes focus on enhancing error logging across various components of the system. Key modifications include simplifying error messages to reduce verbosity and improve clarity. Specific identifiers are often omitted to streamline logs, although this may impact debugging. Additionally, the error handling in some methods now distinguishes between valid and malformed inputs, contributing to clearer diagnostics. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- metadata-io/src/main/java/com/linkedin/metadata/systemmetadata/ESSystemMetadataDAO.java (1 hunks)
- metadata-service/auth-servlet-impl/src/main/java/com/datahub/auth/authentication/AuthServiceController.java (3 hunks)
- metadata-service/schema-registry-servlet/src/main/java/io/datahubproject/openapi/schema/registry/SchemaRegistryController.java (1 hunks)
Additional comments not posted (4)
metadata-service/schema-registry-servlet/src/main/java/io/datahubproject/openapi/schema/registry/SchemaRegistryController.java (1)
310-314
: Good practice: Distinguishing between valid and malformed topic names.The updated logging logic enhances error reporting by distinguishing between valid and malformed topic names. This improves the clarity and security of the logs.
metadata-service/auth-servlet-impl/src/main/java/com/datahub/auth/authentication/AuthServiceController.java (3)
126-126
: Simplified error logging for JSON parsing.The error message for JSON parsing has been simplified to reduce verbosity and mitigate log injection risks. Ensure that this change does not hinder debugging.
Verification successful
Simplified error logging verified successfully.
The error message for JSON parsing is appropriately simplified, consistent across the codebase, and does not include JSON content, aligning with best practices for reducing verbosity and mitigating log injection risks. No issues found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify JSON parsing error logging across the codebase. # Test: Search for JSON parsing error logs. Expect: Simplified error messages without JSON content. rg --type java 'Failed to parse json'Length of output: 1151
389-389
: Simplified error logging for analytics event tracking.The error message for analytics event tracking has been simplified to reduce verbosity and mitigate log injection risks. Ensure that this change does not hinder debugging.
241-241
: Simplified error logging for invalid invite tokens.The error message for invalid invite tokens has been simplified to reduce verbosity and mitigate log injection risks. Ensure that this change does not hinder debugging.
Verification successful
Simplified error logging confirmed without hindering debugging.
The error logging for invalid invite tokens is appropriately simplified to reduce verbosity and mitigate log injection risks. The surrounding code handles invalid tokens by returning a
BAD_REQUEST
response, which should provide sufficient indication for debugging purposes.
- The simplified log message is located in
AuthServiceController.java
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify invite token error logging across the codebase. # Test: Search for invite token error logs. Expect: Simplified error messages without token content. rg --type java 'Invalid invite token'Length of output: 196
Script:
#!/bin/bash # Search for other logging or handling mechanisms related to invite tokens in the AuthServiceController.java file rg --type java 'invite token' -A 5 -B 5 metadata-service/auth-servlet-impl/src/main/java/com/datahub/auth/authentication/AuthServiceController.javaLength of output: 1057
metadata-io/src/main/java/com/linkedin/metadata/systemmetadata/ESSystemMetadataDAO.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Checklist
Background:
CodeQL scans detect high vulnerability in the code base. Its due to logging user inputs. We are logging the error instead of the json / user inputs. CodeQL detects it as vulnerability and by resolving this, the overall vulnerabilty score improves. And it had no impacts.
Summary by CodeRabbit