From a713bc0d95d6bcff0b163fca9c5cec5a494a2bfa Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Fri, 17 Aug 2018 22:45:41 -0400 Subject: [PATCH] [JENKINS-53016] Harden (Model)HyperlinkNote against display names containing newlines (#3580) * Harden ModelHyperlinkNote against display names containing newlines * Add regression test * Harden HyperlinkNote as well * Remove unnecessary wildcard in function type * Restrict new method to be extra careful --- .../java/hudson/console/HyperlinkNote.java | 17 +++- .../hudson/console/ModelHyperlinkNote.java | 10 +-- .../hudson/console/HyperlinkNoteTest.java | 83 +++++++++++++++++++ 3 files changed, 100 insertions(+), 10 deletions(-) create mode 100644 test/src/test/java/hudson/console/HyperlinkNoteTest.java diff --git a/core/src/main/java/hudson/console/HyperlinkNote.java b/core/src/main/java/hudson/console/HyperlinkNote.java index 79fbbffc70..8ed5039993 100644 --- a/core/src/main/java/hudson/console/HyperlinkNote.java +++ b/core/src/main/java/hudson/console/HyperlinkNote.java @@ -27,10 +27,13 @@ import hudson.Extension; import hudson.MarkupText; import jenkins.model.Jenkins; import org.jenkinsci.Symbol; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.Stapler; import org.kohsuke.stapler.StaplerRequest; import java.io.IOException; +import java.util.function.BiFunction; import java.util.logging.Level; import java.util.logging.Logger; @@ -75,8 +78,20 @@ public class HyperlinkNote extends ConsoleNote { } public static String encodeTo(String url, String text) { + return encodeTo(url, text, HyperlinkNote::new); + } + + @Restricted(NoExternalUse.class) + static String encodeTo(String url, String text, BiFunction constructor) { + // If text contains newlines, then its stored length will not match its length when being + // displayed, since the display length will only include text up to the first newline, + // which will cause an IndexOutOfBoundsException in MarkupText#rangeCheck when + // ConsoleAnnotationOutputStream converts the note into markup. That stream treats '\n' as + // the sole end-of-line marker on all platforms, so we ignore '\r' because it will not + // break the conversion. + text = text.replace('\n', ' '); try { - return new HyperlinkNote(url,text.length()).encode()+text; + return constructor.apply(url,text.length()).encode()+text; } catch (IOException e) { // impossible, but don't make this a fatal problem LOGGER.log(Level.WARNING, "Failed to serialize "+HyperlinkNote.class,e); diff --git a/core/src/main/java/hudson/console/ModelHyperlinkNote.java b/core/src/main/java/hudson/console/ModelHyperlinkNote.java index 784934708d..127911cc76 100644 --- a/core/src/main/java/hudson/console/ModelHyperlinkNote.java +++ b/core/src/main/java/hudson/console/ModelHyperlinkNote.java @@ -5,8 +5,6 @@ import hudson.model.*; import jenkins.model.Jenkins; import org.jenkinsci.Symbol; -import java.io.IOException; -import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.Nonnull; @@ -57,13 +55,7 @@ public class ModelHyperlinkNote extends HyperlinkNote { } public static String encodeTo(String url, String text) { - try { - return new ModelHyperlinkNote(url,text.length()).encode()+text; - } catch (IOException e) { - // impossible, but don't make this a fatal problem - LOGGER.log(Level.WARNING, "Failed to serialize "+ModelHyperlinkNote.class,e); - return text; - } + return HyperlinkNote.encodeTo(url, text, ModelHyperlinkNote::new); } @Extension @Symbol("hyperlinkToModels") diff --git a/test/src/test/java/hudson/console/HyperlinkNoteTest.java b/test/src/test/java/hudson/console/HyperlinkNoteTest.java new file mode 100644 index 0000000000..c65019f292 --- /dev/null +++ b/test/src/test/java/hudson/console/HyperlinkNoteTest.java @@ -0,0 +1,83 @@ +/* + * The MIT License + * + * Copyright 2018 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package hudson.console; + +import hudson.model.FreeStyleProject; +import java.io.IOException; +import java.io.StringReader; +import java.io.StringWriter; +import java.nio.charset.StandardCharsets; +import org.apache.commons.io.IOUtils; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; + +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.assertThat; + +public class HyperlinkNoteTest { + + @Rule + public JenkinsRule r = new JenkinsRule(); + + @Issue("JENKINS-53016") + @Test + public void textWithNewlines() throws Exception { + String url = r.getURL().toString()+"test"; + String noteText = "\nthis string\nhas newline\r\ncharacters\n\r"; + String input = HyperlinkNote.encodeTo(url, noteText); + String noteTextSanitized = input.substring(input.length() - noteText.length()); + // Throws IndexOutOfBoundsException before https://github.com/jenkinsci/jenkins/pull/3580. + String output = annotate(input); + assertThat(output, allOf( + containsString("href='" + url + "'"), + containsString(">" + noteTextSanitized + ""))); + } + + @Issue("JENKINS-53016") + @Test + public void textWithNewlinesModelHyperlinkNote() throws Exception { + FreeStyleProject p = r.createFreeStyleProject(); + String noteText = "\nthis string\nhas newline\r\ncharacters\n\r"; + String input = ModelHyperlinkNote.encodeTo(p, noteText); + String noteTextSanitized = input.substring(input.length() - noteText.length()); + // Throws IndexOutOfBoundsException before https://github.com/jenkinsci/jenkins/pull/3580. + String output = annotate(input); + assertThat(output, allOf( + containsString("href='" + r.getURL().toString()+p.getUrl() + "'"), + containsString(new ModelHyperlinkNote("", 0).extraAttributes()), + containsString(">" + noteTextSanitized + ""))); + } + + private static String annotate(String text) throws IOException { + StringWriter writer = new StringWriter(); + try (ConsoleAnnotationOutputStream out = new ConsoleAnnotationOutputStream(writer, null, null, StandardCharsets.UTF_8)) { + IOUtils.copy(new StringReader(text), out); + } + return writer.toString(); + } +} -- GitLab