fix(files): authorize ownership before loading generated content

This commit is contained in:
mateaix 2026-09-14 00:14:34 +08:00
parent 7e45a26755
commit ea1a28399c
4 changed files with 166 additions and 75 deletions

View File

@ -279,39 +279,35 @@ public class GeneratedFileCache {
* JVM restarts; expired entries are removed as a side-effect.
*/
public Optional<Entry> 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<Owner> 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<Entry> getForWorkspace(String id, @Nullable Long workspaceId) {
Optional<Entry> 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<Owner> 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);
}
}

View File

@ -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))
.<ResponseEntity<?>>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;
}

View File

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

View File

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