From 27f78d2347bfcd856812971758f2060c7d1e91eb Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Thu, 8 Aug 2024 16:27:48 +0200 Subject: [PATCH] Fix permission errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The BaseReceivePackFactory re-used the GitReceiveHook. The problem is, that the hook is not thread safe. Due to the re-usage, the repository could have been changed during processing post receive hooks. With this, the factory will always create a new receive pack. Pushed-by: Rene Pfeuffer Co-authored-by: René Pfeuffer Committed-by: René Pfeuffer --- gradle/changelog/wrong_permissions.yaml | 2 ++ .../protocolcommand/git/BaseReceivePackFactory.java | 7 +++++-- .../sonia/scm/web/CollectingPackParserListener.java | 2 +- .../src/main/java/sonia/scm/web/GitReceiveHook.java | 7 ++++++- .../git/BaseReceivePackFactoryTest.java | 12 ++++++++++-- 5 files changed, 24 insertions(+), 6 deletions(-) create mode 100644 gradle/changelog/wrong_permissions.yaml diff --git a/gradle/changelog/wrong_permissions.yaml b/gradle/changelog/wrong_permissions.yaml new file mode 100644 index 0000000000..95ee4be6f6 --- /dev/null +++ b/gradle/changelog/wrong_permissions.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Wrong repositories in post-receive hooks under some circumstances diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/BaseReceivePackFactory.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/BaseReceivePackFactory.java index 9f32812817..99a304e3cd 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/BaseReceivePackFactory.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/protocolcommand/git/BaseReceivePackFactory.java @@ -39,17 +39,19 @@ import sonia.scm.web.GitReceiveHook; public abstract class BaseReceivePackFactory implements ReceivePackFactory { + private final GitChangesetConverterFactory converterFactory; private final GitRepositoryHandler handler; - private final GitReceiveHook hook; + private final HookEventFacade hookEventFacade; private final GitRepositoryConfigStoreProvider storeProvider; protected BaseReceivePackFactory(GitChangesetConverterFactory converterFactory, GitRepositoryHandler handler, HookEventFacade hookEventFacade, GitRepositoryConfigStoreProvider storeProvider) { + this.converterFactory = converterFactory; this.handler = handler; + this.hookEventFacade = hookEventFacade; this.storeProvider = storeProvider; - this.hook = new GitReceiveHook(converterFactory, hookEventFacade, handler); } @Override @@ -57,6 +59,7 @@ public abstract class BaseReceivePackFactory implements ReceivePackFactory ReceivePack receivePack = createBasicReceivePack(connection, repository); receivePack.setAllowNonFastForwards(isNonFastForwardAllowed(repository)); + GitReceiveHook hook = new GitReceiveHook(converterFactory, hookEventFacade, handler); receivePack.setPreReceiveHook(hook); receivePack.setPostReceiveHook(hook); // apply collecting listener, to be able to check which commits are new diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/CollectingPackParserListener.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/CollectingPackParserListener.java index a4988f0daa..fc6b8e67ee 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/CollectingPackParserListener.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/CollectingPackParserListener.java @@ -55,7 +55,7 @@ public class CollectingPackParserListener implements PackParserListener private Set newObjectIds; - public CollectingPackParserListener(GitReceiveHook hook) { + private CollectingPackParserListener(GitReceiveHook hook) { this.hook = hook; } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceiveHook.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceiveHook.java index c34ac01ec9..208717bfc2 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceiveHook.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/web/GitReceiveHook.java @@ -43,7 +43,9 @@ import java.io.IOException; import java.util.Collection; import java.util.List; - +/** + * This class is not thread safe! + */ public class GitReceiveHook implements PreReceiveHook, PostReceiveHook { private static final Logger LOG = LoggerFactory.getLogger(GitReceiveHook.class); @@ -96,6 +98,9 @@ public class GitReceiveHook implements PreReceiveHook, PostReceiveHook { GitHookContextProvider context = new GitHookContextProvider(converterFactory, rpack, receiveCommands, repository, repositoryId); if (type == RepositoryHookType.POST_RECEIVE) { + if (postReceiveContext != null) { + throw new IllegalStateException("Looks like this hook is re-used. This must not happen."); + } postReceiveContext = context; } else { hookEventFacade.handle(repositoryId).fireHookEvent(type, context); diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/protocolcommand/git/BaseReceivePackFactoryTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/protocolcommand/git/BaseReceivePackFactoryTest.java index e59c5b5896..202abd0cb0 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/protocolcommand/git/BaseReceivePackFactoryTest.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/protocolcommand/git/BaseReceivePackFactoryTest.java @@ -50,6 +50,8 @@ import java.io.File; import java.io.IOException; import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.sameInstance; import static org.junit.Assert.*; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -88,8 +90,7 @@ public class BaseReceivePackFactoryTest { when(handler.getConfig()).thenReturn(gitConfig); when(handler.getRepositoryId(repository.getConfig())).thenReturn("heart-of-gold"); - ReceivePack receivePack = new ReceivePack(repository); - when(wrappedReceivePackFactory.create(request, repository)).thenReturn(receivePack); + when(wrappedReceivePackFactory.create(request, repository)).thenAnswer(invocation -> new ReceivePack(repository)); when(gitRepositoryConfigStoreProvider.getGitRepositoryConfig("heart-of-gold")).thenReturn(gitRepositoryConfig); @@ -130,4 +131,11 @@ public class BaseReceivePackFactoryTest { assertFalse(receivePack.isAllowNonFastForwards()); } + @Test + public void shouldNotReUseGitReceiveHook() throws Exception { + ReceivePack receivePack1 = factory.create(request, repository); + ReceivePack receivePack2 = factory.create(request, repository); + + assertThat(receivePack1.getPostReceiveHook(), not(sameInstance(receivePack2.getPostReceiveHook()))); + } }