fix `+` in json #3663

shargon · 2025-01-08T19:08:24Z

Description

Fix #2612
Based on https://github.com/neo-project/neo/pull/2050/files

Type of change

Optimization (the change is only an optimization)
Style (the change is only a code style for better maintenance or standard purpose)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

JsonTest_Object

Test Configuration:

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

src/Neo.Json/JToken.cs

roman-khimov · 2025-01-09T19:03:41Z

This can lead to some state diff, needs to be checked.

tests/Neo.UnitTests/SmartContract/UT_JsonSerializer.cs

cschuchardt88

This is how you do it Check out https://www.unicode.org/charts/PDF/U0000.pdf for Basic Latin Unicode block (U+0021-U+007F).

src/Neo.Json/JToken.cs

cschuchardt88

@shargon
I fixed the issue on the branch for you. Check and test changes.

src/Neo.Json/JToken.cs

cschuchardt88 · 2025-01-17T20:52:36Z

src/Neo.Json/JString.cs

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


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.

But it works with newtonsoft (according your reference)

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

cschuchardt88 · 2025-01-17T20:53:27Z

tests/Neo.UnitTests/SmartContract/UT_JsonSerializer.cs

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

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


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

AnnaShaleva · 2025-01-20T16:18:23Z

tests/Neo.UnitTests/SmartContract/UT_JsonSerializer.cs

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

-            json = @" {""\uAAAA"":   true}";
+            json = @" {""test"":""+""} ";


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

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

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

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

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

AnnaShaleva · 2025-01-20T16:20:48Z

src/Neo.Json/JToken.cs

+
+        static JToken()
+        {
+            DefaultEncoder = JavaScriptEncoder.Create(UnicodeRanges.BasicLatin, UnicodeRanges.CjkUnifiedIdeographs);


Does it change the old behaviour except the + symbol?

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

erikzhang · 2025-10-10T02:50:30Z

Maybe we can use UnsafeRelaxedJsonEscaping?
https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/character-encoding#serialize-all-characters

cschuchardt88 · 2025-10-10T16:05:01Z

Maybe we can use UnsafeRelaxedJsonEscaping? https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/character-encoding#serialize-all-characters

We tried that and works great. However it allows for XSS Injection and other security problems.

shargon · 2025-10-10T16:39:55Z

Maybe we can use UnsafeRelaxedJsonEscaping? https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/character-encoding#serialize-all-characters

I remember that we tried with bad results, other characters are not scaped, maybe the '+' I don't remember very well, maybe @roman-khimov yes

roman-khimov · 2025-10-10T19:57:49Z

See #2050. While the problem is valid and it'd be nice to fix it, I fear changing anything can break things.

cschuchardt88 · 2025-10-11T02:22:55Z

Maybe we can use UnsafeRelaxedJsonEscaping? https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/character-encoding#serialize-all-characters

I remember that we tried with bad results, other characters are not scaped, maybe the '+' I don't remember very well, maybe @roman-khimov yes

I based off the things I tested on my local machine it worked for + sign.. But now I remember that Escape characters and Strict UTf-8 didnt work.

erikzhang · 2025-10-11T06:27:14Z

We tried that and works great. However it allows for XSS Injection and other security problems.

Putting strings directly into HTML without processing will always bring security issues. I don't think solving the XSS injection problem should be implemented by the contract interaction interface.

Try fix

0917647

shargon commented Jan 8, 2025

View reviewed changes

src/Neo.Json/JToken.cs Outdated Show resolved Hide resolved

cschuchardt88 added Waiting for Review Work in Progress and removed Waiting for Review labels Jan 9, 2025

Merge branch 'master' into json-plus

6a77c33

Merge branch 'master' into json-plus

83b1394

cschuchardt88 reviewed Jan 13, 2025

View reviewed changes

tests/Neo.UnitTests/SmartContract/UT_JsonSerializer.cs Show resolved Hide resolved

cschuchardt88 reviewed Jan 15, 2025

View reviewed changes

src/Neo.Json/JToken.cs Outdated Show resolved Hide resolved

src/Neo.Json/JToken.cs Outdated Show resolved Hide resolved

Fixed unsafe relaxing

3c15c6f

cschuchardt88 reviewed Jan 17, 2025

View reviewed changes

Merge branch 'master' into json-plus

ebb00e7

cschuchardt88 reviewed Jan 17, 2025

View reviewed changes

src/Neo.Json/JToken.cs Outdated Show resolved Hide resolved

Allow plus sign +

9bb0fe7

cschuchardt88 reviewed Jan 17, 2025

View reviewed changes

cschuchardt88 assigned shargon and cschuchardt88 Jan 17, 2025

AnnaShaleva reviewed Jan 20, 2025

View reviewed changes

Merge branch 'master' into json-plus

be4c2ce

Jim8y mentioned this pull request Jan 23, 2025

[Jan.23] Core Dev Regular Meeting Proposal Discussion #3690

Closed

shargon added the Discussion Initial issue state - proposed but not yet accepted label Jan 23, 2025

fix + in json #3663

Are you sure you want to change the base?

fix + in json #3663

Uh oh!

Conversation

shargon commented Jan 8, 2025

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Uh oh!

roman-khimov commented Jan 9, 2025

Uh oh!

Uh oh!

cschuchardt88 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cschuchardt88 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cschuchardt88 Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shargon Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Jan 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Jan 17, 2025

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

shargon Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shargon Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnnaShaleva Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

erikzhang commented Oct 10, 2025

Uh oh!

cschuchardt88 commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shargon commented Oct 10, 2025

Uh oh!

roman-khimov commented Oct 10, 2025

Uh oh!

cschuchardt88 commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erikzhang commented Oct 11, 2025

Uh oh!

Reviewers

Assignees

fix `+` in json #3663

fix `+` in json #3663

cschuchardt88 left a comment •

edited

Loading

cschuchardt88 left a comment •

edited

Loading

cschuchardt88 Jan 17, 2025 •

edited

Loading

cschuchardt88 Jan 18, 2025 •

edited

Loading

cschuchardt88 Jan 20, 2025 •

edited

Loading

shargon Jan 20, 2025 •

edited

Loading

cschuchardt88 Jan 20, 2025 •

edited

Loading

cschuchardt88 commented Oct 10, 2025 •

edited

Loading

cschuchardt88 commented Oct 11, 2025 •

edited

Loading