[JENKINS-63254][JENKINS-47101] Insecure Groovy String Interpolation Warnings

pom.xml

@@ -74,7 +74,7 @@
             <dependency>
                 <groupId>io.jenkins.tools.bom</groupId>
                 <artifactId>bom-2.176.x</artifactId>
-                <version>9</version>
+                <version>11</version>
                 <scope>import</scope>
                 <type>pom</type>
             </dependency>
@@ -84,6 +84,7 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-step-api</artifactId>
+            <version>2.23-rc566.c1fa948b49be</version>
         </dependency>
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
@@ -124,6 +125,7 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-step-api</artifactId>
+            <version>2.23-rc566.c1fa948b49be</version>
             <classifier>tests</classifier>
             <scope>test</scope>
         </dependency>
@@ -157,6 +159,7 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins</groupId>
             <artifactId>credentials-binding</artifactId>
+            <version>1.24-rc370.8c9bfdc92872</version>
             <scope>test</scope>
         </dependency>
         <dependency>

src/main/java/org/jenkinsci/plugins/workflow/cps/ContextVariableSet.java

@@ -81,6 +81,7 @@ private static final class DynamicContextQuery {
         }
     }
 
+    @FunctionalInterface
     interface ThrowingSupplier<T> {
         T get() throws IOException;
     }

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java

@@ -155,7 +155,7 @@ public class CpsStepContext extends DefaultStepContext { // TODO add XStream cla
     private transient List<CpsBodyInvoker> bodyInvokers = new ArrayList<>();
 
     /**
-     * While {@link CpsStepContext} has not received teh response, maintains the body closure.
+     * While {@link CpsStepContext} has not received the response, maintains the body closure.
      *
      * This is the implicit closure block passed to the step invocation.
      */
@@ -181,15 +181,22 @@ public class CpsStepContext extends DefaultStepContext { // TODO add XStream cla
     private transient volatile boolean loadingThreadGroup;
 
     @CpsVmThreadOnly
-    CpsStepContext(StepDescriptor step, CpsThread thread, FlowExecutionOwner executionRef, FlowNode node, @CheckForNull Closure body) {
+    CpsStepContext(StepDescriptor step, CpsThread thread, FlowExecutionOwner executionRef, FlowNode node) {
         this.threadId = thread.id;
         this.executionRef = executionRef;
         this.id = node.getId();
         this.node = node;
-        this.body = body != null ? thread.group.export(body) : null;
         this.stepDescriptorId = step.getId();
     }
 
+    public void setBody(@Nonnull Closure bodyToSet, @Nonnull CpsThread thread) {
+        if (this.body == null) {
+            this.body = thread.group.export(bodyToSet);
+        } else {
+            throw new IllegalStateException("Context already has a body");
+        }
+    }
+
     /**
      * Obtains {@link StepDescriptor} that represents the step this context is invoking.
      *

src/main/java/org/jenkinsci/plugins/workflow/cps/DSL.java

@@ -88,6 +88,8 @@
 import org.kohsuke.stapler.ClassDescriptor;
 import org.kohsuke.stapler.NoStaplerConstructorException;
 
+import javax.annotation.Nullable;
+
 /**
  * Calls {@link Step}s and other DSL objects.
  */
@@ -217,23 +219,23 @@ protected Object invokeStep(StepDescriptor d, Object args) {
      * @param args The arguments passed to the step.
      */
     protected Object invokeStep(StepDescriptor d, String name, Object args) {
-        final NamedArgsAndClosure ps = parseArgs(args, d);
-
-        CpsThread thread = CpsThread.current();
-
-        FlowNode an;
 
         // TODO: generalize the notion of Step taking over the FlowNode creation.
         boolean hack = d instanceof ParallelStep.DescriptorImpl || d instanceof LoadStep.DescriptorImpl;
 
-        if (ps.body == null && !hack) {
+        FlowNode an;
+        CpsThread thread = CpsThread.current();
+        if (!d.takesImplicitBlockArgument() && !hack) {
             an = new StepAtomNode(exec, d, thread.head.get());
         } else {
             an = new StepStartNode(exec, d, thread.head.get());
         }
 
+        CpsStepContext context = new CpsStepContext(d, thread, handle, an);
+        EnvironmentWatcher envWatcher = EnvironmentWatcher.of(context, exec);
+        NamedArgsAndClosure ps = parseArgs(args, d, envWatcher);
+        context.setBody(ps.body, thread);
         // Ensure ArgumentsAction is attached before we notify even synchronous listeners:
-        final CpsStepContext context = new CpsStepContext(d, thread, handle, an, ps.body);
         try {
             // No point storing empty arguments, and ParallelStep is a special case where we can't store its closure arguments
             if (ps.namedArgs != null && !(ps.namedArgs.isEmpty()) && isKeepStepArguments() && !(d instanceof ParallelStep.DescriptorImpl)) {
@@ -257,16 +259,20 @@ protected Object invokeStep(StepDescriptor d, String name, Object args) {
         boolean sync;
         ClassLoader originalLoader = Thread.currentThread().getContextClassLoader();
         try {
+            TaskListener listener = context.get(TaskListener.class);
             if (unreportedAmbiguousFunctions.remove(name)) {
-                reportAmbiguousStepInvocation(context, d);
+                reportAmbiguousStepInvocation(context, d, listener);
             }
             d.checkContextAvailability(context);
             Thread.currentThread().setContextClassLoader(CpsVmExecutorService.ORIGINAL_CONTEXT_CLASS_LOADER.get());
             if (Util.isOverridden(StepDescriptor.class, d.getClass(), "newInstance", Map.class)) {
                 s = d.newInstance(ps.namedArgs);
             } else {
                 DescribableModel<? extends Step> stepModel = DescribableModel.of(d.clazz);
-                s = stepModel.instantiate(ps.namedArgs, context.get(TaskListener.class));
+                s = stepModel.instantiate(ps.namedArgs, listener);
+            }
+            if (envWatcher != null) {
+                envWatcher.logResults(listener);
             }
 
             // Persist the node - block start and end nodes do their own persistence.
@@ -354,7 +360,7 @@ protected Object invokeDescribable(String symbol, Object _args) {
 
         // The only time a closure is valid is when the resulting Describable is immediately executed via a meta-step
         NamedArgsAndClosure args = parseArgs(_args, metaStep!=null && metaStep.takesImplicitBlockArgument(),
-                UninstantiatedDescribable.ANONYMOUS_KEY, singleArgumentOnly);
+                UninstantiatedDescribable.ANONYMOUS_KEY, singleArgumentOnly, null);
         UninstantiatedDescribable ud = new UninstantiatedDescribable(symbol, null, args.namedArgs);
 
         if (metaStep==null) {
@@ -414,32 +420,27 @@ protected Object invokeDescribable(String symbol, Object _args) {
                 ud = new UninstantiatedDescribable(symbol, null, dargs);
                 margs.put(p.getName(),ud);
 
-                return invokeStep(metaStep, symbol, new NamedArgsAndClosure(margs, args.body));
+                return invokeStep(metaStep, symbol, new NamedArgsAndClosure(margs, args.body, null));
             } catch (Exception e) {
                 throw new IllegalArgumentException("Failed to prepare "+symbol+" step",e);
             }
         }
     }
 
-    private void reportAmbiguousStepInvocation(CpsStepContext context, StepDescriptor d) {
+    private void reportAmbiguousStepInvocation(CpsStepContext context, StepDescriptor d, @Nullable TaskListener listener) {
         Exception e = null;
-        try {
-            TaskListener listener = context.get(TaskListener.class);
-            if (listener != null) {
-                List<String> ambiguousClassNames = StepDescriptor.all().stream()
-                        .filter(sd -> sd.getFunctionName().equals(d.getFunctionName()))
-                        .map(sd -> sd.clazz.getName())
-                        .collect(Collectors.toList());
-                String message = String.format("Warning: Invoking ambiguous Pipeline Step ‘%1$s’ (%2$s). " +
-                        "‘%1$s’ could refer to any of the following steps: %3$s. " +
-                        "You can invoke steps by class name instead to avoid ambiguity. " +
-                        "For example: steps.'%2$s'(...)",
-                        d.getFunctionName(), d.clazz.getName(), ambiguousClassNames);
-                listener.getLogger().println(message);
-                return;
-            }
-        } catch (InterruptedException | IOException temp) {
-            e = temp;
+        if (listener != null) {
+            List<String> ambiguousClassNames = StepDescriptor.all().stream()
+                    .filter(sd -> sd.getFunctionName().equals(d.getFunctionName()))
+                    .map(sd -> sd.clazz.getName())
+                    .collect(Collectors.toList());
+            String message = String.format("Warning: Invoking ambiguous Pipeline Step ‘%1$s’ (%2$s). " +
+                            "‘%1$s’ could refer to any of the following steps: %3$s. " +
+                            "You can invoke steps by class name instead to avoid ambiguity. " +
+                            "For example: steps.'%2$s'(...)",
+                    d.getFunctionName(), d.clazz.getName(), ambiguousClassNames);
+            listener.getLogger().println(message);
+            return;
         }
         LOGGER.log(Level.FINE, "Unable to report ambiguous step invocation for: " + d.getFunctionName(), e);
     }
@@ -458,14 +459,16 @@ private static int preallocatedHashmapCapacity(int elementsToHold) {
     static class NamedArgsAndClosure {
         final Map<String,Object> namedArgs;
         final Closure body;
+        final List<String> msgs;
 
-        private NamedArgsAndClosure(Map<?,?> namedArgs, Closure body) {
+        private NamedArgsAndClosure(Map<?,?> namedArgs, Closure body, @Nullable EnvironmentWatcher envWatcher) {
             this.namedArgs = new LinkedHashMap<>(preallocatedHashmapCapacity(namedArgs.size()));
             this.body = body;
+            this.msgs = new ArrayList<>();
 
             for (Map.Entry<?,?> entry : namedArgs.entrySet()) {
                 String k = entry.getKey().toString().intern(); // coerces GString and more
-                Object v = flattenGString(entry.getValue());
+                Object v = flattenGString(entry.getValue(), envWatcher);
                 this.namedArgs.put(k, v);
             }
         }
@@ -481,14 +484,18 @@ private NamedArgsAndClosure(Map<?,?> namedArgs, Closure body) {
      * but better to do it here in the Groovy-specific code so we do not need to rely on that.
      * @return {@code v} or an equivalent with all {@link GString}s flattened, including in nested {@link List}s or {@link Map}s
      */
-    private static Object flattenGString(Object v) {
+    private static Object flattenGString(Object v, @Nullable EnvironmentWatcher envWatcher) {
         if (v instanceof GString) {
-            return v.toString();
+            String flattened = v.toString();
+            if (envWatcher != null) {
+                envWatcher.scan(flattened);
+            }
+            return flattened;
         } else if (v instanceof List) {
             boolean mutated = false;
             List<Object> r = new ArrayList<>();
             for (Object o : ((List<?>) v)) {
-                Object o2 = flattenGString(o);
+                Object o2 = flattenGString(o, envWatcher);
                 mutated |= o != o2;
                 r.add(o2);
             }
@@ -498,9 +505,9 @@ private static Object flattenGString(Object v) {
             Map<Object,Object> r = new LinkedHashMap<>(preallocatedHashmapCapacity(((Map) v).size()));
             for (Map.Entry<?,?> e : ((Map<?, ?>) v).entrySet()) {
                 Object k = e.getKey();
-                Object k2 = flattenGString(k);
+                Object k2 = flattenGString(k, envWatcher);
                 Object o = e.getValue();
-                Object o2 = flattenGString(o);
+                Object o2 = flattenGString(o, envWatcher);
                 mutated |= k != k2 || o != o2;
                 r.put(k2, o2);
             }
@@ -510,20 +517,20 @@ private static Object flattenGString(Object v) {
         }
     }
 
-    static NamedArgsAndClosure parseArgs(Object arg, StepDescriptor d) {
+    static NamedArgsAndClosure parseArgs(Object arg, StepDescriptor d, EnvironmentWatcher envWatcher) {
         boolean singleArgumentOnly = false;
         try {
             DescribableModel<?> stepModel = DescribableModel.of(d.clazz);
             singleArgumentOnly = stepModel.hasSingleRequiredParameter() && stepModel.getParameters().size() == 1;
             if (singleArgumentOnly) {  // Can fetch the one argument we need
                 DescribableParameter dp = stepModel.getSoleRequiredParameter();
                 String paramName = (dp != null) ? dp.getName() : null;
-                return parseArgs(arg, d.takesImplicitBlockArgument(), paramName, singleArgumentOnly);
+                return parseArgs(arg, d.takesImplicitBlockArgument(), paramName, singleArgumentOnly, envWatcher);
             }
         } catch (NoStaplerConstructorException e) {
             // Ignore steps without databound constructors and treat them as normal.
         }
-        return parseArgs(arg,d.takesImplicitBlockArgument(), loadSoleArgumentKey(d), singleArgumentOnly);
+        return parseArgs(arg,d.takesImplicitBlockArgument(), loadSoleArgumentKey(d), singleArgumentOnly, envWatcher);
     }
 
     /**
@@ -548,19 +555,21 @@ static NamedArgsAndClosure parseArgs(Object arg, StepDescriptor d) {
      * @param soleArgumentKey
      *      If the context in which this method call happens allow implicit sole default argument, specify its name.
      *      If null, the call must be with names arguments.
+//     * @param envVars
+     *      The environment variables of the context
      */
-    static NamedArgsAndClosure parseArgs(Object arg, boolean expectsBlock, String soleArgumentKey, boolean singleRequiredArg) {
+    static NamedArgsAndClosure parseArgs(Object arg, boolean expectsBlock, String soleArgumentKey, boolean singleRequiredArg, @Nullable EnvironmentWatcher envWatcher) {
         if (arg instanceof NamedArgsAndClosure)
             return (NamedArgsAndClosure) arg;
         if (arg instanceof Map) // TODO is this clause actually used?
-            return new NamedArgsAndClosure((Map) arg, null);
+            return new NamedArgsAndClosure((Map) arg, null, envWatcher);
         if (arg instanceof Closure && expectsBlock)
-            return new NamedArgsAndClosure(Collections.<String,Object>emptyMap(),(Closure)arg);
+            return new NamedArgsAndClosure(Collections.<String,Object>emptyMap(),(Closure)arg, envWatcher);
 
         if (arg instanceof Object[]) {// this is how Groovy appears to pack argument list into one Object for invokeMethod
             List a = Arrays.asList((Object[])arg);
             if (a.size()==0)
-                return new NamedArgsAndClosure(Collections.<String,Object>emptyMap(),null);
+                return new NamedArgsAndClosure(Collections.<String,Object>emptyMap(),null, envWatcher);
 
             Closure c=null;
 
@@ -575,21 +584,21 @@ static NamedArgsAndClosure parseArgs(Object arg, boolean expectsBlock, String so
                 if (!singleRequiredArg ||
                         (soleArgumentKey != null && mapArg.size() == 1 && mapArg.containsKey(soleArgumentKey))) {
                     // this is how Groovy passes in Map
-                    return new NamedArgsAndClosure(mapArg, c);
+                    return new NamedArgsAndClosure(mapArg, c, envWatcher);
                 }
             }
 
             switch (a.size()) {
             case 0:
-                return new NamedArgsAndClosure(Collections.<String,Object>emptyMap(),c);
+                return new NamedArgsAndClosure(Collections.<String,Object>emptyMap(),c, envWatcher);
             case 1:
-                return new NamedArgsAndClosure(singleParam(soleArgumentKey, a.get(0)), c);
+                return new NamedArgsAndClosure(singleParam(soleArgumentKey, a.get(0)), c, envWatcher);
             default:
                 throw new IllegalArgumentException("Expected named arguments but got "+a);
             }
         }
 
-        return new NamedArgsAndClosure(singleParam(soleArgumentKey, arg), null);
+        return new NamedArgsAndClosure(singleParam(soleArgumentKey, arg), null, envWatcher);
     }
     private static Map<String,Object> singleParam(String soleArgumentKey, Object arg) {
         if (soleArgumentKey != null) {

src/main/java/org/jenkinsci/plugins/workflow/cps/EnvironmentWatcher.java

@@ -0,0 +1,65 @@
+package org.jenkinsci.plugins.workflow.cps;
+
+import hudson.EnvVars;
+import hudson.model.Run;
+import hudson.model.TaskListener;
+import org.jenkinsci.plugins.workflow.cps.view.EnvironmentWatcherRunReport;
+import org.jenkinsci.plugins.workflow.flow.FlowExecutionOwner;
+import org.jenkinsci.plugins.workflow.steps.EnvironmentExpander;
+
+import javax.annotation.Nonnull;
+import java.io.IOException;
+import java.io.Serializable;
+import java.util.List;
+import java.util.Set;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import java.util.stream.Collectors;
+
+public class EnvironmentWatcher implements Serializable {
+    private EnvVars envVars;
+    private Set<String> watchedVars;
+    private List<String> scanResults;
+    private static EnvironmentWatcherRunReport runReport;
+
+    private static final Logger LOGGER = Logger.getLogger(EnvironmentWatcher.class.getName());
+
+    public static EnvironmentWatcher of(CpsStepContext context, CpsFlowExecution exec) {
+        try {
+            EnvVars contextEnvVars = context.get(EnvVars.class);
+            EnvironmentExpander contextExpander = context.get(EnvironmentExpander.class);
+            if (contextEnvVars != null && contextExpander != null) {
+                if (runReport == null) {
+                    FlowExecutionOwner owner = exec.getOwner();
+                    if (owner != null && owner.getExecutable() instanceof Run) {
+                        runReport = ((Run) owner.getExecutable()).getAction(EnvironmentWatcherRunReport.class);
+                    }
+                }
+                if (runReport != null) {
+                    return new EnvironmentWatcher(contextEnvVars, contextExpander);
+                }
+            }
+        } catch (InterruptedException | IOException e) {
+            LOGGER.log(Level.FINE, "Unable to create EnvironmentWatcher instance.\n" + e.getMessage());
+        }
+        return null;
+    }
+
+    public EnvironmentWatcher(@Nonnull EnvVars envVars, @Nonnull EnvironmentExpander expander) {
+        this.envVars = envVars;
+        watchedVars = expander.getSensitiveVars();
+    }
+
+    public void scan(String text) {
+        scanResults = watchedVars.stream().filter(e -> text.contains(envVars.get(e))).collect(Collectors.toList());
+    }
+
+    public void logResults(TaskListener listener) {
+        if (scanResults != null && !scanResults.isEmpty()) {
+            listener.getLogger().println("The following Groovy string may be insecure. Use single quotes to prevent leaking secrets via Groovy interpolation. Affected variables: "  + scanResults.toString());
+            runReport.record(scanResults);
+        }
+    }
+
+    private static final long serialVersionUID = 1L;
+}