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

Adds support for delayed injection. #56

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MicahZoltu
Copy link

A privacy respecting wallet will not inject into the page until after the user tells it to so websites cannot fingerprint the user. Unfortunately, this script as currently written doesn't support this pattern because it only calls handleEthereum once and if the timeout is hit before ethereum is injected this will unsubscribe from the event listener and refuse any further attempts at connecting things up.

By checking for ethereum in window first in handleEthereum, if the timeout is hit but nothing is hooked up yet then the system will continue listening for the ethereum#initialized event indefinitely.

A privacy respecting wallet will not inject into the page until *after* the user tells it to so websites cannot fingerprint the user.  Unfortunately, this script as currently written doesn't support this pattern because it only calls `handleEthereum` once and if the `timeout` is hit before `ethereum` is injected this will unsubscribe from the event listener and refuse any further attempts at connecting things up.

By checking for `ethereum` in window first in `handleEthereum`, if the timeout is hit but nothing is hooked up yet then the system will continue listening for the `ethereum#initialized` event indefinitely.
@MicahZoltu MicahZoltu requested a review from a team as a code owner July 24, 2022 16:36
@mcmire
Copy link
Contributor

mcmire commented Oct 3, 2022

Hi @MicahZoltu, sorry for taking so long to reply to this. Can you give us an example of which wallet this pattern would support? And what do you mean by "A privacy respecting wallet will not inject into the page until after the user tells it"? How would the user tell it? Really, I'm looking for a way I can replicate this issue so that we can confirm that it's fixed by your change.

@Gudahtt
Copy link
Member

Gudahtt commented Oct 3, 2022

This package assumes that the provider is injected right away (possibly asynchronously, but still initiated without delay), but some wallets might not do that. If a dapp uses this package to detect the provider, but the user chose to inject the provider five minutes after loading the page, the timeout would have already expired. The result would be a console error about there being no provider, and the dapp would have been handed null in place of a provider. The dapp also wouldn't be notified of the late injection.

This change ensures that the listener is never removed, so late injections are correctly detected and we don't have that console error.

I'm unsure about this change because it (seemingly) would make the call to detectProvider hang indefinitely if the user truly had no wallet. I would like this library to be useful in detecting late-injected providers somehow though.

@MicahZoltu
Copy link
Author

MicahZoltu commented Oct 4, 2022

Hi @MicahZoltu, sorry for taking so long to reply to this. Can you give us an example of which wallet this pattern would support? And what do you mean by "A privacy respecting wallet will not inject into the page until after the user tells it"? How would the user tell it? Really, I'm looking for a way I can replicate this issue so that we can confirm that it's fixed by your change.

Unfortunately, there aren't any wallets that do this because most dapps use this package, and the dapp breaks when a wallet doesn't auto-inject if the dapp is using this package. I am trying to build a wallet and ran into this issue, which forced me to "do the bad thing" and auto-inject because without it, my wallet doesn't work with any dapps out there.

Extensions can be configured such that they only inject into the page when they are activated (their icon is clicked). This not only allows the user to choose which pages get injected into, but it also makes it so the extension does not need the "read and write data on all pages" permission which is insanely permissive/risky.

To Replicate

If you happen to have the source code for a wallet that you can build, just change the permissions to activeTab (https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/permissions#activetab_permission) and remove the tabs permission. Then load a page, wait a bit, then click the extension icon (which will trigger the injection script to run).

@MicahZoltu
Copy link
Author

I'm unsure about this change because it (seemingly) would make the call to detectProvider hang indefinitely if the user truly had no wallet. I would like this library to be useful in detecting late-injected providers somehow though.

The promise will not resolve, but it is easy enough to set a timeout and have your application present something to the user after a while. That being said, I think an infinite spinner would be fine here, with some text like "Waiting for browser wallet to inject... You may need to click the browser extension to activate your browser wallet for this page."

@MicahZoltu
Copy link
Author

To reproduce, you can try just running this in the browser console some time after the dapp is loaded:

var request = async ({method, params}) => { if (method === 'eth_requestAccounts') return ['0xd8da6bf26964af9d7eed9e03e53415d37aa96045']; else if (method === 'eth_chainId') return '0x1'; else return []; }
var send = async (method, params) => { if (typeof method === 'object') { return await request({method: method.method, params: method.params}) } else { return await request({ method, params }) } }
var sendAsync = async (payload, callback) => { request(payload).then(result => callback(null, {jsonrpc: '2.0', id: payload.id, result })).catch(error => callback({ jsonrpc: '2.0', id: payload.id, error: { code: error.code, message: error.message, data: { ...error.data, stack: error.stack } } }, null)) }
var ethereumOn = async (kind, callback) => console.log(`window.ethereum.on called for ${kind}`)
window.ethereum = { request, send, sendAsync, on: ethereumOn }

It is just a dummy provider, but it should be enough to trigger the behavior and verify a fix.

@Gudahtt
Copy link
Member

Gudahtt commented Oct 5, 2022

When I first read this issue, I was hoping we could have some sort of feature-detection, to try to discover wallets that have delayed injection. But for extensions that only inject into the page on-click, they don't even have a contentscript process until after being clicked. So there's no way for the website to communicate with the extension, unless the extension has externally_connectable set to * (which Firefox seemingly has no plans to support anytime soon).

Reliable feature detection would require promoting a standard. That would be a tough sell if it relies upon a specific Chrome-only permission being set. Oh well.

The promise will not resolve, but it is easy enough to set a timeout and have your application present something to the user after a while. That being said, I think an infinite spinner would be fine here, with some text like "Waiting for browser wallet to inject... You may need to click the browser extension to activate your browser wallet for this page."

I see, thanks for the suggestion. That could work. It would make this library more difficult to use, but, maybe we can make up for that in other ways, like via documentation or by wrapping this in an easier-to-use library that handles more of this for devs.

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.

3 participants