fixed: check the routing of method and route it to flow if connected. by zzggo · Pull Request #1044 · onflow/FRW-Extension

zzggo · 2025-06-10T08:08:14Z

Closes #1040

Related Issue

Closes #1040

Summary of Changes

Need Regression Testing

Yes
No

Risk Assessment

Low
Medium
High

Additional Notes

Screenshots (if applicable)

Closes #1040

github-actions · 2025-06-10T08:08:30Z

PR Summary

Enhanced the wallet's messaging system by implementing a secure bridge between content scripts and the main world. Added support for EIP-6963 wallet discovery protocol and improved cross-origin iframe communication. Removed static content script injection in favor of dynamic registration through Chrome's scripting API.

Changes

File	Summary
`_raw/manifest/manifest.dev.json`	Removed static content script declarations and added `scripting` permission. Updated host permissions to allow `<all_urls>` for broader compatibility.
`_raw/manifest/manifest.pro.json`	Synchronized changes with dev manifest: removed static content scripts, added `scripting` permission, and updated host permissions to `<all_urls>`.
`src/background/service/content-script-injection.ts`	Created new service to handle dynamic registration of content scripts using Chrome's scripting API. Implements separate registration for message bridge, page provider, and flow scripts.
`src/content-script/message-bridge.ts`	Created bridge to handle communication between isolated and main worlds. Manages UUID generation and injection for secure message routing between contexts.
`src/content-script/pageProvider/eth/index.ts`	Refactored provider initialization to support EIP-6963 wallet discovery protocol. Added UUID-based message routing and improved cross-origin iframe support.
`src/content-script/pageProvider/eth/utils/message/post-message.ts`	Implemented secure PostMessage class for cross-origin communication, with origin validation and dynamic target origin handling for iframe support.
`e2e/d-app/test-eip6963.html`	Created test page for EIP-6963 wallet discovery protocol with debugging tools and visual wallet display.
`e2e/d-app/eip6963-d-app.test.ts`	Added end-to-end tests for EIP-6963 wallet discovery functionality using Playwright test framework.
`src/background/controller/provider/rpcFlow.ts`	Modified RPC flow to handle chain ID requests without requiring permissions, enabling better multi-wallet support.
`src/content-script/pageProvider/eth/utils/message/externally-connectable.ts`	Implemented direct extension messaging with fallback to postMessage for improved communication reliability.

autogenerated by presubmit.ai

github-actions

🚨 Pull request needs attention.

Review Summary

Commits Considered (1)

a7e9252: fixed: check the routing of method and route it to flow if connected.

Closes #1040

Files Processed (1)

src/content-script/pageProvider/eth/index.ts (10 hunks)

Actionable Comments (1)

src/content-script/pageProvider/eth/index.ts [488-493]

possible issue: "Potential race condition in connection check"

Skipped Comments (3)

src/content-script/pageProvider/eth/index.ts [381-391]

maintainability: "Consider making method lists configurable"
src/content-script/pageProvider/eth/index.ts [500-511]

best practice: "Error handling missing in async callback"
src/content-script/pageProvider/eth/index.ts [515-527]

best practice: "Inconsistent payload type checking"

github-actions · 2025-06-10T08:08:59Z

+                    // Check if user is connected to Flow Wallet
+                    const connectedToFlowWallet = isConnectedToFlowWallet(flowProvider);
+
+                    if (data && shouldRouteToFlowWallet(data.method, connectedToFlowWallet)) {
+                      return flowProvider.request(data);
+                    }


The connection status check and the subsequent method call are not atomic. The connection status could change between the check and the actual request. Consider adding a mechanism to ensure consistency or handle potential state changes.

…jection service - Added 'scripting' permission to both development and production manifests. - Updated 'host_permissions' to allow '<all_urls>'. - Introduced ContentScriptInjectionService to manage content script registration dynamically. - Removed legacy content script and replaced it with a message bridge for communication between the background script and the main world. - Updated EthereumProvider to utilize the new message bridge for communication. Closes #1040

github-actions

🚨 Pull request needs attention.

Review Summary

Commits Considered (1)

cf7725f: refactor: update manifest permissions and implement content script injection service
Added 'scripting' permission to both development and production manifests.
Updated 'host_permissions' to allow '<all_urls>'.
Introduced ContentScriptInjectionService to manage content script registration dynamically.
Removed legacy content script and replaced it with a message bridge for communication between the background script and the main world.
Updated EthereumProvider to utilize the new message bridge for communication.

Closes #1040

Files Processed (11)

_raw/manifest/manifest.dev.json (1 hunk)
_raw/manifest/manifest.pro.json (1 hunk)
build/webpack.common.config.ts (1 hunk)
src/background/index.ts (2 hunks)
src/background/service/content-script-injection.ts (1 hunk)
src/background/service/index.ts (1 hunk)
src/content-script/index.ts (1 hunk)
src/content-script/message-bridge.ts (1 hunk)
src/content-script/pageProvider/eth/index.ts (16 hunks)
src/content-script/pageProvider/eth/utils/message/externally-connectable.ts (1 hunk)
src/content-script/pageProvider/eth/utils/message/post-message.ts (1 hunk)

Actionable Comments (3)

_raw/manifest/manifest.dev.json [45-45]

security: "Overly broad host permissions could pose security risks"
src/content-script/message-bridge.ts [80-83]

security: "Potential security risk in global UUID storage"
src/content-script/pageProvider/eth/utils/message/post-message.ts [13-13]

security: "Insecure message passing configuration"

Skipped Comments (2)

src/content-script/pageProvider/eth/index.ts [617-626]

possible bug: "Missing error handling in provider routing logic"
src/content-script/message-bridge.ts [17-17]

possible issue: "Unreliable timing-based initialization"

github-actions · 2025-06-11T05:58:44Z

+    (window as any).__frwUUIDs = {
+      bridgeUUID: this.uuid,
+      walletUUID: walletUUID,
+    };


Storing sensitive UUIDs in window.__frwUUIDs makes them globally accessible to any script on the page. This could allow malicious scripts to intercept or manipulate the communication channel. Consider using a more secure storage mechanism or scope the UUIDs to a closure.

github-actions · 2025-06-11T05:58:44Z

+      throw new Error('UUID is required for PostMessage communication');
+    }
+    this._uuid = uuid;
+    this._targetOrigin = '*'; // We'll restrict this in production


Using '*' as targetOrigin in postMessage is unsafe as it allows any website to receive the messages. This could lead to sensitive information leakage. Specify the exact target origin or implement proper origin validation.

How should we fix this?

@tombeckenham Instead of using '*' as the targetOrigin, you should:

Define the exact origin(s) you want to communicate with. For example:

this._targetOrigin = 'https://your-trusted-domain.com';

If you need to support multiple origins, you can:

Either maintain a whitelist of allowed origins and validate against it in the _handleMessage method:

private _allowedOrigins = ['https://domain1.com', 'https://domain2.com']; private _handleMessage = (event: MessageEvent) => { if (!this._allowedOrigins.includes(event.origin)) { return; } // rest of your handler code };

Or pass the allowed origin as a parameter to the constructor:

constructor(uuid: string, targetOrigin: string) { super(); if (!uuid) { throw new Error('UUID is required for PostMessage communication'); } if (!targetOrigin || targetOrigin === '*') { throw new Error('A specific targetOrigin is required for security'); } this._uuid = uuid; this._targetOrigin = targetOrigin; }

The second approach with constructor injection is generally better as it makes the security requirement explicit and allows the consuming code to specify the correct origin for their use case.

Closes #1040

github-actions

🚨 Pull request needs attention.

Review Summary

Commits Considered (1)

6feecec: fixed: connection conflict

Closes #1040

Files Processed (2)

src/background/controller/provider/rpcFlow.ts (1 hunk)
src/content-script/pageProvider/eth/index.ts (16 hunks)

Actionable Comments (2)

src/background/controller/provider/rpcFlow.ts [60-65]

possible issue: "Potential unintended removal of wallet connection check"
src/content-script/pageProvider/eth/index.ts [633-636]

possible bug: "Race condition in wallet routing logic"

Skipped Comments (1)

src/content-script/pageProvider/eth/index.ts [71-78]

best practice: "Short timeout duration for wallet UUID initialization"

github-actions · 2025-06-11T06:59:44Z

      mapMethod !== 'walletWatchAsset' &&
      mapMethod !== 'walletConnect' &&
      mapMethod !== 'walletDisconnect' &&
-      mapMethod !== 'walletConnect' &&
      !Reflect.getMetadata('SAFE', providerController, mapMethod)
    ) {
      if (!permissionService.hasPermission(origin) || !(await Wallet.isUnlocked())) {


The condition check for mapMethod !== 'walletConnect' appears twice in the original code, and one instance was removed. However, this could indicate that there was a need to check for different types of wallet connections. Consider reviewing if removing one of these checks was intentional and doesn't break any functionality.

github-actions · 2025-06-11T06:59:45Z

+                    if (data && shouldRouteToFlowWallet(data.method, connectedToFlowWallet)) {
+                      return flowProvider.request(data);
+                    }
+                    // Route other methods to the default provider


The shouldRouteToFlowWallet check is performed after getting the connection status, but the connection status could change between these checks. Consider implementing a transaction-like pattern or adding a lock mechanism to ensure the routing decision remains valid throughout the request execution.

- Updated the request ID pool size for better performance. - Improved comments for clarity on the proxy's role in multi-wallet interoperability. - Enhanced security checks in the PostMessage class to ensure safe communication with cross-origin iframes. - Adjusted the target origin handling for postMessage to default to the current window's origin. Closes #1040

github-actions

🚨 Pull request needs attention.

Review Summary

Commits Considered (1)

57f50a3: refactor: enhance message handling and security in PostMessage class
Updated the request ID pool size for better performance.
Improved comments for clarity on the proxy's role in multi-wallet interoperability.
Enhanced security checks in the PostMessage class to ensure safe communication with cross-origin iframes.
Adjusted the target origin handling for postMessage to default to the current window's origin.

Closes #1040

Files Processed (3)

src/content-script/pageProvider/eth/index.ts (16 hunks)
src/content-script/pageProvider/eth/utils/message/index.ts (1 hunk)
src/content-script/pageProvider/eth/utils/message/post-message.ts (1 hunk)

Actionable Comments (1)

src/content-script/pageProvider/eth/utils/message/post-message.ts [47-51]

security: "Potential security issue in message source validation"

Skipped Comments (3)

src/content-script/pageProvider/eth/index.ts [73-74]

best practice: "Potential memory leak in interval/timeout cleanup"
src/content-script/pageProvider/eth/utils/message/index.ts [12-12]

possible issue: "Potential resource limitation issue"
src/content-script/pageProvider/eth/index.ts [614-615]

possible bug: "Missing error handling in ethereum getter"

github-actions · 2025-06-11T07:17:25Z

+    if (event.source !== window && event.source !== window.self) {
+      if (!Array.from(window.frames).includes(event.source as Window)) {
+        return;
+      }
+    }


The source validation could be bypassed by a malicious iframe. Instead of using Array.from(window.frames).includes(), consider implementing a whitelist of allowed origins and validating both the source and origin of the message:

if (!allowedOrigins.includes(event.origin) || (event.source !== window && event.source !== window.parent && !trustedFrames.has(event.source))) { return; }

Closes #1040

github-actions

🚨 Pull request needs attention.

Review Summary

Commits Considered (1)

399138c: Add template to serve for EIP-6963 testing

Closes #1040

Files Processed (4)

e2e/d-app/eip6963-d-app.test.ts (1 hunk)
e2e/d-app/test-eip6963.html (1 hunk)
e2e/utils/loader.ts (1 hunk)
playwright.config.ts (1 hunk)

Actionable Comments (1)

e2e/d-app/eip6963-d-app.test.ts [24-25]

best practice: "Unreliable test setup using fixed delay"

Skipped Comments (1)

e2e/d-app/test-eip6963.html [272-280]

possible issue: "Resource leak in interval timer"

github-actions · 2025-06-11T23:18:53Z

+  // Wait a bit for the server to start (or poll the port)
+  await new Promise((resolve) => setTimeout(resolve, 3000));


Using a fixed delay (setTimeout) to wait for server startup is unreliable and could lead to flaky tests. Consider implementing a proper server readiness check by polling the server endpoint until it responds, with a reasonable timeout:

async function waitForServer(port: number, timeout = 10000) { const start = Date.now(); while (Date.now() - start < timeout) { try { await fetch(`http://localhost:${port}`); return true; } catch { await new Promise(resolve => setTimeout(resolve, 100)); } } throw new Error('Server failed to start'); }

…n-cant-open-popup Closes #1040

github-actions

✅ LGTM!

Review Summary

Commits Considered (1)

f5c56fa: Merge branch 'dev' into 1040-bug-kitty-punch-referral-code-transaction-cant-open-popup

Closes #1040

Files Processed (7)

build/prepareManifest.ts (1 hunk)
build/webpack.common.config.ts (1 hunk)
pnpm-lock.yaml (0 hunks)
src/background/controller/provider/rpcFlow.ts (1 hunk)
src/background/index.ts (2 hunks)
src/content-script/index.ts (2 hunks)
src/content-script/pageProvider/eth/index.ts (16 hunks)

Actionable Comments (0)

Skipped Comments (3)

src/content-script/pageProvider/eth/index.ts [79-85]

possible issue: "Short timeout duration may cause initialization issues"
src/content-script/pageProvider/eth/index.ts [88-98]

performance: "Aggressive polling interval may impact performance"
src/content-script/pageProvider/eth/index.ts [863-866]

enhancement: "Fixed delay for late dApp detection may be unreliable"

cursor

Bug: Request ID Pool Size Cuts Cause High Load Failures

The request ID pool size was reduced from 1000 to 100 without justification, significantly limiting concurrent requests. This 90% reduction can lead to request ID exhaustion under high load, causing new requests to fail or race conditions due to premature ID reuse.

src/content-script/pageProvider/eth/utils/message/index.ts#L11-L12

FRW-Extension/src/content-script/pageProvider/eth/utils/message/index.ts

Lines 11 to 12 in f5c56fa

    
           // max concurrent request limit 
        
           protected _requestIdPool = [...Array(100).keys()];

Fix in Cursor

Bug: `ethChainId` Method Requires Unexpected User Permission

The ethChainId method was inadvertently removed from the list of safe methods, causing it to unexpectedly require user permission/approval. This breaks dApps, as ethChainId is a read-only method typically expected to function without requiring user interaction or wallet connection.

src/background/controller/provider/rpcFlow.ts#L62-L63

FRW-Extension/src/background/controller/provider/rpcFlow.ts

Lines 62 to 63 in f5c56fa

    
           mapMethod !== 'walletDisconnect' && 
        
           // mapMethod !== 'ethChainId' &&

Fix in Cursor

BugBot free trial expires on July 22, 2025
You have used $1.07 of your $50.00 spend limit so far. Manage your spend limit in the Cursor dashboard.

Was this report helpful? Give feedback by reacting with 👍 or 👎

fixed: check the routing of method and route it to flow if connected.

a7e9252

Closes #1040

zzggo linked an issue Jun 10, 2025 that may be closed by this pull request

[BUG] Kitty punch referral code transaction cant open popup #1040

Closed

github-actions Bot reviewed Jun 10, 2025

View reviewed changes

tombeckenham temporarily deployed to development June 11, 2025 05:58 — with GitHub Actions Inactive

github-actions Bot reviewed Jun 11, 2025

View reviewed changes

tombeckenham temporarily deployed to development June 11, 2025 05:59 — with GitHub Actions Inactive

fixed: connection conflict

6feecec

Closes #1040

zzggo temporarily deployed to development June 11, 2025 06:59 — with GitHub Actions Inactive

github-actions Bot reviewed Jun 11, 2025

View reviewed changes

zzggo temporarily deployed to development June 11, 2025 07:00 — with GitHub Actions Inactive

tombeckenham temporarily deployed to development June 11, 2025 07:16 — with GitHub Actions Inactive

github-actions Bot reviewed Jun 11, 2025

View reviewed changes

tombeckenham temporarily deployed to development June 11, 2025 07:18 — with GitHub Actions Inactive

Add template to serve for EIP-6963 testing

399138c

Closes #1040

tombeckenham temporarily deployed to development June 11, 2025 23:18 — with GitHub Actions Inactive

github-actions Bot reviewed Jun 11, 2025

View reviewed changes

tombeckenham temporarily deployed to development June 11, 2025 23:20 — with GitHub Actions Inactive

Merge branch 'dev' into 1040-bug-kitty-punch-referral-code-transactio…

f5c56fa

…n-cant-open-popup Closes #1040

tombeckenham temporarily deployed to development June 26, 2025 22:55 — with GitHub Actions Inactive

github-actions Bot reviewed Jun 26, 2025

View reviewed changes

cursor Bot reviewed Jun 26, 2025

View reviewed changes

tombeckenham had a problem deploying to development June 26, 2025 22:57 — with GitHub Actions Failure

		// Wait a bit for the server to start (or poll the port)
		await new Promise((resolve) => setTimeout(resolve, 3000));

	// max concurrent request limit
	protected _requestIdPool = [...Array(100).keys()];

	mapMethod !== 'walletDisconnect' &&
	// mapMethod !== 'ethChainId' &&

Uh oh!

Conversation

zzggo commented Jun 10, 2025

Related Issue

Summary of Changes

Need Regression Testing

Risk Assessment

Additional Notes

Screenshots (if applicable)

Uh oh!

github-actions Bot commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Changes

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

github-actions Bot Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

Uh oh!

github-actions Bot Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

tombeckenham Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

github-actions Bot Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

github-actions Bot Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

github-actions Bot Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Bug: Request ID Pool Size Cuts Cause High Load Failures

Bug: `ethChainId` Method Requires Unexpected User Permission

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Jun 10, 2025 •

edited

Loading