blocknative / web3-onboard

Client library to onboard users to web3 apps
https://onboard.blocknative.com/
MIT License
832 stars 490 forks source link

CORS issue with Ledger #1996

Open charlie-kim opened 10 months ago

charlie-kim commented 10 months ago

Current Behavior

The app crashes with CORS error when I try to connect with Ledger desktop or mobile app. Connecting with ledger via Metamask works fine.

Access to fetch at 'https://proxyseg.api.live.ledger.com//v1/projects/XXX/settings' from origin 'https://YYY.xyz' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

_GET https://proxyseg.api.live.ledger.com//v1/projects/XXX/settings net::ERRFAILED 404 (Not Found)

{context: 'client'} 'User rejected'

{context: 'client'} Error: No matching key. history: ZZZZ

Project identifier(XXX in the example above) is different from WalletConnect project ID. I am not sure where XXX is coming from. I couldn't find any concept of "project" in ledger live document.

I did set Allowed Domains in WalletConnect config. And the domain is also verified.

Expected Behavior

Sign in successfully without crash.

Steps To Reproduce

import ledgerModule from "@web3-onboard/ledger";

const ledger = ledgerModule({
  enableDebugLogs: true,
  walletConnectVersion: 2,
  requiredChains: [5],
  projectId='ABCD', // Project ID from WalletConnect
});

What package is effected by this issue?

@web3-onboard/ledger

Is this a build or a runtime issue?

Runtime

Package Version

2.5.2

Node Version

18.12.0

What browsers are you seeing the problem on?

Chrome

Relevant log output

No response

Anything else?

Mobile login also crashes.

IMG_2855

Sanity Check

charlie-kim commented 10 months ago

@Adamj1232 @aaronbarnardsound @leightkt can you please help with this issue?

hnbt commented 7 months ago

@Adamj1232 @aaronbarnardsound @leightkt @charlie-kim Wondering if theres been an update. We're seeing this exact error too.

Adamj1232 commented 7 months ago

@charlie-kim I am unable to reproduce with our examples. Can you try removing a couple of the init options down to only the projectId as seen here https://github.com/blocknative/react-demo/blob/66f1f388c0af2eafcb37d4e47e3b0c9bf3c297b2/src/services.js#L105 this is also deployed at https://reactdemo.blocknative.com/ if you would like to test there. Is your Allow Domains on the cloud.walletconnect setting using a full path and no *s?

charlie-kim commented 7 months ago

@Adamj1232 I am not sure how you were able to run with just projectId option. I am getting typescript error below with projectId only.

Argument of type '{ projectId: string; }' is not assignable to parameter of type 'LedgerOptions | undefined'.
  Property 'walletConnectVersion' is missing in type '{ projectId: string; }' but required in type 'LedgerOptionsWCv2'.ts(2345)

Allowed Domains for walletconnect project is set using * as below. I am not sure that's the problem though. https://*.raincards.xyz https://*.api.live.ledger.com

Also, I talked to ledger support. They have no idea what https://proxyseg.api.live.ledger.com//v1/projects URL is and why web3-onboard is making request to it. Do you know?

Wozacosta commented 7 months ago

Hey @charlie-kim @Adamj1232 @hnbt,

The call to 'https://proxyseg.api.live.ledger.com//v1/projects/XXX/settings' was made by the ledger-connect-kit and was used by the segment library in there to route analytics data.

It was documented here

In the latest version of this connect-kit (starting at 1.1.11), the whole analytics part of the package was removed to remove potential attack vectors. It thus removes the need to setup any CSP, and should fix those CORS issues.

Note that for security reasons, the connect-kit-loader is now deprecated and only loads the connect-kit version 1.1.8.

To that end, I've opened a PR that uses the latest (1.1.12) connect-kit directly from the package manager and removes the use of the connect-kit-loader

charlie-kim commented 7 months ago

Thanks @Wozacosta for your input. I should try when the PR is merged.