fix:34329-fixed upload svg logo in the workspace api #34977
Conversation
WalkthroughThe recent changes enhance security for SVG uploads in the Changes
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 (
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 9cf62ca and 2681b0245f35b9e9fe5044d9932f4218161a6479.
Files ignored due to path filters (1)
app/server/appsmith-server/src/test/resources/test_assets/invalid.svgis excluded by!**/*.svg
Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AssetServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.java (1 hunks)
Additional comments not posted (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AssetServiceCEImpl.java (2)
46-46: Added SVG content type to allowed lists.The addition of
image/svg+xmlto bothALLOWED_CONTENT_TYPESandALLOWED_CONTENT_TYPES_STRis consistent and necessary for the new feature to function correctly.Also applies to: 51-51
69-77: Review SVG content validation logic.The logic to check if the content type is SVG and validate it using
isSvgSafeappears correct. However, it's crucial to ensure thatIOUtils.toStringdoes not cause any side effects or exceptions when converting theDataBufferto a string, as this could affect the subsequent operations if the buffer's position is not reset correctly.Verification successful
Verification successful: No side effects found with IOUtils.toString usage.
- The
dataBuffer.readPosition(0)method is used to reset the buffer's position, ensuring subsequent operations are not affected.- The
isSvgSafemethod thoroughly validates SVG content against potential malicious code.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that IOUtils.toString does not affect subsequent buffer operations. # Test: Search for issues related to IOUtils.toString usage with DataBuffer. rg --type java 'IOUtils.toString' -A 5Length of output: 1025
Script:
#!/bin/bash # Description: Verify the implementation of isSvgSafe method. # Test: Search for the definition of isSvgSafe method. ast-grep --lang java --pattern 'boolean isSvgSafe($_) { $$$ }' # Description: Verify other usages of IOUtils.toString in the codebase. # Test: Search for IOUtils.toString usage in the codebase. rg --type java 'IOUtils.toString'Length of output: 301
Script:
#!/bin/bash # Description: Locate the definition of isSvgSafe method. # Test: Search for the isSvgSafe method in the codebase. rg --type java 'isSvgSafe'Length of output: 317
Script:
#!/bin/bash # Description: Extract the implementation of the isSvgSafe method. # Test: Search for the isSvgSafe method implementation in AssetServiceCEImpl.java. rg --type java 'private boolean isSvgSafe' -A 20Length of output: 3015
Script:
#!/bin/bash # Description: Verify the usage of dataBuffer.readPosition(0) to ensure it resets the buffer's position correctly. # Test: Search for dataBuffer.readPosition usage in the codebase. rg --type java 'dataBuffer.readPosition'Length of output: 455
app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.java (2)
1557-1582: Review of the test method for uploading an invalid SVG logo.The test
uploadWorkspaceLogo_invalidSvgappropriately handles the scenario where an invalid SVG file is uploaded. It uses mocking to simulate the upload and checks for the correct error handling. The use ofexpectErrorMatchesto assert the specific error message is a good practice. Ensure that the error handling for invalid SVGs is consistent across other parts of the application.Verification successful
Ensure consistent error handling for invalid SVG uploads across the application.
The error handling for invalid SVG uploads appears to be consistent across various parts of the application. The specific error message "Please upload a valid svg." is used in multiple places, ensuring uniform error feedback for users.
- Files and locations to review for consistency:
app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AssetServiceCEImpl.javaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent error handling for invalid SVG uploads across the application. # Test: Search for error handling consistency. rg --type java 'AppsmithError.VALIDATION_FAILURE' -A 5Length of output: 13313
1525-1554: Review of the test method for uploading a valid SVG logo.The test
uploadWorkspaceLogo_validSvgcorrectly mocks a file part with SVG content and tests the upload functionality. The assertions are appropriate, checking that the logo URL is set and the asset is not null. Ensure that the test covers all relevant aspects of the SVG upload process, including edge cases not covered by this basic validity check.Verification successful
Edge Cases for SVG Uploads Verified
The
WorkspaceServiceTest.javafile includes tests for various edge cases related to SVG uploads, including:
- Valid SVG Upload:
uploadWorkspaceLogo_validSvg- Invalid SVG Upload:
uploadWorkspaceLogo_invalidSvg- Large File Upload:
uploadWorkspaceLogo_largeFilePartThese tests ensure that the SVG upload functionality is thoroughly validated for different scenarios.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all edge cases for SVG upload are covered in tests. # Test: Search for additional SVG-related tests. rg --type java 'upload.*svg' -A 5Length of output: 1667
Script:
#!/bin/bash # Description: Verify that edge cases for SVG upload are covered in tests. # Test: Search for tests related to SVG upload with various edge cases. rg --type java 'upload.*(svg|logo)' -A 10Length of output: 4592
Script:
#!/bin/bash # Description: Verify that edge cases for SVG upload related to file size limits and malformed SVG content are covered in tests. # Test: Search for tests related to SVG upload with file size limits and malformed SVG content. rg --type java 'upload.*(svg|logo)' -A 20Length of output: 8911
Script:
#!/bin/bash # Description: List the contents of the WorkspaceServiceTest.java file to manually verify the presence of tests for SVG upload edge cases. # Test: List the contents of the WorkspaceServiceTest.java file. cat app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.javaLength of output: 89437
...server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AssetServiceCEImpl.java
Outdated
Show resolved
Hide resolved
|
Hi @somangshu , @riteshkew: Could you please add the reviewer for this PR. |
app/server/appsmith-server/src/test/resources/test_assets/invalid.svg
Outdated
Show resolved
Hide resolved
raushan3737
force-pushed
the
fix/34329_workspace_settings_upload_svg_logo_fix
branch
from
2681b02 to
7c9d8d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 2681b0245f35b9e9fe5044d9932f4218161a6479 and 7c9d8d7649c68e0975644e01bca15d7597ee7149.
Files ignored due to path filters (1)
app/server/appsmith-server/src/test/resources/test_assets/invalid.svgis excluded by!**/*.svg
Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AssetServiceCEImpl.java (4 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.java
Additional comments not posted (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AssetServiceCEImpl.java (3)
72-81: LGTM! But consider improving readability and error handling.The changes to handle SVG files and validate them using
isSvgSafeare correct.However, consider the following improvements:
- Use a more descriptive variable name instead of
isSvgType.- Improve error handling by logging the invalid SVG content for debugging purposes.
- Boolean isSvgType = MediaType.valueOf("image/svg+xml").equals(contentType); + Boolean isSvg = MediaType.valueOf("image/svg+xml").equals(contentType); - if (isSvgType) { + if (isSvg) { String svgContent = IOUtils.toString(dataBuffer.asInputStream(), StandardCharsets.UTF_8); dataBuffer.readPosition(0); if (!isSvgSafe(svgContent)) { log.error("Invalid SVG content: {}", svgContent); throw new AppsmithException(AppsmithError.VALIDATION_FAILURE, "Please upload a valid svg."); } }
88-110: LGTM! But add comments to regex patterns for maintainability.The
isSvgSafemethod effectively uses regex patterns to identify malicious content in SVG files.However, consider adding comments to each regex pattern to explain what it specifically targets for better maintainability.
String[] patterns = { // Checks for <script> tags which can contain malicious JavaScript "<script\\b[^<]*(?:(?!<\\/script>)<[^<]*)*<\\/script>", // Checks for <style> tags which can be used for CSS-based attacks "<style\\b[^<]*(?:(?!<\\/style>)<[^<]*)*<\\/style>", // Checks for <a> tags which might be used for phishing attacks "<a\\b[^>]*>", // Checks for closing </a> tags "</a>", // Checks for inline event handlers or styles "\\s(on[a-zA-Z]+|style)\\s*=\\s*(['\"]).*?\\2", // Checks for href or src attributes using JavaScript: URIs "\\s(href|src)\\s*=\\s*(['\"])javascript:[^'\"]*\\2" };
Line range hint
111-137:
LGTM! But consider improving readability and error handling.The changes to include SVG in the allowed content types and handle SVG validation are correct.
However, consider the following improvements:
- Use a more descriptive variable name instead of
firstPart.- Improve error handling by logging the content type for debugging purposes.
- final Part firstPart = fileParts.get(0); + final Part filePart = fileParts.get(0); - final MediaType contentType = firstPart.headers().getContentType(); + final MediaType contentType = filePart.headers().getContentType(); if (contentType == null || !ALLOWED_CONTENT_TYPES.contains(contentType)) { log.error("Invalid content type: {}", contentType); return Mono.error(new AppsmithException( AppsmithError.VALIDATION_FAILURE, "Please upload a valid image. Only JPEG, PNG, SVG and ICO are allowed.")); }
raushan3737
force-pushed
the
fix/34329_workspace_settings_upload_svg_logo_fix
branch
from
7c9d8d7 to
eab5e5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 7c9d8d7649c68e0975644e01bca15d7597ee7149 and eab5e5e.
Files ignored due to path filters (2)
app/server/appsmith-server/src/test/resources/test_assets/invalid.svgis excluded by!**/*.svgapp/server/appsmith-server/src/test/resources/test_assets/valid.svgis excluded by!**/*.svg
Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AssetServiceCEImpl.java (4 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/WorkspaceServiceTest.java
Additional comments not posted (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AssetServiceCEImpl.java (6)
49-51: LGTM! Added support for SVG content type.The addition of
image/svg+xmlto theALLOWED_CONTENT_TYPESset is correct and necessary for supporting SVG uploads.
54-54: LGTM! Added support for SVG content type string.The addition of
image/svg+xmlto theALLOWED_CONTENT_TYPES_STRset is correct and necessary for supporting SVG uploads.
88-109: Consider adding comments to regex patterns inisSvgSafe.The
isSvgSafemethod uses regex patterns to validate SVG content. This approach is effective, but consider adding comments to each pattern for maintainability.+ // Checks for <script> tags which can contain malicious JavaScript - "<script\\b[^<]*(?:(?!<\\/script>)<[^<]*)*<\\/script>", + // Checks for <style> tags which can be used for CSS-based attacks - "<style\\b[^<]*(?:(?!<\\/style>)<[^<]*)*<\\/style>", + // Checks for <a> tags which might be used for phishing attacks - "<a\\b[^>]*>", + // Checks for closing </a> tags - "</a>", + // Checks for inline event handlers or styles - "\\s(on[a-zA-Z]+|style)\\s*=\\s*(['\"]).*?\\2", + // Checks for href or src attributes using JavaScript: URIs - "\\s(href|src)\\s*=\\s*(['\"])javascript:[^'\"]*\\2"
Line range hint
111-129:
LGTM! Updated upload method to handle SVG files.The changes in the
uploadmethod ensure that SVG files are validated and correctly handled during the upload process.
Line range hint
169-175:
LGTM! Updated createAsset method to validate image types.The changes in the
createAssetmethod ensure that only valid image types, including SVGs, are accepted.
72-81: LGTM! Enhanced SVG validation logic.The changes in
checkImageTypeValidationcorrectly handle SVGs and validate their content for safety. This is crucial for preventing security vulnerabilities.However, ensure the
isSvgSafemethod covers all common SVG vulnerabilities.Verification successful
LGTM! Enhanced SVG validation logic.
The changes in
checkImageTypeValidationcorrectly handle SVGs and validate their content for safety. This is crucial for preventing security vulnerabilities. TheisSvgSafemethod is used only in the expected context and employs regex patterns to detect potentially harmful content.However, ensure the
isSvgSafemethod covers all common SVG vulnerabilities.
- Verify the comprehensiveness of the regex patterns in
isSvgSafe.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the SVG validation logic. # Test: Search for the `isSvgSafe` method usage. Expect: The method should be used only in `checkImageTypeValidation`. rg --type java -A 5 $'isSvgSafe'Length of output: 1941
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hi @abhvsn, @nidhi-nair, Could you please review the PR. |
|
Hi @raushan3737 ! Apologies for not being able to pick up this PR for review. We're currently backed up on tasks in our backlog but we've put this PR into the backlog and will be getting to this soon. Appreciate your patience with us. 🙏🏽 |
nidhi-nair
added
Community Contributor
|
Hi @nidhi-nair, Can you please review this PR if possible. It has been opened since last 2 month. Thank you. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
Fix-34329
Fixed the workspace setting upload logo api to accept the svg type.
Implemented a method
isSvgSafeto validate svg. If svg is safe it will be uploaded else, we will show Please provide valid svg message to the user.Added the unit test case for the api to accept the svg.
Snapshot:
Before:
After:
In case of invalid svg(containing script tag):
Summary by CodeRabbit
New Features
Tests