diff --git a/server/build.gradle b/server/build.gradle index 06f71a5a7..bee01674c 100644 --- a/server/build.gradle +++ b/server/build.gradle @@ -276,7 +276,10 @@ testlogger { } test { - jvmArgs = ['--enable-native-access=ALL-UNNAMED', '-Xmx4096m', '-Xshare:off'] // due to https://github.com/netty/netty/issues/15161 + // -Xmx raised from 4096m: the full suite caches many distinct Spring test contexts + // (@SpringBootTest/@WebMvcTest each get their own), and 4 GB was occasionally too tight, + // observed as an OutOfMemoryError during unrelated context bootstrapping on CI. + jvmArgs = ['--enable-native-access=ALL-UNNAMED', '-Xmx6144m', '-Xshare:off'] // due to https://github.com/netty/netty/issues/15161 useJUnitPlatform() } diff --git a/server/src/main/java/org/eclipse/openvsx/adapter/WebResourceService.java b/server/src/main/java/org/eclipse/openvsx/adapter/WebResourceService.java index a53ed6642..168845825 100644 --- a/server/src/main/java/org/eclipse/openvsx/adapter/WebResourceService.java +++ b/server/src/main/java/org/eclipse/openvsx/adapter/WebResourceService.java @@ -18,8 +18,10 @@ import java.util.zip.ZipFile; import io.micrometer.observation.annotation.Observed; +import org.apache.commons.io.FileUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Value; import org.springframework.cache.annotation.Cacheable; import org.springframework.http.HttpStatus; import org.springframework.stereotype.Service; @@ -34,6 +36,7 @@ import org.eclipse.openvsx.util.ErrorResultException; import org.eclipse.openvsx.util.FileUtil; import org.eclipse.openvsx.util.NamingUtil; +import org.eclipse.openvsx.util.SizeLimitInputStream; import org.eclipse.openvsx.util.UrlUtil; import static org.eclipse.openvsx.cache.CacheService.*; @@ -48,17 +51,29 @@ public class WebResourceService { private final FilesCacheKeyGenerator filesCacheKeyGenerator; private final JsonMapper jsonMapper; + // Limit the decompressed size of a single served web resource. Disabled (unbounded, matching + // pre-existing behavior) by default: unlike the handful of known-small metadata files bounded by + // ArchiveUtil.MAX_ENTRY_SIZE at publish time, an arbitrary file requested through /vscode/unpkg + // can legitimately be large (e.g. a bundled WASM binary), so there's no one-size-fits-all default + // that doesn't risk rejecting something that published successfully. A negative value disables the + // limit; set a positive byte value to opt into bounding it, e.g. to guard against a small VSIX + // containing a highly compressed entry that could exhaust the java.io.tmpdir filesystem when + // extracted on request. + private final long maxFileSize; + public WebResourceService( StorageUtilService storageUtil, RepositoryService repositories, CacheService cache, - FilesCacheKeyGenerator filesCacheKeyGenerator + FilesCacheKeyGenerator filesCacheKeyGenerator, + @Value("${ovsx.caching.files-webresource.max-file-size:-1}") long maxFileSize ) { this.storageUtil = storageUtil; this.repositories = repositories; this.cache = cache; this.filesCacheKeyGenerator = filesCacheKeyGenerator; this.jsonMapper = JsonMapper.shared(); + this.maxFileSize = maxFileSize; } public Path getExtensionDownload(String namespace, String extension, String targetPlatform, String version) { @@ -154,9 +169,31 @@ private String getFileExtension(ZipEntry fileEntry) { } private void writeBinaryFile(Path file, ZipFile zip, ZipEntry fileEntry) { + var declaredSize = fileEntry.getSize(); + if (maxFileSize >= 0) { + if (declaredSize < 0) { + throw new ErrorResultException("The file " + fileEntry.getName() + " has an unknown size."); + } + if (declaredSize > maxFileSize) { + var maxSize = FileUtils.byteCountToDisplaySize(maxFileSize); + throw new ErrorResultException( + "The file " + fileEntry.getName() + " exceeds the size limit of " + maxSize + ".", + HttpStatus.CONTENT_TOO_LARGE); + } + } + FileUtil.writeSync(file, p -> { try (var in = zip.getInputStream(fileEntry)) { - Files.copy(in, p); + if (maxFileSize >= 0) { + // Wrap in SizeLimitInputStream bounded to the declared size: a zip entry can lie + // about its uncompressed size, so this stops the copy the moment more bytes than + // declared have been read instead of trusting the header. + try (var limited = new SizeLimitInputStream(in, declaredSize)) { + Files.copy(limited, p); + } + } else { + Files.copy(in, p); + } } catch (IOException e) { throw new UncheckedIOException(e); } diff --git a/server/src/main/java/org/eclipse/openvsx/cache/CacheConfig.java b/server/src/main/java/org/eclipse/openvsx/cache/CacheConfig.java index edafe3b0c..2ee919e5b 100644 --- a/server/src/main/java/org/eclipse/openvsx/cache/CacheConfig.java +++ b/server/src/main/java/org/eclipse/openvsx/cache/CacheConfig.java @@ -75,12 +75,21 @@ public Cache extensionCache( @Bean public Cache webResourceCache( @Value("${ovsx.caching.files-webresource.tti:PT1H}") Duration timeToIdle, - @Value("${ovsx.caching.files-webresource.max-size:150}") long maxSize + @Value("${ovsx.caching.files-webresource.max-size:150}") long maxEntries, + // 2 GiB total; each entry's weight is its file size in bytes rather than a flat 1, so + // this bounds the cache's disk footprint instead of just the number of cached files. + @Value("${ovsx.caching.files-webresource.max-total-size:2147483648}") long maxTotalSize ) { + // Charge every entry at least this much of the weight budget, so at most maxEntries entries + // can ever fit regardless of how small they are -- the same cache then respects both limits: + // total bytes stay within maxTotalSize (a file's real size still counts when it's the larger + // of the two), and the entry count stays within maxEntries (its floor share of the budget). + var floorWeight = maxEntries > 0 ? Math.ceilDiv(maxTotalSize, maxEntries) : 0; return Caffeine.newBuilder() .removalListener(new ExpiredFileListener()) .expireAfterAccess(timeToIdle) - .maximumSize(maxSize) + .maximumWeight(maxTotalSize) + .weigher(new FileSizeWeigher(floorWeight)) .scheduler(Scheduler.systemScheduler()) .recordStats() .build(); diff --git a/server/src/main/java/org/eclipse/openvsx/cache/FileSizeWeigher.java b/server/src/main/java/org/eclipse/openvsx/cache/FileSizeWeigher.java new file mode 100644 index 000000000..5cbe23e53 --- /dev/null +++ b/server/src/main/java/org/eclipse/openvsx/cache/FileSizeWeigher.java @@ -0,0 +1,64 @@ +/****************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation. + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0. + * + * SPDX-License-Identifier: EPL-2.0 + *****************************************************************************/ +package org.eclipse.openvsx.cache; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; + +import com.github.benmanes.caffeine.cache.Weigher; +import org.jspecify.annotations.NonNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Weighs a cached {@link Path} by its file size in bytes, so a {@code maximumWeight} cache bounds + * its total disk footprint instead of just the number of cached files. + *

+ * Every entry is floored to at least {@code floorWeight} of the weight budget, even if its actual + * file is smaller. Combined with a cache's {@code maximumWeight(maxTotalSize)}, setting + * {@code floorWeight = ceil(maxTotalSize / maxEntries)} makes the same cache respect both a total + * byte budget and a maximum entry count at once: no more than {@code maxEntries} entries can ever + * fit, since each one claims at least a {@code 1/maxEntries} share of the budget, while entries + * larger than that share are still weighed by their real size so the byte budget itself holds. + */ +public class FileSizeWeigher implements Weigher { + private static final Logger logger = LoggerFactory.getLogger(FileSizeWeigher.class); + + private final long floorWeight; + + /** + * @param floorWeight the minimum weight charged to every entry, regardless of its actual file + * size; pass {@code 0} to weigh purely by actual file size. + */ + public FileSizeWeigher(long floorWeight) { + this.floorWeight = floorWeight; + } + + @Override + public int weigh(@NonNull Object key, @NonNull Object value) { + if (!(value instanceof Path path)) { + return 1; + } + + try { + var size = Math.max(Files.size(path), floorWeight); + return (int) Math.min(size, Integer.MAX_VALUE); + } catch (IOException e) { + // Can't determine the size, e.g. the file was already deleted; weigh it heavily so + // it doesn't linger and displace entries whose size is known. + logger.warn("Failed to determine size of cached file {}", path, e); + return Integer.MAX_VALUE; + } + } +} diff --git a/server/src/main/java/org/eclipse/openvsx/util/FileUtil.java b/server/src/main/java/org/eclipse/openvsx/util/FileUtil.java index 1b70d9ac0..190228446 100644 --- a/server/src/main/java/org/eclipse/openvsx/util/FileUtil.java +++ b/server/src/main/java/org/eclipse/openvsx/util/FileUtil.java @@ -9,6 +9,7 @@ * ****************************************************************************** */ package org.eclipse.openvsx.util; +import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.util.Collections; @@ -16,8 +17,13 @@ import java.util.Map; import java.util.function.Consumer; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + public class FileUtil { + private static final Logger logger = LoggerFactory.getLogger(FileUtil.class); + private static final Map LOCKS; static { @@ -34,7 +40,10 @@ private FileUtil() { } /*** - * Write to file synchronously, if it doesn't already exist. + * Write to file synchronously, if it doesn't already exist. If the writer fails partway + * through, the partial file is deleted so a later call doesn't mistake it for a completed + * write (writeSync only (re)invokes the writer when the path doesn't yet exist) and so it + * doesn't linger on disk. * @param path File path to write to * @param writer Writes to file */ @@ -45,8 +54,21 @@ public static void writeSync(Path path, Consumer writer) { } synchronized (lock) { if (!Files.exists(path)) { - writer.accept(path); + try { + writer.accept(path); + } catch (RuntimeException e) { + deleteQuietly(path); + throw e; + } } } } + + private static void deleteQuietly(Path path) { + try { + Files.deleteIfExists(path); + } catch (IOException e) { + logger.warn("Failed to delete partial file {}", path, e); + } + } } diff --git a/server/src/test/java/org/eclipse/openvsx/adapter/VSCodeAPITest.java b/server/src/test/java/org/eclipse/openvsx/adapter/VSCodeAPITest.java index 7fa314e24..448644a7a 100644 --- a/server/src/test/java/org/eclipse/openvsx/adapter/VSCodeAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/adapter/VSCodeAPITest.java @@ -1478,7 +1478,7 @@ WebResourceService webResourceService( CacheService cache, FilesCacheKeyGenerator filesCacheKeyGenerator ) { - return new WebResourceService(storageUtil, repositories, cache, filesCacheKeyGenerator); + return new WebResourceService(storageUtil, repositories, cache, filesCacheKeyGenerator, 33_554_432L); } @Bean diff --git a/server/src/test/java/org/eclipse/openvsx/adapter/WebResourceServiceTest.java b/server/src/test/java/org/eclipse/openvsx/adapter/WebResourceServiceTest.java new file mode 100644 index 000000000..8f4b729fb --- /dev/null +++ b/server/src/test/java/org/eclipse/openvsx/adapter/WebResourceServiceTest.java @@ -0,0 +1,138 @@ +/******************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0 + * + * SPDX-License-Identifier: EPL-2.0 + ********************************************************************************/ +package org.eclipse.openvsx.adapter; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; + +import org.junit.jupiter.api.Test; + +import org.eclipse.openvsx.cache.CacheService; +import org.eclipse.openvsx.cache.FilesCacheKeyGenerator; +import org.eclipse.openvsx.repositories.RepositoryService; +import org.eclipse.openvsx.storage.StorageUtilService; +import org.eclipse.openvsx.util.ErrorResultException; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; +import static org.springframework.http.HttpStatus.CONTENT_TOO_LARGE; +import static org.springframework.http.HttpStatus.INTERNAL_SERVER_ERROR; + +class WebResourceServiceTest { + + private final FilesCacheKeyGenerator filesCacheKeyGenerator = new FilesCacheKeyGenerator(); + + @Test + void testRejectsEntryDeclaredLargerThanLimit() throws Exception { + var extensionDownloadPath = resourcePath("wrong-size.zip"); + var service = newService(8192L); + + assertThatThrownBy( + () -> service.getWebResource( + "ns", + "ext", + null, + "1.0.0", + "extension/README.md", + extensionDownloadPath)) + .isExactlyInstanceOf(ErrorResultException.class) + .hasMessage("The file extension/README.md exceeds the size limit of 8 KB.") + .satisfies(e -> assertThat(((ErrorResultException) e).getStatus()).isEqualTo(CONTENT_TOO_LARGE)); + + // rejected before any extraction: nothing should have been written to the file cache + var cachedPath = filesCacheKeyGenerator + .generateCachedWebResourcePath("ns", "ext", null, "1.0.0", "extension/README.md", ".md"); + assertThat(Files.exists(cachedPath)).isFalse(); + } + + @Test + void testStopsCopyWhenActualBytesExceedDeclaredSize() throws Exception { + // extension/package.json declares a size of 0 but its compressed payload decompresses to + // more than 0 bytes, simulating a zip entry whose header lies about the decompressed size + var extensionDownloadPath = resourcePath("wrong-size.zip"); + var service = newService(33_554_432L); + + assertThatThrownBy( + () -> service.getWebResource( + "ns", + "ext", + null, + "1.0.0", + "extension/package.json", + extensionDownloadPath)) + .isExactlyInstanceOf(ErrorResultException.class) + .hasMessageContaining("File size exceeds limit of 0 bytes") + .satisfies(e -> assertThat(((ErrorResultException) e).getStatus()).isEqualTo(INTERNAL_SERVER_ERROR)); + + // the partial file written before the limit tripped must not be left behind + var cachedPath = filesCacheKeyGenerator + .generateCachedWebResourcePath("ns", "ext", null, "1.0.0", "extension/package.json", ".json"); + assertThat(Files.exists(cachedPath)).isFalse(); + } + + @Test + void testAllowsLargeFileWhenLimitIsDisabled() throws Exception { + // extension/README.md declares 31781 bytes -- larger than the 8 KB limit rejected in + // testRejectsEntryDeclaredLargerThanLimit -- but a negative maxFileSize (the default) means + // no file is too large, matching the pre-existing behavior of this endpoint. + var extensionDownloadPath = resourcePath("wrong-size.zip"); + var service = newService(-1L); + + var cachedPath = service.getWebResource( + "ns", + "ext", + null, + "1.0.0", + "extension/README.md", + extensionDownloadPath); + + assertThat(cachedPath).isNotNull(); + assertThat(Files.size(cachedPath)).isEqualTo(31781); + Files.deleteIfExists(cachedPath); + } + + @Test + void testServesFileWithinLimit() throws Exception { + var extensionDownloadPath = resourcePath("todo-tree.zip"); + var service = newService(33_554_432L); + + var cachedPath = service.getWebResource( + "ns", + "ext", + null, + "1.0.0", + "extension/package.json", + extensionDownloadPath); + + assertThat(cachedPath).isNotNull(); + assertThat(Files.size(cachedPath)).isEqualTo(44712); + Files.deleteIfExists(cachedPath); + } + + private WebResourceService newService(long maxFileSize) { + return new WebResourceService( + mock(StorageUtilService.class), + mock(RepositoryService.class), + mock(CacheService.class), + filesCacheKeyGenerator, + maxFileSize); + } + + private Path resourcePath(String name) throws Exception { + var url = getClass().getResource("/org/eclipse/openvsx/util/" + name); + assertThat(url).isNotNull(); + return Paths.get(url.toURI()); + } +} diff --git a/server/src/test/java/org/eclipse/openvsx/cache/FileSizeWeigherTest.java b/server/src/test/java/org/eclipse/openvsx/cache/FileSizeWeigherTest.java new file mode 100644 index 000000000..47a7dd2fb --- /dev/null +++ b/server/src/test/java/org/eclipse/openvsx/cache/FileSizeWeigherTest.java @@ -0,0 +1,69 @@ +/******************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0 + * + * SPDX-License-Identifier: EPL-2.0 + ********************************************************************************/ +package org.eclipse.openvsx.cache; + +import java.nio.file.Files; +import java.nio.file.Path; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import static org.assertj.core.api.Assertions.assertThat; + +class FileSizeWeigherTest { + + private final FileSizeWeigher weigher = new FileSizeWeigher(0); + + @Test + void testWeighsByFileSize(@TempDir Path tmpDir) throws Exception { + var path = tmpDir.resolve("resource.bin"); + Files.write(path, new byte[1234]); + + assertThat(weigher.weigh("key", path)).isEqualTo(1234); + } + + @Test + void testWeighsMissingFileAsMax() { + var path = Path.of("/does/not/exist"); + + assertThat(weigher.weigh("key", path)).isEqualTo(Integer.MAX_VALUE); + } + + @Test + void testWeighsNonPathValueAsOne() { + assertThat(weigher.weigh("key", "not-a-path")).isEqualTo(1); + } + + @Test + void testFloorsSmallFilesToTheFloorWeight(@TempDir Path tmpDir) throws Exception { + // Simulates a cache sized for at most 10 entries out of a 10_000 byte budget: a file far + // smaller than its 1000-byte share is still charged the full share, so 10 of them exhaust + // the budget and a maximumWeight(10_000) cache can never hold more than 10 entries. + var path = tmpDir.resolve("tiny.bin"); + Files.write(path, new byte[10]); + var weigher = new FileSizeWeigher(1000); + + assertThat(weigher.weigh("key", path)).isEqualTo(1000); + } + + @Test + void testStillWeighsByActualSizeWhenAboveTheFloor(@TempDir Path tmpDir) throws Exception { + // A file bigger than its floor share is charged its real size, so the byte budget is still + // the binding constraint for entries that are individually large. + var path = tmpDir.resolve("large.bin"); + Files.write(path, new byte[5000]); + var weigher = new FileSizeWeigher(1000); + + assertThat(weigher.weigh("key", path)).isEqualTo(5000); + } +} diff --git a/server/src/test/java/org/eclipse/openvsx/util/FileUtilTest.java b/server/src/test/java/org/eclipse/openvsx/util/FileUtilTest.java new file mode 100644 index 000000000..4a7fd6520 --- /dev/null +++ b/server/src/test/java/org/eclipse/openvsx/util/FileUtilTest.java @@ -0,0 +1,78 @@ +/******************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License 2.0 which is available at + * https://www.eclipse.org/legal/epl-2.0 + * + * SPDX-License-Identifier: EPL-2.0 + ********************************************************************************/ +package org.eclipse.openvsx.util; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +class FileUtilTest { + + @Test + void testDeletesPartialFileWhenWriterFails(@TempDir Path tmpDir) { + var path = tmpDir.resolve("partial.tmp"); + + assertThatThrownBy(() -> FileUtil.writeSync(path, p -> { + try { + Files.writeString(p, "partial content"); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + throw new RuntimeException("write failed partway through"); + })).hasMessage("write failed partway through"); + + assertThat(Files.exists(path)).isFalse(); + } + + @Test + void testRetriesAfterAFailedWrite(@TempDir Path tmpDir) { + var path = tmpDir.resolve("retry.tmp"); + + assertThatThrownBy(() -> FileUtil.writeSync(path, p -> { + throw new RuntimeException("first attempt fails"); + })).hasMessage("first attempt fails"); + assertThat(Files.exists(path)).isFalse(); + + // a later call for the same path must not see a leftover partial file and skip writing + FileUtil.writeSync(path, p -> { + try { + Files.writeString(p, "content"); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + }); + + assertThat(Files.exists(path)).isTrue(); + } + + @Test + void testDoesNotOverwriteAnExistingFile() throws IOException { + var path = Files.createTempFile("openvsx-file-util-test", ".tmp"); + try { + Files.writeString(path, "original"); + FileUtil.writeSync(path, p -> { + throw new AssertionError("writer must not run when the file already exists"); + }); + assertThat(Files.readString(path)).isEqualTo("original"); + } finally { + Files.deleteIfExists(path); + } + } +}