dotnet / dotnet/aspnetcore

NewtonsoftJsonInputFormatter uses incorrect ModelStateDictionary keys for members containing single quotes

Open
#39,069 2 comments 0 reactions 0 assignees View on GitHub
area-mvc bug feature-mvc-formatting
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.