Introduce DID validator#3701
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a session signing envelope system for Rooch blockchain to enable Bitcoin wallet compatibility while maintaining backward compatibility with existing authentication methods. The main purpose is to allow Bitcoin wallets that only support message signing (like UniSat, OKX) to sign Rooch transactions without changing the underlying signature algorithms.
Key changes include:
- Introduction of signing envelope types (RawTxHash, BitcoinMessageV0, WebAuthnV0) to decouple signature algorithms from message formatting
- Implementation of both v1 (legacy) and v2 (envelope-aware) authenticator payload formats for backward compatibility
- Addition of comprehensive test coverage for all envelope types and payload formats
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/typescript/rooch-sdk/src/crypto/index.ts | Exports new envelope-related classes and interfaces |
| sdk/typescript/rooch-sdk/src/crypto/envelope.ts | Core envelope implementation with message builders for different signing types |
| sdk/typescript/rooch-sdk/src/crypto/envelope.test.ts | Comprehensive test suite for envelope functionality |
| sdk/typescript/rooch-sdk/src/crypto/authenticator.ts | Extended authenticator to support envelope-based session creation |
| sdk/typescript/rooch-sdk/src/crypto/authenticator.test.ts | Tests for envelope integration with authenticator |
| frameworks/rooch-framework/sources/tests/session_validator_test.move | Move tests for session validator envelope functionality |
| frameworks/rooch-framework/sources/session_key.move | Added envelope constants and utility functions for canonical template building |
| frameworks/rooch-framework/sources/auth_validator/session_validator.move | Core validation logic supporting both v1 and v2 payload formats with envelope handling |
| frameworks/rooch-framework/doc/session_validator.md | Updated documentation for new structs and functionality |
| frameworks/rooch-framework/doc/session_key.md | Updated documentation for new envelope-related functions |
| docs/dev-guide/session_signing_envelope.md | Comprehensive specification document for the signing envelope system |
|
Docker images for this PR are available:
Pull commands:
|
db60da6 to
97b4993
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| export function varintByteNum(input: number): Bytes { | ||
| if (input < 253) { | ||
| if (input <= 0xfc) { | ||
| // Changed from < 253 to <= 252 (0xFC) for clarity |
There was a problem hiding this comment.
The comment states '252 (0xFC)' but 0xFC equals 252 in decimal, not the boundary value. The condition should be documented as '<= 252' or '<= 0xFC' without the redundant decimal conversion.
| // Changed from < 253 to <= 252 (0xFC) for clarity | |
| // For input <= 0xFC (252), encode as a single byte |
| // Convert back to bytes | ||
| let sHex = sBig.toString(16) | ||
| if (sHex.length % 2 === 1) sHex = '0' + sHex | ||
| s = new Uint8Array(sHex.match(/.{1,2}/g)!.map((byte) => parseInt(byte, 16))) |
There was a problem hiding this comment.
Using non-null assertion operator (!) on regex match result is unsafe. The regex could return null for empty strings, causing a runtime error. Add null check: const matches = sHex.match(/.{1,2}/g); if (!matches) throw new Error('Invalid hex string'); return new Uint8Array(matches.map(...))
| s = new Uint8Array(sHex.match(/.{1,2}/g)!.map((byte) => parseInt(byte, 16))) | |
| const matches = sHex.match(/.{1,2}/g); | |
| if (!matches) throw new Error('Invalid hex string in canonicalizeS'); | |
| s = new Uint8Array(matches.map((byte) => parseInt(byte, 16))); |
| assert!( | ||
| auth_payload.envelope == ENVELOPE_RAW_TX_HASH || | ||
| auth_payload.envelope == ENVELOPE_BITCOIN_MESSAGE_V0 || | ||
| auth_payload.envelope == ENVELOPE_WEBAUTHN_V0, | ||
| ErrorInvalidEnvelopeType | ||
| ); |
There was a problem hiding this comment.
[nitpick] The envelope validation uses multiple OR conditions which is error-prone for maintenance. Consider using a helper function like is_valid_envelope(envelope: u8): bool or a vector of valid envelopes for easier extension and better readability.
| const ErrorValidateFunctionCallBeyondSessionScope: u64 = 1013; | ||
|
|
||
| /// DID VM fragment encoding prefix | ||
| const DID_VM_FRAGMENT_PREFIX: vector<u8> = b"DID_VM:"; |
There was a problem hiding this comment.
The prefix 'DID_VM:' might collide with legitimate session key data. Consider using a more unique prefix with special characters or length that's unlikely to occur in real session keys, such as '\x00DID_VM\x00' or increasing the prefix length.
| const DID_VM_FRAGMENT_PREFIX: vector<u8> = b"DID_VM:"; | |
| const DID_VM_FRAGMENT_PREFIX: vector<u8> = b"\x00DID_VM\x00"; |
| console.log('[DIDAuthenticator] DIDAuthPayload:', { | ||
| envelope, | ||
| vmFragment, | ||
| signature: toHEX(signature), | ||
| message: message ? String.fromCharCode(...message) : null, | ||
| }) |
There was a problem hiding this comment.
Logging sensitive data like signatures and messages to console can expose private information. Consider removing this debug log or protecting it with a development-only flag to prevent information leakage in production.
| console.log('[DIDAuthenticator] DIDAuthPayload:', { | |
| envelope, | |
| vmFragment, | |
| signature: toHEX(signature), | |
| message: message ? String.fromCharCode(...message) : null, | |
| }) | |
| if (process.env.NODE_ENV === 'development') { | |
| console.log('[DIDAuthenticator] DIDAuthPayload:', { | |
| envelope, | |
| vmFragment, | |
| signature: toHEX(signature), | |
| message: message ? String.fromCharCode(...message) : null, | |
| }) | |
| } |
|
|
||
| // Compare byte arrays manually | ||
| if (actualChallenge.length !== txHash.length) { | ||
| console.error('[WebAuthnUtils.validateChallenge] Length mismatch:', { |
There was a problem hiding this comment.
Using console.error for validation failures might spam logs and expose internal state. Consider using a proper logging framework with configurable levels or throwing specific error types that callers can handle appropriately.
Summary
Summary about this PR