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

fix: Fix/10029 background bridge origin #13698

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

Cal-L
Copy link
Contributor

@Cal-L Cal-L commented Feb 24, 2025

Description

This PR updates the BackgroundBridge to properly set the origin property instead of using hostname. This is important because the origin that dependents consume. Previously, we used hostname, which omitted protocol. Changing to origin will resolve that

Related issues

Fixes: #11029

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot metamaskbot added the team-mobile-platform Mobile Platform team label Feb 24, 2025
@Cal-L Cal-L added Run Smoke E2E Triggers smoke e2e on Bitrise needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Feb 24, 2025
Copy link
Contributor

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 7bfbb3c
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/b00325ff-55c7-4541-9f9e-ea34e045bcbb

Note

  • This comment will auto-update when build completes
  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link

socket-security bot commented Feb 26, 2025

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@types/pump@1.1.3 None 0 3.57 kB types
npm/@types/readable-stream@4.0.18 None +1 63.4 kB types

View full report↗︎

Have feedback? Participate in our User Experience Survey 📊

) => `(function () {
try {
window.postMessage(${JSON.stringify(message)}, '${origin}');
} catch (e) {
//Nothing to do
}
})()`;

export const JS_IFRAME_POST_MESSAGE_TO_PROVIDER = (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed since not used for a while

@@ -18,7 +18,7 @@ const USER_REJECTED_ERROR_CODE = 4001;
/**
* Returns a middleware that appends the DApp origin to request
* @param {{ origin: string }} opts - The middleware options
* @returns {Function}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating to use the correct types

@@ -595,6 +626,7 @@ export function getNormalizedTxState(state) {
: undefined;
}

// FIXME: Check if the url here is origin and not hostname
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify that this is not hostname

@@ -40,6 +40,10 @@ module.exports = {
test: './app/core/Engine/Engine.ts',
plugins: [['@babel/plugin-transform-private-methods', { loose: true }]],
},
{
test: './app/core/BackgroundBridge/BackgroundBridge.ts',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BackgroundBridge uses # prefix for private

@@ -440,6 +441,7 @@
"@types/react-native-svg-charts": "^5.0.12",
"@types/react-native-vector-icons": "^6.4.13",
"@types/react-native-video": "^5.0.14",
"@types/readable-stream": "^4.0.18",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converted requires to imports, so needed types for both packages

* @param subject - The subject to get the permitted accounts for
* @returns A selector that returns the permitted accounts for the given subject
*/
export const selectPermittedAccounts = (subject: string) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add new selector for getting permitted accounts by subject


export const selectPermissionControllerState = (state: RootState) =>
state.engine.backgroundState.PermissionController;
state.engine.backgroundState
.PermissionController as PermissionControllerState<PermissionConstraint>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Give selector return type

@@ -295,3 +295,7 @@ declare module '@metamask/react-native-actionsheet' {
}

declare module '@metamask/react-native-search-api';

// Resolve BackgroundBridge import errors related to TS
declare module '@metamask/eth-json-rpc-filters';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these libs are JS. These are added to supress TS errors associated with importing them

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
23.1% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) Run Smoke E2E Triggers smoke e2e on Bitrise team-mobile-platform Mobile Platform team
Projects
Status: Needs dev review
2 participants