You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: resolve extension resources for the target platform VS Code asks for - #2183
Stacked on #2181 — base is fix/target-platform-validation, so this diff shows only the unpkg change. #2181 supplies the @Validated/@Pattern wiring on VSCodeAPI that this reuses; without it the two PRs would both add those lines and conflict. Merge that first, or say so and I will retarget this at main.
Reimplements #758 (June 2023) on current main. Fixes#744.
What is broken
VS Code encodes the target platform into the version segment when it resolves an extension's resources — <version>+<target>, web for a web extension (microsoft/vscode#180525, microsoft/vscode#182072). The unpkg endpoint never learned to read it:
That extension really does publish 1.23.0 for both universal and web, and the second URL is what a web VS Code sends — so its resources do not load at all in vscode.dev or github.dev.
Even without the suffix the endpoint could not have told the two apart: it passed null down to the download lookup, which matched whichever version came first.
The change
Smaller than #758, because the plumbing already takes a target platform — WebResourceService.getExtensionDownload(namespace, extension, targetPlatform, version) and browseExtensionPackage(...) both have the parameter, and browseExtensionPackage was using it only to compose an error message. So this is mostly a matter of resolving a target at the edge and passing it down.
Where the target comes from.?target= if given, otherwise a +<target> suffix on the version. The suffix is taken only when it names a platform:
A version may legitimately carry semver build metadata — 1.2.3+build.5 — which SemanticVersion.VERSION_PATH_PARAM_REGEX explicitly permits. "Text after the last +" would read build.5 as a target, so the validation is what makes the heuristic safe rather than merely convenient. There is a test for it.
Directory listings re-append the target.browseExtensionPackage built its child URLs from the bare version, so without this, walking into a subdirectory silently drops back to whichever version matches first — the same bug one level down. universal is left off, since it is the absence of a target rather than a target.
Upstream forwarding.UpstreamVSCodeService passes the target on as ?target=, so a mirror forwards it rather than resolving it against its own registry. Query-parameter form rather than the + suffix because both are accepted and it needs no escaping decisions.
Tests
testBrowseTopDirForATargetPlatformInTheVersion — 0.16.6+web resolves, and the returned listing URLs keep +web
testBrowseTopDirForATargetPlatformParameter — the same via ?target=web
testBrowseKeepsSemverBuildMetadataInTheVersion — 1.2.3+build.5 is looked up whole, with a null target, verified on the repository call
The first two fail without the change. The third passes either way, which is right — it guards against over-eager parsing, so it should hold both before and after.
Full server suite green (1214 tests); spotlessCheck and format.sh clean.
It cannot be rebased: it is written against ResponseEntity<byte[]> and a codebase with no WebResourceService, across 8 files and 2½ years of drift. Its repository addition is also redundant now — findFileByType already resolves a target-specific version. The design is amvanbaren's and this follows it, including the two details easiest to miss: validating the suffix before stripping it, and putting the target back on the listing URLs.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
VSCodeAPI validates targetPlatform with a regex that allows empty strings on endpoints with a default value, which can allow ?targetPlatform= to pass validation but then incorrectly 404 due to downstream lookups using "" as the platform.
Pull request overview
This PR updates the VS Code compatibility layer so /vscode/unpkg/** resolves and browses extension resources for the target platform VS Code requests (either via ?target= or a +<target> suffix on the version), and adds request-side validation for targetPlatform where it’s supplied as a query/body parameter.
Changes:
Parse target platform for unpkg browsing from ?target= or a validated +<target> version suffix and pass it through to the browse services.
Preserve the target platform when generating directory listing URLs so subsequent navigation doesn’t “fall back” to an arbitrary matching version.
Enforce targetPlatform allowable values via Bean Validation annotations across registry and VS Code adapter endpoints, with tests covering rejections/acceptance.
TargetPlatform.NAMES_PARAM_REGEX intentionally allows an empty string, but on this endpoint an empty ?targetPlatform= overrides the defaultValue and then propagates as "" into repository lookups (via LocalVSCodeService), which can lead to an incorrect 404. Since this endpoint already has a default (universal), it’s safer to reject an explicit empty value by validating against NAMES_PATH_PARAM_REGEX (non-empty) instead.
This issue also appears on line 350 of the same file.
@RequestParam(defaultValue = TargetPlatform.NAME_UNIVERSAL)
@Pattern(
regexp = TargetPlatform.NAMES_PARAM_REGEX,
message = "parameter must be a supported target platform"
)
TargetPlatform.NAMES_PARAM_REGEX allows an empty string, but on this endpoint an empty ?targetPlatform= overrides the defaultValue and then propagates as "" into repository lookups (via LocalVSCodeService), which can lead to an incorrect 404. Since this endpoint already has a default (universal), it’s safer to reject an explicit empty value by validating against NAMES_PATH_PARAM_REGEX (non-empty) instead.
@RequestParam(defaultValue = TargetPlatform.NAME_UNIVERSAL)
@Pattern(
regexp = TargetPlatform.NAMES_PARAM_REGEX,
message = "parameter must be a supported target platform"
)
Re: Copilot's review — the suppressed comments on VSCodeAPI.java:228 and :354 about ?targetPlatform= (empty value) bypassing the universal default and propagating as "" into repository lookups:
This doesn't reproduce. @RequestParam(defaultValue = ...) treats a present-but-empty value the same as an absent one — that's the documented contract (defaultValue: "used as a fallback when the request parameter is not provided or has an empty value"), and it's enforced in AbstractNamedValueMethodArgumentResolver.resolveArgument, which explicitly re-substitutes the default when the resolved arg equals "". So ?targetPlatform= on getAsset/download resolves to "universal" before the @Pattern check ever runs, not "".
I confirmed this with a test (?targetPlatform= against /vscode/asset/...): it does not 404 the way an unrecognized value like win32-bogus does, and swapping NAMES_PARAM_REGEX for NAMES_PATH_PARAM_REGEX (rejecting empty) makes no behavioral difference, since the empty string never reaches validation. Leaving the code as-is.
… for
VS Code encodes the target platform into the version segment when it resolves
an extension's resources - `<version>+<target>`, `web` for a web extension
(microsoft/vscode#180525). The unpkg endpoint never learned to read it, so such
a request 404s:
/vscode/unpkg/BroadcomMFD/hlasm-language-support/1.23.0/... 200
/vscode/unpkg/BroadcomMFD/hlasm-language-support/1.23.0+web/... 404
That extension publishes 1.23.0 for both `universal` and `web`, and the second
URL is the one a web VS Code sends, so its resources do not load at all.
Without a target the endpoint also could not have distinguished them: it passed
null down to the download lookup, which then matched whichever version came
first. The plumbing already took a target platform - WebResourceService's
getExtensionDownload and browseExtensionPackage both have the parameter, and
browseExtensionPackage used it only to compose an error message - so this is
mostly a matter of resolving one at the edge and passing it along.
The target comes from `?target=`, or from a `+<target>` suffix on the version
when that is absent. The suffix is only taken when it names a platform: a
version may legitimately carry semver build metadata (`1.2.3+build.5`), which
SemanticVersion's own regex permits, and that belongs to the version.
Directory listings re-append the target to the version in the URLs they hand
back. Without that, walking into a subdirectory silently drops to whichever
version matches first, which is the same bug one level down.
UpstreamVSCodeService forwards the target as a query parameter, so a mirror
passes it on rather than resolving it against its own registry.
Reimplements #758, which reported and fixed this in June 2023 - credit to
@amvanbaren. That PR cannot be rebased: it is written against
ResponseEntity<byte[]> and a codebase without WebResourceService, and the
repository method it adds is now redundant since findFileByType already
resolves a target-specific version.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UpstreamVSCodeServiceTest still had a call to browse() with the old
4-argument signature, which no longer compiled once targetPlatform was
added to the interface. Fix it and add a test that actually exercises
the new upstream ?target= forwarding, since the existing calls all
passed null and never covered that branch.
Also document the second 400 cause (an unsupported target platform)
on the browse endpoint's OpenAPI response, alongside the existing
built-in-namespace case.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A published version's own semver build metadata can itself name a
platform, e.g. 0.16.6+web. The suffix-stripping added for VS Code's
<version>+<target> browse requests read that as version 0.16.6 on
target web, so a universal build actually published under 0.16.6+web
could never be reached.
Try the version as given first; only fall back to splitting off a
target suffix once that lookup comes back empty.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ral version
Checking the version exactly as given before the split reading meant
every ordinary <version>+<target> request - the case VS Code actually
sends, and the reason this suffix handling exists - paid for a doomed
lookup before the one that succeeds. Swap the order: the split reading
runs first, and the literal version is only tried if that misses, so
the common case stays a single lookup.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Browsing a version whose own suffix looks like a target (e.g.
0.16.6+web) with an explicit ?target=universal resolved correctly, but
the returned directory listing dropped the target for universal
builds. Following one of those URLs re-enters with no target
parameter, so the split reading takes over and misreads the suffix as
the target - resolving the wrong build.
Only omit the target from listed URLs when the version's own suffix
isn't itself ambiguous; keep it otherwise, universal included. Factor
the version-suffix check into TargetPlatform.targetInVersionSuffix so
VSCodeAPI's split reading and this share one implementation.
Also fix a stale test name/comment left over from reordering the
split vs. exact-version lookup in the previous commit.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The newly added validation path is not covered. VSCodeAPITest verifies invalid target-platform rejection for the existing asset and vspackage parameters, but only valid target values are exercised for /vscode/unpkg. Add a request with target=win32-bogus that asserts the 400 response so the endpoint's dependency on class-level @Validated and this parameter constraint cannot regress unnoticed.
Document allowable target-platform values in OpenAPI schema
The new public target parameter omits the allowableValues schema used by the other target-platform parameters in this class (for example, VSCodeAPI.java:229-248). Consequently, the generated OpenAPI contract does not expose the finite set this endpoint accepts even though validation rejects every other value. Add the same platform schema here.
…tion
The browse endpoint's new target parameter validates against the same
set of platforms as targetPlatform elsewhere in this class, but was
missing the matching allowableValues schema, so the generated OpenAPI
contract didn't expose it. Add it, and add a regression test for the
existing 400 rejection, mirroring the coverage the other
target-platform parameters already have.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addressed the two "previously missed" items from the latest Copilot review, fixed in f6d9703:
Added the allowableValues schema to the target parameter on browse, matching the targetPlatform parameter elsewhere in this class.
Added testBrowseRejectsAnUnknownTargetPlatform, covering the existing 400 rejection for /vscode/unpkg the way testAssetRejectsAnUnknownTargetPlatform and testVspackageRejectsAnUnknownTargetPlatform already cover the other endpoints.
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
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.
Reimplements #758 (June 2023) on current
main. Fixes #744.What is broken
VS Code encodes the target platform into the version segment when it resolves an extension's resources —
<version>+<target>,webfor a web extension (microsoft/vscode#180525, microsoft/vscode#182072). The unpkg endpoint never learned to read it:That extension really does publish
1.23.0for bothuniversalandweb, and the second URL is what a web VS Code sends — so its resources do not load at all in vscode.dev or github.dev.Even without the suffix the endpoint could not have told the two apart: it passed
nulldown to the download lookup, which matched whichever version came first.The change
Smaller than #758, because the plumbing already takes a target platform —
WebResourceService.getExtensionDownload(namespace, extension, targetPlatform, version)andbrowseExtensionPackage(...)both have the parameter, andbrowseExtensionPackagewas using it only to compose an error message. So this is mostly a matter of resolving a target at the edge and passing it down.Where the target comes from.
?target=if given, otherwise a+<target>suffix on the version. The suffix is taken only when it names a platform:A version may legitimately carry semver build metadata —
1.2.3+build.5— whichSemanticVersion.VERSION_PATH_PARAM_REGEXexplicitly permits. "Text after the last+" would readbuild.5as a target, so the validation is what makes the heuristic safe rather than merely convenient. There is a test for it.Directory listings re-append the target.
browseExtensionPackagebuilt its child URLs from the bare version, so without this, walking into a subdirectory silently drops back to whichever version matches first — the same bug one level down.universalis left off, since it is the absence of a target rather than a target.Upstream forwarding.
UpstreamVSCodeServicepasses the target on as?target=, so a mirror forwards it rather than resolving it against its own registry. Query-parameter form rather than the+suffix because both are accepted and it needs no escaping decisions.Tests
testBrowseTopDirForATargetPlatformInTheVersion—0.16.6+webresolves, and the returned listing URLs keep+webtestBrowseTopDirForATargetPlatformParameter— the same via?target=webtestBrowseKeepsSemverBuildMetadataInTheVersion—1.2.3+build.5is looked up whole, with a null target, verified on the repository callThe first two fail without the change. The third passes either way, which is right — it guards against over-eager parsing, so it should hold both before and after.
Full server suite green (1214 tests);
spotlessCheckandformat.shclean.On #758
It cannot be rebased: it is written against
ResponseEntity<byte[]>and a codebase with noWebResourceService, across 8 files and 2½ years of drift. Its repository addition is also redundant now —findFileByTypealready resolves a target-specific version. The design is amvanbaren's and this follows it, including the two details easiest to miss: validating the suffix before stripping it, and putting the target back on the listing URLs.Suggest closing #758 in favour of this.