Add Foundry fuzz tests for Strings.escapeJSON#6466
Add Foundry fuzz tests for Strings.escapeJSON#6466Sandra-Cai wants to merge 4 commits intoOpenZeppelin:masterfrom
Conversation
The Foundry test file for Strings had no coverage for escapeJSON, which was recently modified to handle control character escaping. Two fuzz tests are added: - testEscapeJSONLength: output is never shorter than input - testEscapeJSON: validates the output is well-formed JSON string content (no raw control chars, no unescaped quotes, every backslash starts a valid escape sequence) Made-with: Cursor
🦋 Changeset detectedLatest commit: d14ffea The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
WalkthroughTwo new test functions were added to the Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/utils/Strings.t.sol`:
- Around line 69-75: The test currently checks for the "\u00" prefix but doesn't
validate the two hex digits that follow; update the branch where next == 0x75 to
(1) ensure the buffer has both hex nibbles present (use assertLt(i + 5,
escaped.length) or equivalent) and (2) add assertions that escaped[i + 3] and
escaped[i + 4] are valid hex characters by testing each byte against ranges
'0'-'9', 'A'-'F', and 'a'-'f' (use uint8 comparisons), keeping references to the
same variables (escaped, i, next) so the test fails on malformed Unicode
escapes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: caa982c8-e9ad-4a8d-bc2b-e2865b713ab7
📒 Files selected for processing (1)
test/utils/Strings.t.sol
| if (next == 0x75) { | ||
| // \u00XX | ||
| assertLt(i + 4, escaped.length); | ||
| assertEq(uint8(escaped[i + 1]), 0x30); | ||
| assertEq(uint8(escaped[i + 2]), 0x30); | ||
| i += 4; | ||
| } else { |
There was a problem hiding this comment.
Validate the final XX bytes in \u00XX are hex digits.
Line 72-74 verifies \u00 shape but does not verify that the last two bytes are [0-9a-fA-F]. This weakens the “well-formed JSON escape” property and can miss malformed unicode escapes.
Suggested patch
if (next == 0x75) {
// \u00XX
assertLt(i + 4, escaped.length);
assertEq(uint8(escaped[i + 1]), 0x30);
assertEq(uint8(escaped[i + 2]), 0x30);
+ uint8 hi = uint8(escaped[i + 3]);
+ uint8 lo = uint8(escaped[i + 4]);
+ assertTrue(
+ (hi >= 0x30 && hi <= 0x39) || (hi >= 0x41 && hi <= 0x46) || (hi >= 0x61 && hi <= 0x66)
+ );
+ assertTrue(
+ (lo >= 0x30 && lo <= 0x39) || (lo >= 0x41 && lo <= 0x46) || (lo >= 0x61 && lo <= 0x66)
+ );
i += 4;
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (next == 0x75) { | |
| // \u00XX | |
| assertLt(i + 4, escaped.length); | |
| assertEq(uint8(escaped[i + 1]), 0x30); | |
| assertEq(uint8(escaped[i + 2]), 0x30); | |
| i += 4; | |
| } else { | |
| if (next == 0x75) { | |
| // \u00XX | |
| assertLt(i + 4, escaped.length); | |
| assertEq(uint8(escaped[i + 1]), 0x30); | |
| assertEq(uint8(escaped[i + 2]), 0x30); | |
| uint8 hi = uint8(escaped[i + 3]); | |
| uint8 lo = uint8(escaped[i + 4]); | |
| assertTrue( | |
| (hi >= 0x30 && hi <= 0x39) || (hi >= 0x41 && hi <= 0x46) || (hi >= 0x61 && hi <= 0x66) | |
| ); | |
| assertTrue( | |
| (lo >= 0x30 && lo <= 0x39) || (lo >= 0x41 && lo <= 0x46) || (lo >= 0x61 && lo <= 0x66) | |
| ); | |
| i += 4; | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/utils/Strings.t.sol` around lines 69 - 75, The test currently checks for
the "\u00" prefix but doesn't validate the two hex digits that follow; update
the branch where next == 0x75 to (1) ensure the buffer has both hex nibbles
present (use assertLt(i + 5, escaped.length) or equivalent) and (2) add
assertions that escaped[i + 3] and escaped[i + 4] are valid hex characters by
testing each byte against ranges '0'-'9', 'A'-'F', and 'a'-'f' (use uint8
comparisons), keeping references to the same variables (escaped, i, next) so the
test fails on malformed Unicode escapes.
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
1 participant
The Foundry test file for Strings has fuzz tests for the parse/toString functions but nothing for escapeJSON. Given that escapeJSON was recently changed (#6344) and does nontrivial bit manipulation with raw memory writes, it seems like a good candidate for property-based testing.
I added two fuzz tests. The first one just checks that escaping never shrinks the string. The second one is the interesting one, it walks the output and checks that it's actually valid JSON string content: no raw control chars leaked through, no bare double quotes, and every backslash is the start of a real escape sequence (\b \t \n \f \r \ " or \u00XX). Both pass with 5k runs.