From 30324b1d469925a10083f9133afb5ceaed38b4c2 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Sat, 8 Aug 2026 12:59:44 +0200 Subject: [PATCH] fix: limit decompressed size when serving vscode/unpkg web resources WebResourceService.writeBinaryFile extracted a VSIX zip entry to disk with no bound on the decompressed size. A small, highly compressed VSIX could therefore expand into an oversized file under java.io.tmpdir when a single entry was requested via /vscode/unpkg/, and the upload-time size check only limits the compressed package. - Reject entries whose declared size exceeds a new ovsx.caching.files-webresource.max-file-size limit (default 32 MB, matching ArchiveUtil.MAX_ENTRY_SIZE) before extracting, and bound the actual copy with SizeLimitInputStream so a zip entry with an inconsistent header can't produce more bytes than declared. - FileUtil.writeSync now deletes a partially written file if the writer fails, instead of leaving it behind. Previously a failed write (e.g. disk full) left a truncated file that permanently blocked retries at that cache path, since writeSync only (re-)invokes the writer when the file doesn't already exist. - Switch the web resource cache from counting entries (maximumSize) to weighing them by file size (maximumWeight + new FileSizeWeigher), configurable via ovsx.caching.files-webresource.max-total-size (default 2 GiB), so the cache is bounded by total disk usage rather than file count. Co-Authored-By: Claude Sonnet 5 --- .../openvsx/adapter/WebResourceService.java | 30 ++++- .../eclipse/openvsx/cache/CacheConfig.java | 7 +- .../openvsx/cache/FileSizeWeigher.java | 46 +++++++ .../org/eclipse/openvsx/util/FileUtil.java | 26 +++- .../openvsx/adapter/VSCodeAPITest.java | 2 +- .../adapter/WebResourceServiceTest.java | 117 ++++++++++++++++++ .../openvsx/cache/FileSizeWeigherTest.java | 46 +++++++ .../eclipse/openvsx/util/FileUtilTest.java | 78 ++++++++++++ 8 files changed, 344 insertions(+), 8 deletions(-) create mode 100644 server/src/main/java/org/eclipse/openvsx/cache/FileSizeWeigher.java create mode 100644 server/src/test/java/org/eclipse/openvsx/adapter/WebResourceServiceTest.java create mode 100644 server/src/test/java/org/eclipse/openvsx/cache/FileSizeWeigherTest.java create mode 100644 server/src/test/java/org/eclipse/openvsx/util/FileUtilTest.java 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..29a88f402 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,24 @@ public class WebResourceService { private final FilesCacheKeyGenerator filesCacheKeyGenerator; private final JsonMapper jsonMapper; + // Limit the decompressed size of a single served web resource, default 32 MB matching + // ArchiveUtil.MAX_ENTRY_SIZE. Without this, a small VSIX containing a highly compressed entry + // could exhaust the java.io.tmpdir filesystem when that entry is 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:33554432}") 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 +164,23 @@ private String getFileExtension(ZipEntry fileEntry) { } private void writeBinaryFile(Path file, ZipFile zip, ZipEntry fileEntry) { + var declaredSize = fileEntry.getSize(); + 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); + // 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 in = zip.getInputStream(fileEntry); var limited = new SizeLimitInputStream(in, declaredSize)) { + Files.copy(limited, 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..9765bae38 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,15 @@ 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 + // 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 ) { return Caffeine.newBuilder() .removalListener(new ExpiredFileListener()) .expireAfterAccess(timeToIdle) - .maximumSize(maxSize) + .maximumWeight(maxTotalSize) + .weigher(new FileSizeWeigher()) .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..3af838b30 --- /dev/null +++ b/server/src/main/java/org/eclipse/openvsx/cache/FileSizeWeigher.java @@ -0,0 +1,46 @@ +/****************************************************************************** + * 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. + */ +public class FileSizeWeigher implements Weigher { + private static final Logger logger = LoggerFactory.getLogger(FileSizeWeigher.class); + + @Override + public int weigh(@NonNull Object key, @NonNull Object value) { + if (!(value instanceof Path path)) { + return 1; + } + + try { + return (int) Math.min(Files.size(path), 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 d4ddc3b2d..3952aefb8 100644 --- a/server/src/test/java/org/eclipse/openvsx/adapter/VSCodeAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/adapter/VSCodeAPITest.java @@ -1376,7 +1376,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..17f9eba2c --- /dev/null +++ b/server/src/test/java/org/eclipse/openvsx/adapter/WebResourceServiceTest.java @@ -0,0 +1,117 @@ +/******************************************************************************** + * 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 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..b5c3f93d8 --- /dev/null +++ b/server/src/test/java/org/eclipse/openvsx/cache/FileSizeWeigherTest.java @@ -0,0 +1,46 @@ +/******************************************************************************** + * 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(); + + @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); + } +} 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); + } + } +}