From 159934fbb09ca42d4a52f7a4030df8a0d918e011 Mon Sep 17 00:00:00 2001 From: Kirill Merkushev Date: Wed, 16 Mar 2016 00:27:25 +0300 Subject: [PATCH 1/2] prevent JS in link to github --- .../plugins/github/GithubLinkAction.java | 9 ++--- .../jenkinsci/plugins/github/util/XSSApi.java | 27 +++++++++++++++ .../plugins/github/util/XSSApiTest.java | 34 +++++++++++++++++++ 3 files changed, 66 insertions(+), 4 deletions(-) create mode 100644 src/main/java/org/jenkinsci/plugins/github/util/XSSApi.java create mode 100644 src/test/java/org/jenkinsci/plugins/github/util/XSSApiTest.java diff --git a/src/main/java/com/coravy/hudson/plugins/github/GithubLinkAction.java b/src/main/java/com/coravy/hudson/plugins/github/GithubLinkAction.java index 4f9c63901..b92c5f4b4 100644 --- a/src/main/java/com/coravy/hudson/plugins/github/GithubLinkAction.java +++ b/src/main/java/com/coravy/hudson/plugins/github/GithubLinkAction.java @@ -1,12 +1,13 @@ package com.coravy.hudson.plugins.github; -import java.util.Collection; -import java.util.Collections; - import hudson.Extension; import hudson.model.Action; import hudson.model.Job; import jenkins.model.TransientActionFactory; +import org.jenkinsci.plugins.github.util.XSSApi; + +import java.util.Collection; +import java.util.Collections; /** * Add the Github Logo/Icon to the sidebar. @@ -33,7 +34,7 @@ public String getIconFileName() { @Override public String getUrlName() { - return projectProperty.getProjectUrl().baseUrl(); + return XSSApi.asValidHref(projectProperty.getProjectUrl().baseUrl()); } @SuppressWarnings("rawtypes") diff --git a/src/main/java/org/jenkinsci/plugins/github/util/XSSApi.java b/src/main/java/org/jenkinsci/plugins/github/util/XSSApi.java new file mode 100644 index 000000000..42f6bb9d2 --- /dev/null +++ b/src/main/java/org/jenkinsci/plugins/github/util/XSSApi.java @@ -0,0 +1,27 @@ +package org.jenkinsci.plugins.github.util; + +import java.net.MalformedURLException; +import java.net.URL; + +/** + * @author lanwen (Merkushev Kirill) + */ +public final class XSSApi { + private XSSApi() { + } + + /** + * Method to filter invalid url for XSS. This url can be inserted to href safely + * + * @param urlString unsafe url + * + * @return safe url + */ + public static String asValidHref(String urlString) { + try { + return new URL(urlString).toExternalForm(); + } catch (MalformedURLException e) { + return ""; + } + } +} diff --git a/src/test/java/org/jenkinsci/plugins/github/util/XSSApiTest.java b/src/test/java/org/jenkinsci/plugins/github/util/XSSApiTest.java new file mode 100644 index 000000000..a6670c96e --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/github/util/XSSApiTest.java @@ -0,0 +1,34 @@ +package org.jenkinsci.plugins.github.util; + +import com.tngtech.java.junit.dataprovider.DataProvider; +import com.tngtech.java.junit.dataprovider.DataProviderRunner; +import com.tngtech.java.junit.dataprovider.UseDataProvider; +import org.junit.Test; +import org.junit.runner.RunWith; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; + +/** + * @author lanwen (Merkushev Kirill) + */ +@RunWith(DataProviderRunner.class) +public class XSSApiTest { + + @DataProvider + public static Object[][] links() { + return new Object[][]{ + new Object[]{"javascript:alert(1);//", ""}, + new Object[]{"http://abcxyz.com?a=b&c=d';alert(1);//", "http://abcxyz.com?a=b&c=d';alert(1);//"}, + new Object[]{"http://github.com/bla/bla", "http://github.com/bla/bla"}, + new Object[]{"https://github.com/bla/bla", "https://github.com/bla/bla"}, + new Object[]{"https://company.com/bla", "https://company.com/bla"} + }; + } + + @Test + @UseDataProvider("links") + public void shouldSanitizeUrl(String url, String expected) throws Exception { + assertThat(XSSApi.asValidHref(url), is(expected)); + } +} From 12aaa78273a484224030c051417cef3d30275a2d Mon Sep 17 00:00:00 2001 From: Kirill Merkushev Date: Wed, 16 Mar 2016 17:19:07 +0300 Subject: [PATCH 2/2] add logging for url cleanup and more tests with links --- .../org/jenkinsci/plugins/github/util/XSSApi.java | 9 +++++++++ .../jenkinsci/plugins/github/util/XSSApiTest.java | 15 +++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/util/XSSApi.java b/src/main/java/org/jenkinsci/plugins/github/util/XSSApi.java index 42f6bb9d2..1bbc09b06 100644 --- a/src/main/java/org/jenkinsci/plugins/github/util/XSSApi.java +++ b/src/main/java/org/jenkinsci/plugins/github/util/XSSApi.java @@ -1,12 +1,20 @@ package org.jenkinsci.plugins.github.util; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import java.net.MalformedURLException; import java.net.URL; /** * @author lanwen (Merkushev Kirill) */ +@Restricted(NoExternalUse.class) public final class XSSApi { + private static final Logger LOG = LoggerFactory.getLogger(XSSApi.class); + private XSSApi() { } @@ -21,6 +29,7 @@ public static String asValidHref(String urlString) { try { return new URL(urlString).toExternalForm(); } catch (MalformedURLException e) { + LOG.debug("Malformed url - {}, empty string will be returned", urlString); return ""; } } diff --git a/src/test/java/org/jenkinsci/plugins/github/util/XSSApiTest.java b/src/test/java/org/jenkinsci/plugins/github/util/XSSApiTest.java index a6670c96e..4ce33af75 100644 --- a/src/test/java/org/jenkinsci/plugins/github/util/XSSApiTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/util/XSSApiTest.java @@ -6,6 +6,7 @@ import org.junit.Test; import org.junit.runner.RunWith; +import static java.lang.String.format; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; @@ -19,16 +20,26 @@ public class XSSApiTest { public static Object[][] links() { return new Object[][]{ new Object[]{"javascript:alert(1);//", ""}, + new Object[]{"javascript:alert(1)://", ""}, new Object[]{"http://abcxyz.com?a=b&c=d';alert(1);//", "http://abcxyz.com?a=b&c=d';alert(1);//"}, new Object[]{"http://github.com/bla/bla", "http://github.com/bla/bla"}, new Object[]{"https://github.com/bla/bla", "https://github.com/bla/bla"}, - new Object[]{"https://company.com/bla", "https://company.com/bla"} + new Object[]{"https://company.com/bla", "https://company.com/bla"}, + new Object[]{"/company.com/bla", ""}, + new Object[]{"//", ""}, + new Object[]{"//text", ""}, + new Object[]{"//text/", ""}, + new Object[]{"ftp://", "ftp:"}, + new Object[]{"ftp://a", "ftp://a"}, + new Object[]{"text", ""}, + new Object[]{"github.com/bla/bla", ""}, + new Object[]{"http://127.0.0.1/", "http://127.0.0.1/"}, }; } @Test @UseDataProvider("links") public void shouldSanitizeUrl(String url, String expected) throws Exception { - assertThat(XSSApi.asValidHref(url), is(expected)); + assertThat(format("For %s", url), XSSApi.asValidHref(url), is(expected)); } }