From c76c9ca2c86317f902f443db2c5704d4bf6311c0 Mon Sep 17 00:00:00 2001 From: bitpshr Date: Thu, 27 Sep 2018 14:19:09 -0400 Subject: EIP-1102: updated implementation --- app/scripts/background.js | 5 +- app/scripts/contentscript.js | 61 ++++++++++++++++---- app/scripts/controllers/preferences.js | 4 +- app/scripts/controllers/provider-approval.js | 84 ++++++++++++++++++++++++++++ app/scripts/inpage.js | 23 ++++---- app/scripts/metamask-controller.js | 21 ++++++- app/scripts/platforms/extension.js | 12 ++++ 7 files changed, 182 insertions(+), 28 deletions(-) create mode 100644 app/scripts/controllers/provider-approval.js (limited to 'app/scripts') diff --git a/app/scripts/background.js b/app/scripts/background.js index 2a3c5b08b..078e84928 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -256,7 +256,8 @@ function setupController (initState, initLangCode) { showUnconfirmedMessage: triggerUi, unlockAccountMessage: triggerUi, showUnapprovedTx: triggerUi, - showWatchAssetUi: showWatchAssetUi, + openPopup: openPopup, + closePopup: notificationManager.closePopup.bind(notificationManager), // initial state initState, // initial locale code @@ -447,7 +448,7 @@ function triggerUi () { * Opens the browser popup for user confirmation of watchAsset * then it waits until user interact with the UI */ -function showWatchAssetUi () { +function openPopup () { triggerUi() return new Promise( (resolve) => { diff --git a/app/scripts/contentscript.js b/app/scripts/contentscript.js index 33523eb46..060343031 100644 --- a/app/scripts/contentscript.js +++ b/app/scripts/contentscript.js @@ -11,6 +11,7 @@ const PortStream = require('extension-port-stream') const inpageContent = fs.readFileSync(path.join(__dirname, '..', '..', 'dist', 'chrome', 'inpage.js')).toString() const inpageSuffix = '//# sourceURL=' + extension.extension.getURL('inpage.js') + '\n' const inpageBundle = inpageContent + inpageSuffix +let originApproved = false // Eventually this streaming injection could be replaced with: // https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.exportFunction @@ -20,24 +21,24 @@ const inpageBundle = inpageContent + inpageSuffix // MetaMask will be much faster loading and performant on Firefox. if (shouldInjectWeb3()) { - setupInjection() + injectScript(inpageBundle) setupStreams() + listenForProviderRequest() } /** - * Creates a script tag that injects inpage.js + * Injects a script tag into the current document + * + * @param {string} content - Code to be executed in the current document */ -function setupInjection () { +function injectScript (content) { try { - // inject in-page script - var scriptTag = document.createElement('script') - scriptTag.textContent = inpageBundle - scriptTag.onload = function () { this.parentNode.removeChild(this) } - var container = document.head || document.documentElement - // append as first child + const container = document.head || document.documentElement + const scriptTag = document.createElement('script') + scriptTag.textContent = content container.insertBefore(scriptTag, container.children[0]) } catch (e) { - console.error('Metamask injection failed.', e) + console.error('Metamask script injection failed.', e) } } @@ -54,6 +55,16 @@ function setupStreams () { const pluginPort = extension.runtime.connect({ name: 'contentscript' }) const pluginStream = new PortStream(pluginPort) + // Until this origin is approved, cut-off publicConfig stream writes at the content + // script level so malicious sites can't snoop on the currently-selected address + pageStream._write = function (data, encoding, cb) { + if (typeof data === 'object' && data.name && data.name === 'publicConfig' && !originApproved) { + cb() + return + } + LocalMessageDuplexStream.prototype._write.apply(pageStream, arguments) + } + // forward communication plugin->inpage pump( pageStream, @@ -97,6 +108,36 @@ function setupStreams () { mux.ignoreStream('publicConfig') } +/** + * Establishes listeners for requests to fully-enable the provider from the dapp context + * and for full-provider approvals and rejections from the background script context. Dapps + * should not post messages directly and should instead call provider.enable(), which + * handles posting these messages automatically. + */ +function listenForProviderRequest () { + window.addEventListener('message', (event) => { + if (event.source !== window) { return } + if (!event.data || !event.data.type || event.data.type !== 'ETHEREUM_ENABLE_PROVIDER') { return } + extension.runtime.sendMessage({ + action: 'init-provider-request', + origin: event.source.location.hostname, + }) + }) + + extension.runtime.onMessage.addListener(({ action }) => { + if (!action) { return } + switch (action) { + case 'approve-provider-request': + originApproved = true + injectScript(`window.dispatchEvent(new CustomEvent('ethereumprovider', { detail: {}}))`) + break + case 'reject-provider-request': + injectScript(`window.dispatchEvent(new CustomEvent('ethereumprovider', { detail: { error: 'User rejected provider access' }}))`) + break + } + }) +} + /** * Error handler for page to plugin stream disconnections diff --git a/app/scripts/controllers/preferences.js b/app/scripts/controllers/preferences.js index dc6fecaf5..ffb593b09 100644 --- a/app/scripts/controllers/preferences.js +++ b/app/scripts/controllers/preferences.js @@ -46,7 +46,7 @@ class PreferencesController { this.diagnostics = opts.diagnostics this.network = opts.network this.store = new ObservableStore(initState) - this.showWatchAssetUi = opts.showWatchAssetUi + this.openPopup = opts.openPopup this._subscribeProviderType() } // PUBLIC METHODS @@ -567,7 +567,7 @@ class PreferencesController { } const tokenOpts = { rawAddress, decimals, symbol, image } this.addSuggestedERC20Asset(tokenOpts) - return this.showWatchAssetUi().then(() => { + return this.openPopup().then(() => { const tokenAddresses = this.getTokens().filter(token => token.address === normalizeAddress(rawAddress)) return tokenAddresses.length > 0 }) diff --git a/app/scripts/controllers/provider-approval.js b/app/scripts/controllers/provider-approval.js new file mode 100644 index 000000000..e9b1b8e16 --- /dev/null +++ b/app/scripts/controllers/provider-approval.js @@ -0,0 +1,84 @@ +const ObservableStore = require('obs-store') + +/** + * A controller that services user-approved requests for a full Ethereum provider API + */ +class ProviderApprovalController { + /** + * Creates a ProviderApprovalController + * + * @param {Object} [config] - Options to configure controller + */ + constructor ({ closePopup, openPopup, platform, publicConfigStore } = {}) { + this.store = new ObservableStore() + this.closePopup = closePopup + this.openPopup = openPopup + this.platform = platform + this.publicConfigStore = publicConfigStore + this.approvedOrigins = {} + platform && platform.addMessageListener && platform.addMessageListener(({ action, origin }) => { + action && action === 'init-provider-request' && this.handleProviderRequest(origin) + }) + } + + /** + * Called when a tab requests access to a full Ethereum provider API + * + * @param {string} origin - Origin of the window requesting full provider access + */ + handleProviderRequest (origin) { + this.store.updateState({ providerRequests: [{ origin }] }) + if (this.approvedOrigins[origin]) { + this.approveProviderRequest(origin) + return + } + this.openPopup && this.openPopup() + } + + /** + * Called when a user approves access to a full Ethereum provider API + * + * @param {string} origin - Origin of the target window to approve provider access + */ + approveProviderRequest (origin) { + this.closePopup && this.closePopup() + const requests = this.store.getState().providerRequests || [] + this.platform && this.platform.sendMessage({ action: 'approve-provider-request' }, { active: true }) + this.publicConfigStore.emit('update', this.publicConfigStore.getState()) + const providerRequests = requests.filter(request => request.origin !== origin) + this.store.updateState({ providerRequests }) + this.approvedOrigins[origin] = true + } + + /** + * Called when a tab rejects access to a full Ethereum provider API + * + * @param {string} origin - Origin of the target window to reject provider access + */ + rejectProviderRequest (origin) { + this.closePopup && this.closePopup() + const requests = this.store.getState().providerRequests || [] + this.platform && this.platform.sendMessage({ action: 'reject-provider-request' }, { active: true }) + const providerRequests = requests.filter(request => request.origin !== origin) + this.store.updateState({ providerRequests }) + } + + /** + * Clears any cached approvals for user-approved origins + */ + clearApprovedOrigins () { + this.approvedOrigins = {} + } + + /** + * Determines if a given origin has been approved + * + * @param {string} origin - Domain origin to check for approval status + * @returns {boolean} - True if the origin has been approved + */ + isApproved (origin) { + return this.approvedOrigins[origin] + } +} + +module.exports = ProviderApprovalController diff --git a/app/scripts/inpage.js b/app/scripts/inpage.js index b885a7e05..25bfe1416 100644 --- a/app/scripts/inpage.js +++ b/app/scripts/inpage.js @@ -31,19 +31,18 @@ var inpageProvider = new MetamaskInpageProvider(metamaskStream) inpageProvider.setMaxListeners(100) // Augment the provider with its enable method -inpageProvider.enable = function (options = {}) { +inpageProvider.enable = function () { return new Promise((resolve, reject) => { - if (options.mockRejection) { - reject('User rejected account access') - } else { - inpageProvider.sendAsync({ method: 'eth_accounts', params: [] }, (error, response) => { - if (error) { - reject(error) - } else { - resolve(response.result) - } - }) - } + window.addEventListener('ethereumprovider', ({ detail }) => { + if (typeof detail.error !== 'undefined') { + reject(detail.error) + } else { + inpageProvider.publicConfigStore.once('update', () => { + resolve(inpageProvider.send({ method: 'eth_accounts' }).result) + }) + } + }) + window.postMessage({ type: 'ETHEREUM_ENABLE_PROVIDER' }, '*') }) } diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 1f6a8659b..cffc5797b 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -37,6 +37,7 @@ const TransactionController = require('./controllers/transactions') const BalancesController = require('./controllers/computed-balances') const TokenRatesController = require('./controllers/token-rates') const DetectTokensController = require('./controllers/detect-tokens') +const ProviderApprovalController = require('./controllers/provider-approval') const nodeify = require('./lib/nodeify') const accountImporter = require('./account-import-strategies') const getBuyEthUrl = require('./lib/buy-eth-url') @@ -89,7 +90,7 @@ module.exports = class MetamaskController extends EventEmitter { this.preferencesController = new PreferencesController({ initState: initState.PreferencesController, initLangCode: opts.initLangCode, - showWatchAssetUi: opts.showWatchAssetUi, + openPopup: opts.openPopup, network: this.networkController, }) @@ -219,6 +220,13 @@ module.exports = class MetamaskController extends EventEmitter { this.typedMessageManager = new TypedMessageManager({ networkController: this.networkController }) this.publicConfigStore = this.initPublicConfigStore() + this.providerApprovalController = new ProviderApprovalController({ + closePopup: opts.closePopup, + openPopup: opts.openPopup, + platform: opts.platform, + publicConfigStore: this.publicConfigStore, + }) + this.store.updateStructure({ TransactionController: this.txController.store, KeyringController: this.keyringController.store, @@ -248,6 +256,7 @@ module.exports = class MetamaskController extends EventEmitter { NoticeController: this.noticeController.memStore, ShapeshiftController: this.shapeshiftController.store, InfuraController: this.infuraController.store, + ProviderApprovalController: this.providerApprovalController.store, }) this.memStore.subscribe(this.sendUpdate.bind(this)) } @@ -263,7 +272,10 @@ module.exports = class MetamaskController extends EventEmitter { }, version, // account mgmt - getAccounts: async () => { + getAccounts: async ({ origin }) => { + // Expose no accounts if this origin has not been approved, preventing + // account-requring RPC methods from completing successfully + if (origin !== 'MetaMask' && !this.providerApprovalController.isApproved(origin)) { return [] } const isUnlocked = this.keyringController.memStore.getState().isUnlocked const selectedAddress = this.preferencesController.getSelectedAddress() // only show address if account is unlocked @@ -349,6 +361,7 @@ module.exports = class MetamaskController extends EventEmitter { const noticeController = this.noticeController const addressBookController = this.addressBookController const networkController = this.networkController + const providerApprovalController = this.providerApprovalController return { // etc @@ -437,6 +450,10 @@ module.exports = class MetamaskController extends EventEmitter { // notices checkNotices: noticeController.updateNoticesList.bind(noticeController), markNoticeRead: noticeController.markNoticeRead.bind(noticeController), + + approveProviderRequest: providerApprovalController.approveProviderRequest.bind(providerApprovalController), + clearApprovedOrigins: providerApprovalController.clearApprovedOrigins.bind(providerApprovalController), + rejectProviderRequest: providerApprovalController.rejectProviderRequest.bind(providerApprovalController), } } diff --git a/app/scripts/platforms/extension.js b/app/scripts/platforms/extension.js index 71b162dd0..9ef0d22c9 100644 --- a/app/scripts/platforms/extension.js +++ b/app/scripts/platforms/extension.js @@ -57,6 +57,18 @@ class ExtensionPlatform { } } + addMessageListener (cb) { + extension.runtime.onMessage.addListener(cb) + } + + sendMessage (message, query = {}) { + extension.tabs.query(query, tabs => { + tabs.forEach(tab => { + extension.tabs.sendMessage(tab.id, message) + }) + }) + } + _showConfirmedTransaction (txMeta) { this._subscribeToNotificationClicked() -- cgit