From d0a0700275236f83d961491d8deef506f9e34840 Mon Sep 17 00:00:00 2001 From: kl Date: Thu, 8 Oct 2026 14:04:15 +0800 Subject: [PATCH 1/9] fix: reject control characters in FTP command arguments --- .../main/java/cn/keking/utils/FtpUtils.java | 13 ++++++ .../keking/utils/FtpCommandSecurityTests.java | 46 +++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 server/src/test/java/cn/keking/utils/FtpCommandSecurityTests.java diff --git a/server/src/main/java/cn/keking/utils/FtpUtils.java b/server/src/main/java/cn/keking/utils/FtpUtils.java index e3ac7c469..0a1859430 100644 --- a/server/src/main/java/cn/keking/utils/FtpUtils.java +++ b/server/src/main/java/cn/keking/utils/FtpUtils.java @@ -154,9 +154,22 @@ private static FtpConnectionInfo parseFtpConnectionInfo(String ftpUrl, String ft } catch (URISyntaxException e) { throw new IOException("无效的FTP URL: " + ftpUrl, e); } + // URI#getPath decodes percent escapes. Validate the values actually + // sent as FTP command arguments, before opening any connection. + validateCommandArgument(info.remoteFilePath); + validateCommandArgument(info.username); + validateCommandArgument(info.password); + validateCommandArgument(new String(info.remoteFilePath.getBytes(info.controlEncoding), + StandardCharsets.ISO_8859_1)); return info; } + private static void validateCommandArgument(String value) throws IOException { + if (value != null && value.chars().anyMatch(c -> c < 0x20 || c == 0x7f)) { + throw new IOException("FTP command arguments must not contain control characters"); + } + } + /** * FTP连接信息对象 */ diff --git a/server/src/test/java/cn/keking/utils/FtpCommandSecurityTests.java b/server/src/test/java/cn/keking/utils/FtpCommandSecurityTests.java new file mode 100644 index 000000000..d5b4235a2 --- /dev/null +++ b/server/src/test/java/cn/keking/utils/FtpCommandSecurityTests.java @@ -0,0 +1,46 @@ +package cn.keking.utils; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.test.util.ReflectionTestUtils; + +import java.io.IOException; + +import static org.junit.jupiter.api.Assertions.*; + +class FtpCommandSecurityTests { + @ParameterizedTest + @ValueSource(strings = { + "file.txt%0D%0ANOOP", "file.txt%0d%0aNOOP/clean.txt", + "file.txt%0D%0ANOOP?fullfilename=clean.txt", "file.txt%0aNOOP", "file.txt%00" + }) + void rejectsDecodedControlsBeforeDownloadOrPreviewConnects(String path) { + String url = "ftp://127.0.0.1/" + path; + IOException download = assertThrows(IOException.class, + () -> FtpUtils.download(url, "1", "unused", "user", "pass", "UTF-8")); + IOException preview = assertThrows(IOException.class, + () -> FtpUtils.preview(url, "1", "unused", "user", "pass", "UTF-8")); + assertTrue(download.getMessage().contains("control characters")); + assertTrue(preview.getMessage().contains("control characters")); + } + + @Test + void rejectsControlsInCredentialsBeforeConnecting() { + IOException error = assertThrows(IOException.class, + () -> FtpUtils.preview("ftp://127.0.0.1/file.txt", "1", "unused", + "user\r\nNOOP", "pass", "UTF-8")); + assertTrue(error.getMessage().contains("control characters")); + error = assertThrows(IOException.class, + () -> FtpUtils.download("ftp://127.0.0.1/file.txt", "1", "unused", + "user", "pass\nNOOP", "UTF-8")); + assertTrue(error.getMessage().contains("control characters")); + } + + @Test + void preservesOrdinaryEncodedNamesAndDoesNotDoubleDecode() { + Object info = ReflectionTestUtils.invokeMethod(FtpUtils.class, "parseFtpConnectionInfo", + "ftp://files.example/a%20b/%E4%B8%AD%E6%96%87%250D.txt", "21", "user", "pass", "UTF-8"); + assertEquals("/a b/中文%0D.txt", ReflectionTestUtils.getField(info, "remoteFilePath")); + } +} From 91b46383d1a33f41e88c6d3b513a39fd30aa8655 Mon Sep 17 00:00:00 2001 From: kl Date: Thu, 8 Oct 2026 14:06:58 +0800 Subject: [PATCH 2/9] fix: confine upload writes and folder creation to demo storage --- .../keking/web/controller/FileController.java | 105 +++++++++--------- .../controller/UploadPathSecurityTests.java | 87 +++++++++++++++ 2 files changed, 142 insertions(+), 50 deletions(-) create mode 100644 server/src/test/java/cn/keking/web/controller/UploadPathSecurityTests.java diff --git a/server/src/main/java/cn/keking/web/controller/FileController.java b/server/src/main/java/cn/keking/web/controller/FileController.java index 43c2586ba..67a2730b1 100644 --- a/server/src/main/java/cn/keking/web/controller/FileController.java +++ b/server/src/main/java/cn/keking/web/controller/FileController.java @@ -29,6 +29,9 @@ import java.io.InputStream; import java.io.OutputStream; import java.nio.file.DirectoryStream; +import java.nio.file.FileAlreadyExistsException; +import java.nio.file.LinkOption; +import java.nio.file.StandardOpenOption; import java.nio.file.Files; import java.nio.file.InvalidPathException; import java.nio.file.Path; @@ -154,29 +157,24 @@ public String getStats() { @PostMapping("/fileUpload") public ReturnResponse fileUpload(@RequestParam("file") MultipartFile file, @RequestParam(value = "path", defaultValue = "") String path) { - ReturnResponse checkResult = this.fileUploadCheck(file, path); + ReturnResponse checkResult = this.fileUploadCheck(file); if (checkResult.isFailure()) { return checkResult; } - String uploadPath = fileDir + demoPath; - if (!ObjectUtils.isEmpty(path)) { - uploadPath += path + File.separator; - } - - File outFile = new File(uploadPath); - if (!outFile.exists() && !outFile.mkdirs()) { - logger.error("创建文件夹【{}】失败,请检查目录权限!", uploadPath); - return ReturnResponse.failure("创建文件夹失败,请检查目录权限!"); - } - String fileName = checkResult.getContent().toString(); - logger.info("上传文件:{}{}", uploadPath, fileName); - - try (InputStream in = file.getInputStream(); - OutputStream out = Files.newOutputStream(Paths.get(uploadPath + fileName))) { - StreamUtils.copy(in, out); + try { + Path target = createUploadDirectory(path).resolve(fileName); + try (InputStream in = file.getInputStream(); + OutputStream out = Files.newOutputStream(target, StandardOpenOption.CREATE_NEW, + StandardOpenOption.WRITE, LinkOption.NOFOLLOW_LINKS)) { + StreamUtils.copy(in, out); + } return ReturnResponse.success(null); + } catch (FileAlreadyExistsException e) { + return ReturnResponse.failure("存在同名文件,请先删除原有文件再次上传"); + } catch (SecurityException | InvalidPathException e) { + return ReturnResponse.failure("非法目录路径"); } catch (IOException e) { logger.error("文件上传失败", e); return ReturnResponse.failure("文件上传失败"); @@ -195,30 +193,49 @@ public ReturnResponse createFolder(@RequestParam(value = "path", default return ReturnResponse.failure("文件夹名称不能为空"); } - if (KkFileUtils.isIllegalFileName(folderName)) { + if (KkFileUtils.isIllegalFileName(folderName) || folderName.contains("/") + || folderName.contains("\\") || folderName.contains(":") + || folderName.indexOf('\0') >= 0 || folderName.endsWith(".") || folderName.endsWith(" ")) { return ReturnResponse.failure("非法文件夹名称"); } - String basePath = fileDir + demoPath; - if (!ObjectUtils.isEmpty(path)) { - basePath += path + File.separator; - } + Files.createDirectory(createUploadDirectory(path).resolve(folderName)); + return ReturnResponse.success(); + } catch (FileAlreadyExistsException e) { + return ReturnResponse.failure("文件夹已存在"); + } catch (SecurityException | InvalidPathException e) { + return ReturnResponse.failure("非法目录路径"); + } catch (Exception e) { + logger.error("创建文件夹异常", e); + return ReturnResponse.failure("创建文件夹失败"); + } + } - File newFolder = new File(basePath + folderName); - if (newFolder.exists()) { - return ReturnResponse.failure("文件夹已存在"); + /** Create only relative upload directories, without following symbolic links. */ + private Path createUploadDirectory(String requestedPath) throws IOException { + String value = requestedPath == null ? "" : requestedPath.replace('\\', '/'); + if (value.startsWith("/") || value.indexOf(':') >= 0 || value.indexOf('\0') >= 0) { + throw new SecurityException("Absolute paths are not allowed"); + } + Path relative = Paths.get(value); + for (Path segment : relative) { + String name = segment.toString(); + if ("..".equals(name) || (!".".equals(name) && (name.endsWith(".") || name.endsWith(" ")))) { + throw new SecurityException("Parent path segments are not allowed"); } - - if (newFolder.mkdirs()) { - logger.info("创建文件夹:{}", newFolder.getAbsolutePath()); - return ReturnResponse.success(); - } else { - logger.error("创建文件夹失败:{}", newFolder.getAbsolutePath()); - return ReturnResponse.failure("创建文件夹失败,请检查目录权限"); + } + Path root = Files.createDirectories(Paths.get(fileDir)).toRealPath(); + Path current = root; + for (Path segment : Paths.get(demoDir).resolve(relative).normalize()) { + current = current.resolve(segment); + try { + Files.createDirectory(current); + } catch (FileAlreadyExistsException e) { + if (!Files.isDirectory(current, LinkOption.NOFOLLOW_LINKS)) { + throw new SecurityException("Upload directory must not be a symbolic link or file"); + } } - } catch (Exception e) { - logger.error("创建文件夹异常", e); - return ReturnResponse.failure("创建文件夹失败:" + e.getMessage()); } + return current; } @PostMapping("/deleteFile") @@ -722,7 +739,7 @@ private Map convertFileInfoToMap(FileInfoWrapper info, String pa /** * 上传文件前校验 */ - private ReturnResponse fileUploadCheck(MultipartFile file, String path) { + private ReturnResponse fileUploadCheck(MultipartFile file) { if (ConfigConstants.getFileUploadDisable()) { return ReturnResponse.failure("文件上传接口已禁用"); } @@ -734,7 +751,8 @@ private ReturnResponse fileUploadCheck(MultipartFile file, String path) if (!KkFileUtils.isAllowedUpload(fileName)) { return ReturnResponse.failure("不允许上传的文件类型: " + fileName); } - if (KkFileUtils.isIllegalFileName(fileName)) { + if (KkFileUtils.isIllegalFileName(fileName) || fileName.contains(":") || fileName.indexOf('\0') >= 0 + || fileName.endsWith(".") || fileName.endsWith(" ")) { return ReturnResponse.failure("不允许上传的文件名: " + fileName); } FileType type = FileType.typeFromFileName(fileName); @@ -742,11 +760,6 @@ private ReturnResponse fileUploadCheck(MultipartFile file, String path) return ReturnResponse.failure("该文件格式还不支持预览,请联系管理员,添加该格式: " + fileName); } - // 判断是否存在同名文件 - if (existsFile(fileName, path)) { - return ReturnResponse.failure("存在同名文件,请先删除原有文件再次上传"); - } - return ReturnResponse.success(fileName); } @@ -817,12 +830,4 @@ public Object directory(String urls) { return RarUtils.getTree(fileUrl); } - private boolean existsFile(String fileName, String path) { - String fullPath = fileDir + demoPath; - if (!ObjectUtils.isEmpty(path)) { - fullPath += path + File.separator; - } - File file = new File(fullPath + fileName); - return file.exists(); - } } diff --git a/server/src/test/java/cn/keking/web/controller/UploadPathSecurityTests.java b/server/src/test/java/cn/keking/web/controller/UploadPathSecurityTests.java new file mode 100644 index 000000000..71af129fb --- /dev/null +++ b/server/src/test/java/cn/keking/web/controller/UploadPathSecurityTests.java @@ -0,0 +1,87 @@ +package cn.keking.web.controller; + +import cn.keking.config.ConfigConstants; +import org.junit.jupiter.api.*; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.mock.web.MockMultipartFile; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.setup.MockMvcBuilders; + +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.jupiter.api.Assertions.*; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*; + +class UploadPathSecurityTests { + @TempDir Path temp; + private String oldDir; + private Boolean oldDisabled; + private MockMvc mvc; + + @BeforeEach void setup() throws Exception { + oldDir = ConfigConstants.getFileDir(); + oldDisabled = ConfigConstants.getFileUploadDisable(); + ConfigConstants.setFileDirValue(Files.createDirectory(temp.resolve("files")).toString()); + ConfigConstants.setFileUploadDisableValue(false); + mvc = MockMvcBuilders.standaloneSetup(new FileController()).build(); + } + + @AfterEach void restore() { + ConfigConstants.setFileDirValue(oldDir); + ConfigConstants.setFileUploadDisableValue(oldDisabled); + } + + private MockMultipartFile file() { + return new MockMultipartFile("file", "canary.pdf", "application/pdf", "CANARY".getBytes()); + } + + @ParameterizedTest + @ValueSource(strings = {"../../outside", "..\\..\\outside", "/tmp/outside", "C:\\outside", + "\\\\server\\share", "a/../outside", ".. /outside", "bad\0path"}) + void refusesTraversalAtBothHttpEndpoints(String path) throws Exception { + mvc.perform(multipart("/fileUpload").file(file()).param("path", path)) + .andExpect(jsonPath("$.code").value(1)); + mvc.perform(post("/createFolder").param("path", path).param("folderName", "new")) + .andExpect(jsonPath("$.code").value(1)); + assertFalse(Files.exists(temp.resolve("outside"))); + } + + @Test void preservesNestedUploadsAndRefusesOverwrite() throws Exception { + mvc.perform(multipart("/fileUpload").file(file()).param("path", "nested/child")) + .andExpect(jsonPath("$.code").value(0)); + Path target = temp.resolve("files/demo/nested/child/canary.pdf"); + assertEquals("CANARY", Files.readString(target)); + mvc.perform(multipart("/fileUpload").file(file()).param("path", "nested/child")) + .andExpect(jsonPath("$.code").value(1)); + mvc.perform(post("/createFolder").param("path", "nested\\child").param("folderName", "new")) + .andExpect(jsonPath("$.code").value(0)); + assertTrue(Files.isDirectory(target.getParent().resolve("new"))); + } + + @Test void refusesParentSymlinksAndExistingFileSymlinks() throws Exception { + Path root = Files.createDirectory(temp.resolve("files/demo")); + Path outside = Files.createDirectory(temp.resolve("outside")); + Files.createSymbolicLink(root.resolve("link"), outside); + mvc.perform(multipart("/fileUpload").file(file()).param("path", "link/new")) + .andExpect(jsonPath("$.code").value(1)); + mvc.perform(post("/createFolder").param("path", "link/new").param("folderName", "new")) + .andExpect(jsonPath("$.code").value(1)); + assertFalse(Files.exists(outside.resolve("new"))); + Files.createSymbolicLink(root.resolve("canary.pdf"), outside.resolve("missing.pdf")); + mvc.perform(multipart("/fileUpload").file(file())) + .andExpect(jsonPath("$.code").value(1)); + assertFalse(Files.exists(outside.resolve("missing.pdf"))); + } + + @Test void uploadDisableStillBlocksBothEndpoints() throws Exception { + ConfigConstants.setFileUploadDisableValue(true); + mvc.perform(multipart("/fileUpload").file(file())).andExpect(jsonPath("$.code").value(1)); + mvc.perform(post("/createFolder").param("folderName", "new")) + .andExpect(jsonPath("$.code").value(1)); + assertFalse(Files.exists(temp.resolve("files/demo"))); + } +} From 93931f6a7a7e305545c49b0230d7078efe7630e2 Mon Sep 17 00:00:00 2001 From: kl Date: Thu, 8 Oct 2026 14:11:19 +0800 Subject: [PATCH 3/9] fix: validate every preview source before applying trust policies --- .../main/java/cn/keking/utils/WebUtils.java | 63 ++++++++++------ .../cn/keking/web/filter/TrustDirFilter.java | 13 +++- .../cn/keking/web/filter/TrustHostFilter.java | 10 ++- .../filter/PreviewSourceSecurityTests.java | 75 +++++++++++++++++++ 4 files changed, 133 insertions(+), 28 deletions(-) create mode 100644 server/src/test/java/cn/keking/web/filter/PreviewSourceSecurityTests.java diff --git a/server/src/main/java/cn/keking/utils/WebUtils.java b/server/src/main/java/cn/keking/utils/WebUtils.java index beaa9a3e5..8fe20b3c4 100644 --- a/server/src/main/java/cn/keking/utils/WebUtils.java +++ b/server/src/main/java/cn/keking/utils/WebUtils.java @@ -241,32 +241,50 @@ public static String encodeUrlFileName(String url) { * @return url */ public static String getSourceUrl(ServletRequest request) { - String url = request.getParameter("url"); - String urls = request.getParameter("urls"); - String currentUrl = request.getParameter("currentUrl"); - String urlPath = request.getParameter("urlPath"); + List urls = getSourceUrls(request); + return urls.isEmpty() ? null : urls.get(0); + } + + /** Validate every candidate consumed by preview endpoints, including all images. */ + public static List getSourceUrls(ServletRequest request) { + List urls = new ArrayList<>(); String encryption = request.getParameter("encryption"); - if (StringUtils.isNotBlank(url)) { - return decodeUrl(url,encryption); - } - if (StringUtils.isNotBlank(currentUrl)) { - return decodeUrl(currentUrl,encryption); - } - if (StringUtils.isNotBlank(urlPath)) { - return decodeUrl(urlPath,encryption); - } - if (StringUtils.isNotBlank(urls)) { - urls = decodeUrl(urls,encryption); - String[] images = urls.split("\\|"); - return images[0]; + try { + for (String name : List.of("url", "currentUrl", "urlPath", "urls")) { + String[] values = request.getParameterValues(name); + if (values == null) { + continue; + } + for (String value : values) { + if (StringUtils.isBlank(value)) { + continue; + } + // picturesPreview decodes currentUrl as Base64 independently of encryption. + String decoded = "currentUrl".equals(name) + ? decodeBase64String(value, StandardCharsets.UTF_8) : decodeUrl(value, encryption); + if (decoded == null) { + return List.of(""); + } + if ("urls".equals(name)) { + urls.addAll(Arrays.asList(decoded.split("\\|", -1))); + } else { + urls.add(decoded); + } + } + } + } catch (RuntimeException e) { + return List.of(""); } - return null; + return urls; } /** * 判断地址是否正确 * 高 2022/12/17 */ public static boolean isValidUrl(String url) { + if (url == null) { + return false; + } String regStr = "^((https|http|ftp|rtsp|mms|file)://)";//[.?*]表示匹配的就是本身 Pattern pattern = Pattern.compile(regStr); Matcher matcher = pattern.matcher(url); @@ -350,10 +368,13 @@ public static String urlSecurity(String url) { * @return host */ public static String getHost(String urlStr) { + if (urlStr == null) { + return null; + } try { - URL url = new URL(urlStr); - return url.getHost().toLowerCase(); - } catch (MalformedURLException ignored) { + URL url = normalizedURL(urlStr); + return url.getHost().toLowerCase(Locale.ROOT); + } catch (GalimatiasParseException | MalformedURLException | IllegalArgumentException ignored) { } return null; } diff --git a/server/src/main/java/cn/keking/web/filter/TrustDirFilter.java b/server/src/main/java/cn/keking/web/filter/TrustDirFilter.java index f71e9fd31..f4bb5a953 100644 --- a/server/src/main/java/cn/keking/web/filter/TrustDirFilter.java +++ b/server/src/main/java/cn/keking/web/filter/TrustDirFilter.java @@ -13,6 +13,7 @@ import org.springframework.util.StringUtils; import jakarta.servlet.*; +import jakarta.servlet.http.HttpServletResponse; import java.io.File; import java.io.IOException; @@ -20,6 +21,7 @@ import java.net.URLDecoder; import java.nio.charset.StandardCharsets; import java.util.Locale; +import java.util.List; /** * @author : kl (http://kailing.pub) @@ -45,9 +47,12 @@ public void init(FilterConfig filterConfig) { @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { - String url = WebUtils.getSourceUrl(request); - if (!allowPreview(url)) { - response.getWriter().write(this.notTrustDirView); + List urls = WebUtils.getSourceUrls(request); + if (urls.isEmpty() || urls.stream().anyMatch(url -> !allowPreview(url))) { + if (response instanceof HttpServletResponse httpResponse) { + httpResponse.setStatus(HttpServletResponse.SC_FORBIDDEN); + } + response.getWriter().write(this.notTrustDirView == null ? "Forbidden" : this.notTrustDirView); response.getWriter().close(); } else { chain.doFilter(request, response); @@ -61,7 +66,7 @@ public void destroy() { private boolean allowPreview(String urlPath) { // 判断URL是否合法 - if (KkFileUtils.isIllegalFileName(urlPath) || !StringUtils.hasText(urlPath) || !WebUtils.isValidUrl(urlPath)) { + if (!StringUtils.hasText(urlPath) || KkFileUtils.isIllegalFileName(urlPath) || !WebUtils.isValidUrl(urlPath)) { return false; } try { diff --git a/server/src/main/java/cn/keking/web/filter/TrustHostFilter.java b/server/src/main/java/cn/keking/web/filter/TrustHostFilter.java index f7547ad55..88cf17843 100644 --- a/server/src/main/java/cn/keking/web/filter/TrustHostFilter.java +++ b/server/src/main/java/cn/keking/web/filter/TrustHostFilter.java @@ -5,6 +5,7 @@ import java.io.IOException; import java.util.Map; +import java.util.List; import java.util.Locale; import java.util.concurrent.ConcurrentHashMap; import java.nio.charset.StandardCharsets; @@ -48,9 +49,12 @@ public void init(FilterConfig filterConfig) { @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { - String url = WebUtils.getSourceUrl(request); - String host = WebUtils.getHost(url); - if (isNotTrustHost(host) || !WebUtils.isValidUrl(url)) { + List urls = WebUtils.getSourceUrls(request); + String rejectedUrl = urls.stream() + .filter(url -> !WebUtils.isValidUrl(url) || isNotTrustHost(WebUtils.getHost(url))) + .findFirst().orElse(null); + if (urls.isEmpty() || rejectedUrl != null) { + String host = rejectedUrl == null ? null : WebUtils.getHost(rejectedUrl); String currentHost = host == null ? "UNKNOWN" : host; if (response instanceof HttpServletResponse httpServletResponse) { httpServletResponse.setStatus(HttpServletResponse.SC_FORBIDDEN); diff --git a/server/src/test/java/cn/keking/web/filter/PreviewSourceSecurityTests.java b/server/src/test/java/cn/keking/web/filter/PreviewSourceSecurityTests.java new file mode 100644 index 000000000..1bc694cc5 --- /dev/null +++ b/server/src/test/java/cn/keking/web/filter/PreviewSourceSecurityTests.java @@ -0,0 +1,75 @@ +package cn.keking.web.filter; + +import cn.keking.config.ConfigConstants; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; + +import java.nio.charset.StandardCharsets; +import java.util.Base64; +import java.util.concurrent.atomic.AtomicBoolean; + +import static org.junit.jupiter.api.Assertions.*; + +class PreviewSourceSecurityTests { + private String previousLocalDir; + @BeforeEach void setup() { + previousLocalDir = ConfigConstants.getLocalPreviewDir(); + ConfigConstants.setLocalPreviewDirValue("/allowed-preview-root"); + ConfigConstants.setTrustHostValue("files.example"); + ConfigConstants.setNotTrustHostValue("127.0.0.1"); + } + @AfterEach void restore() { + org.springframework.test.util.ReflectionTestUtils.setField(ConfigConstants.class, "localPreviewDir", previousLocalDir); + ConfigConstants.setTrustHostValue("default"); + ConfigConstants.setNotTrustHostValue("default"); + } + private String encoded(String url) { + return Base64.getEncoder().encodeToString(url.getBytes(StandardCharsets.UTF_8)); + } + private MockHttpServletResponse filter(MockHttpServletRequest request) throws Exception { + MockHttpServletResponse response = new MockHttpServletResponse(); + AtomicBoolean reached = new AtomicBoolean(); + new TrustHostFilter().doFilter(request, response, (req, res) -> reached.set(true)); + assertEquals(response.getStatus() == 200, reached.get()); + return response; + } + @ParameterizedTest + @ValueSource(strings = {"url", "currentUrl", "urls"}) + void allowedDecoysCannotHideBlockedFetchTarget(String decoy) throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/getCorsFile"); + request.setParameter(decoy, encoded("https://files.example/allowed.pdf")); + request.setParameter("urlPath", encoded("http://127.0.0.1/private.pdf")); + assertEquals(403, filter(request).getStatus()); + } + @Test void validatesEveryImageAndEveryRepeatedParameter() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/picturesPreview"); + request.setParameter("urls", encoded("https://files.example/a.jpg|http://127.0.0.1/b.jpg")); + assertEquals(403, filter(request).getStatus()); + request = new MockHttpServletRequest("GET", "/getCorsFile"); + request.setParameter("urlPath", encoded("https://files.example/a.pdf"), encoded("http://127.0.0.1/a.pdf")); + assertEquals(403, filter(request).getStatus()); + } + @Test void preservesAllowedRequestsAndRejectsMissingOrMalformedSources() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/getCorsFile"); + assertEquals(403, filter(request).getStatus()); + request.setParameter("urlPath", "%%%bad"); + assertEquals(403, filter(request).getStatus()); + request.setParameter("urlPath", encoded("https://files.example/a.pdf")); + assertEquals(200, filter(request).getStatus()); + request.setParameter("url", encoded("https://files.example/b.pdf")); + assertEquals(200, filter(request).getStatus()); + } + @Test void directoryFilterAlsoRejectsHiddenFileTargets() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/getCorsFile"); + request.setParameter("url", encoded("https://files.example/a.pdf")); + request.setParameter("urlPath", encoded("file:///outside-preview-root/private.pdf")); + MockHttpServletResponse response = new MockHttpServletResponse(); + new TrustDirFilter().doFilter(request, response, (req, res) -> fail("Unsafe URL reached controller")); + assertEquals(403, response.getStatus()); + } +} From 84369c032ba482c1c30033260346ca861deb3dc4 Mon Sep 17 00:00:00 2001 From: kl Date: Thu, 8 Oct 2026 14:11:19 +0800 Subject: [PATCH 4/9] fix: constrain preview page and agent parameters to safe scalar values --- .../controller/OnlinePreviewController.java | 17 +++- .../PreviewParameterSecurityTests.java | 84 +++++++++++++++++++ 2 files changed, 98 insertions(+), 3 deletions(-) create mode 100644 server/src/test/java/cn/keking/web/controller/PreviewParameterSecurityTests.java diff --git a/server/src/main/java/cn/keking/web/controller/OnlinePreviewController.java b/server/src/main/java/cn/keking/web/controller/OnlinePreviewController.java index 666e322be..3d5e4d155 100644 --- a/server/src/main/java/cn/keking/web/controller/OnlinePreviewController.java +++ b/server/src/main/java/cn/keking/web/controller/OnlinePreviewController.java @@ -86,8 +86,8 @@ public String onlinePreview(@RequestParam String url, highlightall= KkFileUtils.htmlEscape(highlightall); model.addAttribute("highlightall", highlightall); - model.addAttribute("page", page); - model.addAttribute("kkagent", kkagent); + model.addAttribute("page", normalizePage(page)); + model.addAttribute("kkagent", Boolean.toString(Boolean.parseBoolean(kkagent))); model.addAttribute("file", fileAttribute); FilePreview filePreview = previewFactory.get(fileAttribute); logger.info("预览文件url:{},previewType:{}", fileUrl, fileAttribute.getType()); @@ -98,6 +98,17 @@ public String onlinePreview(@RequestParam String url, return filePreview.filePreviewHandle(fileUrl, model, fileAttribute); //统一在这里处理 url } + private static String normalizePage(String page) { + if (page != null && page.matches("[0-9]{1,10}")) { + try { + return Integer.toString(Integer.parseInt(page)); + } catch (NumberFormatException ignored) { + // Out-of-range values use the same default as missing values. + } + } + return "0"; + } + @GetMapping( "/picturesPreview") public String picturesPreview(@RequestParam String urls, @RequestParam(required = false) String key, @@ -287,4 +298,4 @@ public String addQueueTask(@RequestParam String url, cacheService.addQueueTask(fileUrls); return "success"; } -} \ No newline at end of file +} diff --git a/server/src/test/java/cn/keking/web/controller/PreviewParameterSecurityTests.java b/server/src/test/java/cn/keking/web/controller/PreviewParameterSecurityTests.java new file mode 100644 index 000000000..4f8b4f158 --- /dev/null +++ b/server/src/test/java/cn/keking/web/controller/PreviewParameterSecurityTests.java @@ -0,0 +1,84 @@ +package cn.keking.web.controller; + +import cn.keking.config.ConfigConstants; +import cn.keking.model.FileAttribute; +import cn.keking.service.FileHandlerService; +import cn.keking.service.FilePreview; +import cn.keking.service.FilePreviewFactory; +import freemarker.template.Configuration; +import jakarta.servlet.http.HttpServletRequest; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.ui.ExtendedModelMap; + +import java.io.StringWriter; +import java.util.Base64; + +import static org.junit.jupiter.api.Assertions.*; + +class PreviewParameterSecurityTests { + private String previousKey; + @BeforeEach void setup() { + previousKey = ConfigConstants.getKey(); + ConfigConstants.setKeyValue("false"); + } + @AfterEach void restore() { ConfigConstants.setKeyValue(previousKey); } + + private ExtendedModelMap preview(String page, String agent) { + FileHandlerService handler = new FileHandlerService(null) { + @Override public FileAttribute getFileAttribute(String url, HttpServletRequest request) { + FileAttribute file = new FileAttribute(); + file.setName("document.pdf"); + file.setSuffix("pdf"); + return file; + } + }; + FilePreviewFactory factory = new FilePreviewFactory(null) { + @Override public FilePreview get(FileAttribute file) { + return (url, model, attribute) -> { + model.addAttribute("pdfUrl", url); + return "pdf"; + }; + } + }; + ExtendedModelMap model = new ExtendedModelMap(); + OnlinePreviewController controller = new OnlinePreviewController(factory, handler, null, null); + String url = Base64.getEncoder().encodeToString("https://files.example/document.pdf".getBytes()); + assertEquals("pdf", controller.onlinePreview(url, null, null, "false", page, agent, + model, new MockHttpServletRequest())); + return model; + } + + @ParameterizedTest + @ValueSource(strings = {"';alert(1);//", "\";alert(1);//", "1", + "1;alert(1)", "-1", "2147483648", "999999999999999999999"}) + void rendersOnlySafeScalarValuesThroughControllerAndPdfTemplate(String payload) throws Exception { + ExtendedModelMap model = preview(payload, payload); + assertEquals("0", model.get("page")); + assertEquals("false", model.get("kkagent")); + model.put("baseUrl", "https://preview.example/"); + for (String name : new String[]{"watermarkXSpace", "watermarkYSpace", "watermarkAlpha", + "watermarkWidth", "watermarkHeight", "watermarkAngle"}) { + model.put(name, "0"); + } + Configuration cfg = new Configuration(Configuration.VERSION_2_3_32); + cfg.setClassForTemplateLoading(getClass(), "/web"); + cfg.setClassicCompatible(true); + StringWriter output = new StringWriter(); + cfg.getTemplate("pdf.ftl").process(model, output); + String html = output.toString(); + assertTrue(html.contains("var kkagent = 'false';")); + assertTrue(html.contains("#page=0")); + assertFalse(html.contains("alert(1)")); + } + + @Test void preservesSupportedValues() { + assertEquals("12", preview("0012", "false").get("page")); + assertEquals("2147483647", preview("2147483647", "true").get("page")); + assertEquals("true", preview("1", "true").get("kkagent")); + } +} From 8530c0a7133927edc8e0bd0d4c6195828dd4f7d0 Mon Sep 17 00:00:00 2001 From: kl Date: Thu, 8 Oct 2026 14:17:07 +0800 Subject: [PATCH 5/9] fix: render archive trees only within authorized previews --- .../service/impl/CompressFilePreviewImpl.java | 8 ++ .../main/java/cn/keking/utils/RarUtils.java | 61 +++++++++----- .../keking/web/controller/FileController.java | 17 ---- server/src/main/resources/web/compress.ftl | 14 +--- .../ArchiveDirectorySecurityTests.java | 84 +++++++++++++++++++ 5 files changed, 131 insertions(+), 53 deletions(-) create mode 100644 server/src/test/java/cn/keking/web/controller/ArchiveDirectorySecurityTests.java diff --git a/server/src/main/java/cn/keking/service/impl/CompressFilePreviewImpl.java b/server/src/main/java/cn/keking/service/impl/CompressFilePreviewImpl.java index 13fc0791f..de817baaf 100644 --- a/server/src/main/java/cn/keking/service/impl/CompressFilePreviewImpl.java +++ b/server/src/main/java/cn/keking/service/impl/CompressFilePreviewImpl.java @@ -8,6 +8,8 @@ import cn.keking.service.FileHandlerService; import cn.keking.service.CompressFileReader; import cn.keking.utils.KkFileUtils; +import cn.keking.utils.RarUtils; +import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.poi.EncryptedDocumentException; import org.slf4j.Logger; @@ -78,6 +80,12 @@ public String filePreviewHandle(String url, Model model, FileAttribute fileAttri } model.addAttribute("fileName", fileName); model.addAttribute("fileTree", fileTree); + try { + model.addAttribute("archiveTreeJson", new ObjectMapper().writeValueAsString(RarUtils.getTree(fileTree))); + } catch (IOException | SecurityException e) { + logger.warn("Unable to read the previewed archive directory", e); + return otherFilePreview.notSupportedFile(model, fileAttribute, "该压缩包目录无法读取!"); + } return COMPRESS_FILE_PREVIEW_PAGE; } } diff --git a/server/src/main/java/cn/keking/utils/RarUtils.java b/server/src/main/java/cn/keking/utils/RarUtils.java index 6a46d392e..f5a140fa5 100644 --- a/server/src/main/java/cn/keking/utils/RarUtils.java +++ b/server/src/main/java/cn/keking/utils/RarUtils.java @@ -5,6 +5,12 @@ import org.slf4j.LoggerFactory; import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.LinkOption; +import java.nio.file.DirectoryStream; import java.io.UnsupportedEncodingException; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; @@ -19,7 +25,6 @@ **/ public class RarUtils { private static final Logger logger = LoggerFactory.getLogger(RarUtils.class); - private static final String fileDir = ConfigConstants.getFileDir(); public static byte[] getUTF8BytesFromGBKString(String gbkStr) { int n = gbkStr.length(); @@ -108,30 +113,40 @@ public static boolean isMessyCode(String strName) { /** * 读取文件目录树 */ - public static List getTree(String rootPath) { - List nodes = new ArrayList<>(); - File file = new File(fileDir+rootPath); - ZtreeNodeVo node = traverse(file); - nodes.add(node); - return nodes; + public static List getTree(String rootPath) throws IOException { + Path fileRoot = Paths.get(ConfigConstants.getFileDir()).toRealPath(); + Path archiveRoot = fileRoot.resolve(rootPath).normalize(); + if (archiveRoot.equals(fileRoot) || !archiveRoot.startsWith(fileRoot)) { + throw new SecurityException("Only an individual archive directory may be listed"); + } + archiveRoot = archiveRoot.toRealPath(); + if (archiveRoot.equals(fileRoot) || !archiveRoot.startsWith(fileRoot) || !Files.isDirectory(archiveRoot)) { + throw new SecurityException("Archive directory escapes file storage"); + } + return List.of(traverse(archiveRoot, fileRoot, archiveRoot)); } - private static ZtreeNodeVo traverse(File file) { - ZtreeNodeVo pathNodeVo = new ZtreeNodeVo(); - pathNodeVo.setId(file.getAbsolutePath().replace(fileDir, "").replace("\\", "/")); - pathNodeVo.setName(file.getName()); - pathNodeVo.setPid(file.getParent().replace(fileDir, "").replace("\\", "/")); - if (file.isDirectory()) { - List subNodeVos = new ArrayList<>(); - File[] subFiles = file.listFiles(); - if (subFiles == null) { - return pathNodeVo; - } - for (File subFile : subFiles) { - ZtreeNodeVo subNodeVo = traverse(subFile); - subNodeVos.add(subNodeVo); + + private static ZtreeNodeVo traverse(Path file, Path fileRoot, Path archiveRoot) throws IOException { + ZtreeNodeVo node = new ZtreeNodeVo(); + node.setId(fileRoot.relativize(file).toString().replace('\\', '/')); + node.setName(file.getFileName().toString()); + node.setPid(file.equals(archiveRoot) ? "" : fileRoot.relativize(file.getParent()).toString().replace('\\', '/')); + if (Files.isDirectory(file, LinkOption.NOFOLLOW_LINKS)) { + List children = new ArrayList<>(); + try (DirectoryStream entries = Files.newDirectoryStream(file)) { + for (Path child : entries) { + // Do not follow links into other cached documents or outside storage. + if (Files.isSymbolicLink(child)) { + continue; + } + if (Files.isRegularFile(child, LinkOption.NOFOLLOW_LINKS) + || Files.isDirectory(child, LinkOption.NOFOLLOW_LINKS)) { + children.add(traverse(child, fileRoot, archiveRoot)); + } + } } - pathNodeVo.setChildren(subNodeVos); + node.setChildren(children); } - return pathNodeVo; + return node; } } diff --git a/server/src/main/java/cn/keking/web/controller/FileController.java b/server/src/main/java/cn/keking/web/controller/FileController.java index 67a2730b1..57e77e06c 100644 --- a/server/src/main/java/cn/keking/web/controller/FileController.java +++ b/server/src/main/java/cn/keking/web/controller/FileController.java @@ -6,7 +6,6 @@ import cn.keking.utils.CaptchaUtil; import cn.keking.utils.DateUtils; import cn.keking.utils.KkFileUtils; -import cn.keking.utils.RarUtils; import cn.keking.utils.WebUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -814,20 +813,4 @@ private ReturnResponse deleteFileCheck(HttpServletRequest request, Strin return ReturnResponse.success(fileName); } - @GetMapping("/directory") - public Object directory(String urls) { - String fileUrl; - try { - fileUrl = WebUtils.decodeUrl(urls,"base64"); - } catch (Exception ex) { - String errorMsg = String.format(BASE64_DECODE_ERROR_MSG, "url"); - return ReturnResponse.failure(errorMsg); - } - fileUrl = fileUrl.replaceAll("http://", ""); - if (KkFileUtils.isIllegalFileName(fileUrl)) { - return ReturnResponse.failure("不允许访问的路径:"); - } - return RarUtils.getTree(fileUrl); - } - } diff --git a/server/src/main/resources/web/compress.ftl b/server/src/main/resources/web/compress.ftl index d40aef7cc..8be245d7a 100644 --- a/server/src/main/resources/web/compress.ftl +++ b/server/src/main/resources/web/compress.ftl @@ -944,22 +944,10 @@ } function loadTree() { - var url = "http://" + '${fileTree}'; setTreeStatus("目录加载中", "is-loading"); treeSummaryEl.textContent = "正在准备目录结构..."; showTreeState("loading"); - $.ajax({ - type: "get", - url: "${baseUrl}directory?urls=" + encodeURIComponent(Base64.encode(url)), - success: function (res) { - initTree(res); - }, - error: function () { - showTreeState("error"); - setTreeStatus("目录加载失败", "is-error"); - treeSummaryEl.textContent = "目录读取失败,请重试。"; - } - }); + initTree(JSON.parse('${archiveTreeJson?js_string}')); } document.getElementById("retryTree").addEventListener("click", function () { diff --git a/server/src/test/java/cn/keking/web/controller/ArchiveDirectorySecurityTests.java b/server/src/test/java/cn/keking/web/controller/ArchiveDirectorySecurityTests.java new file mode 100644 index 000000000..a8c32cf8a --- /dev/null +++ b/server/src/test/java/cn/keking/web/controller/ArchiveDirectorySecurityTests.java @@ -0,0 +1,84 @@ +package cn.keking.web.controller; + +import cn.keking.config.ConfigConstants; +import cn.keking.model.FileAttribute; +import cn.keking.service.FileHandlerService; +import cn.keking.service.ZtreeNodeVo; +import cn.keking.service.impl.CompressFilePreviewImpl; +import cn.keking.utils.RarUtils; +import freemarker.template.Configuration; +import org.junit.jupiter.api.*; +import org.junit.jupiter.api.io.TempDir; +import org.springframework.test.web.servlet.setup.MockMvcBuilders; +import org.springframework.ui.ExtendedModelMap; + +import java.io.StringWriter; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.*; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +class ArchiveDirectorySecurityTests { + @TempDir Path temp; + private String previousDir; + private Boolean previousCache; + @BeforeEach void setup() throws Exception { + previousDir = ConfigConstants.getFileDir(); + previousCache = ConfigConstants.isCacheEnabled(); + ConfigConstants.setFileDirValue(temp.toString()); + ConfigConstants.setCacheEnabledValueValue(true); + Files.createDirectories(temp.resolve("archive.zip_/nested")); + Files.writeString(temp.resolve("archive.zip_/nested/visible.txt"), "visible"); + Files.writeString(temp.resolve("private-document.pdf"), "secret"); + } + @AfterEach void restore() { + ConfigConstants.setFileDirValue(previousDir); + ConfigConstants.setCacheEnabledValueValue(previousCache); + } + + @Test void publicDirectoryEnumerationRouteIsAbsent() throws Exception { + MockMvcBuilders.standaloneSetup(new FileController()).build() + .perform(get("/directory").param("urls", "aHR0cDovLy8=")) + .andExpect(status().isNotFound()); + } + @Test void treeIsConfinedAndDoesNotFollowLinksOrExposeAbsoluteParents() throws Exception { + Files.createSymbolicLink(temp.resolve("archive.zip_/secret-link"), temp.resolve("private-document.pdf")); + Files.createSymbolicLink(temp.resolve("archive.zip_/loop"), temp.resolve("archive.zip_")); + List tree = RarUtils.getTree("archive.zip_"); + assertEquals("", tree.get(0).pid); + assertEquals(1, tree.get(0).children.size()); + assertEquals("archive.zip_/nested/visible.txt", tree.get(0).children.get(0).children.get(0).id); + assertThrows(SecurityException.class, () -> RarUtils.getTree("")); + assertThrows(SecurityException.class, () -> RarUtils.getTree("/")); + assertThrows(SecurityException.class, () -> RarUtils.getTree("../")); + } + @Test void cachedArchivePreviewRendersItsTreeWithoutAnEnumerationRequest() throws Exception { + Files.writeString(temp.resolve("archive.zip_/quote'\"