feat(connector): add ClientId #6973
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds ClientID support to the connector system, enabling isolated state management per DApp client (Dapp browser, Chrome extension, wallet-connect).
Key changes include:
- Database migration to add
client_idcolumn with composite PRIMARY KEY (url, client_id) - API updates to include
ClientIDfield in relevant structs and function signatures - Backward compatibility support for existing clients using empty string as default client ID
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| walletdatabase/migrations/sql/1759312232_add_client_id_to_connector_dapps.up.sql | Migration script to add client_id column and composite primary key |
| signal/events_connector.go | Added ClientID field to ConnectorDApp struct |
| services/connector/database/persistence.go | Updated database operations to use URL+ClientID composite key |
| services/connector/database/persistence_test.go | Added comprehensive tests for multi-client scenarios |
| services/connector/commands/*.go | Updated all command handlers to use ClientID in database operations |
| services/connector/api.go | Modified API methods to accept and use ClientID parameter |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Jenkins BuildsClick to see older builds (18)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6973 +/- ##
===========================================
+ Coverage 55.44% 59.53% +4.08%
===========================================
Files 832 834 +2
Lines 120083 120115 +32
===========================================
+ Hits 66584 71513 +4929
+ Misses 46530 41325 -5205
- Partials 6969 7277 +308
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| if args.ClientID == "" { | ||
| args.ClientID = DefaultClientID | ||
| } |
There was a problem hiding this comment.
I think we must not do this, as it introduces a vulnerability.
Examples
Imagine this case:
- User permits
OpenSeadApp with WalletConnect - The application is running and exposing the connector service to
localhost - Now any local app can connect to
localhost:8586and withClientID: wallet-connectand no UI popup will appear.
Transactions would still require manual approval, but at least the list of accounts will be available this way.
Proposal
ClientID should not be set by the client, but only by status-go / status-desktop.
So we must set ClientID inside "secure environment":
- inside
status-go- knowing that the connection is coming from HTTP/WS - inside
status-desktop(coming fromCallRPC) - we trust it, because the C-bindings connection is secure enough.
So I think it's closer to ConnectionType rather than ClientID.
A problem
There might still be a clash between some constant httpClientID = "http-client" that we define in status-go and an arbitrary string coming from status-desktop.
We can control status-desktop client ids, but we can't control all other potential clients of status-go.
So not sure how we should go with this 🤔
Perhaps it's ok if we just define the forbidden constant in status-go docs.
There was a problem hiding this comment.
I agree – passing clientId directly looks like a vulnerability. It would be best to set the clientId (connection‑type) at the moment the connection is created
I think clientId is still useful because the desktop C‑bindings (will) expose both wallet‑connect and dapp‑browser.
Proposed approach:
http/wscreates a connection withconnection-type="http/public", possibly with a pre‑defined clientId- For status‑desktop use
connection-type="desktop/secure". - In the
*v2methods we will allow the caller to pass aclientId. The connector will respect theclientIdonly when the connection type is secure.
In other words, use connection-type for general protection and clientId for state isolation per client.
Additionally, I’d add a session identifier to the Login response so that an established connection can’t be hijacked by another application
friofry
force-pushed
the
ab/issue-6972-connector-client-id
branch
from
6a7f16c to
9ceeeea
Compare
friofry
force-pushed
the
ab/issue-6972-connector-client-id
branch
from
afae65c to
4b0b6d6
Compare
3 participants
fixes #6972
Allow isolated state management per dapp (Dapp browser, chrome extension, wallet-connect)
Database:
client_idcolumn with composite PRIMARY KEY (url, client_id). Existing records migrated with client_id = '' for backward compatibility.API:
ClientIDfield toRPCRequest,ConnectorDApp,DAppDefaultClientIDconstant for backward compatibility. Old clients(browser extension) automatically use empty string "".