5
\$\begingroup\$

I have a system that accepts free text from users. This can either be plain text or a heavily limited subset of HTML. I've written the below with Html Agility Pack and Microsoft's AntiXss library. I have had to remove a few things from the below code which do things like whitespace normalization, punctuation normalization etc. as I know they're already fine.

There's some additional functionality which aims to delete some common html patterns (from pasting from MS Word) which include pointlessly nested spans (<span><span><span>A word</span></span></span>) and <p><br></p>.

Expected use case:

HtmlUtility.SanitizeResponse("<p>An html response with bad stuff <script> EVIL SCRIPT </script> </p>")
// <p>An html response with bad stuff EVIL SCRIPT  </p>

I want to retain as much of the original text as I can.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;
using HtmlAgilityPack;
using Encoder = Microsoft.Security.Application.Encoder;

public static class HtmlUtility
{
    public static HashSet<string> WhiteSpaceSignificantAttributes = new HashSet<string>
            { "class", "alt", "title", "style" };

    private static readonly Dictionary<string, string[]> AllowedResponseHtml =
        new Dictionary<string, string[]>
        {
            ["p"] = new[] { "class" },
            ["a"] = new[] { "href", "title", "class" },
            ["strong"] = new string[0],
            ["em"] = new string[0],
            ["span"] = new string[0],
            ["br"] = new string[0]
        };

    private static readonly HashSet<string> BlockElements = new HashSet<string>(StringComparer.OrdinalIgnoreCase)
            {
                "address", "article", "aside", "blockquote", "canvas", "dd", "div", "dl", "fieldset", "figcaption",
                "figure", "footer", "form", "h1", "h2", "h3", "h4", "h5", "h6", "header", "hgroup", "hr", "main",
                "nav", "noscript", "ol", "output", "p", "pre", "section", "table", "tfoot", "ul", "video"
            };

    private static readonly HashSet<string> FlowContentElements = new HashSet<string>(StringComparer.OrdinalIgnoreCase)
            {
                "a", "abbr", "address", "article", "aside", "audio", "b", "bdo", "bdi", "blockquote", "br", "button",
                "canvas", "cite", "code", "command", "data", "datalist", "del", "details", "dfn", "div", "dl", "em",
                "embed", "fieldset", "figure", "footer", "form", "h1", "h2", "h3", "h4", "h5", "h6", "header", "hgroup",
                "hr", "i", "iframe", "img", "input", "ins", "kbd", "keygen", "label", "main", "map", "mark", "math",
                "menu", "meter", "nav", "noscript", "object", "ol", "output", "p", "pre", "progress", "q", "ruby", "s",
                "samp", "script", "section", "select", "small", "span", "strong", "sub", "sup", "svg", "table", "template",
                "textarea", "time", "ul", "var", "video", "wbr", "text"
            };


    private static void RemoveNonPoImages(HtmlNode rootNode)
    {
        var nonPoImages = rootNode.Descendants("img").Where(
            e =>
            {
                var src = e.GetAttributeValue("src", string.Empty).ToLowerInvariant();
                return src == string.Empty || !src.StartsWith("/resources/");
            }).ToList();

        for (var i = nonPoImages.Count - 1; i >= 0; i--)
        {
            nonPoImages[i].ParentNode.RemoveChild(nonPoImages[i]);
        }
    }

    private static void RemoveNonWhitelistedAttributes(
        HtmlNode rootNode,
        bool allowUrls,
        IDictionary<string, string[]> allowedHtml)
    {
        foreach (var tag in allowedHtml)
        {
            var validTag = tag;

            var attributes =
                rootNode.DescendantsAndSelf()
                    .Where(n => n.Name == validTag.Key)
                    .Where(n => n.HasAttributes)
                    .SelectMany(n => n.Attributes.ToArray());

            foreach (var attribute in attributes)
            {
                if (!validTag.Value.Contains(attribute.Name))
                {
                    attribute.Remove();
                    continue;
                }
                if (attribute.Name == "href" || attribute.Name == "src")
                {
                    // remove anything we dislike: NB this does remain inevitably insecure
                    var unacceptables = new[] { "script", "<", ">", "'", "\"" };

                    if (!allowUrls
                        || (attribute.Value != null && unacceptables.Any(u => attribute.Value.Contains(u))))
                    {
                        attribute.Remove();
                    }
                    else
                    {
                        attribute.Value = Encoder.HtmlAttributeEncode(attribute.Value);
                    }
                }
                else if (WhiteSpaceSignificantAttributes.Contains(attribute.Name))
                {
                    // Allow spaces but still encode each word.
                    attribute.Value =
                        string.Join(" ", (attribute.Value ?? string.Empty).Split(
                            new[] { ' ' },
                            StringSplitOptions.RemoveEmptyEntries).Select(Encoder.HtmlAttributeEncode));
                }
                else
                {
                    attribute.Value = Encoder.HtmlAttributeEncode(attribute.Value);
                }
            }
        }
    }

    public static HtmlDocument GetHtml(string source)
    {
        var html = new HtmlDocument
        {
            OptionFixNestedTags = true,
            OptionAutoCloseOnEnd = true,
            OptionDefaultStreamEncoding = Encoding.UTF8
        };

        html.LoadHtml(source);

        return html;
    }


    private static bool HtmlIsJustText(HtmlNode rootNode)
    {
        return rootNode == null || rootNode.Descendants().All(n => n.NodeType == HtmlNodeType.Text);
    }

    public static string SanitizeResponse(string response)
    {
        // temporarily change flags so we render 'proper' empty <p></p> tags
        HtmlNode.ElementsFlags["p"] = HtmlElementFlag.Closed;

        var rootNode = GetRootNode(NormalizeResponseBody(response));
        if (HtmlIsJustText(rootNode))
        {
            return response;
        }

        var whitelist = AllowedResponseHtml;

        // Omitted some code which can add additional things to the whitelist

        RemoveLinebreakingParagraphs(rootNode);
        ConvertBoldToStrong(rootNode);
        ConvertItalicToEm(rootNode);
        ReplaceNonWhitelistedNodes(rootNode, whitelist.Select(kvp => kvp.Key).ToArray());
        RemoveNonWhitelistedAttributes(rootNode, true, whitelist);
        RemoveNonPoImages(rootNode);
        RemoveSuperfluousSpans(rootNode);
        var temp = rootNode.InnerHtml;

        // restore flags
        HtmlNode.ElementsFlags["p"] = HtmlElementFlag.Empty | HtmlElementFlag.Closed;

        return temp;
    }

    private static void RemoveSuperfluousSpans(HtmlNode rootNode)
    {
        if (rootNode.HasChildNodes)
        {
            for (var i = rootNode.ChildNodes.Count - 1; i >= 0; i--)
            {
                RemoveSuperfluousSpans(rootNode.ChildNodes[i]);
            }
        }

        // Remove element if it is a span with only text nodes or a single span as a child
        if (rootNode.Name == "span" && (rootNode.ChildNodes.All(n => n.NodeType == HtmlNodeType.Text) || (rootNode.ChildNodes.Count == 1 && rootNode.ChildNodes.All(n => n.Name == "span"))))
        {
            rootNode.ParentNode.ReplaceChild(HtmlNode.CreateNode(rootNode.InnerHtml), rootNode);
        }
    }

    private static void RemoveLinebreakingParagraphs(HtmlNode rootNode)
    {
        var badLineBreaks =
            rootNode.Descendants("p")
                .Where(
                    n =>
                    n.ChildNodes.All(
                        c =>
                        (c.NodeType == HtmlNodeType.Text && string.IsNullOrWhiteSpace(c.InnerText))
                        || c.Name == "br"))
                .ToList();

        for (var i = badLineBreaks.Count - 1; i >= 0; i--)
        {
            var node = badLineBreaks[i];
            node.ParentNode.RemoveChild(node);
        }
    }

    private static string NormalizeResponseBody(string response)
    {
        if (response == null)
        {
            throw new ArgumentNullException(nameof(response));
        }
        response = response.Replace("&nbsp;", " ");
        return Regex.Replace(response, "\\s+", " ");
    }

    private static void ReplaceNonWhitelistedNodes(HtmlNode node, string[] whitelist)
    {
        if (node.HasChildNodes)
        {
            ReplaceChildren(node, whitelist);
        }

        if (node.NodeType != HtmlNodeType.Element || whitelist.Contains(node.Name))
        {
            return;
        }
        // Can use inner html as children already sanitized
        var replacement = BlockElements.Contains(node.Name)
                            && FlowContentElements.Contains(node.ParentNode.Name)
                                ? HtmlNode.CreateNode($"<p>{node.InnerHtml}</p>")
                                : HtmlNode.CreateNode($"<span>{node.InnerHtml}</span>");

        node.ParentNode.ReplaceChild(replacement, node);
    }

    private static void ReplaceChildren(HtmlNode parent, string[] whitelist)
    {
        for (var i = parent.ChildNodes.Count - 1; i >= 0; i--)
        {
            ReplaceNonWhitelistedNodes(parent.ChildNodes[i], whitelist);
        }
    }

    private static void ConvertItalicToEm(HtmlNode rootNode)
    {
        var italics = rootNode.Descendants("i").ToList();
        for (var i = italics.Count - 1; i >= 0; i--)
        {
            var italic = italics[i];
            var newNode = HtmlNode.CreateNode($"<em>{italic.InnerText}</em>");
            italic.ParentNode.ReplaceChild(newNode, italic);
        }
    }

    private static void ConvertBoldToStrong(HtmlNode rootNode)
    {
        var bolds = rootNode.Descendants("b").ToList();
        for (var i = bolds.Count - 1; i >= 0; i--)
        {
            var bold = bolds[i];
            var newNode = HtmlNode.CreateNode($"<strong>{bold.InnerText}</strong>");
            bold.ParentNode.ReplaceChild(newNode, bold);
        }
    }

    private static HtmlNode GetRootNode(string response)
    {
        var html = GetHtml(response);

        var rootNode = html?.DocumentNode;
        return rootNode;
    }
}

All of the code feels really ugly to me, so I'd like some suggestions for improvements in overall approach and my coding style.

\$\endgroup\$
2
\$\begingroup\$

Magic String/Lists

Personally I'd reduce away those lists of strings that're really just configuration settings. e.g. FlowContentElements could become something like this:

private static List<string> FlowContentElements
{
    get
    {
        if (FlowContentElementsCache == null)
        {
            // Load list from xml or some source here.
        }

        return FlowContentElementsCache;
    }
}
private static List<string> FlowContentElementsCache;

This way on first use it'll cache the list and then from there on out you will have the list already loaded. This way if you were to ever change what is white-listed or want to use this in another project it's already setup to be re-configured.

Pointless variables

In many locations you have something like this: var validTag = tag; where you don't actually need the variable. The places validTag and tag are used are as follows:

var attributes = rootNode
    .DescendantsAndSelf()
    .Where(n => n.Name == validTag.Key)
    .Where(n => n.HasAttributes)
    .SelectMany(n => n.Attributes.ToArray());

if (!validTag.Value.Contains(attribute.Name))
{
    attribute.Remove();
    continue;
}

Neither of which need the variable re-declared. So you very much could just use tag.

var attributes = rootNode
    .DescendantsAndSelf()
    .Where(n => n.Name == tag.Key)
    .Where(n => n.HasAttributes)
    .SelectMany(n => n.Attributes.ToArray());

if (!tag.Value.Contains(attribute.Name))
{
    attribute.Remove();
    continue;
}

Again,

private static HtmlNode GetRootNode(string response)
{
    var html = GetHtml(response);

    var rootNode = html?.DocumentNode;
    return rootNode;
}

could be

private static HtmlNode GetRootNode(string response)
{
    var html = GetHtml(response);
    return html?.DocumentNode;
}

If you are using the variables for debugging purposes you can type "result" into the watch window and it'll display a result (reference here, VS2013+).

Simplify

if (rootNode.Name == "span" && (rootNode.ChildNodes.All(n => n.NodeType == HtmlNodeType.Text) || (rootNode.ChildNodes.Count == 1 && rootNode.ChildNodes.All(n => n.Name == "span"))))
{
    // code from `RemoveSuperfluousSpans`
}

Any way you format this, it's ugly. Break these large or even just hard to read boolean expressions down to much more managable and readable expressions like this: (naming might be wrong but you get the point)

private static bool IsSpan(HtmlNode node)
{
    return node.Name == "span";
}

private static bool IsSuperfuousSpan(HtmlNode rootNode)
{
    return HasOnlyTextChildren(rootNode) || HasOnlyASpanChild(rootNode);
}

private static bool HasOnlyTextChildren(HtmlNode node)
{
    return node.ChildNodes.All(n => n.NodeType == HtmlNodeType.Text);
}

private static bool HasOnlyASpanChild(HtmlNode node)
{
    return node.ChildNodes.Count == 1 && node.ChildNodes.All(n => n.Name == "span");
}

Then your if-statement becomes:

if (IsSpan(rootNode) && IsSuperfuousSpan(rootNode))
{
    // code from `RemoveSuperfluousSpans`
}
\$\endgroup\$
  • \$\begingroup\$ Thanks - definitely agree with your point about the confusing if statements and pointless variables... I prefer the hard coded lists over configuration because it's a content category defined elsewhere so won't be changing. \$\endgroup\$ – RobH Aug 27 '15 at 13:32
  • \$\begingroup\$ I'm lazy so I ironicly lean towards making it easy for me to do things in the future which is the reason for the first suggestion haha. \$\endgroup\$ – Shelby115 Aug 27 '15 at 13:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.