Repository navigation
fix: remove the max-override-size ceiling on extension size overrides - #2297
Merged
Merged
Conversation
The dashboard admin who can grant a per-namespace override can also set the registry-wide default to any value above zero, live, no restart, which raises the effective ceiling for every namespace at once - a strictly more powerful, completely unchecked lever. Bounding only the narrower override left the check unable to actually constrain the trust model it was meant to protect. ovsx.publishing.max-override-size was never part of a release (added after v1.2.0), so it can be dropped outright with no deprecation path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
It lacks a regression test proving values above the former ceiling are accepted.
Review effort: Balanced
Findings: 1
What changed in this PR
Removes the unpublished extension-size override ceiling, aligning override limits with the already-unbounded global default.
Changes:
- Removes the
max-override-sizeconfiguration and validation. - Allows any positive
longoverride size. - Updates tests and documentation for the removed ceiling.
| File | Description |
|---|---|
PublishingConfig.java |
Removes the ceiling property and validation. |
ExtensionSizeLimitService.java |
Removes upper-bound enforcement. |
PublishingConfigTest.java |
Removes obsolete property tests. |
ExtensionSizeLimitServiceTest.java |
Removes ceiling-related mocks and rejection test. |
configuration.md |
Removes documentation for the ceiling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses Copilot review feedback on #2297: deleting the old ceiling-rejection test didn't prove a value above the former 1 GiB cap is now accepted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses Copilot review feedback on #2297: the comment described the former ceiling rather than the current invariant, which AGENTS.md's comment-style rule excludes. The test name already names the scenario. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
5 of 6 tasks
netomi
added a commit
that referenced
this pull request
Oct 7, 2026
…ze ceiling (#2303) * fix: validate publish size limits against the scanning limit Restores ovsx.publishing.max-override-size (removed in #2297 because it bounded only overrides, leaving the registry-wide default unbounded) as a hard, service-wide ceiling: it now bounds both the admin-adjustable default and every size override, so neither path can drift past it independently. Startup additionally fails if that ceiling exceeds ovsx.scanning.max-archive-size-bytes, since scanning silently skips whatever is past its own archive-size limit instead of failing closed - closing the gap in #2302 where a publish-accepted package could be larger than what scanning actually inspects. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: drop the scan-limit cross-check, it compared the wrong units Review feedback on #2303: the publish-side ceiling bounds the compressed .vsix size on disk (ExtensionService#createExtensionFile writes the upload as-is), while ovsx.scanning.max-archive-size-bytes bounds the sum of uncompressed ZIP entry sizes (ArchiveUtil#enforceArchiveLimits reads ZipEntry#getSize, which the API defines as uncompressed). Comparing the two at startup doesn't guarantee a package under the ceiling is fully scanned - a small, highly-compressible package can still exceed the uncompressed scan limit. Closing that gap for real needs a different approach, so the cross-check is dropped rather than left making a claim it can't back up. Also restores the "whole number of bytes" wording the settings page's size validation message dropped when the ceiling range was added to it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: remove stale scan-validation claims left in docs/comments Review feedback: PublishingConfig#maxOverrideSize's javadoc and a PublishingConfigTest comment still claimed the startup check against ovsx.scanning.max-archive-size-bytes, which was already dropped in the previous commit. Reword both to describe only what's actually enforced now. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: close review findings on permission mismatch and wording - Soften PublishingConfig#maxOverrideSize's javadoc from "hard ceiling" to "write-time ceiling", matching doc/configuration.md's existing caveat that lowering it does not reach a default or override already stored above it. - The Size overrides page requires manage_extensions, but sourced its client-side ceiling from GET /admin/settings, which requires manage_settings - a permission split the fine-grained admin roles (#2269) make real. A user with only manage_extensions got a silent 403 there and never saw the validation. Moves maxOverrideSize onto GET /admin/size-overrides (SizeOverrideListJson), which the page already has permission for. - extension-registry-types.ts: fix "the default above" reading as a noun phrase in the Settings.maxOverrideSize doc comment. - settings.tsx: stop flooring the ceiling's MB conversion in the validation message - a 1.5 MiB ceiling reported "at most 1 MB", understating what the server actually accepts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: show the exact byte ceiling instead of a rounded MB approximation Review feedback: rounding the MB conversion can overstate the ceiling near a boundary - 2 MiB - 1 byte rounded to "2.00 MB", naming a value (2 MB) the server actually rejects. The exact byte count is now the authoritative part of the validation message, with the MB figure only an approximation alongside it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
ovsx.publishing.max-override-size(#2287) bounded only the narrower, scoped lever of the two the admin dashboard exposes: a per-namespace/extension size override. The same dashboard admin who creates an override can also set the registry-wide Default max extension size on the Settings page to any value above zero — live, no restart, no ceiling check (SettingsService.updateFromJsononly validates> 0). Since the stream-time ceiling ismax(default, highest override), raising the default raises the effective ceiling for every namespace at once, unchecked.So the check restricted the narrower, lower-blast-radius path while leaving a strictly more powerful, completely unbounded path open to the same actor. Given the trust model already lets dashboard admins set an unbounded global default, a ceiling on overrides adds friction without closing any actual gap.
ovsx.publishing.max-override-sizewas added after the v1.2.0 release and has never shipped, so it's removed outright with no deprecation fallback.What changed
PublishingConfig: droppedmaxOverrideSize/MAX_OVERRIDE_SIZEand its@PostConstructvalidation.ExtensionSizeLimitService:requireValidSizeno longer checks a ceiling (still rejects<= 0); dropped the now-unusedPublishingConfigdependency.doc/configuration.md: removed the property's reference entry and the "every override is bounded" consequence in the Extension size limits section.PublishingConfigTestandExtensionSizeLimitServiceTest; the rest pass unchanged.Test plan
./gradlew compileJava compileTestJava./gradlew spotlessCheck./gradlew unitTests./gradlew test(full container tier) — not run, no Docker daemon available in this environment🤖 Generated with Claude Code