From da7debfd65514265847f8efb93b23781c20f9925 Mon Sep 17 00:00:00 2001 From: alee Date: Wed, 18 Dec 2024 15:27:29 -0500 Subject: [PATCH 1/2] Reintroduce automatic hyperlinking of URLs in JUnit test output, safely --- pom.xml | 5 ++ .../java/hudson/tasks/test/TestResult.java | 80 ++++++++++++++++++- .../hudson/tasks/junit/CaseResultTest.java | 18 +++-- 3 files changed, 92 insertions(+), 11 deletions(-) diff --git a/pom.xml b/pom.xml index c5799993..645ea651 100644 --- a/pom.xml +++ b/pom.xml @@ -104,6 +104,11 @@ org.jenkins-ci.plugins.workflow workflow-step-api + + com.googlecode.owasp-java-html-sanitizer + owasp-java-html-sanitizer + 20220608.1 + io.jenkins configuration-as-code diff --git a/src/main/java/hudson/tasks/test/TestResult.java b/src/main/java/hudson/tasks/test/TestResult.java index b3850cd7..dd7992ad 100644 --- a/src/main/java/hudson/tasks/test/TestResult.java +++ b/src/main/java/hudson/tasks/test/TestResult.java @@ -32,6 +32,11 @@ import java.util.Map; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import org.owasp.html.HtmlPolicyBuilder; +import org.owasp.html.PolicyFactory; /** * A class that represents a general concept of a test result, without any @@ -43,6 +48,15 @@ public abstract class TestResult extends TestObject { private static final Logger LOGGER = Logger.getLogger(TestResult.class.getName()); + private static final PolicyFactory POLICY_DEFINITION = new HtmlPolicyBuilder() + .allowElements("a") + .allowUrlProtocols("https") + .allowUrlProtocols("http") + .allowAttributes("href").onElements("a") + .toFactory(); + + private static final Pattern LINK_REGEX_PATTERN = Pattern.compile("\\b(https?://[^\\s)<>\"]+)"); + /** * If the concept of a parent action is important to a subclass, then it should * provide a non-noop implementation of this method. @@ -284,16 +298,76 @@ public String toPrettyString() { } /** - * Annotate some text -- what does this do? - * @param text Text to use to annotate the actions. + * Escapes "&" and "<" characters in the provided text, with the goal of preventing any HTML tags present + * in the text from being rendered / interpreted as HTML. + * + * @param text The text to sanitize + * @return The sanitized text + */ + private static String naiveHtmlSanitize(String text) { + return text.replace("&", "&").replace("<", "<"); + } + + private static String escapeHtmlAndMakeLinksClickable(String text) { + if (text == null) { + return null; + } + + StringBuilder annotatedTxtBuilder = new StringBuilder(); + + Matcher linkMatcher = LINK_REGEX_PATTERN.matcher(text); + + // Goal: Find all the things that look like URLs in the text, and convert them to clickable + // tags so they render as clickable links when viewing test output. + + int lastMatchEndIdxExclusive = 0; + while (linkMatcher.find()) { + // Group 1 in the regex is just the URL + String linkUrl = linkMatcher.group(1); + + // Sanitize the final HTML tag we produce to make sure there's nothing malicious in there + String sanitizedLinkHtmlTag = POLICY_DEFINITION.sanitize( + String.format("%s", linkUrl, linkUrl)); + + annotatedTxtBuilder + // Append all the chars in-between the last link and this current one, and run that substring + // through naive HTML sanitization since we don't want anything other than the tags we're + // spawning to actually render as HTML. + .append(naiveHtmlSanitize(text.substring(lastMatchEndIdxExclusive, linkMatcher.start()))) + // Append our clickable tag + .append(sanitizedLinkHtmlTag); + + lastMatchEndIdxExclusive = linkMatcher.end(); + } + + // Finish up by sanitizing + appending all the remaining text after the last URL we found + annotatedTxtBuilder.append(naiveHtmlSanitize(text.substring(lastMatchEndIdxExclusive))); + + return annotatedTxtBuilder.toString(); + } + + /** + * All JUnit test output (error message, stack trace, std out, std err) shown on the single test case result page + * is passed through this method when the page is rendered, which processes / sanitizes the text to make it + * suitable for HTML rendering. Attempts to auto-detect URLs in the test output and convert them to clickable + * links for convenience. All other HTML will be escaped. + *

+ * Additionally, passes the test output through all the TestActions associated with this result, giving them a + * chance to apply their own custom rendering / transformations. + *

+ * Note: The test output shown when expanding cases on the full "testReport" page is *not* passed through this + * method, and instead relies on the Jelly "escape-by-default" flag to escape test output for HTML rendering, + * which is why links are not clickable in that context. * + * @param text Text to use to annotate the actions. * @return the provided text HTML-escaped. */ public String annotate(String text) { if (text == null) { return null; } - text = text.replace("&", "&").replace("<", "<"); + + text = escapeHtmlAndMakeLinksClickable(text); for (TestAction action : getTestActions()) { text = action.annotate(text); diff --git a/src/test/java/hudson/tasks/junit/CaseResultTest.java b/src/test/java/hudson/tasks/junit/CaseResultTest.java index ccd985e1..62d7401e 100644 --- a/src/test/java/hudson/tasks/junit/CaseResultTest.java +++ b/src/test/java/hudson/tasks/junit/CaseResultTest.java @@ -94,19 +94,21 @@ public void testIssue20090516() throws Exception { // piggy back tests for annotate methods assertOutput(cr, "plain text", "plain text"); - assertOutput(cr, "line #1\nhttp://nowhere.net/\nline #2\n", "line #1\nhttp://nowhere.net/\nline #2\n"); - assertOutput(cr, "failed; see http://nowhere.net/", "failed; see http://nowhere.net/"); - assertOutput(cr, "failed (see http://nowhere.net/)", "failed (see http://nowhere.net/)"); + assertOutput(cr, "line #1\nhttp://nowhere.net/\nline #2\n", "line #1\n" + "http://nowhere.net/\n" + "line #2\n"); + assertOutput(cr, "failed; see http://nowhere.net/", "failed; see http://nowhere.net/"); + assertOutput(cr, "failed (see http://nowhere.net/)", "failed (see http://nowhere.net/)"); assertOutput( cr, "http://nowhere.net/ - failed: http://elsewhere.net/", - "http://nowhere.net/ - failed: " + "http://elsewhere.net/"); - assertOutput(cr, "https://nowhere.net/", "https://nowhere.net/"); + "http://nowhere.net/ - failed: http://elsewhere.net/"); + assertOutput(cr, "https://nowhere.net/", "https://nowhere.net/"); assertOutput(cr, "stuffhttp://nowhere.net/", "stuffhttp://nowhere.net/"); assertOutput(cr, "a < b && c < d", "a < b && c < d"); - assertOutput(cr, "see ", "see <http://nowhere.net/>"); - assertOutput(cr, "http://google.com/?q=stuff&lang=en", "http://google.com/?q=stuff&lang=en"); - assertOutput(cr, "http://localhost:8080/stuff/", "http://localhost:8080/stuff/"); + assertOutput(cr, "see ", "see <http://nowhere.net/>"); + assertOutput(cr, "http://google.com/?q=stuff&lang=en", "http://google.com/?q=stuff&lang=en"); + assertOutput(cr, "http://localhost:8080/stuff/", "http://localhost:8080/stuff/"); + assertOutput(cr,"https://google.com\"onclick=alert(1)\"", + "https://google.com\"onclick=alert(1)\""); } /** From 69568f4aef349e8896be0f95d2ed7a4ffd92796c Mon Sep 17 00:00:00 2001 From: alee Date: Wed, 18 Dec 2024 16:08:03 -0500 Subject: [PATCH 2/2] Applied linting suggestions from spotless --- pom.xml | 10 +++--- .../java/hudson/tasks/test/TestResult.java | 8 ++--- .../hudson/tasks/junit/CaseResultTest.java | 32 +++++++++++++++---- 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/pom.xml b/pom.xml index 645ea651..6c4bbc87 100644 --- a/pom.xml +++ b/pom.xml @@ -59,6 +59,11 @@ + + com.googlecode.owasp-java-html-sanitizer + owasp-java-html-sanitizer + 20220608.1 + com.pivovarit parallel-collectors @@ -104,11 +109,6 @@ org.jenkins-ci.plugins.workflow workflow-step-api - - com.googlecode.owasp-java-html-sanitizer - owasp-java-html-sanitizer - 20220608.1 - io.jenkins configuration-as-code diff --git a/src/main/java/hudson/tasks/test/TestResult.java b/src/main/java/hudson/tasks/test/TestResult.java index dd7992ad..136dc277 100644 --- a/src/main/java/hudson/tasks/test/TestResult.java +++ b/src/main/java/hudson/tasks/test/TestResult.java @@ -34,7 +34,6 @@ import java.util.logging.Logger; import java.util.regex.Matcher; import java.util.regex.Pattern; - import org.owasp.html.HtmlPolicyBuilder; import org.owasp.html.PolicyFactory; @@ -52,7 +51,8 @@ public abstract class TestResult extends TestObject { .allowElements("a") .allowUrlProtocols("https") .allowUrlProtocols("http") - .allowAttributes("href").onElements("a") + .allowAttributes("href") + .onElements("a") .toFactory(); private static final Pattern LINK_REGEX_PATTERN = Pattern.compile("\\b(https?://[^\\s)<>\"]+)"); @@ -326,8 +326,8 @@ private static String escapeHtmlAndMakeLinksClickable(String text) { String linkUrl = linkMatcher.group(1); // Sanitize the final HTML tag we produce to make sure there's nothing malicious in there - String sanitizedLinkHtmlTag = POLICY_DEFINITION.sanitize( - String.format("%s", linkUrl, linkUrl)); + String sanitizedLinkHtmlTag = + POLICY_DEFINITION.sanitize(String.format("%s", linkUrl, linkUrl)); annotatedTxtBuilder // Append all the chars in-between the last link and this current one, and run that substring diff --git a/src/test/java/hudson/tasks/junit/CaseResultTest.java b/src/test/java/hudson/tasks/junit/CaseResultTest.java index 62d7401e..9783b94e 100644 --- a/src/test/java/hudson/tasks/junit/CaseResultTest.java +++ b/src/test/java/hudson/tasks/junit/CaseResultTest.java @@ -94,9 +94,18 @@ public void testIssue20090516() throws Exception { // piggy back tests for annotate methods assertOutput(cr, "plain text", "plain text"); - assertOutput(cr, "line #1\nhttp://nowhere.net/\nline #2\n", "line #1\n" + "http://nowhere.net/\n" + "line #2\n"); - assertOutput(cr, "failed; see http://nowhere.net/", "failed; see http://nowhere.net/"); - assertOutput(cr, "failed (see http://nowhere.net/)", "failed (see http://nowhere.net/)"); + assertOutput( + cr, + "line #1\nhttp://nowhere.net/\nline #2\n", + "line #1\n" + "http://nowhere.net/\n" + "line #2\n"); + assertOutput( + cr, + "failed; see http://nowhere.net/", + "failed; see http://nowhere.net/"); + assertOutput( + cr, + "failed (see http://nowhere.net/)", + "failed (see http://nowhere.net/)"); assertOutput( cr, "http://nowhere.net/ - failed: http://elsewhere.net/", @@ -104,10 +113,19 @@ public void testIssue20090516() throws Exception { assertOutput(cr, "https://nowhere.net/", "https://nowhere.net/"); assertOutput(cr, "stuffhttp://nowhere.net/", "stuffhttp://nowhere.net/"); assertOutput(cr, "a < b && c < d", "a < b && c < d"); - assertOutput(cr, "see ", "see <http://nowhere.net/>"); - assertOutput(cr, "http://google.com/?q=stuff&lang=en", "http://google.com/?q=stuff&lang=en"); - assertOutput(cr, "http://localhost:8080/stuff/", "http://localhost:8080/stuff/"); - assertOutput(cr,"https://google.com\"onclick=alert(1)\"", + assertOutput( + cr, "see ", "see <http://nowhere.net/>"); + assertOutput( + cr, + "http://google.com/?q=stuff&lang=en", + "http://google.com/?q=stuff&lang=en"); + assertOutput( + cr, + "http://localhost:8080/stuff/", + "http://localhost:8080/stuff/"); + assertOutput( + cr, + "https://google.com\"onclick=alert(1)\"", "https://google.com\"onclick=alert(1)\""); }