Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(83547): Adding escaping of property name for GetPath. #84688

Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1311,7 +1311,7 @@ private SourceText GetPropertyNameInitialization(ContextGenerationSpec contextSp

foreach (KeyValuePair<string, string> name_varName_pair in _propertyNames)
{
writer.WriteLine($$"""private static readonly {{JsonEncodedTextTypeRef}} {{name_varName_pair.Value}} = {{JsonEncodedTextTypeRef}}.Encode("{{name_varName_pair.Key}}");""");
writer.WriteLine($$"""private static readonly {{JsonEncodedTextTypeRef}} {{name_varName_pair.Value}} = {{JsonEncodedTextTypeRef}}.Encode({{FormatStringLiteral(name_varName_pair.Key)}});""");
}

return CompleteSourceFileAndReturnText(writer);
Expand Down Expand Up @@ -1343,7 +1343,8 @@ private static string FormatJsonSerializerDefaults(JsonSerializerDefaults defaul
private static string GetCreateValueInfoMethodRef(string typeCompilableName) => $"{CreateValueInfoMethodName}<{typeCompilableName}>";

private static string FormatBool(bool value) => value ? "true" : "false";
private static string FormatStringLiteral(string? value) => value is null ? "null" : $"\"{value}\"";
private static string FormatStringLiteral(string? value)
=> value is null ? "null" : SyntaxFactory.Literal(value).ToFullString();
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Method used to generate JsonTypeInfo given options instance
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,34 @@ public static byte[] EscapeValue(
return escapedString;
}

public static string GetPropertyName(
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
string rawPropertyName,
JavaScriptEncoder? encoder)
{
int indexOfFirstCharacterToEncode;
string returnValue;

indexOfFirstCharacterToEncode = JsonWriterHelper.NeedsEscaping(rawPropertyName.AsSpan(), encoder);
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved

if (indexOfFirstCharacterToEncode != -1)
{
returnValue = GetEscapedPropertyName(rawPropertyName, encoder);
}
else
{
returnValue = rawPropertyName;
}

return returnValue;
}
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved

private static string GetEscapedPropertyName(
string rawPropertyName,
JavaScriptEncoder? encoder)
{
return (encoder ?? JavaScriptEncoder.Default).Encode(rawPropertyName);
}

eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
private static byte[] GetEscapedPropertyNameSection(
ReadOnlySpan<byte> utf8Value,
int firstEscapeIndexVal,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Text.Encodings.Web;

namespace System.Text.Json.Nodes
{
Expand Down Expand Up @@ -206,7 +207,7 @@ internal override void GetPath(ref ValueStringBuilder path, JsonNode? child)
if (propertyName.AsSpan().ContainsSpecialCharacters())
{
path.Append("['");
path.Append(propertyName);
path.Append(JsonHelpers.GetPropertyName(propertyName, JavaScriptEncoder.Default));
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
path.Append("']");
}
else
Expand Down
50 changes: 50 additions & 0 deletions src/libraries/System.Text.Json/tests/Common/PropertyNameTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,18 @@ public async Task BuiltInPolicyDeserializeMatch()
await DeserializeAndAssert(JsonNamingPolicy.KebabCaseUpper, @"{""MY-INT16"":1}", 1);
}

[Theory]
[MemberData(nameof(SuccessDeserialize_TestData))]
public async Task SuccessDeserialize(string valueForDeserialize, ClassWithEscapablePropertyName expectedValue)
{
ClassWithEscapablePropertyName actualValue;

actualValue = await Serializer.DeserializeWrapper<ClassWithEscapablePropertyName>(valueForDeserialize);

Assert.Equal(expectedValue.MyFunnyProperty, actualValue.MyFunnyProperty);
Assert.Equal(expectedValue.MyFunnyProperty2, actualValue.MyFunnyProperty2);
}

private async Task DeserializeAndAssert(JsonNamingPolicy policy, string json, short expected)
{
var options = new JsonSerializerOptions { PropertyNamingPolicy = policy };
Expand Down Expand Up @@ -494,5 +506,43 @@ public class ClassWithSpecialCharacters
[JsonPropertyName("\uA000_2")] // Valid C# property name: \uA000_2
public int YiIt_2 { get; set; }
}

public static IEnumerable<object[]> SuccessDeserialize_TestData()
{
yield return new object[]
{
"{\"abc[!@#№$;%:^&?*()-+~`|]'def'\":\"valueFromMyFunnyPropertyTestCase1\"}",
new ClassWithEscapablePropertyName
{
MyFunnyProperty = "valueFromMyFunnyPropertyTestCase1"
},
};
yield return new object[]
{
"{\"abc[!@#№$;%:^&?*()-+~`|]'def'\":\"valueFromMyFunnyPropertyTestCase1\", \"withQuote\\\"\": \"valueFromMyFunnyProperty2\"}",
new ClassWithEscapablePropertyName
{
MyFunnyProperty = "valueFromMyFunnyPropertyTestCase1",
MyFunnyProperty2 = "valueFromMyFunnyProperty2"
},
};
yield return new object[]
{
"{\"abc[!@#\\u2116$;%:^\\u0026?*()-\\u002B~\\u0060|]\\u0027def\\u0027\":\"valueFromMyFunnyPropertyTestCase2\"}",
new ClassWithEscapablePropertyName
{
MyFunnyProperty = "valueFromMyFunnyPropertyTestCase2"
},
};
}

public class ClassWithEscapablePropertyName
{
[JsonPropertyName("abc[!@#№$;%:^&?*()-+~`|]'def'")]
public string MyFunnyProperty { get; set; }

[JsonPropertyName("withQuote\"")]
public string MyFunnyProperty2 { get; set; }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public PropertyNameTests_Metadata()
[JsonSerializable(typeof(Dictionary<string, OverridePropertyNameDesignTime_TestClass>))]
[JsonSerializable(typeof(Dictionary<string, int>))]
[JsonSerializable(typeof(int))]
[JsonSerializable(typeof(ClassWithEscapablePropertyName))]
[JsonSerializable(typeof(ClassWithSpecialCharacters))]
[JsonSerializable(typeof(ClassWithPropertyNamePermutations))]
[JsonSerializable(typeof(ClassWithUnicodeProperty))]
Expand All @@ -43,6 +44,7 @@ public PropertyNameTests_Default()
[JsonSerializable(typeof(Dictionary<string, OverridePropertyNameDesignTime_TestClass>))]
[JsonSerializable(typeof(Dictionary<string, int>))]
[JsonSerializable(typeof(int))]
[JsonSerializable(typeof(ClassWithEscapablePropertyName))]
[JsonSerializable(typeof(ClassWithSpecialCharacters))]
[JsonSerializable(typeof(ClassWithPropertyNamePermutations))]
[JsonSerializable(typeof(ClassWithUnicodeProperty))]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using Xunit;

namespace System.Text.Json.Nodes.Tests
Expand Down Expand Up @@ -76,6 +77,17 @@ public static void GetPathAndRoot()
Assert.Equal("$[0].Child", node[0]["Child"].GetPath());
}

[Theory]
[MemberData(nameof(GetPath_ShouldReturnExpectedValue_TestData))]
public static void GetPath_ShouldReturnExpectedValue(JsonNode jsonNode, string expectedValue)
{
string actualValue;

actualValue = jsonNode.GetPath();

Assert.Equal(expectedValue, actualValue);
}

[Fact]
public static void GetPath_SpecialCharacters()
{
Expand Down Expand Up @@ -180,5 +192,49 @@ public static void Parent_Object()
parent.Add("MyProp", child);
Assert.True(child.Options.Value.PropertyNameCaseInsensitive);
}

public static IEnumerable<object[]> GetPath_ShouldReturnExpectedValue_TestData()
{
yield return new object[]
{
JsonNode.Parse("""{"$myRoot":{"foo['bar":"baz"}}""")["$myRoot"]["foo['bar"],
"$.$myRoot['foo[\\u0027bar']"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading these tests right, they're still not creating the correct paths. $myRoot isn't valid for the dot syntax and needs to be bracketed.

Copy link
Member

@eiriktsarpalis eiriktsarpalis Oct 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Maximys would it be possible to revisit this? Thanks!

};
yield return new object[]
{
JsonNode.Parse("""{"$myRoot":{"foo[\"bar":"baz"}}""")["$myRoot"]["foo[\"bar"],
"$.$myRoot['foo[\\u0022bar']"
};
yield return new object[]
{
JsonNode.Parse("""{"$myRoot":{"foo['b\"ar":"baz"}}""")["$myRoot"]["foo['b\"ar"],
"$.$myRoot['foo[\\u0027b\\u0022ar']"
};
yield return new object[]
{
JsonNode.Parse("""{"$myRoot": {"myRoot'child": {"myRoot'child'secondLevelChild": "value1"}}}""")["$myRoot"]["myRoot'child"]["myRoot'child'secondLevelChild"],
"$.$myRoot['myRoot\\u0027child']['myRoot\\u0027child\\u0027secondLevelChild']"
};
yield return new object[]
{
JsonNode.Parse("""{"$myRoot": {"myRoot\"child": {"myRoot\"child\"secondLevelChild": "value1"}}}""")["$myRoot"]["myRoot\"child"]["myRoot\"child\"secondLevelChild"],
"$.$myRoot['myRoot\\u0022child']['myRoot\\u0022child\\u0022secondLevelChild']"
};
yield return new object[]
{
JsonNode.Parse("""{"$myRoot": {"myRoot\"child": {"myRoot'child\"secondLevelChild": "value1"}}}""")["$myRoot"]["myRoot\"child"]["myRoot'child\"secondLevelChild"],
"$.$myRoot['myRoot\\u0022child']['myRoot\\u0027child\\u0022secondLevelChild']"
};
yield return new object[]
{
JsonNode.Parse("""{"$myRoot": {"myRoot'child": {"secondLevelChildWithoutEscaping": "value2"}}}""")["$myRoot"]["myRoot'child"]["secondLevelChildWithoutEscaping"],
"$.$myRoot['myRoot\\u0027child'].secondLevelChildWithoutEscaping"
};
yield return new object[]
{
JsonNode.Parse("""{"$myRoot": {"myRoot\"child": {"secondLevelChildWithoutEscaping": "value2"}}}""")["$myRoot"]["myRoot\"child"]["secondLevelChildWithoutEscaping"],
"$.$myRoot['myRoot\\u0022child'].secondLevelChildWithoutEscaping"
};
}
}
}