small changes#996
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThis PR consolidates NFT URI validation by extracting a hardcoded 512-byte limit to a shared constant, standardizes key padding logic across ed25519 and secp256k1 keypair implementations, and includes minor documentation updates to Python environment setup instructions. ChangesNFT URI Consolidation and Keypairs Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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 `@xrpl/constants.py`:
- Around line 85-88: The docstring for the constant MAX_NFT_URI_LENGTH is
incorrect about units: update the docstring for MAX_NFT_URI_LENGTH to state that
512 is a hex-character limit (not bytes) and optionally note it corresponds to
256 bytes (256 bytes × 2 hex chars/byte) to match the XRPL spec and downstream
error messages; locate the constant MAX_NFT_URI_LENGTH in xrpl/constants.py and
replace the "in bytes" wording with "in hex characters (corresponds to 256
bytes)".
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e8676709-0e2d-4f25-8f7f-0371d36a8020
📒 Files selected for processing (7)
CONTRIBUTING.mdxrpl/constants.pyxrpl/core/keypairs/ed25519.pyxrpl/core/keypairs/helpers.pyxrpl/core/keypairs/secp256k1.pyxrpl/models/transactions/nftoken_mint.pyxrpl/models/transactions/nftoken_modify.py
|
@ledhed2222 Looks like there are some changes that modify actual source code. Can you elaborate in the PR description about context of each change and it's purpose. |
|
@Patel-Raj11 yes i'm mostly putting the nft max length in a shared location so that also note that i'm editing code that i wrote originally mostly ;) |
![semgrep-companion-app[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
| """ | ||
| sha_hash = hashlib.sha256(public_key).digest() | ||
| return bytes(RIPEMD160.new(sha_hash).digest()) | ||
| return bytes(RIPEMD160.new(hashlib.sha256(public_key).digest()).digest()) |
There was a problem hiding this comment.
Semgrep identified an issue in your code:
RIPEMD160 hash algorithm used in get_account_id is cryptographically broken and vulnerable to collision attacks that could allow account ID spoofing.
More details about this
The get_account_id function uses RIPEMD160, a cryptographic hash algorithm that has known weaknesses and is no longer considered secure for protecting sensitive data.
Exploit scenario:
- An attacker collects multiple public keys and their corresponding account IDs generated by this function.
- The attacker exploits known collision vulnerabilities in RIPEMD160 to find two different public keys that produce the same account ID hash.
- By providing a crafted public key, the attacker can impersonate another account since the collision causes both keys to map to the same
account_id. - This allows the attacker to hijack transactions or access resources intended for the legitimate account holder.
The chained hashing (SHA-256 followed by RIPEMD160) does provide some additional protection compared to RIPEMD160 alone, but RIPEMD160's cryptographic weaknesses mean this implementation remains vulnerable to collision attacks that could compromise account identity verification.
To resolve this comment:
💡 Follow autofix suggestion
| return bytes(RIPEMD160.new(hashlib.sha256(public_key).digest()).digest()) | |
| return bytes(SHA3_512.new(hashlib.sha256(public_key).digest()).digest()) |
View step-by-step instructions
-
Check whether this
account_idformat is required to match an external protocol or network format.
If this value must remain compatible with XRPL account IDs, you cannot safely replaceRIPEMD160without also changing the protocol contract for every caller that consumes this value. -
If this hash is only used internally, replace
RIPEMD160with a modern hash from the standard library, such ashashlib.sha256(...),hashlib.sha3_256(...), orhashlib.blake2b(..., digest_size=32).
For example, change the second hash fromRIPEMD160.new(...)to ahashlibcall over the existinghashlib.sha256(public_key).digest()result. -
Remove the
import Crypto.Hash.RIPEMD160 as RIPEMD160import after you stop using it.
This keeps the function on supported hash primitives and avoids keeping insecure crypto dependencies in use. -
Update the function contract if needed, because replacing
RIPEMD160changes the output size from 20 bytes to a larger value such as 32 bytes.
If callers expect a fixed-length identifier, make that new size explicit in the docstring and in any serialization logic that stores or transmits the value. -
Alternatively, if you must keep a 20-byte internal identifier but do not need XRPL compatibility, derive it from a modern hash instead of
RIPEMD160, such as taking the first 20 bytes ofhashlib.sha256(...).digest().
This is still based on a stronger hash family, but only use this if every producer and consumer of the identifier can be updated together.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by pycryptodome-sha1-or-ripemd160.
You can view more details about this finding in the Semgrep AppSec Platform.
2 participants
High Level Overview of Change
just looking at the state of repo and noticed a few easy changes :)
Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan