[JENKINS-60866] Make st:bind tag and JavaScript proxy work without in…

…line JS (#385) * [JENKINS-60866] Make st:bind tag work without inline JS * Also have a separate script file if the bound object is null * Add support for JavaScript proxy * Make this work for SECURITY-595 in Jenkins * Delete unused code * As requested, don't document when this was deprecated * Fix typo * Make WithWellKnownURL with with CSP-compliant Stapler proxy * Rename "methods" to (JavaScript)UrlNames to reduce ambiguity * Remove obsolete TODO * Fix "/null" restOfPath, note behavior with unexpected branch * Restrict identifiers further, allow fallback with unsafe var names * Simplify string joining * Javadoc improvements * More Javadoc fixes --------- Co-authored-by: Daniel Beck <[email protected]> Co-authored-by: Basil Crow <[email protected]>
jenkinsci · Feb 14, 2024 · ed17667 · ed17667
1 parent 7cbad37
commit ed17667
Show file tree

Hide file tree

Showing 5 changed files with 246 additions and 37 deletions.
diff --git a/core/src/main/java/org/kohsuke/stapler/RequestImpl.java b/core/src/main/java/org/kohsuke/stapler/RequestImpl.java
@@ -41,6 +41,7 @@
 import org.apache.commons.fileupload.disk.DiskFileItemFactory;
 import org.apache.commons.fileupload.servlet.ServletFileUpload;
 import org.jvnet.tiger_types.Lister;
+import org.kohsuke.stapler.bind.Bound;
 import org.kohsuke.stapler.bind.BoundObjectTable;
 import org.kohsuke.stapler.lang.Klass;
 import org.kohsuke.stapler.lang.MethodRef;
@@ -183,6 +184,12 @@ public String createJavaScriptProxy(Object toBeExported) {
         return getBoundObjectTable().bind(toBeExported).getProxyScript();
     }
 
+    @Override
+    public RenderOnDemandParameters createJavaScriptProxyParameters(Object toBeExported) {
+        final Bound bound = getBoundObjectTable().bind(toBeExported);
+        return new RenderOnDemandParameters("makeStaplerProxy", bound.getURL(), getWebApp().getCrumbIssuer().issueCrumb(), bound.getBoundJavaScriptUrlNames());
+    }
+
     @Override
     public Stapler getStapler() {
         return stapler;

diff --git a/core/src/main/java/org/kohsuke/stapler/StaplerRequest.java b/core/src/main/java/org/kohsuke/stapler/StaplerRequest.java
@@ -23,6 +23,7 @@
 
 package org.kohsuke.stapler;
 
+import java.util.Set;
 import org.apache.commons.beanutils.BeanUtils;
 import org.apache.commons.beanutils.ConvertUtils;
 import org.apache.commons.fileupload.FileItem;
@@ -515,6 +516,36 @@ public interface StaplerRequest extends HttpServletRequest {
      * a proxy on the client side.
      *
      * Short cut for {@code getBoundObjectTable().bind(toBeExported).getProxyScript()}
+     *
+     * @deprecated Use {@link #createJavaScriptProxyParameters(Object)} and invoke {@code makeStaplerProxy} yourself.
      */
+    @Deprecated
     String createJavaScriptProxy(Object toBeExported);
+
+    /**
+     * Return value of {@link #createJavaScriptProxyParameters(Object)}
+     */
+    final class RenderOnDemandParameters {
+        public final String proxyMethod;
+        public final String url;
+        public final String crumb;
+        public final Set<String> urlNames;
+
+        public RenderOnDemandParameters(String proxyMethod, String url, String crumb, Set<String> urlNames) {
+            this.proxyMethod = proxyMethod;
+            this.url = url;
+            this.crumb = crumb;
+            this.urlNames = urlNames;
+        }
+
+        public String getUrlNames() {
+            return String.join(",", urlNames);
+        }
+    }
+
+    /**
+     * Exports the given Java object as a JavaScript proxy and returns the parameters needed to call
+     * {@code makeStaplerProxy}.
+     */
+    RenderOnDemandParameters createJavaScriptProxyParameters(Object toBeExported);
 }
diff --git a/core/src/main/java/org/kohsuke/stapler/bind/Bound.java b/core/src/main/java/org/kohsuke/stapler/bind/Bound.java
@@ -23,14 +23,16 @@
 
 package org.kohsuke.stapler.bind;
 
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Collectors;
 import org.kohsuke.stapler.HttpResponse;
 import org.kohsuke.stapler.MetaClass;
+import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.WebApp;
 
 import java.lang.reflect.Method;
 import java.util.Arrays;
-import java.util.Collection;
-import java.util.Set;
 
 /**
  * Handles to the object bound via {@link BoundObjectTable}.
@@ -63,34 +65,78 @@ public abstract class Bound implements HttpResponse {
      * talks back to the bound object that this handle represents.
      */
     public final String getProxyScript() {
-        StringBuilder buf = new StringBuilder("makeStaplerProxy('").append(getURL()).append("','").append(
-                WebApp.getCurrent().getCrumbIssuer().issueCrumb()
-        ).append("',[");
+        return getProxyScript(getURL(), getTarget().getClass());
+    }
+
+    /**
+     * Returns the URL to the proxy script for the specified {@link org.kohsuke.stapler.bind.Bound}.
+     *
+     * @param variableName the variable to assign to the bound object
+     * @param bound the bound object, or {@code null} if none.
+     * @return the URL to the proxy script for the specified {@link org.kohsuke.stapler.bind.Bound}, starting with the context path
+     */
+    public static String getProxyScriptURL(String variableName, Bound bound) {
+        if (bound == null) {
+            return Stapler.getCurrentRequest().getContextPath() + BoundObjectTable.SCRIPT_PREFIX + "/null?var=" + variableName;
+        } else {
+            return bound.getProxyScriptURL(variableName);
+        }
+    }
+
+    /**
+     * Returns the URL for the standalone proxy script of this {@link org.kohsuke.stapler.bind.Bound}.
+     *
+     * @param variableName the name of the JS variable to assign
+     * @return the URL for the standalone proxy script of this {@link org.kohsuke.stapler.bind.Bound}, starting with the context path
+     */
+    public final String getProxyScriptURL(String variableName) {
+        final String methodsList = String.join(",", getBoundJavaScriptUrlNames(getTarget().getClass()));
+        // The URL looks like it has some redundant elements, but only if it's not a WithWellKnownURL
+        return Stapler.getCurrentRequest().getContextPath() + BoundObjectTable.SCRIPT_PREFIX + getURL() + "?var=" + variableName + "&methods=" + methodsList;
+    }
 
-        boolean first=true;
-        for (Method m : getTarget().getClass().getMethods()) {
-            Collection<String> names;
+    private static Set<String> getBoundJavaScriptUrlNames(Class<?> clazz) {
+        Set<String> names = new HashSet<>();
+        for (Method m : clazz.getMethods()) {
             if (m.getName().startsWith("js")) {
-                names = Set.of(camelize(m.getName().substring(2)));
+                names.add(camelize(m.getName().substring(2)));
             } else {
                 JavaScriptMethod a = m.getAnnotation(JavaScriptMethod.class);
-                if (a!=null) {
-                    names = Arrays.asList(a.name());
-                    if (names.isEmpty())
-                        names = Set.of(m.getName());
-                } else
-                    continue;
-            }
-
-            for (String n : names) {
-                if (first)  first = false;
-                else        buf.append(',');
-                buf.append('\'').append(n).append('\'');
+                if (a != null) {
+                    if (a.name().length == 0) {
+                        names.add(m.getName());
+                    } else {
+                        names.addAll(Arrays.asList(a.name()));
+                    }
+                }
             }
         }
-        buf.append("])");
-
-        return buf.toString();
+        return names;
+    }
+
+    /**
+     * Returns a collection of all JS bound methods of the target's type.
+     * @return a collection of all JS bound methods of the target's type
+     */
+    public final Set<String> getBoundJavaScriptUrlNames() {
+        return getBoundJavaScriptUrlNames(getTarget().getClass());
+    }
+
+    public static String getProxyScript(String url, Class<?> clazz) {
+        return getProxyScript(url, getBoundJavaScriptUrlNames(clazz).toArray(String[]::new));
+    }
+
+    /**
+     * Returns the Stapler proxy script for the specified URL and method names
+     *
+     * @param url URL to proxied object
+     * @param methods list of method names
+     * @return the Stapler proxy script for the specified URL and method names
+     */
+    public static String getProxyScript(String url, String[] methods) {
+        final String crumb = WebApp.getCurrent().getCrumbIssuer().issueCrumb();
+        final String methodNamesList = Arrays.stream(methods).sorted().map(it -> "'" + it + "'").collect(Collectors.joining(","));
+        return "makeStaplerProxy('" + url + "','" + crumb + "',[" + methodNamesList +  "])";
     }
 
     private static String camelize(String name) {

diff --git a/core/src/main/java/org/kohsuke/stapler/bind/BoundObjectTable.java b/core/src/main/java/org/kohsuke/stapler/bind/BoundObjectTable.java
@@ -24,9 +24,14 @@
 package org.kohsuke.stapler.bind;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import java.io.PrintWriter;
+import java.util.Arrays;
+import java.util.logging.Level;
+import org.apache.commons.lang.StringUtils;
 import org.kohsuke.stapler.Ancestor;
 import org.kohsuke.stapler.HttpResponse;
 import org.kohsuke.stapler.HttpResponses;
+import org.kohsuke.stapler.QueryParameter;
 import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.StaplerFallback;
 import org.kohsuke.stapler.StaplerRequest;
@@ -51,6 +56,93 @@
  * @author Kohsuke Kawaguchi
  */
 public class BoundObjectTable implements StaplerFallback {
+
+    public static boolean isValidJavaScriptIdentifier(String variableName) {
+        // Ultimately this will be used as a JS identifier, so we need (a subset of) what's valid there.
+        // The primary purpose of this check however is to prevent injection attacks.
+        return variableName.matches("^[a-zA-Z_][a-zA-Z0-9_]*$");
+    }
+
+    public static boolean isValidJavaIdentifier(String name) {
+        if (name == null || StringUtils.isBlank(name)) {
+            return false;
+        }
+        if (!Character.isJavaIdentifierStart(name.charAt(0)) || Character.codePointAt(name, 0) > 255) {
+            return false;
+        }
+        // Limit characters to legal Java identifier parts in Latin-1 that aren't ignorable
+        return name.substring(1)
+                .chars()
+                .allMatch(it -> Character.isJavaIdentifierPart(it) && !Character.isIdentifierIgnorable(it) && it < 256);
+    }
+
+    /**
+     * This serves the script content for a bound object. Support CSP-compatible st:bind and similar methods of making
+     * objects accessible to JS.
+     *
+     * @param req
+     * @param rsp
+     * @param var the variable name to assign the Stapler proxy to
+     * @param methods the list of methods (needed for {@link WithWellKnownURL})
+     * @throws IOException
+     */
+    public void doScript(StaplerRequest req, StaplerResponse rsp, @QueryParameter String var, @QueryParameter String methods) throws IOException {
+        final String boundUrl = req.getRestOfPath();
+
+        if (var == null) {
+            return;
+        }
+
+        if (!isValidJavaScriptIdentifier(var)) {
+            LOGGER.log(Level.FINE, () -> "Rejecting invalid JavaScript identifier: " + var);
+            return;
+        }
+
+        rsp.setContentType("application/javascript");
+        final PrintWriter writer = rsp.getWriter();
+
+        if ("/null".equals(boundUrl)) {
+            /* This is the case when the object was null in the first place */
+            writer.append(var).append(" = null;");
+            return;
+        }
+
+        final String script;
+
+        /* If this is not a WithWellKnownURL, look UUID up in bound object table and return null if not found. */
+        final String contextAndPrefix = Stapler.getCurrentRequest().getContextPath() + PREFIX;
+        if (boundUrl.startsWith(contextAndPrefix)) {
+            final String id = boundUrl.replace(contextAndPrefix, "");
+            final Table table = resolve(false);
+            if (table == null) {
+                rsp.sendError(404);
+                return;
+            }
+            Object object = table.resolve(id);
+            if (object == null) {
+                /* Support null bound objects */
+                writer.append(var).append(" = null;");
+                return;
+            }
+            script = Bound.getProxyScript(boundUrl, object.getClass());
+        } else {
+            if (methods == null) {
+                /* This will result in an empty file rather than an explicit null assignment,
+                   but it's unexpected to have a WithWellKnownURL without ?methods query parameter. */
+                return;
+            }
+            final String[] methodsArray = methods.split(",");
+            if (Arrays.stream(methodsArray).anyMatch(it -> !isValidJavaIdentifier(it))) {
+                LOGGER.log(Level.FINE, () -> "Rejecting method list that includes an invalid Java identifier: " + methods);
+                // TODO Alternatively, filter out invalid method names and only include valid ones.
+                //  Could help with non-malicious but encoding related issues
+                return;
+            }
+            script = Bound.getProxyScript(boundUrl, methodsArray);
+        }
+        writer.append(var).append(" = ").append(script).append(";");
+    }
+
     @Override
     public Table getStaplerFallback() {
         return resolve(false);
@@ -264,6 +356,7 @@ private Object writeReplace() {
     }
 
     public static final String PREFIX = "/$stapler/bound/";
+    static final String SCRIPT_PREFIX = "/$stapler/bound/script";
 
     /**
      * True to activate debug logging of session fragments.

diff --git a/jelly/src/main/java/org/kohsuke/stapler/jelly/BindTag.java b/jelly/src/main/java/org/kohsuke/stapler/jelly/BindTag.java
@@ -23,13 +23,16 @@
 
 package org.kohsuke.stapler.jelly;
 
+import edu.umd.cs.findbugs.annotations.CheckForNull;
 import org.apache.commons.jelly.JellyTagException;
 import org.apache.commons.jelly.XMLOutput;
 import org.jvnet.maven.jellydoc.annotation.NoContent;
 import org.jvnet.maven.jellydoc.annotation.Required;
 import org.kohsuke.stapler.WebApp;
 import org.kohsuke.stapler.bind.Bound;
+import org.kohsuke.stapler.bind.BoundObjectTable;
 import org.xml.sax.SAXException;
+import org.xml.sax.helpers.AttributesImpl;
 
 /**
  * Binds a server-side object to client side so that JavaScript can call into server.
@@ -68,25 +71,54 @@ public void doTag(XMLOutput out) throws JellyTagException {
         a.doTag(out);
 
         try {
-            String expr;
-            if (javaObject==null) {
-                expr = "null";
+            if (javaObject == null) {
+                if (varName == null) {
+                    // Legacy mode if no 'var' is specified and we bound 'null': Write 'null' expression inline, without <script> wrapper.
+                    out.write("null");
+                } else {
+                    // Modern: Write a script tag whose 'src' points to DataBoundTable#doScript, with a special suffix indicating the null bound object.
+                    writeScriptTag(out, null);
+                }
             } else {
                 Bound h = WebApp.getCurrent().boundObjectTable.bind(javaObject);
-                expr = h.getProxyScript();
-            }
 
-            if (varName==null) {
-                // this mode (of writing just the expression) needs to be used with caution because
-                // the adjunct tag above might produce <script> tag.
-                out.write(expr);
-            } else {
-                out.startElement("script");
-                out.write(varName + "=" + expr + ";");
-                out.endElement("script");
+                if (varName == null) {
+                    // Legacy mode if no 'var' is specified: Write the expression inline, without <script> wrapper.
+                    // Doing this is deprecated as it cannot be done with Content-Security-Policy unless 'unsafe-inline' is allowed.
+                    // Additionally, this mode needs to be used with caution because the adjunct tag above might produce a <script> tag.
+                    out.write(h.getProxyScript());
+                } else if (!BoundObjectTable.isValidJavaScriptIdentifier(varName)) {
+                    // Legacy mode if 'var' is not a safe variable name: Write the expression inline within <script> wrapper.
+                    // Doing this is deprecated as it cannot be done with Content-Security-Policy unless 'unsafe-inline' is allowed.
+                    out.startElement("script");
+                    out.write(varName + "=" + h.getProxyScript() + ";");
+                    out.endElement("script");
+                } else {
+                    // Modern: Write a script tag whose 'src' points to DataBoundTable#doScript
+                    writeScriptTag(out, h);
+                }
             }
         } catch (SAXException e) {
             throw new JellyTagException(e);
         }
     }
+
+    /**
+     * Writes a {@code <script>} tag whose {@code src} attribute points to
+     * {@link BoundObjectTable#doScript(org.kohsuke.stapler.StaplerRequest, org.kohsuke.stapler.StaplerResponse, String, String)}.
+     * @param out XML output
+     * @param bound Wrapper for the bound object
+     * @throws SAXException if something goes wrong writing XML
+     */
+    private void writeScriptTag(XMLOutput out, @CheckForNull Bound bound) throws SAXException {
+        final AttributesImpl attributes = new AttributesImpl();
+        if (bound == null) {
+            attributes.addAttribute("", "src", "src", "", Bound.getProxyScriptURL(varName, null));
+        } else {
+            attributes.addAttribute("", "src", "src", "", bound.getProxyScriptURL(varName));
+        }
+        attributes.addAttribute("", "type", "type", "", "application/javascript");
+        out.startElement("script", attributes);
+        out.endElement("script");
+    }
 }