7
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ I've rolled back Rev 3 → 2, for editing away an issue that was helpfully pointed out in an answer. \$\endgroup\$ – 200_success Nov 16 '15 at 23:22
3
\$\begingroup\$

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);
   }
\$\endgroup\$
  • \$\begingroup\$ Well I never expected a response to this after 3.5 years...that should be worth a Tumbleweed badge. Apologies for the stringVal.IsNullOrEmpty; that is a reference to an extension method, which really isn't appropriate in a code review question unless it is central to the issue at hand. Will fix. Will consider the rest of your answer but I haven't looked at that code in Years. \$\endgroup\$ – Matthew Nichols Nov 16 '15 at 23:04
  • \$\begingroup\$ @MatthewNichols: Just working the zombies :) \$\endgroup\$ – ChrisWue Nov 17 '15 at 8:17

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.