Possible Duplicate:
Links break when when URL includes angle brackets

Observe.

If I write this:

[with T = int](((LinkedList<int>&)(& list2))

verbatim in a SO post, we see this:

[with T = int](((LinkedList&)(& list2))

But in the preview, it looks like this:

&%29%28& list2%29">with T = int

Regardless of whatever else, the %29-ness would seem to be superfluous, no?

share|improve this question
    
What hyperlink are you trying to make with that? –  random Jan 12 '13 at 2:36
    
That's URL encoding. –  Chichiray Jan 12 '13 at 2:36
    
@random: No hyperlink whatsoever. The bug is that it looks like a hyperlink afterwards, sort of. Admittedly, and I have only just noticed this, the input resembles Markdown link syntax. I still wouldn't expect the literal &29&28, though -- either a link that I didn't expect, or no change. –  Lightness Races in Orbit Jan 12 '13 at 3:25
    
To downvoter -- if it makes it clearer, that "it looks like this" is the verbatim output in the preview window. It's not resultant rendered source. There is no reason for that to be the output. Try it yourself. –  Lightness Races in Orbit Jan 12 '13 at 3:27
1  
The issue is the same as in the duplicate, but the solution in your case is just marking code as code, with the additional advantage that it's much more readable. –  balpha Jan 12 '13 at 10:21
    
@balpha: Sure - it was only when a newbie had posted the text outside of code that I spotted this rendering artefact. I did of course codefy it as well as posting this :) –  Lightness Races in Orbit Jan 12 '13 at 16:07
    
And you're right, it's an inconsistency between preview and final version, but since both versions are broken (just in different ways), it's not a major one. –  balpha Jan 12 '13 at 16:13
add comment

marked as duplicate by Asad, hims056, balpha Jan 12 '13 at 10:10

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

1 Answer

Problem:

The HTML sanitizer used for the markdown preview is pretty rudimentary. The initial output from your markdown (after URL encoding) is this:

<p>
    <a href="%28%28LinkedList<int>&amp;%29%28&amp; list2%29">
        with T = int
    </a>
</p>

This is what you would expect as the output, but the sanitizer then tries to whitelist tags, by running the output past this regex:

function sanitizeHtml(html) {
    return html.replace(/<[^>]*>?/gi, sanitizeTag);
}

// Here is the replacement callback

function sanitizeTag(tag) {
    if (tag.match(basic_tag_whitelist) || tag.match(a_white) || tag.match(img_white))
        return tag;
    else
        return "";
} 

Since the string <a href="%28%28LinkedList<int> matches <[^>]*>, it gets lumped together as a single tag and removed, leaving the rest of the attribute and the closing angle bracket for <a... > in limbo. So far we have:

<p>
    &amp;%29%28&amp; list2%29">
        with T = int
    </a>
</p>

This is a paragraph containing the last three quarters of a partially decapitated anchor element. The parser now tries to balance tags, so it removes the headless </a>:

<p>
    &amp;%29%28&amp; list2%29">with T = int
</p>

This is the output you see.

Solution:

To work with the existing setup, here is a much more bulletproof regex that I found in this article:

/<\/?\w+((\s+\w+(\s*=\s*(?:".*?"|'.*?'|[^'">\s]+))?)+\s*|\s*)\/?>/gi

When using this regex, the opening anchor tag is correctly and completely identified. It still doesn't match the whitelist (the href doesn't begin with a protocol) so it is removed completely, leaving:

<p>
    with T = int</a>
</p>

Once again, the tag balancing kicks in, which leaves you with:

<p>
    with T = int
</p>

This is much nicer, and I assume would be the expected behavior for links with invalid URLs.

Ideally, though...

share|improve this answer
    
You're missing the point here. The sanitizer is not parsing HTML, and using your replacement would be the worst we could do. The sanitizer is matching anything that could possibly be understood as an HTML tag by a browser and checks these results agains a very narrow whitelist. Anything that does not match this whitelist is removed. Your suggestion opens a huge security hole on our site (e.g. <script ignore-me>doEvil()</script ignore-me> is suddenly allowed, and the browser happily runs that.). Your explanation of the OP's issue is correct, but your proposed solution is absolutely wrong. –  balpha Jan 12 '13 at 10:09
    
@balpha The sanitiser is trying to identify the fragments of HTML that the whitelist is supposed to operate on, right? The problem is that it is identifying a piece of string that is a non-tag as a tag, because it is misinterpreting a closing angle bracket in an attribute as the end of the tag. The snippet you are proposing is not actually allowed with the suggested regex, it outputs "<p>doEvil()</p>", stripping the script tags since they aren't on the whitelist. The regex I have suggested actually does cover anything that could possibly be interpreted as a HTML tag. –  Asad Jan 12 '13 at 12:51
    
@balpha Maybe I wasn't clear about the fact that I was suggesting replacing the first regex (the one that identifies tags to test), not replacing the actual whitelist(s). –  Asad Jan 12 '13 at 12:55
    
I did understand this. Your regex does let my example through, because it fails to match it as something that should be checked against the whitelist in the first place, and thus leaves it untouched. The short and simple (in your words, "rudimentary") regex is absolutely correct, and so is removing the characters in question here (whether they should appear to begin with is a different question, but unrelated to the sanitizer). Replacing the regex by anything that matches less than what the current version does is a huge XSS vulnerability. –  balpha Jan 12 '13 at 13:29
    
@balpha Ah yes I see what you mean now, but what about parsing attributes when parsable, and reverting to the loose definition when that fails. Something like: /<\/?\w+(((\s+\w+?(\s*=\s*(?:".*?"|'.*?'|[^'">\s]+))?)+\s*|\s*)|([^>]*))\/?>/gi . Note the |([^>]*) –  Asad Jan 12 '13 at 13:49
    
@balpha If it fails to find pattern for attributes, it will backtrack and settle for the first > that it spots. –  Asad Jan 12 '13 at 13:57
    
Which is still vulnerable; the ? after the closing angle bracket is important. But even if fixing that (and assuming there isn't anything else that either of us missed), what's the point? There is no gain from this (in your answer, you notice yourself that with your version you don't get the intended result either; it's just a different kind of broken). But for no gain, there is a huge cost in your proposal: You're making our core defense against cross-site scripting more complicated and thus harder to maintain. –  balpha Jan 12 '13 at 14:09
    
In the context of security, simplicity is key. Reasoning about regexes is hard enough (as evidenced here), and every character added to this regex increases the chance of missing something and thus opening up a hole. And again, the gain you're getting from it is minimal at best. In my opinion it's actually zero, but in either case it's not worth sacrificing our security for it. –  balpha Jan 12 '13 at 14:09
    
@balpha The reason I was suggesting this was because it fixes the bug mentioned in the dupe question. It isn't really a different kind of broken; provided your URL is a valid one and happens to contain angle brackets, your link should get linkified correctly. You have a good point about the maintenance difficulty though, especially since this is such a minor nitpick. –  Asad Jan 12 '13 at 14:13
    
The solution here is for the OP to mark up his code as code. Trying to "fix" our parser to somehow allow both limited HTML, Markdown and arbitrary code that might look like HTML or Markdown is a fool's errand. –  Shog9 Jan 12 '13 at 16:00
    
To be clear, it's not my code and I did mark it as code at the same time as I posted this bug report. I just thought it worth bringing up even if the resolution is WONTFIX –  Lightness Races in Orbit Jan 12 '13 at 16:09
    
@Shog9 Like I've said before, the reason this is a bug isn't because it breaks on arbitrary code that resembles the markdown for links, but because it breaks on perfectly valid links like the one in the dupe. There is probably a better way to do it than my suggestion, but as balpha has pointed out, this is a very localised issue and isn't worth the time it would require. –  Asad Jan 12 '13 at 17:07
    
@Asad: there are "perfectly valid links" that aren't even valid (unencoded) in HTML - much less in Markdown. You can't look at these things in a vacuum. –  Shog9 Jan 12 '13 at 17:23
    
Hey @balpha, sorry to bug you again; just an idea, couldn't you add a post conversion hook that encodes angle brackets in attributes before sanitising? That fixes the problem and leaves the sanitiser alone. –  Asad Jan 14 '13 at 12:54
    
@balpha Totally know you have more important stuff to work on, I'm just bored. :P –  Asad Jan 14 '13 at 13:03
add comment

Not the answer you're looking for? Browse other questions tagged .