From 05618923f038d51ad280618a77626987e404304d Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Thu, 3 Sep 2026 22:32:44 +0200 Subject: [PATCH] refactor: replace the remaining EntityManager.merge updates with find Second and final batch of the #989 audit, covering the 13 sites left after the previous one. These all did intend an update, so the hazard was subtler: merge writes every column of the detached copy, meaning any field that moved on the row since the caller loaded it is silently reverted. That is how the original outage happened, and these sites had long windows - a VSIX download and scan, a storage round-trip - between load and write. Where the service itself owns the mutation, it now finds the row by id and sets just its own field: EclipseService.updateUserData, CustomerService.deactivateRateLimitToken, ExtensionVersionIntegrityService.setSignatureKeyPair, PublishExtensionVersionService.markExtensionAsPotentiallyMalicious, AccessTokenService.scheduleTokenExpirationNotification, SetPreReleaseJobService and CheckPotentiallyMaliciousExtensionVersionsService. The last two are the pair that could revert each other: both loaded an ExtensionVersion, did slow VSIX work, then merged the whole row, so whichever finished second undid the other's field. PublishExtensionVersionService.activateExtension already used find, so this makes its neighbours consistent with it. MigrationService.updateResource and RenameDownloadsService.updateResource are now updateResourceSize and updateResourceName. Their old signatures could not say which field the caller had changed, which is precisely why they had to merge the whole row; naming them for the field they persist removes the hazard and stops the next caller expecting a different field to be written. Each had exactly one caller. The merge-before-remove pair (MigrationService.deleteFileResource and the old namespace in ChangeNamespaceService) now find and remove, skipping a row that has already gone instead of having merge try to resurrect it. ChangeNamespaceService keeps one merge, now documented. Its caller mutates the detached namespace before handing it over (ChangeNamespaceJobRequestHandler sets logoName), so that update belongs to the caller and only merge can carry it - find-then-set would silently drop the logo rename. Its updatedResources loop, by contrast, only ever changes the name, so that one writes just the name. Three new tests on markExtensionAsPotentiallyMalicious cover the flag landing on the managed row, a concurrent activation surviving the write (the reverted-column case), and a version purged in the meantime being ignored. Fixes #989 Co-Authored-By: Claude Opus 5 (1M context) --- .../accesstoken/AccessTokenService.java | 13 ++++-- .../openvsx/admin/ChangeNamespaceService.java | 28 ++++++++++-- .../openvsx/eclipse/EclipseService.java | 10 ++++- ...allyMaliciousExtensionVersionsService.java | 14 ++++-- .../FileResourceSizeJobRequestHandler.java | 2 +- .../openvsx/migration/MigrationService.java | 19 ++++++-- .../RenameDownloadsJobRequestHandler.java | 2 +- .../migration/RenameDownloadsService.java | 12 ++++- .../migration/SetPreReleaseJobService.java | 16 +++++-- .../ExtensionVersionIntegrityService.java | 10 ++++- .../PublishExtensionVersionService.java | 10 ++++- .../openvsx/ratelimit/CustomerService.java | 15 +++++-- ...FileResourceSizeJobRequestHandlerTest.java | 8 ++-- .../PublishExtensionVersionServiceTest.java | 45 +++++++++++++++++++ 14 files changed, 170 insertions(+), 34 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java index a3b821f6f..6a56bdb54 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java @@ -377,12 +377,17 @@ public int expireAccessTokens() { @Transactional public void scheduleTokenExpirationNotification(PersonalAccessToken token) { - token = entityManager.merge(token); - if (token.getType().isNotify() && !token.isNotified()) { + // find, not merge: only `notified` is this method's to change, and merging the whole + // detached token reverted any column that moved since it was loaded - see #989. + var managedToken = entityManager.find(PersonalAccessToken.class, token.getId()); + if (managedToken == null) { + return; + } + if (managedToken.getType().isNotify() && !managedToken.isNotified()) { try { - mail.scheduleAccessTokenExpiryNotification(token); + mail.scheduleAccessTokenExpiryNotification(managedToken); } finally { - token.setNotified(true); + managedToken.setNotified(true); } } } diff --git a/server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java b/server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java index 78dad75f5..40d0c38b7 100644 --- a/server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java +++ b/server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java @@ -61,16 +61,24 @@ public void changeNamespaceInDatabase( if (createNewNamespace) { entityManager.persist(newNamespace); } else { + // Deliberately a merge, unlike the rest of #989's call sites: the caller mutates this + // detached namespace before handing it over (ChangeNamespaceJobRequestHandler sets + // logoName on it), so the update belongs to the caller and only merge can carry it. + // find-then-set would silently drop that rename. newNamespace = entityManager.merge(newNamespace); } changeExtensionNamespace(extensions, newNamespace); changeMembershipNamespace(oldNamespace, newNamespace, removeOldNamespace); - updatedResources.forEach(entityManager::merge); + renameResources(updatedResources); if (removeOldNamespace) { - oldNamespace = entityManager.merge(oldNamespace); - entityManager.remove(oldNamespace); + // find, not merge: this row is about to go, so merging every column of a detached copy + // first was a wasted UPDATE - and would have resurrected a row already deleted. + var managedOldNamespace = entityManager.find(Namespace.class, oldNamespace.getId()); + if (managedOldNamespace != null) { + entityManager.remove(managedOldNamespace); + } } cache.evictSitemap(); @@ -78,6 +86,20 @@ public void changeNamespaceInDatabase( search.updateSearchEntries(extensions.filter(Extension::isActive).toList()); } + /** + * Applies the names the caller computed for the copied resources. Only the name changes there + * (see {@code ChangeNamespaceJobRequestHandler}), so this writes that field rather than merging + * every column of each detached resource back - see #989. + */ + private void renameResources(List updatedResources) { + for (var resource : updatedResources) { + var managedResource = entityManager.find(FileResource.class, resource.getId()); + if (managedResource != null) { + managedResource.setName(resource.getName()); + } + } + } + private void changeExtensionNamespace(Streamable extensions, Namespace newNamespace) { for (var extension : extensions) { // findExtensions ran inside changeNamespaceInDatabase's transaction, so these are diff --git a/server/src/main/java/org/eclipse/openvsx/eclipse/EclipseService.java b/server/src/main/java/org/eclipse/openvsx/eclipse/EclipseService.java index 800dc617c..f09522e24 100644 --- a/server/src/main/java/org/eclipse/openvsx/eclipse/EclipseService.java +++ b/server/src/main/java/org/eclipse/openvsx/eclipse/EclipseService.java @@ -195,8 +195,14 @@ public EclipseProfile getPublicProfile(String personId) { */ @Transactional public void updateUserData(UserData user, EclipseProfile profile) { - user = entityManager.merge(user); - user.setEclipsePersonId(profile.getName()); + // find-then-set rather than merge: merge writes every column of the detached copy, so + // anything that changed on the row since it was loaded gets reverted. Only the field + // below is this method's to change - see #989. + var managedUser = entityManager.find(UserData.class, user.getId()); + if (managedUser == null) { + return; + } + managedUser.setEclipsePersonId(profile.getName()); } public void enrichUserJsonWithPublisherAgreement(UserJson json, UserData user) { diff --git a/server/src/main/java/org/eclipse/openvsx/migration/CheckPotentiallyMaliciousExtensionVersionsService.java b/server/src/main/java/org/eclipse/openvsx/migration/CheckPotentiallyMaliciousExtensionVersionsService.java index 667b758d0..00090cf5b 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/CheckPotentiallyMaliciousExtensionVersionsService.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/CheckPotentiallyMaliciousExtensionVersionsService.java @@ -33,9 +33,9 @@ public CheckPotentiallyMaliciousExtensionVersionsService(EntityManager entityMan @Transactional public void checkPotentiallyMaliciousExtensionVersion(ExtensionVersion extVersion, TempFile extensionFile) { + boolean isMalicious; try (var extProcessor = new ExtensionProcessor(extensionFile)) { - boolean isMalicious = extProcessor.isPotentiallyMalicious(); - extVersion.setPotentiallyMalicious(isMalicious); + isMalicious = extProcessor.isPotentiallyMalicious(); if (isMalicious) { logger.atWarn() .setMessage("Extension version is potentially malicious: {}") @@ -43,6 +43,14 @@ public void checkPotentiallyMaliciousExtensionVersion(ExtensionVersion extVersio .log(); } } - entityManager.merge(extVersion); + + // find-then-set rather than merge: merge writes every column of the detached copy, so + // anything that changed on the row since it was loaded gets reverted. Only the field + // below is this method's to change - see #989. + var managedVersion = entityManager.find(ExtensionVersion.class, extVersion.getId()); + if (managedVersion == null) { + return; + } + managedVersion.setPotentiallyMalicious(isMalicious); } } diff --git a/server/src/main/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandler.java b/server/src/main/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandler.java index 51c8d2e96..758b08755 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandler.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandler.java @@ -73,7 +73,7 @@ public void run(MigrationJobRequest jobRequest) throws Exception { try { resource.setSize(migrations.getFileSize(resource)); - migrations.updateResource(resource); + migrations.updateResourceSize(resource); } catch (FileNotFoundInStorageException e) { // The object backing this resource is confirmed gone, not just temporarily // unreachable -- there's nothing further this job can do about that diff --git a/server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java b/server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java index d3c14a150..f4e119d46 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java @@ -126,9 +126,18 @@ public FileResource getResource(MigrationJobRequest jobRequest) { return entityManager.find(FileResource.class, jobRequest.getEntityId()); } + /** + * Persists the size its caller determined for {@code resource}, and nothing else. Named for the + * one field it writes: a generic "update this resource" could only be implemented by merging the + * whole detached copy, which reverts any column that changed since it was loaded (#989). + */ @Transactional - public void updateResource(FileResource resource) { - entityManager.merge(resource); + public void updateResourceSize(FileResource resource) { + var managedResource = entityManager.find(FileResource.class, resource.getId()); + if (managedResource == null) { + return; + } + managedResource.setSize(resource.getSize()); } @Retryable @@ -161,8 +170,10 @@ public void persistFileResource(FileResource resource) { @Transactional public void deleteFileResource(FileResource resource) { - resource = entityManager.merge(resource); - entityManager.remove(resource); + var managedResource = entityManager.find(FileResource.class, resource.getId()); + if (managedResource != null) { + entityManager.remove(managedResource); + } } public FileResource getFileResource(ExtensionVersion extVersion, String type) { diff --git a/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsJobRequestHandler.java b/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsJobRequestHandler.java index 88783d387..7991982ef 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsJobRequestHandler.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsJobRequestHandler.java @@ -54,7 +54,7 @@ public void run(MigrationJobRequest jobRequest) throws Exception { migrations.removeFile(download); download.setName(name); - service.updateResource(download); + service.updateResourceName(download); } logger.info("Updated download name to: {}", name); diff --git a/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java b/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java index 93e41b23c..a9d6fd6e8 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java @@ -43,8 +43,16 @@ public FileResource cloneResource(FileResource resource, String name) { return clone; } + /** + * Persists the new name its caller set on {@code resource}, and nothing else - see + * {@code MigrationService#updateResourceSize} for why this is named for its field. + */ @Transactional - public void updateResource(FileResource resource) { - entityManager.merge(resource); + public void updateResourceName(FileResource resource) { + var managedResource = entityManager.find(FileResource.class, resource.getId()); + if (managedResource == null) { + return; + } + managedResource.setName(resource.getName()); } } diff --git a/server/src/main/java/org/eclipse/openvsx/migration/SetPreReleaseJobService.java b/server/src/main/java/org/eclipse/openvsx/migration/SetPreReleaseJobService.java index 479b128e7..549977d65 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/SetPreReleaseJobService.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/SetPreReleaseJobService.java @@ -44,11 +44,21 @@ public List getExtensionVersions(MigrationJobRequest jobReques @Transactional public void updatePreviewAndPreRelease(ExtensionVersion extVersion, TempFile extensionFile) { + boolean preRelease; + boolean preview; try (var extProcessor = new ExtensionProcessor(extensionFile)) { - extVersion.setPreRelease(extProcessor.isPreRelease()); - extVersion.setPreview(extProcessor.isPreview()); + preRelease = extProcessor.isPreRelease(); + preview = extProcessor.isPreview(); } - entityManager.merge(extVersion); + // find-then-set rather than merge: merge writes every column of the detached copy, so + // anything that changed on the row since it was loaded gets reverted. Only the field + // below is this method's to change - see #989. + var managedVersion = entityManager.find(ExtensionVersion.class, extVersion.getId()); + if (managedVersion == null) { + return; + } + managedVersion.setPreRelease(preRelease); + managedVersion.setPreview(preview); } } diff --git a/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java b/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java index 3d0c3fea8..9f0592868 100644 --- a/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java +++ b/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java @@ -97,8 +97,14 @@ public boolean verifyExtensionVersion(TempFile extensionFile, TempFile signature @Transactional public void setSignatureKeyPair(ExtensionVersion extVersion, SignatureKeyPair keyPair) { - extVersion = entityManager.merge(extVersion); - extVersion.setSignatureKeyPair(keyPair); + // find-then-set rather than merge: merge writes every column of the detached copy, so + // anything that changed on the row since it was loaded gets reverted. Only the field + // below is this method's to change - see #989. + var managedVersion = entityManager.find(ExtensionVersion.class, extVersion.getId()); + if (managedVersion == null) { + return; + } + managedVersion.setSignatureKeyPair(keyPair); } public TempFile generateSignature(TempFile extensionFile, SignatureKeyPair keyPair) throws IOException { diff --git a/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionService.java b/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionService.java index 620544826..928c30f45 100644 --- a/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionService.java +++ b/server/src/main/java/org/eclipse/openvsx/publish/PublishExtensionVersionService.java @@ -91,8 +91,14 @@ public void persistResource(FileResource resource) { @Transactional public void markExtensionAsPotentiallyMalicious(ExtensionVersion extVersion) { - extVersion = entityManager.merge(extVersion); - extVersion.setPotentiallyMalicious(true); + // find-then-set rather than merge: merge writes every column of the detached copy, so + // anything that changed on the row since it was loaded gets reverted. Only the field + // below is this method's to change - see #989. + var managedVersion = entityManager.find(ExtensionVersion.class, extVersion.getId()); + if (managedVersion == null) { + return; + } + managedVersion.setPotentiallyMalicious(true); } @Transactional diff --git a/server/src/main/java/org/eclipse/openvsx/ratelimit/CustomerService.java b/server/src/main/java/org/eclipse/openvsx/ratelimit/CustomerService.java index f939832dc..5215542b2 100644 --- a/server/src/main/java/org/eclipse/openvsx/ratelimit/CustomerService.java +++ b/server/src/main/java/org/eclipse/openvsx/ratelimit/CustomerService.java @@ -193,8 +193,17 @@ private String generateTokenValue() { @Transactional public ResultJson deactivateRateLimitToken(RateLimitToken token) { - token = entityManager.merge(token); - token.setActive(false); - return ResultJson.success("Deactivated rate limit token for customer " + token.getCustomer().getName() + "."); + // find-then-set rather than merge: merge writes every column of the detached copy, so + // anything that changed on the row since it was loaded gets reverted. Only the field + // below is this method's to change - see #989. + var managedToken = entityManager.find(RateLimitToken.class, token.getId()); + if (managedToken == null) { + // RateLimitAPI already established the token exists before calling, so this only + // happens if the row went away in between; matches how this class reports a missing row. + throw new ErrorResultException("Rate limit token not found: " + token.getId()); + } + managedToken.setActive(false); + return ResultJson + .success("Deactivated rate limit token for customer " + managedToken.getCustomer().getName() + "."); } } diff --git a/server/src/test/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandlerTest.java b/server/src/test/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandlerTest.java index 3f88b0dbd..92a2510e8 100644 --- a/server/src/test/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandlerTest.java +++ b/server/src/test/java/org/eclipse/openvsx/migration/FileResourceSizeJobRequestHandlerTest.java @@ -58,8 +58,8 @@ void run_recordsSizeForEveryUnsizedResourceOfTheExtensionVersionAndSkipsAlreadyS new FileResourceSizeJobRequestHandler(migrations).run(jobRequest); - verify(migrations).updateResource(unsized); - verify(migrations, never()).updateResource(alreadySized); + verify(migrations).updateResourceSize(unsized); + verify(migrations, never()).updateResourceSize(alreadySized); verify(migrations, never()).getFileSize(alreadySized); } @@ -78,8 +78,8 @@ void run_skipsAMissingResourceButStillProcessesTheRestOfTheExtensionVersion() th assertThatCode(() -> handler.run(jobRequest)).doesNotThrowAnyException(); - verify(migrations, never()).updateResource(missing); - verify(migrations).updateResource(present); + verify(migrations, never()).updateResourceSize(missing); + verify(migrations).updateResourceSize(present); } private ExtensionVersion extVersion() { diff --git a/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionServiceTest.java b/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionServiceTest.java index 9659740b6..268e95fc0 100644 --- a/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/publish/PublishExtensionVersionServiceTest.java @@ -179,6 +179,51 @@ void mirrorResource_recordsTheSizeOfTheExtractedBytes() throws Exception { verify(entityManager).persist(resource); } + // #989: markExtensionAsPotentiallyMalicious used to merge the whole detached extVersion to set + // one boolean, so any column that had moved since the caller loaded it was reverted - the same + // failure mode that once left published extensions inactive. It now writes the flag on the + // managed row, matching what activateExtension above already did. + @Test + void markExtensionAsPotentiallyMalicious_flagsTheManagedRow() { + var stale = version(1L, false); + var managed = version(1L, false); + when(entityManager.find(ExtensionVersion.class, 1L)).thenReturn(managed); + + svc.markExtensionAsPotentiallyMalicious(stale); + + assertThat(managed.isPotentiallyMalicious()).isTrue(); + verify(entityManager, never()).merge(any()); + } + + @Test + void markExtensionAsPotentiallyMalicious_leavesConcurrentChangesAlone() { + // The caller's copy was loaded while the version was still inactive; it has since been + // activated. Merging that stale copy back would have deactivated it again. + var stale = version(1L, false); + stale.setActive(false); + var managed = version(1L, false); + managed.setActive(true); + when(entityManager.find(ExtensionVersion.class, 1L)).thenReturn(managed); + + svc.markExtensionAsPotentiallyMalicious(stale); + + assertThat(managed.isPotentiallyMalicious()).isTrue(); + assertThat(managed.isActive()).as("a concurrent activation must survive the flag write").isTrue(); + } + + // The row can be purged between the caller loading it and this running; merge would have tried + // to resurrect it rather than doing nothing. + @Test + void markExtensionAsPotentiallyMalicious_ignoresAVersionThatIsGone() { + var stale = version(1L, false); + when(entityManager.find(ExtensionVersion.class, 1L)).thenReturn(null); + + svc.markExtensionAsPotentiallyMalicious(stale); + + verify(entityManager, never()).merge(any()); + verify(entityManager, never()).persist(any()); + } + private ExtensionVersion version(long id, boolean removed) { var namespace = new Namespace(); namespace.setName("redhat");