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

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

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

Conversation

DarkLord017
Copy link

@DarkLord017 DarkLord017 commented Feb 14, 2025

Fixes #5251

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)

Copy link

changeset-bot bot commented Feb 14, 2025

🦋 Changeset detected

Latest commit: 38601d2

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
Copy link
Author

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

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

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
contracts/utils/Strings.sol Outdated Show resolved Hide resolved
@DarkLord017
Copy link
Author

Screenshot 2025-02-16 at 11 37 54 AM 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
Copy link
Author

U can review it now @Amxx

@Amxx
Copy link
Collaborator

Amxx commented Feb 18, 2025

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 Amxx closed this Feb 18, 2025
@Amxx
Copy link
Collaborator

Amxx commented Feb 18, 2025

(closed by missclick)

@Amxx Amxx reopened this Feb 18, 2025
@Amxx
Copy link
Collaborator

Amxx commented Feb 18, 2025

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
Copy link
Author

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 Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for escaping JSON to Strings lib
2 participants