-
-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Add firefox window.postMessage support #48
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
Conversation
|
Updated dependencies detected. Learn more about Socket for GitHub ↗︎
|
|
I think its probably fine for now but maybe we should add a way to connect in firefox that doesn't depend on CAIP-294 announcement? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM! Great work 💪
| this.disconnect(); | ||
| } | ||
|
|
||
| this.#listener = (messageEvent: MessageEvent) => { |
Check warning
Code scanning / CodeQL
Missing origin verification in `postMessage` handler Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 8 months ago
To fix the problem, we need to verify the origin of the incoming message in the messageEvent handler. This involves checking the origin property of the messageEvent object to ensure it matches a trusted origin. We will add a check for the origin before processing the message further.
- Identify the trusted origin(s) that should be allowed to send messages.
- Modify the
messageEventhandler to include a check for theoriginproperty. - If the origin matches the trusted origin, proceed with the existing checks and message handling. Otherwise, ignore the message.
-
Copy modified lines R24-R27
| @@ -23,2 +23,6 @@ | ||
| this.#listener = (messageEvent: MessageEvent) => { | ||
| const trustedOrigin = 'https://www.example.com'; // Replace with the actual trusted origin | ||
| if (messageEvent.origin !== trustedOrigin) { | ||
| return; | ||
| } | ||
| const { target, data } = messageEvent.data; |
| <p className="wallet-extension-id"> | ||
| Extension ID: {wallet.params.extensionId} | ||
| {wallet.params.extensionId === WINDOW_POST_MESSAGE_ID | ||
| ? null | ||
| : 'Extension ID: '} | ||
| {wallet.params.extensionId} | ||
| </p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original suggested change always renders a <p> tag, even when there's no content. This isn't semantically correct HTML, as an empty paragraph doesn't convey any meaning and will always render regardless of the extensionId provided.
I suggest we do something like:
| <p className="wallet-extension-id"> | |
| Extension ID: {wallet.params.extensionId} | |
| {wallet.params.extensionId === WINDOW_POST_MESSAGE_ID | |
| ? null | |
| : 'Extension ID: '} | |
| {wallet.params.extensionId} | |
| </p> | |
| {wallet.params.extensionId !== WINDOW_POST_MESSAGE_ID && ( | |
| <p className="wallet-extension-id"> | |
| Extension ID: {wallet.params.extensionId} | |
| </p> | |
| )} |
This way we only renders the <p> element when there's content to display.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github won't let me apply your suggestion. so i'll apply it manually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh sorry, so code doesn't result in an empty
tag. Effectively what is happening is.
if wallet.params.extensionId === 'window.postMessage' then <p ...> window.postMessage
else <p ...> Extension ID: {wallet.params.extensionId}
I want to show the extensionId always, but i don't want the Extension ID: text if wallet.params.extensionId === 'window.postMessage'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** * Exposes the Multichain API over window.postMessage for Firefox * Enables Flask e2e Multichain API tests for Firefox [](https://codespaces.new/MetaMask/metamask-extension/pull/30142?quickstart=1) ## **Related issues** Test Dapp: MetaMask/test-dapp-multichain#48 ## **Manual testing steps** `yarn start:flask:mv2` In a non-chromium browser, i.e. Firefox Without helper packages ``` window.addEventListener('message', (event) => { const { target, data } = event.data; if ( target !== "metamask-inpage" || data?.name !== 'metamask-multichain-provider' ) { return; } console.log(data.data) }) const caipPostMessage = (data) => { window.postMessage({ target: 'metamask-contentscript', data: { name: 'metamask-multichain-provider', data } }, location.origin) } caipPostMessage({ jsonrpc: '2.0', method: 'wallet_createSession', params: { optionalScopes: {} } }) ``` With helper packages (psuedo-ish code) ``` import { WindowPostMessageStream } from '@metamask/post-message-stream'; import ObjectMultiplex from '@metamask/object-multiplex'; import { pipeline } from 'readable-stream'; const metamaskStream = new WindowPostMessageStream({ name: INPAGE, target: CONTENT_SCRIPT, }); const metamaskMux = new ObjectMultiplex(metamaskStream) const caipStream = window.metamaskMux.createStream('metamask-provider-caip') pipeline(metamaskMux, metamaskStream, metamaskMux, (err) => { console.log({err}) }); caipStream.on('data', console.log) caipStream.write({jsonrpc: '2.0', method: 'wallet_createSession', params:{optionalScopes: {}} }) ``` ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com> Co-authored-by: Alex <adonesky@gmail.com> Co-authored-by: Mark Stacey <markjstacey@gmail.com> Co-authored-by: ffmcgee <51971598+ffmcgee725@users.noreply.github.com> Co-authored-by: Frederik Bolding <frederik.bolding@gmail.com> Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Adds window.postMessage support for firefox
Extension: MetaMask/metamask-extension#30142
Ticket: https://github.com/orgs/MetaMask/projects/146/views/6?pane=issue&itemId=102893257&issue=MetaMask%7CMetaMask-planning%7C4459
Firefox:
firefox.mov
Chromium:
chromium.mov