feat: quarantine based on scanner malicious verdict - #1991
Conversation
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.
There was a problem hiding this comment.
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
maliciousVerdictsupport toScanner.Result(with helpers) and extend result construction viaResult.of(...). - Add
maliciousPathto remote-scanner response configuration and implement boolean extraction inHttpResponseExtractor. - 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.
gnugomez
left a comment
There was a problem hiding this comment.
overall is looking good to me, the tests are really helpful.
- Rename malicious-path to malicious-verdict-path for clarity - Let unsupported response formats fall through to the default case instead of throwing per-format exceptions - Break parseResult into smaller, single-purpose methods - Fall back to finding-based enforcement (with a warning) instead of failing the scan when the configured verdict path doesn't resolve to a boolean, since that can happen for benign reasons (misconfig, provider response changes) and shouldn't block scanning outright - Make an explicit malicious verdict the sole gatekeeper for Result.isClean(): a malicious verdict is never clean regardless of finding count, and a benign verdict is always clean regardless of finding count, removing the previous contradiction where a result could be isClean() and isMalicious() at the same time
netomi
left a comment
There was a problem hiding this comment.
LGTM, thanks for the changes, it is now much more clear imho.
The reason why I wanted to make the maliciousVerdict path fail-safe is to be more independent of 3rd party changes, so if this is not present although its configured, it just falls back to the case without it which is perfectly fine imho.
Wire malicious-verdict-path to $.verdictData.isMalicious so quarantine follows the explicit Argus verdict from eclipse-openvsx/openvsx#1991. Rebase of #13402 (feat/argus-malicious-verdict by janbro) onto current aws-main -- that branch predated the openvsx-ef image split entirely, so everything else in it (Dockerfile, yara MAX_RECURSION, website deps, issue template) was stale drift already resolved elsewhere. Targets application-staging.yml since that's now where Argus config for staging lives (see #13408). Co-authored-by: janbro <1494590+janbro@users.noreply.github.com> Signed-off-by: Thomas Neidhart <thomas.neidhart@gmail.com>
Wire malicious-verdict-path to $.verdictData.isMalicious so quarantine follows the explicit Argus verdict from eclipse-openvsx/openvsx#1991. Rebase of #13402 (feat/argus-malicious-verdict by janbro) onto current aws-main -- that branch predated the openvsx-ef image split entirely, so everything else in it (Dockerfile, yara MAX_RECURSION, website deps, issue template) was stale drift already resolved elsewhere. Targets application-staging.yml since that's now where Argus config for staging lives (see #13408). Signed-off-by: Thomas Neidhart <thomas.neidhart@gmail.com> Co-authored-by: janbro <1494590+janbro@users.noreply.github.com>
Summary
Some security scanners return an explicit
isMaliciousverdict 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.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
enforcedsetting allows. A scanner explicitly configured withenforced: falsenever quarantines, regardless of verdict.Follow-up
This is the first stage of enabling
isMaliciousverdict parsing for existing scanners in production. After this PR is merged, application configuration will need to be updated to add theisMaliciousJSONPath to each scanner's result mapping.