search

polkadot-js / extension

Simple browser extension for managing Polkadot and Substrate network accounts in a browser. Allows the signing of extrinsics using these accounts. Also provides a simple interface for compliant extensions for dapps.
Apache License 2.0
965 stars 403 forks source link

Fix: Prevent authorization request from incorrect origin due to chrome pre-rendering fixes #1293 #1308

Closed F-OBrien closed 3 months ago

F-OBrien commented 5 months ago

We cannot rely on sender.tab.url when chrome preloads and prerenders pages as it will not match the url of the site being loaded. This PR changes the primary from to sender.url and only uses sender.tab.url if sender.url is not available.

See discussion in #1293

Edit: Also PR #1278 introduced a bug by adding

  if (!sender) {
    throw new Error('Unable to extract message sender');
  }

in the case of getActiveTabs no port is passed causing this to throw an error preventing the extension from initializing.

This PR fixes that by only throwing if (!isExtension && !sender)

TarikGul commented 3 months ago

If you can pull in master that would be amazing so the CI can run again, and I will do a proper review of this PR.

TarikGul commented 3 months ago

@Tbaut Could you also check this out when you have a chance?

F-OBrien commented 3 months ago

@TarikGul I've rebased rebased on master as requested. FYI Talisman also applied a similar fix today https://github.com/TalismanSociety/talisman/pull/1349

Tbaut commented 3 months ago

Sure, I'm not familiar with the issue (despite having seen and followed the linked one). I'll take a look.

Tbaut commented 3 months ago

@F-OBrien you mention in the last comment: https://github.com/polkadot-js/extension/issues/1293#issuecomment-1900267262 that it's better to not support iframes (I agree) and then say

sender.documentLifecycle === 'prerender' to use the sender.url as the from

But this is not what you have done here. Is there a reason for it?

F-OBrien commented 3 months ago

@F-OBrien you mention in the last comment: #1293 (comment) that it's better to not support iframes (I agree) and then say

sender.documentLifecycle === 'prerender' to use the sender.url as the from

But this is not what you have done here. Is there a reason for it?

@Tbaut It's been a while since I looked at this. From what I recall I believe I concluded that in all cases using sender.url should be the preferred solution and not only if sender.documentLifecycle === 'prerender' so didn't see a point in an explicit check for it. sender.tab.url is the cause of the incorrect origin issue and also gives rise to my concern related to iframes.

I did consider fully removing sender.tab.url like talisman has done but I was not familiar enough with chrome extensions or the reason why the tab url was initially chosen when developing the extension to know if these is a specific edge case where you would have a sender.tab.url and not a sender.url. hence why I just swapped them rather than removing the tab url.

Tbaut commented 3 months ago

Thank you so much for the research and the fix :pray:

polkadot-js-bot commented 3 months ago

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.