Potential fix for code scanning alert no. 1: Uncontrolled data used in path expression #2
Conversation
…n path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if hasattr(self, 'logger'): | ||
| self.logger.warning(f"Rejected remote file with invalid filename: {filename}") | ||
| return | ||
| file_path = (dir_path / filename).resolve() |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix the problem, we should ensure that the constructed file path is safely contained within the intended uploads directory, regardless of the user-supplied filename. This involves normalizing the path after joining the directory and filename, and then checking that the normalized path is a subpath of the intended directory. The current code does this with .resolve() and a string prefix check, but the initial filename validation could be improved by using a stricter sanitization function, such as werkzeug.utils.secure_filename, or by explicitly rejecting any suspicious characters. Since we can only use well-known libraries and not assume the presence of werkzeug, we should further sanitize the filename by allowing only safe characters (e.g., alphanumerics, underscores, hyphens, and dots), and then perform the normalization and containment check as before. The changes should be made in the region around line 5567–5577 in netbbsd.py.
Required changes:
- Add a stricter filename sanitization step before constructing the path.
- Optionally, define a helper function to sanitize filenames.
- Ensure the path normalization and containment check are robust.
| @@ -5566,4 +5566,14 @@ | ||
| return | ||
| # Validate filename: must not contain path separators or be absolute | ||
| if os.path.sep in filename or (os.path.altsep and os.path.altsep in filename) or Path(filename).is_absolute(): | ||
| # Validate filename: must be a safe basename (no path separators, no absolute, only safe chars) | ||
| import re | ||
| def sanitize_filename(name: str) -> str: | ||
| # Only allow alphanumerics, underscores, hyphens, and dots | ||
| return re.sub(r'[^A-Za-z0-9._-]', '_', name) | ||
| safe_filename = sanitize_filename(filename) | ||
| if ( | ||
| os.path.sep in safe_filename or | ||
| (os.path.altsep and os.path.altsep in safe_filename) or | ||
| Path(safe_filename).is_absolute() or | ||
| safe_filename in ('', '.', '..') | ||
| ): | ||
| if hasattr(self, 'logger'): | ||
| @@ -5571,3 +5581,3 @@ | ||
| return | ||
| file_path = (dir_path / filename).resolve() | ||
| file_path = (dir_path / safe_filename).resolve() | ||
| # Ensure file_path is within dir_path |
Potential fix for https://github.com/Thiesi/NetBBSD/security/code-scanning/1
To fix this vulnerability, we need to ensure that the constructed file path is contained within the intended uploads directory and does not allow directory traversal or writing outside the designated area. The best way to do this is to normalize the path using
os.path.normpathorPath.resolve(), and then check that the resulting path is a child of the intended uploads directory. If the check fails, the file should not be written and an error should be logged. Additionally, we should reject absolute paths and sanitize the filename to remove any path separators.The changes should be made in the region where
file_pathis constructed and used (lines 5567–5571). We should:No new methods are strictly required, but we may want to add a helper function for filename validation if desired.
Suggested fixes powered by Copilot Autofix. Review carefully before merging.