NewtonsoftJsonInputFormatter uses incorrect ModelStateDictionary keys for members containing single quotes
- Dominant language
- C#
- Stars
- 38.4k
- Forks
- 10.9k
- Avg merge
- 2d 10h
- Merged PRs (30d)
- 281
Description
Given JSON content like `"[{\"It's a key\": 1234556}]"` which is deserialized as `IDictionary` (the value is too big for a short making this an error), `NewtonsoftJsonInputFormatter` adds an entry to the `ModelStateDictionary` with the key `"['It\\'s a key'].It's a key"` instead of `['It\\'s a key']` as expected.
```C#
[Fact]
public virtual async Task JsonFormatter_EscapedKeys_SingleQuote()
{
var expectedKey = JsonFormatter_EscapedKeys_SingleQuote_Expected;
// Arrange
var content = "{\"It's a key\": 1234556}";
var formatter = GetInputFormatter();
var contentBytes = Encoding.UTF8.GetBytes(content);
var httpContext = GetHttpContext(contentBytes);
var formatterContext = CreateInputFormatterContext(
typeof(IDictionary), httpContext);
// Act
var result = await formatter.ReadAsync(formatterContext);
// Assert
Assert.True(result.HasError);
Assert.Collection(
formatterContext.ModelState.OrderBy(k => k.Key),
kvp =>
{
// This fails with Expected: ['It\'s a key'], Actual: ['It\'s a key'].It's a key
Assert.Equal("['It\\'s a key']", kvp.Key);
});
}
```
This is caused by logic in `NewtonsoftJsonInputFormatter`'s `ErrorHandler` that appends the `ErrorContext.Member` to the `ErrorContext.Path` when the `Path` doesn't already end with `Member` in order to better report missing required properties.
https://github.com/dotnet/aspnetcore/blob/28ecab610cce3afd17305b3929ba5c39c63ef02d/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs#L238-L263
`ErrorContext.Path` escapes the `'`, but `ErrorContext.Member` doesn't meaning the `!path.EndsWith()` checks don't cover this scenario. We could scan `ErrorContext.Member` for any `'` characters and manually escape it before doing the `!path.EndsWith()` checks, but that feels like playing whac-a-mole. I'm guessing there are even more edge cases the `!path.EndsWith()` checks don't cover.
I really want to get rid of the `addMember` logic altogether and have Json.NET either give us the path we want to begin with or expose an `ErrorContext.ErrorType` so we only do the `addMember` logic for missing required properties where we know the member is never in the path. The latter option was proposed a while back but never implemennted in Json.NET. See https://github.com/JamesNK/Newtonsoft.Json/issues/1903
See https://github.com/dotnet/aspnetcore/pull/39058#discussion_r769917550 for more context about this issue.
Contributor guide
Assessment
This issue has not been assessed yet.