Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow canceling on uncancelable pages on T3B1 #4506

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Disallow canceling on uncancelable pages on T3B1 #4506

merged 1 commit into from
Jan 16, 2025

Conversation

ibz
Copy link
Contributor

@ibz ibz commented Jan 15, 2025

This PR fixes an issue that happens when the "middle" button is pressed on a page that should not handle middle or left button presses. If, when releasing the "middle" button (left + right), the left one is the first one to be released, this triggered a left button press which would cancel an otherwise uncancelable page, like this one:

image

To reproduce the bug (before the fix):

  • trezorctl device setup
  • keep confirming ever page until you arrive at the page shown in the screenshot above
  • press the left button and keep it pressed
  • while the left button is pressed, press the right button and keep both pressed
  • release the left button
  • now the process is canceled even though it should not be possible to cancel it

Note: this fix affects both the ButtonController and the ButtonPage. Normally one of the two would be enough to fix the issue, but I decided to fix it in both places that have knowledge of the missing cancel button. This duplication indicates that there is some redundancy in the logic that could be perhaps refactored.

@ibz ibz self-assigned this Jan 15, 2025
@ibz ibz linked an issue Jan 15, 2025 that may be closed by this pull request
Copy link

github-actions bot commented Jan 15, 2025

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3T1 Safe 5 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@ibz ibz changed the title Disallow canceling on uncancelable pages Disallow canceling on uncancelable pages on T3B1 Jan 15, 2025
@ibz ibz marked this pull request as ready for review January 16, 2025 07:13
@ibz ibz requested a review from prusnak as a code owner January 16, 2025 07:13
@ibz ibz requested review from matejcik and obrusvit and removed request for prusnak January 16, 2025 07:13
@matejcik
Copy link
Contributor

code ACK. did you intend to do the click-test, like we talked about?

@ibz
Copy link
Contributor Author

ibz commented Jan 16, 2025

did you intend to do the click-test, like we talked about?

I actually did not manage to write a proper click test that replicates the bug.

Basically I need to simulate releasing one button while the other one is still pressed. This should in theory work, because one can pass a hold_ms to the button click and core/src/apps/debug/__init__.py implements this by doing an await which means the next button press should get a chance to happen while the first one is still pressed. But in my observations, a 2nd button press always happened only after the first one was released (even if the hold_ms of the first press was larger) - so I don't really know what happens there.

I also thought of somehow making the "middle button" know which button it should release first (since there is always an order in which they are released) - but that means modifying the debug protobuf message somehow, which seems too much for this tiny corner case.

What do you think?

@matejcik
Copy link
Contributor

yeah, that would be problematic. let's leave it without a test then.

Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

utACK

@ibz ibz merged commit dd3bf25 into main Jan 16, 2025
95 checks passed
@ibz ibz deleted the ibz/cancel branch January 16, 2025 12:54
@bosomt
Copy link

bosomt commented Jan 17, 2025

QA OK

  • i can hold LB and still navigate via RB when there is not possible to navigate via LB and possible to navigate via RB
  • i can hold RB and navigate v LB when there is possible to navigate via RB
  • i was not able to replicate issue anymore
  • hold to .... works as expected
  • middle button works as expected

Info:

  • Device: Trezor T3B1 2.8.7 regular (revision dd3bf25)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

Backup fails unexpectedly when certain keys are pressed on TS3.
3 participants