toddams / RazorLight

Template engine based on Microsoft's Razor parsing engine for .NET Core
Apache License 2.0
1.51k stars 260 forks source link

fix bad ISO-8859-1 encoding #316

Closed Yuki-Nagato closed 4 years ago

jzabroski commented 4 years ago

@Yuki-Nagato Thanks, this looks good. I'll likely try to push this and release a fix tonight, assuming tests pass on my local.

CC @toddams @bjcull

jzabroski commented 4 years ago

@Yuki-Nagato Take a look at this review by a .NET Security Expert: https://github.com/aspnet/HttpAbstractions/issues/315#issuecomment-106832714

Effectively, he is saying your change is good for security reasons, but... also mentions the idea of exposing a whitelist via DI for people who may want to optimize page size at the risk of utf7 security issues.

What do you think?

Yuki-Nagato commented 4 years ago

I don't really understand why we care about the underlying Unicode encoding (e.g. UTF-7). RazorLight renders models at the charactor (string) level, so we just need to care about the HTML entity encoding.

My intention is that I expect the code

string template = "<div>@Model.Content</div>";
var model = new { Content = "長門有希 <^_^>" };

returns the result

"<div>長門有希 &lt;^_^&gt;</div>"

but not

"&#x9577;&#x9580;&#x6709;&#x5E0C; &lt;^_^&gt;"

The subject of the charactor encoding is not related to the string renderer.

jzabroski commented 4 years ago

I don't really understand why we care about the underlying Unicode encoding (e.g. UTF-7).

Hi @Yuki-Nagato - The reason you should care is explained by Barry Dorrans of the .NET Security team, in the post I linked:

defaulting to encoding everything over and above ASCII is what makes it 100% safe for the other things to consume, so I'm not minded to change the default encoder configuration

On the other hand, I also see the first solution Barry proposed for MVC 5 is no longer possible:

HtmlEncoder.Default = new HtmlEncoder(UnicodeRanges.All);

The HtmlEncoder.Default property is now getter-only in .NET Core.

I have not tried his second solution and, to be honest, don't understand why it works:

serviceColl.AddWebEncoders(o => { 
    o.CodePointFilter = new CodePointFilter(UnicodeRanges.All);
});

But it makes sense to me the above way would be ideal. That said, this page: https://aspnetcore.readthedocs.io/en/stable/security/cross-site-scripting.html#customizing-the-encoders

Seems to suggest the "new right way" is:

services.AddSingleton<HtmlEncoder>(
  HtmlEncoder.Create(allowedRanges: UnicodeRanges.All));
jzabroski commented 4 years ago

@Yuki-Nagato The other suggestion/question is, did you try this from the FAQ: https://github.com/toddams/RazorLight/#encoding