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

Suite reacts poorly to unknown ButtonRequests #1642

Closed
matejcik opened this issue Apr 28, 2020 · 5 comments
Closed

Suite reacts poorly to unknown ButtonRequests #1642

matejcik opened this issue Apr 28, 2020 · 5 comments

Comments

@matejcik
Copy link

This breaks forward compatibility: I won't be able to use a newer fw with older Suite, if the new FW contains new ButtonRequestTypes (or possibly other enums too)

I implemented ButtonRequestType.PinEntry and send it for PIN entry dialogs. Can provide FW with this.
Now when I go to change PIN, after confirming "do you really want to enable PIN", Trezor freezes (because it does not receive a reply) and Suite shows:

Error: l.find(...) is undefined

Presumably some call somewhere is trying to resolve the ButtonRequestType and fails.
No errors in console, nothing relevant in "show log"

@tsusanka
Copy link
Contributor

tsusanka commented Apr 28, 2020

Responding to ButtonRequest with ButtonAck even for unknown types would help a lot (and maybe log such occurrence).

@matejcik
Copy link
Author

Noting that in the long run, we want to replace reliance on ButtonRequest codes with something smarter. So this is probably low priority.

@szymonlesisz
Copy link
Contributor

szymonlesisz commented Apr 28, 2020

suite it self doesn't touch transport/protobuf layer at all, it's a trezor-connect job, suite is just a consumer.
So whenever we add new ButtonRequest it needs to be implemented in trezor-connect and suite doesn't have to do anything. It will receive ButtonRequest from connect and display default "follow instructions on device" message

Responding to ButtonRequest with ButtonAck even for unknown types would help a lot (and maybe log such occurrence).

That's what is happening, every ButtonRequest gets Ack back:
https://github.com/trezor/connect/blob/develop/src/js/device/DeviceCommands.js#L667-L673

The problem is that trezor-connect received unknown protobuf message PinEntry and failed on parsing (inside trezor-link)

@tsusanka
Copy link
Contributor

tsusanka commented Apr 29, 2020

That's what is happening, every ButtonRequest gets Ack back:
trezor/connect:src/js/device/DeviceCommands.js@develop#L667-L673

The problem is that trezor-connect received unknown protobuf message PinEntry and failed on parsing (inside trezor-link)

Can we modify this that trezor-link does not fail on parsing in such case but only logs some warning and sends ButtonAck even for new unknown ButtonRequestType? Or we do not want it? Or it is not possible?

@szymonlesisz
Copy link
Contributor

sure we can, but this project needs a massive refactoring.
moving issue there

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

No branches or pull requests

3 participants