Chris Jester-Young friends

[C#] On Strip Html Comments

2009-11-16T04:25:38-08:00

That "invalid HTML tag" case is really tough, a real edge condition. If you discard that one, it's pretty easy. I'd recommend doing a first pass to remove all invalid HTML tags, first. Like.. er.. <Ã§123>.

    public static string StripHtmlComments(string html)
    {
        if (html == null)
        {
            throw new ArgumentNullException("html");
        }

        if (html.IndexOf("<!", StringComparison.Ordinal) < 0)
        {
            return html;
        }

        return Regex.Replace(html, "(?<!='|=\"|=)<![^>]+>", "");
    }

[C#] On Balance HTML Tags

2008-07-30T20:45:34-07:00

> I would worry that you're using the wrong tool for the job, and focusing too much on micro performance increases.

Well, to play Devil's Advocate: I would worry that you're writing far too much code, and overengineering a solution with too many complex, unnecessary external dependencies. :)

[C#] On Balance HTML Tags

2008-07-30T20:43:13-07:00

> Tags will not get balanced if 'ignore' tags and regular tags are incorrectly nested.

This is by design; I don't view incorrect nesting as a problem that would prevent page rendering, like an unclosed <div> or <td> would. Am I wrong?

[C#] On Balance HTML Tags

2008-07-13T07:37:40-07:00

It doesn't break Corbin's code, it breaks mine. The point was that a single concatenation is fairly cheap, to the tune of 15ms over 4,000 iterations.

In general, yes, concatenation is expensive. But one per loop isn't really significant in my benchmarking. As to why the List<>.Contains is 25% slower than String.Contains, I dunno. But it is..

[C#] On Balance HTML Tags

2008-07-12T00:46:05-07:00

> Another problem is the concatenation ("<" + tagname). Doing that every iteration adds up, too.

The difference is fairly small.. even if I (erroneously) use just the tagname, no concatenation, improvement is marginal. Result of 4,000 iterations:

tagname (broken!!)

543 ms
546 ms
534 ms

"<" + tagname

569 ms
558
551

String.Concat("<", tagname)

581 ms
555
551

// ** DO NOT USE **
// THIS IS BAD BROKEN CODE, ONLY DISPLAYED FOR BENCHMARKING PURPOSES!
if (!tagpaired[i] && !ignoredtags.Contains(tagname))

// as coded
if (!tagpaired[i] && !ignoredtags.Contains("<" + tagname))

// alternate
if (!tagpaired[i] && !ignoredtags.Contains(String.Concat("<", tagname)))

[C#] On Balance HTML Tags

2008-07-12T00:39:12-07:00

> I would think that a list's Contains method should be at least as fast to evaluate, depending on the algorithm that String.Contains uses.

And yet it is not. Result of 4,000 iterations:

use List<string> for ignored tags

650 ms
641
635

use String.Contains for ignored tags

585 ms
565
549

var ignoredtags = new List<String> { "p", "img", "br" };

if (!tagpaired[i] && !ignoredtags.Contains(tagname))
{

[C#] On Balance HTML Tags

2008-07-12T00:30:52-07:00

Wow, Corbin, thank you. That's really nice -- it's even faster, and does more! Testing with 4,000 iterations on medium sized input:

my BalanceTags:

569 ms
549
549

your (improved) BalanceTags

566 ms
537
524

Ah, yes, a List of Matches eg List<Match> ! Why didn't I think of that..

[C#] On Balance HTML Tags

2008-07-11T10:44:23-07:00

> What's wrong with a list or set here?

Much, much, MUCH slower in my benchmarking. The simple String.Contains() test is extremely fast to evaluate, which is an issue when we're calling it in a loop.

> Example that actually fixes up the tags.

The agility pack is 4x slower than this routine in my testing -- result of 4,000 runs on medium sized input:

Regex based Balance
563
560
555

HtmlAgilityPack balance
2048
1990
1974

Also, HtmlAgilityPack may fix up orphaned <begin> tags but it strips out orphaned </end> tags just like I do..

I need to add the <li> elements to the ignore list as well; those don't need to be closed. I know I never close mine..

[C#] On Sanitize HTML

2008-07-11T10:28:06-07:00

> your overall approach is more like a blacklist: look inside this string for "bad" stuff and erase it

I see what you mean vs. HTMLEncoding() the whole thing, and then writing regular expressions to replace the escaped whitelisted entities with valid HTML. I just never thought of it this way.

The other advantage of that approach is that content is not deleted.

[C#] On Balance HTML Tags

2008-07-11T09:25:44-07:00

> p isn't self closing

Well, it should be. :) We have a lot of data with unclosed <p> tags for testing. Also, since I am using the ban-hammer of REMOVAL, it's not a fun tag to make mistakes with for people who enter a lot of unclosed <p> tags and have JavaScript disabled, so they can input anything in our editor textbox.

> iterating over a list in reverse is plain ugly (because you can't use 'foreach').

That's necessary so we can continue to manipulate the string while the size is changing -- otherwise it's a PITA when we go forward; the original tag match locations downstream of each replace are no longer valid and must be adjusted with an offset for every deletion.

[C#] Balance HTML Tags

2008-07-11T08:40:17-07:00

For the subset of HTML tags we care about, this function ensures that all tags in the input HTML are balanced (but not correctly nested, necessarily) -- it does this by *removing* any extra opening or closing tags it finds in the HTML string. Note: this routine is NOT designed to be a XSS sanitizer! It assumes it is running on safe, pre-sanitized HTML.

UPDATED: 2009-11-15 small bugfix with nested tags; 30% performance improvement

private static Regex _namedtags = new Regex
    (@"</?(?<tagname>\w+)[^>]*(\s|$|>)",
    RegexOptions.Singleline | RegexOptions.ExplicitCapture | RegexOptions.Compiled);

/// <summary>
/// attempt to balance HTML tags in the html string
/// by removing any unmatched opening or closing tags
/// IMPORTANT: we *assume* HTML has *already* been 
/// sanitized and is safe/sane before balancing!
/// 
/// CODESNIPPET: A8591DBA-D1D3-11DE-947C-BA5556D89593
/// </summary>
public static string BalanceTags(string html)
{
    if (String.IsNullOrEmpty(html)) return html;

    // convert everything to lower case; this makes
    // our case insensitive comparisons easier
    MatchCollection tags = _namedtags.Matches(html.ToLowerInvariant());

    // no HTML tags present? nothing to do; exit now
    int tagcount = tags.Count;
    if (tagcount == 0) return html;

    string tagname;
    string tag;
    const string ignoredtags = "<p><img><br><li><hr>";
    int match;
    var tagpaired = new bool[tagcount];
    var tagremove = new bool[tagcount];

    // loop through matched tags in forward order
    for (int ctag = 0; ctag < tagcount; ctag++)
    {
        tagname = tags[ctag].Groups["tagname"].Value;

        // skip any already paired tags
        // and skip tags in our ignore list; assume they're self-closed
        if (tagpaired[ctag] || ignoredtags.Contains("<" + tagname + ">"))
            continue;

        tag = tags[ctag].Value;
        match = -1;

        if (tag.StartsWith("</"))
        {
            // this is a closing tag
            // search backwards (previous tags), look for opening tags
            for (int ptag = ctag - 1; ptag >= 0; ptag--)
            {
                string prevtag = tags[ptag].Value;
                if (!tagpaired[ptag] && prevtag.Equals("<" + tagname, StringComparison.InvariantCulture))
                {
                    // minor optimization; we do a simple possibly incorrect match above
                    // the start tag must be <tag> or <tag{space} to match
                    if (prevtag.StartsWith("<" + tagname + ">") || prevtag.StartsWith("<" + tagname + " "))
                    {
                        match = ptag;
                        break;
                    }
                }
            }
        }
        else
        {
            // this is an opening tag
            // search forwards (next tags), look for closing tags
            for (int ntag = ctag + 1; ntag < tagcount; ntag++)
            {
                if (!tagpaired[ntag] && tags[ntag].Value.Equals("</" + tagname + ">", StringComparison.InvariantCulture))
                {
                    match = ntag;
                    break;
                }
            }
        }

        // we tried, regardless, if we got this far
        tagpaired[ctag] = true;
        if (match == -1)
            tagremove[ctag] = true; // mark for removal
        else
            tagpaired[match] = true; // mark paired
    }

    // loop through tags again, this time in reverse order
    // so we can safely delete all orphaned tags from the string
    for (int ctag = tagcount - 1; ctag >= 0; ctag--)
    {
        if (tagremove[ctag])
        {
            html = html.Remove(tags[ctag].Index, tags[ctag].Length);
            System.Diagnostics.Debug.WriteLine("unbalanced tag removed: " + tags[ctag]);
        }
    }

    return html;
}

[C#] On Sanitize HTML

2008-06-22T02:29:47-07:00

> I'm just guessing; since you write C#, I can't actually run your code. I'm supposing something like this will happen:

Well, you don't need C#, you could use a regex testing tool of your choice to see what actually happens. Remember, our intial tag match is "<[^>]*?(>|$)"

"<foo<bar><scr<bar>ipt/>"

remove first <foo<bar> ==> "<scr<bar>ipt/>"
remove first <scr<bar> ==> "ipt/>"

resulting string is "ipt/>".

(and anyway, "first match" is moot; I added 2 lines of code so it actually replaces the correct location for each match now, to prevent any possibility of unwanted side-effect IndexOf() matches.)

[C#] On Sanitize HTML

2008-06-21T16:35:49-07:00

> but what if somebody copies and pastes from the web

Nick, I don't think that's the use case here.

1) If you are pasting a code sample it'll be inside a <pre><code> block (in the WMD editor, highlight the code snippet and use CTRL+K or press the "code" toolbar button) and thus fully escaped, so it'll appear as-is.

2) If you are composing question or answer text, you'd probably paste from the textual content of the web page, not the view source.

> By replacing the first match you introduce another class of (more subtle) vulnerabilities

You're implying that a later bad tag would somehow appear earlier in the string as a substring, but I cannot for the life of me come up with an example of that working. If a tag is contained within another tag, it'll get stripped by the new tag match that uses either > or $ (end of text) as the end match.

Can you provide an example of this that works, given the new tag match regex? I can't come up with one.

[C#] On Sanitize HTML

2008-06-21T04:48:35-07:00

Hi Chris -- those are both *excellent* points, and I verified both. Working on it.. the second one is fairly easy (just replace the one instance) but the unclosed tag thing is rough. Suggestions?

[C#] Sanitize HTML

2008-06-20T08:24:46-07:00

Takes a provided HTML string and removes any potentially dangerous XSS HTML tags using a whitelist approach. Useful when you want to allow a small subset of "safe" HTML tags in user content.

UPDATED: July 11th to reflect all refactorings, plus optimizing for speed. Now 2x faster!
UPDATED: Sept 1st, bugfixes
UPDATED: May 14th, simplify and improve whitelist strictness

private static Regex _tags = new Regex("<[^>]*(>|$)",
    RegexOptions.Singleline | RegexOptions.ExplicitCapture | RegexOptions.Compiled);
private static Regex _whitelist = new Regex(@"
    ^</?(b(lockquote)?|code|d(d|t|l|el)|em|h(1|2|3)|i|kbd|li|ol|p(re)?|s(ub|up|trong|trike)?|ul)>$|
    ^<(b|h)r\s?/?>$",
    RegexOptions.Singleline | RegexOptions.ExplicitCapture | RegexOptions.Compiled | RegexOptions.IgnorePatternWhitespace);
private static Regex _whitelist_a = new Regex(@"
    ^<a\s
    href=""(\#\d+|(https?|ftp)://[-a-z0-9+&@#/%?=~_|!:,.;\(\)]+)""
    (\stitle=""[^""<>]+"")?\s?>$|
    ^</a>$",
    RegexOptions.Singleline | RegexOptions.ExplicitCapture | RegexOptions.Compiled | RegexOptions.IgnorePatternWhitespace);
private static Regex _whitelist_img = new Regex(@"
    ^<img\s
    src=""https?://[-a-z0-9+&@#/%?=~_|!:,.;\(\)]+""
    (\swidth=""\d{1,3}"")?
    (\sheight=""\d{1,3}"")?
    (\salt=""[^""<>]*"")?
    (\stitle=""[^""<>]*"")?
    \s?/?>$",
    RegexOptions.Singleline | RegexOptions.ExplicitCapture | RegexOptions.Compiled | RegexOptions.IgnorePatternWhitespace);


/// <summary>
/// sanitize any potentially dangerous tags from the provided raw HTML input using 
/// a whitelist based approach, leaving the "safe" HTML tags
/// CODESNIPPET:4100A61A-1711-4366-B0B0-144D1179A937
/// </summary>
public static string Sanitize(string html)
{
    if (String.IsNullOrEmpty(html)) return html;

    string tagname;
    Match tag;

    // match every HTML tag in the input
    MatchCollection tags = _tags.Matches(html);
    for (int i = tags.Count - 1; i > -1; i--)
    {
        tag = tags[i];
        tagname = tag.Value.ToLowerInvariant();
        
        if(!(_whitelist.IsMatch(tagname) || _whitelist_a.IsMatch(tagname) || _whitelist_img.IsMatch(tagname)))
        {
            html = html.Remove(tag.Index, tag.Length);
            System.Diagnostics.Debug.WriteLine("tag sanitized: " + tagname);
        }
    }

    return html;
}