Possible code injection

[mindhaq]

When using autolink on a text including a link like this one

www.google.com"onclick="alert('gotcha!')

And render the output as it is suggested in the example:

String result = Autolink.renderLinks(input, links, (link, text, sb) -> {
    sb.append("<a href=\"");
    sb.append(text, link.getBeginIndex(), link.getEndIndex());
    sb.append("\">");
    sb.append(text, link.getBeginIndex(), link.getEndIndex());
    sb.append("</a>");
});

the output will be

<a href="www.google.com"onclick="alert('gotcha!')">www.google.com"onclick="alert('gotcha!')</a>"

which is strictly speaking invalid HTML, but browsers will still execute the click handler. See https://jsfiddle.net/vLjLLo8n/2/ to try it out.

I understand that appending a subsequence to the StringBuilder is the more efficient than providing the link as a String, but to make this secure, you would need to get the substring and perform encoding on it.

So, for example using OWASP Java Encoder, the rendering needs to be done like this:

String result = Autolink.renderLinks(input, links, (link, text, sb) -> {
    String linkString = new StringBuilder().append(text, link.getBeginIndex(), link.getEndIndex()).toString();

    sb.append("<a href=\"");
    sb.append(Encode.forHtmlAttribute(linkString));
    sb.append("\">");
    sb.append(Encode.forHtml(linkString));
    sb.append("</a>");
});

resulting in a safe output:

<a href="www.google.com&#34;onclick=&#34;alert(&#39;gotcha!&#39;)">www.google.com&#34;onclick=&#34;alert(&#39;gotcha!&#39;)</a>

Easiest fix for this particular problem would probably be if autolink would not include single or double quotes, or any other character not legal in a URL.
(EDIT: single quotes are legal characters)

A possibly breaking API change would be to provide the linkString as part of the LinkSpan interface.

[robinst]

Yeah, I'm fully aware of this, that's why there is a note for that example:

Note that it doesn't handle escaping at all

But maybe I should either:

• Not include this example at all, as it's dangerous if someone copied it like that
• Change the example so that it escapes all parts of text
• Change the example so that it parses the input as HTML and only finds links in text (to prevent double linking)

Note that even your suggested change of using Encode would have problems in the real world, as it wouldn't escape text outside links.

What do you think?

[mindhaq]

First, the URLs really should not include characters not allowed in URLs. According to this post on Stackoverflow double quotes are not legal.

Giving an example in the readme is important, I wouldn't have learned about the AutoLink class otherwise. As a general effort to spread knowledge about important security best practices, it's maybe a good thing to include usage of Encode.

If you are using the library as a tool to create HTML out of some kind of plaintext, that plaintext needs to be sanitized before using it to autolink. That's what I do in my real world application; only during autolinking some of it becomes unwanted code again.

[robinst]

I was following Rinku's behavior in this case. Having said that, I don't think stopping URL's at " would be a problem in practice, so I'm open to changing it. Or did you want to work on that?

I'll update the example when I find some time.

[robinst]

Ok, I've:

• Pushed a change to stop URLs at "
• Created PR Add extractSpans method so that we can deprecate renderLinks (#21) #25 to deprecate renderLinks by making it possible to write the example in a much more straightforward way (and handle escaping correctly for the rest of the text too):

Iterable<Span> spans = linkExtractor.extractSpans(input);

StringBuilder sb = new StringBuilder();
for (Span span : spans) {
    String text = input.substring(span.getBeginIndex(), span.getEndIndex());
    if (span instanceof LinkSpan) {
        // span is a URL
        sb.append("<a href=\"");
        sb.append(Encode.forHtmlAttribute(text));
        sb.append("\">");
        sb.append(Encode.forHtml(text));
        sb.append("</a>");
    } else {
        // span is plain text before/after link
        sb.append(Encode.forHtml(text));
    }
}
result = sb.toString();

[robinst]

@mindhaq nothing? Ok. I'm gonna merge that PR.

[robinst]

I have since released this change as 0.9.0, see CHANGELOG: https://github.com/robinst/autolink-java/blob/master/CHANGELOG.md#090---2018-06-04