-
Notifications
You must be signed in to change notification settings - Fork 352
fix: limit decompressed size when serving vscode/unpkg web resources #2060
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<Object, Object> { | ||
| 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); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we account for empty files here? With a weight of 0, they do not count toward maximumWeight, and the previous limit of 150 entries is gone.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure yet what is the most reasonable thing to do here. Even if you give empty files a weight it will not contribute much the the total. Combining a weight with the previous max-size would be the most robust imho. |
||
| } 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; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if ArchiveUtil.MAX_ENTRY_SIZE is the right default here. It only covers entries passed to ArchiveUtil.readEntry during publishing, while this check applies to every file requested through /vscode/unpkg. That could allow a larger entry at publish time and reject it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the design of that endpoint does not suit use-cases where you can serve files from within the extension due to the way this data is provided. So serving large files from this endpoint is flawed anyway, need to think about it a bit more.