fix: sanitize filenames in upload paths to prevent path traversal#8879
fix: sanitize filenames in upload paths to prevent path traversal#8879sriramveeraghanta wants to merge 4 commits intopreviewfrom
Conversation
…SA-v57h-5999-w7xp) Add server-side filename sanitization across all file upload endpoints to prevent path traversal sequences (../) in user-supplied filenames from being incorporated into S3 object keys. While S3 keys are flat strings and not vulnerable to filesystem traversal, this adds defense-in-depth and prevents S3 key pollution. Changes: - Add sanitize_filename() utility in path_validator.py - Sanitize filenames in get_upload_path() for FileAsset and IssueAttachment models - Sanitize name parameter in all upload view endpoints
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Endpoint as Upload Endpoint
participant Sanitizer as sanitize_filename()
participant Model as get_upload_path / Asset Model
participant S3 as S3 / Storage
Client->>Endpoint: POST upload (filename/name)
Endpoint->>Sanitizer: sanitize_filename(name)
Sanitizer-->>Endpoint: sanitized_name or None
alt sanitized_name falsy and endpoint uses fallback
Endpoint->>Model: use "unnamed" or uuid component to build asset_key
else sanitized_name truthy
Endpoint->>Model: use sanitized_name to build asset_key
end
Model->>S3: generate/upload path using sanitized component
S3-->>Endpoint: presigned/post response
Endpoint-->>Client: response (asset metadata, asset_key)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR hardens file upload handling by sanitizing user-supplied filenames before they are incorporated into storage object keys / upload paths, mitigating path traversal–style input and S3 key pollution concerns related to GHSA-v57h-5999-w7xp.
Changes:
- Added a shared
sanitize_filename()utility for upload/path contexts. - Applied filename sanitization in
get_upload_path()forFileAssetandIssueAttachment. - Applied filename sanitization across multiple upload/presign endpoints (app, api, space), including asset duplication key generation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/plane/utils/path_validator.py | Introduces sanitize_filename() used across upload flows. |
| apps/api/plane/db/models/asset.py | Sanitizes filename inside FileAsset upload path generation. |
| apps/api/plane/db/models/issue.py | Sanitizes filename inside issue attachment upload path generation. |
| apps/api/plane/app/views/asset/v2.py | Sanitizes request filename across multiple app asset upload endpoints and in duplication key generation. |
| apps/api/plane/app/views/issue/attachment.py | Sanitizes request filename for issue attachment presign flow. |
| apps/api/plane/api/views/asset.py | Sanitizes request filename for user/server/generic asset presign endpoints. |
| apps/api/plane/api/views/issue.py | Sanitizes request filename for issue attachment presign endpoint. |
| apps/api/plane/space/views/asset.py | Sanitizes request filename for space deploy-board asset upload flow. |
Comments suppressed due to low confidence (1)
apps/api/plane/api/views/issue.py:1869
sanitize_filename(...)returns'unnamed'for missing/empty input, sonameis no longer falsy when the client omits thenamefield. This makes the laterif not namevalidation ineffective and changes behavior vs the documented request serializer (which requiresname). Either validate the rawrequest.data.get('name')separately or adjust the validation/error messaging and serializer/docs to match the new fallback behavior.
name = sanitize_filename(request.data.get("name"))
type = request.data.get("type", False)
size = request.data.get("size")
external_id = request.data.get("external_id")
external_source = request.data.get("external_source")
| # Take only the basename to remove any directory components | ||
| filename = os.path.basename(filename) | ||
|
|
||
| # Remove any remaining path traversal sequences | ||
| filename = filename.replace("..", "") | ||
|
|
There was a problem hiding this comment.
os.path.basename() does not treat backslashes (\) as path separators on POSIX, so inputs like ..\\..\\evil.txt will not have directory components stripped as intended. Consider normalizing separators (e.g., replace \ with /) or explicitly handling Windows paths before taking the basename.
| # Remove leading dots (hidden files) | ||
| filename = filename.lstrip(".") | ||
|
|
||
| # Strip whitespace | ||
| filename = filename.strip() | ||
|
|
There was a problem hiding this comment.
Whitespace is stripped after lstrip('.'), so a filename like ' .env' becomes '.env' after .strip() and the leading dot is no longer removed. Strip whitespace before removing leading dots (or re-run the leading-dot removal after stripping) so hidden-file prefixes can’t bypass sanitization.
| # Remove leading dots (hidden files) | |
| filename = filename.lstrip(".") | |
| # Strip whitespace | |
| filename = filename.strip() | |
| # Strip whitespace before removing leading dots so hidden-file | |
| # prefixes cannot be preserved by surrounding spaces. | |
| filename = filename.strip() | |
| # Remove leading dots (hidden files) | |
| filename = filename.lstrip(".") |
| name = sanitize_filename(request.data.get("name")) | ||
| type = request.data.get("type") | ||
| size = int(request.data.get("size", settings.FILE_SIZE_LIMIT)) | ||
| project_id = request.data.get("project_id") |
There was a problem hiding this comment.
Because sanitize_filename(...) falls back to 'unnamed', name will no longer be falsy when the client omits/empties the name field. That makes the later if not name validation/error path ineffective and changes the API behavior vs the documented request serializer (which marks name as required). Consider validating the raw input separately (if name must be provided) or updating the validation/error message and serializer/docs to match the new fallback behavior.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/plane/utils/path_validator.py`:
- Around line 15-33: The sanitize_filename function currently uses
os.path.basename but doesn't normalize Windows-style backslashes, allowing
inputs like "..\\..\\evil.txt" to bypass directory stripping; before calling
os.path.basename in sanitize_filename, replace backslashes with forward slashes
(e.g., filename = filename.replace("\\\\", "/")) so both Windows and POSIX
separators are normalized, then proceed with os.path.basename and the existing
removal of ".." and null bytes to ensure path traversal sequences are reliably
stripped across platforms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e1f7f45-8639-4b68-82f2-e06c098e7fcd
📒 Files selected for processing (8)
apps/api/plane/api/views/asset.pyapps/api/plane/api/views/issue.pyapps/api/plane/app/views/asset/v2.pyapps/api/plane/app/views/issue/attachment.pyapps/api/plane/db/models/asset.pyapps/api/plane/db/models/issue.pyapps/api/plane/space/views/asset.pyapps/api/plane/utils/path_validator.py
- Remove unused `import re` - Normalize backslashes to forward slashes before os.path.basename() so Windows-style paths (e.g. ..\..\..\evil.txt) are handled on POSIX - Strip whitespace before removing leading dots so " .env" is caught - Return None instead of "unnamed" for empty input so existing `if not name` validation guards remain effective - Add `or "unnamed"` fallback at call sites that lack a name guard
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/app/views/asset/v2.py (1)
761-767:⚠️ Potential issue | 🟠 MajorDuplicate flow still propagates unsanitized filename metadata, and this block fails lint.
Line 761 is currently too long (ruff E501), and Line 764 preserves the original unsanitized name. Reuse a single sanitized value for both
destination_keyandattributes["name"]to keep behavior consistent and avoid carrying unsafe legacy names forward.🔧 Proposed fix
- destination_key = f"{workspace.id}/{uuid.uuid4().hex}-{sanitize_filename(original_asset.attributes.get('name')) or 'unnamed'}" + sanitized_name = sanitize_filename(original_asset.attributes.get("name")) or "unnamed" + destination_key = f"{workspace.id}/{uuid.uuid4().hex}-{sanitized_name}" duplicated_asset = FileAsset.objects.create( attributes={ - "name": original_asset.attributes.get("name"), + "name": sanitized_name, "type": original_asset.attributes.get("type"), "size": original_asset.attributes.get("size"), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/app/views/asset/v2.py` around lines 761 - 767, The destination key and duplicated asset attributes currently use the unsanitized original name and the long destination_key line triggers ruff E501; compute a sanitized_name once (e.g., sanitized_name = sanitize_filename(original_asset.attributes.get("name")) or "unnamed"), use that sanitized_name when building destination_key and set attributes["name"] to sanitized_name in the FileAsset.objects.create call (reference symbols: destination_key, duplicated_asset, FileAsset.objects.create, sanitize_filename, original_asset.attributes.get("name")) and split/format the destination_key construction into multiple concatenated parts or use f-string wrapping to satisfy line-length linting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/api/plane/app/views/asset/v2.py`:
- Around line 761-767: The destination key and duplicated asset attributes
currently use the unsanitized original name and the long destination_key line
triggers ruff E501; compute a sanitized_name once (e.g., sanitized_name =
sanitize_filename(original_asset.attributes.get("name")) or "unnamed"), use that
sanitized_name when building destination_key and set attributes["name"] to
sanitized_name in the FileAsset.objects.create call (reference symbols:
destination_key, duplicated_asset, FileAsset.objects.create, sanitize_filename,
original_asset.attributes.get("name")) and split/format the destination_key
construction into multiple concatenated parts or use f-string wrapping to
satisfy line-length linting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 196afb65-a490-45ad-9714-bcec781e7ac4
📒 Files selected for processing (7)
apps/api/plane/api/views/asset.pyapps/api/plane/app/views/asset/v2.pyapps/api/plane/app/views/issue/attachment.pyapps/api/plane/db/models/asset.pyapps/api/plane/db/models/issue.pyapps/api/plane/space/views/asset.pyapps/api/plane/utils/path_validator.py
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/plane/db/models/asset.py
- apps/api/plane/space/views/asset.py
- apps/api/plane/api/views/asset.py
- apps/api/plane/utils/path_validator.py
Summary
sanitize_filename()utility that strips path traversal sequences (../), null bytes, directory components, and leading dots from user-supplied filenamesget_upload_path()model functionsContext
A security advisory reported that user-supplied filenames flow unsanitized into S3 object keys via
asset_keyconstruction andget_upload_path(). While S3 keys are flat strings (no filesystem traversal), this fix adds defense-in-depth against S3 key pollution and protects against risk if the storage backend were ever changed.Files changed:
apps/api/plane/utils/path_validator.py— newsanitize_filename()functionapps/api/plane/db/models/asset.py— sanitize inget_upload_path()apps/api/plane/db/models/issue.py— sanitize inget_upload_path()apps/api/plane/app/views/asset/v2.py— sanitize in 3 upload endpoints + duplicate endpointapps/api/plane/app/views/issue/attachment.py— sanitize in attachment uploadapps/api/plane/api/views/asset.py— sanitize in 3 API upload endpointsapps/api/plane/api/views/issue.py— sanitize in issue attachment APIapps/api/plane/space/views/asset.py— sanitize in space entity uploadTest plan
../sequences are stripped to basename.) have leading dots strippedSummary by CodeRabbit