feat: add chain data source and address to about page#2013
feat: add chain data source and address to about page#2013im-adithya merged 6 commits intogetAlby:masterfrom
Conversation
frnandu
left a comment
frnandu
left a comment
There was a problem hiding this comment.
Hi, thanks for your PR, please resolve conflicts with master and address the feedback.
📝 WalkthroughWalkthroughEnriches the API InfoResponse with chain data source fields, populates them for LDK backends via a new LDKService.GetChainDataSource(), and surfaces the values to frontend types and the About UI when present. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend
participant API
participant LDKService
participant Config
Frontend->>API: GET /info
API->>LDKService: svc.GetLNClient() -> type assert chainSourceProvider
alt LN client supports chain source
API->>LDKService: GetChainDataSource()
LDKService->>Config: read env/config (bitcoind/electrum/esplora)
Config-->>LDKService: source type + endpoint
LDKService-->>API: sanitized type + address
end
API-->>Frontend: InfoResponse (includes chainDataSourceType/address)
Frontend->>Frontend: render About UI with chain data source
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
api/api.go (1)
1326-1328:⚠️ Potential issue | 🟠 MajorDo not expose internal LND address in
/api/info.Publishing
LNDAddressleaks internal node endpoint details and should be omitted from public info responses.🔧 Proposed fix
switch backendType { case config.LDKBackendType: // Only LDK supports this right now. Using a local interface here @@ if ldkService, ok := api.svc.GetLNClient().(chainSourceProvider); ok { info.ChainDataSourceType, info.ChainDataSourceAddress = ldkService.GetChainDataSource() } - - case config.LNDBackendType: - info.ChainDataSourceType = "lnd" - info.ChainDataSourceAddress, _ = api.cfg.Get("LNDAddress", "") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/api.go` around lines 1326 - 1328, The code sets info.ChainDataSourceAddress from the internal LNDAddress when handling config.LNDBackendType; remove that exposure by not reading or assigning api.cfg.Get("LNDAddress", "") to info.ChainDataSourceAddress in the LND branch (leave it empty or masked instead), so the /api/info response no longer leaks internal node endpoint details; update any tests or callers that assume this field is populated for LND to expect empty/masked value.lnclient/ldk/ldk.go (1)
2469-2480:⚠️ Potential issue | 🟠 MajorRedact chain-source address secrets and return complete bitcoind endpoint.
This method can leak credentials if a URL contains userinfo, and the bitcoind branch currently drops the RPC port (incomplete address).
🔧 Proposed fix
+import "net/url" + +func sanitizeAddress(raw string) string { + u, err := url.Parse(raw) + if err != nil || u.Host == "" { + return raw + } + u.User = nil + return u.String() +} + func (ls *LDKService) GetChainDataSource() (string, string) { if host := ls.cfg.GetEnv().LDKBitcoindRpcHost; host != "" { - return "bitcoind", host + port := ls.cfg.GetEnv().LDKBitcoindRpcPort + if port != "" { + return "bitcoind", host + ":" + port + } + return "bitcoind", host } if host := ls.cfg.GetEnv().LDKElectrumServer; host != "" { - return "electrum", host + return "electrum", sanitizeAddress(host) } // Fallback to Esplora host := ls.cfg.GetEnv().LDKEsploraServer - return "esplora", host + return "esplora", sanitizeAddress(host) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lnclient/ldk/ldk.go` around lines 2469 - 2480, GetChainDataSource currently can leak credentials from userinfo in URLs and returns an incomplete bitcoind address; fix by parsing the configured endpoints (ls.cfg.GetEnv().LDKBitcoindRpcHost, LDKElectrumServer, LDKEsploraServer) as URLs, strip any URL.User (redact username/password) before returning, and for the bitcoind branch ensure the returned host includes an explicit host:port (use the URL's port if present, otherwise append a sensible RPC port from config or a default such as 8332) so the returned tuple is "bitcoind" plus a complete, credential-free endpoint; update GetChainDataSource to perform parsing, userinfo removal, and port normalization for all branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api/api.go`:
- Around line 1326-1328: The code sets info.ChainDataSourceAddress from the
internal LNDAddress when handling config.LNDBackendType; remove that exposure by
not reading or assigning api.cfg.Get("LNDAddress", "") to
info.ChainDataSourceAddress in the LND branch (leave it empty or masked
instead), so the /api/info response no longer leaks internal node endpoint
details; update any tests or callers that assume this field is populated for LND
to expect empty/masked value.
In `@lnclient/ldk/ldk.go`:
- Around line 2469-2480: GetChainDataSource currently can leak credentials from
userinfo in URLs and returns an incomplete bitcoind address; fix by parsing the
configured endpoints (ls.cfg.GetEnv().LDKBitcoindRpcHost, LDKElectrumServer,
LDKEsploraServer) as URLs, strip any URL.User (redact username/password) before
returning, and for the bitcoind branch ensure the returned host includes an
explicit host:port (use the URL's port if present, otherwise append a sensible
RPC port from config or a default such as 8332) so the returned tuple is
"bitcoind" plus a complete, credential-free endpoint; update GetChainDataSource
to perform parsing, userinfo removal, and port normalization for all branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 437e44b6-3f94-4f2c-b50f-4315277255d0
📒 Files selected for processing (5)
api/api.goapi/models.gofrontend/src/screens/settings/About.tsxfrontend/src/types.tslnclient/ldk/ldk.go
| } | ||
| } | ||
|
|
||
| func (ls *LDKService) GetChainDataSource() (string, string) { |
There was a problem hiding this comment.
Instead of adding an LDK only method, can we return this in lnclient.NodeInfo optionally (*string) cc @rolznz, wdyt?
There was a problem hiding this comment.
I think this is slightly different type of data, it's more internal configuration - do apps need to know about it? (a bit different than the network or alias for example)
|
ACK |
|
@frnandu do you see any possibility of this being used by LND or CLN? otherwise I wonder if this should be a custom node command in LDK only. |
|
I think this is mainly for LDK, so that later we can also give an option to change this to the user's self-hosted onchain esplora/electrum/bitcoind which is something that most wallets also provide and IMO quite an important and necessary feature for serious wallets. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lnclient/ldk/ldk.go (1)
2485-2517: Credentials properly stripped; consider edge case fallback.The implementation correctly addresses the prior review feedback by setting
u.User = nilto strip credentials. The URL parsing with"//"prefix fallback andnet.JoinHostPortfor IPv6 are well-handled.One minor edge case: if both
url.Parse(endpoint)andurl.Parse("//" + endpoint)fail to produce a valid host (lines 2486-2492), the function returns the original endpoint unchanged. For truly malformed but credential-containing inputs, this could theoretically leak credentials. However, this is a defensive fallback for unlikely inputs.Optional: Return empty string or error indicator for unparseable endpoints
func sanitizeChainEndpoint(endpoint string, port string) string { u, err := url.Parse(endpoint) if err != nil || u.Host == "" { u, err = url.Parse("//" + endpoint) } if err != nil { - return endpoint + return "" } u.User = nil host := u.Hostname() if host == "" { - return endpoint + return "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lnclient/ldk/ldk.go` around lines 2485 - 2517, sanitizeChainEndpoint currently returns the original endpoint if URL parsing fails or no host is found, which can leak credentials; update sanitizeChainEndpoint so that when both url.Parse(endpoint) and url.Parse("//"+endpoint) fail or host is empty it does not return the raw input but instead returns a safe fallback (e.g., empty string or a clearly sanitized indicator) or strip credentials manually before returning; locate and modify the error/host-fallback branch inside sanitizeChainEndpoint (around the url.Parse handling and the host=="" check) to ensure no userinfo from the original endpoint can be returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lnclient/ldk/ldk.go`:
- Around line 2485-2517: sanitizeChainEndpoint currently returns the original
endpoint if URL parsing fails or no host is found, which can leak credentials;
update sanitizeChainEndpoint so that when both url.Parse(endpoint) and
url.Parse("//"+endpoint) fail or host is empty it does not return the raw input
but instead returns a safe fallback (e.g., empty string or a clearly sanitized
indicator) or strip credentials manually before returning; locate and modify the
error/host-fallback branch inside sanitizeChainEndpoint (around the url.Parse
handling and the host=="" check) to ensure no userinfo from the original
endpoint can be returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02d9cfce-be2e-42a6-8765-b39b6276390a
📒 Files selected for processing (3)
frontend/src/screens/settings/About.tsxlnclient/ldk/ldk.golnclient/ldk/ldk_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/screens/settings/About.tsx
4 participants
I noticed #1948 has been open for a while so I decided to take a crack at it.
GetChainDataSourcehelper to detect LDK verification mediumchainDataSourceTypeandchainDataSourceAddressin/api/infoSummary by CodeRabbit