NullReferenceException with invalid style attribute due to incorrect null check in HtmlSanitizer.SanitizeStyle

[LunNova]

When attempting to sanitize <font style="t"></font> an exception is thrown:

System.NullReferenceException: Object reference not set to an instance of an object.
at AngleSharp.Extensions.FormatExtensions.ToCss(IStyleFormattable style, IStyleFormatter formatter)
at Ganss.XSS.HtmlSanitizer.SanitizeStyle(IHtmlElement element, String baseUrl)
at Ganss.XSS.HtmlSanitizer.DoSanitize(IHtmlDocument dom, IElement context, String baseUrl, IMarkupFormatter outputFormatter)

In SanitizeStyle, the following null check is used to guard against calling ToCss on a null ICssStyleDeclaration:

      if (element.GetAttribute("style") == null)
        return;
      element.SetAttribute("style", element.Style.ToCss());

This is not correct, as the getter of Style in element calls CreateStyle. It may return null even if the style attribute is non-null.

Here is the implementation of CreateStyle:

    protected ICssStyleDeclaration CreateStyle()
    {
      if ((this.Flags & (NodeFlags.HtmlMember | NodeFlags.SvgMember)) != NodeFlags.None)
      {
        Document owner = this.Owner;
        IConfiguration options1 = owner.Options;
        IBrowsingContext context = owner.Context;
        ICssStyleEngine cssStyleEngine = options1.GetCssStyleEngine();
        if (cssStyleEngine != null)
        {
          string ownAttribute = this.GetOwnAttribute(AttributeNames.Style);
          StyleOptions options2 = new StyleOptions(context)
          {
            Element = (IElement) this
          };
          ICssStyleDeclaration declaration = cssStyleEngine.ParseDeclaration(ownAttribute, options2);
          IBindable bindable = declaration as IBindable;
          if (bindable == null)
            return declaration;
          bindable.Changed += (Action<string>) (value => this.UpdateAttribute(AttributeNames.Style, value));
          return declaration;
        }
      }
      return (ICssStyleDeclaration) null;
    }

I am not sure what the correct behaviour is when the style attribute is non-null but Style is null. Should the style attribute be cleared in that case, as it means the style attribute either isn't expected on that element type or could not be parsed?

[LunNova]

Oops. This occurred due to using a custom parser configuration which (unintentionally) didn't have the CssParserOptions you set. When those are used, the issue does not occur.

I'm leaving this open as the code still seems fragile, and there may be other ways to cause Style to return null, so I think it still should be null checked.

Of course, if you disagree, you're free to close this! :)

[304NotModified]

so I think it still should be null checked.

Agree with that.

[mganss]

Thanks for the bug report. I've fixed this by removing the style attribute if the Style property is null.