Add metadata escaping safety baseline#87
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds on-chain JSON string escaping for name/description/image/animation_url, enforces raw-attribute structural validation in updateImagesAndAttributes (rejecting control characters, unterminated strings, mismatched delimiters, and breakout fragments), adds tests that decode base64 tokenURIs and assert escaped JSON parses, and updates ADR/docs/ops to record implemented and remaining P1-META-006 items. ChangesMetadata Escaping and Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review Please review this first P1-META-006 slice on the latest head. Scope note: this PR intentionally handles JSON string escaping and raw attribute structural breakout guards only; generated HTML/JavaScript escaping or rejection, semantic attribute schema validation/structured attributes, URI policy, invalid UTF-8 policy, numeric size limits, and render-sandbox tests remain documented follow-up work under #51. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR Info ==="
gh pr view 87 --repo 6529-Collections/6529Stream --json title,body,files,baseRefName,headRefName | jq '{title,baseRefName,headRefName,files:[.files[].path]}'
echo ""
echo "=== Changed Files ==="
gh pr diff 87 --repo 6529-Collections/6529Stream 2>/dev/null | head -3000Length of output: 49316 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Check for assembly patterns or existing bytes helpers in StreamCore ==="
rg -n "assembly|mstore|mload|_truncate\|_hex" smart-contracts/StreamCore.sol | head -40
echo ""
echo "=== Full _escapeJsonString context including _truncateBytes ==="
rg -n "_truncateBytes\|_hexNibble\|_escapeJson\|_requireSafe\|_advanceRaw" smart-contracts/StreamCore.sol
echo ""
echo "=== Check for any use of LibString, Base64, or similar helpers ==="
rg -n "import\|using\|LibString\|Base64\|StringUtils" smart-contracts/StreamCore.sol | head -20
echo ""
echo "=== Gas: how is tokenURI typically called - view or write? ==="
rg -n "tokenURI\|onchain_animation\|_buildOnchainJson" smart-contracts/StreamCore.sol | head -30
echo ""
echo "=== Check existing tests for whitespace-only or empty attributes ==="
rg -n 'attributes.*""' test/ --include="*.sol" | head -20
echo ""
echo "=== Verify _advanceRawAttributeTopLevelSeparator whitespace handling ==="
rg -n "0x20\|0x09\|0x0a\|0x0d" smart-contracts/StreamCore.sol | head -40Length of output: 1366 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Assembly or low-level bytes patterns ==="
grep -n "assembly\|mstore\|mload" smart-contracts/StreamCore.sol | head -20 || echo "none found"
echo ""
echo "=== Helper function names added in this PR ==="
grep -n "truncateBytes\|hexNibble\|escapeJson\|requireSafe\|advanceRaw" smart-contracts/StreamCore.sol | head -40
echo ""
echo "=== Imports and external libraries ==="
grep -n "^import\|^using" smart-contracts/StreamCore.sol | head -20
echo ""
echo "=== tokenURI function signature ==="
grep -n "function tokenURI\|function _buildOnchain\|function _onchain" smart-contracts/StreamCore.sol | head -20
echo ""
echo "=== Empty/whitespace attributes in tests ==="
grep -rn 'attributes' test/ | grep '""' | head -20 || echo "none found"
echo ""
echo "=== Depth==0 whitespace handling in validator (0x20 checks) ==="
grep -n "0x20\|character != " smart-contracts/StreamCore.sol | head -30
echo ""
echo "=== Line count for the full StreamCore.sol ==="
wc -l smart-contracts/StreamCore.solLength of output: 3693
Review: PR
|
| Area | Finding | Severity |
|---|---|---|
_escapeJsonString |
Correct per RFC 8259; all named escapes + \u00XX fallback |
✅ |
_hexNibble |
Correct lowercase hex output | ✅ |
_truncateBytes |
O(n) loop; style-consistent with codebase; view-only path |
ℹ️ Minor |
_requireSafeRawAttributes control-char ordering |
Checked before inString branch — correct |
✅ |
| Terminal state revert conditions | All four termination edge cases covered | ✅ |
Mismatched brackets {] |
Intentional by design; documented | ℹ️ By design |
| Test: empty attributes positive path | Missing; low regression risk but worth adding | ℹ️ Minor |
| Test: multi-item positive path | Missing; comma-separator logic not exercised positively | ℹ️ Minor |
| Docs/traceability | Consistent across all changed docs | ✅ |
The implementation is sound for the stated P1-META-006 first slice. The two suggested tests are the only non-blocking gaps I found.
|
@coderabbitai review Follow-up pushed at |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@smart-contracts/StreamCore.sol`:
- Around line 181-187: The parser currently only tracks nesting depth in
RawAttributeValidationState and therefore treats '}' and ']' interchangeably;
update the state and logic in _requireSafeRawAttributes to record the container
kind per nesting level (e.g., push/pop a stack of container types like OBJECT vs
ARRAY) and validate that closing delimiters match the most recent container kind
so mismatches like "{]" or "[}" are rejected; modify the
RawAttributeValidationState to include the stack (or a bytes/uint8[]
representing container kinds) and adjust the push on '{'/'[' and the
pop/expectation on '}'/']' accordingly throughout the parsing logic referenced
around _requireSafeRawAttributes and the related validation branches.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18c95883-70c6-477b-9dc1-92cffe63e08d
📒 Files selected for processing (10)
docs/adr/0006-metadata-freeze.mddocs/known-blockers.mddocs/metadata.mddocs/status.mdops/AUTONOMOUS_RUN.mdops/ROADMAP.mdsmart-contracts/StreamCore.soltest/README.mdtest/StreamMetadataEscaping.t.soltest/helpers/CharacterizationTestBase.sol
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Commit a8ea6db details ==="
gh api repos/6529-Collections/6529Stream/commits/a8ea6db --jq '{sha,message:.commit.message,files:[.files[].filename]}'
echo ""
echo "=== New tests in StreamMetadataEscaping.t.sol ==="
gh api repos/6529-Collections/6529Stream/commits/a8ea6db --jq '.files[] | select(.filename | contains("Escaping")) | .patch'Length of output: 3377
All gaps I flagged are resolved. The PR is good to merge from my side. ( ´･‿･｀)っ 🐇 |
|
@coderabbitai review Follow-up pushed at |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Commit 1daa794 details ==="
gh api repos/6529-Collections/6529Stream/commits/1daa794 --jq '{sha,message:.commit.message,files:[.files[].filename]}'
echo ""
echo "=== Patch for StreamCore.sol ==="
gh api repos/6529-Collections/6529Stream/commits/1daa794 --jq '.files[] | select(.filename | contains("StreamCore")) | .patch'
echo ""
echo "=== Patch for test file ==="
gh api repos/6529-Collections/6529Stream/commits/1daa794 --jq '.files[] | select(.filename | contains("Escaping")) | .patch'Length of output: 5254
Tests: Both cases in All original findings are now resolved. The PR is good to merge. ( ´･‿･｀)っ 🐇 |
1 participant
Summary
Refs #51.
This implements the first P1-META-006 metadata safety slice:
name,description,image,animation_url)]/}breakout attemptstokenURIoutput and feed it through Foundry's JSON parserThis deliberately does not close #51. Remaining P1-META-006 work includes generated HTML/JavaScript escaping or rejection, semantic attribute schema validation or structured attributes, URI policy, invalid UTF-8 policy, numeric size limits, and render-sandbox tests.
Local validation
$env:Path="$HOME\.foundry\bin;$env:Path"; forge test --match-contract StreamMetadataEscapingTest -vvvpassed with 9 tests$env:Path="$HOME\.foundry\bin;$env:Path"; forge test --match-path 'test/StreamMetadata*.t.sol' -vvvpassed with 31 metadata tests before the CodeRabbit follow-ups; the full suite includes the final 9-test escaping coverage$env:Path="$HOME\.foundry\bin;$env:Path"; make checkpassedpowershell -ExecutionPolicy Bypass -File scripts\check.ps1passedforge fmt --checkon touched Solidity files passedgit diff --checkpassed.venv-tools\Scriptsproduced the known non-zero baseline of 718 findings with high/medium unchanged at4/19CodeRabbit follow-up
CodeRabbit comment
4677192409reported the implementation correct and well-scoped, and suggested two non-blocking positive-path tests. Commita8ea6dbadds both tests and reruns full local validation.CodeRabbit inline comment
3393290836correctly flagged that the raw-attribute parser tracked nesting depth but not container type. Commit1daa794adds compact container-kind tracking, rejects mismatched{]and[}delimiters, and reruns full local validation.Notes for review
attributesis still a raw caller-authored fragment. This PR adds a structural guard to prevent obvious array/object breakout and malformed delimiter states, but it intentionally does not claim full JSON schema validation for attributes. That remains documented as follow-up #51 work.Summary by CodeRabbit
New Features
Tests
Documentation