fix(keepkey/eip712): say why the device refused, and fall back when it cannot parse - #64
Merged
Merged
Conversation
…t cannot parse
Two defects that combine into a dead end for x402 payments on firmware 7.14.2.
1. The refusal text was thrown away.
} catch (error) {
console.error({ error });
throw new Error("Failed to sign typed ETH message");
}
transport.call throws the raw failure event, so the device's own words are
sitting at error.message.message -- and this replaced all of them with one
string. "Enable AdvancedMode to blind-sign typed hashes", "Structured EIP-712
disabled pending canonical display hardening" and an unplugged cable were
indistinguishable to the user, and the difference between them is the only
part they can act on.
Now: surface the firmware's text when there is one, rethrow real Errors
unchanged (ActionCancelled keeps its identity), and fall back to the generic
string only for a non-Error with nothing to say.
2. x402 hard-failed instead of degrading.
isX402Eip3009() routes EIP-3009 TransferWithAuthorization to the structured
endpoint. Firmware 7.14.2 withdrew that endpoint -- its JSON parser could not
guarantee the displayed value was the value being hashed -- so the call is
answered with a Failure and the payment simply died, reporting the generic
message above.
Now the structured attempt is caught, and ONLY a refusal that means "this
device has no structured endpoint" falls through to the hashed path:
Failure_UnexpectedMessage from firmware predating the message, or the
"Structured EIP-712 disabled" text from firmware that has withdrawn it.
Every other failure propagates, because masking a real error with a silent
downgrade is how a signing bug becomes invisible.
Detection is by ATTEMPT, not by version number. There is no capability bit for
this, and a version table would need editing on every branch that toggles the
flag. Retrying is safe because the firmware refuses at the top of the handler,
before it touches session state.
The fallback is not a silent loss of protection: the hashed path still shows
the device's blind-sign warning and still requires AdvancedMode. It is the same
treatment every other typed-data payload already gets on this firmware.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The answering side of the streaming protocol: the device drives, this owns the document and serves whatever it addresses. Three parts, each independently tested (18 cases, all green): parseSolidityType -- "uint256", "bytes32", "Person[3]", "int16[2][][4]" into the wire description. array_levels is in WRITTEN order, left to right, because that is the order encodeType reproduces; reversing it silently changes typeHash and therefore the signature. It refuses a bare "uint"/"int". That is not canonical EIP-712, and the old firmware accepted it and hashed it as 256 bits -- producing a type string no compliant verifier reproduces. Refusing at the host means the device never sees it. encodeValue -- one leaf as the exact bytes the device will hash AND display. Raw big-endian at the declared width, never JSON. The device does no number parsing at all, which is what removes the old path's strtoll ceiling of 2^63-1 -- i.e. every unlimited ERC-20 approval ever issued, the most common permit there is. There is a test for exactly that value. The flip side is that width correctness is now entirely the host's job, so this is strict rather than lenient: out-of-range values throw, a JS number that has already lost precision throws rather than being silently converted into a number the caller never had, and a wrong-width address or bytesN throws. resolveMemberPath -- walks a device-supplied member_path into the document. path[0] is 0 for domain, 1 for message. A path stopping on an ARRAY is the device asking for its length; a path stopping on a STRUCT is a protocol error, because the device walks into structs rather than asking for them. Fixtures are Permit2's PermitSingle nesting PermitDetails -- the payload that started this, and the one a flat-structs-only design cannot sign. Note for anyone extending this: BigInt LITERALS (1n) are ES2020 syntax and this repo targets ES2016 with an es2020 lib, so the BigInt function is available but the literal form does not compile. Hence the ZERO/ONE/EIGHT constants.
…oder All five were in code written today, all five confirmed by independent verification, and four of them sign the wrong thing rather than merely failing. 1. A fixed array dimension of ZERO was conflated with a dynamic one. "uint256[0]" parsed to arrayLevels [0], byte-identical to "uint256[]" -- and 0 is the wire's dynamic sentinel, so the device spells it back as "[]". Confirmed against ethers 5.7.2: Foo(uint256[0] a) and Foo(uint256[] a) have different hashStructs. Leading zeros re-spelled the same way. Both refused. 2. Non-canonical integer widths were silently NORMALISED. "uint0256" became uint256 and was hashed as "uint256", while a verifier reading the document sees "uint0256". Same failure as the bare "uint" this module already refused, one spelling further on. Same for "bytes032". The integer regex is now anchored to digits, so a struct legitimately named "interest" is not caught by it. 3. and 4. A declared fixed dimension was never checked against the document, in either the element branch or the length branch. address[2] carrying three elements reported length 3 and served all three. The dimension is part of the type string and therefore part of typeHash, so this signs a document whose type says two -- and the device cannot notice, because the only count it ever sees is the one we give it. 5. Dynamic bytes and string had no length cap, while EthereumTypedDataValueAck.value is max_size:1024. A 2000-byte string encoded fine and then could not be sent, so the ceremony died at the transport layer where the error cannot name the field. 30 tests, all green. Every case above has a regression test that fails against the previous version.
eslint jest/no-conditional-expect was right to object. The union returned by resolveMemberPath was being narrowed with an if, so the assertions sat inside a branch -- and a guarded expect that never runs proves nothing while reading as if it did. asValue() narrows by throwing instead, which fails loudly on the wrong variant and keeps every expect on the main path. Plus import sort and one prettier wrap.
The answering half of the walk. The DEVICE leads -- it asks for one struct definition or one leaf value at a time, and runEip712Walk answers until a signature comes back. The host never chooses the order, and that is the property, not an accident of the API: the device hashes what it displays, in the order it picked, so a host that answered a different question would produce a digest that does not verify. `call` is injected rather than taking a Transport, so the loop is testable against a scripted device with no USB. Five cases, modelling the exact sequence the firmware state machine emits for Permit2 PermitSingle -- domain first, then the message, walking into PermitDetails: - every struct and value answered in order, and the nested uint160 comes back as exactly 20 big-endian bytes; - a nested struct's member list served in declaration order, with uint160 reported as size 20 (BYTES, as the wire wants); - an undefined struct REFUSED rather than answered with an empty member list, which the device would hash as a valid empty struct and sign a document neither side meant; - an unexpected message rejected instead of continuing blindly; - a device that never finishes the walk cut off at 512 round trips rather than hanging the host. eip712Wire.ts hand-writes the five messages because they are not in the published @keepkey/device-protocol package yet -- same approach as LoadClearsignSigner in ethereum.ts, and deletable the day the package ships generated classes. member_path is decoded accepting BOTH packed and unpacked repeated uint32: the device's encoder is not ours to pin, and assuming one form would break on the other. 35 tests green.
Host to device only -- the device never sends a StructAck, so there is nothing to parse. eslint was right that the parameter is dead; the underscore says it is dead on purpose rather than forgotten. Also: the previous commit's lint check printed OK unconditionally, so this got through. The check now reports its own exit status.
…path
The wiring that makes the walk reachable. ethSignTypedData now tries the
streaming path for EVERY document, not just x402, and falls back only when the
device says it cannot do it.
Three pieces:
1. typeRegistry registers message types 1704-1708 by hand. The registry builds
itself from Messages.MessageType, and the published device-protocol package
does not carry these yet, so the reducers cannot see them. Without this the
transport can SEND a request and then fail to decode the reply -- the walk
would stall on its first StructRequest. Delete the block when the package
ships generated classes; the reducers pick them up on their own.
2. RawPayload carries already-serialised bytes through transport.call, which
wants a jspb.Message. The walk produces bytes; re-encoding them through a
second object is a chance for the two encodings to differ, and the whole
point of this protocol is that they cannot.
3. The fallback predicate now covers two refusals rather than one:
- "Structured EIP-712 disabled" -- 7.14.2 withdrew the old endpoint.
- "arrays are not supported" -- the new walk is present but cannot walk
THIS document.
The second matters more than it looks. PermitBatch and Seaport nest arrays,
and a hard failure there would read as a bug rather than a limitation.
Degrading costs the user the field display and keeps the payment working,
which is exactly the deal every typed-data payload gets today.
Everything else still propagates. A refused screen, a malformed document, a
value that does not match its declared type -- those are real answers, and
masking them behind a silent downgrade is how a signing bug becomes
invisible.
AdvancedMode is deliberately NOT in that list: the hashed path requires it too,
so falling back would fail again with a worse message.
35 tests green.
The mock answered EVERY call as Ethereum712TypesValues:
const phase = request.getEip712typevals() ?? 0;
Now that ethSignTypedData tries streaming first for every document, the first
call is message type 1704 carrying a RawPayload, and that line threw
"request.getEip712typevals is not a function".
The mock was modelling a device that cannot exist -- one that answers an
unknown message type as though it understood it. Real firmware without the
streaming endpoint rejects 1704 with Failure_UnexpectedMessage, which is
exactly what 7.14.x does, so the mock now does that.
This makes the test cover MORE than it did: the x402 payload still reaches the
old structured endpoint, and it now gets there through the fallback rather than
because nothing else was tried. That fallback is the path every device in the
field takes today, and until now nothing exercised it.
The count assertion said two. It is three: the streaming probe this mock rejects, then the two old-path calls. Asserting two would be asserting that we never TRY the streaming path -- which is the opposite of the intended behaviour and would go green precisely when the feature stopped working. The call types are now asserted explicitly (1704, then 114 twice) so the sequence is visible rather than implied by a count.
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
1 participant
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.
Two defects that combine into a dead end for x402 payments on firmware 7.14.2.
1. The refusal text was thrown away
transport.callthrows the raw failure event, so the device's own words sit aterror.message.message— and this replaced all of them with one string. "Enable AdvancedMode to blind-sign typed hashes", "Structured EIP-712 disabled pending canonical display hardening", and an unplugged cable were indistinguishable to the user. The difference between them is the only part they can act on.Now: surface the firmware's text when there is one, rethrow real
Errors unchanged soActionCancelledkeeps its identity, and use the generic string only for a non-Error with nothing to say.2. x402 hard-failed instead of degrading
isX402Eip3009()routes EIP-3009TransferWithAuthorizationto the structured endpoint. Firmware 7.14.2 withdrew that endpoint, so the call is answered with a Failure and the payment simply died — reporting the generic message above.Now the structured attempt is caught, and only a refusal meaning "this device has no structured endpoint" falls through to the hashed path:
Failure_UnexpectedMessagefrom firmware predating the message, or the"Structured EIP-712 disabled"text from firmware that withdrew it. Every other failure propagates — masking a real error with a silent downgrade is how a signing bug becomes invisible.Detection is by attempt, not version number. There is no capability bit for this, and a version table would need editing on every branch that toggles the flag. Retrying is safe: the firmware refuses at the top of the handler, before touching session state.
The fallback is not a silent loss of protection — the hashed path still shows the blind-sign warning and still requires AdvancedMode.