Closed tommysor closed 2 years ago
What’s the behavior in the JS library for the Chinese characters?
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 ?
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 = {
'&': '&',
'<': '<',
'>': '>',
'"': '"',
"'": ''',
'`': '`',
'=': '='
};
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(""");
break;
case '&':
target.Write("&");
break;
case '<':
target.Write("<");
break;
case '>':
target.Write(">");
break;
default:
if (value > 159)
{
target.Write("&#");
target.Write((int)value);
target.Write(";");
}
else target.Write(value);
break;
}
}
}
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.
@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.
@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?
@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 xx; 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.
@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.
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.jsCan you please also add corresponding documentation to the README?
Added section to README.
Rebase your branch to the latest master
and it will merge automatically 👍
Kudos, SonarCloud Quality Gate passed!
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 forHandlebars_Should_Encode_Chinese
pass.~~Both tests for
Handlebars_Should_Not_Encode_Chinese
still fail (output is Chinese characters, rather than expected xxx;). I don't know if this should be considered correct behavior, or possibly a separate issue.~~Looking forward to your comments.