fix: harden file download flow

2026-04-30 10:30:26 +08:00
parent 3555d19b26
commit aef59e354a
24 changed files with 2316 additions and 2443 deletions
@@ -0,0 +1,110 @@
+package com.svnlog.web.controller;
+
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.mockito.Mockito;
+import org.springframework.test.web.servlet.MockMvc;
+import org.springframework.test.web.servlet.setup.MockMvcBuilders;
+
+import com.svnlog.web.service.AiWorkflowService;
+import com.svnlog.web.service.HealthService;
+import com.svnlog.web.service.OutputFileService;
+import com.svnlog.web.service.SettingsService;
+import com.svnlog.web.service.SvnPresetService;
+import com.svnlog.web.service.SvnWorkflowService;
+import com.svnlog.web.service.TaskService;
+
+import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
+import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
+
+class AppControllerDownloadTest {
+
+    @TempDir
+    Path tempDir;
+
+    @Test
+    void shouldDownloadMarkdownWithAttachmentHeaders() throws Exception {
+        final OutputFileService outputFileService = buildOutputFileService();
+        final Path file = outputFileService.resolveInOutput("nested/报告 2026.md");
+        Files.createDirectories(file.getParent());
+        Files.write(file, "# hello".getBytes(StandardCharsets.UTF_8));
+
+        buildMockMvc(outputFileService)
+                .perform(get("/api/files/download").param("path", "nested/报告 2026.md"))
+                .andExpect(status().isOk())
+                .andExpect(header().string("X-Content-Type-Options", "nosniff"))
+                .andExpect(header().string("Content-Disposition", Matchers.containsString("attachment;")))
+                .andExpect(header().string("Content-Disposition", Matchers.containsString("filename*=UTF-8''")))
+                .andExpect(content().contentType("text/markdown;charset=UTF-8"))
+                .andExpect(content().string("# hello"));
+    }
+
+    @Test
+    void shouldDownloadExcelWithOfficeContentType() throws Exception {
+        final OutputFileService outputFileService = buildOutputFileService();
+        final byte[] payload = new byte[] { 0x01, 0x02, 0x03, 0x04 };
+        Files.write(outputFileService.resolveInOutput("report.xlsx"), payload);
+
+        final byte[] response = buildMockMvc(outputFileService)
+                .perform(get("/api/files/download").param("path", "report.xlsx"))
+                .andExpect(status().isOk())
+                .andExpect(content().contentType("application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"))
+                .andReturn()
+                .getResponse()
+                .getContentAsByteArray();
+
+        Assertions.assertArrayEquals(payload, response);
+    }
+
+    @Test
+    void shouldReturnStructuredErrorWhenFileMissing() throws Exception {
+        final OutputFileService outputFileService = buildOutputFileService();
+
+        buildMockMvc(outputFileService)
+                .perform(get("/api/files/download").param("path", "missing.md"))
+                .andExpect(status().isBadRequest())
+                .andExpect(jsonPath("$.status").value(400))
+                .andExpect(jsonPath("$.error").value("文件不存在: missing.md"));
+    }
+
+    @Test
+    void shouldRejectPathTraversal() throws Exception {
+        final OutputFileService outputFileService = buildOutputFileService();
+
+        buildMockMvc(outputFileService)
+                .perform(get("/api/files/download").param("path", "../secret.txt"))
+                .andExpect(status().isBadRequest())
+                .andExpect(jsonPath("$.status").value(400))
+                .andExpect(jsonPath("$.error", Matchers.containsString("非法文件路径")));
+    }
+
+    private OutputFileService buildOutputFileService() {
+        final OutputFileService outputFileService = new OutputFileService();
+        outputFileService.setOutputRoot(tempDir.resolve("outputs").toString());
+        return outputFileService;
+    }
+
+    private MockMvc buildMockMvc(OutputFileService outputFileService) {
+        final AppController controller = new AppController(
+                Mockito.mock(SvnWorkflowService.class),
+                Mockito.mock(AiWorkflowService.class),
+                Mockito.mock(TaskService.class),
+                outputFileService,
+                Mockito.mock(SettingsService.class),
+                Mockito.mock(SvnPresetService.class),
+                Mockito.mock(HealthService.class)
+        );
+        return MockMvcBuilders.standaloneSetup(controller)
+                .setControllerAdvice(new GlobalExceptionHandler())
+                .build();
+    }
+}
@@ -21,7 +21,11 @@ class AiWorkflowServiceTest {
    void shouldResolveDeepSeekProviderByDefault() {
        final AiWorkflowService service = new AiWorkflowService(
                buildOutputFileService(),
-                new SettingsService(buildOutputFileService(), new SvnPresetService()),
+                new SettingsService(
+                        buildOutputFileService(),
+                        new SettingsPersistenceService(),
+                        new SvnPresetService()
+                ),
                new AiInputValidator()
        );

@@ -35,7 +39,11 @@ class AiWorkflowServiceTest {
    @Test
    void shouldResolveOpenAiCompatibleModelsAndUrl() {
        final OutputFileService outputFileService = buildOutputFileService();
-        final SettingsService settingsService = new SettingsService(outputFileService, new SvnPresetService());
+        final SettingsService settingsService = new SettingsService(
+                outputFileService,
+                new SettingsPersistenceService(),
+                new SvnPresetService()
+        );
        settingsService.updateSettings(
                null,
                SettingsService.PROVIDER_OPENAI_COMPATIBLE,
@@ -44,6 +52,8 @@ class AiWorkflowServiceTest {
                "deepseek-v4-flash",
                "deepseek-v4-pro",
                null,
+                null,
+                null,
                null
        );
        final AiWorkflowService service = new AiWorkflowService(outputFileService, settingsService, new AiInputValidator());
@@ -75,7 +85,11 @@ class AiWorkflowServiceTest {
    @Test
    void shouldParseCompatibleStreamWhenOnlyContentIsReturned() throws Exception {
        final OutputFileService outputFileService = buildOutputFileService();
-        final SettingsService settingsService = new SettingsService(outputFileService, new SvnPresetService());
+        final SettingsService settingsService = new SettingsService(
+                outputFileService,
+                new SettingsPersistenceService(),
+                new SvnPresetService()
+        );
        settingsService.updateSettings(
                null,
                SettingsService.PROVIDER_OPENAI_COMPATIBLE,
@@ -84,6 +98,8 @@ class AiWorkflowServiceTest {
                "deepseek-v4-flash",
                "deepseek-v4-pro",
                null,
+                null,
+                null,
                null
        );
        final AiWorkflowService service = new AiWorkflowService(outputFileService, settingsService, new AiInputValidator());
@@ -127,7 +143,7 @@ class AiWorkflowServiceTest {
        private final String openaiApiKey;

        private StubSettingsService(OutputFileService outputFileService, String openaiBaseUrl, String openaiApiKey) {
-            super(outputFileService, new SvnPresetService());
+            super(outputFileService, new SettingsPersistenceService(), new SvnPresetService());
            this.openaiBaseUrl = openaiBaseUrl;
            this.openaiApiKey = openaiApiKey;
        }
@@ -26,7 +26,11 @@ class HealthServiceTest {
    void shouldReturnDetailedHealthAfterSettingsExpansion() throws Exception {
        final OutputFileService outputFileService = new OutputFileService();
        outputFileService.setOutputRoot(tempDir.resolve("outputs").toString());
-        final SettingsService settingsService = new SettingsService(outputFileService, new SvnPresetService());
+        final SettingsService settingsService = new SettingsService(
+                outputFileService,
+                new SettingsPersistenceService(),
+                new SvnPresetService()
+        );
        taskService = new TaskService(new TaskPersistenceService(), outputFileService);
        final HealthService healthService = new HealthService(outputFileService, settingsService, taskService);

@@ -1,59 +1,210 @@
 package com.svnlog.web.service;

+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.util.EnumSet;
 import java.util.Map;
-import java.nio.file.Paths;
+import java.util.Set;

-import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Assumptions;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;

+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 class SettingsServiceTest {

    @TempDir
    Path tempDir;

-    @Test
-    void shouldReturnDefaultProviderAndOpenAiDefaults() throws Exception {
-        final OutputFileService outputFileService = new OutputFileService();
-        outputFileService.setOutputRoot(tempDir.resolve("outputs").toString());
-        final SettingsService settingsService = new SettingsService(outputFileService, new SvnPresetService());
+    private String originalUserDir;

-        final Map<String, Object> settings = settingsService.getSettings();
-
-        Assertions.assertEquals(SettingsService.PROVIDER_DEEPSEEK, settings.get("provider"));
-        Assertions.assertEquals("http://127.0.0.1:5001/v1", settings.get("openaiBaseUrl"));
-        Assertions.assertEquals("sk-f8b3f43e1fdd4f50b287050f08a6c7ed", settings.get("openaiApiKey"));
-        Assertions.assertEquals("deepseek-v4-flash", settings.get("openaiStageOneModel"));
-        Assertions.assertEquals("deepseek-v4-pro", settings.get("openaiStageTwoModel"));
+    @AfterEach
+    void restoreUserDir() {
+        if (originalUserDir != null) {
+            System.setProperty("user.dir", originalUserDir);
+        }
    }

    @Test
-    void shouldUpdateProviderAndOpenAiSettings() throws Exception {
-        final OutputFileService outputFileService = new OutputFileService();
-        outputFileService.setOutputRoot(tempDir.resolve("outputs").toString());
-        final SettingsService settingsService = new SettingsService(outputFileService, new SvnPresetService());
-        final String newOutputDir = tempDir.resolve("custom-output").toString();
+    void shouldPersistAndReloadSettingsFromOutputDirectory() throws IOException {
+        useTempWorkingDirectory();
+        final Path customOutputDir = tempDir.resolve("custom-output");

+        final SettingsService settingsService = newSettingsService();
        settingsService.updateSettings(
-                "sk-deepseek-runtime",
+                "deepseek-key",
                SettingsService.PROVIDER_OPENAI_COMPATIBLE,
-                "http://localhost:5001/v1/",
-                "sk-openai-runtime",
-                "deepseek-v4-flash",
-                "deepseek-v4-pro",
-                newOutputDir,
+                "http://localhost:9000/v1",
+                "openai-key",
+                "model-stage-1",
+                "model-stage-2",
+                "svn-user",
+                "svn-pass",
+                customOutputDir.toString(),
                "preset-2"
        );

-        final Map<String, Object> settings = settingsService.getSettings();
+        assertTrue(Files.exists(customOutputDir.resolve("settings.json")));
+        assertTrue(Files.exists(tempDir.resolve("outputs").resolve("settings.json")));

-        Assertions.assertEquals(SettingsService.PROVIDER_OPENAI_COMPATIBLE, settings.get("provider"));
-        Assertions.assertEquals("http://localhost:5001/v1/", settings.get("openaiBaseUrl"));
-        Assertions.assertEquals("sk-openai-runtime", settings.get("openaiApiKey"));
-        Assertions.assertEquals("deepseek-v4-flash", settings.get("openaiStageOneModel"));
-        Assertions.assertEquals("deepseek-v4-pro", settings.get("openaiStageTwoModel"));
-        Assertions.assertEquals("preset-2", settings.get("defaultSvnPresetId"));
-        Assertions.assertEquals(Paths.get(newOutputDir).toAbsolutePath().normalize().toString(), settings.get("outputDir"));
+        final SettingsService reloadedService = newSettingsService();
+        final Map<String, Object> settings = reloadedService.getSettings();
+
+        assertEquals(SettingsService.PROVIDER_OPENAI_COMPATIBLE, settings.get("provider"));
+        assertEquals("http://localhost:9000/v1", settings.get("openaiBaseUrl"));
+        assertEquals("model-stage-1", settings.get("openaiStageOneModel"));
+        assertEquals("model-stage-2", settings.get("openaiStageTwoModel"));
+        assertEquals("svn-user", settings.get("svnUsername"));
+        assertEquals(customOutputDir.toAbsolutePath().normalize().toString(), settings.get("outputDir"));
+        assertEquals("preset-2", settings.get("defaultSvnPresetId"));
+        assertEquals("openai-key", reloadedService.getOpenaiApiKey());
+        assertEquals("deepseek-key", reloadedService.pickActiveKey(null));
+        assertEquals("svn-pass", reloadedService.resolveSvnCredentials(null, null).getPassword());
+    }
+
+    @Test
+    void shouldCreateMissingOutputDirectoryWhenSavingSettings() throws IOException {
+        useTempWorkingDirectory();
+        final Path missingOutputDir = tempDir.resolve("missing-output");
+
+        final SettingsService settingsService = newSettingsService();
+        settingsService.updateSettings(
+                null,
+                SettingsService.PROVIDER_DEEPSEEK,
+                null,
+                null,
+                null,
+                null,
+                null,
+                null,
+                missingOutputDir.toString(),
+                "preset-1"
+        );
+
+        assertTrue(Files.isDirectory(missingOutputDir));
+        assertTrue(Files.exists(missingOutputDir.resolve("settings.json")));
+    }
+
+    @Test
+    void shouldFailWhenOutputPathIsAFile() throws IOException {
+        useTempWorkingDirectory();
+        final Path occupiedPath = tempDir.resolve("occupied-output");
+        Files.write(occupiedPath, "not-a-directory".getBytes(StandardCharsets.UTF_8));
+
+        final SettingsService settingsService = newSettingsService();
+
+        final IllegalStateException exception = assertThrows(
+                IllegalStateException.class,
+                () -> settingsService.updateSettings(
+                        null,
+                        SettingsService.PROVIDER_DEEPSEEK,
+                        null,
+                        null,
+                        null,
+                        null,
+                        null,
+                        null,
+                        occupiedPath.toString(),
+                        "preset-1"
+                )
+        );
+
+        assertTrue(exception.getMessage().contains("输出路径不是目录"));
+        assertEquals(tempDir.resolve("outputs").toAbsolutePath().normalize().toString(),
+                settingsService.getSettings().get("outputDir"));
+    }
+
+    @Test
+    void shouldFailWhenOutputDirectoryIsNotWritable() throws IOException {
+        useTempWorkingDirectory();
+        final Path readOnlyDir = tempDir.resolve("read-only-output");
+        Files.createDirectories(readOnlyDir);
+
+        final Set<PosixFilePermission> originalPermissions = Files.getPosixFilePermissions(readOnlyDir);
+        Files.setPosixFilePermissions(readOnlyDir, EnumSet.of(
+                PosixFilePermission.OWNER_READ,
+                PosixFilePermission.OWNER_EXECUTE
+        ));
+        Assumptions.assumeFalse(Files.isWritable(readOnlyDir), "Test requires a non-writable directory");
+
+        final SettingsService settingsService = newSettingsService();
+        try {
+            final IllegalStateException exception = assertThrows(
+                    IllegalStateException.class,
+                    () -> settingsService.updateSettings(
+                            null,
+                            SettingsService.PROVIDER_DEEPSEEK,
+                            null,
+                            null,
+                            null,
+                            null,
+                            null,
+                            null,
+                            readOnlyDir.toString(),
+                            "preset-1"
+                    )
+            );
+
+            assertTrue(exception.getMessage().contains("输出目录不可写"));
+            assertEquals(tempDir.resolve("outputs").toAbsolutePath().normalize().toString(),
+                    settingsService.getSettings().get("outputDir"));
+        } finally {
+            Files.setPosixFilePermissions(readOnlyDir, originalPermissions);
+        }
+    }
+
+    @Test
+    void shouldReturnSanitizedSettingsWhileKeepingRecoveredSecretsInRuntime() throws IOException {
+        useTempWorkingDirectory();
+
+        final SettingsService settingsService = newSettingsService();
+        settingsService.updateSettings(
+                "deepseek-key",
+                SettingsService.PROVIDER_DEEPSEEK,
+                "http://localhost:5001/v1",
+                "openai-key",
+                "stage-1",
+                "stage-2",
+                "svn-user",
+                "svn-pass",
+                tempDir.resolve("runtime-output").toString(),
+                "preset-1"
+        );
+
+        final SettingsService reloadedService = newSettingsService();
+        final Map<String, Object> settings = reloadedService.getSettings();
+
+        assertFalse(settings.containsKey("apiKey"));
+        assertFalse(settings.containsKey("openaiApiKey"));
+        assertFalse(settings.containsKey("svnPassword"));
+        assertEquals(Boolean.TRUE, settings.get("apiKeyConfigured"));
+        assertEquals(Boolean.TRUE, settings.get("openaiApiKeyConfigured"));
+        assertEquals(Boolean.TRUE, settings.get("svnCredentialsConfigured"));
+        assertEquals("deepseek-key", reloadedService.pickActiveKey(null));
+        assertEquals("openai-key", reloadedService.getOpenaiApiKey());
+        assertEquals("svn-pass", reloadedService.resolveSvnCredentials(null, null).getPassword());
+        assertNotNull(settings.get("apiKeySource"));
+    }
+
+    private SettingsService newSettingsService() {
+        final OutputFileService outputFileService = new OutputFileService();
+        outputFileService.setOutputRoot(tempDir.resolve("outputs").toString());
+        final SettingsPersistenceService settingsPersistenceService = new SettingsPersistenceService();
+        final SvnPresetService svnPresetService = new SvnPresetService();
+        return new SettingsService(outputFileService, settingsPersistenceService, svnPresetService);
+    }
+
+    private void useTempWorkingDirectory() {
+        originalUserDir = System.getProperty("user.dir");
+        System.setProperty("user.dir", tempDir.toAbsolutePath().normalize().toString());
    }
 }