fix(ICP_Rosetta): DEFI-3005: Bind displayed request metadata to the signed payload - #11616
Draft
mbjorkqvist wants to merge 2 commits into
Draft
mbjorkqvist wants to merge 2 commits into
mbjorkqvist wants to merge 2 commits into
Conversation
…igned payload The `RequestType` carried alongside each `HttpCanisterUpdate` in an unsigned or signed transaction is plain CBOR metadata that no signature covers, while the update's canister id, method name, argument, nonce and sender are all covered by `update.id()`. `/construction/parse` and the submit path both took the neuron index (and, where present, the controller) from that unsigned wrapper, even though `/construction/payloads` also records the index in the signed nonce and in the neuron subaccount inside the signed argument. Require the two to agree. Every neuron request type now checks the displayed index against the signed nonce and the signed subaccount against `H(controller, neuron_index)`, which also authenticates the displayed controller on the request types that carry one. `STAKE` is bound to the signed memo, `NEURON_INFO` to the signed subaccount and `LIST_NEURONS` to the signed page number; the governance canister id and the method name are checked too, and a request that identifies its neuron by id, or that sets the legacy `id` field, is rejected rather than described unverified. Validation is shared between `/construction/parse` and the submit path's request reconstruction so the operations shown before signing, after signing and after submission cannot diverge, and it fails closed: `/construction/payloads` is the only producer of these transactions and always emits the two representations consistently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Some signed targets and envelope variants can still diverge from the operations Rosetta displays or submits.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds validation tying Rosetta neuron metadata to signed canister updates.
Changes:
- Introduces shared signed-target verification.
- Applies validation during parsing and submission.
- Adds regression tests for metadata tampering.
File summaries
| File | Description |
|---|---|
signed_target.rs |
Implements metadata verification and tests. |
request.rs |
Validates requests during submission reconstruction. |
construction_parse.rs |
Validates parsed transactions and adds regression coverage. |
lib.rs |
Exposes the verification module. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ot just the neuron Addresses review findings on the previous commit, which bound the neuron a request targets but left three ways for the displayed operations to still describe something other than what gets submitted. A signed request carries one envelope per ingress expiry, each independently signed, and the submit path broadcasts whichever is currently valid rather than the first. Validating one envelope was therefore not enough: a request could pair an expired envelope matching the wrapper with a currently valid envelope for a different neuron, amount or recipient. Require every envelope to be the same update as the one described, differing only in ingress expiry -- which is exactly what `/construction/combine` produces, and which covers divergence in any payload field rather than an enumerated few. Bind the signed command to the operation the wrapper names. Parsing re-decoded the command in each of its per-operation handlers, but the submit path reports `START_DISSOLVE` and `STOP_DISSOLVE` from the wrapper without decoding the payload at all, so rewriting only that variant made submit name one operation while another executed. Submit is reachable without parsing first. Bind the controller in a signed stake request: naming a third party claims `H(controller, memo)` rather than the signer's neuron, while the request was still displayed against the signer. Also drop a doc comment left stranded on the extracted test helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The security-sensitive validation spans multiple governance command formats and transaction paths, warranting final human review despite no confirmed defects.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rosetta's construction flow pairs each signable
HttpCanisterUpdatewith aRequestTypethat travels in the transaction wrapper. The update is covered bythe caller's signature; the wrapper is not.
/construction/parseand the submitpath nonetheless read the neuron index — and, where present, the controller —
from the wrapper, so the operations Rosetta reports were not necessarily a
description of the payload being signed and broadcast.
They are now. Every neuron request type must show metadata that is derivable
from and consistent with the signed update, and a request that fails the check
is rejected rather than described unverified. The validation is shared between
/construction/parseand the submit path's request reconstruction, so theoperations shown before signing, after signing, and after submission cannot
disagree.
This matters most for a deployment that verifies intent on a separate offline
instance, and for the commands whose value is unbounded: a full-stake
DISBURSEand a 100%DISBURSE_MATURITYreport no amount, so the neuron theoperation names is the only indication of how much value moves.
Tracked as DEFI-3005.
🤖 Generated with Claude Code