From 7eb3f6f5ba1cfa1bd146465e873e9acab11214d4 Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Fri, 14 Feb 2025 13:49:17 +0530 Subject: [PATCH 01/10] ref: added functionality to escapeJSONstrings (ref: #5251) --- contracts/utils/Strings.sol | 47 +++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/contracts/utils/Strings.sol b/contracts/utils/Strings.sol index 9e5f1877b99..b77cf913a30 100644 --- a/contracts/utils/Strings.sol +++ b/contracts/utils/Strings.sol @@ -15,6 +15,15 @@ library Strings { bytes16 private constant HEX_DIGITS = "0123456789abcdef"; uint8 private constant ADDRESS_LENGTH = 20; + uint256 constant SPECIAL_CHARS_LOOKUP = + (1 << 8) | // backspace + (1 << 9) | // tab + (1 << 10) | // newline + (1 << 12) | // form feed + (1 << 13) | // carriage return + (1 << 34) | // double quote + (1 << 47) | // forward slash + (1 << 92); // backslash /** * @dev The `value` string doesn't fit in the specified `length`. @@ -438,4 +447,42 @@ library Strings { value := mload(add(buffer, add(0x20, offset))) } } + + 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 + uint256 outputLength = 0; + + for (uint256 i; i < buffer.length; ) { + 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; + } + + unchecked { ++i; } + } + + // Trim the output to the correct length + bytes memory finalOutput = new bytes(outputLength); + for (uint256 i = 0; i < outputLength; i++) { + finalOutput[i] = output[i]; + } + + return string(finalOutput); +} + + + } From c30dc427eb14a4dc4e83ef3d6e43a952a9b341e9 Mon Sep 17 00:00:00 2001 From: Sambhav Jain <136801346+DarkLord017@users.noreply.github.com> Date: Fri, 14 Feb 2025 16:42:38 +0530 Subject: [PATCH 02/10] Apply suggestions from code review Co-authored-by: Hadrien Croubois --- contracts/utils/Strings.sol | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/contracts/utils/Strings.sol b/contracts/utils/Strings.sol index b77cf913a30..3a32a43f405 100644 --- a/contracts/utils/Strings.sol +++ b/contracts/utils/Strings.sol @@ -15,15 +15,15 @@ library Strings { bytes16 private constant HEX_DIGITS = "0123456789abcdef"; uint8 private constant ADDRESS_LENGTH = 20; - uint256 constant SPECIAL_CHARS_LOOKUP = - (1 << 8) | // backspace - (1 << 9) | // tab - (1 << 10) | // newline - (1 << 12) | // form feed - (1 << 13) | // carriage return - (1 << 34) | // double quote - (1 << 47) | // forward slash - (1 << 92); // backslash + uint256 private constant SPECIAL_CHARS_LOOKUP = + (1 << 0x08) | // backspace + (1 << 0x09) | // tab + (1 << 0x0a) | // newline + (1 << 0x0c) | // form feed + (1 << 0x0d) | // carriage return + (1 << 0x22) | // double quote + (1 << 0x2f) | // forward slash + (1 << 0x5c); // backslash /** * @dev The `value` string doesn't fit in the specified `length`. From 159822e968fa6a7721b06d69bb12ac09b1e93790 Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Sun, 16 Feb 2025 11:32:30 +0530 Subject: [PATCH 03/10] added size to increase while looping in memory (ref: #5251) --- contracts/utils/Strings.sol | 64 ++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/contracts/utils/Strings.sol b/contracts/utils/Strings.sol index 3a32a43f405..ecce394809c 100644 --- a/contracts/utils/Strings.sol +++ b/contracts/utils/Strings.sol @@ -449,40 +449,44 @@ library Strings { } 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 - uint256 outputLength = 0; - - for (uint256 i; i < buffer.length; ) { - 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; - } + bytes memory buffer = bytes(input); + bytes memory output = new bytes(buffer.length); + uint256 outputLength = 0; - unchecked { ++i; } - } + for (uint256 i; i < buffer.length; ) { + bytes1 char = buffer[i]; - // Trim the output to the correct length - bytes memory finalOutput = new bytes(outputLength); - for (uint256 i = 0; i < outputLength; i++) { - finalOutput[i] = output[i]; - } + if (((SPECIAL_CHARS_LOOKUP & (1 << uint8(char))) != 0)) { - return string(finalOutput); -} + if (outputLength >= buffer.length - 1) { + assembly { + mstore(output, add(outputLength, 2)) + mstore(0x40, add(add(output, 0x20), mul(div(add(add(outputLength, 2), 31), 32), 32))) + } + } + 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 { + if (outputLength >= buffer.length) { + assembly { + mstore(output, add(outputLength, 1)) + mstore(0x40, add(add(output, 0x20), mul(div(add(add(outputLength, 1), 31), 32), 32))) + } + } + output[outputLength++] = char; + } + ++i; + } + return string(output); + } } From 59ec3356fc5c3d92272ba0550b08d2488c143ee1 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 18 Feb 2025 10:55:46 +0100 Subject: [PATCH 04/10] Improve code, add tests and fix lint --- contracts/utils/Strings.sol | 90 +++++++++++++++++-------------------- test/utils/Strings.test.js | 7 +++ 2 files changed, 48 insertions(+), 49 deletions(-) diff --git a/contracts/utils/Strings.sol b/contracts/utils/Strings.sol index ecce394809c..eaecd2fd754 100644 --- a/contracts/utils/Strings.sol +++ b/contracts/utils/Strings.sol @@ -15,15 +15,15 @@ library Strings { bytes16 private constant HEX_DIGITS = "0123456789abcdef"; uint8 private constant ADDRESS_LENGTH = 20; - uint256 private constant SPECIAL_CHARS_LOOKUP = - (1 << 0x08) | // backspace - (1 << 0x09) | // tab - (1 << 0x0a) | // newline - (1 << 0x0c) | // form feed - (1 << 0x0d) | // carriage return - (1 << 0x22) | // double quote - (1 << 0x2f) | // forward slash - (1 << 0x5c); // backslash + uint256 private constant SPECIAL_CHARS_LOOKUP = + (1 << 0x08) | // backspace + (1 << 0x09) | // tab + (1 << 0x0a) | // newline + (1 << 0x0c) | // form feed + (1 << 0x0d) | // carriage return + (1 << 0x22) | // double quote + (1 << 0x2f) | // forward slash + (1 << 0x5c); // backslash /** * @dev The `value` string doesn't fit in the specified `length`. @@ -436,57 +436,49 @@ library Strings { } /** - * @dev Reads a bytes32 from a bytes array without bounds checking. - * - * NOTE: making this function internal would mean it could be used with memory unsafe offset, and marking the - * assembly block as such would prevent some optimizations. + * @dev Escape special characters in JSON strings. This can be usefull to prevent JSON injection in NFT metadata. */ - function _unsafeReadBytesOffset(bytes memory buffer, uint256 offset) private pure returns (bytes32 value) { - // This is not memory safe in the general case, but all calls to this private function are within bounds. - assembly ("memory-safe") { - value := mload(add(buffer, add(0x20, offset))) - } - } - - function _escapeJsonString(string memory input) private pure returns (string memory) { + function escapeJSON(string memory input) internal pure returns (string memory) { bytes memory buffer = bytes(input); - bytes memory output = new bytes(buffer.length); + bytes memory output = new bytes(2 * buffer.length); // worst case scenario uint256 outputLength = 0; - for (uint256 i; i < buffer.length; ) { - bytes1 char = buffer[i]; - + for (uint256 i; i < buffer.length; ++i) { + bytes1 char = buffer[i]; if (((SPECIAL_CHARS_LOOKUP & (1 << uint8(char))) != 0)) { - - if (outputLength >= buffer.length - 1) { - assembly { - mstore(output, add(outputLength, 2)) - mstore(0x40, add(add(output, 0x20), mul(div(add(add(outputLength, 2), 31), 32), 32))) - } - } - - 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'; + 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 if (char == 0x2F) output[outputLength++] = "/"; + else if (char == 0x5C) output[outputLength++] = "\\"; } else { - if (outputLength >= buffer.length) { - assembly { - mstore(output, add(outputLength, 1)) - mstore(0x40, add(add(output, 0x20), mul(div(add(add(outputLength, 1), 31), 32), 32))) - } - } output[outputLength++] = char; } + } - ++i; + // 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))))) } + return string(output); } - + + /** + * @dev Reads a bytes32 from a bytes array without bounds checking. + * + * NOTE: making this function internal would mean it could be used with memory unsafe offset, and marking the + * assembly block as such would prevent some optimizations. + */ + function _unsafeReadBytesOffset(bytes memory buffer, uint256 offset) private pure returns (bytes32 value) { + // This is not memory safe in the general case, but all calls to this private function are within bounds. + assembly ("memory-safe") { + value := mload(add(buffer, add(0x20, offset))) + } + } } diff --git a/test/utils/Strings.test.js b/test/utils/Strings.test.js index 0b2d87190d3..6682f1d6e57 100644 --- a/test/utils/Strings.test.js +++ b/test/utils/Strings.test.js @@ -339,4 +339,11 @@ describe('Strings', function () { } }); }); + + describe('Escape JSON string', function () { + for (const input of ['', 'a', '{"a":"b/c"}', 'a\tb\nc\\d"e\rf/g\fh\bi']) + it.only(`escape ${JSON.stringify(input)}`, async function () { + await expect(this.mock.$escapeJSON(input)).to.eventually.equal(JSON.stringify(input).slice(1, -1)); + }); + }); }); From 77bb93b4e477c1dc72f5a1ceb037ac6cf86619c7 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 18 Feb 2025 10:58:25 +0100 Subject: [PATCH 05/10] add changeset --- .changeset/nice-cherries-reply.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/nice-cherries-reply.md diff --git a/.changeset/nice-cherries-reply.md b/.changeset/nice-cherries-reply.md new file mode 100644 index 00000000000..2ca3223eee1 --- /dev/null +++ b/.changeset/nice-cherries-reply.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`Strings`: Add `espaceJSON` that escapes special characters in JSON strings. From 38601d2ebe2063032c64f0770bf9633666b9ae10 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 19 Feb 2025 09:29:40 +0100 Subject: [PATCH 06/10] Update contracts/utils/Strings.sol --- contracts/utils/Strings.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/Strings.sol b/contracts/utils/Strings.sol index eaecd2fd754..09f0f349251 100644 --- a/contracts/utils/Strings.sol +++ b/contracts/utils/Strings.sol @@ -436,7 +436,7 @@ library Strings { } /** - * @dev Escape special characters in JSON strings. This can be usefull to prevent JSON injection in NFT metadata. + * @dev Escape special characters in JSON strings. This can be useful to prevent JSON injection in NFT metadata. */ function escapeJSON(string memory input) internal pure returns (string memory) { bytes memory buffer = bytes(input); From c7ab6c02c62607d92cab384ad6b093aee82cd90c Mon Sep 17 00:00:00 2001 From: DarkLord017 Date: Thu, 20 Feb 2025 19:45:18 +0530 Subject: [PATCH 07/10] Fixed linter issue and removed escaping of forward slashes (ref: #5251) --- contracts/utils/Strings.sol | 25 ++++++++++++++----------- lib/erc4626-tests | 2 +- lib/forge-std | 2 +- lib/halmos-cheatcodes | 2 +- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/contracts/utils/Strings.sol b/contracts/utils/Strings.sol index 09f0f349251..d458a48d52c 100644 --- a/contracts/utils/Strings.sol +++ b/contracts/utils/Strings.sol @@ -22,7 +22,6 @@ library Strings { (1 << 0x0c) | // form feed (1 << 0x0d) | // carriage return (1 << 0x22) | // double quote - (1 << 0x2f) | // forward slash (1 << 0x5c); // backslash /** @@ -446,20 +445,24 @@ library Strings { 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++] = "\\"; + output[outputLength++] = bytes1(uint8(0x5C)); // backslash + if (char == 0x08) + output[outputLength++] = bytes1(uint8(0x62)); // b + else if (char == 0x09) + output[outputLength++] = bytes1(uint8(0x74)); // t + else if (char == 0x0A) + output[outputLength++] = bytes1(uint8(0x6E)); // n + else if (char == 0x0C) + output[outputLength++] = bytes1(uint8(0x66)); // f + else if (char == 0x0D) + output[outputLength++] = bytes1(uint8(0x72)); // r + else if (char == 0x22) + output[outputLength++] = bytes1(uint8(0x22)); // " + else if (char == 0x5C) output[outputLength++] = bytes1(uint8(0x5C)); // \ } else { output[outputLength++] = char; } } - // write the actual length and deallocate unused memory assembly ("memory-safe") { mstore(output, outputLength) diff --git a/lib/erc4626-tests b/lib/erc4626-tests index 232ff9ba819..8b1d7c2ac24 160000 --- a/lib/erc4626-tests +++ b/lib/erc4626-tests @@ -1 +1 @@ -Subproject commit 232ff9ba8194e406967f52ecc5cb52ed764209e9 +Subproject commit 8b1d7c2ac248c33c3506b1bff8321758943c5e11 diff --git a/lib/forge-std b/lib/forge-std index 3b20d60d14b..1eea5bae12a 160000 --- a/lib/forge-std +++ b/lib/forge-std @@ -1 +1 @@ -Subproject commit 3b20d60d14b343ee4f908cb8079495c07f5e8981 +Subproject commit 1eea5bae12ae557d589f9f0f0edae2faa47cb262 diff --git a/lib/halmos-cheatcodes b/lib/halmos-cheatcodes index 7328abe1004..c0d865508c0 160000 --- a/lib/halmos-cheatcodes +++ b/lib/halmos-cheatcodes @@ -1 +1 @@ -Subproject commit 7328abe100445fc53885c21d0e713b95293cf14c +Subproject commit c0d865508c0fee0a11b97732c5e90f9cad6b65a5 From 74c3d8f92e243ba62d42eb2f81aad5d5301307fd Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 24 Feb 2025 21:17:35 +0100 Subject: [PATCH 08/10] minimize changes --- lib/erc4626-tests | 2 +- lib/forge-std | 2 +- lib/halmos-cheatcodes | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/erc4626-tests b/lib/erc4626-tests index 8b1d7c2ac24..232ff9ba819 160000 --- a/lib/erc4626-tests +++ b/lib/erc4626-tests @@ -1 +1 @@ -Subproject commit 8b1d7c2ac248c33c3506b1bff8321758943c5e11 +Subproject commit 232ff9ba8194e406967f52ecc5cb52ed764209e9 diff --git a/lib/forge-std b/lib/forge-std index 1eea5bae12a..3b20d60d14b 160000 --- a/lib/forge-std +++ b/lib/forge-std @@ -1 +1 @@ -Subproject commit 1eea5bae12ae557d589f9f0f0edae2faa47cb262 +Subproject commit 3b20d60d14b343ee4f908cb8079495c07f5e8981 diff --git a/lib/halmos-cheatcodes b/lib/halmos-cheatcodes index c0d865508c0..7328abe1004 160000 --- a/lib/halmos-cheatcodes +++ b/lib/halmos-cheatcodes @@ -1 +1 @@ -Subproject commit c0d865508c0fee0a11b97732c5e90f9cad6b65a5 +Subproject commit 7328abe100445fc53885c21d0e713b95293cf14c From 17186f7f8e1868cb6981d7fd0d94f09364132fbc Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 24 Feb 2025 21:44:23 +0100 Subject: [PATCH 09/10] more readable with solhint disable --- contracts/utils/Strings.sol | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/contracts/utils/Strings.sol b/contracts/utils/Strings.sol index d458a48d52c..7fa6f778aad 100644 --- a/contracts/utils/Strings.sol +++ b/contracts/utils/Strings.sol @@ -445,20 +445,17 @@ library Strings { for (uint256 i; i < buffer.length; ++i) { bytes1 char = buffer[i]; if (((SPECIAL_CHARS_LOOKUP & (1 << uint8(char))) != 0)) { - output[outputLength++] = bytes1(uint8(0x5C)); // backslash - if (char == 0x08) - output[outputLength++] = bytes1(uint8(0x62)); // b - else if (char == 0x09) - output[outputLength++] = bytes1(uint8(0x74)); // t - else if (char == 0x0A) - output[outputLength++] = bytes1(uint8(0x6E)); // n - else if (char == 0x0C) - output[outputLength++] = bytes1(uint8(0x66)); // f - else if (char == 0x0D) - output[outputLength++] = bytes1(uint8(0x72)); // r - else if (char == 0x22) - output[outputLength++] = bytes1(uint8(0x22)); // " - else if (char == 0x5C) output[outputLength++] = bytes1(uint8(0x5C)); // \ + 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 == 0x5C) output[outputLength++] = "\\"; + else if (char == 0x22) { + // solhint-disable-next-line quotes + output[outputLength++] = '"'; + } } else { output[outputLength++] = char; } From 7ffa24f1008ff8c45dd1251f1fd60f9873230009 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 24 Feb 2025 21:54:22 +0100 Subject: [PATCH 10/10] Update test/utils/Strings.test.js --- test/utils/Strings.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/utils/Strings.test.js b/test/utils/Strings.test.js index 6682f1d6e57..8d5e2401ab1 100644 --- a/test/utils/Strings.test.js +++ b/test/utils/Strings.test.js @@ -342,7 +342,7 @@ describe('Strings', function () { describe('Escape JSON string', function () { for (const input of ['', 'a', '{"a":"b/c"}', 'a\tb\nc\\d"e\rf/g\fh\bi']) - it.only(`escape ${JSON.stringify(input)}`, async function () { + it(`escape ${JSON.stringify(input)}`, async function () { await expect(this.mock.$escapeJSON(input)).to.eventually.equal(JSON.stringify(input).slice(1, -1)); }); });