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

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);

Check failure

Code scanning / SonarCloud

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

Change this code to not construct the path from user-controlled data. See more on SonarQube Cloud

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

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);

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks Low

Change this code to not log user-controlled data. See more on SonarQube Cloud
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);

Check failure

Code scanning / SonarCloud

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

Change this code to not construct the path from user-controlled data. See more on SonarQube Cloud
} catch (IOException e) {
logger.error(e.toString());
deleteFile(filePath);
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

Change this code to not log user-controlled data. See more on SonarQube Cloud
logger.info("[+] Successfully uploaded {}", filePath);

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks Low

Change this code to not log user-controlled data. See more on SonarQube Cloud
return String.format("You successfully uploaded '%s'", filePath);

Check failure

Code scanning / SonarCloud

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

Change this code to not reflect user-controlled data. See more on SonarQube Cloud
}
Comment on lines +81 to +155
Copy link

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()) {

Check warning

Code scanning / SonarCloud

Accessing files should not lead to filesystem oracle attacks Medium

Change this code to not construct the path from user-controlled data. See more on SonarQube Cloud

Check warning

Code scanning / SonarCloud

Accessing files should not lead to filesystem oracle attacks Medium

Change this code to not construct the path from user-controlled data. See more on SonarQube Cloud
if (delFile.delete()) {

Check failure

Code scanning / SonarCloud

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

Change this code to not construct the path from user-controlled data. See more on SonarQube Cloud
logger.info("[+] " + filePath + " delete successfully!");

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks Low

Change this code to not log user-controlled data. See more on SonarQube Cloud
return;
}
}
logger.info(filePath + " delete failed!");

Check notice

Code scanning / SonarCloud

Logging should not be vulnerable to injection attacks Low

Change this code to not log user-controlled data. See more on SonarQube Cloud
}
Comment on lines +157 to +166
Copy link

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

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;
}
}