diff --git a/src/main/java/jenkins/plugins/git/DisableHooks.java b/src/main/java/jenkins/plugins/git/DisableHooks.java index e7c0fc4735..3ae75796b3 100644 --- a/src/main/java/jenkins/plugins/git/DisableHooks.java +++ b/src/main/java/jenkins/plugins/git/DisableHooks.java @@ -23,8 +23,11 @@ class DisableHooks implements RepositoryCallback { public Object invoke(Repository repo, VirtualChannel channel) throws IOException, InterruptedException { final String VAL = Functions.isWindows() ? DISABLED_WIN : DISABLED_NIX; final StoredConfig repoConfig = repo.getConfig(); - repoConfig.setString("core", null, "hooksPath", VAL); - repoConfig.save(); + // Skip the rewrite, and its config.lock, when hooksPath is already disabled. JENKINS-71349 + if (!VAL.equals(repoConfig.getString("core", null, "hooksPath"))) { + repoConfig.setString("core", null, "hooksPath", VAL); + repoConfig.save(); + } return null; } } diff --git a/src/main/java/jenkins/plugins/git/UnsetHooks.java b/src/main/java/jenkins/plugins/git/UnsetHooks.java index 54cebbb89f..c5811e0222 100644 --- a/src/main/java/jenkins/plugins/git/UnsetHooks.java +++ b/src/main/java/jenkins/plugins/git/UnsetHooks.java @@ -21,7 +21,10 @@ class UnsetHooks implements RepositoryCallback { public Object invoke(Repository repo, VirtualChannel channel) throws IOException, InterruptedException { final StoredConfig repoConfig = repo.getConfig(); final String val = repoConfig.getString("core", null, "hooksPath"); - if (val != null && !val.isEmpty() && !DisableHooks.DISABLED_NIX.equals(val) && !DisableHooks.DISABLED_WIN.equals(val)) { + if (val == null) { + return null; // nothing to unset; avoid a redundant config.lock write. JENKINS-71349 + } + if (!val.isEmpty() && !DisableHooks.DISABLED_NIX.equals(val) && !DisableHooks.DISABLED_WIN.equals(val)) { LOGGER.warning(() -> "core.hooksPath explicitly set to %s and will be left intact on %s.".formatted(val, repo.getDirectory())); } else { repoConfig.unset("core", null, "hooksPath"); diff --git a/src/test/java/jenkins/plugins/git/GitHooksConfigurationTest.java b/src/test/java/jenkins/plugins/git/GitHooksConfigurationTest.java index 9d12b22c01..a2f996fe7e 100644 --- a/src/test/java/jenkins/plugins/git/GitHooksConfigurationTest.java +++ b/src/test/java/jenkins/plugins/git/GitHooksConfigurationTest.java @@ -26,6 +26,8 @@ import hudson.EnvVars; import hudson.model.TaskListener; +import java.io.File; +import java.nio.file.Files; import java.util.Random; import org.eclipse.jgit.lib.StoredConfig; import org.jenkinsci.plugins.gitclient.Git; @@ -203,4 +205,74 @@ void testConfigure_3argsFalse() throws Exception { String hooksPath = getCoreHooksPath(); assertThat(hooksPath, is(NULL_HOOKS_PATH)); } + + private File createConfigLock() throws Exception { + return client.withRepository((repo, channel) -> { + File lock = new File(repo.getDirectory(), "config.lock"); + Files.deleteIfExists(lock.toPath()); + assertTrue(lock.createNewFile()); + return lock; + }); + } + + // JENKINS-71349: re-disabling an already-disabled repo must not save(). A planted stale + // config.lock would otherwise make save() throw LockFailedException. + @Test + void testConfigureDisabledIsNoOpWhenAlreadyDisabled() throws Exception { + GitHooksConfiguration.configure(client, false); + assertThat(getCoreHooksPath(), is(NULL_HOOKS_PATH)); + + File configLock = createConfigLock(); + try { + GitHooksConfiguration.configure(client, false); + assertThat(getCoreHooksPath(), is(NULL_HOOKS_PATH)); + assertTrue(configLock.exists(), "DisableHooks must not touch an unrelated stale lock"); + } finally { + Files.deleteIfExists(configLock.toPath()); + } + } + + // The guard must skip only the disabled sentinel, never a different value (SECURITY-2754). + @Test + void testConfigureDisabledOverwritesNonDisabledValue() throws Exception { + setCoreHooksPath(ALTERNATE_HOOKS_PATH); + assertThat(getCoreHooksPath(), is(ALTERNATE_HOOKS_PATH)); + + GitHooksConfiguration.configure(client, false); + + assertThat(getCoreHooksPath(), is(NULL_HOOKS_PATH)); + } + + @Test + void testConfigureDisabledRepeatedlyIsNoOp() throws Exception { + GitHooksConfiguration.configure(client, false); + assertThat(getCoreHooksPath(), is(NULL_HOOKS_PATH)); + + File configLock = createConfigLock(); + try { + for (int i = 0; i < 5; i++) { + GitHooksConfiguration.configure(client, false); + } + assertThat(getCoreHooksPath(), is(NULL_HOOKS_PATH)); + assertTrue(configLock.exists(), "repeated disable must not touch the stale lock"); + } finally { + Files.deleteIfExists(configLock.toPath()); + } + } + + // JENKINS-71349, allow-hooks path: UnsetHooks must not save() when hooksPath is already absent. + @Test + void testConfigureAllowedIsNoOpWhenHooksPathAbsent() throws Exception { + GitHooksConfiguration.configure(client, true); + assertThat(getCoreHooksPath(), is(nullValue())); + + File configLock = createConfigLock(); + try { + GitHooksConfiguration.configure(client, true); + assertThat(getCoreHooksPath(), is(nullValue())); + assertTrue(configLock.exists(), "UnsetHooks must not touch the stale lock when hooksPath is absent"); + } finally { + Files.deleteIfExists(configLock.toPath()); + } + } }