HtmlEncoding consistent with rules in handlebars.js

[tommysor]

The original issue this PR was intended to solve have been fixed in PR #477.
This PR now deals with general rules for encoding in Handlebars.Net vs handlebars.js.

Using this PR since it contains the history of how this change came to be.

[WIP] Configuration.NoEscape inconsistent fix

Regarding issue: HandlebarsDotNet.Handlebars.Configuration.NoEscape Applied Inconsistently #468

Change in commit: Reset SuppressEncoding to configured instead of false. (old) UnencodedStatementVisitor resets value to previous (new) makes both tests for Handlebars_Should_Encode_Chinese pass.

Both tests for Handlebars_Should_Not_Encode_Chinese still fail (output is Chinese characters, rather than expected &#xxxx;).
I don't know if this should be considered correct behavior, or possibly a separate issue.

Looking forward to your comments.

[rexm]

What’s the behavior in the JS library for the Chinese characters?

[tommysor]

What’s the behavior in the JS library for the Chinese characters?

I have no idea. Or any idea of how to figure it out.
Closest I see is this page: http://tryhandlebarsjs.com/
But I don't know what settings they are using and if that matches what's relevant for this case.

On http://tryhandlebarsjs.com/ with "expression with raw HTML"
"<" is shown as <
While non-ascii characters "öä ñ øå 产品" are shown as entered.

Any insight here @zjklee or @tonysneed ?

[tommysor]

I had another look, still cant test if I'm correct.
Looks to me like the JS library does not escape anything except this small list of chars.

https://github.com/handlebars-lang/handlebars.js/blob/master/lib/handlebars/utils.js

const escape = {
  '&': '&',
  '<': '&lt;',
  '>': '&gt;',
  '"': '&quot;',
  "'": '&#x27;',
  '`': '&#x60;',
  '=': '&#x3D;'
};

const badChars = /[&<>"'`=]/g,
  possible = /[&<>"'`=]/;

function escapeChar(chr) {
  return escape[chr];
}

[...]

export function escapeExpression(string) {
  if (typeof string !== 'string') {
    // don't escape SafeStrings, since they're already safe
    if (string && string.toHTML) {
      return string.toHTML();
    } else if (string == null) {
      return '';
    } else if (!string) {
      return string + '';
    }

    // Force a string conversion as this will be done by the append regardless and
    // the regex test will do this transparently behind the scenes, causing issues if
    // an object's to string has escaped characters in it.
    string = '' + string;
  }

  if (!possible.test(string)) {
    return string;
  }
  return string.replace(badChars, escapeChar);
}

https://github.com/Handlebars-Net/Handlebars.Net/blob/master/source/Handlebars/IO/HtmlEncoder.cs

[...]

private static void EncodeImpl<T>(T text, TextWriter target) where T: IEnumerator<char>
{
	while (text.MoveNext())
	{
		var value = text.Current;
		switch (value)
		{
			case '"':
				target.Write("&quot;");
				break;
			case '&':
				target.Write("&amp;");
				break;
			case '<':
				target.Write("&lt;");
				break;
			case '>':
				target.Write("&gt;");
				break;

			default:
				if (value > 159)
				{
					target.Write("&#");
					target.Write((int)value);
					target.Write(";");
				}
				else target.Write(value);
				break;
		}
	}
}

[tommysor]

Cleaned up branch. The two commits now address 2 separate issues.

Commit: UnencodedStatementVisitor resets value to previous
is a strait forward bugfix, and I believe it should be enough to fix the specific problem in issue #468.

Commit: HtmlEncoder escape chars based on https://github.com/handlebars-lang/…
is a danger zone.
Encoding rules copied from JS based on my understanding.
I left out (with comment) 2 chars ( ' and = ) because encoding them breaks existing tests.
Would likely break real world implementations that rely on current behavior.

[tonysneed]

@tommysor Thanks for working to fix #468. I'll pull your branch to see if the tests pass in my repro.

@rexm @zjklee I'm looking forward to your review of this PR.

[tonysneed]

@tommysor I puled your branch and ran my tests. The tests which set Handlebars.Configuration.NoEscape = true all pass.

However, setting Handlebars.Configuration.NoEscape = false has no effect. The Chinese characters are not encoded when they should be. Is this expected behavior?

[tommysor]

@tonysneed That, I believe, is the $1,000 question for this pull request.

Under assumption: Existing encoding rules in Handlebars.Net is correct (https://github.com/Handlebars-Net/Handlebars.Net/blob/master/source/Handlebars/IO/HtmlEncoder.cs).
Then expected result is that the Chinese characters are encoded to &#xxx; when NoEscape = false.
This is what you should get if you cherry pick only the commit UnencodedStatementVisitor resets value to previous.
@tonysneed Your tests will not pass as is. They will pass if you make a change in Class.hbs to double curlys {{> properties}} from triple curlys {{{> properties}}}.

Under assumption: Encoding rules in JavaScript repo is correct (https://github.com/handlebars-lang/handlebars.js/blob/master/lib/handlebars/utils.js).
Then expected result is that the Chinese characters are kept unchanged regardless of NoEscape setting.
This is the current behavior of this branch. Some deviations from js rules would need some cleanup work.

Maintainers (so @rexm and/or @zjklee) should decide the direction for this.
My own suggestion would be that I split the commit UnencodedStatementVisitor resets value to previous out into a new pull request. That commit can be released as a patch with very low chance of breaking anything.
While the change to general handling of encoding would be a separate larger change, or possibly not done, according to the maintainers decision.

[oformaniuk]

@tommysor , thanks for taking time for creating the PR.
I'd probably go with a safer option - first merge non-breaking changes. In a separate PR introduce the breaking changes but hide them behind feature toggle. The default behaviour should stay the same in current version but can be changed in the next major release.

[tommysor]

Add feature toggle Compatibility.UseLegacyHandlebarsNetHtmlEncoding

• true: No change from current behavior (default).
• false: Use rules from https://github.com/handlebars-lang/handlebars.js/blob/master/lib/handlebars/utils.js

[oformaniuk]

Can you please also add corresponding documentation to the README?

[tommysor]

Can you please also add corresponding documentation to the README?

Added section to README.

[oformaniuk]

Rebase your branch to the latest master and it will merge automatically 👍

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
16.6% Duplication