add FileUpload.java #2
Conversation
Summary by CodeRabbit
Walkthrough该次提交在 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as 用户
participant Controller as FileUpload 控制器
participant Validator as 文件验证器
participant Storage as 文件存储
participant Logger as 日志记录器
Client->>Controller: 请求上传页面 (普通/图片)
alt 普通文件上传
Controller->>Validator: 检查文件是否为空
alt 文件为空
Controller->>Client: 返回错误提示
else
Controller->>Storage: 保存文件
Controller->>Logger: 记录上传成功
Controller->>Client: 返回上传成功信息
end
else 图片上传
Controller->>Validator: 检查扩展名、MIME 及图片内容
alt 验证失败
Controller->>Storage: 删除错误文件
Controller->>Client: 返回错误信息
else
Controller->>Storage: 保存图片文件
Controller->>Logger: 记录上传成功
Controller->>Client: 返回成功信息
end
end
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/main/java/org/joychou/controller/FileUpload2.java (2)
38-42: 方法命名与描述存在拼写错误。
注释“uplaod any file”拼写应为“upload”。同时方法index()返回“upload”视图，命名含义不够直观，可能改为更具描述性的名称，例如“uploadAnyFilePage”。
191-197: 图片校验逻辑通过ImageIO.read()判断是否为图片，思路可行，但仍需注意恶意文件伪造风险。
若攻击者提供的文件部分头部信息恰好能通过ImageIO.read()解析，仍可能常规通过；建议配合更严格的签名解析或额外安全校验。您需要更严谨的图片内容检测吗？如有需要，我可以协助编写更高强度的检测逻辑或帮助打开新 issue。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/org/joychou/controller/FileUpload2.java(1 hunks)
🔇 Additional comments (1)
src/main/java/org/joychou/controller/FileUpload2.java (1)
1-2: 类命名与功能保持一致，整体无大问题。
此处仅是包名与类定义的起始部分，看起来合理，未发现语法或逻辑错误。
| public class FileUpload { | ||
|
|
||
| // Save the uploaded file to this folder | ||
| private static final String UPLOADED_FOLDER = "/tmp/"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
硬编码上传目录可能会带来安全与灵活性问题。
建议通过配置文件或环境变量来管理上传目录，避免在生产环境中直接写死“/tmp/”路径，并确认文件权限。
| @PostMapping("/upload") | ||
| public String singleFileUpload(@RequestParam("file") MultipartFile file, | ||
| RedirectAttributes redirectAttributes) { | ||
| if (file.isEmpty()) { | ||
| // 赋值给uploadStatus.html里的动态参数message | ||
| redirectAttributes.addFlashAttribute("message", "Please select a file to upload"); | ||
| return "redirect:/file/status"; | ||
| } | ||
|
|
||
| try { | ||
| // Get the file and save it somewhere | ||
| byte[] bytes = file.getBytes(); | ||
| Path path = Paths.get(UPLOADED_FOLDER + file.getOriginalFilename()); | ||
| Files.write(path, bytes); | ||
|
|
||
| redirectAttributes.addFlashAttribute("message", | ||
| "You successfully uploaded '" + UPLOADED_FOLDER + file.getOriginalFilename() + "'"); | ||
|
|
||
| } catch (IOException e) { | ||
| redirectAttributes.addFlashAttribute("message", "upload failed"); | ||
| logger.error(e.toString()); | ||
| } | ||
|
|
||
| return "redirect:/file/status"; | ||
| } |
There was a problem hiding this comment.
文件名直接拼接至路径存在潜在路径穿越风险。
当前未对 file.getOriginalFilename() 做目录穿越检测，若上传文件名包含 ../ 等路径可导致覆盖任意文件。建议使用安全的文件名过滤或自动生成随机文件名，避免产生安全隐患。
| // only upload picture | ||
| @PostMapping("/upload/picture") | ||
| @ResponseBody | ||
| public String uploadPicture(@RequestParam("file") MultipartFile multifile) throws Exception { | ||
| if (multifile.isEmpty()) { | ||
| return "Please select a file to upload"; | ||
| } | ||
|
|
||
| String fileName = multifile.getOriginalFilename(); | ||
| String Suffix = fileName.substring(fileName.lastIndexOf(".")); // 获取文件后缀名 | ||
| String mimeType = multifile.getContentType(); // 获取MIME类型 | ||
| String filePath = UPLOADED_FOLDER + fileName; | ||
| File excelFile = convert(multifile); | ||
|
|
||
|
|
||
| // 判断文件后缀名是否在白名单内 校验1 | ||
| String[] picSuffixList = {".jpg", ".png", ".jpeg", ".gif", ".bmp", ".ico"}; | ||
| boolean suffixFlag = false; | ||
| for (String white_suffix : picSuffixList) { | ||
| if (Suffix.toLowerCase().equals(white_suffix)) { | ||
| suffixFlag = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!suffixFlag) { | ||
| logger.error("[-] Suffix error: " + Suffix); | ||
| deleteFile(filePath); | ||
| return "Upload failed. Illeagl picture."; | ||
| } | ||
|
|
||
|
|
||
| // 判断MIME类型是否在黑名单内 校验2 | ||
| String[] mimeTypeBlackList = { | ||
| "text/html", | ||
| "text/javascript", | ||
| "application/javascript", | ||
| "application/ecmascript", | ||
| "text/xml", | ||
| "application/xml" | ||
| }; | ||
| for (String blackMimeType : mimeTypeBlackList) { | ||
| // 用contains是为了防止text/html;charset=UTF-8绕过 | ||
| if (SecurityUtil.replaceSpecialStr(mimeType).toLowerCase().contains(blackMimeType)) { | ||
| logger.error("[-] Mime type error: " + mimeType); | ||
| deleteFile(filePath); | ||
| return "Upload failed. Illeagl picture."; | ||
| } | ||
| } | ||
|
|
||
| // 判断文件内容是否是图片 校验3 | ||
| boolean isImageFlag = isImage(excelFile); | ||
| deleteFile(randomFilePath); | ||
|
|
||
| if (!isImageFlag) { | ||
| logger.error("[-] File is not Image"); | ||
| deleteFile(filePath); | ||
| return "Upload failed. Illeagl picture."; | ||
| } | ||
|
|
||
|
|
||
| try { | ||
| // Get the file and save it somewhere | ||
| byte[] bytes = multifile.getBytes(); | ||
| Path path = Paths.get(UPLOADED_FOLDER + multifile.getOriginalFilename()); | ||
| Files.write(path, bytes); | ||
| } catch (IOException e) { | ||
| logger.error(e.toString()); | ||
| deleteFile(filePath); | ||
| return "Upload failed"; | ||
| } | ||
|
|
||
| logger.info("[+] Safe file. Suffix: {}, MIME: {}", Suffix, mimeType); | ||
| logger.info("[+] Successfully uploaded {}", filePath); | ||
| return String.format("You successfully uploaded '%s'", filePath); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
图片上传检查逻辑较完善，但以下细节可进一步增强安全性与可维护性：
- 变量命名不当：
excelFile命名与图片类型不符，会引起混淆。 - 存在多处返回“Illeagl picture.”的字符串，单词拼写应为“Illegal”。
SecurityUtil.replaceSpecialStr()的实现细节未知，需确保其不会误删关键字符或弱化安全校验。- 使用静态成员
randomFilePath保存动态文件路径容易出现并发覆盖问题，多线程下存在 race condition 风险。
| private void deleteFile(String filePath) { | ||
| File delFile = new File(filePath); | ||
| if(delFile.isFile() && delFile.exists()) { | ||
| if (delFile.delete()) { | ||
| logger.info("[+] " + filePath + " delete successfully!"); | ||
| return; | ||
| } | ||
| } | ||
| logger.info(filePath + " delete failed!"); | ||
| } |
There was a problem hiding this comment.
缺少对删除文件操作的更多限制与验证。
deleteFile() 方法未校验传入路径是否在指定上传目录内；可能导致删除系统任意文件。建议增加目录约束或白名单校验。
| /** | ||
| * 为了使用ImageIO.read() | ||
| * | ||
| * 不建议使用transferTo，因为原始的MultipartFile会被覆盖 | ||
| * https://stackoverflow.com/questions/24339990/how-to-convert-a-multipart-file-to-file | ||
| */ | ||
| private File convert(MultipartFile multiFile) throws Exception { | ||
| String fileName = multiFile.getOriginalFilename(); | ||
| String suffix = fileName.substring(fileName.lastIndexOf(".")); | ||
| UUID uuid = Generators.timeBasedGenerator().generate(); | ||
| randomFilePath = UPLOADED_FOLDER + uuid + suffix; | ||
| // 随机生成一个同后缀名的文件 | ||
| File convFile = new File(randomFilePath); | ||
| boolean ret = convFile.createNewFile(); | ||
| if (!ret) { | ||
| return null; | ||
| } | ||
| FileOutputStream fos = new FileOutputStream(convFile); | ||
| fos.write(multiFile.getBytes()); | ||
| fos.close(); | ||
| return convFile; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
convert 方法中缺少文件大小限制及异常处理机制。
在将 MultipartFile 写入磁盘时，若文件过大或写入受限，可能出现资源耗尽或异常未捕获的问题。使用 try-with-resources 可确保 FileOutputStream 正常关闭。
|
| // Get the file and save it somewhere | ||
| byte[] bytes = file.getBytes(); | ||
| Path path = Paths.get(UPLOADED_FOLDER + file.getOriginalFilename()); | ||
| Files.write(path, bytes); |
Check failure
Code scanning / SonarCloud
I/O function calls should not be vulnerable to path injection attacks High
| } | ||
| } | ||
| if (!suffixFlag) { | ||
| logger.error("[-] Suffix error: " + Suffix); |
Check notice
Code scanning / SonarCloud
Logging should not be vulnerable to injection attacks Low
| // Get the file and save it somewhere | ||
| byte[] bytes = multifile.getBytes(); | ||
| Path path = Paths.get(UPLOADED_FOLDER + multifile.getOriginalFilename()); | ||
| Files.write(path, bytes); |
Check failure
Code scanning / SonarCloud
I/O function calls should not be vulnerable to path injection attacks High
| return "Upload failed"; | ||
| } | ||
|
|
||
| logger.info("[+] Safe file. Suffix: {}, MIME: {}", Suffix, mimeType); |
Check notice
Code scanning / SonarCloud
Logging should not be vulnerable to injection attacks Low
| } | ||
|
|
||
| logger.info("[+] Safe file. Suffix: {}, MIME: {}", Suffix, mimeType); | ||
| logger.info("[+] Successfully uploaded {}", filePath); |
Check notice
Code scanning / SonarCloud
Logging should not be vulnerable to injection attacks Low
|
|
||
| private void deleteFile(String filePath) { | ||
| File delFile = new File(filePath); | ||
| if(delFile.isFile() && delFile.exists()) { |
Check warning
Code scanning / SonarCloud
Accessing files should not lead to filesystem oracle attacks Medium
|
|
||
| private void deleteFile(String filePath) { | ||
| File delFile = new File(filePath); | ||
| if(delFile.isFile() && delFile.exists()) { |
Check warning
Code scanning / SonarCloud
Accessing files should not lead to filesystem oracle attacks Medium
| private void deleteFile(String filePath) { | ||
| File delFile = new File(filePath); | ||
| if(delFile.isFile() && delFile.exists()) { | ||
| if (delFile.delete()) { |
Check failure
Code scanning / SonarCloud
I/O function calls should not be vulnerable to path injection attacks High
| File delFile = new File(filePath); | ||
| if(delFile.isFile() && delFile.exists()) { | ||
| if (delFile.delete()) { | ||
| logger.info("[+] " + filePath + " delete successfully!"); |
Check notice
Code scanning / SonarCloud
Logging should not be vulnerable to injection attacks Low
| return; | ||
| } | ||
| } | ||
| logger.info(filePath + " delete failed!"); |
Check notice
Code scanning / SonarCloud
Logging should not be vulnerable to injection attacks Low
No description provided.