From ed786ba197e0a5b484d989a907acfee3dc135851 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 30 Oct 2024 20:25:55 -0400 Subject: [PATCH] Handle `FormException` from `SecureGroovyScript` (#726) --- .../lockableresources/LockableResourcesManager.java | 8 +++++++- .../lockableresources/RequiredResourcesProperty.java | 9 ++++++--- .../actions/LockableResourcesRootAction.java | 9 ++++++--- .../plugins/lockableresources/queue/LockRunListener.java | 2 +- .../queue/LockableResourcesQueueTaskDispatcher.java | 9 ++++----- .../lockableresources/queue/LockableResourcesStruct.java | 8 +++++++- .../util/SerializableSecureGroovyScript.java | 8 +++++++- .../plugins/lockableresources/FreeStyleProjectTest.java | 5 ++--- .../util/SerializableSecureGroovyScriptTest.java | 2 +- 9 files changed, 41 insertions(+), 19 deletions(-) diff --git a/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java b/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java index 94dddd59e..fce804f98 100644 --- a/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java +++ b/src/main/java/org/jenkins/plugins/lockableresources/LockableResourcesManager.java @@ -18,6 +18,7 @@ import hudson.Extension; import hudson.Util; import hudson.console.ModelHyperlinkNote; +import hudson.model.Descriptor; import hudson.model.Run; import java.io.IOException; import java.io.PrintStream; @@ -482,7 +483,12 @@ public List tryQueue( return null; } - final SecureGroovyScript systemGroovyScript = requiredResources.getResourceMatchScript(); + final SecureGroovyScript systemGroovyScript; + try { + systemGroovyScript = requiredResources.getResourceMatchScript(); + } catch (Descriptor.FormException x) { + throw new ExecutionException(x); + } boolean candidatesByScript = (systemGroovyScript != null); List candidates = requiredResources.required; // default candidates diff --git a/src/main/java/org/jenkins/plugins/lockableresources/RequiredResourcesProperty.java b/src/main/java/org/jenkins/plugins/lockableresources/RequiredResourcesProperty.java index f058f6260..983bcf8e3 100644 --- a/src/main/java/org/jenkins/plugins/lockableresources/RequiredResourcesProperty.java +++ b/src/main/java/org/jenkins/plugins/lockableresources/RequiredResourcesProperty.java @@ -14,6 +14,7 @@ import hudson.Util; import hudson.model.AbstractProject; import hudson.model.AutoCompletionCandidates; +import hudson.model.Descriptor; import hudson.model.Item; import hudson.model.Job; import hudson.model.JobProperty; @@ -45,7 +46,8 @@ public RequiredResourcesProperty( String resourceNamesVar, String resourceNumber, String labelName, - @CheckForNull SecureGroovyScript resourceMatchScript) { + @CheckForNull SecureGroovyScript resourceMatchScript) + throws Descriptor.FormException { super(); if (resourceNames == null || resourceNames.trim().isEmpty()) { @@ -84,11 +86,12 @@ public RequiredResourcesProperty( @Deprecated @ExcludeFromJacocoGeneratedReport public RequiredResourcesProperty( - String resourceNames, String resourceNamesVar, String resourceNumber, String labelName) { + String resourceNames, String resourceNamesVar, String resourceNumber, String labelName) + throws Descriptor.FormException { this(resourceNames, resourceNamesVar, resourceNumber, labelName, null); } - private Object readResolve() { + private Object readResolve() throws Descriptor.FormException { // SECURITY-368 migration logic if (resourceMatchScript == null && labelName != null diff --git a/src/main/java/org/jenkins/plugins/lockableresources/actions/LockableResourcesRootAction.java b/src/main/java/org/jenkins/plugins/lockableresources/actions/LockableResourcesRootAction.java index 269928c94..d3af6e85a 100644 --- a/src/main/java/org/jenkins/plugins/lockableresources/actions/LockableResourcesRootAction.java +++ b/src/main/java/org/jenkins/plugins/lockableresources/actions/LockableResourcesRootAction.java @@ -12,6 +12,7 @@ import edu.umd.cs.findbugs.annotations.NonNull; import hudson.Extension; import hudson.model.Api; +import hudson.model.Descriptor; import hudson.model.RootAction; import hudson.model.Run; import hudson.security.AccessDeniedException3; @@ -297,7 +298,7 @@ private void informPerformanceIssue() { // --------------------------------------------------------------------------- @Restricted(NoExternalUse.class) // used by jelly - public Queue getQueue() { + public Queue getQueue() throws Descriptor.FormException { List currentQueueContext = List.copyOf(LockableResourcesManager.get().getCurrentQueuedContext()); Queue queue = new Queue(); @@ -325,7 +326,8 @@ public Queue() { // ------------------------------------------------------------------------- @Restricted(NoExternalUse.class) // used by jelly - public void add(final LockableResourcesStruct resourceStruct, final QueuedContextStruct context) { + public void add(final LockableResourcesStruct resourceStruct, final QueuedContextStruct context) + throws Descriptor.FormException { QueueStruct queueStruct = new QueueStruct(resourceStruct, context); queue.add(queueStruct); if (resourceStruct.queuedAt == 0) { @@ -362,7 +364,8 @@ public static class QueueStruct { String id = null; Run build; - public QueueStruct(final LockableResourcesStruct resourceStruct, final QueuedContextStruct context) { + public QueueStruct(final LockableResourcesStruct resourceStruct, final QueuedContextStruct context) + throws Descriptor.FormException { this.requiredResources = resourceStruct.required; this.requiredLabel = resourceStruct.label; this.requiredNumber = resourceStruct.requiredNumber; diff --git a/src/main/java/org/jenkins/plugins/lockableresources/queue/LockRunListener.java b/src/main/java/org/jenkins/plugins/lockableresources/queue/LockRunListener.java index caceece3e..82d43318a 100644 --- a/src/main/java/org/jenkins/plugins/lockableresources/queue/LockRunListener.java +++ b/src/main/java/org/jenkins/plugins/lockableresources/queue/LockRunListener.java @@ -49,7 +49,7 @@ public void onStarted(Run build, TaskListener listener) { if (resources != null) { if (resources.requiredNumber != null || !resources.label.isEmpty() - || resources.getResourceMatchScript() != null) { + || resources.getResourceMatchScriptText() != null) { required.addAll(lrm.getResourcesFromProject(proj.getFullName())); } else { required.addAll(resources.required); diff --git a/src/main/java/org/jenkins/plugins/lockableresources/queue/LockableResourcesQueueTaskDispatcher.java b/src/main/java/org/jenkins/plugins/lockableresources/queue/LockableResourcesQueueTaskDispatcher.java index 1645ea20a..97a504270 100644 --- a/src/main/java/org/jenkins/plugins/lockableresources/queue/LockableResourcesQueueTaskDispatcher.java +++ b/src/main/java/org/jenkins/plugins/lockableresources/queue/LockableResourcesQueueTaskDispatcher.java @@ -30,7 +30,6 @@ import java.util.logging.Logger; import org.jenkins.plugins.lockableresources.LockableResource; import org.jenkins.plugins.lockableresources.LockableResourcesManager; -import org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SecureGroovyScript; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; @@ -55,7 +54,7 @@ public CauseOfBlockage canRun(Queue.Item item) { if (resources == null || (resources.required.isEmpty() && resources.label.isEmpty() - && resources.getResourceMatchScript() == null)) { + && resources.getResourceMatchScriptText() == null)) { return null; } @@ -68,7 +67,7 @@ public CauseOfBlockage canRun(Queue.Item item) { LOGGER.finest(project.getName() + " trying to get resources with these details: " + resources); - if (resourceNumber > 0 || !resources.label.isEmpty() || resources.getResourceMatchScript() != null) { + if (resourceNumber > 0 || !resources.label.isEmpty() || resources.getResourceMatchScriptText() != null) { Map params = new HashMap<>(); // Inject Build Parameters, if possible and applicable to the "item" type @@ -153,11 +152,11 @@ public String getShortDescription() { if (!this.rscStruct.required.isEmpty()) { return "Waiting for resource instances " + rscStruct.required; } else { - final SecureGroovyScript systemGroovyScript = this.rscStruct.getResourceMatchScript(); + final String systemGroovyScript = this.rscStruct.getResourceMatchScriptText(); if (systemGroovyScript != null) { // Empty or not... just keep the logic in sync // with tryQueue() in LockableResourcesManager - if (systemGroovyScript.getScript().isEmpty()) { + if (systemGroovyScript.isEmpty()) { return "Waiting for resources identified by custom script (which is empty)"; } else { return "Waiting for resources identified by custom script"; diff --git a/src/main/java/org/jenkins/plugins/lockableresources/queue/LockableResourcesStruct.java b/src/main/java/org/jenkins/plugins/lockableresources/queue/LockableResourcesStruct.java index 8b824930f..b200ad150 100644 --- a/src/main/java/org/jenkins/plugins/lockableresources/queue/LockableResourcesStruct.java +++ b/src/main/java/org/jenkins/plugins/lockableresources/queue/LockableResourcesStruct.java @@ -12,6 +12,7 @@ import edu.umd.cs.findbugs.annotations.Nullable; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import hudson.EnvVars; +import hudson.model.Descriptor; import java.io.Serializable; import java.util.ArrayList; import java.util.Date; @@ -126,7 +127,7 @@ public LockableResourcesStruct(@Nullable List resources, @Nullable Strin * @since 2.1 */ @CheckForNull - public SecureGroovyScript getResourceMatchScript() { + public SecureGroovyScript getResourceMatchScript() throws Descriptor.FormException { if (resourceMatchScript == null && serializableResourceMatchScript != null) { // this is probably high defensive code, because resourceMatchScript = serializableResourceMatchScript.rehydrate(); @@ -134,6 +135,11 @@ public SecureGroovyScript getResourceMatchScript() { return resourceMatchScript; } + @CheckForNull + public String getResourceMatchScriptText() { + return serializableResourceMatchScript != null ? serializableResourceMatchScript.getScript() : null; + } + @Override public String toString() { String str = ""; diff --git a/src/main/java/org/jenkins/plugins/lockableresources/util/SerializableSecureGroovyScript.java b/src/main/java/org/jenkins/plugins/lockableresources/util/SerializableSecureGroovyScript.java index 299b7cab1..2603a850e 100644 --- a/src/main/java/org/jenkins/plugins/lockableresources/util/SerializableSecureGroovyScript.java +++ b/src/main/java/org/jenkins/plugins/lockableresources/util/SerializableSecureGroovyScript.java @@ -26,6 +26,7 @@ import edu.umd.cs.findbugs.annotations.CheckForNull; import edu.umd.cs.findbugs.annotations.NonNull; import edu.umd.cs.findbugs.annotations.Nullable; +import hudson.model.Descriptor; import hudson.util.FormValidation; import java.io.Serializable; import java.net.MalformedURLException; @@ -77,7 +78,12 @@ public SerializableSecureGroovyScript(@CheckForNull SecureGroovyScript secureScr } @CheckForNull - public SecureGroovyScript rehydrate() { + public String getScript() { + return script; + } + + @CheckForNull + public SecureGroovyScript rehydrate() throws Descriptor.FormException { if (script == null) { return null; } diff --git a/src/test/java/org/jenkins/plugins/lockableresources/FreeStyleProjectTest.java b/src/test/java/org/jenkins/plugins/lockableresources/FreeStyleProjectTest.java index a23e9d62a..75cf28c0d 100644 --- a/src/test/java/org/jenkins/plugins/lockableresources/FreeStyleProjectTest.java +++ b/src/test/java/org/jenkins/plugins/lockableresources/FreeStyleProjectTest.java @@ -24,7 +24,6 @@ import java.io.IOException; import java.util.ArrayList; import java.util.List; -import java.util.concurrent.ExecutionException; import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; import jenkins.model.Jenkins; @@ -216,7 +215,7 @@ public void approvalRequired() throws Exception { } @Test - public void autoCreateResource() throws IOException, InterruptedException, ExecutionException { + public void autoCreateResource() throws Exception { FreeStyleProject f = j.createFreeStyleProject("f"); assertNull(LockableResourcesManager.get().fromName("resource1")); f.addProperty(new RequiredResourcesProperty("resource1", null, null, null, null)); @@ -230,7 +229,7 @@ public void autoCreateResource() throws IOException, InterruptedException, Execu } @Test - public void competingLabelLocks() throws IOException, InterruptedException, ExecutionException { + public void competingLabelLocks() throws Exception { LockableResourcesManager.get().createResourceWithLabel("resource1", "group1"); LockableResourcesManager.get().createResourceWithLabel("resource2", "group2"); LockableResourcesManager.get().createResource("shared"); diff --git a/src/test/java/org/jenkins/plugins/lockableresources/util/SerializableSecureGroovyScriptTest.java b/src/test/java/org/jenkins/plugins/lockableresources/util/SerializableSecureGroovyScriptTest.java index 91f6b592b..868be5d88 100644 --- a/src/test/java/org/jenkins/plugins/lockableresources/util/SerializableSecureGroovyScriptTest.java +++ b/src/test/java/org/jenkins/plugins/lockableresources/util/SerializableSecureGroovyScriptTest.java @@ -14,7 +14,7 @@ public class SerializableSecureGroovyScriptTest { public JenkinsRule r = new JenkinsRule(); @Test - public void testRehydrate() { + public void testRehydrate() throws Exception { SerializableSecureGroovyScript nullCheck = new SerializableSecureGroovyScript(null); assertNull("SerializableSecureGroovyScript null check", nullCheck.rehydrate());