ASP.NET MVC 3 ModelBinder with string sanitizing

Question

I need to allow incoming HTML in string parameters in my projects action methods, so we have disabled Input Validation. I have a good HTML sanitizer; the review I am interested in is the way I bound it into my project.

I have the following Model Binder:

    public class EIMBaseModelBinder : DefaultModelBinder
    {
        public override object BindModel(ControllerContext controllerContext, ModelBindingContext bindingContext)
        {
            var boundValue = base.BindModel(controllerContext, bindingContext);
            return bindingContext.ModelType == typeof(string) ? HtmlCleaner.SanitizeHtml((string)boundValue) : boundValue;
        }

        protected override void SetProperty(ControllerContext controllerContext, ModelBindingContext bindingContext, PropertyDescriptor propertyDescriptor, object value)
       {
            if (propertyDescriptor.PropertyType == typeof(string))
            {
              var stringVal = value as string;
              value = stringVal.IsNullOrEmpty() ? null : HtmlCleaner.SanitizeHtml(stringVal);
            }

            base.SetProperty(controllerContext, bindingContext, propertyDescriptor, value);
       }
    }

I set it as my DefaultBinder in my set up and require that all custom model binders inherit from it. I know I can't completely defend against developers not following this rule, but we are a small team so I think we can police that well enough.

I have some basic unit testing pushing both string primitive values and strings as property values through the binder and those work as expected. I will be asking the security team to do some penetration tests.

Can anyone see either a better way to have hooked into the incoming data or a base I have missed?

Answer 1

Can't comment on whether there is a better way or a better place in the framework to do this but some general remarks:

1. This doesn't make much sense: stringVal.IsNullOrEmpty() - should be string.IsNullOrEmpty(stringVal).

2. You are inconsistent: In the first method you check the type and use a direct cast vs in the second method you check the type and then use as - either you trust the type is correct or you don't. Also in SetProperty you try to not pass null or empty strings to the sanitizer helper while in BindModel you don't.

3. It would make sense if the helper method would support null and empty strings.

With the above the cleaned up code for SetProperty should probably look like this:

   protected override void SetProperty(ControllerContext controllerContext, ModelBindingContext bindingContext, PropertyDescriptor propertyDescriptor, object value)
   {
        if (propertyDescriptor.PropertyType == typeof(string))
        {
            value = HtmlCleaner.SanitizeHtml((string)value);
        }

        base.SetProperty(controllerContext, bindingContext, propertyDescriptor, value);
   }