Skip to content

fix(decode_qr): validate signmessage QR payload structure before indexing (fixes #1058) - #1059

Open
muskan-creates352 wants to merge 1 commit into
SeedSigner:devfrom
muskan-creates352:fix-signmessage-qr-indexerror
Open

muskan-creates352 wants to merge 1 commit into
SeedSigner:devfrom
muskan-creates352:fix-signmessage-qr-indexerror

Conversation

@muskan-creates352

Copy link
Copy Markdown

Fixes #1058

Description

When scanning a single-frame QR payload starting with signmessage, SignMessageQrDecoder.add() previously performed direct indexing on parts[1], parts[2], and format splitting without validating the structure first. Scanning an incomplete or malformed QR code (e.g. signmessage or signmessage m/84h/0h/0h) resulted in an unhandled IndexError: list index out of range and a crash rather than returning DecodeQRStatus.INVALID.

Changes Made

  1. src/seedsigner/models/decode_qr.py:

    • Added validation check to ensure len(parts) >= 3 before accessing positional elements.
    • Added check for format prefix separator : in parts[2].
    • Added safe extraction for message content after f"{fmt}:".
    • Returns DecodeQRStatus.INVALID gracefully with informative logging if validation fails.
  2. tests/test_sign_message_qr_decoder.py:

    • Created dedicated test suite for SignMessageQrDecoder.
    • Added test for valid signmessage payload decoding and derivation path normalization.
    • Added test for messages containing spaces.
    • Added tests for malformed payloads with insufficient parts (verifying no IndexError is raised).
    • Added test for missing colon format prefix and unsupported formats.

Testing

  • Ran new unit tests with pytest (6 passed in 0.27s).
  • Ran full test suite across the project; all 319 existing tests continue to pass.

@wolgwang1729 wolgwang1729 left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM. Reviewed the changes and tested the decoder thoroughly.

The fix resolves the unhandled IndexError reported in #1058 when scanning incomplete signmessage QR codes. All 6 unit tests added in tests/test_sign_message_qr_decoder.py pass as expected.

Apart from the tests included in the PR, I also verified the decoder against these additional payloads:

Payload Decoder Status Result
signmessage ascii:hello INVALID Missing derivation path token caught
signmessage m/84h/0h/0h :msg INVALID Missing format prefix before colon rejected
signmessage m/84h/0h/0h ASCII:Hello INVALID Case-sensitive format requirement enforced
signmessage m/84h/0h/0h ascii: COMPLETE Empty message parsed safely without exception
signmessage m/84h/0h/0h ascii:Line 1\nLine 2 COMPLETE Multi-line messages preserved
signmessage m/84h/0h/0h ascii:Time: 12:00:00 COMPLETE Multiple colons inside message text preserved
signmessage m/84h/0h/0h ascii:A B COMPLETE Irregular inter-token whitespace handled
signmessage m/84'/0'/0'/0/0 ascii:Hello COMPLETE Pre-normalized single-quote derivation path supported

Valid signmessage payloads continue to decode without regression.

Also, kindly follow the repository PR template (.github/pull_request_template.md).

@wolgwang1729

Copy link
Copy Markdown

For additional context, I came across a few edge cases that fall slightly outside this decoder patch, but may be worth noting or addressing in a follow-up:

  1. Short / partial derivation paths downstream:
    Three-token payloads with a short derivation path (such as signmessage m/84h ascii:Hello or relative paths like signmessage 0/0 ascii:Hello) satisfy len(parts) >= 3 and return COMPLETE. When Message Signing is enabled in settings, these short paths trigger an IndexError downstream in embit_utils.parse_derivation_path() at the view level. This is pre-existing behavior on dev, but worth keeping in mind if the goal is making the entire sign-message flow resilient to malformed QR inputs.

  2. Multisig derivation paths (m/48h):
    Standard BIP-48 multisig paths (e.g. signmessage m/48h/0h/0h/2/0 ascii:Hello) decode as COMPLETE, but raise Exception("Not implemented") downstream in parse_derivation_path(). Gracefully routing these to NotYetImplementedView could be handled in the view layer.

  3. Command token matching:
    Because DecodeQR.detect_segment_type() checks s.startswith("signmessage"), variations like signmessage_v2 or signmessage:foo are routed to this decoder and accepted as long as there are three space-separated tokens. Checking parts[0] == "signmessage" in the decoder could strictly enforce the expected command name.

  4. Format prefix splitting:
    segment.split(format_prefix, 1) searches from the start of the whole string. In the unlikely event that a derivation path contains the substring ascii:, splitting occurs there instead of at the third token.

None of these are regressions from this PR, but tracking them could help make the sign-message pipeline fully robust end-to-end.

@muskan-creates352

Copy link
Copy Markdown
Author

Thank you so much for the thorough review and testing across all those edge cases, @wolffgang1729! Really appreciate the detailed feedback.

Regarding the additional downstream cases and command/format token handling you mentioned, I would be more than happy to tackle them in follow-up issues and PRs once this lands!

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.

SignMessageQrDecoder raises unhandled IndexError on malformed signmessage QR data

2 participants