NewtonsoftJsonInputFormatter uses incorrect ModelStateDictionary keys for members containing single quotes

[halter73]

Given JSON content like "[{\"It's a key\": 1234556}]" which is deserialized as IDictionary<string, short> (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.

    [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<string, short>), 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.

aspnetcore/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs

Lines 238 to 263 in 28ecab6

	var path = eventArgs.ErrorContext.Path;
	var member = eventArgs.ErrorContext.Member?.ToString();
	var addMember = !string.IsNullOrEmpty(member);
	if (addMember)
	{
	// Path.Member case (path.Length < member.Length) needs no further checks.
	if (path.Length == member!.Length)
	{
	// Add Member in Path.Memb case but not for Path.Path.
	addMember = !string.Equals(path, member, StringComparison.Ordinal);
	}
	else if (path.Length > member.Length)
	{
	// Finally, check whether Path already ends with Member.
	if (member[0] == '[')
	{
	addMember = !path.EndsWith(member, StringComparison.Ordinal);
	}
	else
	{
	addMember = !path.EndsWith("." + member, StringComparison.Ordinal)
	&& !path.EndsWith("['" + member + "']", StringComparison.Ordinal)
	&& !path.EndsWith("[" + member + "]", StringComparison.Ordinal);
	}
	}
	}

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 JamesNK/Newtonsoft.Json#1903

See #39058 (comment) for more context about this issue.

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.