feat: add non-custodial integration test and Snap docs (#135)#155
feat: add non-custodial integration test and Snap docs (#135)#155salindne wants to merge 1 commit intofeature/111-d-registrationfrom
Conversation
Integration test script (test-prepare-execute.go) covering: - Happy path: register external user, mint, prepare, sign, execute - Expired transfer (past TTL → 410) - Replay prevention (double execute → 404) - Wrong fingerprint (→ 403) - Custodial rejection (→ 400) Add docs/non-custodial-snap-testing.md with complete Snap integration guide: cryptographic requirements, API reference, error tables, RPC methods, JavaScript dapp example, and curl testing commands. Update e2e-local config with test user addresses.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly advances the non-custodial signing feature by providing robust integration testing and thorough documentation. It ensures the two-step transfer flow for external signers functions correctly under various conditions and offers a complete guide for developers to integrate MetaMask Snaps, thereby enhancing the system's security and external wallet compatibility. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an integration test for the non-custodial transfer flow and adds comprehensive documentation for MetaMask Snap integration. However, the changes reveal a critical security vulnerability in the registration process: the registration endpoints lack replay protection and message content validation. This could allow an attacker to hijack a user's account by associating a malicious Canton key with their EVM address using any valid signature from that user. This issue must be remediated in the API server implementation and the documentation updated accordingly. Additionally, I've provided a couple of suggestions to improve clarity in both the new documentation and the test script.
|
|
||
| The server rejects messages where the timestamp differs from server time by more than **5 minutes**. | ||
|
|
||
| Registration endpoints (`/register/*`) do **not** enforce this format. |
There was a problem hiding this comment.
The documentation explicitly states that "Registration endpoints (/register/*) do not enforce this format," referring to the timed message format {action}:{unix_timestamp_seconds} used for replay protection in other endpoints. This indicates a critical security flaw in the registration process.
Without enforcing a specific message format and a timestamp (replay protection), the registration endpoints are vulnerable to authentication bypass. An attacker who obtains any valid EIP-191 signature from a user (e.g., a signature for a login message on a different service) can use that signature and message to register an arbitrary canton_public_key to the user's EVM address. Once a Canton key is registered, the attacker can use it to sign and execute non-custodial transfers on behalf of the user, leading to a total loss of funds.
Recommendation:
- Modify the server implementation (specifically in
pkg/user/service/http.go) to enforce the timed message format for all registration endpoints. - Validate that the message content is specific to the registration action and includes the fingerprint of the Canton public key being registered (e.g.,
register-external:{fingerprint}:{unix_timestamp_seconds}). - Update this documentation to reflect the mandatory security requirements.
| } | ||
| ``` | ||
|
|
||
| Store the `fingerprint` — it's needed for execute requests (as `signed_by`). The `public_key_fingerprint` from step 1a is the Canton key fingerprint used for `signed_by`. |
There was a problem hiding this comment.
The explanation here is a bit confusing. It first mentions storing the fingerprint from the /register response for use in signed_by, but then correctly clarifies that it's the public_key_fingerprint from the /register/prepare-topology response that should be used. This could lead to confusion for developers implementing a client.
To improve clarity, I suggest rephrasing this to distinguish between the two fingerprint fields more clearly. For example:
"The public_key_fingerprint returned from the prepare-topology step (1a) must be used as the signed_by field in the execute request (step 2b). Note that the register response (1b) also contains a fingerprint field, but this is an internal user identifier and should not be used for signing."
| _ = hash // SignHashDER expects the pre-hashed 32 bytes | ||
| derSig, err := kp.SignHashDER(hash[:]) |
There was a problem hiding this comment.
The line _ = hash is a no-op since hash is used on the next line. It appears to be a leftover from development and can be removed to improve code clarity.
| _ = hash // SignHashDER expects the pre-hashed 32 bytes | |
| derSig, err := kp.SignHashDER(hash[:]) | |
| derSig, err := kp.SignHashDER(hash[:]) |
1 participant
Summary
Sub-issue E (#135) of the non-custodial signing feature (#111).
scripts/testing/test-prepare-execute.go):docs/non-custodial-snap-testing.md):Test plan
go build ./...— compiles./scripts/testing/bootstrap-local.sh --cleango run scripts/testing/test-prepare-execute.go -config config.docker.yaml -test-config config.e2e-test.yaml -local -skip-dockergo run scripts/testing/interop-demo.goStacked PR 4/4 for #111 — depends on #154