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.

Sign up for GitHub

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

add checks for inline javascript #868

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

add checks for inline javascript #868

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f45a7c8
add checks for inline javascript
mawinter69 Oct 30, 2024
4b74eb8
Merge branch 'master' into jelly-verifier
mawinter69 Oct 30, 2024
261c314
add link to csp page
mawinter69 Oct 30, 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
61 changes: 59 additions & 2 deletions src/main/java/org/jvnet/hudson/test/JellyTestSuiteBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,11 @@
import java.io.File;
import java.io.IOException;
import java.net.URL;
import java.util.ArrayList;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.concurrent.Callable;
import java.util.jar.JarEntry;
Expand All @@ -36,7 +39,11 @@
import junit.framework.TestResult;
import junit.framework.TestSuite;
import org.apache.commons.io.FileUtils;
import org.dom4j.Attribute;
import org.dom4j.Document;
import org.dom4j.Element;
import org.dom4j.Namespace;
import org.dom4j.Node;
import org.dom4j.ProcessingInstruction;
import org.dom4j.io.SAXReader;
import org.jvnet.hudson.test.junit.GroupedTest;
Expand Down Expand Up @@ -88,6 +95,8 @@
private final URL jelly;
private final JellyClassLoaderTearOff jct;
private final boolean requirePI;
private List<String> errors = new ArrayList<>();
private boolean inlineJs = false;

JellyCheck(URL jelly, String name, JellyClassLoaderTearOff jct, boolean requirePI) {
super(name);
Expand All @@ -103,11 +112,59 @@
if (requirePI) {
ProcessingInstruction pi = dom.processingInstruction("jelly");
if (pi == null || !pi.getText().contains("escape-by-default")) {
throw new AssertionError("<?jelly escape-by-default='true'?> is missing in "+jelly);
errors.add("<?jelly escape-by-default='true'?> is missing in "+jelly);
}

}
// TODO: what else can we check statically? use of taglibs?

Check warning on line 118 in src/main/java/org/jvnet/hudson/test/JellyTestSuiteBuilder.java

View check run for this annotation

ci.jenkins.io / Open Tasks Scanner

TODO 

NORMAL:
what else can we check statically? use of taglibs?
checkScriptElement(dom);
checkJavaScriptAttributes(dom);
if (!errors.isEmpty()) {
if (inlineJs) {
errors.add("Please visit https://www.jenkins.io/doc/developer/security/csp/ for more details.");
}
String message = String.join("\n", errors);
throw new AssertionError(message);
}
}

private void checkJavaScriptAttributes(Document dom) {
List<Node> allNodes = dom.selectNodes("//*");
allNodes.forEach(n -> {
Element element = (Element) n;
Attribute onclick = element.attribute("onclick");
Attribute checkUrl = element.attribute("checkUrl");
Attribute checkDependsOn = element.attribute("checkDependsOn");
if (checkUrl != null && checkDependsOn == null) {
errors.add("Usage of 'checkUrl' without 'checkDependsOn' in "+jelly);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this is clear enough to people, e.g. in most cases people need to add a checkDependsOn="".

Personally I think this isn't clear enough on its own.

(It would be good if this could be fixed so it doesn't need the empty checkDependsOn)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably when we have CSP enabled by default and all plugins are adjusted we could remove an empty checkDependsOn, until then we need it to find out if we have to run the eval on the checkUrl.
We could add a link to https://www.jenkins.io/doc/developer/security/csp/ whenever there is an inline javascript found.

inlineJs = true;
}
if (onclick != null && element.getNamespace() != Namespace.NO_NAMESPACE) {
errors.add("Usage of 'onclick' from a taglib in "+jelly);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we link to a page explaining how to fix this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When used via a taglib it mean the resulting tag will also have that onclick handler. Afaik for all places in core that define an onclick in a taglib it is marked as deprecated. So the usage of onclick from a taglib is not really different from an onclick on a plain html element.

inlineJs = true;
}
List<Attribute> attributes = element.attributes();
if (element.getNamespace() == Namespace.NO_NAMESPACE && !attributes.isEmpty()) {
attributes.forEach(a -> {
if (a.getName().startsWith("on")) {
errors.add("Usage of inline event handler '" + a.getName() + "' in "+jelly);
inlineJs = true;
}
});
}
});
}

private void checkScriptElement(Document dom) {
List<Node> scriptTags = dom.selectNodes("//script");
scriptTags.forEach( n -> {
Element element = (Element) n;
String typeAttribute = element.attributeValue("type");
if (element.attributeValue("src") == null && (typeAttribute == null ||
!"application/json".equals(typeAttribute.toLowerCase(Locale.US)))) {
errors.add("inline <script> element in "+jelly);
inlineJs = true;
}
});
}

private boolean isConfigJelly() {
Expand Down