feat(near): add signature verification tests + MetaMask provenance pin

[shahan-khatchadourian-anchorage]

Fourth PR of the NEAR chain-support stack (stacked on #428). Adds the signed-vector test coverage for the intents preset's verify.rs, which #428 leaves untested because render.rs needs those functions to compile.

Stack: A1 (#425) -> A2 (#426) -> A3 (#428) -> A4 (this) -> B (identity + wiring) -> C (docs site) -> D (wallet-signed token metadata).

Entirely #[cfg(test)]-scoped: the diff adds a test module to verify.rs, two fixture files, and k256 as a dev-dependency. No production code, so no production attack surface.

Coverage

raw_ed25519 (tests/fixtures/_vector_raw_ed25519.input) -- a signed vector verbatim from defuse/v0.4.2, core/src/payload/multi.rs:120. Proves defuse's canonical verification runs off-chain (pure-Rust crypto via near-sdk's non-contract-usage mode) and fails closed rather than panicking on a tampered signature. The vector's inner body carries a pre-v0.4.x "deadline":{"timestamp":...} that the pinned Deadline cannot parse, and the signature covers that body byte-for-byte, so it exercises verification only: the test asserts extraction fails with the format reason instead of discarding it, and names pipeline_decodes_and_renders_intent_section as where current-format ed25519 extraction is covered.

ERC-191 (tests/fixtures/_vector_erc191.input) -- a deterministically generated vector (fixed test key, RFC6979 signing via k256). The three verification tests read the committed fixture, so a generator regression shows up as a signature that no longer verifies rather than as a fixture that rewrites itself and still passes; build_erc191_vector() backs only the drift check. Covers a valid signature recovering exactly the signing key, and a tampered payload recovering a different key -- ERC-191 "verification" is recovery, so tampering does not fail verification, which is why account binding is the comparison against the claimed signer rather than the recovery itself.

The recovery-id guard -- near-crypto's native ecrecover panics on an out-of-range recovery id instead of returning None. Both an arbitrary out-of-range value and Ethereum's v=27 are pinned to MalformedEncoding.

Real-wallet reference vectors

Both come verbatim from the pinned dependency's own test suites, cited in-file so provenance is checkable by diffing against those files rather than by trusting this description. Both wallets signed with the same private key, so they share one reference public key.

ERC-191 / MetaMask (defuse/v0.4.2, erc191/src/lib.rs:72-95, documented there as a signature MetaMask produced) is used three ways:

• this crate's ERC-191 signing path must reproduce those exact bytes -- meaningful across implementations because MetaMask/ethereumjs and k256 both use deterministic ECDSA (RFC6979), so the same key and message must produce byte-identical output;
• those same bytes must recover the reference public key through decode_args -> verify_and_extract, so the pin covers the production verification path and not just the signing helper;
• the same signature with its v byte left as the wallet emits it (28) must report MalformedEncoding, which backs the guard with a real wallet's encoding rather than a synthetic value.

TIP-191 / TRON (defuse/v0.4.2, tip191/src/lib.rs:83-95) gets the same end-to-end recovery and guard pair. TIP-191 shares the recovery-id guard with ERC-191 and otherwise has no coverage, and since it is the same key signed under TRON's prefix, a prehash regression in either standard is caught.

The private key is a long-published throwaway test key from that external suite, not a secret generated or exposed here.

Standards covered

RawEd25519 and Erc191 (signed vectors plus the real-wallet pin), Tip191 (real-wallet pin). Not covered: Nep413 -- no upstream vector exists, so one has to be generated -- along with Sep53, TonConnect and WebAuthn.

Verification

• cargo build --locked -p visualsign-near -- clean.
• cargo test -p visualsign-near -- 106/106 pass, under both default and --features diagnostics.
• cargo clippy -p visualsign-near --all-targets -- -D warnings, cargo fmt --check -- clean.
• cargo clippy -p parser_app --no-default-features --features near --all-targets -- -D warnings -- clean (narrow-build variant).
• Review re-derived both cryptographic vectors against the pinned upstream source byte-for-byte, and traced the guard through to the panic it prevents: bypassing it panics with InvalidRecoveryId at both v=27 and v=29, while RecoveryId::from_i32 accepts 0..=3 and v in {2,3} returns Err without panicking -- so the < 4 threshold is exact, not conservative.
• No new crates enter the graph: k256 0.13.4 was already in Cargo.lock; the only lockfile change is signature 2.0.0 appearing in k256's dep list from the newly enabled signing feature.

[Copilot]

Pull request overview

Adds missing unit-test coverage for NEAR Intents signature verification (verify.rs) to ensure both native ed25519 verification and ERC-191 (secp256k1) recovery-based verification behave correctly off-chain, including a provenance pin against a published MetaMask reference signature.

Changes:

• Added a comprehensive #[cfg(test)] module for presets/intents/verify.rs, covering valid/invalid ed25519 signatures, ERC-191 recovery behavior under tampering, and invalid recovery-id handling.
• Introduced pinned fixture vectors for raw_ed25519 and deterministically generated ERC-191 signed payloads, including a self-consistency check against the generator.
• Added k256 as a dev-dependency (test-only) and updated the lockfile accordingly.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/chain_parsers/visualsign-near/src/presets/intents/verify.rs	Adds signature verification/recovery tests, deterministic ERC-191 vector generator, and MetaMask reference signature pin.
src/chain_parsers/visualsign-near/tests/fixtures/_vector_raw_ed25519.input	Adds a pinned raw_ed25519 signed test vector fixture.
src/chain_parsers/visualsign-near/tests/fixtures/_vector_erc191.input	Adds a pinned ERC-191 signed test vector fixture matching the in-test generator.
src/chain_parsers/visualsign-near/Cargo.toml	Adds k256 as a dev-dependency for deterministic secp256k1 test signing.
src/Cargo.lock	Records the added dev-dependency resolution (k256 and its transitive deps).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[prasanna-anchorage]

Built and ran this locally: 57 tests pass, clippy --all-targets -D warnings clean. The MetaMask provenance pin is a genuinely good idea -- using RFC6979 determinism to give a generated vector real-wallet equivalence without a wallet in the loop is a nice trick, and it's the kind of thing that makes the ERC-191 path trustworthy rather than merely tested. Splitting recovery-semantics (erc191_tampered_payload_recovers_a_different_key) out from pass/fail semantics is the right distinction to draw, and the comment explaining it is the clearest statement of the binding problem anywhere in the stack.

Three comments below. The first is a real behavioral gap this PR's own fixtures set up but don't close; the second is me revising something I told you on #428.

---

Stack-level pattern, folding in rather than repeating per-PR

Across #426, #428 and this one, almost every finding I've raised is one rule violated two ways:

The payload must not assert more than the code verified, and must not silently show less than what's being authorized.

Asserted-not-verified: Network: NEAR Mainnet never derived from the request (#426 -- and #430 confirms --network is accepted and ignored, so this is now a defect, not a design question) - Signature: valid where the recovered key isn't bound to the signer (#428) - Deadline computed from an enclave clock that isn't necessarily set (#428) - and comment 1 below, where signature verification failed is asserted about a signature that's cryptographically fine.

Silently omitted: mt_withdraw rendering 1 of 3 declared tokens (#428) - empty actions producing a complete-looking payload (#426) - min_gas dropped (#428).

Worth saying explicitly that you already apply this rule well in specific places -- the storage_deposit/msg regression tests, "extraction failure must not silently omit the envelope", deny_unknown_fields failing closed on ft args, the (unresolved <asset_id>) fallback. Each of those has a comment explaining why, so this isn't a knowledge gap; it's an instinct applied unevenly. My suggestion is to write it down as a crate-level invariant in presets/intents/mod.rs or the crate docs and treat it as a checklist item for the rest of the stack, rather than rediscovering it per-PR.

The place it matters most is #432: wallet-signed token metadata means attacker-influenceable decimals, and tokens.rs already carries the note that a wrong decimals "silently misrenders amounts". That's this exact failure mode with a 1377-line surface, and it's still draft -- cheapest possible moment to set the rule.

Small non-blocking note: I take the point in the description that the ERC-191 key is a long-published upstream test vector, and I agree it isn't a leak. Only practical concern is that committing a 32-byte hex private key tends to trip secret scanners; a comment linking to the upstream file it came from would save whoever triages that alert.

[prasanna-anchorage]

29 is comfortably outside every convention in play, which means this test passes without touching the one value that actually matters. The MetaMask pin below establishes that real wallets emit v = recovery_id + 27 -- you write exactly that (sig65[64] = recovery_id.to_byte() + 27) -- but nothing here tests what the parser does when a signature in that form arrives. It's the gap between the two halves of this PR.

I ran it. Same key, same message, same 64 signature bytes, only the v encoding differing:

v=0   -> Standard=erc191 | Signature=valid (recovered secp256k1:jCrfJ5wjMBroDh6Mefb1boha...)
v=27  -> Standard=erc191 | Warning=signature: signature verification failed

has_invalid_secp256k1_recovery_id rejects anything >= 4, so v=27/28 short-circuits to Invalid before verify() runs. Rejecting is defensible -- defuse's wire format is v in {0,1}, and upstream normalizes 27/28 only inside its own test helper (fix_v_in_signature in erc191/src/lib.rs), never in the production verify() path -- so the contract would reject it too.

What's wrong is the message. "signature verification failed" means the cryptography did not check out, which on a signing screen reads as tampering or fraud. Here the cryptography is perfect and the encoding convention is merely Ethereum's rather than defuse's. A reviewer seeing that on a genuine MetaMask signature would draw exactly the wrong conclusion.

Suggest distinguishing the two at the diagnostic level -- something like malformed signature encoding: recovery id 27, expected 0-3 (Ethereum v=27/28 must be normalized) -- and adding v=27 as a test case here alongside 29. Whether to normalize instead of reject is a separate call I'd leave to you; the contract's behavior is a fair argument for rejecting.

[shahan-khatchadourian-anchorage]

Fixed — SignatureCheck gained a MalformedEncoding(String) variant, distinct from Invalid. The recovery-id rejection now returns a specific reason instead of a bool, with a callout when the id is 27/28 (Ethereum's v = recovery_id + 27, what MetaMask actually emits). The old v=29 test only exercised an arbitrary out-of-range value; split into two tests now — 29 and 27 — each asserting MalformedEncoding with the right message content. Went with rejecting (not normalizing), per your note that the contract would reject it too — the fix here is purely about not mislabeling a valid signature with a wrong encoding as a failed one.

[shahan-khatchadourian-anchorage]

Moving the production half of this to #428, same reasoning as the verify.rs:193 thread above: SignatureCheck, the recovery-id guard and render_signature all live in #428, so the variant and the new render arm belong there rather than in a tests-only PR.

What went to #428 (98f448b): the MalformedEncoding(String) variant, has_invalid_secp256k1_recovery_id -> invalid_secp256k1_recovery_id_reason returning the reason, the matching render_signature arm, and a render test asserting a signer sees the reason as a warning and no Signature field claiming the check passed.

What stayed here: the tests that exercise it. The v=29 and v=27 split you asked for, plus a new one that feeds the MetaMask reference signature with its v byte exactly as the wallet emits it (28) and asserts MalformedEncoding — so the realistic case is now pinned by real wallet bytes rather than a synthetic value. TIP-191 shares this guard and had no coverage at all, so it gets the same pair using upstream's TRON reference vector.

#429 is now test-only again, which is what its security review was based on.

[prasanna-anchorage]

Revising something I said on #428. I told you the parser can't verify key-to-account binding because it has no chain access, so the fix there was wording. That's true for named accounts and wrong for both vectors in this PR -- and this line is why.

Here you derive signer_id from the public key: 0x + keccak(pubkey64)[12..]. That derivation runs in reverse just as well, so for an EVM-style signer the binding is checkable offline from the recovered key alone.

The raw_ed25519 vector has the same property. I base58-decoded its public_key:

ed25519:8rVvtHWFr8hasdQGGD5WiQBTyr4iH2ruEPPVfj491RPN
  -> 74affa71ab030d400fdfa1bed033dfa6fd3ae34f92d17c046ebe368e80d53751
fixture signer_id:
     74affa71ab030d400fdfa1bed033dfa6fd3ae34f92d17c046ebe368e80d53751   (identical)

That's a NEAR implicit account -- the account id is the hex of the ed25519 key. So both fixtures this PR adds are cases where binding is fully computable with no network call, and neither test asserts it.

That makes the #428 recommendation tiered rather than "always hedge":

• signer_id is 64 hex chars -> compare against hex(recovered_ed25519_key); mismatch is a hard finding, not a hedge.
• signer_id is 0x + 40 hex -> compare against keccak(recovered_secp256k1_pubkey)[12..].
• anything else (named account like alice.near) -> genuinely needs chain access; hedge the wording as originally suggested.

The first two are the common Intents shapes, so this converts most of that finding from a disclaimer into a real check. Both fixtures here are ready-made positive test cases, and a negative one is a two-line edit to signer_id. Probably belongs in A3/B rather than this tests-only PR, but I wanted it recorded against the vectors that demonstrate it.

[shahan-khatchadourian-anchorage]

Good catch, and you're right this doesn't belong in this tests-only PR — implemented in #428 instead, where render_signature/render_envelope actually live. Replied there with details: #428 (see the thread on the original signature-wording comment).

[prasanna-anchorage]

This is the only test that reads _vector_erc191.input. The three tests that actually exercise verification all call build_erc191_vector() and re-derive the bytes in-process, so the committed file never reaches the code under test.

That inverts what a pinned fixture is for. If the generator regresses, UPDATE_TESTDATA=1 rewrites the file to match the broken output, this test then agrees, and the verification tests were never reading the file anyway -- the whole ERC-191 suite goes green on wrong bytes. The fixture can self-certify.

Compare the raw_ed25519 side, which gets this right: include_bytes! at line 65 pulls in a vector copied from an external suite that this crate cannot regenerate, so it's a true pin and the tests consume it directly.

What currently saves the ERC-191 vector is erc191_generator_reproduces_metamask_reference_signature -- but that pins the generator against a different key and message, so it constrains the signing path without constraining this vector's contents.

Cheapest fix is to make the verification tests read the committed file the way the ed25519 ones do, and keep build_erc191_vector() solely for the drift check. Then a generator bug shows up as a signature that no longer verifies, rather than as a fixture that quietly moved.

[shahan-khatchadourian-anchorage]

Fixed — added ERC191_VECTOR (include_bytes! on the committed fixture, matching the existing raw_ed25519 VECTOR pattern) and switched all three verification tests (erc191_valid_signature_recovers_the_signer_key, erc191_tampered_payload_recovers_a_different_key, and the recovery-id tests) to decode it directly instead of calling build_erc191_vector() fresh. build_erc191_vector() now backs only erc191_vector_matches_generator, the drift check.

[pepe-anchor]

Peer Review Summary — NEAR stack #429 (COMMENT)\n\n5 findings: 2 LOW, 3 INFO. Detailed inline comments below.\n\nKey concern: 4 of 7 MultiPayload signature standards untested. TIP-191 lacks tampered-message detection test.

---

AI Review on behalf of @pepe-anchor. Please flag any inaccuracies.

[pepe-anchor]

[LOW] TIP-191 has no tampered-message detection test

RawEd25519 has tampered_signature_is_invalid and ERC-191 has erc191_tampered_payload_recovers_a_different_key, but TIP-191 (tests tip191_tron_reference_signature_recovers_its_key_end_to_end and tip191_shares_the_recovery_id_guard) has no test that modifies the message or signature and asserts verification fails. If TIP-191's verify path had a bug where it always returns Valid, there would be no unit test to catch it. TIP-191 shares the secp256k1 ecrecover path with ERC-191, so the risk is lower, but defense-in-depth demands its own tamper test.

---

Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.

[shahan-khatchadourian-anchorage]

Agreed, fixed in d4f7c31. tip191_tampered_message_recovers_a_different_key signs nothing new -- it feeds the TRON reference signature against a mutated message and asserts the recovered key is not the signer's, the same shape as the ERC-191 tamper test.

One detail worth recording: the assertion is inequality rather than a verification failure, because ERC-191/TIP-191 "verification" is recovery -- a well-formed signature over changed bytes still recovers some key. The test allows an outright recovery failure too, but I checked which arm actually runs (temporarily panicking in the other) and it's the recovered-different-key path, so the test isn't passing through the permissive branch.

[pepe-anchor]

[INFO] ERC-191 tamper test uses fragile string replacement on JSON

erc191_tampered_payload_recovers_a_different_key replaces \\"998\\" with \\"999\\" in the serialized JSON string of ERC191_VECTOR. This works because the fixture is a single-line JSON file, but if the fixture were ever reformatted (pretty-printed, line-wrapped, or the amount values changed), the replacement would silently not match, producing an unmodified payload and causing the test to pass vacuously (asserting the key matches when it should not). A more robust approach would decode the fixture, modify the parsed structure, and re-serialize. The raw_ed25519 tamper test has the same pattern (string replace on the base58 signature), but that's less fragile because it replaces part of a hex-like identifier that is unlikely to change format.

---

Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.

[shahan-khatchadourian-anchorage]

Good catch -- that's a genuine vacuous-pass risk, guarded in d4f7c31.

The test now asserts tampered != good immediately after the replace, so a fixture that gets reformatted or re-valued fails loudly instead of quietly leaving the payload untampered and then "passing" by asserting an untouched payload recovers its own signer's key.

Left the raw_ed25519 tamper test's pattern alone for the reason you give -- it substitutes one base58 character in a signature, which has no reformatting exposure.

[pepe-anchor]

[INFO] ERC-191 fixture is regeneratable, not truly pinned like raw_ed25519

The comment block on line 157-163 says the ERC-191 vector is 'GENERATED deterministically ... and pinned as a fixture file.' Unlike the raw_ed25519 vector (line 108-116: 'verbatim from the pinned dependency's own test suite'), the ERC-191 fixture can be silently regenerated via UPDATE_TESTDATA=1 cargo test -p visualsign-near erc191. If the generator build_erc191_vector() had a regression in its ERC-191 prehash logic, the fixture would be overwritten and the tests would still pass against the new (potentially wrong) fixture. The include_bytes! + erc191_vector_matches_generator drift check is a good design (it catches generator drift in CI), but the description as 'pinned' is misleading compared to the raw_ed25519 provenance pin. The MetaMask reference vector (lines 310-350) is the true non-regeneratable pin for ERC-191.

---

Authored by Claude on behalf of @pepe-anchor. Please flag any inaccuracies, I'm not always right.

[shahan-khatchadourian-anchorage]

Fair -- the word "pinned" was doing two different jobs. Reworded in d4f7c31.

On re-reading, though, the exposure is narrower than the finding states, so the new comment says three things rather than two. erc191_vector_matches_generator fails when committed bytes and generator disagree, so neither drifts silently. And the verification tests decode the committed bytes through the production verify path, so a bug in the generator alone surfaces as a signature that no longer verifies -- regenerating can't bless it into passing. What neither layer catches is a bug in logic the generator and the verifier share, since regeneration moves both together.

That last case is the real gap, and the MetaMask/TRON vectors are the anchor for it, as you say -- no command in this repo can rewrite them. The comment now states the split that way instead of calling self-produced bytes "pinned" in the same sense as the raw_ed25519 vector copied from upstream.