ref: added functionality to escapeJSONstrings (ref: #5251) #5508

DarkLord017 · 2025-02-14T08:23:36Z

A function to escape special characters in JSON strings
Handles key characters like quotes ("), backslashes (), forward slashes (/), and control characters (\n, \t, \r, etc.)
Prevents JSON injection attacks in NFT metadata and other use cases

PR Checklist

Tests
Documentation
Changeset entry (run npx changeset add)

changeset-bot · 2025-02-14T08:23:40Z

🦋 Changeset detected

Latest commit: d18d78d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

DarkLord017 · 2025-02-14T08:26:32Z

Is my approach correct , am i working right on this pr @Amxx ?

contracts/utils/Strings.sol

Amxx · 2025-02-14T08:37:39Z

contracts/utils/Strings.sol

+
+    function _escapeJsonString(string memory input) private pure returns (string memory) {
+    bytes memory buffer = bytes(input);
+    bytes memory output = new bytes(buffer.length * 2); // Allocate max possible space


I'm worried this allocated a lot of memory that is not freed. This is quite expensive. Alternatives are:

count number of "extra chars" in a loop, and allocate the right size directly

resize the output buffer without a copy, and move the free memory ptr, using assembly

contracts/utils/Strings.sol

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>

DarkLord017 · 2025-02-16T06:08:53Z

I tried implementing both changes, but the one I pushed used 207 less gas when tested with 'Test "Hello , World!"'. I've uploaded a screenshot of the other implementation. Which one do you think I should go with?

DarkLord017 · 2025-02-16T09:17:44Z

U can review it now @Amxx

Amxx · 2025-02-18T09:15:06Z

You are doing quite a lot of read/write to the output and 0x40. That can be significantly reduced!
Also, I don't understand why you do outputLength + 2

My version is just

function escapeJson(string memory input) internal pure returns (string memory) {
    bytes memory buffer = bytes(input);
    bytes memory output = new bytes(2 * buffer.length); // worst case scenario
    uint256 outputLength = 0;

    for (uint256 i; i < buffer.length; ++i) {
        bytes1 char = buffer[i]; 

        if (((SPECIAL_CHARS_LOOKUP & (1 << uint8(char))) != 0)) {
            output[outputLength++] = '\\';
            if (char == 0x08) output[outputLength++] = 'b';
            else if (char == 0x09) output[outputLength++] = 't';
            else if (char == 0x0A) output[outputLength++] = 'n';
            else if (char == 0x0C) output[outputLength++] = 'f';
            else if (char == 0x0D) output[outputLength++] = 'r';
            else if (char == 0x22) output[outputLength++] = '"';
            else if (char == 0x2F) output[outputLength++] = '/';
            else if (char == 0x5C) output[outputLength++] = '\\';
        } else {
            output[outputLength++] = char;
        }
    }
    assembly ("memory-safe") {
        // write the actual length
        mstore(output, outputLength) 
        // deallocate unused memory
        mstore(0x40, add(output, shl(5, shr(5, add(outputLength, 63)))))
    }

    return string(output);
}

Note that we need testing ! For that the function has to be internal.

Amxx · 2025-02-18T09:15:36Z

(closed by missclick)

Amxx · 2025-02-18T09:57:16Z

TODO:

testing: using JSON.stringify and removing the first and last caracteres (") almost works. It does NOT escape \
linting: prettier wants use to use '"', but solhint wants "\"".

DarkLord017 · 2025-02-18T12:22:15Z

okay will do

TODO:

testing: using JSON.stringify and removing the first and last caracteres (") almost works. It does NOT escape \

linting: prettier wants use to use '"', but solhint wants "\"".

contracts/utils/Strings.sol

socket-security · 2025-02-20T13:59:08Z

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

…Zeppelin#5251)

DarkLord017 · 2025-02-21T06:17:18Z

U can review it now @Amxx

contracts/utils/Strings.sol

Amxx · 2025-02-24T20:15:36Z

Is it correct to drop the forward slash escaping?

test/utils/Strings.test.js

DarkLord017 · 2025-02-26T09:26:41Z

Is it correct to drop the forward slash escaping?

I read this then removed that
https://stackoverflow.com/questions/1580647/json-why-are-forward-slashes-escaped

arr00 · 2025-02-28T21:04:30Z

We should have a comment that this can only be used inside double quote strings. Single quotes are not escaped.

ernestognw

Technically correct, left some minor suggestions. I would prefer merging rather than addressing those.

contracts/utils/Strings.sol

ernestognw · 2025-03-02T14:54:43Z

contracts/utils/Strings.sol

+        // write the actual length and deallocate unused memory
+        assembly ("memory-safe") {
+            mstore(output, outputLength)
+            mstore(0x40, add(output, shl(5, shr(5, add(outputLength, 63)))))


Same here (audit will report consistency)

Suggested change

mstore(0x40, add(output, shl(5, shr(5, add(outputLength, 63)))))

mstore(64, add(output, shl(5, shr(5, add(outputLength, 63)))))

ernestognw · 2025-03-02T14:59:09Z

contracts/utils/Strings.sol

+     */
+    function escapeJSON(string memory input) internal pure returns (string memory) {
+        bytes memory buffer = bytes(input);
+        bytes memory output = new bytes(2 * buffer.length); // worst case scenario


I don't like that this is allocating worst-case scenario memory. Ideally, it should be just buffer.length, iterate and count the characters to escape, then indeed escape them. The tradeoff here is perhaps a bit more computation (i.e. more checks over) so I'm not convinced it's an actual saving.

We tested to have two loops:

one loop that counts the times escaping is needed

then we allocate exactly the right size

then we loop a second time.

It ends up being more expensive.

An alternative is to do all the allocation manually (basically we move the fmp ourselves)

…/openzeppelin-contracts into escape_functionality

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com> Co-authored-by: Arr00 <13561405+arr00@users.noreply.github.com> Signed-off-by: Hadrien Croubois <hadrien.croubois@gmail.com>

ref: added functionality to escapeJSONstrings (ref: OpenZeppelin#5251)

7eb3f6f

Amxx reviewed Feb 14, 2025

View reviewed changes

contracts/utils/Strings.sol Outdated Show resolved Hide resolved

Amxx reviewed Feb 14, 2025

View reviewed changes

contracts/utils/Strings.sol Outdated Show resolved Hide resolved

Amxx reviewed Feb 14, 2025

View reviewed changes

contracts/utils/Strings.sol Outdated Show resolved Hide resolved

DarkLord017 and others added 3 commits February 14, 2025 16:42

Apply suggestions from code review

c30dc42

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>

added size to increase while looping in memory (ref: OpenZeppelin#5251)

159822e

Merge branch 'master' into escape_functionality

7ed23f6

Amxx closed this Feb 18, 2025

Amxx reopened this Feb 18, 2025

Improve code, add tests and fix lint

59ec335

add changeset

77bb93b

Amxx reviewed Feb 19, 2025

View reviewed changes

contracts/utils/Strings.sol Outdated Show resolved Hide resolved

Update contracts/utils/Strings.sol

38601d2

DarkLord017 marked this pull request as ready for review February 20, 2025 12:47

Merge branch 'OpenZeppelin:master' into escape_functionality

5bfb4cf

DarkLord017 force-pushed the escape_functionality branch from 20e5acc to 5bfb4cf Compare February 20, 2025 14:03

Fixed linter issue and removed escaping of forward slashes (ref: Open…

c7ab6c0

…Zeppelin#5251)

Amxx reviewed Feb 24, 2025

View reviewed changes

contracts/utils/Strings.sol Outdated Show resolved Hide resolved

Amxx added 3 commits February 24, 2025 21:16

Merge branch 'master' into escape_functionality

9102cc1

minimize changes

74c3d8f

more readable with solhint disable

17186f7

Amxx reviewed Feb 24, 2025

View reviewed changes

test/utils/Strings.test.js Outdated Show resolved Hide resolved

Update test/utils/Strings.test.js

7ffa24f

Amxx previously approved these changes Feb 26, 2025

View reviewed changes

Amxx added this to the 5.3 milestone Feb 26, 2025

Amxx requested review from arr00, ernestognw and luiz-lvj February 26, 2025 10:01

add warning

ec41401

arr00 dismissed Amxx’s stale review via ec41401 March 1, 2025 18:41

arr00 previously approved these changes Mar 1, 2025

View reviewed changes

ernestognw previously approved these changes Mar 2, 2025

View reviewed changes

Amxx added 2 commits March 3, 2025 15:21

address PR comments

dba25a2

Merge branch 'escape_functionality' of https://github.com/DarkLord017…

d18d78d

…/openzeppelin-contracts into escape_functionality

Amxx dismissed stale reviews from ernestognw and arr00 via d18d78d March 3, 2025 14:22

Amxx approved these changes Mar 3, 2025

View reviewed changes

Amxx merged commit fa995ef into OpenZeppelin:master Mar 3, 2025
15 checks passed

Amxx mentioned this pull request Mar 3, 2025

Release v5.3 cherrypick #2 #5526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref: added functionality to escapeJSONstrings (ref: #5251) #5508

ref: added functionality to escapeJSONstrings (ref: #5251) #5508

DarkLord017 commented Feb 14, 2025 •

edited

Loading

changeset-bot bot commented Feb 14, 2025 •

edited

Loading

DarkLord017 commented Feb 14, 2025

Amxx Feb 14, 2025

DarkLord017 commented Feb 16, 2025

DarkLord017 commented Feb 16, 2025

Amxx commented Feb 18, 2025 •

edited

Loading

Amxx commented Feb 18, 2025 •

edited

Loading

Amxx commented Feb 18, 2025

DarkLord017 commented Feb 18, 2025

socket-security bot commented Feb 20, 2025 •

edited

Loading

DarkLord017 commented Feb 21, 2025

Amxx commented Feb 24, 2025

DarkLord017 commented Feb 26, 2025

arr00 commented Feb 28, 2025

ernestognw left a comment

ernestognw Mar 2, 2025

ernestognw Mar 2, 2025

Amxx Mar 3, 2025

	mstore(0x40, add(output, shl(5, shr(5, add(outputLength, 63)))))
	mstore(64, add(output, shl(5, shr(5, add(outputLength, 63)))))

ref: added functionality to escapeJSONstrings (ref: #5251) #5508

ref: added functionality to escapeJSONstrings (ref: #5251) #5508

Conversation

DarkLord017 commented Feb 14, 2025 • edited Loading

PR Checklist

changeset-bot bot commented Feb 14, 2025 • edited Loading

🦋 Changeset detected

DarkLord017 commented Feb 14, 2025

Amxx Feb 14, 2025

Choose a reason for hiding this comment

DarkLord017 commented Feb 16, 2025

DarkLord017 commented Feb 16, 2025

Amxx commented Feb 18, 2025 • edited Loading

Amxx commented Feb 18, 2025 • edited Loading

Amxx commented Feb 18, 2025

DarkLord017 commented Feb 18, 2025

socket-security bot commented Feb 20, 2025 • edited Loading

DarkLord017 commented Feb 21, 2025

Amxx commented Feb 24, 2025

DarkLord017 commented Feb 26, 2025

arr00 commented Feb 28, 2025

ernestognw left a comment

Choose a reason for hiding this comment

ernestognw Mar 2, 2025

Choose a reason for hiding this comment

ernestognw Mar 2, 2025

Choose a reason for hiding this comment

Amxx Mar 3, 2025

Choose a reason for hiding this comment

DarkLord017 commented Feb 14, 2025 •

edited

Loading

changeset-bot bot commented Feb 14, 2025 •

edited

Loading

Amxx commented Feb 18, 2025 •

edited

Loading

Amxx commented Feb 18, 2025 •

edited

Loading

socket-security bot commented Feb 20, 2025 •

edited

Loading