JsonStringEnumConverter ignores 'allowIntegerValues' when deserializing quoted numeric values

[DenisAloner]

.NET 5
JsonStringEnumConverter ignores JsonNumberHandling.Strict on deserialization.
Example: https://dotnetfiddle.net/IsP6Dn

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

---

.NET 5
JsonStringEnumConverter ignores JsonNumberHandling.Strict on deserialization.
Example: https://dotnetfiddle.net/IsP6Dn

Author:	DenisAloner
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[eiriktsarpalis]

This by design, number handling only applies to numeric types and not enums (this includes the default enum converter and not just JsonStringEnumConverter).

Out of curiosity, what would be your expected behavior in that snippet? Assuming strict number handling was respected, it would directly contradict what JsonStringEnumConverter is supposed to do (that is, express enums as strings as opposed to their underlying numeric values).

[DenisAloner]

Thank you for your quick response!
I expected that these options will result in the following:
{"Value" : 0} - not valid
{"Value" : "0"} - not valid
{"Value" : "On"} - valid
{"Value" : "Off"} - valid
But the example shows that these options give the following:
{"Value" : 0} - not valid
{"Value" : "0"} - valid
{"Value" : "On"} - valid
{"Value" : "Off"} - valid

[eiriktsarpalis]

I see; while the issue is unrelated to JsonNumberHandling, the allowIntegerValues parameter is not honored when the undefined enum is encoded as a string. Minimal reproduction:

using System;
using System.Text.Json;
using System.Text.Json.Serialization;

public class Program
{
    public static void Main()
    {
        var options = new JsonSerializerOptions { Converters = { new JsonStringEnumConverter(allowIntegerValues: false) } };
        Console.WriteLine(JsonSerializer.Deserialize<MyEnum>("\"42\"", options)); // does not fail
        Console.WriteLine(JsonSerializer.Deserialize<MyEnum>("42", options)); // fails as expected
    }
    
    public enum MyEnum
    {
        Value = 1
    }
}

I would say we should fix this.

[wsugarman]

Back to the original question, I think allowing integers encoded as strings violates the spirit of this parameter. For example, developers may use JsonStringEnumConverter in their ASP.NET Core projects such their users can specify enumeration names in the request body. This is great because the names are much more description than using numbers. However, I think developers may also assume that they've also decoupled their enum integral values from their web APIs, but this isn't the case. A user could still pass the integral values as strings and successfully bind to the model.

I understand that accepting both the string and integral representations has precedence with use cases like Enum.TryParse, but personally I would like the option to impose more strictness on my JSON parsing. It would be a welcomed addition if this was provided out-of-the-box.

[eiriktsarpalis]

Moving to Future, as we won't have time to work on this in the .NET 7 timeframe.

[layomia]

Up for grabs. I believe the fix here would be to enforce that only named constants are permitted when reading enum values as strings and allowIntegerValues is false.

The relevant code would need to be added to this method.

[davisnw]

As of 2022-12-19, the documentation at https://learn.microsoft.com/en-us/dotnet/api/system.text.json.serialization.jsonstringenumconverter.-ctor?view=net-7.0#system-text-json-serialization-jsonstringenumconverter-ctor(system-text-json-jsonnamingpolicy-system-boolean) states:

allowIntegerValues Boolean

true to allow undefined enum values; otherwise, false. When true, if an enum value isn't defined, it will output as a number rather than a string.

This clearly implies that you should never receive undefined enum values when parsing with allowIntegerValues: false.

I could see this discrepancy leading to security vulnerabilities in some api usage contexts.

(Note: for some reason the links to file feedback on the documentation via github is not available for that page)

[ckarcz]

workaround for now https://stackoverflow.com/a/74890774/929401

[ghost]

Added needs-breaking-change-doc-created label because this issue has the breaking-change label.

1. Create and link to this issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.

Tagging @dotnet/compat for awareness of the breaking change.

[eiriktsarpalis]

The fix is technically a breaking change since disabling integers no longer accepts strings containing integers. The workaround is trivial (just enable integers) but we should still document it.