fix(release): unbreak the tag build, and make the report show what CI actually ran - #365
Merged
BitHighlander merged 14 commits intoAug 9, 2026
Conversation
The report is rendered from the merged JUnit (Python integration + the four native firmware unit XMLs) but --validate-junit was handed the Python JUnit alone. Any catalog entry naming a native unit test therefore resolved to "missing" and failed the gate, so no native test could ever be catalogued -- the firmware-unit XMLs were merged in and then unusable, which is half the reason 432 native tests were invisible to the release report. Validate against the merged file. The requirement that canonical Python evidence exist and be non-empty is enforced earlier, before the merge, so this weakens nothing. Verified locally against the 7.15.0 RC artifacts from run 31284108490: the trigger renders 325 catalogued tests and exits 0, and injecting a failure into Storage::PinKdfV16RewrapsToV19AfterCorrectPin makes it exit non-zero with "report catalog validation failed". Pairs with python-keepkey "report: catalog the 7.15 seed-generation evidence and state the report's scope"; the pin must move for section K to appear.
test_dylib_confirm_flow.py contains two tests and neither has ever run in CI. The main python job skips the file (it needs KK_TRANSPORT=dylib), and the dylib job invoked test_dylib_screenshot.py only, so both were invisible -- the skip reasons appear in the JUnit but no gate reads them. They are not equivalent, and treating them as one file is what kept both out. test_features_round_trip is the pure Initialize -> Features path with no confirm at all; its own docstring says it should always pass. That one runs now. test_load_device_with_auto_confirm stays skipped, and deliberately: it is blocked on an open firmware defect, not on test plumbing. confirm_helper busy-loops on a ButtonAck the dylib consumed but never delivered, which is the same path keepkey-vault's wipe_device hangs on, and no pytest-timeout method can interrupt a C-level kkemu_poll(). Adding the file wholesale would convert a silent gap into a job that burns its 25-minute timeout. Named explicitly rather than by file so the distinction is visible at the call site instead of living only in a skip decorator.
…iant 27970b0 gave the regular variant an empty suffix so its published assets keep their plain names. Three of the four renames in "Rename artifacts" also insert the version, so they stay distinct. The fourth does not: mv HASHES.txt "HASHES${SUFFIX}.txt" With SUFFIX empty that is `mv HASHES.txt HASHES.txt`, and GNU coreutils does not treat a same-file move as a no-op -- it errors "are the same file" and exits 1. The step runs under the default `bash -e`, so the regular firmware's release job dies there, after the build and the hashing have already succeeded. Only the bitcoin-only leg would publish. Nothing caught this because release.yml runs on tag push only: the branch's 28/28 green CI never executes it, and the last green release run (rc26, 2026-08-04) predates the suffix change -- it still renamed to "HASHES-${VARIANT}.txt", which is always distinct. The first thing to exercise this path would have been the v7.15.0 tag itself. Skip the move when there is nothing to rename. The regular variant's intended output name is plain HASHES.txt, which is what it already has. Verified by running the step's script under `bash -e` for both matrix legs: empty suffix exits 0 leaving HASHES.txt, "-bitcoin-only" exits 0 leaving HASHES-bitcoin-only.txt.
The SRAM budget gate has always enforced correctly -- verified against the shipped 7.15.0 RC ELF, where the reserve measures 17,940 B against a 16,384 B budget, and all three of its failure modes exit 1: reserve breached, largest frame eating the margin, and an empty .su archive (it refuses to pass the frame-margin check without stack-usage data rather than treating "no records" as "largest frame = 0"). But its output only ever existed in a job log nobody opens. The frame-arena fix -- the RC8 msg_write boot fault, the single largest SRAM change in this release -- therefore had no reviewable evidence anywhere, and an RC audit recorded it as having zero coverage. It did not; nothing surfaced it. Mirror the report into the run summary, per variant. The status handling is deliberate. `tee` would otherwise swallow a non-zero exit, so pipefail is on; and the status is captured rather than allowed to abort the step under `set -e`, because a FAILING gate is exactly when the numbers need to reach the summary. The step re-exits with the captured status, so a breach still fails the build. Verified by extracting the step's script and running it against stubs for both outcomes: pass exits 0, breach exits 1 and the summary still contains the ::error:: line. shellcheck clean.
…screen
The per-output shielded confirm built
"Send shielded ZEC?\n<unified address>\nAmount: %s"
for a three-row body. A unified address is 106 characters -- three full rows
on its own, as the captured display-address frame shows (38 + 38 + 30). The
standard notification does not scroll or paginate: draw_string stops emitting
as soon as a character will not fit, with no indication. So the screen
rendered the question plus the first 76 characters of the address, and
silently dropped the remaining 30 along with the entire amount line.
This is the gate that was supposed to show the value. total_amount on the
summary prompt is taken straight from the host message, and the contract
comment in the signing path says so outright -- it calls total_amount "a
summary prompt" and states that Orchard output recipients and values "have
their own verification gates". This confirm is that gate. The device does
bind value to cmx, so a host cannot present an inconsistent note; the defect
is that the user approved a value they were never shown.
Both pools are affected. is_ironwood selects only which compute_cmx runs;
the address rendering and the confirm are the shared tail of the same
function, so Ironwood and Orchard render the identical screen.
Amount first on a body that cannot overflow, then the full address through
layout_zcash_address_text_notification -- the layout built for a 106-char UA
and already used by the display-address flow.
VERIFICATION LIMITS, stated plainly:
- The overflow is established from the code and from a measured 106-char
UA in a real captured frame, not from a rendering of this screen.
- Nothing in CI executes it. ZcashSignPCZT is never sent to a device
anywhere in the suite -- every test in test_msg_zcash_sign_pczt drives a
ScriptedTransport with canned responses. The device-driven Zcash tests
cover transparent signing, display-address, FVK and seed fingerprint.
- So this compiles in CI and is not otherwise exercised. It MUST be walked
on hardware during the RC round, and both screens photographed.
- The flow now emits two ButtonRequests per shielded output instead of
one. Hosts that loop on ButtonRequest/ButtonAck are unaffected; anything
counting exact requests needs updating.
Moves deps/python-keepkey to 3bbf996, which adds the first test that actually sends ZcashSignPCZT to a device. Until now every PCZT test drove a scripted transport with canned responses, so the on-device shielded path -- including the per-output confirm this branch fixes -- was never executed by anything. The new test is a real regression test for that fix, not a companion to it: run against the shipped 7.15.0 RC emulator it fails with "expected 2 ConfirmOutput screens, got 1". This pin is what lets CI run it against the fixed firmware. Also brings the report catalog changes: native firmware unit tests can now be referenced at all, the header states how much of the run the catalog covers, and the shielded confirm screens are finally requested as screenshots. PRACTICE PIN -- 3bbf996 is on an unmerged fork branch (BitHighlander/python-keepkey#30, stacked on keepkey/python-keepkey#211). Re-pin to a merged master SHA before tagging v7.15.0, along with device-protocol and trezor-firmware.
…f it
Two defects, both found by the integration job failing on this branch.
1. A failing test run threw away everything it had produced.
The step comments say "always extract reports", and the upload steps carry
if: always(). Neither helped, because the step runs under the default
`bash -e` and captured exit codes with
docker compose up --exit-code-from python-keepkey python-keepkey; PY_RC=$?
A bare failing command aborts immediately under -e, so the abort happened
before the assignment and before the docker cp block. Nothing was ever
copied out of the container, and if: always() had nothing to upload.
The run that exposed this captured four OLED frames of the Zcash shielded
confirm -- the first shielded frames this project has ever produced -- and
uploaded none of them. Its JUnit and PDF went the same way. Evidence
disappearing exactly when a run fails is the worst possible time for it.
`|| RC=$?` keeps the assignment and the extraction on the path while still
failing the step afterwards. Verified by running the step's own script with
a deliberately failing container: extraction is reached, RCs are captured,
and the step still exits 1. With the semicolon form the extraction line
never prints.
2. The in-container catalog validation could not see native unit tests.
python-keepkey-tests.sh validated against the Python JUnit alone, so every
catalog entry naming a native firmware unit test resolved to "missing" --
the same defect already fixed in scripts/generate-test-report.py, in a
second call site. The test-reports volume is shared rw with the
firmware-unit container, which runs first, so those XMLs are already
present; merge them before validating and before rendering.
If they are absent this falls back to Python-only and native entries then
fail as "missing" — it still fails closed rather than quietly passing.
Verified against the real artifacts from the failing run: merging the
native XMLs resolves K3-K11, leaving only the Z22-Z25 entries that the
older Python JUnit predates.
fsm_msgZcashTransparentInput checked that msg->index matched current_transparent_input, but never that either was inside transparent_inputs[ZCASH_MAX_TRANSPARENT_INPUTS], an 8-element static array. Nothing stops a host sending more ZcashTransparentInput messages after the declared count is consumed. The ack loop stops asking, but the handler still accepts them, and current_transparent_input keeps incrementing -- so index 8, 9, 10 and beyond each wrote a fully host-controlled ZcashTransparentInputState past the end of the array: amount, a 32-byte prevout txid, the script_pubkey and the whole address_n array, straight into whatever follows it in static memory. Reject an index outside the declared count or the array bound, and refuse once the expected inputs have all arrived.
The review chain ran VERIFIED, then `else if (schema_verified)`, then OPAQUE, and only rejected SOL_TX_REVIEW_MALFORMED in the final else. So the schema arm shadowed the rejection: an attested KKSOLSC1 schema matching one partially-parsed instruction was enough to reach the schema review screens and then the signature, on a message the parser had already declared malformed. A schema describes a single instruction; it says nothing about the rest of a transaction that failed to parse. Reject malformed up front, as its own guard rather than by restoring it to the end of the chain -- branch ordering is what hid it the first time. The final else stays as a backstop for any future review state.
…igning fsm_msgEthereumTxMetadata already refused to run during signing, with a comment explaining exactly why: clearing the binding mid-flight lets a host approve a benign decode, suppressing the blind-sign gate, then stream attacker-chosen calldata for the remaining chunks. Two other paths reached the same clear with no such guard. fsm_msgLoadClearsignSigner ends in signed_metadata_store_signer() -> signed_metadata_clear(), which drops stored_metadata along with relied_on_metadata. It now carries the same prologue as its sibling. fsm_msgClearSession was worse: an unauthenticated MSG_IN needing no button press, which called session_clear() -> signed_metadata_clear_signers() -> signed_metadata_clear() while Ethereum signing continued across it. signed_metadata_enforce() then saw relied=false and passed. It aborted only Zcash; fsm_msgInitialize and fsm_msgCancel abort every engine, and so does this now. The bitcoin-only build already provides an ethereum_signing_abort() no-op for exactly these always-on handlers.
…dition The five BIP-341 SHA-256 contexts were initialised inside the non-overwintered `else`, while every update and finalise site for them is gated only on `coin->has_taproot && coin->taproot`. `overwintered` arrives from the host on SignTx, so setting it on a taproot coin skipped the initialisation and left the use sites live: the sighash was then built over whatever those static contexts held -- uninitialised on the first signature after boot, and carried over from the previous transaction afterwards. Initialise them on the taproot condition alone, outside the overwintered branch. Whether a non-Zcash coin should accept overwintered at all is a separate question and deliberately not changed here.
…, size the bech32 buffers Three things, one of which is a self-inflicted regression the new device test caught within one CI run. 1. RESTORES the Zcash shielded two-screen output confirm. b21f52c was committed from a working tree that had silently rolled back to the pre-fix content, so it reverted the fix from 56807a7 while adding the index bound. Nothing in the diff summary showed it. The device test added for exactly this defect failed on the next run with "expected 2 ConfirmOutput screens, got 1" -- the same message it produces against the unfixed release build. An audit of every fix on this branch found this and the output bound below were the only two casualties; the other eleven survived intact. 2. Bounds the transparent OUTPUT index, the sibling of the input overflow. Fixing fsm_msgZcashTransparentInput did not cover fsm_msgZcashTransparentOutput: a different array with its own free-running counter. After the declared outputs are stored the dispatch moves to transparent inputs without incrementing current_transparent_input, which leaves the output handler re-armed -- so a host that ignores the ack and keeps sending outputs walks current_transparent_output past n_transparent_outputs, writing a host-controlled amount and a 128-byte script_pubkey past transparent_outputs[8], onto transparent_inputs[0] and then outside the struct. 3. Sizes every Tendermint-family bech32_decode buffer to its documented contract. segwit_addr.h requires hrp >= BECH32_MAX_HRP_LEN + 1 (84) and data >= strlen(input) - 8. Every caller in signtx_tendermint.c, osmosis.c, thorchain.c, mayachain.c, tendermint.c and binance.c used char hrp[45] and uint8_t decoded[38] against address fields whose proto max_size is 53 -- so a long address overflowed both. bech32_decode fills these buffers BEFORE validating the checksum, so the usual `if (!bech32_decode(...)) return false;` guard never prevented it: a rejected address had already written past the end.
draw_string() renders characters until one no longer fits the canvas and then stops. Only BODY_ROWS rows of body text are on screen -- row 4 starts at y=66 on a 64px display -- so any longer body was drawn in part, with no ellipsis and nothing to tell the user that something had been cut. The worst instance is the ERC-20 approve screen, which for any realistic amount or a ticker past five characters drew 39 of the spender's 42 hex characters and dropped the trailing '?'. An attacker who grinds an address matching a known-good contract in its visible prefix gets a screen the user cannot distinguish from the real one. It is one of ~30 sites: Solana stake authorities, TRON recipients, THORChain asset chains, the "NOT verified by KeepKey" warning and the passphrase echo all lose their tails the same way. Fix it once, where every confirm passes through. confirm_helper() now splits an over-long body into screen-sized pages, each with its own ButtonRequest and its own press, and titles them "1/2", "2/2". Bodies that already fit take exactly the path they did before -- same single screen, same title, no extra press -- so only the flows that were already broken change shape. Custom layouts are left alone: they place and size their own body, and layout_zcash_address_text_notification deliberately renders 106 characters as 38+38+30. calc_str_page() gets an early exit. Line count cannot fall as a prefix grows, so the first prefix that does not fit settles the page; without it the scan ran the full body length for every page and a 350-character confirm paused for roughly a second on device. Verified: three board unit tests drive confirm_body_split() over the real ethereum.c bodies -- pages reproduce the body exactly, no page exceeds BODY_ROWS, and fitting bodies stay on one page. Both mutants fail them: forcing a single page reproduces the original truncation, and allowing four rows trips the overflow assertion. The early exit was checked against the old full scan over 19,200 random string/width/row combinations.
… wrong b024f8b made confirm() page a body too long for BODY_ROWS instead of silently dropping its tail. Two memo vectors cross that line, so the swap tests waited on presses nobody queued and hung until CI killed the job: unit-tests at its 10-minute timeout, python-integration-tests at its 30 — both reported "cancelled", which reads like flake rather than a wrong expectation. python-integration-tests runs firmware-unit before the pyk suite, so the pyk tests never ran at all. Measured against the real font tables rather than guessed: Confirm swap asset USDT-0xdac17f95...\n on chain ETH 4 rows -> 2 pages Confirm to 0x<238 d's> 8 rows -> 3 pages so MemoSwapFullFormShowsAffiliate is 5 presses (2+1+1+1) and MemoExactBufferCapacityKeepsLastChar is 6 (1+3+1+1), in both the thorchain and mayachain suites. Nothing else in the suite pages: the 42-character destination addresses land on exactly 3 rows and still fit. That the USDT screen paged is the bug being fixed showing itself — the tail of that contract address was never reaching the display. kkconfirm_preload() now queues one trailing rejection as a sentinel. An unbudgeted screen consumes it, is rejected, and the call under test returns false immediately, so a wrong count fails in milliseconds with a negative drain instead of hanging; kkconfirm_drain() discounts the sentinel so existing "== 0" assertions keep their meaning. Verified by mutation: with preload(4) against 5 screens the test fails in 5 ms on drain() == -2. 405/405 firmware-unit tests pass.
BitHighlander
merged commit Aug 9, 2026
4a54ca5
into
release/715-audit-06-bitcoin-security
30 checks passed
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.
Targets #364's branch so 7.15 stays a single PR.
Three independent defects found while verifying the RC coverage gaps. All three are invisible to the branch's 28/28 green CI, for three different reasons.
1.
release.ymlwould have failed thev7.15.0tag build — RELEASE BLOCKER27970b0c6gave the regular variant an empty suffix. Three of the four renames in "Rename artifacts" also insert the version so they stay distinct; the fourth does not:GNU coreutils does not treat a same-file move as a no-op — it errors "are the same file" and exits 1. The step runs under the default
bash -e, so the regular firmware's release job dies there, after the build and hashing already succeeded. Only the bitcoin-only leg would publish.Why nothing caught it:
release.ymlruns on tag push only. The branch's CI never executes it, and the last green release run (rc26, 2026-08-04) predates the suffix change — it still renamed toHASHES-${VARIANT}.txt, always distinct. The first thing to exercise this path would have been thev7.15.0tag itself.Verified by running the step's own script under
bash -efor both matrix legs: empty suffix exits 0 leavingHASHES.txt,-bitcoin-onlyexits 0 leavingHASHES-bitcoin-only.txt.(Note for anyone re-checking: BSD/macOS
mvreturns 0 for a same-file move. Don't test this on a Mac and conclude it's fine.)2. The test report could not reference a single native unit test
--validate-junitwas handed the Python JUnit alone, while the PDF is rendered from the merged JUnit (Python + the four native firmware-unit XMLs). Any catalog entry naming a native test therefore resolved tomissingand failed the gate — so no native test could ever be catalogued. The firmware-unit XMLs were merged in and then unusable.This is half of why 432 native gtests were invisible to the release report; the other half is in the paired python-keepkey PR. The consequence was real: an RC audit grepped the PDF, found no
diceand no PIN KDF, and reported both as zero-coverage. Both had run green in the same CI run.Validate against the merged file. The canonical-Python-evidence requirement is enforced earlier, before the merge, so nothing is weakened.
Verified against run 31284108490: the trigger renders 325 catalogued tests and exits 0; injecting a failure into
Storage::PinKdfV16RewrapsToV19AfterCorrectPinmakes it exit non-zero with "report catalog validation failed".3.
test_dylib_confirm_flowran nowhere — and its two tests are not equivalentThe main python job skips the file (needs
KK_TRANSPORT=dylib) and the dylib job invokedtest_dylib_screenshot.pyonly, so both tests were invisible. Treating them as one file is what kept both out:test_features_round_trip— pureInitialize -> Features, never entersconfirm_helper; its own docstring says it should always pass. Now runs.test_load_device_with_auto_confirm— stays skipped, deliberately. It is blocked on an open firmware defect, not on test plumbing:confirm_helperbusy-loops on a ButtonAck the dylib consumed but never delivered — the same path keepkey-vault'swipe_devicehangs on — and nopytest-timeoutmethod can interrupt a C-levelkkemu_poll(). Adding the file wholesale would convert a silent gap into a job that burns its 25-minute timeout.Named explicitly rather than by file so the distinction lives at the call site instead of only in a skip decorator.
Pairs with
BitHighlander/python-keepkey#30 (stacked on keepkey/python-keepkey#211). The pyk pin must move for the new report section to appear — each half is inert without the other.