Ensure SystemTextJsonHelper always HTML encodes output. #12808
Conversation
pranavkm
changed the title
pranavkm
added
the
ask-mode
|
|
||
| private static JsonSerializerOptions CreateHtmlSafeSerializerOptions(JsonSerializerOptions serializerOptions) | ||
| { | ||
| var htmlSafeOptions = new JsonSerializerOptions |
There was a problem hiding this comment.
@ahsonkhan @steveharter - these are the kinds of cases where a copy constructor would be massively useful.
There was a problem hiding this comment.
I see. Being tracked by an issue that @pranavkm opened earlier: https://github.com/dotnet/corefx/issues/39939
Pilchie
added
the
area-mvc
src/Mvc/Mvc.Core/src/JsonOptions.cs
Outdated
| @@ -26,6 +27,9 @@ public class JsonOptions | |||
|
|
|||
| // Use camel casing for properties | |||
| PropertyNamingPolicy = JsonNamingPolicy.CamelCase, | |||
|
|
|||
| // Does not encode all non-ASCII characters which matches Json.NET"s behavior. | |||
| Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping, | |||
There was a problem hiding this comment.
I believe this is a dangerous place to put this line of code. The UnsafeRelaxedJsonEscaping property is only intended to be used in specific scenarios where the stack has tight control over how the data will be written to the wire and how the client will consume the data. Any other usage is dangerous, hence the term "unsafe" in the name.
Here, the part that causes me concern is that ASP.NET is configuring this setting in a place very far removed from its intended known-good usage scenario. Specifically, this particular escaping should only be set if we know for absolute certain that the response will contain a header Content-Type: application/json; charset=utf=8, but this line of code is so far removed from the component that sets the response Content-Type header that this risks introducing a vulnerability due to misuse.
There was a problem hiding this comment.
What the heck is the point in having safe defaults if asp.net is going to dump all over them and turn it off?
There was a problem hiding this comment.
We might be able to localize this to the Json Formatter when the encoding is Uf8 and leave the defaults untouched. Would that be sufficient?
| { | ||
| var jsonSerializerOptions = jsonOptions.JsonSerializerOptions; | ||
|
|
||
| if (jsonSerializerOptions.Encoder is null) |
There was a problem hiding this comment.
🆙 📅
This limits the encoder to use the relaxed encoding specifically in the output formatter assuming the user hasn't already configured it.
There was a problem hiding this comment.
Spoke to @GrabYourPitchforks offline. We decided limited this to the formatter is reasonable. The JsonHelper always defaults to using the Default encoding.
src/Mvc/Mvc.Core/src/Infrastructure/JsonSerializerOptionsCopyConstructor.cs
Show resolved
Hide resolved
ghost
deleted the
Ensure JsonSerializer always HTML encodes output.
characters by default. This makes the encoding comparable to Json.NET's defaults
Fixes #9946
Fixes #11459
Description
System.Text.Json introduced a feature that allows configuring the encoding behavior including preventing encoding HTML content. In one case, MVC needs to guarantee the generated content is always HTML safe. This is important to prevent XSS.
Regression
The feature was added to System.Text.Json fairly recently and this is a reaction.
Customer Impact
n/a
Risk
n/a