Fix path traversal vulnerability in product export endpoint#39
Open
devin-ai-integration[bot] wants to merge 4 commits intomainfrom
Open
Fix path traversal vulnerability in product export endpoint#39devin-ai-integration[bot] wants to merge 4 commits intomainfrom
devin-ai-integration[bot] wants to merge 4 commits intomainfrom
Conversation
- Add input validation to reject filenames with path traversal sequences - Implement whitelist validation for allowed characters - Add canonical path verification to ensure files stay within exports directory - Add file existence check before reading - Replace detailed error messages with generic ones to prevent information disclosure Addresses OWASP A01:2021 - Broken Access Control Co-Authored-By: mason.batchelor@cognition.ai <masonbatchelor81@gmail.com>
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
- Add app.exports.dir property to application.properties - Use constructor injection with @value to inject configurable export directory - Convert from File to Path APIs throughout exportProductData method - Use toRealPath(), resolve(), normalize() for secure path validation - Use Files.readString() with StandardCharsets.UTF_8 to avoid S1943 - Maintain all existing security validations (regex, traversal checks, etc.) Co-Authored-By: mason.batchelor@cognition.ai <masonbatchelor81@gmail.com>
- Add ProductControllerTest with 7 test cases covering path traversal protection - Test valid file access, double-dot rejection, backslash rejection, invalid characters - Test nonexistent file handling and Spring routing behavior - Achieves >80% coverage for new code to pass SonarQube quality gate Co-Authored-By: mason.batchelor@cognition.ai <masonbatchelor81@gmail.com>
- Add test for null filename validation (bypasses Spring routing) - Add test for forward slash in filename validation (bypasses Spring routing) - These direct controller invocations cover validation branches unreachable via MockMvc - Pushes new code coverage from 77.14% to >80% to pass SonarQube quality gate Co-Authored-By: mason.batchelor@cognition.ai <masonbatchelor81@gmail.com>
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
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.
Fix path traversal vulnerability in product export endpoint
Summary
Fixed a critical path traversal vulnerability in the
/api/products/export/{filename}endpoint that allowed attackers to read arbitrary files on the server. The endpoint previously accepted user-controlled filenames without validation, enabling directory traversal attacks like../../../etc/passwd.Changes made:
Multi-layer input validation for the filename parameter:
.., /, \)^[a-zA-Z0-9._-]+$)/tmp/exports/Fixed information disclosure vulnerability by replacing detailed error messages with generic ones
Fixed SonarQube S1075 code smell by externalizing the hardcoded
/tmp/exports/path toapplication.propertiesasapp.exports.dirModernized file handling by converting from deprecated
FileAPI toPathAPI withStandardCharsets.UTF_8Added comprehensive test coverage (9 test cases) to verify path traversal protection and achieve >80% code coverage for SonarQube quality gate
Review & Testing Checklist for Human
CRITICAL: Symlink security - The code uses
toRealPath(LinkOption.NOFOLLOW_LINKS)which does NOT resolve symlinks. This could potentially allow a symlink inside/tmp/exports/to point outside the directory. Consider whether symlinks should be resolved (removeLinkOption.NOFOLLOW_LINKS) or if symlinks should be explicitly blocked.Verify whitelist regex - The filename validation uses
^[a-zA-Z0-9._-]+$. Confirm this matches your requirements for valid export filenames. It blocks spaces, special characters, and Unicode.Test path traversal attempts - Manually test the endpoint with various attack payloads:
Verify export directory configuration - The
app.exports.dirproperty is set to/tmp/exports/inapplication.properties. Confirm this is appropriate for your production environment and that the directory exists with proper permissions.End-to-end testing - Test the export functionality through the frontend to ensure legitimate use cases still work correctly after the security hardening.
Notes
Devin Session: https://app.devin.ai/sessions/621fce980cef4368aaebd00fd00ccae9
Requested by: mason.batchelor@cognition.ai (@mbatchelor81)