feat: verified check scanner - #2074
Conversation
A new scanner, that on publish checks upstream for same extension and if exists, enforces locally that publisher is also owner of the NS.
|
a side-effect of doing the check as an async scanner is that the user does not have feedback about the reason why its not activated yet. We will also need to improve the UI to clearly indicate a reason as this one is quite distinct and how to proceed with publication i.e. how to claim the namespace. |
There was a problem hiding this comment.
Pull request overview
Adds a new synchronous publish-time scanner that queries the upstream VS Code Marketplace for an extension ID collision and (when configured) blocks/quarantines publishing unless namespace ownership requirements are met.
Changes:
- Introduces
VSCodeGalleryExistenceCheckScannerto query the upstream galleryextensionqueryAPI and emit a high-severity threat on conflicts. - Adds
VSCodeGalleryExistenceCheckConfigfor enabling/required/enforced flags and gallery URL configuration. - Adds unit tests covering upstream-down, upstream-missing, and upstream-present scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| server/src/main/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScanner.java | New scanner implementation that checks upstream extension existence and enforces local namespace constraints. |
| server/src/main/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckConfig.java | New Spring config bean for scanner toggles and upstream gallery URL. |
| server/src/test/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScannerTest.java | New unit tests validating scanner behavior across key scenarios. |
Suppressed comments (2)
server/src/test/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScannerTest.java:148
- This test name says a threat is raised when the namespace is verified, but the assertions expect a clean result with 0 threats. Rename to reflect the actual expected behavior.
void startScan_raisesThreat_whenExistsUpstreamAndNamespaceIsVerified() throws Exception {
server/src/main/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckConfig.java:47
- Same as above: the Javadoc property names use the
gallery-existence-checkprefix, but the implementation usesgallery-ownership. These should be consistent.
/**
* Is NS verification check for upstream existing extensions enforced or not.
* <p>
* Property: {@code ovsx.scanning.gallery-existence-check.enforced}
* Default: {@code true}
*/
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| var publishedWith = extVersion.getPublishedWith(); | ||
| var user = publishedWith != null ? publishedWith.getUser() : null; | ||
| if (user != null && repositories.isVerified(namespace, user)) { | ||
| return new Scanner.Invocation.Completed( | ||
| Scanner.Result.clean( | ||
| "Extension exists on the VS Code Marketplace; namespace confirmed as verified.")); | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (8)
server/src/main/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScanner.java:126
- PR description says publishing should be allowed only when the publisher is an owner of the namespace, but the scanner currently allows any member as long as the namespace has an owner (RepositoryService.isVerified). If the intent is to require ownership by the publishing user, this should call isNamespaceOwner instead and the messages should reflect ownership.
if (user != null && repositories.isVerified(namespace, user)) {
return new Scanner.Invocation.Completed(
Scanner.Result.clean(
"Extension exists on the VS Code Marketplace; namespace confirmed as verified."));
}
server/src/test/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScannerTest.java:80
- Minor grammar: "irrelevant of content" should be "irrelevant to content".
/**
* Dummy result; irrelevant of content
*/
server/src/main/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScanner.java:118
- RestClientException handling uses config.isEnforced() to decide whether to fail the scan. That bypasses the scanner's isRequired() semantics: a scanner configured as required but not enforced would silently pass when the upstream is down, even though required scanners should fail-closed on errors.
} catch (RestClientException ex) {
if (config.isEnforced()) {
throw new ScannerException("Failed to perform " + TYPE, ex);
} else {
return new Scanner.Invocation.Completed(
server/src/main/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScanner.java:132
- Threat description still says the namespace is "not verified", but with the intended behavior (publisher must be owner) this should explain that the publishing user is not an owner of the namespace.
var threat = new Scanner.Threat(
"vscode-gallery-namespace-conflict",
"'" + NamingUtil.toExtensionId(extension) + "' already exists on the VS Code Marketplace, " +
"and the target namespace is not verified '" + namespace.getName() + "'.",
"high");
server/src/main/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScanner.java:140
- The Javadoc for upstreamExists talks about returning an Optional, but the method actually returns a boolean. This makes the contract unclear for callers and future maintenance.
/**
* Method reaching upstream; if returns {@code true} or {@code false} only if check was performed and result was
* clear about it. In any other case method will throw.
*/
private boolean upstreamExists(Extension extension) throws RestClientException {
server/src/main/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckConfig.java:98
- The validation error message references the wrong property key (gallery-ownership.gallery-url), which will mislead operators. It should match the documented/used key ovsx.scanning.gallery-existence-check.gallery-url.
if (enabled) {
if (galleryUrl == null || galleryUrl.isEmpty()) {
throw new IllegalStateException("ovsx.scanning.gallery-existence-check.gallery-url must be set");
}
server/src/test/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScannerTest.java:159
- This test name says it "raisesThreat" when the namespace is verified, but the assertions expect a clean result. Renaming it will avoid confusion when reading failures.
@Test
void startScan_raisesThreat_whenExistsUpstreamAndNamespaceIsVerified() throws Exception {
var user = new UserData();
server/src/test/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScannerTest.java:57
- Minor grammar: "irrelevant of content" should be "irrelevant to content".
This issue also appears on line 78 of the same file.
/**
* Dummy result; irrelevant of content
*/
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (5)
server/src/main/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScanner.java:167
upstreamExistscurrently returnsfalsewhen the upstream responds with a null/invalid body (e.g.,result == nullorresult.results() == null). That fails open and contradicts the method Javadoc (“In any other case method will throw”), allowing publishing even though the upstream check was not reliably performed. Treat an unexpected response shape as an error so enforced mode can block (and non-enforced mode can still record a clean result with a summary).
var result = restTemplate
.postForObject(requestUrl, new HttpEntity<>(requestData, headers), ExtensionQueryResult.class);
if (result != null && result.results() != null && !result.results().isEmpty()) {
var item = result.results().getFirst();
return item.extensions() != null && !item.extensions().isEmpty();
}
return false;
server/src/main/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScanner.java:116
- The non-enforced failure path includes
ex.getMessage()in the persisted scan summary.RestClientExceptionmessages often contain request URLs/connection details, which can leak internal network information to API consumers/admin UIs. Prefer logging the exception server-side and returning a generic summary.
return new Scanner.Invocation.Completed(
Scanner.Result.clean(
"Failed to perform " + TYPE + " scan: " + ex.getMessage()));
server/src/main/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScanner.java:132
- Threat description is currently grammatically ambiguous: "target namespace is not verified ''" reads like the name is being described as "not verified". Rephrase to clearly state which namespace is unverified.
var threat = new Scanner.Threat(
"vscode-gallery-namespace-conflict",
"'" + NamingUtil.toExtensionId(extension) + "' already exists on the VS Code Marketplace, " +
"and the target namespace is not verified '" + namespace.getName() + "'.",
"high");
server/src/test/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScannerTest.java:158
- Test name says it "raisesThreat" when the namespace is verified, but the assertions expect a clean result. Rename the test to reflect the behavior it verifies to avoid confusion and false signals when scanning failures occur.
void startScan_raisesThreat_whenExistsUpstreamAndNamespaceIsVerified() throws Exception {
server/src/test/java/org/eclipse/openvsx/scanning/VSCodeGalleryExistenceCheckScannerTest.java:57
- Minor grammar in the comment: "irrelevant of content" is unidiomatic and reads like a typo; rephrase to "content is irrelevant" (or similar) for clarity.
/**
* Dummy non-empty result; irrelevant of content
*/
Do not check if extension is already active.
A new scanner, that on new extension publish (existing active ones are skipped;configurable) checks upstream gallery for same NS+extension, and if exists, enforces locally that target NS is a verified NS.
Changes: