upgrade trezor-connect dependency to latest

prusnak commented 4 years ago

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 commented 4 years ago

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

jcbit commented 4 years ago

bump

ghost commented 4 years ago

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

Thanks

blockomoco commented 4 years ago

any update to this ?

whymarrh commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

epop-cs commented 4 years ago

Hello @whymarrh,

https://github.com/MetaMask/metamask-extension/issues/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 commented 4 years ago

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 commented 4 years ago

Trezor-error

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

ghost commented 4 years ago

Have a look at how Myetherwallet does it. They have it setup correctly, so that you can type your passphrase on your device, rather than a keyboard.

On Mon, Jun 29, 2020 at 3:54 PM ncsolar notifications@github.com wrote:

[image: Trezor-error] https://user-images.githubusercontent.com/8175508/86021101-57865d80-b9ee-11ea-871e-59b7f5c92269.png

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

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MetaMask/metamask-extension/issues/8349#issuecomment-651173909, or unsubscribe https://github.com/notifications/unsubscribe-auth/AP57EHBRNVZQEWZLX62WRBDRZCTLJANCNFSM4MKSI3ZA .

whymarrh commented 4 years ago

@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 commented 4 years ago

Any update when this will be rolled out ? @whymarrh

whymarrh commented 4 years ago

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

HeroHann commented 4 years ago

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 commented 4 years ago

+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 commented 4 years ago

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.)~~

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@PatrykLucka @jennypollack Please update us!

I am no dev, but is it that hard?

danfinlay commented 4 years ago

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.

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:

Any tips or suggestions will be appreciated.

blockomoco commented 4 years ago

Thx for working on this.

@prusnak can you folks please help. Thx

prusnak commented 4 years ago

@danfinlay Show us the code - maybe push this into a branch somewhere?

Prince-Fish commented 4 years ago

So, I went to make some trades today, and surprise this isn't fixed.

What's the update on timing for this critical security vulnerability / functionality break that was disclosed half a year ago?

gruvin commented 4 years ago

LOL sigh Crazy! :'( WTF?

steelbom commented 4 years ago

Any update on this?

ghost commented 4 years ago

@danfinlay a funny suggestion but have you tried testing trezor.io/8 after clearing the cache (https://trezor.io/support/technical/stuck-at-loading/ )? I sometimes get stuck on the loading page as well when trying to connect the trezor with metamask. Usually clearing the cache does the trick for me...

Hope you guys manage to fix this soon :/

chong-he commented 4 years ago

Still waiting the issue to be fixed

mkalen commented 4 years ago

MetaMask does not work anymore with Trezor Model T and the latest firmware (2.3.3), p.find is undefined - see screenshot bild @danfinlay Did you see the offering from prusnak to review code e.g. on a branch? Thanks.

blockomoco commented 4 years ago

@prusnak @danfinlay status ?

Prince-Fish commented 4 years ago

I understand that this is one issue of many. To support fixing this issue, I will donate 0.1 Eth to whoever gets this done by Oct 12 '20

I look forward to sending that amount from my Trezor.

ghost commented 4 years ago

I am not familiar with web dev, but it seems to me that we only need to update dependencies in yarn.lock.

Specifically these two blocks:

(here we need to change to 8.1.15)

trezor-connect@^7.0.1:
  version "7.0.3"
  resolved "https://registry.yarnpkg.com/trezor-connect/-/trezor-connect-7.0.3.tgz#70c4bc26c0966e794fc280a12c1acc9fef88864f"
  integrity sha512-1Y1ajCDF8dC5d2yrCUmVkNqXeOlucamQ6j6Ko7kaqNdge3g9KZ+O48jUwP/eGzei8oUvPZUHd7o4OhDHTlpLCw==
  dependencies:
    "@babel/runtime" "^7.3.1"
    events "^3.0.0"
    whatwg-fetch "^3.0.0"

and

(in here we need to change the last line to trezor-connect "^8.1.15")

eth-trezor-keyring@^0.4.0:
  version "0.4.0"
  resolved "https://registry.yarnpkg.com/eth-trezor-keyring/-/eth-trezor-keyring-0.4.0.tgz#f59c210f95aaf3d7321ae69d2b87a3b8db96a828"
  integrity sha512-7F+C1ztxZStLzmG6r/2/MxjSuxw0aU9T26unJ03fQslktKG9izP+dU2IAJUnWxnyej2ZkfcgcH9M1t32LFbK2A==
  dependencies:
    eth-sig-util "^1.4.2"
    ethereumjs-tx "^1.3.4"
    ethereumjs-util "^5.1.5"
    events "^2.0.0"
    hdkey "0.8.0"
    trezor-connect "^7.0.1"

I will test this later myself and see if it works.

mkalen commented 4 years ago

I am not familiar with web dev, but it seems to me that we only need to update dependencies in yarn.lock.

Unfortunately that's not it. If you read up on the history of this issue you see that there are well documented API breaking changes on the Tezor side and this means more work for integrators (semantic versioning will signal this is the increased major version number, here from 7 to 8, that's also natural as API:s progress).

But you can also see that MetaMask side has aldready upgraded calling code and have it ready for testing. The blocker is that it didn't work and Trezor then offered to help with code review. Solution is within reach.

(I'm not in either team BTW, a MetaMask-user with a Trezor - like you.)

ghost commented 4 years ago

@mkalen I see, too bad then :/ Hopefully, it won't take much longer until this is fixed. I read your previous message regarding a failed transaction with model T (2.3.3) via metamask, its strange cause I still can transact without failures, though it is displaying the same warning posted by ncsolar https://github.com/MetaMask/metamask-extension/issues/8349#issuecomment-651173909

sdtsui commented 4 years ago

Hi - previous contributor to MetaMask here.

I've been getting a lot of questions about Trezor connectivity issues from friends and former clients. While this note may not apply to you -- it's a simple fix if it actually does. I debated opening a new issue or contacting a team member directly, but decided on commenting here since given how long this issue has been open, doing it this way is most likely to reach/help the correct people while being unlikely to confuse others. Admins/Mods, lmk if this comment is better suited for someplace else.

I don't have time enough to go through the full thread, but what I've noticed is that: (i) some users are getting errors like @mkalen 's - p.find(...) undefined with the latest firmware. (ii) some users are getting what amounts to a timeout - in a web wallet, Trezor does not get recognized and is the user is never prompted to export their public key, or enter their PIN. This is closer to what @danfinlay is describing on 09/03.

For users who are getting (i) - this message does not apply. I don't know how to help you yet. For users who are getting (ii), especially if you can get BTC to display, or get it working on other interfaces (like MEW). it might be a quick fix for you. It is some sort of browserside cache issue. See below:

https://www.reddit.com/r/TREZOR/comments/j5awtq/my_trezor_t_is_synchronized_with_exodus_i_have/ https://np.reddit.com/r/ExodusWallet/comments/j5axtu/my_trezor_t_is_synchronized_with_exodus_i_have/g7qwm7g/

For some reason, simply clearing the history in chrome did not work. All I had to do for these users was install brave and set up wallet with Brave's internal fork of MetaMask - the Trezor was recognized right away. This applies to both Trezor Model T and One. This worked despite firmware being out of date. I think this is because it's a clean install, and the Trezor knows to start a new session whereas with the old browser it's expecting an old session with dat either it or bridge no longer has.

I think that it arises when one uses a particular Trezor device with multiple computers, potentially with those computers using multiple hardware wallets. Sometimes, an error "can't find session" was displayed in the browser window after a long (>2min) timeout -- indicating that Bridge was working correctly, but the Trezor felt it had some kind of active session with another computer it could no longer pair with. I don't know why MyCrypto sometimes works -- it might have to do with how they talk to the browser and bridge.

For the MetaMask team - this is where I'd look to debug. Assume the device is has multiple active sessions and might be getting confused -- MyCrypto might be instructing bridge to create a new one and that's why it's working.
For the Trezor team - I think it might be good to look into this with both MyCrypto and MetaMask teams, so that if this error happens again in the future you get a specific error message for it, maybe with the specific device/session the Trezor is looking before it times out. One device I tested with had 10+ sessions and none of them applied to any of the 2 computers we tried, and the Trezor had never been connected to any computers but those. This tells us that it's losing sessions?

In sum - if you use multiple hardware wallets on multiple computers - try using a completely fresh install of all touchpoints (Bridge, Browser + MetaMask, not just reinstalling MetaMask or clearing browser history). I have been able to get this fixed for multiple people who never had Brave installed on their computer, just by installing Brave and getting the Trezor to talk to it.

Otherwise (users with (i)), carry on - it's probably best to wait until they get it all working and release a new version.

Stay safe out there. Standard disclaimer when commenting on wallet stuff in public forums: try these suggestions at your own risk, I am not responsible if your stuff breaks even more as a result of trying this, etc...

ghost commented 4 years ago

@sdtsui I had a similar problem with being stuck on the loading page when connecting the trezor wallet and also got around this by clearing the cache (like you mentioned). Texted this https://github.com/MetaMask/metamask-extension/issues/8349#issuecomment-702332962 but no reply yet :/

yohaaan commented 4 years ago

@whymarrh hi, any update on this? Trezor users keep facing this issue. Thanks!

unicornioPT commented 4 years ago

6 months later and this has not yet been upgraded?! Unbelievable... It's just trezor wallet right... No big deal...

vondra commented 4 years ago

+1 solving the issue.

Message from Trezor support: “Now it is up to MetaMask developers to update Trezor Connect to the latest version. We have already started the task by creating the issue in their GitHub repository. You can follow the progress under the following link: https://github.com/MetaMask/metamask-extension/issues/8349

You can alternatively downgrade your firmware to version 2.3.0 which is compatible with older Trezor Connect. Please see our user manual: https://wiki.trezor.io/Firmware_downgrade ”

blockomoco commented 4 years ago

Can someone from Metamask share status pls. @whymarrh @danfinlay

Prince-Fish commented 4 years ago

Again another week of no update. My displeasure and disappointment with metamask at this point is immense. At this point the only thing I can do is vote with my feet and use another wallet.

6 months to update a damn integration that has been proven to work across other platforms (completed by much smaller and less sophisticated teams). Shameful.

unicornioPT commented 4 years ago

@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.

this did not age well

ghost commented 4 years ago

@danfinlay Can we please get an update on this?

chong-he commented 3 years ago

Hope the issue can be resolved soon

gruvin commented 3 years ago

fwiw, imo, it is Trezor that have let us all down. They were the ones who made the breaking changes. They know Metamask is as popular as it is and that it support(ed) Trezor. It is Trezor's product that is losing out ...

Im going to stop using Trezor as a consequence.

Just exactly how Metamask (or anyone else) is supposed to write code to allow entry of passphrase using only buttons on the Trezor 1 is beyond me and frankly laughable. No one would use it because it would be a horrible user experience.

I have three Trezor 1's and I've already ditched them all. I recently bought a $$$$ Ledger X (BT) but I can't stand the thing. It's horrible by comparison and the expen$ive BlueTooth feature DOESN'T WORK on desktops! So screw them too.

Metmask is awesome, especially on Mobile. Hardware Wallets seem to pretty much all suck the proverbial. Paper cold wallets are FAR superior for storing large sums of crypto anyway.

alangrainger commented 3 years ago

@gruvin I don't have a One, but I believe this feature is just for the T, so I'm not aware that there would be additional UI code to write - just an upgrade of the trezor-connect dependency..... which really seems like it shouldn't have taken 8 months an counting.

If any dev wants to get this done by the end of the year, I'll throw in a US$500 bounty in the crypto of your choice.

yohaaan commented 3 years ago

When reflecting on this ticket, the lack of responsibility (and maybe even capability) is stunning. Im going to stop using Trezor as a consequence. Fairly speaking, the responsibility lies primarily in Metamask's dev team. Although Im not sure if the Trezor team could have been more engaged to resolve this issue (?). Don't care any more to be honest.

Because Metamask is so wide spread among Dapps, Im currently forced to keep on using it. Moving forward only with Ledger.

Unfollowing this thread. Good luck to you all and feel sorry for the users of Metamask + Trezor.

We as Trezor has informed Metamask regarding the change in timely manner and also we do keep reminding them (this issue was created by us as well), unfortunately with no reaction from Metamask so far. You can use Trezor with other ETH Wallets including our native ETH Wallet available at https://suite.trezor.io/

unicornioPT commented 3 years ago

Out of curiosity... How many lines of code/hours are we talking about here? This does not seem to be such a big upgrade.

On Mon, 14 Dec 2020, 13:05 Jan, notifications@github.com wrote:

When reflecting on this ticket, the lack of responsibility (and maybe even capability) is stunning. Im going to stop using Trezor as a consequence. Fairly speaking, the responsibility lies primarily in Metamask's dev team. Although Im not sure if the Trezor team could have been more engaged to resolve this issue (?). Don't care any more to be honest.

Because Metamask is so wide spread among Dapps, Im currently forced to keep on using it. Moving forward only with Ledger.

Unfollowing this thread. Good luck to you all and feel sorry for the users of Metamask + Trezor.

We as Trezor has informed Metamask regarding the change in timely manner and also we do keep reminding them (this issue was created by us as well), unfortunately with no reaction from Metamask so far. You can use Trezor with other ETH Wallets including our native ETH Wallet available at https://suite.trezor.io/

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/MetaMask/metamask-extension/issues/8349#issuecomment-744365262, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHNQS6GPBYUJXCK3KTVHOQLSUXWPTANCNFSM4MKSI3ZA .

yohaaan commented 3 years ago

@unicornioPT I cannot tell really as I'm not familiar with Metamask app (as we do not develop it) but other services have been able to adopt it really quickly, it shouldn't be that big of a change. We've also offered Metamask help (should they be facing any issue with implementation on their side) so the ball is really on their side.

MetaMask / metamask-extension

upgrade trezor-connect dependency to latest #8349