feat: quarantine based on scanner malicious verdict by janbro · Pull Request #1991 · eclipse-openvsx/openvsx

janbro · 2026-07-25T00:22:49Z

Summary

Some security scanners return an explicit isMalicious verdict rather than relying solely on rule/finding matches. This adds support for scanners to report that verdict and uses it to drive quarantine decisions:

malicious = true → the extension is quarantined, even if there are no other findings.
malicious = false → findings are recorded for the audit trail but treated as warnings only, not enforced.
No verdict returned (null) → falls back to the existing behavior, enforcing based on findings alone.

The verdict can only narrow enforcement, never widen it past what a scanner's own enforced setting allows. A scanner explicitly configured with enforced: false never quarantines, regardless of verdict.

Follow-up

This is the first stage of enabling isMalicious verdict parsing for existing scanners in production. After this PR is merged, application configuration will need to be updated to add the isMalicious JSONPath to each scanner's result mapping.

Some security providers return an explicit isMalicious verdict rather than relying solely on rule/finding matches. Add support for scanners to report this verdict; quarantine is enforced when malicious is true, and findings are recorded as warnings when malicious is false.

Reformat multi-argument calls in ExtensionScanPersistenceServiceTest and RemoteScannerTest to match the project's eclipse formatter output.

Copilot

Pull request overview

Adds support for an explicit scanner isMalicious verdict (in addition to traditional finding/rule matches) and wires it through result parsing to influence quarantine decisions while respecting each scanner’s enforced setting.

Changes:

Add maliciousVerdict support to Scanner.Result (with helpers) and extend result construction via Result.of(...).
Add maliciousPath to remote-scanner response configuration and implement boolean extraction in HttpResponseExtractor.
Update completed-scan processing to factor verdicts into enforcement/quarantine outcomes; add focused unit tests.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
server/src/main/java/org/eclipse/openvsx/scanning/Scanner.java	Introduces `maliciousVerdict` on scan results plus helper methods and a new `Result.of(...)` builder.
server/src/main/java/org/eclipse/openvsx/scanning/RemoteScannerProperties.java	Adds `maliciousPath` to response config for mapping scanner verdict booleans.
server/src/main/java/org/eclipse/openvsx/scanning/HttpResponseExtractor.java	Implements `extractBoolean(...)` used to read `isMalicious` from responses.
server/src/main/java/org/eclipse/openvsx/scanning/RemoteScanner.java	Extracts and validates malicious verdict when configured; always returns `Scanner.Result.of(...)`.
server/src/main/java/org/eclipse/openvsx/scanning/ExtensionScanPersistenceService.java	Updates quarantine/enforcement logic to account for explicit benign/malicious verdicts.
server/src/test/java/org/eclipse/openvsx/scanning/ScannerTest.java	Adds unit tests for `Scanner.Result` malicious-verdict helpers.
server/src/test/java/org/eclipse/openvsx/scanning/ResponseExtractorTest.java	Adds unit tests for boolean extraction via JSONPath.
server/src/test/java/org/eclipse/openvsx/scanning/RemoteScannerTest.java	Adds tests for fail-closed behavior when `maliciousPath` is configured but missing/invalid.
server/src/test/java/org/eclipse/openvsx/scanning/RemoteScannerPropertiesTest.java	Extends response-config setter/getter coverage for `maliciousPath`.
server/src/test/java/org/eclipse/openvsx/scanning/ExtensionScanPersistenceServiceTest.java	Adds tests covering verdict vs `enforced` and allowlist interactions in quarantine decisions.

Comments suppressed due to low confidence (1)

server/src/main/java/org/eclipse/openvsx/scanning/ExtensionScanPersistenceService.java:465

The auto-generated quarantine summary for a malicious verdict (e.g. "Marked malicious by scanner verdict...") can be overwritten a few lines later by a scanner-provided summary. That can leave the audit trail showing an unrelated/benign message (e.g. "ok") even though the extension was quarantined due to the malicious verdict. Consider preserving the reason by appending a short suffix when quarantining on a malicious verdict.

                checkResult = ScanCheckResult.CheckResult.QUARANTINE;
                summary = threatCount == 0
                        ? "Marked malicious by scanner verdict (no specific findings)"
                        : String.format(
                                "Found %d threat(s) - %d enforced",
                                threatCount,
                                saveResult.enforcedCount());

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

janbro · 2026-07-27T17:57:14Z

+            boolean unmitigatedMaliciousVerdict = scannerEnforced && result.hasMaliciousVerdict() && threatCount == 0;
+            if (saveResult.hasEnforcedThreats() || unmitigatedMaliciousVerdict) {


This functionality is intended to allow open vsx maintainers to override known false positives returned from a provider, given the file hash of the matched behavior is exactly the same.

gnugomez

overall is looking good to me, the tests are really helpful.

gnugomez · 2026-07-27T08:46:00Z

+
+        return switch (format.toLowerCase()) {
+            case "json" -> extractJsonBoolean(response, path);
+            case "xml" -> throw new UnsupportedOperationException("XML extraction not yet implemented");


question: if not supported "yet" why don't just make it fall under default?

yeah, remove all unsupported exceptions and let it fall through to the default case

Can remove them

gnugomez · 2026-07-27T08:53:18Z

                    responseConfig.getSummaryPath());
        }

+        // Extract explicit malicious verdict, if configured. A configured path that fails to resolve is treated as


thought (non-blocking): given that the scanner result parsing method it's growing it might be worth considering to break it down into some easy to digest methods.

I’ll give it light refactor to make it more readable

gnugomez · 2026-07-27T08:59:12Z

+         * JSONPath to a boolean malicious verdict on the result response (e.g. "$.verdictData.isMalicious"). When set,
+         * quarantine is driven by this verdict rather than "any findings present".
+         */
+        private String maliciousPath;


question (non-blocking): if this points to a verdict, why don't we name it after that? malicious could become misleading since it doesn't implies the boolean nature that the value will have.

yeah lets rename that to maliciousVerdictPath to make it clear

Will rename 👍

netomi · 2026-07-27T09:21:14Z

+                    responseConfig.getFormat(),
+                    responseConfig.getMaliciousPath());
+            if (maliciousVerdict == null) {
+                throw new ScannerException(


If a malicousPath is defined but could not be extracted that could be due to various reasons:

misconfiguration

service did not include the verdict

Failing the scan if the verdict is not present is oth not desirable. If no verdict is present then we should continue with the normal threats path imho to make that more failure resiliient.

aren't we already failing for similar reasons above?

I had made the behavior fail closed rather than just warn since a misconfiguration for receiving verdicts from a provider felt significant enough to raise immediate concerns. Otherwise the configured integration may be silently (or quietly) allowing publishing of extension with malicious verdicts due to a misconfiguration or API change of the underlying provider. We do still fallback to the legacy behavior where any findings block publishing, so if instead its easier to setup monitoring we could just log a warning

netomi · 2026-07-27T09:27:28Z

+         */
+        @NonNull
+        public static Result of(@NonNull List<Threat> threats, @Nullable String summary, @Nullable Boolean malicious) {
+            return new Result(threats.isEmpty(), threats, summary, malicious);


Is this really correct? If there are no threats but the scan would still bear a maliciousVerdict the result would still be clean.

I have seen cases were the threat score was high but there was no specific threat identified and the scan was marked clean. I think the maliciousVerdict should also act as a gate keeper: if its true, the scan should not be marked as clean at all.

I see in the code above the 2 states are treated differently:

clean

malicious verdict

however that is also a bit error prone as you need to take that always into account when doing changes to this code

so a result can be isClean() while at the same time isMalicious()

Yeah clean was originally assigned to scans which returned no findings, which was associated with a clean verdict but in doing so conflated the meaning. Will see what I can do to see if we can strictly use that to refer to verdicts and potentially drop clean as an association with the number of findings

janbro added 2 commits July 24, 2026 20:21

style: apply eclipse formatter line wrapping to scanning tests

4eeebba

Reformat multi-argument calls in ExtensionScanPersistenceServiceTest and RemoteScannerTest to match the project's eclipse formatter output.

netomi requested review from Copilot and netomi July 27, 2026 08:45

Copilot started reviewing on behalf of netomi July 27, 2026 08:46 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

gnugomez reviewed Jul 27, 2026

View reviewed changes

netomi reviewed Jul 27, 2026

View reviewed changes

		boolean unmitigatedMaliciousVerdict = scannerEnforced && result.hasMaliciousVerdict() && threatCount == 0;
		if (saveResult.hasEnforcedThreats() \|\| unmitigatedMaliciousVerdict) {

Uh oh!

Conversation

janbro commented Jul 25, 2026

Summary

Follow-up

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnugomez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

netomi Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netomi Jul 27, 2026 •

edited

Loading