Skip to content

Commit 02889da

Browse files
committed
fix(tests): gate on firmware capability, not on host bindings or source text
Two gates were checking something other than what they claimed, and both went green on a branch where the thing they gate was absent. requires_message() asks whether python-keepkey's OWN bindings define a message. That is a property of the pinned submodule, not of the firmware under test, so it passes on every branch regardless. The structured EIP-712 suite used it, and on feat/passkeys-7.16 -- which has no eip712_stream.c at all -- four tests failed as though the feature were broken rather than absent. Replaced with requires_structured_eip712(), which probes the device: firmware without the walk answers the opening message with Failure_UnexpectedMessage. Any OTHER failure deliberately does NOT skip, because "present but misbehaving" must never be mistaken for "absent" -- that is how a skipped test becomes a silent pass. test_burned_versions_have_no_reader asserted the ABSENCE of a `case StorageVersion_18:` label, reasoning that falling to the default is what sends a burned format to the wipe path. There is no default: storage_fromFlash omits one deliberately so -Werror=switch names any version we forget. So an unlisted version does not fall anywhere, it breaks the ARM build -- which is exactly what happened. Now asserts the real property: the labels exist, and what they dispatch to is SUS_Invalid with no storage_readVxx behind them. Verified the test is not vacuous by injecting a reader and watching it fail.
1 parent 0abf93e commit 02889da

3 files changed

Lines changed: 78 additions & 13 deletions

File tree

tests/common.py

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,43 @@ def requires_taproot(self):
139139
if not getattr(self.client.features, 'supports_taproot', False):
140140
self.skipTest("Firmware does not report supports_taproot")
141141

142+
def requires_structured_eip712(self):
143+
"""Skip unless the FIRMWARE drives the structured EIP-712 walk.
144+
145+
requires_message() cannot answer this. It asks whether
146+
python-keepkey's own bindings define a message, which is a property of
147+
the pinned submodule and not of the firmware under test -- so it passes
148+
on every branch regardless, and a branch without eip712_stream.c fails
149+
these tests as though the feature were broken rather than absent.
150+
151+
Probes the device instead: firmware that does not implement the walk
152+
answers the opening message with Failure_UnexpectedMessage. A firmware
153+
that DOES implement it answers with a struct request, and we cancel.
154+
Anything else is left to fail the test, because "the feature is present
155+
but misbehaving" must never be mistaken for "the feature is absent".
156+
"""
157+
from keepkeylib import messages_ethereum_pb2 as _eth
158+
from keepkeylib import messages_pb2 as _proto
159+
160+
probe = _eth.EthereumSignTypedData()
161+
for n in (0x8000002C, 0x8000003C, 0x80000000, 0, 0):
162+
probe.address_n.append(n)
163+
probe.primary_type = "EIP712Domain"
164+
probe.metamask_v4_compat = True
165+
166+
resp = self.client.call_raw(probe)
167+
if isinstance(resp, _proto.Failure):
168+
self.client.init_device()
169+
if resp.code == _proto.Failure_UnexpectedMessage:
170+
self.skipTest(
171+
"Firmware does not implement structured EIP-712 "
172+
"(EthereumSignTypedData is not handled)")
173+
# Any other Failure is a real problem; let the test run and report it.
174+
return
175+
# Feature is present -- put the device back before the test starts.
176+
self.client.call_raw(_proto.Cancel())
177+
self.client.init_device()
178+
142179
def requires_message(self, msg_name):
143180
"""Skip if firmware does not handle this message type.
144181
Use alongside requires_firmware for per-feature gating:

tests/test_msg_eip712_streaming.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ def setUp(self):
8787
super(TestMsgEip712Streaming, self).setUp()
8888
self.requires_firmware("7.15.0")
8989
self.requires_fullFeature()
90-
self.requires_message("EthereumSignTypedData")
90+
self.requires_structured_eip712()
9191
self.setup_mnemonic_nopin_nopassphrase()
9292
self.client.apply_policy('AdvancedMode', 1)
9393

tests/test_storage_version_gate.py

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -455,22 +455,50 @@ def test_active_flash_format_is_v20(self):
455455
"release (7.15 = V17) and moves in the release commit that tags "
456456
"7.16, not when a format lands in the tree." % self.last_shipped)
457457

458-
def test_burned_versions_have_no_reader(self):
459-
"""18 and 19 must never be parsed by 7.16.
458+
def test_burned_versions_are_dispatched_to_the_wipe_path(self):
459+
"""18 and 19 must never be PARSED by 7.16.
460460
461461
They were real formats in alpha builds before the 7.15 revert, so
462462
devices carrying them exist. A reader for either would parse a
463-
clear-sign identity block or a PIN-KDF blob as passkey state. The
464-
absence of a case in the dispatch is what sends them to the wipe path,
465-
and this test is what stops one being added back by someone tidying up
466-
the switch.
463+
clear-sign identity block or a PIN-KDF blob as passkey state.
464+
465+
This used to assert the absence of a `case StorageVersion_18:` label,
466+
on the theory that falling to the default is what sends them to the
467+
wipe path. That was wrong twice over: storage_fromFlash has NO default
468+
case -- deliberately, so -Werror=switch names any version we forget --
469+
so an unlisted version does not fall anywhere, it fails the ARM build.
470+
471+
So the labels must exist. What must NOT exist is a reader behind them.
472+
Assert the real property: 18 and 19 are dispatched, and what they
473+
dispatch to is SUS_Invalid rather than any storage_readVxx call.
467474
"""
468-
self.assertNotIn("case StorageVersion_18:", self.c,
469-
"18 is a burned format; a reader would misparse blobs "
470-
"written by pre-revert alpha builds")
471-
self.assertNotIn("case StorageVersion_19:", self.c,
472-
"19 is a burned format; a reader would misparse blobs "
473-
"written by pre-revert alpha builds")
475+
for burned in (18, 19):
476+
label = "case StorageVersion_%d:" % burned
477+
self.assertIn(
478+
label, self.c,
479+
"%s must be listed; storage_fromFlash has no default case, so "
480+
"an unlisted version breaks the -Werror=switch build" % label)
481+
482+
# The two labels must sit together and return SUS_Invalid before any
483+
# other case begins. Slice from the first burned label to the next
484+
# `case ` that is not one of the burned ones.
485+
i = self.c.index("case StorageVersion_18:")
486+
rest = self.c[i:]
487+
j = len(rest)
488+
for m in re.finditer(r"\n\s*case StorageVersion_(\w+):", rest):
489+
if m.group(1) not in ("18", "19"):
490+
j = m.start()
491+
break
492+
arm = rest[:j]
493+
494+
self.assertIn(
495+
"SUS_Invalid", arm,
496+
"the burned versions must return SUS_Invalid (the wipe path); "
497+
"arm was:\n%s" % arm)
498+
self.assertNotIn(
499+
"storage_read", arm,
500+
"a reader behind a burned version would misparse blobs written by "
501+
"pre-revert alpha builds; arm was:\n%s" % arm)
474502

475503
def test_version_never_drops_below_a_shipped_release(self):
476504
"""Lowering STORAGE_VERSION wipes every device upgrading FROM a shipped

0 commit comments

Comments
 (0)