Option to bypass GetContent HTML encoding in TagHelper

[khalidabuhakmeh]

The current implementation of TagHelperContent.GetContent automatically HTML encodes when called from within the TagHelper implementation. Using Dave Paquette's implementation of Markdown Taghelper, I am able to reproduce the unexpected behavior of premature encoding.

Implementation

[HtmlTargetElement("p", Attributes = "markdown")]
[HtmlTargetElement("markdown")]
[OutputElementHint("p")]
public class MarkdownTagHelper : TagHelper
{
    public async override Task ProcessAsync(TagHelperContext context, TagHelperOutput output)
    {
        if (output.TagName == "markdown")
        {
            output.TagName = "p";
        }
        var childContent = await context.GetChildContentAsync();

        // This call to get markdownContent is HTML encoded
        // prematurely and strips meaningful newline characters
        // necessary for the markdown output
        var markdownContent = childContent.GetContent();

        var markdown = new MarkdownSharp.Markdown();
        var htmlContent = markdown.Transform(markdownContent);
        output.Content.SetContentEncoded(htmlContent);
    }
}

Input

### Features\r\n\r\n* API namespace cleanup for tag helpers ([#578](https://github.com/aspnet/Razor/issues/578))\r\n* Refactor WriteAttribute API surface. ([#177](https://github.com/aspnet/Razor/issues/177))\r\n\r\n### Bugs Fixed\r\n\r\n* Multiple TagHelpers targeting same element cannot effectively communicate to children. ([#571](https://github.com/aspnet/Razor/issues/571))\r\n* Compilation error within tag attribute shows generated code instead of markup ([#569](https://github.com/aspnet/Razor/issues/569))\r\n* Specifying `RestrictChildren` and empty `HtmlTargetElement` results in an error ([#562](https://github.com/aspnet/Razor/issues/562))\r\n* `TreeStructureChanged` marked as `true` when whitespace is added to a page with `TagHelper`s ([#553](https://github.com/aspnet/Razor/issues/553))\r\n* Expose `FilePath` on `MappingLocation` to let Razor editor know mapping origination. ([#552](https://github.com/aspnet/Razor/issues/552))\r\n\r\n

Expected

Actual

Proposed Solution

I expect to be able to pull the exact contents of the tag element, even if there is content that is potentially unsafe at runtime. The responsibility of HTML encoding should be left up to the TagHelper developer.

---

http://www.khalidabuhakmeh.com/asp-net-5-mvc-6-taghelpers-and-the-cake

[DamianEdwards]

Thanks for the write up. Here's a gist I did to demonstrate the issue too: https://gist.github.com/DamianEdwards/d57810d28d977058e702

[khalidabuhakmeh]

I did notice that this "fixes" the issue, like you suggested.

<markdown>@(new HtmlString(release.Body))</markdown>

It just smells.

[DamianEdwards]

Yeah this is a layering issue. The Razor parser is emitting code that always evaluates the content inside a Tag Helper as a Razor expression, and thus writes it out that way, which always HTML encodes. I think we need to either:

1. Allow for the Tag Helper to declare that it doesn't want the content encoded
2. Always capture the encoded and unencoded result of the expression then let the Tag Helper get at each via API

[dougbu]

@DamianEdwards suggestion 1 is similar to letting tag helpers declare authors should treat the element content like a bound string property. As such we'd need some new tooling support.

For suggestion 2, note one issue with concepts such as "encoded and unencoded result of the expression" is the mix of scopes in a .cshtml file. There is often no single expression. Even an attribute bound to a string property of a tag helper can contain more than just C#. Doubled HTML encoding won't be eradicated.

Separately, binding to a string property (instead of using the element body) is almost a workaround for this issue. Except in <text> and similar corner cases, nothing is encoded before the tag helper gets it.

[DamianEdwards]

@dougbu why would suggestion 1 require tooling support? It's just changing the behavior of contained expressions, not literal content.

[dougbu]

why would suggestion 1 require tooling support? It's just changing the behavior of contained expressions, not literal content.

Because users HTML encode literal content when they're typing in an HTML context.

[DamianEdwards]

Example?

[dougbu]

<tagHelper>I love me some &lt;tagHelper&gt;.</TagHelper>

[DamianEdwards]

@dougbu ah yes, of course. A tooling feature isn't a huge deal, and frankly we could ship a version without it. This will be applicable only to very particular Tag Helpers. Let's chat about it in person this week.

[NTaylorMullen]

@DamianEdwards and I worked through the implications of this issue and decided that in order to fix this we'll add an overload to GetChildContentAsync that takes an encoder. It's not ideal since you'll also have a GetContent(HtmlEncoder) overload; the idea behind it is that TagHelper rendering has two pieces:

1. Prepare the body to be rendered. This essentially builds out a buffer used to render final content, in cases like @("<p>This should be encoded</p>") it needs to execute the code and encode it when preparing. - GetChildContentAsync
2. Render the body with the specified encoder. - GetContent

Both parts need the encoder.

[dougbu]

@NTaylorMullen please loop in @Eilon on these conversations or talk to me about it tomorrow. What you're suggesting will be incorrect in at least some cases e.g. literal text between the start and end tags. Note as well that this approach assumes all encoding is deferred until GetContent(HtmlEncoder) (ick) is called.

[NTaylorMullen]

@dougbu I think you misunderstood the suggestion.

Note as well that this approach assumes all encoding is deferred until GetContent(HtmlEncoder) (ick) is called.

The suggested approach does not defer all encoding. Anything that is @_____ (aka RazorPage.WriteTo where the value is not a string and not an IHtmlContent will be encoded with the encoder passed to GetChildContentAsync. Therefore, to get the "raw" value of the body users would be able to pass in no-op encoders into GetChildContentAsync and GetContent methods.

[dougbu]

suggested approach does not defer all encoding

Huh? It assumes everything inside that @ expression has not yet been encoded -- that's deferral.

The approach also assumes literal strings contain neither characters that are already encoded <tag> nor characters that need to be encoded <tag>, depending on how the .cshtml author thought it would be handled.

[NTaylorMullen]

Huh? It assumes everything inside that @ expression has not yet been encoded -- that's deferral.

Oh, I wouldn't consider that deferred. That's just how Razor has always worked. Anything after @ is run through WriteTo which may or may not encode the value. In this new case, it'd just use a different encoder IF it decided a value should be encoded.

The approach also assumes literal strings contain neither characters that are already encoded <tag> nor characters that need to be encoded , depending on how the .cshtml author thought it would be handled.

It's just Razor, it doesn't make any assumptions, I'm not seeing how this is different than classic Razor. If you want your stuff to not be encoded and its a string, wrap it in @Html.Raw.