Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle FormException from SecureGroovyScript #726

Merged
merged 1 commit into from
Oct 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -482,7 +483,12 @@ public List<LockableResource> 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<LockableResource> candidates = requiredResources.required; // default candidates

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -297,7 +298,7 @@ private void informPerformanceIssue() {

// ---------------------------------------------------------------------------
@Restricted(NoExternalUse.class) // used by jelly
public Queue getQueue() {
public Queue getQueue() throws Descriptor.FormException {
List<QueuedContextStruct> currentQueueContext =
List.copyOf(LockableResourcesManager.get().getCurrentQueuedContext());
Queue queue = new Queue();
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
}

Expand All @@ -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<String, Object> params = new HashMap<>();

// Inject Build Parameters, if possible and applicable to the "item" type
Expand Down Expand Up @@ -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";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -126,14 +127,19 @@ public LockableResourcesStruct(@Nullable List<String> 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();
}
return resourceMatchScript;
}

@CheckForNull
public String getResourceMatchScriptText() {
return serializableResourceMatchScript != null ? serializableResourceMatchScript.getScript() : null;
}

@Override
public String toString() {
String str = "";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand All @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

public class SerializableSecureGroovyScriptTest {
@Test
public void testRehydrate() {
public void testRehydrate() throws Exception {
SerializableSecureGroovyScript nullCheck = new SerializableSecureGroovyScript(null);
assertNull("SerializableSecureGroovyScript null check", nullCheck.rehydrate());

Expand Down