Potential fix for code scanning alert no. 24: Uncontrolled data used in path expression#12
Draft
Rajeshcn26 wants to merge 1 commit intomainfrom
Draft
Potential fix for code scanning alert no. 24: Uncontrolled data used in path expression#12Rajeshcn26 wants to merge 1 commit intomainfrom
Rajeshcn26 wants to merge 1 commit intomainfrom
Conversation
…in path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a path traversal security vulnerability (code scanning alert #24) in the file download endpoint by implementing proper path validation and normalization.
Changes:
- Added path traversal protection to the
DownloadFilemethod using Path.Combine, GetFullPath, and directory boundary validation - Replaced direct string concatenation for file paths with secure path handling that prevents directory traversal attacks
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Potential fix for https://github.com/xebia/ShipServiceApp/security/code-scanning/24
In general, to fix uncontrolled path usage, you must ensure that user input cannot cause the application to read or write outside an intended directory. This is typically done by combining the user input with a trusted base directory, resolving the combined path to its canonical form, and then checking that the resolved path is still inside the base directory. Alternatively, if the input should be just a simple filename, you can reject any input that contains path separators or
...For this endpoint, the intent is clearly to only serve files from
/var/www/files/. The minimal change that preserves existing behavior is to (1) define a base directory, (2) safely combine it withfilenameusingPath.Combine, (3) canonicalize the resulting path withPath.GetFullPath, and (4) verify that the canonical path starts with the canonical base directory plus a directory separator; if not, returnBadRequest. This prevents traversal using..or absolute paths while still allowing all legitimate filenames that live under the base directory.Concretely in
Controllers/VulnerableController.cs, insideDownloadFilewe will:baseDirectorystring set to"/var/www/files".System.IO.Path.Combine(baseDirectory, filename)and thenSystem.IO.Path.GetFullPath(...)to obtainfilePath.baseDirectoryFullPath = System.IO.Path.GetFullPath(baseDirectory)and checkfilePath.StartsWith(baseDirectoryFullPath + System.IO.Path.DirectorySeparatorChar). If this fails, returnBadRequest("Invalid file path.").We do not need any new
usingstatements because we already refer toSystem.IO.Filefully qualified, and we can similarly callSystem.IO.Pathfully qualified.Suggested fixes powered by Copilot Autofix. Review carefully before merging.