Skip to content

Commit 5d641f4

Browse files
netominetomi
authored
Change: make malicious zip file checks a synchronous PublishCheck (#1721)
* change: make malicious zip file checks a synchronous PublishCheck if scanning is enabled and ensure that the flag potentially_malicious is correctly persisted otherwise * rework PR to not pass an existing ExtensionProcessor for PublishChecks but rather create it on demand as its cheap, also add a unit test * remove spurious debug statement
1 parent fe0f003 commit 5d641f4

18 files changed

+228
-113
lines changed

‎server/src/dev/resources/application.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ ovsx:
206206
"contact": "infrastructure@eclipse-foundation.org"
207207
}
208208
scanning:
209-
enabled: false
209+
enabled: true
210210
# Shared archive limits for all scanning checks (secret detection, blocklist, etc.)
211211
max-archive-size-bytes: 1073741824 # 1 GB total archive limit
212212
max-single-file-bytes: 268435456 # 256 MB per-file limit

‎server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionHandler.java‎

Lines changed: 7 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,8 @@
2020
import org.eclipse.openvsx.UserService;
2121
import org.eclipse.openvsx.adapter.VSCodeIdNewExtensionJobRequest;
2222
import org.eclipse.openvsx.entities.*;
23-
import org.eclipse.openvsx.entities.ScanCheckResult.CheckCategory;
24-
import org.eclipse.openvsx.entities.ScanCheckResult.CheckResult;
2523
import org.eclipse.openvsx.extension_control.ExtensionControlService;
2624
import org.eclipse.openvsx.repositories.RepositoryService;
27-
import org.eclipse.openvsx.scanning.ExtensionScanPersistenceService;
2825
import org.eclipse.openvsx.scanning.ExtensionScanService;
2926
import org.eclipse.openvsx.util.*;
3027
import org.jobrunr.scheduling.JobRequestScheduler;
@@ -58,7 +55,6 @@ public class PublishExtensionVersionHandler {
5855
private final ExtensionValidator validator;
5956
private final ExtensionControlService extensionControl;
6057
private final ExtensionScanService scanService;
61-
private final ExtensionScanPersistenceService scanPersistenceService;
6258

6359
public PublishExtensionVersionHandler(
6460
PublishExtensionVersionService service,
@@ -69,8 +65,7 @@ public PublishExtensionVersionHandler(
6965
UserService users,
7066
ExtensionValidator validator,
7167
ExtensionControlService extensionControl,
72-
ExtensionScanService scanService,
73-
ExtensionScanPersistenceService scanPersistenceService
68+
ExtensionScanService scanService
7469
) {
7570
this.service = service;
7671
this.integrityService = integrityService;
@@ -81,7 +76,6 @@ public PublishExtensionVersionHandler(
8176
this.validator = validator;
8277
this.extensionControl = extensionControl;
8378
this.scanService = scanService;
84-
this.scanPersistenceService = scanPersistenceService;
8579
}
8680

8781
public boolean isLicenseRequired() {
@@ -276,48 +270,15 @@ private void doPublish(TempFile extensionFile, ExtensionService extensionService
276270

277271
service.storeResource(extensionFile);
278272
service.persistResource(download);
279-
try(var processor = new ExtensionProcessor(extensionFile)) {
280-
extVersion.setPotentiallyMalicious(processor.isPotentiallyMalicious());
281-
if (extVersion.isPotentiallyMalicious()) {
273+
try (var processor = new ExtensionProcessor(extensionFile)) {
274+
// to keep backwards compatibility, mark extension versions as potentially malicious
275+
// if no scan service is enabled and the vsix file contains entries with extra fields.
276+
if (!scanService.isEnabled() && processor.isPotentiallyMalicious()) {
277+
service.markExtensionAsPotentiallyMalicious(extVersion);
282278
logger.atWarn()
283279
.setMessage("Extension version is potentially malicious: {}")
284280
.addArgument(() -> NamingUtil.toLogFormat(extVersion))
285281
.log();
286-
287-
// Record as a publish check failure and reject the extension
288-
if (scan != null) {
289-
var now = TimeUtil.getCurrentUTC();
290-
var checkType = "MALICIOUS_ZIP_CHECK";
291-
var reason = "VSIX contains zip entries with potentially harmful extra fields";
292-
293-
// Record the check result for audit trail
294-
scanPersistenceService.recordCheckResult(
295-
scan,
296-
checkType,
297-
CheckCategory.PUBLISH_CHECK,
298-
CheckResult.REJECT,
299-
now, // startedAt
300-
now, // completedAt
301-
1, // filesScanned - the vsix file
302-
1, // findingsCount
303-
reason,
304-
null, // errorMessage
305-
null, // scannerJobId - not a scanner job
306-
true
307-
);
308-
309-
// Also record as validation failure for the failures list
310-
scanPersistenceService.recordValidationFailure(
311-
scan,
312-
checkType,
313-
"EXTRA_FIELDS_DETECTED", // ruleName
314-
reason,
315-
true // enforced
316-
);
317-
318-
scanService.rejectScan(scan);
319-
logger.info("Scan {} rejected due to potentially malicious extension", scan.getId());
320-
}
321282
return;
322283
}
323284

@@ -326,7 +287,7 @@ private void doPublish(TempFile extensionFile, ExtensionService extensionService
326287
service.persistResource(tempFile.getResource());
327288
};
328289

329-
if(integrityService.isEnabled()) {
290+
if (integrityService.isEnabled()) {
330291
var keyPair = extVersion.getSignatureKeyPair();
331292
if(keyPair != null) {
332293
try(var signature = integrityService.generateSignature(extensionFile, keyPair)) {

‎server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionService.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,12 @@ public void persistResource(FileResource resource) {
6666
entityManager.persist(resource);
6767
}
6868

69+
@Transactional
70+
public void markExtensionAsPotentiallyMalicious(ExtensionVersion extVersion) {
71+
extVersion = entityManager.merge(extVersion);
72+
extVersion.setPotentiallyMalicious(true);
73+
}
74+
6975
@Transactional
7076
@CacheEvict(value = CACHE_SITEMAP, allEntries = true)
7177
public void activateExtension(ExtensionVersion extVersion, ExtensionService extensions) {

‎server/src/main/java/org/eclipse/openvsx/scanning/BlocklistCheckService.java‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -97,10 +97,6 @@ public String getUserFacingMessage(List<Failure> failures) {
9797

9898
@Override
9999
public Result check(Context context) {
100-
if (context.extensionFile() == null) {
101-
return Result.pass();
102-
}
103-
104100
var blockedFiles = checkForBlockedFiles(context.extensionFile());
105101
if (blockedFiles.isEmpty()) {
106102
return Result.pass();

‎server/src/main/java/org/eclipse/openvsx/scanning/ExtensionScanCompletionService.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import org.springframework.transaction.annotation.Transactional;
3636

3737
import java.time.LocalDateTime;
38+
import java.util.ArrayList;
3839
import java.util.HashSet;
3940
import java.util.List;
4041
import java.util.Set;
@@ -457,8 +458,8 @@ private boolean completeExtensionScan(String scanId, List<ScannerJob> jobs) {
457458
if (!failedJobs.isEmpty()) {
458459
// Check if any REQUIRED scanners failed
459460
// Optional scanners (typically external) can fail without blocking activation
460-
List<ScannerJob> requiredFailedJobs = new java.util.ArrayList<>();
461-
List<ScannerJob> optionalFailedJobs = new java.util.ArrayList<>();
461+
List<ScannerJob> requiredFailedJobs = new ArrayList<>();
462+
List<ScannerJob> optionalFailedJobs = new ArrayList<>();
462463

463464
for (ScannerJob failedJob : failedJobs) {
464465
// Record ERROR result for audit trail (if not already recorded)

‎server/src/main/java/org/eclipse/openvsx/scanning/ExtensionScanService.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,10 @@
2222
import org.jobrunr.scheduling.JobRequestScheduler;
2323
import org.slf4j.Logger;
2424
import org.slf4j.LoggerFactory;
25-
import org.springframework.dao.DataIntegrityViolationException;
2625
import org.springframework.stereotype.Component;
2726

2827
import jakarta.annotation.Nonnull;
2928
import jakarta.annotation.Nullable;
30-
import java.time.LocalDateTime;
3129
import java.util.List;
3230
import java.util.stream.Collectors;
3331

@@ -111,7 +109,7 @@ private ExtensionScan initializeScan(
111109

112110
/**
113111
* Run validation checks and record results.
114-
*
112+
* <p>
115113
* Delegates the actual check execution to ExtensionScanner,
116114
* then records findings and manages state transitions.
117115
*/

‎server/src/main/java/org/eclipse/openvsx/scanning/GitleaksRulesService.java‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -243,29 +243,29 @@ private void startRedisSubscriber() {
243243

244244
private void subscribeLoop() {
245245
AtomicInteger backoffMs = new AtomicInteger(1000);
246-
ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(
247-
new NamedThreadFactory("gitleaks-rules-reconnect"));
248246

249-
while (running && !Thread.currentThread().isInterrupted()) {
250-
ScheduledFuture<?> resetTask = null;
251-
try {
252-
resetTask = executor.schedule(() -> backoffMs.set(1000), 10, TimeUnit.SECONDS);
253-
logger.debug("Subscribing to gitleaks rules update channel");
254-
jedisCluster.subscribe(this, RULES_UPDATE_CHANNEL);
255-
} catch (Exception e) {
256-
if (!running) break;
257-
logger.warn("Gitleaks rules subscriber disconnected, reconnecting in {}s: {}",
258-
backoffMs.get() / 1000, e.getMessage());
259-
if (resetTask != null) resetTask.cancel(true);
247+
try (var executor = Executors.newSingleThreadScheduledExecutor(new NamedThreadFactory("gitleaks-rules-reconnect"))) {
248+
while (running && !Thread.currentThread().isInterrupted()) {
249+
ScheduledFuture<?> resetTask = null;
260250
try {
261-
Thread.sleep(backoffMs.get());
262-
backoffMs.set(Math.min(backoffMs.get() * 2, 30000));
263-
} catch (InterruptedException ignored) {
264-
break;
251+
resetTask = executor.schedule(() -> backoffMs.set(1000), 10, TimeUnit.SECONDS);
252+
logger.debug("Subscribing to gitleaks rules update channel");
253+
jedisCluster.subscribe(this, RULES_UPDATE_CHANNEL);
254+
} catch (Exception e) {
255+
if (!running) break;
256+
logger.warn("Gitleaks rules subscriber disconnected, reconnecting in {}s: {}",
257+
backoffMs.get() / 1000, e.getMessage());
258+
if (resetTask != null) resetTask.cancel(true);
259+
try {
260+
Thread.sleep(backoffMs.get());
261+
backoffMs.set(Math.min(backoffMs.get() * 2, 30000));
262+
} catch (InterruptedException ignored) {
263+
break;
264+
}
265265
}
266266
}
267+
executor.shutdownNow();
267268
}
268-
executor.shutdownNow();
269269
}
270270

271271
@Override

‎server/src/main/java/org/eclipse/openvsx/scanning/HttpClientExecutor.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,10 @@
3636

3737
/**
3838
* Executes HTTP requests based on configuration.
39-
*
39+
* <p>
4040
* Handles different HTTP methods, body types (JSON, multipart, form-urlencoded),
4141
* headers, query parameters, file uploads, and authentication.
42-
*
42+
* <p>
4343
* Use static factory methods to create instances with scanner-specific configs.
4444
*/
4545
public class HttpClientExecutor {

‎server/src/main/java/org/eclipse/openvsx/scanning/HttpTemplateEngine.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public String process(String template, Map<String, String> placeholders) {
4848
}
4949

5050
Matcher matcher = PLACEHOLDER_PATTERN.matcher(template);
51-
StringBuffer result = new StringBuffer();
51+
StringBuilder result = new StringBuilder();
5252

5353
while (matcher.find()) {
5454
String placeholderName = matcher.group(1);

‎server/src/main/java/org/eclipse/openvsx/scanning/MaliciousZipCheckService.java‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/********************************************************************************
2+
* Copyright (c) 2026 Contributors to the Eclipse Foundation
3+
*
4+
* See the NOTICE file(s) distributed with this work for additional
5+
* information regarding copyright ownership.
6+
*
7+
* This program and the accompanying materials are made available under the
8+
* terms of the Eclipse Public License 2.0 which is available at
9+
* https://www.eclipse.org/legal/epl-2.0
10+
*
11+
* SPDX-License-Identifier: EPL-2.0
12+
********************************************************************************/
13+
package org.eclipse.openvsx.scanning;
14+
15+
import org.eclipse.openvsx.ExtensionProcessor;
16+
import org.springframework.core.annotation.Order;
17+
import org.springframework.stereotype.Service;
18+
19+
import java.util.List;
20+
21+
/**
22+
* Service for checking extension files for potentially malicious zip extra fields.
23+
* <p>
24+
* Implements PublishCheck to be auto-discovered by PublishCheckRunner.
25+
* Always enabled and enforced.
26+
*/
27+
@Service
28+
@Order(0)
29+
public class MaliciousZipCheckService implements PublishCheck {
30+
31+
public static final String CHECK_TYPE = "MALICIOUS_ZIP_CHECK";
32+
private static final String RULE_NAME = "EXTRA_FIELDS_DETECTED";
33+
private static final String MESSAGE = "extension file contains zip entries with potentially harmful extra fields";
34+
private static final String USER_MESSAGE = "Extension contains zip entries with unsupported extra fields";
35+
36+
@Override
37+
public String getCheckType() {
38+
return CHECK_TYPE;
39+
}
40+
41+
@Override
42+
public boolean isEnabled() {
43+
return true;
44+
}
45+
46+
@Override
47+
public boolean isEnforced() {
48+
return true;
49+
}
50+
51+
@Override
52+
public String getUserFacingMessage(List<Failure> failures) {
53+
return USER_MESSAGE;
54+
}
55+
56+
@Override
57+
public PublishCheck.Result check(Context context) {
58+
try (var processor = new ExtensionProcessor(context.extensionFile())) {
59+
var potentiallyMalicious = processor.isPotentiallyMalicious();
60+
if (potentiallyMalicious) {
61+
return PublishCheck.Result.fail(RULE_NAME, MESSAGE);
62+
}
63+
}
64+
65+
return PublishCheck.Result.pass();
66+
}
67+
}

0 commit comments

Comments
 (0)