Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
198 changes: 198 additions & 0 deletions src/main/java/org/joychou/controller/FileUpload2.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
package org.joychou.controller;

import com.fasterxml.uuid.Generators;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.*;
import org.springframework.web.multipart.MultipartFile;
import org.springframework.web.servlet.mvc.support.RedirectAttributes;

import javax.imageio.ImageIO;
import java.awt.image.BufferedImage;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.UUID;

import org.joychou.security.SecurityUtil;


/**
* File upload.
*
* @author JoyChou @ 2018-08-15
*/
@Controller
@RequestMapping("/file")
public class FileUpload {

// Save the uploaded file to this folder
private static final String UPLOADED_FOLDER = "/tmp/";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

硬编码上传目录可能会带来安全与灵活性问题。
建议通过配置文件或环境变量来管理上传目录,避免在生产环境中直接写死“/tmp/”路径,并确认文件权限。

private final Logger logger = LoggerFactory.getLogger(this.getClass());
private static String randomFilePath = "";

// uplaod any file
@GetMapping("/any")
public String index() {
return "upload"; // return upload.html page
}

// only allow to upload pictures
@GetMapping("/pic")
public String uploadPic() {
return "uploadPic"; // return uploadPic.html page
}

@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);
Comment thread Fixed

Check failure

Code scanning / SonarCloud

I/O function calls should not be vulnerable to path injection attacks

<!--SONAR_ISSUE_KEY:AZWiEJwneLlEgZLMlwPN-->Change this code to not construct the path from user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=cccfeng_java-sec-code&issues=AZWiEJwneLlEgZLMlwPN&open=AZWiEJwneLlEgZLMlwPN&pullRequest=2">SonarQube Cloud</a></p>

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";
}
Comment on lines +50 to +74

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

文件名直接拼接至路径存在潜在路径穿越风险。
当前未对 file.getOriginalFilename() 做目录穿越检测,若上传文件名包含 ../ 等路径可导致覆盖任意文件。建议使用安全的文件名过滤或自动生成随机文件名,避免产生安全隐患。


@GetMapping("/status")
public String uploadStatus() {
return "uploadStatus";
}

// 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);
Comment thread Fixed

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks

<!--SONAR_ISSUE_KEY:AZWiEJwneLlEgZLMlwPQ-->Change this code to not log user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=cccfeng_java-sec-code&issues=AZWiEJwneLlEgZLMlwPQ&open=AZWiEJwneLlEgZLMlwPQ&pullRequest=2">SonarQube Cloud</a></p>
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);
Comment thread Fixed

Check failure

Code scanning / SonarCloud

I/O function calls should not be vulnerable to path injection attacks

<!--SONAR_ISSUE_KEY:AZWiEJwneLlEgZLMlwPM-->Change this code to not construct the path from user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=cccfeng_java-sec-code&issues=AZWiEJwneLlEgZLMlwPM&open=AZWiEJwneLlEgZLMlwPM&pullRequest=2">SonarQube Cloud</a></p>
} catch (IOException e) {
logger.error(e.toString());
deleteFile(filePath);
return "Upload failed";
}

logger.info("[+] Safe file. Suffix: {}, MIME: {}", Suffix, mimeType);
Comment thread Fixed

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks

<!--SONAR_ISSUE_KEY:AZWiEJwneLlEgZLMlwPR-->Change this code to not log user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=cccfeng_java-sec-code&issues=AZWiEJwneLlEgZLMlwPR&open=AZWiEJwneLlEgZLMlwPR&pullRequest=2">SonarQube Cloud</a></p>
logger.info("[+] Successfully uploaded {}", filePath);
Comment thread Fixed

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks

<!--SONAR_ISSUE_KEY:AZWiEJwneLlEgZLMlwPT-->Change this code to not log user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=cccfeng_java-sec-code&issues=AZWiEJwneLlEgZLMlwPT&open=AZWiEJwneLlEgZLMlwPT&pullRequest=2">SonarQube Cloud</a></p>
return String.format("You successfully uploaded '%s'", filePath);
Comment thread Fixed

Check failure

Code scanning / SonarCloud

Endpoints should not be vulnerable to reflected cross-site scripting (XSS) attacks

<!--SONAR_ISSUE_KEY:AZWiEJwneLlEgZLMlwPO-->Change this code to not reflect user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=cccfeng_java-sec-code&issues=AZWiEJwneLlEgZLMlwPO&open=AZWiEJwneLlEgZLMlwPO&pullRequest=2">SonarQube Cloud</a></p>
}
Comment on lines +81 to +155

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

图片上传检查逻辑较完善,但以下细节可进一步增强安全性与可维护性:

  1. 变量命名不当:excelFile 命名与图片类型不符,会引起混淆。
  2. 存在多处返回“Illeagl picture.”的字符串,单词拼写应为“Illegal”。
  3. SecurityUtil.replaceSpecialStr() 的实现细节未知,需确保其不会误删关键字符或弱化安全校验。
  4. 使用静态成员 randomFilePath 保存动态文件路径容易出现并发覆盖问题,多线程下存在 race condition 风险。


private void deleteFile(String filePath) {
File delFile = new File(filePath);
if(delFile.isFile() && delFile.exists()) {
Comment thread Fixed
Comment thread Fixed

Check warning

Code scanning / SonarCloud

Accessing files should not lead to filesystem oracle attacks

<!--SONAR_ISSUE_KEY:AZWiEJwneLlEgZLMlwPU-->Change this code to not construct the path from user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=cccfeng_java-sec-code&issues=AZWiEJwneLlEgZLMlwPU&open=AZWiEJwneLlEgZLMlwPU&pullRequest=2">SonarQube Cloud</a></p>

Check warning

Code scanning / SonarCloud

Accessing files should not lead to filesystem oracle attacks

<!--SONAR_ISSUE_KEY:AZWiEJwneLlEgZLMlwPV-->Change this code to not construct the path from user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=cccfeng_java-sec-code&issues=AZWiEJwneLlEgZLMlwPV&open=AZWiEJwneLlEgZLMlwPV&pullRequest=2">SonarQube Cloud</a></p>
if (delFile.delete()) {
Comment thread Fixed

Check failure

Code scanning / SonarCloud

I/O function calls should not be vulnerable to path injection attacks

<!--SONAR_ISSUE_KEY:AZWiEJwneLlEgZLMlwPL-->Change this code to not construct the path from user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=cccfeng_java-sec-code&issues=AZWiEJwneLlEgZLMlwPL&open=AZWiEJwneLlEgZLMlwPL&pullRequest=2">SonarQube Cloud</a></p>
logger.info("[+] " + filePath + " delete successfully!");
Comment thread Fixed

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks

<!--SONAR_ISSUE_KEY:AZWiEJwneLlEgZLMlwPP-->Change this code to not log user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=cccfeng_java-sec-code&issues=AZWiEJwneLlEgZLMlwPP&open=AZWiEJwneLlEgZLMlwPP&pullRequest=2">SonarQube Cloud</a></p>
return;
}
}
logger.info(filePath + " delete failed!");
Comment thread Fixed

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks

<!--SONAR_ISSUE_KEY:AZWiEJwneLlEgZLMlwPS-->Change this code to not log user-controlled data. <p>See more on <a href="https://sonarcloud.io/project/issues?id=cccfeng_java-sec-code&issues=AZWiEJwneLlEgZLMlwPS&open=AZWiEJwneLlEgZLMlwPS&pullRequest=2">SonarQube Cloud</a></p>
}
Comment on lines +157 to +166

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

缺少对删除文件操作的更多限制与验证。
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;
}
Comment on lines +168 to +189

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

convert 方法中缺少文件大小限制及异常处理机制。
在将 MultipartFile 写入磁盘时,若文件过大或写入受限,可能出现资源耗尽或异常未捕获的问题。使用 try-with-resources 可确保 FileOutputStream 正常关闭。


/**
* Check if the file is a picture.
*/
private static boolean isImage(File file) throws IOException {
BufferedImage bi = ImageIO.read(file);
return bi != null;
}
}