Skip to content

Commit

Permalink
SECURITY-3246
Browse files Browse the repository at this point in the history
  • Loading branch information
BorisYaoA committed Oct 18, 2023
1 parent 309cf75 commit 9e09678
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,20 @@
import hudson.plugins.git.GitChangeSet;
import hudson.scm.ChangeLogAnnotator;
import hudson.scm.ChangeLogSet.Entry;
import org.apache.commons.lang.StringUtils;

import javax.annotation.CheckForNull;
import javax.annotation.CheckReturnValue;
import javax.annotation.Nonnull;

import static hudson.Functions.htmlAttributeEscape;
import static java.lang.String.format;

import java.net.URI;
import java.net.URISyntaxException;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;
import java.util.regex.Pattern;

/**
Expand All @@ -26,6 +37,13 @@
@Extension
public class GithubLinkAnnotator extends ChangeLogAnnotator {

private static final Set<String> ALLOWED_URI_SCHEMES = new HashSet<String>();

static {
ALLOWED_URI_SCHEMES.addAll(
Arrays.asList("http", "https"));
}

@Override
public void annotate(Run<?, ?> build, Entry change, MarkupText text) {
final GithubProjectProperty p = build.getParent().getProperty(
Expand All @@ -38,15 +56,18 @@ public void annotate(Run<?, ?> build, Entry change, MarkupText text) {

void annotate(final GithubUrl url, final MarkupText text, final Entry change) {
final String base = url.baseUrl();
boolean isValid = verifyUrl(base);
if (!isValid) {
throw new IllegalArgumentException("The provided Github URL is not valid");
}
for (LinkMarkup markup : MARKUPS) {
markup.process(text, base);
}

if (change instanceof GitChangeSet) {
GitChangeSet cs = (GitChangeSet) change;
final String id = cs.getId();
text.wrapBy("", format(" (<a href='%s'>commit: %s</a>)",
url.commitId(id),
htmlAttributeEscape(url.commitId(id)),
id.substring(0, Math.min(id.length(), 7))));
}
}
Expand All @@ -66,7 +87,7 @@ private static final class LinkMarkup {

void process(MarkupText text, String url) {
for (SubText st : text.findTokens(pattern)) {
st.surroundWith("<a href='" + url + href + "'>", "</a>");
st.surroundWith("<a href='" + htmlAttributeEscape(url) + href + "'>", "</a>");
}
}

Expand All @@ -78,4 +99,34 @@ void process(MarkupText text, String url) {
private static final LinkMarkup[] MARKUPS = new LinkMarkup[]{new LinkMarkup(
"(?:C|c)lose(?:s?)\\s(?<!\\:)(?:#)NUM", // "Closes #123"
"issues/$1")};

@Nonnull
public static String getAllowedUriSchemes() {
return StringUtils.join(ALLOWED_URI_SCHEMES, ',');
}

@CheckReturnValue
@Nonnull
public static boolean verifyUrl(@CheckForNull String urlString) {
if (StringUtils.isBlank(urlString)) {
return false;
}

// Copy of the code from Functions#getActionUrl()
final URI uri;
try {
uri = new URI(urlString);
} catch (URISyntaxException ex) {
return false;
}

// Let's check if the scheme is allowed
String toCheck = uri.getScheme().toLowerCase();
if (!ALLOWED_URI_SCHEMES.contains(toCheck)) {
return false;
}

return true;

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.jvnet.hudson.test.Issue;

import static java.lang.String.format;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
Expand Down Expand Up @@ -80,6 +82,22 @@ public void inputIsExpectedWithChangeSet(String input, String expected) throws E
is(expected + expectedChangeSetAnnotation));
}

//Test to verify that fake url starting with sentences like javascript are not validated
@Test(expected = IllegalArgumentException.class)
@Issue("SECURITY-3246")
public void urlValidationTest() {
GithubLinkAnnotator annotator = new GithubLinkAnnotator();
annotator.annotate(new GithubUrl("javascript:alert(1); //"), null, null);
}

//Test to verify that fake url are not validated
@Test(expected = IllegalArgumentException.class)
@Issue("SECURITY-3246")
public void urlHtmlAttributeValidationTest() {
GithubLinkAnnotator annotator = new GithubLinkAnnotator();
annotator.annotate(new GithubUrl("a' onclick=alert(777) foo='bar/\n"), null, null);
}

private String annotate(final String originalText, GitChangeSet changeSet) {
MarkupText markupText = new MarkupText(originalText);

Expand Down

0 comments on commit 9e09678

Please sign in to comment.