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

[JENKINS-60866] Make st:bind tag and JavaScript proxy work without inline JS #385

Merged
merged 31 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
8c67b73
[JENKINS-60866] Make st:bind tag work without inline JS
daniel-beck Jul 16, 2022
6e13cf8
Also have a separate script file if the bound object is null
daniel-beck Jul 16, 2022
63c818e
Add support for JavaScript proxy
daniel-beck Jul 17, 2022
0a41330
Make this work for SECURITY-595 in Jenkins
daniel-beck Jul 17, 2022
6298feb
Delete unused code
daniel-beck Jul 17, 2022
d9e6ea2
Merge remote-tracking branch 'stapler/master' into csp
daniel-beck Aug 2, 2022
522da35
Merge branch 'jenkinsci:master' into csp
daniel-beck Aug 10, 2022
09335c5
Merge remote-tracking branch 'origin/master' into csp
basil Aug 25, 2022
5f5dd3c
Merge branch 'master' into csp
daniel-beck May 30, 2023
c5b0f60
Merge branch 'master' into csp
daniel-beck Aug 11, 2023
7d73b0b
Merge remote-tracking branch 'origin/csp' into csp
daniel-beck Aug 11, 2023
5db6eea
As requested, don't document when this was deprecated
daniel-beck Aug 14, 2023
5887e70
Fix typo
daniel-beck Aug 14, 2023
09b893a
Make WithWellKnownURL with with CSP-compliant Stapler proxy
daniel-beck Aug 15, 2023
7d53ff7
Merge branch 'master' into csp
daniel-beck Aug 15, 2023
a7d1323
Rename "methods" to (JavaScript)UrlNames to reduce ambiguity
daniel-beck Aug 16, 2023
7bc33e0
Remove obsolete TODO
daniel-beck Aug 16, 2023
34d7e67
Fix "/null" restOfPath, note behavior with unexpected branch
daniel-beck Aug 21, 2023
16f23e3
Restrict identifiers further, allow fallback with unsafe var names
daniel-beck Aug 22, 2023
e4e11ed
Merge branch 'master' into csp
daniel-beck Aug 25, 2023
f67812c
Merge branch 'master' into csp
daniel-beck Sep 20, 2023
84a9ecd
Merge branch 'master' into csp
daniel-beck Sep 25, 2023
742f867
Merge branch 'master' into csp
daniel-beck Oct 6, 2023
81b96c0
Merge branch 'master' into csp
daniel-beck Oct 12, 2023
2ed4a53
Merge branch 'master' into csp
daniel-beck Nov 1, 2023
43e07ad
Merge branch 'master' into csp
daniel-beck Dec 15, 2023
7137f08
Merge branch 'master' into csp
daniel-beck Jan 3, 2024
377842b
Merge branch 'master' into csp
daniel-beck Feb 12, 2024
70afd88
Simplify string joining
daniel-beck Feb 12, 2024
0d1c34f
Javadoc improvements
daniel-beck Feb 13, 2024
dbb41c6
More Javadoc fixes
daniel-beck Feb 14, 2024
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
7 changes: 7 additions & 0 deletions core/src/main/java/org/kohsuke/stapler/RequestImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,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;
Expand Down Expand Up @@ -181,6 +182,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.getBoundMethods());
}

@Override
public Stapler getStapler() {
return stapler;
Expand Down
32 changes: 32 additions & 0 deletions core/src/main/java/org/kohsuke/stapler/StaplerRequest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -40,6 +41,7 @@

import net.sf.json.JSONObject;
import net.sf.json.JSONArray;
import org.apache.commons.lang.StringUtils;
import org.kohsuke.stapler.bind.BoundObjectTable;
import org.kohsuke.stapler.json.SubmittedForm;
import org.kohsuke.stapler.lang.Klass;
Expand Down Expand Up @@ -515,6 +517,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; // TODO Is this useful to not need hard-coding 'makeStaplerProxy' in callers?
public final String url;
public final String crumb;
public final Set<String> methods;

public RenderOnDemandParameters(String proxyMethod, String url, String crumb, Set<String> methods) {
this.proxyMethod = proxyMethod;
this.url = url;
this.crumb = crumb;
this.methods = methods;
}

public String getMethodsString() { // TODO Probably better off in the caller?
return StringUtils.join(methods, ",");
}
}

/**
* Exports the given Java object as a JavaScript proxy and returns the parameters needed to call
* {@code makeStaplerProxy}.
*/
RenderOnDemandParameters createJavaScriptProxyParameters(Object toBeExported);
}
81 changes: 58 additions & 23 deletions core/src/main/java/org/kohsuke/stapler/bind/Bound.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,16 @@

package org.kohsuke.stapler.bind;

import java.util.HashSet;
import java.util.Set;
import org.apache.commons.lang.StringUtils;
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}.
Expand Down Expand Up @@ -63,33 +65,66 @@ 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 this {@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
daniel-beck marked this conversation as resolved.
Show resolved Hide resolved
*/
public static String getProxyScriptUrl(String variableName, Bound bound) {
if (bound == null) {
// TODO Should this hardocde a non-UUID to ensure null bind?
daniel-beck marked this conversation as resolved.
Show resolved Hide resolved
return Stapler.getCurrentRequest().getContextPath() + BoundObjectTable.PREFIX + variableName + "/null";
} else {
return bound.getProxyScriptURL(variableName);
}
}

/**
* Returns the URL for the standalone proxy script.
* @param variableName the name of the JS variable to assign
* @return
daniel-beck marked this conversation as resolved.
Show resolved Hide resolved
*/
public final String getProxyScriptURL(String variableName) {
// TODO Probably wrong with WithWellKnownURL?
return getURL().replace(BoundObjectTable.PREFIX, BoundObjectTable.SCRIPT_PREFIX + variableName + "/");
}

boolean first=true;
for (Method m : getTarget().getClass().getMethods()) {
Collection<String> names;
private static Set<String> getBoundMethods(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 names;
}

/**
* Returns a list of all JS bound methods of the target's type.
* @return a list of all JS bound methods of the target's type
*/
public final Set<String> getBoundMethods() {
return getBoundMethods(getTarget().getClass());
}

public static String getProxyScript(String url, Class<?> clazz) {
StringBuilder buf = new StringBuilder("makeStaplerProxy('").append(url).append("','").append(
WebApp.getCurrent().getCrumbIssuer().issueCrumb()
).append("',[").append(
StringUtils.join(getBoundMethods(clazz).stream().map(n -> "'" + n + "'").toArray(String[]::new), ",")).append("])");
return buf.toString();
}

Expand Down
39 changes: 39 additions & 0 deletions core/src/main/java/org/kohsuke/stapler/bind/BoundObjectTable.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
package org.kohsuke.stapler.bind;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.io.PrintWriter;
import org.apache.commons.lang.StringUtils;
import org.kohsuke.stapler.Ancestor;
import org.kohsuke.stapler.HttpResponse;
import org.kohsuke.stapler.HttpResponses;
Expand All @@ -50,6 +52,42 @@
* @author Kohsuke Kawaguchi
*/
public class BoundObjectTable implements StaplerFallback {

private static boolean isValidIdentifier(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 void doScript(StaplerRequest req, StaplerResponse rsp) throws IOException {
final String variableNameAndId = StringUtils.removeStart(req.getRestOfPath(), "/");
if (!variableNameAndId.contains("/")) {
return;
}
final String variableName = variableNameAndId.split("/")[0];
final String id = variableNameAndId.split("/")[1];

if (!isValidIdentifier(variableName)) {
return;
}

rsp.setContentType("application/javascript");
final PrintWriter writer = rsp.getWriter();
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(variableName).append(" = null;");
return;
}
final String script = Bound.getProxyScript(Stapler.getCurrentRequest().getContextPath() + PREFIX + id, object.getClass());
writer.append(variableName).append(" = ").append(script).append(";");
}

@Override
public Table getStaplerFallback() {
return resolve(false);
Expand Down Expand Up @@ -246,6 +284,7 @@ private WeakRef(Object referent) {
}

public static final String PREFIX = "/$stapler/bound/";
public static final String SCRIPT_PREFIX = "/$stapler/bound/script/";

/**
* True to activate debug logging of session fragments.
Expand Down
39 changes: 27 additions & 12 deletions jelly/src/main/java/org/kohsuke/stapler/jelly/BindTag.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,15 @@

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.xml.sax.SAXException;
import org.xml.sax.helpers.AttributesImpl;

/**
* Binds a server-side object to client side so that JavaScript can call into server.
Expand Down Expand Up @@ -68,25 +70,38 @@ public void doTag(XMLOutput out) throws JellyTagException {
a.doTag(out);

try {
String expr;
if (javaObject==null) {
expr = "null";
if (varName == null) {
out.write("null");
} else {
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) {
// this mode (of writing just the expression) needs to be used with caution because
// the adjunct tag above might produce <script> tag.
// Doing this is deprecated as it cannot be done with CSP enabled
out.write(h.getProxyScript());
} else {
writeScriptTag(out, h);
}
}
} catch (SAXException e) {
throw new JellyTagException(e);
}
}

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));
daniel-beck marked this conversation as resolved.
Show resolved Hide resolved
}
attributes.addAttribute("", "type", "type", "", "application/javascript");
out.startElement("script", attributes);
out.endElement("script");
}
}