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

upgrade trezor-connect dependency to latest #8349

Closed
prusnak opened this issue Apr 17, 2020 · 91 comments · Fixed by #10192
Closed

upgrade trezor-connect dependency to latest #8349

prusnak opened this issue Apr 17, 2020 · 91 comments · Fixed by #10192
Assignees
Labels
area-hardware Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. type-bug

Comments

@prusnak
Copy link

prusnak commented Apr 17, 2020

Please upgrade the trezor-connect[1] dependency of your application to
latest (8.1.5 atm), otherwise your users won't be able to enter the
passphrase on the device.

Thanks!

[1] https://www.npmjs.com/package/trezor-connect

@whymarrh
Copy link
Contributor

whymarrh commented May 4, 2020

We are currently using 7.0.3. The set of breaking changes in v8 is here: CHANGELOG.md

@jcbit
Copy link

jcbit commented Jun 12, 2020

bump

@ghost
Copy link

ghost commented Jun 12, 2020

When will the new version be released which addresses the trezor connect issue prusnak mentioned above?

Thanks

@blockomoco
Copy link

any update to this ?

@whymarrh
Copy link
Contributor

otherwise your users won't be able to enter the passphrase on the device.

What specifically doesn't work? And under what circumstances doesn't it work? I don't understand and this issue doesn't have specific details.

@dpazdan
Copy link

dpazdan commented Jun 29, 2020

further details mentioned here: https://www.reddit.com/r/TREZOR/comments/hhs9k1/what_is_this_error_chrome_8304103116_official/

and a support ticket related to this issue: 38789

@whymarrh
Copy link
Contributor

We'll hopefully update our Trezor logic in 8.1 but in the meantime I do believe it still works.

@epop-cs
Copy link

epop-cs commented Jun 29, 2020

Hello @whymarrh,

#8866 was closed and I was asked to comment here. I do not believe these two tickets represent the same issue. This one is referring to the Trezor Connect API version 8.1.7, while the issue the user reported to support refers to the connection he/she is attempting to make to their physical Trezor wallet through the Metamask UI. It clearly mentions the 2.3.1 version in the screenshots posted inside the initial issue.
switccheo

Can you please advise?

Regards,

Emil

@blockomoco
Copy link

blockomoco commented Jun 29, 2020

Currently (with the outdated Trezor integration) one needs to type the Trezor passphrase in the browser which is not secure. A major security aspect of hardware wallets is to avoid typing secrets into potentially compromised devices (like a computer with internet access). Also, the passphrase needs to be typed in to sign every single transaction, which is bad UX and fairly annoying to be frank.

According to the Trezor team (I sanity checked with them), if you upgrade the Trezor integration these 2 issues should be resolved. So that:
a) the passphrase can be typed in on the Trezor device instead of typing it in the browser
b) one would not need to type the passphrase every time in order to sign transactions

@ncsolar
Copy link

ncsolar commented Jun 29, 2020

Trezor-error

Same warning here, Chrome Version 83.0.4103.116 (Official Build) (64-bit)

@ghost
Copy link

ghost commented Jun 29, 2020 via email

@whymarrh
Copy link
Contributor

whymarrh commented Jun 29, 2020

@epop-cs as @ncsolar and @dpazdan have shared, these do all seem to be related.

@blockomoco thanks for the details, that is clearer.

We'll be working to get this in 8.1, the version after next, and will update here with progress. We understand that this isn't a smooth process right now, we're working to fix it.

@blockomoco
Copy link

blockomoco commented Aug 8, 2020

Any update when this will be rolled out ?
@whymarrh

@whymarrh
Copy link
Contributor

whymarrh commented Aug 10, 2020

Nothing right now, I'll be sure to update here with progress

@HeroHann
Copy link

As it seems the problem still exists. (I guess Brave Wallet is using MetaMask?!. -> So they are having the same problem.)

It is NOT: "not going so smooth"
It IS: "not usable for all Trezor users as this is a security risk"

Please fix this asap.

@blockomoco
Copy link

+1 this is a serious security risk since the password should only be typed on the Trezor device directly and never on a computer with internet access.

Please prioritize this fix. The right thing to do here is fixing and releasing the fix ASAP.

@whymarrh

@steelbom
Copy link

steelbom commented Aug 19, 2020

I also have to second this - an urgent fix would be great.

The issues I'm having:
1. I am unable to move funds from my Trezor (Model T) wallet through metamask.
2. I am unable to transact with that wallet in any platform that utilises metamask (uniswap, etc.)
3. I am forced to enter my passphrase in plain text, rather than having the option to enter it on the device.

About two weeks ago it was working fine on the older version of the Trezor firmware that I was using. I updated to 2.3.2 and I have also updated Trezor Connect as well, but now this issue occurs. This is in Vivaldi, on Windows 10 Pro 2004, with adblock disabled.

Error: link

EDIT: I have confirmed that issue #1 and #2 were unrelated. However, like many others, issue #3 persists.

@danfinlay
Copy link
Contributor

So it sounds like one of the breaking changes Trezor introduced was related to how passwords are entered when connecting with MetaMask?

Does that explain this user's experience? They claim they are unable to use BIP39 password derived accounts at all anymore.

@Prince-Fish
Copy link

Prince-Fish commented Sep 2, 2020

Bump - this issue effectively breaks secure use of Trezor with Metamask. Due to its ubiquity, Trezor users are forced to rely on Metamask to interact with web3 applications. Current version of Metamask effectively breaks possibility of secure Trezor use.

@blockomoco
Copy link

What is the status of this ? It is shocking that this is still unresolved.

Security issues must have the highest priority and urgency. Especially for hardware wallets where people may store significant amount of funds.

Please take action.

@HeroHann

This comment has been minimized.

@danfinlay
Copy link
Contributor

We've begun work on this issue, but have encountered some problems, documenting here to share with Trezor:

The problem is that when trying to connect Trezor, we are redirected to https://connect.trezor.io/8/popup.html#loading and it’s getting stuck on loading screen (exactly the same way when we manually open this site), there’s nothing more we can do. I have latest firmware on my model T and latest trezor-bridge versions.

  1. I tried updating trezor-connect to many different 8.x.x versions (including extended one), but none of them works - extended versions runs some unit tests, but should resolve if we have a public key test is getting timeout from TrezorConnecty.getPublicKey() function. Also tried using .ethereumGetPublicKey() , but it behaves in the same way
  2. Same function works fine using https://trezor.github.io/connect-explorer/#/
  3. I tested same function using their example from https://github.com/trezor/connect/tree/develop/examples/webextension and it also works so it looks like the problem of implementation, not firmware/bridge versions
  4. I also checked how MEW has this implemented and it also looks almost the same :man-shrugging:

Any tips or suggestions will be appreciated.

@blockomoco
Copy link

Thx for working on this.

@prusnak can you folks please help. Thx

@snazzybytes
Copy link

snazzybytes commented Jan 2, 2021

@whymarrh @danfinlay @yohaaan
would this PR be of any help to get the ball rolling? #10131
disclaimer, i have no clue about this codebase, just simply bumping versions and see if the tests pass (some failed, adjusted the expected mock data to get them to pass).

@gotchai
Copy link

gotchai commented Jan 3, 2021

@snazzybytes Thanks for trying to make it done ! Maybe we should try to contact them on twitter or any other way.

@jacobc-eth jacobc-eth added Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. and removed chore labels Jan 3, 2021
@snazzybytes
Copy link

snazzybytes commented Jan 4, 2021

@stx233 yea no problem, sadly i do not have tribal knowledge to actually get it properly done obviously (i work as software engineer, but on the backend side of things with different set of technologies JVM and Golang and other things and languages, not the NPM and javascript on the daily though as is the case here, sorry)

@stx233 also Jacob from MetaMask added the "S1-High" label below your reply, which is a good sign it will be prioritized and squashed soon I hope I'd imagine logically 👨‍🚀 speaking (tagging you twice, in case you notice the good news sooner, cuz it certainly put a smile on my face)

@danfinlay
Copy link
Contributor

danfinlay commented Jan 5, 2021

@yohaaan wrote:

sir this issue has been created by Trezor CTO, we were pro-actively offering help and provided Metamask with info about the change in a timely manner, there is no reply from their side, unfortunately.

This is inaccurate. The MetaMask official replies have ironically become buried by people asking where our replies are. I'm going to lock the thread so that my response here can be more easily found, and we'll be providing updates here as we develop this issue.

We had a developer spend a week on this integration a couple months ago, but ran into some issues introduced by the new Trezor library update that we were not able to overcome with the time allotted to it. We will be spending more time tracking these issues down soon, but needed to focus on some wider-impact improvements, like transaction reliability.

Here were the outputs of his efforts:

The problem is that when trying to connect Trezor, we are redirected to https://connect.trezor.io/8/popup.html#loading and it’s getting stuck on loading screen (exactly the same way when we manually open this site), there’s nothing more we can do. I have latest firmware on my model T and latest trezor-bridge versions.
I tried updating trezor-connect to many different 8.x.x versions (including extended one), but none of them works - extended versions runs some unit tests, but should resolve if we have a public key test is getting timeout from TrezorConnecty.getPublicKey() function. Also tried using .ethereumGetPublicKey() , but it behaves in the same way
Same function works fine using https://trezor.github.io/connect-explorer/#/
I tested same function using their example from https://github.com/trezor/connect/tree/develop/examples/webextension and it also works so it looks like the problem of implementation, not firmware/bridge versions
I also checked how MEW has this implemented and it also looks almost the same :man-shrugging:
I have no clue what is going on, so any tips or suggestions will be appreciated.

Those efforts are on some slightly stale branches, so we're going to bring them up to date before sharing again here, where we're happy to get input from the Trezor team.

We'll also try out the suggested cache-clearing idea during the next sprint we give to Trezor.

We'd also love to have more direct communication with the Trezor team, maybe we could get a shared Slack or Discord going like we have with Ledger, so we can interact a bit more tightly. Please do reach out.

Again, locking this thread as our official replies keep getting buried with wild speculation.

@MetaMask MetaMask locked as off-topic and limited conversation to collaborators Jan 5, 2021
@MetaMask MetaMask deleted a comment from blockomoco Jan 5, 2021
@MetaMask MetaMask deleted a comment from blockomoco Jan 5, 2021
@MetaMask MetaMask unlocked this conversation Jan 9, 2021
@darkwing
Copy link
Contributor

@prusnak Thank you for reporting this issue and driving us forward!

After a few days of debugging, I've found the same issue that the previous engineer found (#8349 (comment)).

Essentially, the TrezorConnect.getPublicKey call never resolves and the user indefinitely sees the loading spinner at https://connect.trezor.io/8/popup.html#loading .

TrezorConnect

No error or warning is reported in the JS console, nor does a promise catch -- just infinite loading screen.

The MetaMask code that calls the TrezorConnect.getPublicKey is in another repository; I've pushed a PR with the update to "trezor-connect@extended": "^8.1.19" here: MetaMask/eth-trezor-keyring#46

A community-submitted PR, #10131 , effectively shims MetaMask to use "trezor-connect@extended": "^8.1.19" which allows you to see the MetaMask-to-Trezor connect flow. Since both repositories show TrezorConnect.getPublicKey stalling, I don't believe it's a specific MetaMask integration issue.

Any advice would be appreciated. This is my top priority and I'll be looking to act as quickly as your team can help. Thank you!

@prusnak
Copy link
Author

prusnak commented Jan 10, 2021

@darkwing Are you 100% sure there is no browser extension messing up with the communication? (privacy badger, adblock, etc.) Have you tried Chrome also?

Pinging @szymonlesisz our Connect guru: Szymon, any ideas, what might be wrong with the Connect integration used by Metamask?

@darkwing
Copy link
Contributor

I read the changelog for 8.0.0 (https://github.com/trezor/connect/blob/develop/CHANGELOG.md#800) and the first point mentioned a communication change but if postMessage is a fallback, I would think everything should work properly on that front, since version 7 has worked for us without problems.

I look forward to any advice from @szymonlesisz ; one question to consider is what could cause the connect website to infinitely show a loading screen. Thank you!

@Gudahtt
Copy link
Member

Gudahtt commented Jan 12, 2021

I've just been testing with MetaMask/eth-trezor-keyring#46 on Chrome (Version 87.0.4280.88 (Official Build) (64-bit)) on Ubuntu, by linking that version of eth-trezor-keyring with the develop branch of the extension. I'm using an otherwise empty Chrome profile, so no blockers. I'm seeing the exact same thing as @darkwing.

@darkwing
Copy link
Contributor

Today we found that there was a seemingly undocumented breaking change to Trezor Connect 8 that, once we fixed, will allow Trezor to work on that version:

https://gist.github.com/darkwing/b68125069533628ea2c27445cb253b99

I'm working to complete the upgrade in MetaMask/eth-trezor-keyring#46; the only current blocker is that our test suite's TrezorConnect.getPublicKey test hangs because that method no longer throws an error when called. In previous versions, it would throw a missing document error, but with the extended package, that's no longer the case. I'm working now to figure out how to fix our tests.

@blockomoco
Copy link

blockomoco commented Feb 3, 2021

@darkwing Which Metamask browser version is going to include this change ?

edit: Ok I see it is under https://github.com/MetaMask/metamask-extension/releases/tag/v9.0.4.
Thx

@shaimo
Copy link

shaimo commented Feb 4, 2021

Any eta for this? I’m still unable to connect my Trezor...

@chong-he
Copy link

chong-he commented Feb 4, 2021

Any eta for this? I’m still unable to connect my Trezor...

Hey the latest Metamask already solved this, v9.0.4.

PS: Thanks to the team, very glad to be able to use Trezor with Metamask now

@shaimo
Copy link

shaimo commented Feb 4, 2021

I’m using v9.0.4 and it doesn’t work for me. Gave up on it...

@chong-he
Copy link

chong-he commented Feb 4, 2021

I’m using v9.0.4 and it doesn’t work for me. Gave up on it...

It really should work. To be clear, we are talking about the issue of inserting passphrase on Trezor T device, not computer.

Do you mean you cannot connect Trezor?

@shaimo
Copy link

shaimo commented Feb 4, 2021

I simply can’t connect Metamask to Trezor. It opens a popup for a second in another tab and then disappears, and then I get an ever going spinning wheel and that’s it. Tried it with two different trezors, and in both Brave and Chrome. Always the same

@Gudahtt
Copy link
Member

Gudahtt commented Feb 4, 2021

@shaimo you're likely experiencing a different problem than what is described in this issue. Could you create a separate issue, and provide any details you can to help us reproduce the problem?

@shaimo
Copy link

shaimo commented Feb 4, 2021

@Gudahtt #10372
Thanks!

@gotchai
Copy link

gotchai commented Feb 5, 2021

Version 9.0.4 is okay.

It recognized trezor model T and we can use it with metamask.

But there is an issue. Here is what I found :

  • Sending BNB from bsc wallet to another bsc wallet on trezor T with metamask : Ok no problem
  • Sending from Trezor T wallet with metamask using bsc to another bsc wallet : it does not work and displays transaction failed (I tried several times, with different browsers FF, chrome, brave). It does not trigger the transaction address confirmation for your information on the model T screen.

how can I display the output message of in order to help you ?

Edit : Transaction 0 failed! Error : Unsupported chain id is displayed
Edit 2 : I am using Binance smart chain

@gotchai
Copy link

gotchai commented Feb 10, 2021

Hi all after firmware 2.3.5 | 10 February 2021

I can confirm you that everything is working with bsc : metamask and trezor. Now validation on bsc is triggered on the Model T.
Congrats to both teams Metamask and Trezor !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-hardware Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.