Skip to content

SFT-3834: Refine Message Signing via MicroSD#636

Merged
mjg-foundation merged 6 commits into
dev-v2.4.0from
SFT-3834-refine-sign-message-with-microsd
May 11, 2026
Merged

SFT-3834: Refine Message Signing via MicroSD#636
mjg-foundation merged 6 commits into
dev-v2.4.0from
SFT-3834-refine-sign-message-with-microsd

Conversation

@mjg-foundation
Copy link
Copy Markdown
Collaborator

format for sparrow parsing

Copy link
Copy Markdown
Collaborator

Coverage gap: SignTextFileFlow is a parallel entry point that's NOT touched

There are two menu entries that produce signed text files via microSD:

  • menus.py:52 "Sign with microSD" (under Sign a Message) → HealthCheckMicrosdFlow(normal_signing=True) → HealthCheckCommonFlow — gets the verify step ✓
  • menus.py:328 "Sign Text File" (under Security) → SignTextFileFlow — does NOT get the verify step ✗

SignTextFileFlow.do_sign (sign_text_file_flow.py:83) goes straight from validate_file to signing with no message preview, no address confirmation. So if the goal is "user must always see the message and address before signing", this PR closes only one of the two doors. A user reaching for "Sign Text File" still sees nothing.

Either:

  • Apply the same verify pattern to SignTextFileFlow.do_sign, or
  • Remove the "Sign Text File" menu entry if HealthCheckMicrosdFlow(normal_signing=True) is meant to supersede it.

This is the most important issue with the PR. Worth raising before merge.

Copy link
Copy Markdown
Collaborator

Also, Address is derived twice

In show_message:

with stash.SensitiveValues() as sv:

  node = sv.derive_path(self.subpath)
  self.address = sv.chain.address(node, self.addr_type)

Then sign_health_check runs sign_text_file_task, which derives the address again and writes self.address = address (overwriting). In practice both derive from the same subpath + addr_type, so they match — but:

  • The user is asked to confirm address X, and then the task's address is what ends up in the signed output. There's no assertion that they're equal.
  • The seed is briefly in memory twice when once would do.

Cleaner pattern: derive the address in the task (or in a non-sensitive way from xpub), pass it to show_message for confirmation, then sign. As-is, it's not a correctness bug, but it weakens the "WYSIWYG signing" guarantee that's the entire point of the verify step.

@mjg-foundation mjg-foundation changed the title SFT-3834: added verify step for microsd message signing, fixed export SFT-3834: Refine Message Signing via MicroSD May 8, 2026
@mjg-foundation
Copy link
Copy Markdown
Collaborator Author

mjg-foundation commented May 9, 2026

Cleaner pattern: derive the address in the task (or in a non-sensitive way from xpub), pass it to show_message for confirmation, then sign. As-is, it's not a correctness bug, but it weakens the "WYSIWYG signing" guarantee that's the entire point of the verify step.

There's an issue: the preview is skipped for health checks, so the address derivation during signing may be the first derivation. I made some changes to be absolutely sure there's no mismatch.

"The task now always derives the address from the subpath. When expected_address is provided (message-signing path), it checks that the derived address matches what the user confirmed — if not, it short-circuits with an error before signing. Health checks pass None so the check is skipped entirely.

The error message includes both addresses to make a mismatch diagnosable, though in practice this should never trigger if the flow is correct — it's a defensive assertion rather than expected user-facing error handling."

Copy link
Copy Markdown
Collaborator

@Jacksper13 Jacksper13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Canceling the new preview produces a bogus error instead of a clean cancel.

The bug: in health_check_common_flow.py:show_message (PR-introduced code), cancel sets self.set_result(False). But the caller in health_check_microsd_flow.py:common_flow (line
~68) only treats None as failure:

self.signed_message = await HealthCheckCommonFlow(self.lines, normal_signing=self.normal_signing).run()
if self.signed_message is None: # <-- False is not None, so check fails
self.set_result(False)
return
self.goto(self.write_signed_file) # <-- proceeds with signed_message = False

Then write_signed_file constructs SaveToMicroSDFlow(data=False, ...), and SaveToMicroSDFlow.check_inputs (save_to_microsd_flow.py:50) hits (not self.data and not self.write_fn)
→ True, displaying "Either data or a write function is required to save a file." to the user.

So a user who backs out of the message preview sees a confusing error instead of returning to the menu. This is a user-visible regression introduced by this PR.

Comment thread ports/stm32/boards/Passport/modules/flows/health_check_common_flow.py Outdated
Comment thread ports/stm32/boards/Passport/modules/flows/health_check_common_flow.py Outdated
@mjg-foundation mjg-foundation requested a review from Jacksper13 May 11, 2026 19:35
@mjg-foundation mjg-foundation merged commit db51447 into dev-v2.4.0 May 11, 2026
19 checks passed
Jacksper13 added a commit that referenced this pull request May 11, 2026
Mirrors the WYSIWYG-signing assertion that microSD signing already has
(added in #636). Keeps self.address raw (used as expected_address) and
uses a local display_address for stylization in the preview.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
Jacksper13 added a commit that referenced this pull request May 11, 2026
Restores the file-size cap that the deleted SignTextFileFlow used to
provide before its consolidation in #636. Gated on normal_signing so
health-check files (with their own implicit length constraints) are
unaffected.

Co-Authored-By: Claude Opus 4.7 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants