fix: limit decompressed size when serving vscode/unpkg web resources - #2060
fix: limit decompressed size when serving vscode/unpkg web resources#2060netomi wants to merge 1 commit into
Conversation
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR mitigates potential disk-exhaustion risks when serving VS Code “unpkg” web resources by bounding per-file extraction size, cleaning up partial writes, and making the on-disk cache size-aware.
Changes:
- Add a per-entry decompressed size limit when extracting and caching a requested VSIX entry, with a
413 Content Too Largeresponse when the declared size exceeds the configured maximum. - Ensure
FileUtil.writeSyncdeletes partially written files when the writer fails, allowing retries and preventing corrupted cache artifacts. - Switch the web resource file cache from count-based eviction to byte-size-based eviction using a
FileSizeWeigher, with new configuration for maximum total cache size.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/java/org/eclipse/openvsx/adapter/WebResourceService.java | Enforces declared-size checks and bounds extraction reads with SizeLimitInputStream to prevent oversized decompression. |
| server/src/main/java/org/eclipse/openvsx/util/FileUtil.java | Deletes partial files on writer failure to avoid poisoning the cache path. |
| server/src/main/java/org/eclipse/openvsx/cache/CacheConfig.java | Changes web resource cache eviction from entry-count to total-bytes using maximumWeight + FileSizeWeigher. |
| server/src/main/java/org/eclipse/openvsx/cache/FileSizeWeigher.java | Implements a Caffeine weigher based on actual on-disk file size. |
| server/src/test/java/org/eclipse/openvsx/adapter/VSCodeAPITest.java | Updates test wiring for the new WebResourceService constructor parameter. |
| server/src/test/java/org/eclipse/openvsx/adapter/WebResourceServiceTest.java | Adds coverage for oversized declared entries and “lying” zip headers (bounded copy + no partial file left behind). |
| server/src/test/java/org/eclipse/openvsx/util/FileUtilTest.java | Adds coverage for partial-write cleanup and retry behavior. |
| server/src/test/java/org/eclipse/openvsx/cache/FileSizeWeigherTest.java | Adds coverage for size-based weighing behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @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 |
| } | ||
|
|
||
| try { | ||
| return (int) Math.min(Files.size(path), Integer.MAX_VALUE); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| CacheService cache, | ||
| FilesCacheKeyGenerator filesCacheKeyGenerator | ||
| FilesCacheKeyGenerator filesCacheKeyGenerator, | ||
| @Value("${ovsx.caching.files-webresource.max-file-size:33554432}") long maxFileSize |
There was a problem hiding this comment.
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.
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.
Problem
WebResourceService.getWebResource(used by/vscode/unpkg/{namespace}/{extension}/{version}/{path}) extracts a single file from a published VSIX on first request and caches it underjava.io.tmpdir. The extraction had no bound on the decompressed size — only the compressed VSIX upload is size-limited (ovsx.publishing.max-content-size, 512 MB by default). A VSIX containing one or more highly compressible entries can therefore expand to a much larger file on disk than the uploaded package size when that entry is requested, and repeating this with different files/versions can exhaust the filesystem backing the temp cache.Additionally:
ovsx.caching.files-webresource) evicted by entry count, not total size, so it did nothing to bound disk usage from oversized cached files.FileUtil.writeSyncwould then treat that leftover file as "already written," permanently blocking retries at that cache path.Fix
WebResourceService.writeBinaryFilenow rejects an entry whose declared size exceeds a newovsx.caching.files-webresource.max-file-sizelimit (default 32 MB, matchingArchiveUtil.MAX_ENTRY_SIZE) before extracting anything, returning413 Content Too Large. The actual copy is also bounded bySizeLimitInputStreamagainst the declared size, so an entry with an inconsistent zip header can't produce more bytes than it claimed.FileUtil.writeSyncnow deletes a partially written file if the writer throws, instead of leaving a truncated file that blocks future attempts at that path. This also benefits the Azure/AWS/GCS storage services, which share this helper.webResourceCachebean switched frommaximumSize(entry count) tomaximumWeightwith a newFileSizeWeigherthat weighs each cached file by its actual byte size, configurable viaovsx.caching.files-webresource.max-total-size(default 2 GiB). This is a config rename fromovsx.caching.files-webresource.max-size.Testing
WebResourceServiceTest,FileUtilTest,FileSizeWeigherTestcovering the new behavior (oversized declared entry rejected before extraction, a lying zip header stopped mid-copy with no partial file left behind, normal files still served correctly, partial-write cleanup and retry inFileUtil.writeSync, weigher behavior).VSCodeAPITest,ArchiveUtilTest, and the Azure/AWS/GCS storage test suites still pass.