Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -440,22 +440,29 @@ public CompletedScanResult processCompletedScan(
String summary;
ScanCheckResult.CheckResult checkResult;

if (result.isClean()) {
if (result.isClean() && !result.hasMaliciousVerdict()) {
checkResult = ScanCheckResult.CheckResult.PASSED;
summary = "No threats found";
} else {
threatCount = result.getThreats().size();

// A benign verdict may only downgrade enforcement (never quarantine on findings alone); a malicious
// verdict may never upgrade past the scanner's own enforced setting
boolean findingsEnforced = scannerEnforced && !result.hasBenignVerdict();

// Save threats and get enforcement statistics
var saveResult = saveThreats(job, result, scannerEnforced);
var saveResult = saveThreats(job, result, findingsEnforced);

// Determine check result based on actual enforcement (considers allowlist)
if (saveResult.hasEnforcedThreats()) {
boolean unmitigatedMaliciousVerdict = scannerEnforced && result.hasMaliciousVerdict() && threatCount == 0;
if (saveResult.hasEnforcedThreats() || unmitigatedMaliciousVerdict) {
Comment on lines +457 to +458

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

checkResult = ScanCheckResult.CheckResult.QUARANTINE;
summary = String.format(
"Found %d threat(s) - %d enforced",
threatCount,
saveResult.enforcedCount());
summary = threatCount == 0
? "Marked malicious by scanner verdict (no specific findings)"
: String.format(
"Found %d threat(s) - %d enforced",
threatCount,
saveResult.enforcedCount());
} else {
// Threats found but none enforced (scanner not enforced OR all on allowlist)
checkResult = ScanCheckResult.CheckResult.PASSED;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,23 @@ public String extractString(String response, String format, String path) throws
};
}

/**
* Extract a boolean value from a response. Accepts JSON booleans and string forms ("true"/"false"). Returns null
* when the path is missing or the value is not a boolean.
*/
public Boolean extractBoolean(String response, String format, String path) throws ScannerException {
if (response == null || path == null) {
return null;
}

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove them

case "text" -> throw new UnsupportedOperationException("Text extraction not yet implemented");
default -> throw new IllegalArgumentException("Unsupported format: " + format);
};
}

/**
* Extract a list of objects from a response.
*/
Expand Down Expand Up @@ -79,6 +96,26 @@ private String extractJsonString(String json, String jsonPath) throws ScannerExc
return node != null && !node.isMissingNode() ? node.asString(null) : null;
}

private Boolean extractJsonBoolean(String json, String jsonPath) throws ScannerException {
JsonNode node = pickFirst(parseJson(json), jsonPath);
if (node == null || node.isMissingNode() || node.isNull()) {
return null;
}
if (node.isBoolean()) {
return node.asBoolean();
}
if (node.isString()) {
String value = node.asString(null);
if ("true".equalsIgnoreCase(value)) {
return true;
}
if ("false".equalsIgnoreCase(value)) {
return false;
}
}
return null;
}

private List<Map<String, Object>> extractJsonList(String json, String jsonPath) throws ScannerException {
JsonNode root = parseJson(json);
List<JsonNode> nodes = evaluateJsonPath(root, jsonPath);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import java.io.File;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -405,26 +406,33 @@ private Scanner.Result parseResult(
responseConfig.getSummaryPath());
}

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// a scan failure
Boolean maliciousVerdict = null;
if (responseConfig.getMaliciousPath() != null) {
maliciousVerdict = responseExtractor.extractBoolean(
response,
responseConfig.getFormat(),
responseConfig.getMaliciousPath());
if (maliciousVerdict == null) {
throw new ScannerException(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aren't we already failing for similar reasons above?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"Scanner '" + scannerName + "' has malicious-path '" + responseConfig.getMaliciousPath()
+ "' configured, but it did not resolve to a boolean in the response");
}
}

// Extract threats
String threatsPath = responseConfig.getThreatsPath();
if (threatsPath == null) {
// No threats path - assume clean
return Scanner.Result.clean(summary);
List<Scanner.Threat> threats = Collections.emptyList();
if (threatsPath != null) {
List<Map<String, Object>> threatObjects = responseExtractor.extractList(
response,
responseConfig.getFormat(),
threatsPath);
threats = mapThreats(threatObjects, responseConfig);
}

List<Map<String, Object>> threatObjects = responseExtractor.extractList(
response,
responseConfig.getFormat(),
threatsPath);

// Map threats
List<Scanner.Threat> threats = mapThreats(threatObjects, responseConfig);

if (threats.isEmpty()) {
return Scanner.Result.clean(summary);
} else {
return Scanner.Result.withThreats(threats, summary);
}
return Scanner.Result.of(threats, summary, maliciousVerdict);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,12 @@ public static class ResponseConfig {
// Surfaced verbatim in ScanCheckResult.summary when present
private String summaryPath;

/**
* 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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah lets rename that to maliciousVerdictPath to make it clear

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will rename 👍


// For error detection
private String errorPath; // Path to error message
private String errorCondition; // Expression to detect errors
Expand Down Expand Up @@ -527,6 +533,13 @@ public void setSummaryPath(String summaryPath) {
this.summaryPath = summaryPath;
}

public String getMaliciousPath() {
return maliciousPath;
}
public void setMaliciousPath(String maliciousPath) {
this.maliciousPath = maliciousPath;
}

public String getErrorPath() {
return errorPath;
}
Expand Down
47 changes: 42 additions & 5 deletions server/src/main/java/org/eclipse/openvsx/scanning/Scanner.java
Original file line number Diff line number Diff line change
Expand Up @@ -67,33 +67,51 @@ class Result {
private final boolean clean;
private final List<Threat> threats;
private final String summary;
private final Boolean maliciousVerdict;

private Result(boolean clean, List<Threat> threats, String summary) {
private Result(boolean clean, List<Threat> threats, String summary, Boolean maliciousVerdict) {
this.clean = clean;
this.threats = new ArrayList<>(threats);
this.summary = summary;
this.maliciousVerdict = maliciousVerdict;
}

@NonNull
public static Result clean() {
return new Result(true, Collections.emptyList(), null);
return new Result(true, Collections.emptyList(), null, null);
}

@NonNull
public static Result clean(@Nullable String summary) {
return new Result(true, Collections.emptyList(), summary);
return new Result(true, Collections.emptyList(), summary, null);
}

@NonNull
public static Result withThreats(@NonNull List<Threat> threats) {
return new Result(false, threats, null);
return new Result(false, threats, null, null);
}

@NonNull
public static Result withThreats(@NonNull List<Threat> threats, @Nullable String summary) {
return new Result(false, threats, summary);
return new Result(false, threats, summary, null);
}

/**
* Builds a result directly from a scanner's findings and its explicit malicious verdict (e.g. Argus
* verdictData.isMalicious). Prefer this over clean()/withThreats() whenever a verdict is available: naming a
* malicious result via "clean" reads as contradictory, since findings and the safety verdict are independent
* signals here. "Clean" (no findings) is derived from the threats list itself.
*/
@NonNull
public static Result of(@NonNull List<Threat> threats, @Nullable String summary, @Nullable Boolean malicious) {
return new Result(threats.isEmpty(), threats, summary, malicious);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@netomi netomi Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
* True when there are no findings recorded. This reflects the threats list only — it is not a safety verdict. A
* malicious verdict with zero findings is still isClean() == true; check {@link #hasMaliciousVerdict()} for the
* actual verdict.
*/
public boolean isClean() {
return clean;
}
Expand All @@ -108,6 +126,25 @@ public List<Threat> getThreats() {
public String getSummary() {
return summary;
}

/**
* Explicit malicious verdict from the scanner response, if configured. null means the scanner does not expose
* this signal.
*/
@Nullable
public Boolean isMalicious() {
return maliciousVerdict;
}

/** True when the scanner explicitly returned a malicious verdict. */
public boolean hasMaliciousVerdict() {
return Boolean.TRUE.equals(maliciousVerdict);
}

/** True when the scanner explicitly returned a non-malicious verdict. */
public boolean hasBenignVerdict() {
return Boolean.FALSE.equals(maliciousVerdict);
}
}

/**
Expand Down
Loading