Modernize API shape of HtmlEncoder and related classes

[GrabYourPitchforks]

The classes HtmlEncoder, JavaScriptEncoder, and UrlEncoder are intended to be the One True Way™ of performing HTML-encoding, JavaScript/JSON string-escaping, and URL-encoding from untrusted input framework-wide. Their primary consumer is ASP.NET, but they're also intended to replace the methods on types like WebUtility which don't always generate standards-compliant output and which are disallowed per SDL.

I believe there are two primary problems with the current shape of these APIs. First, they all subclass a shared TextEncoder type (see apisof.net), which doesn't quite make sense given the scenario. These methods aren't intended to act as generic sinks like our Stream, TextWriter, and other abstractions. They're not interchangeable with one another, even though they all share a common shape. These are transforms. It's up to the consuming component to know which specific transform is applicable to the scenario at hand and to flow the class which represents that transform through its internal logic.

Second, they expose char*-consuming abstract methods, which necessitates that any component which wants to call the more powerful versions of these methods drop down to unsafe code. Even worse, since these methods are abstract, we require that anybody who wants to subclass these types drop down to unsafe code as well. This can be made safer and more accessible by using modern types like Span.

Here is my proposal for the new API shape for the HtmlEncoder, JavaScriptEncoder, and UrlEncoder classes.

namespace System.Text.Encodings.Web
{
    // Also the same shape for JavaScriptEncoder and UrlEncoder
    public abstract class HtmlEncoder
    {
        /* factories */
        public static HtmlEncoder Default { get; }
        public static HtmlEncoder Create(TextEncoderSettings settings);
        public static HtmlEncoder Create(params UnicodeRange[] allowedRanges);

        /* protected ctor */
        protected HtmlEncoder();
        
        /* abstract methods - must override */
        public abstract int MaxOutputCharactersPerInputCharacter { get; } // referring to UTF-16 "System.Char"
        public abstract bool RuneMustBeEncoded(Rune rune);
        public abstract bool TryEncodeRune(Rune rune, Span<char> output, out int charsWritten);
        
        /* virtual methods - *may* override as optimization, but not required */
        public virtual string Encode(string value);
        public virtual void Encode(TextWriter output, char[] value, int startIndex, int characterCount);
        public virtual void Encode(TextWriter output, string value, int startIndex, int characterCount);
        public virtual void Encode(TextWriter output, ReadOnlySpan<char> value);
        public virtual void Encode(IBufferWriter<char> output, ReadOnlySpan<char> value);
        public virtual OperationStatus Encode(ReadOnlySpan<char> input, Span<char> output, bool isFinalChunk, out int numCharsConsumed, out int numCharsWritten);
        public virtual int GetIndexOfFirstCharacterToEncode(ReadOnlySpan<char> input);
        
        /* non-virtual methods */
        public void Encode(TextWriter output, string value);
    }
}

The existing TextEncoderSettings and UnicodeRange types remain unchanged.

This addresses both of the concerns I raised above and is generally source-compatible with existing callers. (It may not be binary-compatible due to removal of the base type.) Data from apisof.net shows that the number of callers of the methods which have been removed is insignificant, less than 0.1% of analyzed code. This refactoring also puts us in a position going forward to get UTF-8 support for these APIs.

Open issue: the property MaxOutputCharactersPerInputCharacter is written in terms of UTF-16 System.Char instances, but perhaps it's better off as MaxOutputCharactersPerRune (maximum UTF-16 System.Char elements output per input Rune). This may trade off some internal efficiency in order to make life easier for callers, but the internal implementations may be able to make up for the loss in efficiency through some other mechanism.

[benaadams]

There's also a double encoding pass that takes place text -> encoded HTML -> encoded utf8

Whereas the common desire can be achieved in a single pass text -> encoded utf8 HTML

The double pass means more things need to be encoded as html escape sequences as the end encoding is not taken into account (e.g. most stuff outside ascii range) whereas it could just remain as utf8 rather than be HTML encoded.

[GrabYourPitchforks]

Right. The default behavior of the encoders is that everything outside the ASCII range is encoded anyway, but this is configurable by changing the options given to the Create factory methods. Currently none of the built-in encoders will allow anything outside the BMP range to pass through unscathed regardless of options provided, so "😱" is still going to be output as 😱. But a custom subclass could make this scenario work if necessary.

[benaadams]

But a custom subclass could make this scenario work if necessary.

By not using any of the overloads?

One of the issues is TextWriter and IBufferWriter<char> go via char, so then need a second stage to convert the char to byte in the appropriate encoding.

So the encoding would be HtmlEncoder -> char -> TextWriter -> byte -> Stream

When ideally it would be HtmlEncoder -> byte -> Stream

So maybe (if breaking the inheritance chain of TextEncoder) the class should be
class HtmlEncoder : TextWriter?

[GrabYourPitchforks]

The UTF-8 APIs are not included on this review. When they're introduced in the future, their signatures would follow a pattern akin to below.

public virtual bool TryEncodeRune(Rune rune, Span<Utf8Char> output, out int utf8CharsWritten);
public virtual void Encode(IBufferWriter<Utf8Char> output, ReadOnlySpan<Utf8Char> value);
public virtual Utf8String Encode(Utf8String value);

Whether they're on a separate type or on this type is still up for discussion. (I prefer a different type, but I can be swayed.) But the plan of record is that they will come at some point, which negates the need for encode-then-transcode.

[benaadams]

Would that also then potentially support chary stuff without double pass? e.g.

public virtual void Encode(IBufferWriter<Utf8Char> output, ReadOnlySpan<char> value);
public virtual Utf8String Encode(string value);

[GrabYourPitchforks]

@benaadams I hadn't considered performing transcoding within the Encode routine itself. My gut tells me that this would have lower overall throughput than performing a two-pass, but we'd have to measure.

[terrajobst]

The proposal is to make a breaking change in System.Text.Encodings.Web.dll. We'd like to see a proposal that is additive. One concern is that the shape of the existing types isn't great for supporting UTF8. Given the usage of the types is fairly low (and deriving from them even lower), we might be able to make a breaking change in .NET Core 3.0. But I believe this needs to be well motivated, not just aesthetics.

[benaadams]

I hadn't considered performing transcoding within the Encode routine itself. My gut tells me that this would have lower overall throughput than performing a two-pass, but we'd have to measure.

For Utf8 encoded HTML you only need to encode the HTML chars and the additional overhead of doing HTML becomes quite light.

For general and custom HTML escaping; without specifically targeting Utf8 it becomes much heavier weight, both to check which chars are allowed and disallowed and also pessimistically escaping due to the unknown nature of what chars the final encoding supports; which bloats the html as well as making it more jibberish (as most people don't read escaped unicode sequences very well)

Just focusing on Utf8 (and including control chars as well for good measure) then the pseudocode for testing whether to encode becomes something like:

private static bool IsCharacterHtmlEncoded(char character)
{
    const int htmlEncodedChars = 
        (0x1 << ('<' & 0x1F)) | 
        (0x1 << ('>' & 0x1F)) | 
        (0x1 << ('&' & 0x1F)) | 
        (0x1 << ('\'' & 0x1F)) | 
        (0x1 << ('\"' & 0x1F));

    bool result;
    if (character >= 'A')
    {
        result = false;
    }
    else if (character >= ' ')
    {
        result = ((htmlEncodedChars >> (character & 0x1F)) & 0x1) == 0 ? false : true;
    }
    else
    {
        // Control chars, encode these as &#1;
        result = true;
    }

    return result;
}

And outputting the encoded section something like

private readonly static byte[] _htmlLessThan =  Encoding.UTF8.GetBytes("<");
private readonly static byte[] _htmlGreaterThan = Encoding.UTF8.GetBytes(">");
private readonly static byte[] _htmlAmpersand = Encoding.UTF8.GetBytes("&");
private readonly static byte[] _htmlDoubleQuote = Encoding.UTF8.GetBytes(""");
private readonly static byte[] _htmlSingleQuote = Encoding.UTF8.GetBytes("'");
private readonly static byte[] _htmlEscapeStart = Encoding.UTF8.GetBytes("&#");
private readonly static byte[] _htmlEscapeEnd = Encoding.UTF8.GetBytes(";");
        
private unsafe static int EncodeHtml<T>(ref this BufferWriter<T> buffer, char* input, int length)
    where T : struct, IBufferWriter<byte>
{
    Debug.Assert(IsCharacterHtmlEncoded(input[0]);

    var encoded = 0;
    while (true)
    {
        var ch = input[0];
        switch (ch)
        {
            case '\"':
                buffer.Write(_htmlDoubleQuote);
                break;
            case '&':
                buffer.Write(_htmlAmpersand);
                break;
            case '\'':
                buffer.Write(_htmlSingleQuote);
                break;
            case '<':
                buffer.Write(_htmlLessThan);
                break;
            case '>':
                buffer.Write(_htmlGreaterThan);
                break;
            default:
                buffer.Write(_htmlEscapeStart);
                buffer.WriteNumeric(ch);
                buffer.Write(_htmlEscapeEnd);
                break;
        }

        encoded++;
        length--;

        if (length > 0 && IsCharacterHtmlEncoded(input[1]))
        {
            input++;
            // Next char also should be encoded
            continue;
        }

        break; // Done
    }

    return encoded;
}

[benaadams]

Also if you are doing Utf8 -> Html escaped Utf8 then most of it is just direct output with no transform; with the occasional escaping bump.

Going via HtmlEncoder -> TextWriter -> UTF8Encoding -> Stream is just adding pure overhead

[GrabYourPitchforks]

If the goal is to minimize the number of breaks by trading off a bit of API cleanliness, here's a modified proposal.

namespace System.Text.Encodings.Web
{
    // Existing abstract class
    public abstract class TextEncoder
    {
        /* EXISTING abstract method - has 0% usage via apisof.net
           (it's currently marked EditorBrowsable.Never, that annotation should be removed) */
        public abstract int MaxOutputCharactersPerInputCharacter { get; } // referring to UTF-16 "System.Char"

        /* NEW abstract methods - must override */
        public abstract bool RuneMustBeEncoded(Rune rune);
        public abstract bool TryEncodeRune(Rune rune, Span<char> output, out int charsWritten);
        
        /* virtual methods - *may* override as optimization, but not required
           (some of these methods already exist, some are new overloads) */
        public virtual string Encode(string value);
        public virtual void Encode(TextWriter output, char[] value, int startIndex, int characterCount);
        public virtual void Encode(TextWriter output, string value, int startIndex, int characterCount);
        public virtual void Encode(TextWriter output, ReadOnlySpan<char> value);
        public virtual void Encode(IBufferWriter<char> output, ReadOnlySpan<char> value);
        public virtual OperationStatus Encode(ReadOnlySpan<char> input, Span<char> output, bool isFinalChunk, out int numCharsConsumed, out int numCharsWritten);
        public virtual int GetIndexOfFirstCharacterToEncode(ReadOnlySpan<char> input);
        
        /* existing non-virtual method */
        public void Encode(TextWriter output, string value);

        /* REMOVED methods - each of these methods currently has 0% usage via apisof.net
           (and they're all marked EditorBrowsable.Never anyway) */
        public unsafe abstract int FindFirstCharacterToEncode(char* text, int textLength);
        public unsafe abstract bool TryEncodeUnicodeScalar(int unicodeScalar, char* buffer, int bufferLength, out int numberOfCharactersWritten);
        public abstract bool WillEncode(int unicodeScalar);
    }
}

Key changes from the original proposal:

• Methods which operate on pointers or on 32-bit scalar values are removed in favor of methods which operate on spans or the Rune type. These three methods marked remove all have 0% usage according to apisof.net.
• The HtmlEncoder, UrlEncoder, and JavaScriptStringEncoder types are unchanged and continue to subclass the base TextEncoder type.
• Existing TextEncoder.Encode methods remain unchanged, with some new overloads added.

This should preserve source and binary compatibility for the vast majority of callers. Additionally, since the type no longer exposes abstract pointer-consuming methods, the type should now be subclassable by safe code.

If this proposal appears fine, then a sister type Utf8TextEncoder can come up in parallel to this, with similar methods. Such methods could also be added onto this existing base type without introducing a breaking change, but it may be a bit clunky.

[tarekgh]

@GrabYourPitchforks are we still targeting 3.0 here?

[danmoseley]

@GrabYourPitchforks I am going to assume this is not for 3.0 as at this point it woudl be more of a DCR. Feel free to fix if I am wrong.