Conversation
…lder. Signed-off-by: Mats <matjoh50@gmail.com>
Will most likley need some rework to function better with frontend features and database calls. Signed-off-by: Mats <matjoh50@gmail.com>
Signed-off-by: Mats <matjoh50@gmail.com>
Signed-off-by: Mats <matjoh50@gmail.com>
Signed-off-by: Mats Johansson <matjoh50@gmail.com>
… DP for later fetching. Signed-off-by: Mats Johansson <matjoh50@gmail.com>
…everal files Signed-off-by: Mats Johansson <matjoh50@gmail.com>
# Conflicts: # frontend/package-lock.json # frontend/package.json # src/main/java/org/fungover/system2024/file/FileDTO.java # src/main/java/org/fungover/system2024/file/entity/File.java # src/main/resources/application.properties # src/main/resources/file_schema.sql
Signed-off-by: Mats Johansson <matjoh50@gmail.com>
WalkthroughThis pull request introduces a comprehensive file upload and management system for the application. The changes span multiple components, including backend services, database schema, security configuration, and frontend components. The implementation focuses on enhancing file storage capabilities, introducing features like file ownership, soft deletion, and secure file retrieval. The modifications include new exception handling, storage services, and updated data transfer objects to support more robust file management. Changes
Sequence DiagramsequenceDiagram
participant User
participant FileStorageController
participant StorageService
participant FileRepository
participant FileSystem
User->>FileStorageController: Upload multiple files
FileStorageController->>StorageService: store(files)
StorageService->>FileSystem: Save files
StorageService->>FileRepository: Save file metadata
FileRepository-->>StorageService: Metadata saved
StorageService-->>FileStorageController: Upload complete
FileStorageController-->>User: Success response
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java
Fixed
Show fixed
Hide fixed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (12)
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java (3)
53-57: Typo in exception message "Filed"The message "Filed to store empty file" seems like a typo. Consider correcting it to "Failed to store empty file" to avoid confusion and maintain clarity in error logs.
- throw new StorageException("Filed to store empty file"); + throw new StorageException("Failed to store empty file");
70-71: Extract userId from authenticated contextUsing a hard-coded userId of 123 is likely temporary. Ensure that the userId is retrieved from the authenticated user context or a more robust method to align with security best practices.
Would you like help refactoring to retrieve userId from a secured user context?
82-84: Replace System.err.println with a loggerUsing System.err for logging is acceptable for quick debugging, but a logging framework (e.g., SLF4J) is preferable for managing logs, severity levels, and formatting.
- System.err.println("File upload failed: " + e.getMessage()); + logger.error("File upload failed: {}", e.getMessage());src/main/resources/file_data.sql (1)
4-4: Clarify usage of owner "000"Using "000" instead of null or a valid user ID might cause confusion in queries or logs. Consider clarifying whether "000" is a special placeholder.
src/main/java/org/fungover/system2024/fileupload/StorageProperties.java (2)
15-15: Consider environment-specific default paths and add validationThe hard-coded default path "upload-dir" might not be suitable for all environments (dev, staging, prod).
Consider:
- Using environment-specific defaults
- Adding path validation to ensure the directory:
- Has proper permissions
- Is not in a sensitive location
- Exists or can be created
- Has sufficient disk space
1-16: Add Javadoc documentation for configuration propertiesThe class lacks documentation about the expected format and requirements for the storage location.
Add Javadoc explaining:
+/** + * Configuration properties for file storage. + * The location property specifies the base directory for file uploads. + * + * @property location The base directory path for file uploads. + * Can be absolute or relative to the application root. + * Must be writable by the application. + */ @Setter @Gettersrc/main/java/org/fungover/system2024/file/FileDTO.java (2)
6-16: Enhance error handling in fromFile methodThe fromFile method should validate individual fields and provide more specific error messages.
public static FileDTO fromFile(File file) { if (file == null) { throw new IllegalArgumentException("File is null"); } + Objects.requireNonNull(file.getId(), "File ID cannot be null"); + Objects.requireNonNull(file.getOwner(), "File owner cannot be null"); + Objects.requireNonNull(file.getOriginalFilename(), "Original filename cannot be null"); + Objects.requireNonNull(file.getStoredFilename(), "Stored filename cannot be null"); return new FileDTO( file.getId(),
Line range hint
1-18: Add documentation and toString methodThe class lacks documentation about its purpose and usage.
+/** + * Data Transfer Object for file metadata. + * Used for transferring file information between layers without exposing internal implementation details. + */ public record FileDTO(...)src/main/java/org/fungover/system2024/file/FileRepository.java (1)
3-4: Remove unused importsThe validation imports
@NotNulland@Sizeare not being used in this interface.-import jakarta.validation.constraints.NotNull; -import jakarta.validation.constraints.Size;src/main/java/org/fungover/system2024/file/entity/File.java (2)
12-12: Remove unused importThe
ZipEntryimport is not being used in this class.-import java.util.zip.ZipEntry;
40-42: Remove commented code and add index
- Remove the commented
@ColumnDefaultannotation- Consider adding an index on
soft_deleteif it's frequently used in queries-// @ColumnDefault("0") @Column(name = "soft_delete") + @Index(name = "idx_soft_delete") private Boolean softDelete = false;src/main/java/org/fungover/system2024/fileupload/FileStorageController.java (1)
18-24: Remove Field Injection in Favor of Constructor InjectionThe controller uses both field and constructor injection, which is redundant.
- @Autowired - StorageService storageService; + private final StorageService storageService; public FileStorageController(StorageService storageService) { this.storageService = storageService; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.gitignore(1 hunks)src/main/java/org/fungover/system2024/config/SecurityConfig.java(1 hunks)src/main/java/org/fungover/system2024/file/FileDTO.java(1 hunks)src/main/java/org/fungover/system2024/file/FileRepository.java(1 hunks)src/main/java/org/fungover/system2024/file/entity/File.java(2 hunks)src/main/java/org/fungover/system2024/fileupload/Exceptions/StorageException.java(1 hunks)src/main/java/org/fungover/system2024/fileupload/Exceptions/StorageFileNotFoundException.java(1 hunks)src/main/java/org/fungover/system2024/fileupload/FileStorageController.java(1 hunks)src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java(1 hunks)src/main/java/org/fungover/system2024/fileupload/StorageProperties.java(1 hunks)src/main/java/org/fungover/system2024/fileupload/StorageService.java(1 hunks)src/main/resources/application.properties(1 hunks)src/main/resources/file_data.sql(1 hunks)src/main/resources/file_schema.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (7)
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java (2)
65-67: Ensure thorough security checks
This security check correctly prevents storing files outside the designated directory. Keep verifying that any path-based user input does not allow directory traversal or other unsafe patterns.
126-131: Consider logging invalid fileIds
Currently, invalid file IDs are skipped silently. Consider logging them, so you have a record of skipped or non-existent files.
src/main/resources/file_data.sql (1)
2-3: Potential confusion with placeholders
Storing partial URLs like "www.hello.com" and "www.goodbye.com" as the storedFilename could be misleading if used in real usage. Confirm if these values are placeholders or actual file references.
src/main/java/org/fungover/system2024/fileupload/Exceptions/StorageException.java (1)
3-10: LGTM: StorageException
This basic runtime exception setup with constructors for message and cause is clear and effective.
src/main/java/org/fungover/system2024/fileupload/Exceptions/StorageFileNotFoundException.java (1)
3-10: LGTM: StorageFileNotFoundException
Extending StorageException to handle file-not-found scenarios is a good approach.
src/main/java/org/fungover/system2024/file/entity/File.java (1)
31-33: 🛠️ Refactor suggestion
Add unique constraint for stored filename
To prevent potential conflicts, add a unique constraint on the stored_filename column since it's used for file storage.
- @Column(name = "stored_filename", nullable = false)
+ @Column(name = "stored_filename", nullable = false, unique = true)
private String storedFilename;Likely invalid or redundant comment.
src/main/resources/application.properties (1)
9-9: Verify Development Profile Usage
The active profile is set to development, which disables CSRF protection. Ensure this is changed to production before deployment.
✅ Verification successful
Profile Configuration is Correctly Implemented
The security configuration is properly implemented with separate beans for development and production profiles:
- Development profile has CSRF disabled (as noted in the review)
- Production profile has CSRF protection enabled by default
- The security measures are controlled by profile-specific configurations, which is a good practice
The concern about changing to production profile before deployment is valid, but the implementation already handles this correctly through Spring's profile-specific bean configuration. When the application runs with the production profile, it will automatically use the secure configuration with CSRF enabled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any deployment scripts or CI configurations that might need profile verification
rg -l "spring.profiles.active" .github/ .gitlab-ci.yml Dockerfile* deployment/
Length of output: 239
Script:
#!/bin/bash
# Let's check for security configurations and CSRF settings in the codebase
rg -l "csrf" --type-add 'config:*.{properties,yml,yaml,xml}' -t config
# Check for any Spring Security configuration files
rg -l "WebSecurityConfigurerAdapter|SecurityFilterChain|@EnableWebSecurity" --type java
# Check for different profile configurations
fd -e properties -e yml -e yaml | xargs grep -l "spring.profiles"
Length of output: 333
Script:
#!/bin/bash
# Let's examine the security configuration to see how CSRF is handled
cat src/main/java/org/fungover/system2024/config/SecurityConfig.java
# Let's also check if there are any profile-specific property files
fd -e properties -e yml -e yaml "application-.*"
Length of output: 2271
| String fileExtension = sanitizedFilename.substring(sanitizedFilename.lastIndexOf(".")); | ||
| return UUID.randomUUID().toString() + fileExtension; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate extension extraction for files without '.'
This logic assumes the filename has an extension. If a file is uploaded without any '.', this may cause an IndexOutOfBoundsException at runtime. Consider additional checks to handle files without dots in their names.
String fileExtension = "";
int dotIndex = sanitizedFilename.lastIndexOf(".");
if (dotIndex != -1) {
fileExtension = sanitizedFilename.substring(dotIndex);
}
return UUID.randomUUID().toString() + fileExtension;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String fileExtension = sanitizedFilename.substring(sanitizedFilename.lastIndexOf(".")); | |
| return UUID.randomUUID().toString() + fileExtension; | |
| String fileExtension = ""; | |
| int dotIndex = sanitizedFilename.lastIndexOf("."); | |
| if (dotIndex != -1) { | |
| fileExtension = sanitizedFilename.substring(dotIndex); | |
| } | |
| return UUID.randomUUID().toString() + fileExtension; |
| original_filename VARCHAR(255) | ||
| CHARACTER SET utf8mb4 | ||
| COLLATE utf8mb4_unicode_ci NOT NULL, | ||
| file_url TEXT | ||
| stored_filename VARCHAR(255) | ||
| CHARACTER SET utf8mb4 | ||
| COLLATE utf8mb4_unicode_ci NOT NULL | ||
| ); | ||
| COLLATE utf8mb4_unicode_ci NOT NULL, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add length constraints and index for filename columns
The VARCHAR(255) columns should have appropriate length constraints based on business requirements. Also, consider adding an index for filename lookups.
- original_filename VARCHAR(255)
+ original_filename VARCHAR(100)
CHARACTER SET utf8mb4
COLLATE utf8mb4_unicode_ci NOT NULL,
- stored_filename VARCHAR(255)
+ stored_filename VARCHAR(40)
CHARACTER SET utf8mb4
COLLATE utf8mb4_unicode_ci NOT NULL,
+ INDEX idx_stored_filename (stored_filename),
+ INDEX idx_owner_soft_delete (owner, soft_delete)Committable suggestion skipped: line range outside the PR's diff.
| import org.fungover.system2024.file.entity.File; | ||
|
|
||
| public record FileDTO(String name, String fileUrl) { | ||
| public record FileDTO(Integer id, Integer owner, String originalName, String storedFilename ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation annotations for DTO fields
The DTO lacks input validation which could lead to invalid data being transferred.
+import jakarta.validation.constraints.*;
+
-public record FileDTO(Integer id, Integer owner, String originalName, String storedFilename ) {
+public record FileDTO(
+ @Null(groups = Create.class)
+ @NotNull(groups = Update.class)
+ Integer id,
+
+ @NotNull
+ @Min(1)
+ Integer owner,
+
+ @NotBlank
+ @Size(max = 100)
+ String originalName,
+
+ @NotBlank
+ @Size(max = 40)
+ @Pattern(regexp = "^[a-zA-Z0-9-_]+$")
+ String storedFilename
+) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public record FileDTO(Integer id, Integer owner, String originalName, String storedFilename ) { | |
| import jakarta.validation.constraints.*; | |
| public record FileDTO( | |
| @Null(groups = Create.class) | |
| @NotNull(groups = Update.class) | |
| Integer id, | |
| @NotNull | |
| @Min(1) | |
| Integer owner, | |
| @NotBlank | |
| @Size(max = 100) | |
| String originalName, | |
| @NotBlank | |
| @Size(max = 40) | |
| @Pattern(regexp = "^[a-zA-Z0-9-_]+$") | |
| String storedFilename | |
| ) { |
| ###Path to folder where to save files | ||
| storage.location=src/main/java/org/fungover/system2024/fileupload/ftp | ||
| #storage.location=/data/uploads |
There was a problem hiding this comment.
Security Risk: Unsafe File Storage Location
Storing uploaded files in the source directory poses several risks:
- Files could be accidentally committed to version control
- Source directory may not have proper permissions in production
- Path could be non-existent in production environment
Consider using a dedicated storage directory outside the application source:
-storage.location=src/main/java/org/fungover/system2024/fileupload/ftp
+storage.location=${STORAGE_LOCATION:/var/lib/system2024/uploads}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ###Path to folder where to save files | |
| storage.location=src/main/java/org/fungover/system2024/fileupload/ftp | |
| #storage.location=/data/uploads | |
| ###Path to folder where to save files | |
| storage.location=${STORAGE_LOCATION:/var/lib/system2024/uploads} | |
| #storage.location=/data/uploads |
|
|
||
| @GetMapping("/list/{id}") | ||
| public ResponseEntity getAllUsersFileNames(@PathVariable("id") Integer userId) { | ||
| System.out.println(userId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace System.out with Proper Logging
Using System.out.println is not suitable for production code. Use a proper logging framework instead.
- System.out.println(userId);
+ log.debug("Fetching files for user: {}", userId);Add this import and annotation:
import lombok.extern.slf4j.Slf4j;
@Slf4j
@Controller| public ResponseEntity<String> handleMultiFileUpload(@RequestParam("files") List<MultipartFile> files) { | ||
| try { | ||
| for (MultipartFile file : files) { | ||
| if (file.isEmpty()) { | ||
| return ResponseEntity.badRequest().body("Filed to store empty file"); | ||
| } | ||
| } | ||
| storageService.store(files); | ||
| return ResponseEntity.ok().body("All files uploaded successfully\n"); | ||
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| return ResponseEntity.badRequest().body("Error while uploading files\n"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add File Upload Validations and Improve Error Handling
The current implementation lacks several important validations and proper error handling:
- No file size limits
- No file type validation
- Generic exception handling
- Potential resource leaks
Consider implementing these improvements:
@PostMapping("/")
-public ResponseEntity<String> handleMultiFileUpload(@RequestParam("files") List<MultipartFile> files) {
+public ResponseEntity<String> handleMultiFileUpload(
+ @RequestParam("files") List<MultipartFile> files,
+ @RequestParam(defaultValue = "10MB") @Max(50_000_000) long maxFileSize,
+ @RequestParam(defaultValue = "image/*,application/pdf") String allowedTypes
+) {
try {
for (MultipartFile file : files) {
- if (file.isEmpty()) {
- return ResponseEntity.badRequest().body("Filed to store empty file");
+ if (file.isEmpty() || file.getSize() > maxFileSize ||
+ !isAllowedFileType(file.getContentType(), allowedTypes)) {
+ return ResponseEntity.badRequest().body(
+ String.format("Invalid file: %s (size: %d, type: %s)",
+ file.getOriginalFilename(), file.getSize(),
+ file.getContentType()));
}
}
storageService.store(files);
return ResponseEntity.ok().body("All files uploaded successfully\n");
- } catch (Exception e) {
- e.printStackTrace();
+ } catch (StorageException e) {
+ log.error("Storage error during file upload", e);
return ResponseEntity.badRequest().body("Error while uploading files\n");
+ } catch (Exception e) {
+ log.error("Unexpected error during file upload", e);
+ return ResponseEntity.internalServerError().body("Internal server error\n");
}
}Committable suggestion skipped: line range outside the PR's diff.
| @GetMapping("/list/{id}") | ||
| public ResponseEntity getAllUsersFileNames(@PathVariable("id") Integer userId) { | ||
| System.out.println(userId); | ||
| List<FileDTO> listOfNames = storageService.getListOfFiles(userId); | ||
| return ResponseEntity.ok(listOfNames); | ||
| } |
There was a problem hiding this comment.
Add Security Checks for File Access
The endpoint allows fetching any user's files without proper authorization checks.
Implement proper authorization:
@GetMapping("/list/{id}")
-public ResponseEntity getAllUsersFileNames(@PathVariable("id") Integer userId) {
+public ResponseEntity getAllUsersFileNames(
+ @PathVariable("id") Integer userId,
+ @AuthenticationPrincipal UserDetails userDetails
+) {
+ if (!userDetails.getId().equals(userId) && !userDetails.hasRole("ADMIN")) {
+ return ResponseEntity.status(HttpStatus.FORBIDDEN).build();
+ }
List<FileDTO> listOfNames = storageService.getListOfFiles(userId);
return ResponseEntity.ok(listOfNames);
}Committable suggestion skipped: line range outside the PR's diff.
| @PostMapping("/fetch/") | ||
| @ResponseBody | ||
| public ResponseEntity<ByteArrayResource> fetchMultipleFiles(@RequestBody Map<String, List<Integer>> payload) { | ||
| List<Integer> fileIds = payload.get("request"); | ||
| return storageService.loadAllResourseAsZip(fileIds); | ||
| } |
There was a problem hiding this comment.
Secure File Download Endpoint
The file download endpoint needs similar security checks and proper error handling.
Add authorization and validation:
@PostMapping("/fetch/")
@ResponseBody
-public ResponseEntity<ByteArrayResource> fetchMultipleFiles(@RequestBody Map<String, List<Integer>> payload) {
+public ResponseEntity<ByteArrayResource> fetchMultipleFiles(
+ @RequestBody Map<String, List<Integer>> payload,
+ @AuthenticationPrincipal UserDetails userDetails
+) {
List<Integer> fileIds = payload.get("request");
+ if (!storageService.validateFileAccess(fileIds, userDetails.getId())) {
+ return ResponseEntity.status(HttpStatus.FORBIDDEN).build();
+ }
return storageService.loadAllResourseAsZip(fileIds);
}Committable suggestion skipped: line range outside the PR's diff.
|
There are SonarCloud failed conditions and comments from the coderabbit that I think you should look through before we can review this PR. There is also a few fails in the checks that needs to be solved. 👍 :) Let me or the class know if you need help. |
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java (1)
182-185:⚠️ Potential issueFix potential NullPointerException in extension extraction
The current implementation assumes the filename has an extension and could throw NPE. Add proper validation.
private String generateStoredFilename(String sanitizedFilename) { - String fileExtension = sanitizedFilename.substring(sanitizedFilename.lastIndexOf(".")); + String fileExtension = ""; + int lastDotIndex = sanitizedFilename.lastIndexOf("."); + if (lastDotIndex > 0) { + fileExtension = sanitizedFilename.substring(lastDotIndex); + } return UUID.randomUUID().toString() + fileExtension; }
🧹 Nitpick comments (1)
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java (1)
121-158: Add error handling for ZIP file size limitsThe ZIP file creation doesn't check for potential memory issues when handling large files or many files at once.
public ResponseEntity<ByteArrayResource> loadAllResourseAsZip(List<Integer> fileIds) { + long totalSize = 0; + final long MAX_ZIP_SIZE = 1024 * 1024 * 100; // 100MB limit try (ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream(); ZipOutputStream zipOutputStream = new ZipOutputStream(byteArrayOutputStream)) { for (Integer fileId : fileIds) { File fileData = fileRepository.getFileById(fileId); if (fileData == null) { continue; } Path filePath = load(fileData.getStoredFilename()); + long fileSize = Files.size(filePath); + totalSize += fileSize; + if (totalSize > MAX_ZIP_SIZE) { + throw new StorageException("Total ZIP size exceeds limit of 100MB"); + } Resource resource = loadAsResource(fileData.getStoredFilename());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java(1 hunks)
| System.err.println("File upload failed: " + e.getMessage()); | ||
| throw e; // Rethrow the exception or handle it as necessary |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace System.err with proper logging
Using System.err for logging is not recommended in production code. Use a proper logging framework instead.
-System.err.println("File upload failed: " + e.getMessage());
-throw e; // Rethrow the exception or handle it as necessary
+log.error("File upload failed", e);
+throw new StorageException("Failed to store file", e);Add this import and field:
import lombok.extern.slf4j.Slf4j;
@Slf4j
@Service
public class FileSystemStorageService implements StorageService {| package org.fungover.system2024.fileupload; | ||
|
|
||
| import org.fungover.system2024.file.FileDTO; | ||
| import org.fungover.system2024.file.FileRepository; | ||
| import org.fungover.system2024.file.entity.File; | ||
| import org.fungover.system2024.fileupload.Exceptions.StorageException; | ||
| import org.fungover.system2024.fileupload.Exceptions.StorageFileNotFoundException; | ||
| import org.springframework.beans.factory.annotation.Value; | ||
| import org.springframework.core.io.ByteArrayResource; | ||
| import org.springframework.core.io.Resource; | ||
| import org.springframework.core.io.UrlResource; | ||
| import org.springframework.http.HttpHeaders; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.stereotype.Service; | ||
| import org.springframework.util.FileSystemUtils; | ||
| import org.springframework.web.multipart.MultipartFile; | ||
|
|
||
| import java.io.ByteArrayOutputStream; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.net.MalformedURLException; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.nio.file.Paths; | ||
| import java.nio.file.StandardCopyOption; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
| import java.util.UUID; | ||
| import java.util.stream.Stream; | ||
| import java.util.zip.ZipEntry; | ||
| import java.util.zip.ZipOutputStream; | ||
|
|
||
| @Service | ||
| public class FileSystemStorageService implements StorageService { | ||
|
|
||
|
|
||
| private final FileRepository fileRepository; | ||
| private final Path fileLocation; | ||
|
|
||
| public FileSystemStorageService(FileRepository fileRepository, @Value("${storage.location}") String storageLocation) { | ||
| this.fileRepository = fileRepository; | ||
| this.fileLocation = Paths.get(storageLocation); | ||
| } | ||
|
|
||
|
|
||
| @Override | ||
| public File getFileData(Integer fileId) { | ||
| return fileRepository.getFileById(fileId); | ||
| } | ||
|
|
||
| @Override | ||
| public void store(List<MultipartFile> files) { | ||
| try { | ||
| for (MultipartFile file : files) { | ||
| if (file.isEmpty()) { | ||
| throw new StorageException("Filed to store empty file"); | ||
| } | ||
| String sanitizedFilename = sanitizeFilename(Objects.requireNonNull(file.getOriginalFilename())); | ||
| String storedFilename = generateStoredFilename(sanitizedFilename); | ||
|
|
||
| Path destinationFile = fileLocation.resolve( | ||
| Paths.get(Objects.requireNonNull(storedFilename))) | ||
| .normalize().toAbsolutePath(); | ||
|
|
||
| if (!destinationFile.getParent().equals(fileLocation.toAbsolutePath())) { | ||
| //Security check | ||
| throw new StorageException("Cannot store file outside current directory"); | ||
| } | ||
|
|
||
| // TODO: Change to auth userID | ||
| int userId = 123; | ||
|
|
||
| try (InputStream inputStream = file.getInputStream()) { | ||
| Files.copy(inputStream, destinationFile, StandardCopyOption.REPLACE_EXISTING); | ||
| // Save metadata to the database | ||
| File metadata = new File(); | ||
| metadata.setOwner(userId); | ||
| metadata.setOriginalFilename(file.getOriginalFilename()); | ||
| metadata.setStoredFilename(storedFilename); | ||
| fileRepository.save(metadata); | ||
| } catch (IOException e) { | ||
| System.err.println("File upload failed: " + e.getMessage()); | ||
| throw e; // Rethrow the exception or handle it as necessary | ||
| } | ||
| } | ||
|
|
||
| } catch (IOException e) { | ||
| throw new StorageException("Cannot store file" + e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Stream<Path> loadAll() { | ||
| try { | ||
| return Files.walk(fileLocation, 1) | ||
| .filter(path -> !path.equals(fileLocation)) | ||
| .map(fileLocation::relativize); | ||
| } catch (IOException e) { | ||
| throw new StorageException("Cannot load files"); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Path load(String filename) { | ||
| return fileLocation.resolve(filename); | ||
| } | ||
|
|
||
| @Override | ||
| public Resource loadAsResource(String filename) { | ||
| try { | ||
| Path file = load(filename); | ||
| Resource resource = new UrlResource(file.toUri()); | ||
| if (resource.exists() || resource.isReadable()) { | ||
| return resource; | ||
| } else throw new StorageException("Cannot load file"); | ||
| } catch (MalformedURLException e) { | ||
| throw new StorageFileNotFoundException("Cannot load file" + filename, e); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public ResponseEntity<ByteArrayResource> loadAllResourseAsZip(List<Integer> fileIds) { | ||
| try (ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream(); | ||
| ZipOutputStream zipOutputStream = new ZipOutputStream(byteArrayOutputStream)) { | ||
|
|
||
| for (Integer fileId : fileIds) { | ||
| // Fetch file data | ||
| File fileData = fileRepository.getFileById(fileId); | ||
| if (fileData == null) { | ||
| continue; // Skip invalid file IDs | ||
| } | ||
|
|
||
| // Load file as Resource | ||
| Path filePath = load(fileData.getStoredFilename()); | ||
| Resource resource = loadAsResource(fileData.getStoredFilename()); | ||
|
|
||
| if (resource.exists() && resource.isReadable()) { | ||
| // Add file to ZIP | ||
| zipOutputStream.putNextEntry(new ZipEntry(fileData.getOriginalFilename())); | ||
| Files.copy(filePath, zipOutputStream); | ||
| zipOutputStream.closeEntry(); | ||
| } | ||
| } | ||
|
|
||
| zipOutputStream.finish(); | ||
|
|
||
| // Return the ZIP file as a ResponseEntity | ||
| ByteArrayResource zipResource = new ByteArrayResource(byteArrayOutputStream.toByteArray()); | ||
|
|
||
| return ResponseEntity.ok() | ||
| .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"files.zip\"") | ||
| .body(zipResource); | ||
|
|
||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| return ResponseEntity.internalServerError().build(); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| @Override | ||
| public List<FileDTO> getListOfFiles(Integer userId) { | ||
| List<FileDTO> userFiles = fileRepository.getAllByOwner(userId).stream() | ||
| .map(FileDTO::fromFile) | ||
| .toList(); | ||
| return userFiles; | ||
| } | ||
|
|
||
| @Override | ||
| public void deleteAll() { | ||
| FileSystemUtils.deleteRecursively(fileLocation.toFile()); | ||
| } | ||
|
|
||
|
|
||
| private String sanitizeFilename(String originalFilename) { | ||
| if (originalFilename.contains("..") || originalFilename.contains("/") || originalFilename.contains("\\")) { | ||
| throw new IllegalArgumentException("Invalid filename: " + originalFilename); | ||
| } | ||
| return originalFilename.replaceAll("[^a-zA-Z0-9.-]", "_"); | ||
| } | ||
|
|
||
| private String generateStoredFilename(String sanitizedFilename) { | ||
| String fileExtension = sanitizedFilename.substring(sanitizedFilename.lastIndexOf(".")); | ||
| return UUID.randomUUID().toString() + fileExtension; | ||
| } | ||
| // END BRACKET | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
CSRF Protection is Disabled in Development Profile
The file upload service is vulnerable to CSRF attacks in the development environment as CSRF protection is explicitly disabled (csrf().disable()). While CSRF protection is properly enabled in the production profile using csrf(Customizer.withDefaults()), this creates a security configuration mismatch between environments that could lead to vulnerabilities being missed during testing.
src/main/java/org/fungover/system2024/config/SecurityConfig.java: Remove.csrf().disable()from the development profile and use the same CSRF configuration as production.
🔗 Analysis chain
Verify CSRF protection implementation
The PR objectives mention CSRF protection needs to be confirmed. Let's verify the security configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CSRF configuration
rg -l "csrf\(\)|CsrfConfigurer|@CrossOrigin"
# Search for security configuration classes
ast-grep --pattern 'class $_ extends WebSecurityConfigurerAdapter {
$$$
}'
Length of output: 190
Script:
#!/bin/bash
# Let's examine the SecurityConfig.java file to verify CSRF configuration
cat src/main/java/org/fungover/system2024/config/SecurityConfig.java
Length of output: 2221
| public FileSystemStorageService(FileRepository fileRepository, @Value("${storage.location}") String storageLocation) { | ||
| this.fileRepository = fileRepository; | ||
| this.fileLocation = Paths.get(storageLocation); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add storage location validation in constructor
The constructor should validate if the storage location exists and is writable. This prevents runtime issues when trying to store files.
public FileSystemStorageService(FileRepository fileRepository, @Value("${storage.location}") String storageLocation) {
this.fileRepository = fileRepository;
this.fileLocation = Paths.get(storageLocation);
+ try {
+ Files.createDirectories(this.fileLocation);
+ if (!Files.isWritable(this.fileLocation)) {
+ throw new StorageException("Storage location is not writable: " + storageLocation);
+ }
+ } catch (IOException e) {
+ throw new StorageException("Could not initialize storage location", e);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public FileSystemStorageService(FileRepository fileRepository, @Value("${storage.location}") String storageLocation) { | |
| this.fileRepository = fileRepository; | |
| this.fileLocation = Paths.get(storageLocation); | |
| } | |
| public FileSystemStorageService(FileRepository fileRepository, @Value("${storage.location}") String storageLocation) { | |
| this.fileRepository = fileRepository; | |
| this.fileLocation = Paths.get(storageLocation); | |
| try { | |
| Files.createDirectories(this.fileLocation); | |
| if (!Files.isWritable(this.fileLocation)) { | |
| throw new StorageException("Storage location is not writable: " + storageLocation); | |
| } | |
| } catch (IOException e) { | |
| throw new StorageException("Could not initialize storage location", e); | |
| } | |
| } |
| private String sanitizeFilename(String originalFilename) { | ||
| if (originalFilename.contains("..") || originalFilename.contains("/") || originalFilename.contains("\\")) { | ||
| throw new IllegalArgumentException("Invalid filename: " + originalFilename); | ||
| } | ||
| return originalFilename.replaceAll("[^a-zA-Z0-9.-]", "_"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen filename sanitization
The current sanitization allows dots which could still be used for path traversal in some cases. Consider using a whitelist approach instead.
private String sanitizeFilename(String originalFilename) {
- if (originalFilename.contains("..") || originalFilename.contains("/") || originalFilename.contains("\\")) {
+ if (originalFilename == null || originalFilename.trim().isEmpty()) {
throw new IllegalArgumentException("Invalid filename: " + originalFilename);
}
- return originalFilename.replaceAll("[^a-zA-Z0-9.-]", "_");
+ // Only allow alphanumeric, single dots, and common extensions
+ if (!originalFilename.matches("^[a-zA-Z0-9][a-zA-Z0-9._-]*\\.[a-zA-Z0-9]+$")) {
+ throw new IllegalArgumentException("Invalid filename format: " + originalFilename);
+ }
+ return originalFilename;
}Committable suggestion skipped: line range outside the PR's diff.
| // TODO: Change to auth userID | ||
| int userId = 123; |
There was a problem hiding this comment.
💡 Codebase verification
OAuth2 authentication is implemented but not utilized in FileSystemStorageService
The codebase has OAuth2 authentication implemented with GitHub (as seen in SecurityConfig), and the LoginController correctly retrieves user IDs from OAuth2User. However, the FileSystemStorageService is not utilizing this authentication mechanism. You should:
- Inject the authentication principal into the store method using
@AuthenticationPrincipal OAuth2User principal - Replace the hardcoded userId with
principal.getAttribute("id") - Add null checks for the principal to handle unauthenticated requests
🔗 Analysis chain
Replace hardcoded userId with proper authentication
The TODO comment indicates missing authentication. This is a security concern as it currently uses a hardcoded userId.
Let's verify if authentication is implemented elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for authentication implementation
rg -l "implements.*UserDetails|@AuthenticationPrincipal|SecurityContextHolder"
Length of output: 150
Script:
#!/bin/bash
# Let's check the LoginController implementation and related authentication code
rg -A 10 "implements.*UserDetails|@AuthenticationPrincipal|SecurityContextHolder" src/main/java/org/fungover/system2024/controller/LoginController.java
# Also search for any security configuration
fd -e java -x rg -l "SecurityConfig|WebSecurityConfigurerAdapter|SecurityFilterChain"
# Check if there's any user/authentication service
fd -e java -x rg -l "UserService|AuthenticationService|UserRepository"
# Look for JWT or session-based authentication
rg -l "JWT|JwtToken|@SessionAttributes"
Length of output: 1190
Script:
#!/bin/bash
# Let's check how the OAuth2User ID is used in the codebase
rg -A 5 "getAttribute\(\"id\"\)"
# Check the SecurityConfig implementation
cat src/main/java/org/fungover/system2024/config/SecurityConfig.java
# Check UserService implementation for any authentication-related methods
cat src/main/java/org/fungover/system2024/user/UserService.java
# Look for any Principal or OAuth2User usage in FileSystemStorageService
rg "Principal|OAuth2User" src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java
Length of output: 4048
Signed-off-by: Mats Johansson <matjoh50@gmail.com>
…file-upload-feature
…d Users entity. Started the "validation" process when uploading to later add on the auth security check for the correct users and go/no-go for uploads/downloads. Signed-off-by: Mats Johansson <matjoh50@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java (4)
44-48:⚠️ Potential issueInitialize storage location in constructor
The storage location should be validated during initialization to prevent runtime failures.
This issue was previously identified. The constructor should validate if the storage location exists and is writable.
80-91:⚠️ Potential issueImprove error handling and logging
The error handling needs improvement in several areas:
- Replace System.err with proper logging
- Add specific error messages
- Use try-with-resources correctly
This issue was previously identified. Replace System.err with proper logging using @slf4j.
185-190:⚠️ Potential issueStrengthen filename sanitization
The current sanitization logic is vulnerable to certain edge cases.
This issue was previously identified. Consider using a whitelist approach instead of the current blacklist approach.
192-195:⚠️ Potential issueAdd null and extension validation in generateStoredFilename
The method assumes the filename has an extension and doesn't handle edge cases.
This issue was previously identified. Add proper validation for filenames without extensions.
🧹 Nitpick comments (6)
src/main/java/org/fungover/system2024/file/FileRepository.java (1)
12-12: Add index hint for performance optimizationConsider adding
@Queryannotation with an index hint for thegetAllByOwnermethod to ensure optimal performance when querying files by owner.+ @Query(value = "SELECT f FROM File f USE INDEX (idx_owner_soft_delete) WHERE f.owner = :owner AND f.softDelete = false") List<File> getAllByOwner(User owner);src/main/java/org/fungover/system2024/file/FileDTO.java (1)
5-17: Add Javadoc documentationAdd documentation to explain the purpose of the DTO and its fields, especially the difference between originalName and storedFilename.
+/** + * Data Transfer Object for File entities. + * @param id The unique identifier of the file + * @param owner The ID of the user who owns the file + * @param originalName The original name of the uploaded file + * @param storedFilename The sanitized filename used for storage (UUID-based) + */ public record FileDTO(Integer id, Integer owner, String originalName, String storedFilename) {src/main/resources/file_schema.sql (2)
6-11: Optimize VARCHAR lengths and add filename indexThe VARCHAR(255) for filenames is unnecessarily large. Also, add an index for stored_filename lookups.
- original_filename VARCHAR(255) + original_filename VARCHAR(100) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL, - stored_filename VARCHAR(255) + stored_filename VARCHAR(40) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL, + INDEX idx_stored_filename (stored_filename),
18-25: Remove commented out schemaThe commented schema definition is redundant and could lead to confusion. Remove it to keep the file clean.
src/main/java/org/fungover/system2024/file/entity/File.java (2)
74-76: Improve ManyToOne relationship configurationThe ManyToOne relationship with User is correctly mapped, but consider adding fetch type configuration for better performance control.
@ManyToOne + @ManyToOne(fetch = FetchType.LAZY) @JoinColumn(name = "owner", referencedColumnName = "id", foreignKey = @ForeignKey(name = "fk_owner_id")) private User owner; // Changed from Integer to User
89-91: Review soft delete configurationThe soft delete implementation could be improved by:
- Using Hibernate's built-in soft delete support
- Adding an index for better query performance
- @ColumnDefault("0") - @Column(name = "soft_delete") - private Boolean softDelete = false; + @Column(name = "soft_delete") + @Index(name = "idx_soft_delete") + @Where(clause = "soft_delete = false") + private Boolean softDelete = false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/org/fungover/system2024/file/FileDTO.java(1 hunks)src/main/java/org/fungover/system2024/file/FileRepository.java(1 hunks)src/main/java/org/fungover/system2024/file/entity/File.java(2 hunks)src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java(1 hunks)src/main/resources/application.properties(1 hunks)src/main/resources/file_data.sql(1 hunks)src/main/resources/file_schema.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/file_data.sql
- src/main/resources/application.properties
🔇 Additional comments (4)
src/main/java/org/fungover/system2024/file/FileRepository.java (2)
14-14: Consider returning Optional for better null handling
The getFileById method should return Optional<File> to handle cases where the file is not found, consistent with Spring Data JPA best practices.
- File getFileById(Integer fileId);
+ Optional<File> getFileById(Integer fileId);16-18: Remove redundant method
getStoredFilenameById appears to duplicate the functionality of findStoredFilenameById, but without the null-safety of Optional. Consider removing it to avoid confusion.
src/main/java/org/fungover/system2024/file/FileDTO.java (1)
5-5: Add validation annotations for DTO fields
The DTO lacks input validation which could lead to invalid data being transferred.
src/main/resources/file_schema.sql (1)
5-5: 🛠️ Refactor suggestion
Make owner column NOT NULL and add index
The owner column should be NOT NULL since every file must have an owner. Also, add an index for better query performance.
- owner INT,
+ owner INT NOT NULL,
+ INDEX idx_owner_soft_delete (owner, soft_delete),Likely invalid or redundant comment.
| return new FileDTO( | ||
| file.getId(), | ||
| file.getOwner().getId(), | ||
| file.getOriginalFilename(), | ||
| file.getStoredFilename() | ||
| ); |
There was a problem hiding this comment.
Add null check for owner in fromFile method
The current implementation assumes file.getOwner() is non-null. Add a null check to prevent potential NullPointerException.
return new FileDTO(
file.getId(),
- file.getOwner().getId(),
+ Optional.ofNullable(file.getOwner())
+ .map(User::getId)
+ .orElseThrow(() -> new IllegalArgumentException("File owner is null")),
file.getOriginalFilename(),
file.getStoredFilename()
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new FileDTO( | |
| file.getId(), | |
| file.getOwner().getId(), | |
| file.getOriginalFilename(), | |
| file.getStoredFilename() | |
| ); | |
| return new FileDTO( | |
| file.getId(), | |
| Optional.ofNullable(file.getOwner()) | |
| .map(User::getId) | |
| .orElseThrow(() -> new IllegalArgumentException("File owner is null")), | |
| file.getOriginalFilename(), | |
| file.getStoredFilename() | |
| ); |
| // TODO: Change to auth userID | ||
| User activeuser; | ||
|
|
||
| activeuser = userRepository.findById(1).isPresent() ? userRepository.findById(1).get() : null; |
There was a problem hiding this comment.
Replace hardcoded user ID with proper authentication
Using a hardcoded user ID (1) is a security risk and bypasses authentication.
- // TODO: Change to auth userID
- User activeuser;
- activeuser = userRepository.findById(1).isPresent() ? userRepository.findById(1).get() : null;
+ Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
+ if (!(authentication.getPrincipal() instanceof OAuth2User)) {
+ throw new StorageException("User not authenticated");
+ }
+ OAuth2User oauth2User = (OAuth2User) authentication.getPrincipal();
+ Integer userId = oauth2User.getAttribute("id");
+ User activeuser = userRepository.findById(userId)
+ .orElseThrow(() -> new StorageException("User not found"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TODO: Change to auth userID | |
| User activeuser; | |
| activeuser = userRepository.findById(1).isPresent() ? userRepository.findById(1).get() : null; | |
| Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); | |
| if (!(authentication.getPrincipal() instanceof OAuth2User)) { | |
| throw new StorageException("User not authenticated"); | |
| } | |
| OAuth2User oauth2User = (OAuth2User) authentication.getPrincipal(); | |
| Integer userId = oauth2User.getAttribute("id"); | |
| User activeuser = userRepository.findById(userId) | |
| .orElseThrow(() -> new StorageException("User not found")); |
| public ResponseEntity<ByteArrayResource> loadAllResourseAsZip(List<Integer> fileIds) { | ||
| try (ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream(); | ||
| ZipOutputStream zipOutputStream = new ZipOutputStream(byteArrayOutputStream)) { | ||
|
|
||
| for (Integer fileId : fileIds) { | ||
| // Fetch file data | ||
| File fileData = fileRepository.getFileById(fileId); | ||
| if (fileData == null) { | ||
| continue; // Skip invalid file IDs | ||
| } | ||
|
|
||
| // Load file as Resource | ||
| Path filePath = load(fileData.getStoredFilename()); | ||
| Resource resource = loadAsResource(fileData.getStoredFilename()); | ||
|
|
||
| if (resource.exists() && resource.isReadable()) { | ||
| // Add file to ZIP | ||
| zipOutputStream.putNextEntry(new ZipEntry(fileData.getOriginalFilename())); | ||
| Files.copy(filePath, zipOutputStream); | ||
| zipOutputStream.closeEntry(); | ||
| } | ||
| } | ||
|
|
||
| zipOutputStream.finish(); | ||
|
|
||
| // Return the ZIP file as a ResponseEntity | ||
| ByteArrayResource zipResource = new ByteArrayResource(byteArrayOutputStream.toByteArray()); | ||
|
|
||
| return ResponseEntity.ok() | ||
| .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"files.zip\"") | ||
| .body(zipResource); | ||
|
|
||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| return ResponseEntity.internalServerError().build(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve ZIP file generation security and resource handling
The ZIP file generation needs several improvements:
- Add maximum file size limit
- Validate total ZIP size
- Add proper resource cleanup
@Override
public ResponseEntity<ByteArrayResource> loadAllResourseAsZip(List<Integer> fileIds) {
+ long totalSize = 0;
+ final long MAX_ZIP_SIZE = 100 * 1024 * 1024; // 100MB limit
try (ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
ZipOutputStream zipOutputStream = new ZipOutputStream(byteArrayOutputStream)) {
for (Integer fileId : fileIds) {
File fileData = fileRepository.getFileById(fileId);
if (fileData == null) {
continue;
}
Path filePath = load(fileData.getStoredFilename());
+ if (Files.size(filePath) > MAX_ZIP_SIZE) {
+ throw new StorageException("File too large");
+ }
+ totalSize += Files.size(filePath);
+ if (totalSize > MAX_ZIP_SIZE) {
+ throw new StorageException("Total ZIP size exceeds limit");
+ }
Resource resource = loadAsResource(fileData.getStoredFilename());
if (resource.exists() && resource.isReadable()) {
zipOutputStream.putNextEntry(new ZipEntry(fileData.getOriginalFilename()));
Files.copy(filePath, zipOutputStream);
zipOutputStream.closeEntry();
}
}Committable suggestion skipped: line range outside the PR's diff.
maxlin94
left a comment
maxlin94
left a comment
There was a problem hiding this comment.
The build seems to fail because of
src/components/common/Header.tsx(4,1): error TS6133: 'MenuItemsMobile' is declared but its value is never read.
src/i18n.ts(3,21): error TS2307: Cannot find module 'i18next-http-backend' or its corresponding type declarations.
which should be fixed in branch 89-main-branch-fails-to-build. If you merge with that and fix the SonarCloud issues and look at what coderabbit has to say it should be good to go. Good job 👍
… to see if working as intended) also some small cleanup in Header where there where double renderd menu and lang options. Signed-off-by: Mats Johansson <matjoh50@gmail.com>
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
frontend/src/components/common/MenuItems.tsx (1)
1-4: Introduce a naming convention for types and interfaces [optional].
The newly introducedtype MenuItemandinterface MenuItemsappear correct, but some teams prefer a convention likeIMenuItemfor interfaces. If you have a standard naming convention in place, aligning with it can improve code consistency.frontend/package.json (1)
18-18: Proceed with configuring i18next HTTP backend.
The newly added dependencyi18next-http-backendcan facilitate dynamic loading of translations. Ensure you configure it properly in your i18n setup so that it effectively fetches the translation files, and verify that no unnecessary overhead or misuse of network calls occurs.src/main/java/org/fungover/system2024/fileupload/StorageService.java (1)
14-34: Add interface documentationThe interface methods would benefit from JavaDoc documentation describing their purpose, parameters, return values, and potential exceptions.
src/main/java/org/fungover/system2024/fileupload/FileStorageController.java (1)
18-20: Use constructor injection instead of field injectionField injection with @Autowired is discouraged. The class already has a constructor, so remove the @Autowired annotation.
- @Autowired - StorageService storageService;src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java (1)
136-176: Remove commented-out codeLarge blocks of commented-out code should be removed. If this code is needed for reference, it should be tracked in version control instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
.gitignore(2 hunks)frontend/package.json(1 hunks)frontend/src/App.tsx(1 hunks)frontend/src/components/common/Header.tsx(0 hunks)frontend/src/components/common/MenuItems.tsx(2 hunks)readme.md(0 hunks)src/main/java/org/fungover/system2024/config/SecurityConfig.java(1 hunks)src/main/java/org/fungover/system2024/controller/LoginController.java(1 hunks)src/main/java/org/fungover/system2024/fileupload/FileStorageController.java(1 hunks)src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java(1 hunks)src/main/java/org/fungover/system2024/fileupload/StorageService.java(1 hunks)
💤 Files with no reviewable changes (2)
- readme.md
- frontend/src/components/common/Header.tsx
✅ Files skipped from review due to trivial changes (2)
- src/main/java/org/fungover/system2024/controller/LoginController.java
- frontend/src/App.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- src/main/java/org/fungover/system2024/config/SecurityConfig.java
🧰 Additional context used
🪛 GitHub Check: SonarCloud
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java
[notice] 214-215: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
🔇 Additional comments (5)
frontend/src/components/common/MenuItems.tsx (2)
10-10: Validate final layout after adding the left margin.
The extra left margin (ml-8) can shift your navigation bar layout. Confirm there are no unwanted side effects in different viewport sizes, ensuring the navbar remains visually coherent.
18-18: Indentation change looks fine.
The indentation adjustment doesn’t alter functionality. This is purely aesthetic and doesn’t introduce any issues.
src/main/java/org/fungover/system2024/fileupload/FileStorageController.java (1)
46-59: 🛠️ Refactor suggestion
Fix typo and improve error handling in file upload
The error message contains a typo ("Filed" instead of "Failed"), and the error handling can be improved.
public ResponseEntity<String> handleMultiFileUpload(@RequestParam("files") List<MultipartFile> files) {
try {
for (MultipartFile file : files) {
if (file.isEmpty()) {
- return ResponseEntity.badRequest().body("Filed to store empty file");
+ return ResponseEntity.badRequest().body("Failed to store empty file");
}
}
storageService.store(files);
return ResponseEntity.ok().body("All files uploaded successfully\n");
} catch (Exception e) {
- e.printStackTrace();
+ log.error("Error while uploading files", e);
return ResponseEntity.badRequest().body("Error while uploading files\n");
}
}Likely invalid or redundant comment.
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java (2)
47-51: 🛠️ Refactor suggestion
Add storage location validation in constructor
The constructor should validate if the storage location exists and is writable.
public FileSystemStorageService(FileRepository fileRepository, @Value("${storage.location}") String storageLocation, UserRepository userRepository) {
this.fileRepository = fileRepository;
this.fileLocation = Paths.get(storageLocation);
this.userRepository = userRepository;
+ try {
+ Files.createDirectories(this.fileLocation);
+ if (!Files.isWritable(this.fileLocation)) {
+ throw new StorageException("Storage location is not writable: " + storageLocation);
+ }
+ } catch (IOException e) {
+ throw new StorageException("Could not initialize storage location", e);
+ }
}Likely invalid or redundant comment.
291-294:
Add null and format checks in generateStoredFilename
The method assumes the filename has an extension and doesn't handle edge cases.
private String generateStoredFilename(String sanitizedFilename) {
- String fileExtension = sanitizedFilename.substring(sanitizedFilename.lastIndexOf("."));
+ String fileExtension = "";
+ int lastDotIndex = sanitizedFilename.lastIndexOf(".");
+ if (lastDotIndex > 0) {
+ fileExtension = sanitizedFilename.substring(lastDotIndex);
+ }
return UUID.randomUUID().toString() + fileExtension;
}Likely invalid or redundant comment.
| public ResponseEntity getAllUsersFileNames(@PathVariable("id") Integer userId) { | ||
| System.out.println(userId); | ||
| List<FileDTO> listOfNames = storageService.getListOfFiles(); | ||
| return ResponseEntity.ok(listOfNames); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace System.out with proper logging
Using System.out.println is not suitable for production code. Use a proper logging framework instead.
- System.out.println(userId);
+ log.debug("Fetching files for user: {}", userId);Add these imports:
import lombok.extern.slf4j.Slf4j;
@Slf4j
@Controller| // TODO: Download function, | ||
| // * needs to add security check of user | ||
| // * change to POST request with List to have better UI |
There was a problem hiding this comment.
💡 Codebase verification
Security measures required for file operations
The review comment is valid. The current implementation has several security concerns:
- The
FileStorageControllerlacks proper authorization checks for file operations:/api/files/list/{id}endpoint allows accessing any user's files by ID without verification/api/files/fetch/endpoint has no user-based access control- File upload endpoint
/api/files/is unprotected
While there is a SecurityConfig in place:
- Development profile permits all requests (
permitAll()) - Production profile has basic authentication (
authenticated()), but lacks fine-grained authorization
The TODO comments correctly identify the need for user-specific security checks to prevent unauthorized access to files.
🔗 Analysis chain
Address security TODO comments
The TODO comments indicate missing security checks. This is a critical security concern that needs to be addressed before merging.
Let me check the security configuration to better understand the current security implementation.
Let me check the FileStorageController to understand its current security implementation and endpoints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing security implementations
rg -l "SecurityConfig|@PreAuthorize|@Secured"
Length of output: 112
Script:
#!/bin/bash
# Check the content of SecurityConfig.java and look for any file upload related security configurations
cat src/main/java/org/fungover/system2024/config/SecurityConfig.java
Length of output: 2221
Script:
#!/bin/bash
# Check the content of FileStorageController.java
cat src/main/java/org/fungover/system2024/fileupload/FileStorageController.java
Length of output: 2111
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java
Show resolved
Hide resolved
| try (ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream(); | ||
| ZipOutputStream zipOutputStream = new ZipOutputStream(byteArrayOutputStream)) { | ||
|
|
||
| for (Integer fileId : fileIds) { | ||
| // Fetch file data | ||
| File fileData = fileRepository.getFileById(fileId); | ||
| if (fileData == null) { | ||
| Logger.getLogger(getClass().getName()) | ||
| .log(Level.WARNING, "Invalid file ID: {0}", fileId); | ||
| continue; // Skip invalid file IDs | ||
| } | ||
|
|
||
| // Load file as Resource | ||
| Path filePath = load(fileData.getStoredFilename()); | ||
| Resource resource = loadAsResource(fileData.getStoredFilename()); | ||
|
|
||
| if (resource.exists() && resource.isReadable()) { | ||
| // Add file to ZIP | ||
| zipOutputStream.putNextEntry(new ZipEntry(fileData.getOriginalFilename())); | ||
| Files.copy(filePath, zipOutputStream); | ||
| zipOutputStream.closeEntry(); | ||
| } else { | ||
| Logger.getLogger(getClass().getName()) | ||
| .log(Level.WARNING, "File not readable or does not exist: {0}", fileData.getStoredFilename()); | ||
| } | ||
| } | ||
|
|
||
| zipOutputStream.finish(); | ||
|
|
||
| // Return the ZIP file as a ResponseEntity | ||
| ByteArrayResource zipResource = new ByteArrayResource(byteArrayOutputStream.toByteArray()); | ||
|
|
||
| return ResponseEntity.ok() | ||
| .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"files.zip\"") | ||
| .body(zipResource); | ||
|
|
||
| } catch (IOException e) { | ||
| Logger logger = Logger.getLogger(getClass().getName()); | ||
| logger.log(Level.SEVERE, "An error occurred while creating the ZIP file", e); | ||
|
|
||
| return ResponseEntity.internalServerError().build(); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the gathered information, I can now provide the final response:
Add file size limits and implement memory-safe ZIP generation
The current implementation lacks crucial size limits and memory safeguards:
- No maximum file size configuration for uploads (spring.servlet.multipart.max-file-size)
- No total request size limit (spring.servlet.multipart.max-request-size)
- No limit on the number of files that can be zipped or the final ZIP size
- Uses ByteArrayOutputStream which keeps all data in memory
Required changes:
- Add the following to
application.properties:spring.servlet.multipart.max-file-size=10MB spring.servlet.multipart.max-request-size=10MB app.zip.max-total-size=50MB app.zip.max-files=10
- Replace ByteArrayOutputStream with a temporary file-based solution for ZIP generation
- Add size checks before and during ZIP creation to prevent memory exhaustion
🔗 Analysis chain
Add ZIP file size limits and improve resource handling
The ZIP file generation needs size limits and better resource handling to prevent potential DOS attacks.
Let me gather more information about the file size configurations and resource handling.
Let me check the application properties file for any existing configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any existing file size configurations
rg -l "maxFileSize|MAX_FILE_SIZE|MultipartConfigElement"
Length of output: 96
Script:
#!/bin/bash
# Check the content of the logback configuration and application properties for size limits
cat src/main/resources/logback-spring.xml
echo "---"
# Check for application.properties/yml files
fd -e properties -e yml "application"
Length of output: 1742
Script:
#!/bin/bash
# Look for any upload or file size related configurations in the codebase
rg -A 3 "multipart|file.size|maxFileSize|maxRequestSize|spring.servlet"
Length of output: 1675
Script:
#!/bin/bash
# Check for any resource cleanup or close operations in the service
ast-grep --pattern 'try ($$$) { $$$ }'
Length of output: 7219
Script:
#!/bin/bash
# Look for any existing size validation or checks
rg -A 3 "getSize|length|bytes|fileSize"
Length of output: 5432
Script:
#!/bin/bash
# Check application.properties content
cat src/main/resources/application.properties
Length of output: 1892
Script:
#!/bin/bash
# Look for any file size checks in the controller
ast-grep --pattern 'class FileStorageController { $$$ }'
Length of output: 58
🧰 Tools
🪛 GitHub Check: SonarCloud
[notice] 214-215: Logging should not be vulnerable to injection attacks
Change this code to not log user-controlled data.See more on SonarQube Cloud
…rageService.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java (3)
10-10: Remove unused importThe
StorageAccessimport is not used in the code.-import org.hibernate.cache.spi.support.StorageAccess;
136-176: Remove commented-out codeThe old implementation should be removed as it's no longer needed and maintained in version control.
274-276: Optimize database queriesThe code makes two identical queries to get user files. The result from line 269 can be reused.
- return fileRepository.getAllByOwner(activeUser).stream() + return userFiles.stream() .map(FileDTO::fromFile) .toList();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java(1 hunks)
🔇 Additional comments (6)
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java (6)
47-51: Add storage location validation in constructor
The constructor should validate if the storage location exists and is writable.
97-98: Replace System.err with proper logging
207-249: Add file size limits and improve ZIP generation security
214-216: Fix potential log injection vulnerability
285-290: Strengthen filename sanitization
293-294: Validate extension extraction for files without '.'
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java
Outdated
Show resolved
Hide resolved
…rageService.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java(1 hunks)
🔇 Additional comments (6)
src/main/java/org/fungover/system2024/fileupload/FileSystemStorageService.java (6)
47-51: Add storage location validation in constructor
The constructor should validate if the storage location exists and is writable to prevent runtime issues.
108-134: LGTM! Resource loading implementation is solid
The implementation properly handles file loading and resource management with appropriate error handling.
178-250: Implement memory-safe ZIP generation with size limits
The current implementation uses ByteArrayOutputStream which keeps all data in memory. This could lead to OutOfMemoryError for large files or many files.
254-277: LGTM! File listing implementation is well-structured
The implementation includes proper authentication, error handling, and efficient stream operations.
285-290: Strengthen filename sanitization
The current sanitization allows dots which could still be used for path traversal in some cases.
293-294: Validate extension extraction for files without '.'
The current implementation assumes the filename has an extension, which could cause an IndexOutOfBoundsException.
| public void store(List<MultipartFile> files) { | ||
| try { | ||
| for (MultipartFile file : files) { | ||
| if (file.isEmpty()) { | ||
| throw new StorageException("Filed to store empty file"); | ||
| } | ||
| String sanitizedFilename = sanitizeFilename(Objects.requireNonNull(file.getOriginalFilename())); | ||
| String storedFilename = generateStoredFilename(sanitizedFilename); | ||
|
|
||
| Path destinationFile = fileLocation.resolve( | ||
| Paths.get(Objects.requireNonNull(storedFilename))) | ||
| .normalize().toAbsolutePath(); | ||
|
|
||
| if (!destinationFile.getParent().equals(fileLocation.toAbsolutePath())) { | ||
| //Security check | ||
| throw new StorageException("Cannot store file outside current directory"); | ||
| } | ||
|
|
||
| Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); | ||
| if (!(authentication.getPrincipal() instanceof OAuth2User oauth2User)) { | ||
| throw new StorageException("User not authenticated"); | ||
| } | ||
| Integer userId = oauth2User.getAttribute("id"); | ||
| assert userId != null; | ||
| User activeuser = userRepository.findById(userId) | ||
| .orElseThrow(() -> new StorageException("User not found")); | ||
|
|
||
| try (InputStream inputStream = file.getInputStream()) { | ||
| Files.copy(inputStream, destinationFile, StandardCopyOption.REPLACE_EXISTING); | ||
| // Save metadata to the database | ||
| File metadata = new File(); | ||
| metadata.setOwner(activeuser); | ||
| metadata.setOriginalFilename(file.getOriginalFilename()); | ||
| metadata.setStoredFilename(storedFilename); | ||
| fileRepository.save(metadata); | ||
| } catch (IOException e) { | ||
| System.err.println("File upload failed: " + e.getMessage()); | ||
| throw e; // Rethrow the exception or handle it as necessary | ||
| } | ||
| } | ||
|
|
||
| } catch (IOException e) { | ||
| throw new StorageException("Cannot store file: " + e.getMessage().replaceAll("[\\r\\n]", "")); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add file size validation
The store method should validate the file size before processing to prevent potential DOS attacks and resource exhaustion.
@Override
public void store(List<MultipartFile> files) {
+ final long MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB limit
try {
for (MultipartFile file : files) {
if (file.isEmpty()) {
throw new StorageException("Filed to store empty file");
}
+ if (file.getSize() > MAX_FILE_SIZE) {
+ throw new StorageException("File size exceeds maximum limit of 10MB");
+ }
String sanitizedFilename = sanitizeFilename(Objects.requireNonNull(file.getOriginalFilename()));Committable suggestion skipped: line range outside the PR's diff.
Signed-off-by: Mats Johansson <matjoh50@gmail.com>
…tests passes. Signed-off-by: Mats Johansson <matjoh50@gmail.com>
Signed-off-by: Mats Johansson <matjoh50@gmail.com>
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/resources/ftp/file_data.backup (1)
1-4: Use neutral or officially approved sample data for demonstration.
The inserted records reference what appear to be "Pirated" assets. This might raise concerns or confusion in a production environment or public repository. Consider replacing them with neutral or generic file names such as "SampleVideo" or "DemoFile" to reflect best practices.src/main/resources/ftp/file_schema.backup (1)
1-16: Consider adding ON DELETE CASCADE for referential integrity.
When a user referenced byowneris removed from theusertable, the associated rows in thefiletable could become orphaned. EnablingON DELETE CASCADE(or handling such deletions via application logic) helps maintain data integrity.src/main/resources/data.sql (1)
45-48: Use consistent test data naming with production usage.
Reiterating the earlier point, referencing "Pirated Movie" or "Pirated Game" in a shared environment may cause misunderstandings. Consider using neutral examples to avoid confusion or potential policy concerns.src/main/resources/schema.sql (1)
45-59: Consider additional constraints and indexes for improved data integrity and performanceThe table structure is good but could benefit from the following improvements:
- Add indexes for frequently queried columns (owner, soft_delete)
- Consider adding constraints:
- NOT NULL for owner column
- File size limits
- Created/updated timestamps for audit purposes
Here's a suggested enhancement:
CREATE TABLE IF NOT EXISTS file( id INT AUTO_INCREMENT PRIMARY KEY, - owner INT, + owner INT NOT NULL, original_filename VARCHAR(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL, stored_filename VARCHAR(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL, soft_delete BOOLEAN DEFAULT 0, + file_size BIGINT NOT NULL, + created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, CONSTRAINT fk_owner_id FOREIGN KEY (owner) REFERENCES user(id) + INDEX idx_owner_soft_delete (owner, soft_delete) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/resources/application.properties(1 hunks)src/main/resources/data.sql(1 hunks)src/main/resources/ftp/file_data.backup(1 hunks)src/main/resources/ftp/file_schema.backup(1 hunks)src/main/resources/schema.sql(1 hunks)src/test/java/org/fungover/system2024/System2024ApplicationTests.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/application.properties
🔇 Additional comments (3)
src/test/java/org/fungover/system2024/System2024ApplicationTests.java (1)
12-12: Confirm alignment of the new init script.
The .withInitScript("schema.sql") directive ensures the database container applies the schema from schema.sql. Verify this file includes any recent table additions (including the new file table) to avoid test failures.
src/main/resources/schema.sql (2)
49-54: Verify filename length constraints for security measures
The VARCHAR(255) length for stored_filename needs to be sufficient for:
- UUID-based filenames (typically 36 characters)
- File extensions
- Potential path segments
Also, consider adding a CHECK constraint or trigger to validate filename patterns if your database supports it.
Let's check how filenames are handled in the code:
✅ Verification successful
Let's examine the implementation details of these files to understand how filenames are handled.
VARCHAR(255) length is sufficient for stored filenames
Based on the implementation in FileSystemStorageService.java, the stored filename format is:
- UUID (36 characters)
- File extension (preserved from original file)
- No path segments (explicitly prevented by sanitization)
The current implementation ensures:
- Filenames are sanitized by removing unsafe characters
- Path traversal is prevented by rejecting
..,/, and\ - Stored filenames are generated using UUID + original extension
- Additional path security check prevents storing files outside the target directory
The VARCHAR(255) length is more than adequate as the maximum length would be approximately:
- 36 chars (UUID)
- 1 char (dot)
- Few chars for extension
No additional database constraints are necessary as the application layer implements robust filename validation and sanitization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for filename handling logic
rg -l "UUID|sanitize|stored_filename" --type java
Length of output: 190
Script:
#!/bin/bash
# Check the implementation of filename handling
rg -A 10 "UUID|sanitize|stored_filename" --type java
Length of output: 4759
44-44: Consider data preservation strategy for schema updates
The unconditional DROP TABLE statement could lead to data loss in production environments. Consider implementing proper database migrations using tools like Flyway or Liquibase to manage schema changes safely.
Let's check if there are any migration tools already in use:
✅ Verification successful
DROP TABLE statement is appropriate in this context
Based on the analysis of the schema files and application configuration:
- The application uses
spring.sql.init.mode=always, which means the schema is reinitialized on every application startup - The schema follows a "create if not exists" pattern for all tables except the file table
- The file table has a test version in
file_test_schema.sql, suggesting this is a development/testing environment - No production migration tools are in use, indicating this is likely a development setup
The DROP TABLE statement is consistent with the current development setup where the schema is meant to be recreated on startup. However, before deploying to production:
- Implement proper database migrations
- Remove the
spring.sql.init.mode=alwaysconfiguration - Version the schema changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for migration tool configurations
fd -e xml pom.xml -x grep -l "flyway|liquibase" {} \;
fd -e gradle build.gradle -x grep -l "flyway|liquibase" {} \;
Length of output: 125
Script:
#!/bin/bash
# Let's check for any migration-related files or configurations
fd -t f "migration|flyway|liquibase|changelog|schema" --ignore-case
# Also check for Spring Boot application properties that might configure migrations
fd -e properties -e yml -e yaml -x grep -l "spring.jpa.hibernate.ddl-auto|flyway|liquibase" {} \;
# Look for any existing schema versioning or migration patterns
fd -e sql | grep -i "v[0-9].*\.sql\|migration"
Length of output: 376
Script:
#!/bin/bash
# Let's check the content of schema-related files to understand the current setup
echo "=== Main schema.sql ==="
cat src/main/resources/schema.sql
echo -e "\n=== Test schema ==="
cat src/main/resources/file_test_schema.sql
# Check application properties for database configuration
echo -e "\n=== Application Properties ==="
fd -e properties -e yml -e yaml application -x cat {} \;
Length of output: 3986
AlexanderK88
left a comment
AlexanderK88
left a comment
There was a problem hiding this comment.
Great work on implementing the file management system! The functionality is well-structured and provides a strong base. A few suggestions to enhance it further:
-
Security: Add stricter user-based access control to endpoints like
/list/{id}and/fetch/, and validate file size and type during uploads for added protection. -
Logging: Replace
System.out.printlnwith a logging framework like SLF4J for better production readiness. -
Code Improvements: Use
Optional<File>in repository methods likegetFileByIdfor null safety, and consider standardizing dependency injection for consistency.
This is a solid implementation overall, and these enhancements will make it even stronger for production! Great job so far!
There was a problem hiding this comment.
The commented code can be removed if not needed.
|
|
||
|
|
||
| @PostMapping("/") | ||
| public ResponseEntity<String> handleMultiFileUpload(@RequestParam("files") List<MultipartFile> files) { |
There was a problem hiding this comment.
Suggest that the validation and storing of files are moved to the FileSystemStorageService class. That can make the code easier to read and maintain.
|
|
||
|
|
||
| @Override | ||
| public void store(List<MultipartFile> files) { |
There was a problem hiding this comment.
Suggest to extract suitable parts in store to make the code easier to read and maintain. That might also lead to no need for the explaining comments in code.
|
|
||
|
|
||
| @Override | ||
| public ResponseEntity<ByteArrayResource> loadAllResourcesAsZip(List<Integer> fileIds) { |
There was a problem hiding this comment.
Suggest to extract suitable parts in loadAllResourcesAsZipt to make the code easier to read and maintain.
5 participants
This PR includes endpoints for file upload, download and list for userId owned files.
Upoading files involves of cleaning up from special characters to lower the risk of SQL injections. after that the filenme is set to UUID to make each file more anonumus.
The download API is set to download using .zip file thus the requst will always be a post request nomatter the amount of files to download.
things to todo:
Include OAuth for unique users.
confirm API function with CSRF implementation.
Summary by CodeRabbit
New Features
Security Changes
Configuration Updates
Frontend Modifications
Database Changes