-
Notifications
You must be signed in to change notification settings - Fork 297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lectures
: Use attachment name for file downloads
#9775
Lectures
: Use attachment name for file downloads
#9775
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (2)
Line range hint
611-636
: Enhance error logging for troubleshootingWhile the implementation is robust, the error logging could be more detailed to aid in troubleshooting file download issues.
Consider enhancing the error logging to include more context:
catch (IOException ex) { - log.error("Failed to download file: {} on path: {}", filename, path, ex); + log.error("Failed to download file: {} (replaced name: {}) on path: {}", + filename, replaceFilename.orElse("<none>"), path, ex); return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).build(); }
Line range hint
644-651
: Consider enhancing path traversal protectionWhile the current sanitization is good, consider adding additional protection against path traversal attacks.
Consider adding normalized path validation:
private static void sanitizeFilenameElseThrow(String filename) { String sanitizedFileName = FileService.sanitizeFilename(filename); if (!sanitizedFileName.equals(filename)) { throw new EntityNotFoundException("The filename contains invalid characters. Only characters a-z, A-Z, 0-9, '_', '.' and '-' are allowed!"); } + // Additional protection against path traversal + if (!Paths.get(filename).normalize().toString().equals(filename)) { + throw new EntityNotFoundException("Invalid filename: potential path traversal detected"); + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java
(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (1)
Pattern src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (2)
575-611
: LGTM: Well-structured method overloads
The method overloads are well-organized with clear parameter naming and comprehensive documentation. The delegation pattern reduces code duplication.
434-434
: LGTM: Improved file download naming
The implementation correctly uses the attachment name for downloads, providing a better user experience with meaningful filenames instead of system-generated ones.
Also applies to: 490-490
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manual tested on TS§. Worked as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Tested on TS1]
Works as described, couldn't find any issues.
(Might be unrelated but when deploying this PR to TS2, I was unable to login and got an internal server error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS5, works as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally and worked as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS1, everything works as described
Lectures
: Fix naming of downloaded files by using attachment nameLectures
: Use attachment name for file downloads
Checklist
General
Server
Motivation and Context
Issue: In #9721 the cryptic attachment text was removed from the filename. Unfortunately, only superficially. When actually saving the file, with Ctrl + S, the cryptic timestamp is still there.
In the URL:
But when saving:
Description
Fixes this issue, by simply using the defined attachment name.
Steps for Testing
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Screenshots
With the fix:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
FileResource
class.