Uploads with jobs #2438
Uploads with jobs #2438
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| relativePath = meta.get("relativePath", filename) | ||
| destination = meta.get("destination", '') | ||
|
|
||
| if not temp_file.exists(): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To fix the vulnerability, you must ensure that any file system access involving a path derived from untrusted input (here, upload_id) is properly constrained to the intended directory (settings.TUS_UPLOAD_DIR).
General steps:
- Normalize the path after joining the potentially untrusted
upload_idwithsettings.TUS_UPLOAD_DIR. - Verify that the resulting path is indeed within the intended upload directory.
- If the normalized path escapes (e.g., with
..) or is an absolute path elsewhere, reject it.
Best specific fix:
- Patch the
upload_pathfunction inESSArch_Core/tus/utils.pyto normalize and check the (joined) upload path, and raise an exception if the check fails. - To avoid repeated code, centralize the check in
upload_pathitself, so all code that usesupload_pathormeta_pathgets the benefit. - Use Python's
os.pathorpathlibto do normalization and prefix checking. - You may need to import
osinESSArch_Core/tus/utils.pyif it isn't already imported.
Required changes:
- Edit
ESSArch_Core/tus/utils.py:- Update the
upload_pathfunction to:- Normalize the resulting path (after joining the upload dir and upload id).
- Check that the resulting path is within
settings.TUS_UPLOAD_DIR(by comparing result path with the root). - Raise an exception (e.g.,
ValueError) if the check fails.
- (If not already imported in this file) Import
osforos.path.commonpathor equivalent.
- Update the
| @@ -1,6 +1,7 @@ | ||
| import base64 | ||
| import shutil | ||
| from pathlib import Path | ||
| import os | ||
|
|
||
| from django.conf import settings | ||
|
|
||
| @@ -8,7 +9,13 @@ | ||
|
|
||
|
|
||
| def upload_path(upload_id): | ||
| return Path(settings.TUS_UPLOAD_DIR) / upload_id | ||
| # Ensure the returned path is inside the TUS_UPLOAD_DIR | ||
| root = Path(settings.TUS_UPLOAD_DIR).resolve() | ||
| full_path = (root / upload_id).resolve() | ||
| # Confirm containment (prevent path traversal) | ||
| if not str(full_path).startswith(str(root) + os.sep): | ||
| raise ValueError("Invalid upload_id: path traversal detected") | ||
| return full_path | ||
|
|
||
|
|
||
| def meta_path(upload_id): |
|
|
||
| dest_path.parent.mkdir(parents=True, exist_ok=True) | ||
| # raise ValueError(f'Moved file to {dest_path}') | ||
| shutil.move(str(temp_file), dest_path.as_posix()) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general: To protect against path traversal, the code must ensure that any path constructed from user input (such as upload_id) stays within the intended root directory. This is achieved by normalizing the resulting path and verifying that it is a child of the expected root.
Specifically:
- In
ESSArch_Core/tus/utils.py, theupload_path(upload_id)function should return a path only if the joined, normalized path resides withinsettings.TUS_UPLOAD_DIR. - Use
os.path.normpathor pathlib’sresolve()(withstrict=False) to normalize/join, and check that the result is within the intended upload root. - If the check fails, raise an exception or handle as appropriate.
- This change should occur in
upload_path()and, by extension, in all consumers of this function. This avoids changing functionality elsewhere but guarantees security centrally.
Requirements:
- Import of
osforos.path.commonpathor use of pure pathlib APIs. upload_pathmust normalize and verify the joined path.- To reduce false blocking, ensure that only
upload_ids that try to escape the directory are rejected.
Changes required:
- Edit the definition of
upload_pathinESSArch_Core/tus/utils.pyto validate that the computed path is insidesettings.TUS_UPLOAD_DIR. - Any necessary import in this file.
| @@ -1,14 +1,21 @@ | ||
| import base64 | ||
| import shutil | ||
| from pathlib import Path | ||
|
|
||
| import os | ||
| from django.conf import settings | ||
|
|
||
| from ESSArch_Core.ip.models import InformationPackage, Workarea | ||
|
|
||
|
|
||
| def upload_path(upload_id): | ||
| return Path(settings.TUS_UPLOAD_DIR) / upload_id | ||
| base_dir = Path(settings.TUS_UPLOAD_DIR).resolve() | ||
| # Always treat upload_id as a single filename component | ||
| # If you wish to allow path structure, use join and check as below. Here assumed as a filename. | ||
| path = (base_dir / upload_id).resolve() | ||
| # Ensure the resulting path is actually within base_dir | ||
| if not str(path).startswith(str(base_dir) + os.sep): | ||
| raise ValueError("Invalid upload_id: path traversal detected") | ||
| return path | ||
|
|
||
|
|
||
| def meta_path(upload_id): |
| shutil.move(str(temp_file), dest_path.as_posix()) | ||
|
|
||
| # CLEANUP — Remove metadata file | ||
| metadata_path.unlink(missing_ok=True) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
General fix strategy:
Always validate and restrict any user-supplied value used in the construction of a filesystem path to a safe subdirectory. In this context, the file path must always stay within the intended upload directory.
Concrete fix for this codebase:
Normalize the constructed path (Path(settings.TUS_UPLOAD_DIR) / upload_id), resolve symlinks if possible (e.g., with .resolve() or .absolute()), and verify that the resulting path is still inside TUS_UPLOAD_DIR. If the check fails, raise an exception or return an error to avoid file operations outside the intended directory.
Specific changes to make:
- In
upload_pathand thus transitively inmeta_path, after constructing the candidate path withPath(settings.TUS_UPLOAD_DIR) / upload_id(inupload_path), normalize and validate that the result is still withinTUS_UPLOAD_DIR. - If the check fails in
upload_path, raise an exception. - Only proceed with the operations if the check passes.
- You may need
import osforos.path.commonpathor use thepathlibmethods.
Files/methods to change:
ESSArch_Core/tus/utils.py: editupload_pathto validateupload_idby normalizing the resulting path and checking that it resides insideTUS_UPLOAD_DIR.
| @@ -1,14 +1,21 @@ | ||
| import base64 | ||
| import shutil | ||
| from pathlib import Path | ||
|
|
||
| import os | ||
| from django.conf import settings | ||
|
|
||
| from ESSArch_Core.ip.models import InformationPackage, Workarea | ||
|
|
||
|
|
||
| def upload_path(upload_id): | ||
| return Path(settings.TUS_UPLOAD_DIR) / upload_id | ||
| # Construct candidate path | ||
| base_path = Path(settings.TUS_UPLOAD_DIR).resolve() | ||
| candidate = (base_path / upload_id).resolve() | ||
| try: | ||
| candidate.relative_to(base_path) | ||
| except ValueError: | ||
| raise ValueError("Invalid upload_id: path traversal detected") | ||
| return candidate | ||
|
|
||
|
|
||
| def meta_path(upload_id): |
| cache.set(f'tus_upload_{upload_id}', 'DONE', timeout=3600) # 1 hour | ||
| else: | ||
| cache.set(f'tus_upload_{upload_id}', 'ERROR', timeout=3600) # 1 hour | ||
| return HttpResponse(f"Error finalizing upload: {res}", status=409) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To fix the problem, exception details should not be returned to the client in any HTTP response body. Instead, they should be logged server-side, and a generic error message should be sent to the client. The best way, in this case, is to modify the view in ESSArch_Core/tus/views.py so that if move_uploaded_file() returns anything but "Success", the response is always a generic (non-revealing) error message. Additionally, server-side logging should be added for staff to debug actual causes.
Here's what to do:
- In
ESSArch_Core/tus/views.py, change line 136 so that, instead of including{res}in the response body, it returns a generic message like "Error finalizing upload" (or possibly no message at all). - Log the actual error reason (
res) server-side using Python'sloggingmodule, so that developers/administrators can investigate issues from logs. - If imports for
loggingare not already present in the file, add them. - Do not otherwise alter the signature or behavior of
move_uploaded_file.
All edits are to be made within the regions shown from ESSArch_Core/tus/views.py.
| @@ -1,7 +1,7 @@ | ||
| import ast | ||
| import os | ||
| import uuid | ||
|
|
||
| import logging | ||
| from django.core.cache import cache | ||
| from django.http import HttpResponse | ||
| from rest_framework.decorators import api_view, permission_classes | ||
| @@ -133,7 +133,8 @@ | ||
| cache.set(f'tus_upload_{upload_id}', 'DONE', timeout=3600) # 1 hour | ||
| else: | ||
| cache.set(f'tus_upload_{upload_id}', 'ERROR', timeout=3600) # 1 hour | ||
| return HttpResponse(f"Error finalizing upload: {res}", status=409) | ||
| logging.error(f"Error finalizing upload for upload_id {upload_id}: {res}") | ||
| return HttpResponse("Error finalizing upload", status=409) | ||
|
|
||
| response = HttpResponse(status=204) | ||
| response["Upload-Offset"] = str(new_offset) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2438 +/- ##
==========================================
+ Coverage 62.37% 62.40% +0.02%
==========================================
Files 231 227 -4
Lines 24695 24646 -49
Branches 3070 3070
==========================================
- Hits 15404 15380 -24
+ Misses 8620 8591 -29
- Partials 671 675 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.