diff --git a/mateclaw-server/src/main/java/vip/mate/tool/document/GeneratedFileCache.java b/mateclaw-server/src/main/java/vip/mate/tool/document/GeneratedFileCache.java index 3be55b83..59ca69dd 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/document/GeneratedFileCache.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/document/GeneratedFileCache.java @@ -279,39 +279,35 @@ public class GeneratedFileCache { * JVM restarts; expired entries are removed as a side-effect. */ public Optional get(String id) { + return Optional.ofNullable(getAuthorized(id, owner -> true).entry()); + } + + public enum AccessStatus { FOUND, FORBIDDEN, MISSING } + public record AccessResult(AccessStatus status, @Nullable Entry entry) { } + + /** Authorize ownership metadata before reading or caching a cold content body. */ + public AccessResult getAuthorized(String id, java.util.function.Predicate authorized) { + Objects.requireNonNull(authorized, "authorized"); if (id == null || !ID_RE.matcher(id).matches()) { - return Optional.empty(); + return new AccessResult(AccessStatus.MISSING, null); } - Entry entry = entries.get(id); - if (entry == null) { - entry = loadFromDisk(id); - if (entry != null) { - entries.put(id, entry); + Entry cached = entries.get(id); + if (cached != null) { + if (cached.expired()) { + evict(id); + return new AccessResult(AccessStatus.MISSING, null); } + return authorized.test(new Owner(cached.workspaceId(), cached.ownerUserId(), cached.conversationId())) + ? new AccessResult(AccessStatus.FOUND, cached) : new AccessResult(AccessStatus.FORBIDDEN, null); } - if (entry == null) { - return Optional.empty(); - } - if (entry.expired()) { - evict(id); - return Optional.empty(); - } - return Optional.of(entry); + AccessResult loaded = loadAuthorizedFromDisk(id, authorized); + if (loaded.entry() != null) entries.put(id, loaded.entry()); + return loaded; } public Optional getForWorkspace(String id, @Nullable Long workspaceId) { - Optional entry = get(id); - if (entry.isEmpty()) { - return Optional.empty(); - } - Long ownerWorkspaceId = entry.get().workspaceId(); - if (ownerWorkspaceId == null) { - return entry; - } - if (workspaceId == null || !ownerWorkspaceId.equals(workspaceId)) { - return Optional.empty(); - } - return entry; + return Optional.ofNullable(getAuthorized(id, owner -> owner.workspaceId() == null + || Objects.equals(owner.workspaceId(), workspaceId)).entry()); } /** @@ -503,29 +499,53 @@ public class GeneratedFileCache { } private Entry loadFromDisk(String id) { + return loadAuthorizedFromDisk(id, owner -> true).entry(); + } + + private AccessResult loadAuthorizedFromDisk(String id, java.util.function.Predicate authorized) { + AccessResult missing = new AccessResult(AccessStatus.MISSING, null); Path bin = storageDir.resolve(id).normalize(); Path meta = storageDir.resolve(id + META_SUFFIX).normalize(); - // Containment guard — id is already validated, this is defence in depth. + // NOFOLLOW also applies at open; parent-directory ownership is separate. if (!bin.startsWith(storageDir) || !Files.isRegularFile(bin, LinkOption.NOFOLLOW_LINKS) - || !Files.isRegularFile(meta, LinkOption.NOFOLLOW_LINKS)) { - return null; + || !Files.isRegularFile(meta, LinkOption.NOFOLLOW_LINKS)) return missing; + Metadata before; + try { + before = readDownloadMetadata(meta, id); + } catch (IOException | RuntimeException e) { + log.debug("Could not read generated file metadata id={}: {}", id, e.toString()); + return missing; + } + if (before.expireAt() <= System.currentTimeMillis()) { + evict(id); + return missing; + } + // Keep permission-provider errors distinct from unavailable storage. + if (!authorized.test(new Owner(before.workspaceId(), before.ownerUserId(), before.conversationId()))) { + return new AccessResult(AccessStatus.FORBIDDEN, null); } try { - // Refuse leaf symlinks again at open, including replacement after the - // regular-file check. Parent-directory ownership is a separate boundary. - Metadata parsed; - try (var input = Files.newInputStream(meta, StandardOpenOption.READ, LinkOption.NOFOLLOW_LINKS)) { - parsed = parseMeta(new String(input.readAllBytes(), StandardCharsets.UTF_8), id); - } + // Authorization can take time. Refuse observed metadata replacement + // before opening the body and again before making it available. + if (!before.equals(readDownloadMetadata(meta, id))) return missing; byte[] bytes; try (var input = Files.newInputStream(bin, StandardOpenOption.READ, LinkOption.NOFOLLOW_LINKS)) { bytes = input.readAllBytes(); } - return new Entry(bytes, parsed.filename(), parsed.mimeType(), parsed.expireAt(), - parsed.workspaceId(), parsed.ownerUserId(), parsed.conversationId()); - } catch (Exception e) { - log.warn("Could not load generated file id={}: {}", id, e.toString()); - return null; + if (!before.equals(readDownloadMetadata(meta, id)) + || before.expireAt() <= System.currentTimeMillis()) return missing; + Entry entry = new Entry(bytes, before.filename(), before.mimeType(), before.expireAt(), + before.workspaceId(), before.ownerUserId(), before.conversationId()); + return new AccessResult(AccessStatus.FOUND, entry); + } catch (IOException | RuntimeException e) { + log.debug("Could not load generated file id={}: {}", id, e.toString()); + return missing; + } + } + + private Metadata readDownloadMetadata(Path meta, String id) throws IOException { + try (var input = Files.newInputStream(meta, StandardOpenOption.READ, LinkOption.NOFOLLOW_LINKS)) { + return parseMeta(new String(input.readAllBytes(), StandardCharsets.UTF_8), id); } } diff --git a/mateclaw-server/src/main/java/vip/mate/tool/document/GeneratedFileController.java b/mateclaw-server/src/main/java/vip/mate/tool/document/GeneratedFileController.java index f746c630..f8b5af5c 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/document/GeneratedFileController.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/document/GeneratedFileController.java @@ -45,39 +45,41 @@ public class GeneratedFileController { if (user == null) { return ResponseEntity.status(401).body(Map.of("error", "Unauthorized")); } - return cache.get(id) - .filter(entry -> canDownload(entry, workspaceId, user)) - .>map(entry -> { - String encodedName = URLEncoder.encode(entry.filename(), StandardCharsets.UTF_8) - .replace("+", "%20"); - HttpHeaders headers = new HttpHeaders(); - String mime = entry.mimeType() == null || entry.mimeType().isBlank() - ? "application/octet-stream" - : entry.mimeType(); - headers.setContentType(MediaType.parseMediaType(mime)); - // RFC 5987 filename* lets non-ASCII names round-trip in browsers. - // Images and HTML previews render inline; everything else downloads. - boolean isImage = mime != null && mime.startsWith("image/"); - boolean isHtml = mime != null && mime.toLowerCase().startsWith("text/html"); - String disposition = (isImage || isHtml) ? "inline" : "attachment"; - // Every generated document is untrusted, including SVG served - // inline as an image. Isolate document origins and active content; - // retain static styles/media and explicit downloads for previews. - headers.add("Content-Security-Policy", - "sandbox allow-downloads; default-src 'none'; img-src * data:; " - + "style-src 'unsafe-inline'; font-src * data:; media-src *; " - + "base-uri 'none'; form-action 'none'"); - headers.add("X-Content-Type-Options", "nosniff"); - headers.add(HttpHeaders.CONTENT_DISPOSITION, - disposition + "; filename=\"" + sanitizeAscii(entry.filename()) - + "\"; filename*=UTF-8''" + encodedName); - byte[] content = entry.bytes(); - headers.setContentLength(content.length); - return ResponseEntity.ok().headers(headers).body(content); - }) - .orElseGet(() -> cache.get(id).isPresent() - ? ResponseEntity.status(403).body(Map.of("error", "Workspace permission denied")) - : ResponseEntity.status(404).body(Map.of("error", "File not found or expired"))); + var access = cache.getAuthorized(id, owner -> canDownload(owner.workspaceId(), workspaceId, user)); + if (access.status() == GeneratedFileCache.AccessStatus.FORBIDDEN) { + return ResponseEntity.status(403).body(Map.of("error", "Workspace permission denied")); + } + if (access.status() == GeneratedFileCache.AccessStatus.MISSING) { + return ResponseEntity.status(404).body(Map.of("error", "File not found or expired")); + } + var entry = access.entry(); + String encodedName = URLEncoder.encode(entry.filename(), StandardCharsets.UTF_8) + .replace("+", "%20"); + HttpHeaders headers = new HttpHeaders(); + String mime = entry.mimeType() == null || entry.mimeType().isBlank() + ? "application/octet-stream" + : entry.mimeType(); + headers.setContentType(MediaType.parseMediaType(mime)); + // RFC 5987 filename* lets non-ASCII names round-trip in browsers. + // Images and HTML previews render inline; everything else downloads. + boolean isImage = mime != null && mime.startsWith("image/"); + boolean isHtml = mime != null && mime.toLowerCase().startsWith("text/html"); + String disposition = (isImage || isHtml) ? "inline" : "attachment"; + // Every generated document is untrusted, including SVG served + // inline as an image. Isolate document origins and active content; + // retain static styles/media and explicit downloads for previews. + headers.add("Content-Security-Policy", + "sandbox allow-downloads; default-src 'none'; img-src * data:; " + + "style-src 'unsafe-inline'; font-src * data:; media-src *; " + + "base-uri 'none'; form-action 'none'"); + headers.add("X-Content-Type-Options", "nosniff"); + headers.add(HttpHeaders.CONTENT_DISPOSITION, + disposition + "; filename=\"" + sanitizeAscii(entry.filename()) + + "\"; filename*=UTF-8''" + encodedName); + byte[] content = entry.bytes(); + headers.setContentLength(content.length); + return ResponseEntity.ok().headers(headers).body(content); + } private UserEntity resolveUser(Authentication authentication) { @@ -87,8 +89,7 @@ public class GeneratedFileController { return authService.findByUsername(authentication.getName()); } - private boolean canDownload(GeneratedFileCache.Entry entry, Long currentWorkspaceId, UserEntity user) { - Long ownerWorkspaceId = entry.workspaceId(); + private boolean canDownload(Long ownerWorkspaceId, Long currentWorkspaceId, UserEntity user) { if (ownerWorkspaceId == null) { return true; } diff --git a/mateclaw-server/src/test/java/vip/mate/tool/document/GeneratedFileCachePersistenceTest.java b/mateclaw-server/src/test/java/vip/mate/tool/document/GeneratedFileCachePersistenceTest.java index 8e51832b..375d5716 100644 --- a/mateclaw-server/src/test/java/vip/mate/tool/document/GeneratedFileCachePersistenceTest.java +++ b/mateclaw-server/src/test/java/vip/mate/tool/document/GeneratedFileCachePersistenceTest.java @@ -20,6 +20,16 @@ import static org.junit.jupiter.api.Assertions.*; */ class GeneratedFileCachePersistenceTest { + @Test + void forbiddenWorkspaceLookupDoesNotPopulateColdCache(@TempDir Path dir) { + String id = new GeneratedFileCache(dir).put("body".getBytes(StandardCharsets.UTF_8), "report.txt", "text/plain", + new GeneratedFileCache.Owner(20L, 30L, "conv")); + GeneratedFileCache cold = new GeneratedFileCache(dir); + assertTrue(cold.getForWorkspace(id, 40L).isEmpty()); + assertTrue(((java.util.Map) org.springframework.test.util.ReflectionTestUtils.getField(cold, "entries")).isEmpty()); + assertTrue(cold.getForWorkspace(id, 20L).isPresent()); + } + @Test void coldDownloadRejectsSymbolicLinkToExternalContent(@TempDir Path dir) throws IOException { Path storage = Files.createDirectory(dir.resolve("cache")); diff --git a/mateclaw-server/src/test/java/vip/mate/tool/document/GeneratedFileControllerTest.java b/mateclaw-server/src/test/java/vip/mate/tool/document/GeneratedFileControllerTest.java index 21b7b851..3fc4f980 100644 --- a/mateclaw-server/src/test/java/vip/mate/tool/document/GeneratedFileControllerTest.java +++ b/mateclaw-server/src/test/java/vip/mate/tool/document/GeneratedFileControllerTest.java @@ -4,6 +4,9 @@ import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.test.util.ReflectionTestUtils; +import java.util.Map; import org.junit.jupiter.api.io.TempDir; import org.springframework.http.ResponseEntity; import org.springframework.security.authentication.TestingAuthenticationToken; @@ -13,6 +16,7 @@ import vip.mate.workspace.core.service.WorkspaceService; import java.nio.charset.StandardCharsets; import java.nio.file.Path; +import java.nio.file.Files; import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.mock; @@ -82,6 +86,62 @@ class GeneratedFileControllerTest { assertFalse(policy.contains("allow-same-origin")); } + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void coldDownloadAuthorizesBeforePopulatingContentCache(boolean allowed, @TempDir Path dir) { + String id = new GeneratedFileCache(dir).put("body".getBytes(StandardCharsets.UTF_8), "report.txt", "text/plain", + new GeneratedFileCache.Owner(20L, 30L, "conv")); + GeneratedFileCache cold = new GeneratedFileCache(dir); + Map entries = (Map) ReflectionTestUtils.getField(cold, "entries"); + AuthService authService = mock(AuthService.class); + WorkspaceService workspaceService = mock(WorkspaceService.class); + when(authService.findByUsername("alice")).thenReturn(user(30L, "user")); + when(workspaceService.hasPermissionCached(20L, 30L, "viewer")).thenAnswer(call -> { + assertTrue(entries.isEmpty(), "content must not be loaded into cache before authorization"); + return allowed; + }); + var response = new GeneratedFileController(cold, authService, workspaceService) + .download(id, 20L, new TestingAuthenticationToken("alice", "pw")); + assertEquals(allowed ? 200 : 403, response.getStatusCode().value()); + assertEquals(allowed, entries.containsKey(id)); + } + + @Test + void coldDownloadRejectsOwnershipReplacementDuringPermissionCheck(@TempDir Path dir) { + String id = new GeneratedFileCache(dir).put("body".getBytes(StandardCharsets.UTF_8), "report.txt", "text/plain", + new GeneratedFileCache.Owner(20L, 30L, "conv")); + GeneratedFileCache cold = new GeneratedFileCache(dir); + AuthService authService = mock(AuthService.class); + WorkspaceService workspaceService = mock(WorkspaceService.class); + when(authService.findByUsername("alice")).thenReturn(user(30L, "user")); + when(workspaceService.hasPermissionCached(20L, 30L, "viewer")).thenAnswer(call -> { + Path meta = dir.resolve(id + ".meta"); + String[] fields = Files.readString(meta).split("\t", -1); + fields[3] = "40"; + Files.writeString(meta, String.join("\t", fields)); + Files.writeString(dir.resolve(id), "other workspace body"); + return true; + }); + var response = new GeneratedFileController(cold, authService, workspaceService) + .download(id, 20L, new TestingAuthenticationToken("alice", "pw")); + assertEquals(404, response.getStatusCode().value()); + assertTrue(((Map) ReflectionTestUtils.getField(cold, "entries")).isEmpty()); + } + + @Test + void legacyUnscopedColdFileRemainsAccessibleToAuthenticatedCaller(@TempDir Path dir) { + String id = new GeneratedFileCache(dir).put("legacy".getBytes(StandardCharsets.UTF_8), "report.txt", "text/plain"); + GeneratedFileCache cold = new GeneratedFileCache(dir); + AuthService authService = mock(AuthService.class); + WorkspaceService workspaceService = mock(WorkspaceService.class); + when(authService.findByUsername("alice")).thenReturn(user(30L, "user")); + var controller = new GeneratedFileController(cold, authService, workspaceService); + assertEquals(401, controller.download(id, null, null).getStatusCode().value()); + assertTrue(((Map) ReflectionTestUtils.getField(cold, "entries")).isEmpty()); + assertEquals(200, controller.download(id, null, new TestingAuthenticationToken("alice", "pw")).getStatusCode().value()); + org.mockito.Mockito.verifyNoInteractions(workspaceService); + } + private static UserEntity user(Long id, String role) { UserEntity user = new UserEntity(); user.setId(id);