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.

Sign up for GitHub

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

fix + in json #3663

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

fix + in json #3663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0917647
Try fix
shargon Jan 8, 2025
6a77c33
Merge branch 'master' into json-plus
Jim8y Jan 9, 2025
83b1394
Merge branch 'master' into json-plus
Jim8y Jan 11, 2025
3c15c6f
Fixed unsafe relaxing
cschuchardt88 Jan 17, 2025
ebb00e7
Merge branch 'master' into json-plus
cschuchardt88 Jan 17, 2025
9bb0fe7
Allow plus sign `+`
cschuchardt88 Jan 17, 2025
be4c2ce
Merge branch 'master' into json-plus
Jim8y Jan 23, 2025
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
2 changes: 1 addition & 1 deletion src/Neo.Json/JString.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public override T GetEnum<T>(bool ignoreCase = false)

internal override void Write(Utf8JsonWriter writer)
{
writer.WriteStringValue(Value);
writer.WriteRawValue($"\"{DefaultEncoder.Encode(Value).Replace("\\u002B", "+")}\"");
Copy link
Member

@cschuchardt88 cschuchardt88 Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Keep in mind allowing the + (plus) sign is a security risk, reference: dotnet/runtime#35281
Only for XSS injection though.

Note: After searching for hours with no luck for solution. I came up with this solution on my own. Its dirty and could have bugs or exploits, however am 99% sure we don't. Need more tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

But it works with newtonsoft (according your reference)

Copy link
Member

@cschuchardt88 cschuchardt88 Jan 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Newtonsoft uses it own custom library. It doesn't use dotnet built-in JSON libraries.

}

public override JToken Clone()
Expand Down
10 changes: 10 additions & 0 deletions src/Neo.Json/JToken.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
// Redistribution and use in source and binary forms with or without
// modifications are permitted.

using System.Text.Encodings.Web;
using System.Text.Json;
using System.Text.Unicode;
using static Neo.Json.Utility;

namespace Neo.Json
Expand All @@ -19,6 +21,13 @@ namespace Neo.Json
/// </summary>
public abstract class JToken
{
public readonly static JavaScriptEncoder DefaultEncoder;

static JToken()
{
DefaultEncoder = JavaScriptEncoder.Create(UnicodeRanges.BasicLatin, UnicodeRanges.CjkUnifiedIdeographs);
Copy link
Member

Choose a reason for hiding this comment

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

Does it change the old behaviour except the + symbol?

Copy link
Member

Choose a reason for hiding this comment

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

Checkout #3663 (comment), but it should be the exactly the same with the addition of the + (plus sign).

}

/// <summary>
/// Represents a <see langword="null"/> token.
/// </summary>
Expand Down Expand Up @@ -239,6 +248,7 @@ public byte[] ToByteArray(bool indented)
using Utf8JsonWriter writer = new(ms, new JsonWriterOptions
{
Indented = indented,
Encoder = DefaultEncoder,
SkipValidation = true
});
Write(writer);
Expand Down
4 changes: 1 addition & 3 deletions src/Neo.Json/Utility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,7 @@ static class Utility

static Utility()
{
StrictUTF8 = (Encoding)Encoding.UTF8.Clone();
StrictUTF8.DecoderFallback = DecoderFallback.ExceptionFallback;
StrictUTF8.EncoderFallback = EncoderFallback.ExceptionFallback;
StrictUTF8 = new UTF8Encoding(false, true);
}
}
}
1 change: 1 addition & 0 deletions src/Neo/SmartContract/JsonSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ public static byte[] SerializeToByteArray(StackItem item, uint maxSize)
using Utf8JsonWriter writer = new(ms, new JsonWriterOptions
{
Indented = false,
Encoder = JToken.DefaultEncoder,
SkipValidation = false
});
Stack stack = new();
Expand Down
7 changes: 5 additions & 2 deletions tests/Neo.UnitTests/SmartContract/UT_JsonSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,13 @@ public void JsonTest_Object()

Assert.AreEqual(@"{""test"":true}", parsed.ToString());

json = @" {""\uAAAA"": true}";
json = @" {""test"":""+""} ";
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep this test-case with \uAAAA? It's a nice compatibility test for NeoGo.

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact I think this will be dropped, no way to do it without unsafe relaxing if we use native json

Copy link
Member

@cschuchardt88 cschuchardt88 Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Doesnt use unsafe relaxing currently. And its working fine now.

Copy link
Member Author

@shargon shargon Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Replace is a dirty fix, performance is affected always, not only when it contains +

Copy link
Member

@cschuchardt88 cschuchardt88 Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

RegEx.UnEscape? https://learn.microsoft.com/en-us/dotnet/api/system.text.regularexpressions.regex.unescape?view=net-9.0

Could use: https://github.com/dotnet/runtime/blob/4c224ab16d287c17dbc54236638db91266071465/src/libraries/System.Text.Json/src/System/Text/Json/Reader/JsonReaderHelper.Unescaping.cs#L69

parsed = JObject.Parse(json);
Assert.AreEqual(@"{""test"":""+""}", parsed.ToString());
cschuchardt88 marked this conversation as resolved.
Show resolved Hide resolved

Assert.AreEqual(@"{""\uAAAA"":true}", parsed.ToString());
json = @" {""测试"": true}";
Copy link
Member

Choose a reason for hiding this comment

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

Had to allow UnicodeRanges.CjkUnifiedIdeographs for these characters for json DefaultEncoder.

parsed = JObject.Parse(json);
Assert.AreEqual(@"{""测试"":true}", parsed.ToString());

json = @"{""a"":}";
Assert.ThrowsException<FormatException>(() => JObject.Parse(json));
Expand Down
Loading