fix: add filename length safety check with random suffix #1515
Conversation
Reviewer's Guide by SourceryThis pull request introduces a mechanism to ensure that filenames do not exceed the maximum allowed length. It implements a new helper function, Sequence diagram for filename validation and truncationsequenceDiagram
participant User
participant FilerUtils
participant EnsureSafeLength
User->>FilerUtils: Uploads file with long name
FilerUtils->>FilerUtils: get_valid_filename(filename)
FilerUtils->>EnsureSafeLength: _ensure_safe_length(filename, max_length)
alt filename length > max_length
EnsureSafeLength->>EnsureSafeLength: Truncate filename
EnsureSafeLength->>EnsureSafeLength: Append random suffix
end
EnsureSafeLength-->>FilerUtils: safe_filename
FilerUtils-->>User: Returns safe_filename
Updated class diagram for filer.utils.filesclassDiagram
class filer.utils.files {
+slugify(string)
+get_valid_filename(s)
}
class _ensure_safe_length {
+filename: str
+max_length: int
+random_suffix_length: int
+__init__(filename: str, max_length: int, random_suffix_length: int)
+truncate_and_add_suffix(filename: str, max_length: int, random_suffix_length: int)
}
filer.utils.files -- _ensure_safe_length : uses
note for filer.utils.files "get_valid_filename now uses _ensure_safe_length to limit filename length"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Baraff24 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a setting to control the length of the random suffix.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 4 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Integration of the suggestion from the community and sourcery-ai
I have integrated all the tests suggested by sourcery-ai. Tell me if you think I should change anything else, thanks :) |
|
@Baraff24 I came across another related issue: #1093 The connection is this: Filer generates image thumbnails, these thumbnails have a fixed(see #1093) name being the original name plus some scaling info plus the original suffix. We need to account for space the the thumbnail scaling info. Maybe you can figure out what the longest possible scaling info is, otherwise I suggest to at least keep space for, say, 100 extra characters for the thumbnails. |
@fsbraun The first solution that came to mind was to truncate to 155 characters with the same system in order to leave room for any other characters for all the image sizing settings. What do you think? |
|
@Baraff24 Yes, I think that's necessary! |
|
Honestly, I prefer fixed values in the code (some constant). But it would need to be correct, e.g., 155, but not 255. |
…_ensure_safe_length
@fsbraun I applied your advices into my code. Check it now if you want |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
@fsbraun already done. Now can we merge? |
| hexadecimal suffix of length 16 is appended, resulting in exactly 255 characters. | ||
| """ | ||
| base = "a" * 300 # 300 characters base | ||
| original = f"{base}.jpg" | ||
| result = get_valid_filename(original) | ||
| self.assertEqual( | ||
| len(result), | ||
| self.max_length, | ||
| "Filename length should be exactly 255 characters." |
There was a problem hiding this comment.
The 255s here should probably be 155.
Description
This pull request enhances the file upload process by introducing a new helper function, _ensure_safe_length, which ensures that filenames do not exceed the maximum allowed length (255 characters by default). If a filename is too long, it is truncated and a random hexadecimal suffix is appended to maintain uniqueness and comply with database constraints.
Details
Safe Filename Truncation:
The new _ensure_safe_length function checks the length of the filename and, when necessary, truncates it by reserving space for a random suffix. This suffix (default length of 16 characters) is generated using uuid.uuid4().hex and appended to the truncated filename.
Integration with get_valid_filename:
The get_valid_filename function has been updated to utilize _ensure_safe_length. In addition to slugifying the filename (to remove umlauts and special characters), it now guarantees that the resulting filename complies with the maximum length requirement.
Robustness in Upload Handling:
The changes further strengthen error handling during file uploads by ensuring that invalid content lengths or mismatches between MIME types and file extensions trigger a clear UploadException.
Reference:
This update addresses Issue #1270, ensuring that filenames generated during uploads remain safe for storage.
Related resources
issue: #1270
Checklist
masterSlack to find a “pr review buddy” who is going to review my pull request.
Summary by Sourcery
Implements a filename length safety check to ensure filenames do not exceed the maximum allowed length. It truncates the filename and appends a random hexadecimal suffix to maintain uniqueness and comply with database constraints.
Bug Fixes: