Creating a ModelBinder to Sanitize user input in HTML format

Question

I have a custom model binder which takes user input in HTML format, sanitize the HTML input by removing any script or XSS threat and returns the sanitized HTML string:

public class AllowAndSanitizeHtmlBinder : IModelBinder
{

    private HtmlSanitizer _htmlSanitizer = HtmlSanitizerFactory.CreateInstance();

    public object BindModel(ControllerContext controllerContext, ModelBindingContext bindingContext)
    {
        var request = controllerContext.HttpContext.Request;
        var name = bindingContext.ModelName;

        // get the un-validated user input in HTML format
        var unsanitizedMessageHtmlString = request.Unvalidated[name]; 

        // removed script or any XSS threat from user input
        return _htmlSanitizer.Sanitize(unsanitizedMessageHtmlString); 
    }
}

I am using HtmlSanitizer Nuget package to do this. I need to configure this object and tell it to add "nofollow" to all the links.

So I have created HtmlSanitizerFactory to do this configuration:

public static class HtmlSanitizerFactory
{
    public static HtmlSanitizer CreateInstance()
    {
        var sanitizer = new HtmlSanitizer();
        sanitizer.PostProcessNode += (s, e) =>  (e.Node as IHtmlAnchorElement)?.SetAttribute("rel", "nofollow noreferrer");
        return sanitizer;
    }
}

I am not entirely happy with this code. The reason is, in the future, a developer may forget that HtmlSanitizerFactory should be used for instantiating HtmlSanitizer... so he would create his new ModelBinder and initialize the object like:

HtmlSanitizer _htmlSanitizer = new HtmlSanitizer();

This would introduce a bug because links won't be converted to "nofollow"

I think DI would have worked much better here, but I don't know how to inject a class into a custom ModelBinder

dfhwze · Accepted Answer · 2019-08-14 05:36:50Z

Developer gone 'rogue'

I am not entirely happy with this code. The reason is, in the future, a developer may forget that HtmlSanitizerFactory should be used for instantiating HtmlSanitizer... so he would create his new ModelBinder ..

For this to happen, a developer has to dive in your code and deliberately change

private HtmlSanitizer _htmlSanitizer = HtmlSanitizerFactory.CreateInstance();

to

 private HtmlSanitizer _htmlSanitizer = new HtmlSanitizer();

He would probably have a good reason to. And if not, other developers in the team should review and challenge that change. After all, it's best practice to perform code reviews, typically with pull requests.

Test-Driven Development

You have introduced a dependency on a third-party class in your code. Generally, when providing such API code, you would like to reduce the hard dependencies to a minimum. What you could do, is use its interface IHtmlSanitizer instead. This allows for white-bow testing such as

string unsanitizedMessageHtmlString = ".."; // some input
IHtmlSanitizer sanitizer = Mock<IHtmlSanitizer>(); // using some mock method ..
sanitizer.AssertWasCalled(m => m.Sanitize(unsanitizedMessageHtmlString));

You could go a step further and refactor out the third-party dependency entirely by providing your own interface. An adapter class mapping the third-party interface to yours can than be provided in a different library if you wish interop with that API. I am not suggesting this is a must. You should decide which dependencies you want to include in your own API.

Regression

But it is likely that a year from now, we need to developer a new model binder to sanitize a different HTML input. It is at that point that the developer could forget to initialize the sanitizer using the factory.

In the comments you stated you are worried for new developers to use that class in a wrong way, possibly introducing bugs. These bugs should become apparent when providing unit tests. With white-box tests you can check whether PostProcessNode was registered to. And with black-box tests you could easily verify the content of the sanitized string.

It also wouldn't hurt to provide a team Wiki with guidelines, do's and dont's for new developers to get acquainted with the conventions and avoidable regressions.

Dependency Injection

Instead of calling the factory to get an instance of HtmlSanitizer, you should use Dependency Injection. One way to do so is to have the dependency injected through the constructor.

public AllowAndSanitizeHtmlBinder(IHtmlSanitizer htmlSanitizer)
{
    _htmlSanitizer = htmlSanitizer;
}

You would typically have an Ioc container where you register the dependencies. I don't know enough about the IModelBinder's lifetime scope, so you may also want to register these to the container.

container.RegisterSingleInstance<IHtmlSanitizer>(HtmlSanitizerFactory.CreateInstance());

Resource Management

I don't like the third-party class HtmlSanitizer. The events are never cleared and the class does not implement IDisposable. It's a memory leak waiting to occur. You need to find a way to dispose these instances yourself and make sure to unsubscribe from:

 sanitizer.PostProcessNode += (s, e) =>  (e.Node 
     as IHtmlAnchorElement)?.SetAttribute("red", "nofollow noreferrer");

Stack Exchange Network

Creating a ModelBinder to Sanitize user input in HTML format

1 Answer 1

Developer gone 'rogue'

Test-Driven Development

Regression

Dependency Injection

Resource Management

You must log in to answer this question.

Hot Network Questions

Creating a ModelBinder to Sanitize user input in HTML format

1 Answer 1

Developer gone 'rogue'

Test-Driven Development

Regression

Dependency Injection

Resource Management

You must log in to answer this question.

Related

Hot Network Questions