Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances path traversal protection when downloading assets by resolving paths with os.path.realpath and checking if the requested file is within the asset's directory, supported by a new unit test. It also simplifies admin authorization logic and adjusts logging levels. However, using startswith for path validation is vulnerable to partial directory name matching (e.g., /app/dir and /app/dir_fake). It is recommended to use os.path.commonpath to ensure robust path component comparison.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| localfile = os.path.realpath(localfile) | ||
| realassetpath = os.path.realpath(dir0) | ||
| if not localfile.startswith(realassetpath): | ||
| logger.error(f"Tentative path traversal for asset {asset.id} on path [{path}]") | ||
| return HttpResponse(f"File not found for asset {asset.id}", status=400) |
There was a problem hiding this comment.
Using startswith on resolved paths is vulnerable to path traversal if one directory name is a prefix of another (e.g., /app/dir and /app/dir_fake). Since startswith performs a simple string prefix check, /app/dir_fake would be considered inside /app/dir because /app/dir_fake.startswith(/app/dir) is True.
To prevent this, use os.path.commonpath which compares path components rather than raw strings.
| localfile = os.path.realpath(localfile) | |
| realassetpath = os.path.realpath(dir0) | |
| if not localfile.startswith(realassetpath): | |
| logger.error(f"Tentative path traversal for asset {asset.id} on path [{path}]") | |
| return HttpResponse(f"File not found for asset {asset.id}", status=400) | |
| localfile = os.path.realpath(localfile) | |
| realassetpath = os.path.realpath(dir0) | |
| if os.path.commonpath([realassetpath, localfile]) != realassetpath: | |
| logger.error(f"Tentative path traversal for asset {asset.id} on path [{path}]") | |
| return HttpResponse(f"File not found for asset {asset.id}", status=400) |
Checklist
For all pull requests:
The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):
Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.