Potential fix for code scanning alert no. 3: Uncontrolled data used in path expression#9
Potential fix for code scanning alert no. 3: Uncontrolled data used in path expression#9
Conversation
…n 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 pull request addresses a critical path traversal vulnerability (Code Scanning Alert #3) in the file download API endpoint by implementing path validation to ensure user-provided filenames cannot escape the intended uploads directory.
Changes:
- Replaced direct path construction with secure path resolution
- Added validation to verify resolved paths remain within the uploads root directory
- Improved error handling for invalid filenames
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const resolvedPath = path.resolve(uploadsRoot, String(filename)); | ||
|
|
||
| // Ensure the resolved path is within the uploads root to prevent path traversal | ||
| if (!resolvedPath.startsWith(uploadsRoot + path.sep) && resolvedPath !== uploadsRoot) { |
There was a problem hiding this comment.
The path validation allows resolvedPath to equal uploadsRoot (which happens when filename resolves to the uploads directory itself, e.g., when filename is "." or ""). While this doesn't create a security vulnerability, it results in poor error handling: fs.readFileSync will fail when trying to read a directory, causing a 500 error instead of a proper 400 error with "Invalid filename".
The condition should be simplified to only check if the resolved path is a child of the uploads root, not the root itself. Consider changing to: if (!resolvedPath.startsWith(uploadsRoot + path.sep))
| if (!resolvedPath.startsWith(uploadsRoot + path.sep) && resolvedPath !== uploadsRoot) { | |
| if (!resolvedPath.startsWith(uploadsRoot + path.sep)) { |
|
|
||
| // Securely construct a path under the uploads directory | ||
| const uploadsRoot = path.join(process.cwd(), 'uploads'); | ||
| const resolvedPath = path.resolve(uploadsRoot, String(filename)); |
There was a problem hiding this comment.
Converting filename to a string using String(filename) is good for safety, but this should happen before the path resolution. If filename is null or undefined (which shouldn't happen due to the check on line 11, but could theoretically occur), String(null) becomes "null" and String(undefined) becomes "undefined", which would then be used as literal filename strings.
While the current code has a guard on line 11 that checks if (!filename), it's better to be defensive. Consider validating that filename is a non-empty string before proceeding with path operations.
| if (!resolvedPath.startsWith(uploadsRoot + path.sep) && resolvedPath !== uploadsRoot) { | ||
| return res.status(400).json({ error: 'Invalid filename' }); | ||
| } |
There was a problem hiding this comment.
The current validation doesn't protect against symlink attacks. If a symbolic link exists within the uploads directory that points to a file outside of it, the validation will pass but the file read will access the symlink target outside the allowed directory.
To fully prevent path traversal via symlinks, you should resolve the real path before validation. The PR description mentions this: use fs.realpathSync(resolvedPath) to resolve symlinks, then validate that the real path is still within uploadsRoot. Note that fs.realpathSync will throw an error if the file doesn't exist, so you'll need to handle that case appropriately (either with try-catch or by checking existence first).
See below for a potential fix:
try {
// Resolve symlinks and get the real filesystem path
const realPath = fs.realpathSync(resolvedPath);
// Ensure the real path is within the uploads root to prevent path traversal and symlink attacks
if (!realPath.startsWith(uploadsRoot + path.sep) && realPath !== uploadsRoot) {
return res.status(400).json({ error: 'Invalid filename' });
}
const fileContent = fs.readFileSync(realPath, 'utf8');
Potential fix for https://github.com/github-samples/gitfolio/security/code-scanning/3
In general, the correct fix is to ensure that any path derived from user input is safely constrained to a known root directory. You should normalize the path (using
path.resolve) to eliminate..segments, optionally resolve symlinks (withfs.realpathSyncorfs.realpath), and then verify that the resulting absolute path is still inside the intended root directory. If it is not, you must reject the request.For this specific endpoint, the safest fix that preserves existing functionality is:
const UPLOADS_ROOT = path.join(process.cwd(), 'uploads');.filenameusingpath.resolve(UPLOADS_ROOT, filename). This normalizes the path and collapses..segments.fs.realpathSyncon the resolved path to handle symlinks.filePathstarts withUPLOADS_ROOT + path.sep(or equalsUPLOADS_ROOTif you allow the root itself). If it does not, respond with HTTP 400 or 403 instead of reading the file.filePathinfs.readFileSync.We can make these changes entirely inside
pages/api/download.js, just above and around the currentfilePathconstruction andfs.readFileSynccall. No new imports are required, sincefsandpathare already imported.Suggested fixes powered by Copilot Autofix. Review carefully before merging.