NWC Improvements#42
Conversation
|
Warning Review limit reached
More reviews will be available in 48 minutes and 47 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds Lightning Address support to NWC nodes with structured NIP-47 error handling. It introduces LNURL-verify validation for payment requests, centralizes error translation to preserve wallet error codes, and exposes Lightning Address metadata via ChangesNWC Lightning Address and Error Handling
Sequence Diagram(s)sequenceDiagram
participant App
participant NwcNode
participant URIParse as URI Parser
participant LnurlVerify as LNURL Verify
participant LNURL as LNURL-pay
participant Callback
participant Verify as LNURL-verify
App->>NwcNode: getLightningAddress()
NwcNode->>URIParse: extract lud16 from nwcUri
URIParse-->>NwcNode: lightningAddress
NwcNode->>LnurlVerify: verifyLightningAddressPayRequest
LnurlVerify->>LNURL: resolve Lightning Address
LNURL-->>LnurlVerify: pay endpoint + amount range
LnurlVerify->>Callback: request invoice with amount
Callback-->>LnurlVerify: pr + verify endpoint
alt has verify endpoint
LnurlVerify->>Verify: follow verify URL
Verify-->>LnurlVerify: status OK
LnurlVerify-->>NwcNode: verified
else no verify endpoint
LnurlVerify-->>NwcNode: throw LnurlVerifyUnsupportedError
end
NwcNode-->>App: LightningAddressInfo with flag
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26b40ebacd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }).then( | ||
| () => true, | ||
| () => false, | ||
| ); |
There was a problem hiding this comment.
When the NWC URI contains a malformed lud16 such as lud16=notaddress, verifyLightningAddressPayRequest rejects while parsing the Lightning Address, but this rejection handler converts every failure into lnurlVerifySupported: false. That makes getLightningAddress() return { lightningAddress: 'notaddress', lnurlVerifySupported: false } instead of surfacing the invalid URI, so callers cannot distinguish an unsupported verify endpoint from bad NWC metadata. Validate the parsed lud16 before swallowing only the expected LNURL-verify unsupported/probe failures.
Useful? React with 👍 / 👎.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
bindings/typescript/src/__tests__/nwc.test.ts (1)
127-140: 💤 Low valueConsider consolidating duplicate payInvoice calls.
The test makes two separate calls to
makeNode().payInvoice()with different assertions. This could be refactored to capture the rejection once:- await expect(makeNode().payInvoice({ invoice: BOLT11_INVOICE })).rejects.toMatchObject({ - name: 'NwcError', - code: 'NwcError', - nwcCode: 'QUOTA_EXCEEDED', - nwcMessage: 'quota spent', - operation: 'pay_invoice', - message: 'quota spent', - }); - - await expect(makeNode().payInvoice({ invoice: BOLT11_INVOICE })).rejects.toBeInstanceOf(NwcError); + const error = await makeNode().payInvoice({ invoice: BOLT11_INVOICE }).catch(e => e); + + expect(error).toMatchObject({ + name: 'NwcError', + code: 'NwcError', + nwcCode: 'QUOTA_EXCEEDED', + nwcMessage: 'quota spent', + operation: 'pay_invoice', + message: 'quota spent', + }); + expect(error).toBeInstanceOf(NwcError);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindings/typescript/src/__tests__/nwc.test.ts` around lines 127 - 140, Call makeNode().payInvoice(...) only once and reuse that rejected promise for both assertions: assign the result of makeNode().payInvoice({ invoice: BOLT11_INVOICE }) to a variable (e.g., rejectedPromise) after configuring nwcMocks.payInvoice to throw the Nip47Error, then await expect(rejectedPromise).rejects.toMatchObject({...}) and await expect(rejectedPromise).rejects.toBeInstanceOf(NwcError). Keep references to makeNode, payInvoice, NwcError, nwcMocks.payInvoice, and nwcMocks.Nip47Error to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/typescript/src/lnurl.ts`:
- Around line 397-400: The current check treats empty strings for maybeVerify.pr
and maybeVerify.verify as valid because it only tests typeof, leading to
downstream URL parsing errors instead of throwing LnurlVerifyUnsupportedError;
update the validation in the handler that casts callbackResponse to
Partial<LnurlVerifyInvoiceResponse> (the maybeVerify variable) to additionally
ensure both maybeVerify.pr and maybeVerify.verify are non-empty (e.g., check
they are strings with length > 0 or trim() !== '') and throw
LnurlVerifyUnsupportedError() when either is missing/empty so empty values are
normalized as unsupported verify responses.
In `@bindings/typescript/src/nodes/nwc.ts`:
- Around line 169-186: The current getLightningAddress implementation swallows
all errors from verifyLightningAddressPayRequest and returns
lnurlVerifySupported: false; change it to only treat LnurlVerifyUnsupportedError
as "unsupported" while letting genuine failures propagate: call
verifyLightningAddressPayRequest (from bindings/typescript/src/lnurl.ts) and
catch exceptions only of type LnurlVerifyUnsupportedError to set
lnurlVerifySupported = false, but rethrow any other errors (e.g. instances of
LniError or unknown errors) so they surface to the caller; keep the existing
extraction via extractLightningAddressFromNwcUri and preserve passing
this.options.fetch to the verify call.
In `@crates/lni/lnurl/mod.rs`:
- Around line 266-290: The code in verify_lightning_address_pay_request
currently fetches URLs derived from untrusted LNURL data (via
lightning_address_to_url, well_known.callback and verify_response.verify) using
fetch_lnurl_pay and fetch_lnurl_json_value without validating them first; add
strict URL validation before any outbound fetch: parse each URL (the URL
produced by lightning_address_to_url, the callback URL built by
callback_url_with_amount(&well_known.callback, …), and verify_response.verify),
ensure the scheme is https, reject IP-literal hosts or resolve the hostname and
verify the resolved addresses are not loopback/private/RFC1918/cloud metadata
ranges, or apply a safe-host allowlist; perform these checks in
verify_lightning_address_pay_request immediately before calling fetch_lnurl_pay
and before each fetch_lnurl_json_value, returning ApiError::InvalidInput on
failure.
---
Nitpick comments:
In `@bindings/typescript/src/__tests__/nwc.test.ts`:
- Around line 127-140: Call makeNode().payInvoice(...) only once and reuse that
rejected promise for both assertions: assign the result of
makeNode().payInvoice({ invoice: BOLT11_INVOICE }) to a variable (e.g.,
rejectedPromise) after configuring nwcMocks.payInvoice to throw the Nip47Error,
then await expect(rejectedPromise).rejects.toMatchObject({...}) and await
expect(rejectedPromise).rejects.toBeInstanceOf(NwcError). Keep references to
makeNode, payInvoice, NwcError, nwcMocks.payInvoice, and nwcMocks.Nip47Error to
locate the code to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 633a7b89-7db4-4e44-ae1b-be6f8a9fa097
📒 Files selected for processing (9)
bindings/typescript/src/__tests__/nwc.test.tsbindings/typescript/src/errors.tsbindings/typescript/src/lnurl.tsbindings/typescript/src/nodes/nwc.tsbindings/typescript/src/types.tscrates/lni/lib.rscrates/lni/lnurl/mod.rscrates/lni/nwc/api.rscrates/lni/nwc/lib.rs
| const maybeVerify = callbackResponse as Partial<LnurlVerifyInvoiceResponse>; | ||
| if (typeof maybeVerify.pr !== 'string' || typeof maybeVerify.verify !== 'string') { | ||
| throw new LnurlVerifyUnsupportedError(); | ||
| } |
There was a problem hiding this comment.
Normalize empty pr/verify as unsupported verify responses.
Line [398] only checks typeof, so empty strings pass and then fail later as generic URL parsing errors instead of LnurlVerifyUnsupportedError.
Proposed fix
- if (typeof maybeVerify.pr !== 'string' || typeof maybeVerify.verify !== 'string') {
+ if (
+ typeof maybeVerify.pr !== 'string' ||
+ maybeVerify.pr.trim() === '' ||
+ typeof maybeVerify.verify !== 'string' ||
+ maybeVerify.verify.trim() === ''
+ ) {
throw new LnurlVerifyUnsupportedError();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const maybeVerify = callbackResponse as Partial<LnurlVerifyInvoiceResponse>; | |
| if (typeof maybeVerify.pr !== 'string' || typeof maybeVerify.verify !== 'string') { | |
| throw new LnurlVerifyUnsupportedError(); | |
| } | |
| const maybeVerify = callbackResponse as Partial<LnurlVerifyInvoiceResponse>; | |
| if ( | |
| typeof maybeVerify.pr !== 'string' || | |
| maybeVerify.pr.trim() === '' || | |
| typeof maybeVerify.verify !== 'string' || | |
| maybeVerify.verify.trim() === '' | |
| ) { | |
| throw new LnurlVerifyUnsupportedError(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/typescript/src/lnurl.ts` around lines 397 - 400, The current check
treats empty strings for maybeVerify.pr and maybeVerify.verify as valid because
it only tests typeof, leading to downstream URL parsing errors instead of
throwing LnurlVerifyUnsupportedError; update the validation in the handler that
casts callbackResponse to Partial<LnurlVerifyInvoiceResponse> (the maybeVerify
variable) to additionally ensure both maybeVerify.pr and maybeVerify.verify are
non-empty (e.g., check they are strings with length > 0 or trim() !== '') and
throw LnurlVerifyUnsupportedError() when either is missing/empty so empty values
are normalized as unsupported verify responses.
| async getLightningAddress(): Promise<LightningAddressInfo> { | ||
| const lightningAddress = extractLightningAddressFromNwcUri(this.config.nwcUri); | ||
| if (!lightningAddress) { | ||
| throw new LniError('InvalidInput', 'NWC URI does not include a lud16 Lightning Address.'); | ||
| } | ||
|
|
||
| const lnurlVerifySupported = await verifyLightningAddressPayRequest(lightningAddress, { | ||
| fetch: this.options.fetch, | ||
| }).then( | ||
| () => true, | ||
| () => false, | ||
| ); | ||
|
|
||
| return { | ||
| lightningAddress, | ||
| lnurlVerifySupported, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Error handling may hide genuine failures.
The current implementation catches all errors from verifyLightningAddressPayRequest and returns lnurlVerifySupported: false. This could mask real failures like network errors, invalid Lightning Address format, or HTTP issues.
Based on the LNURL helper (see bindings/typescript/src/lnurl.ts:282-416), verifyLightningAddressPayRequest can throw:
LnurlVerifyUnsupportedErrorwhen the verify endpoint is missing (expected)LniErrorfor network failures, invalid formats, or other issues (should propagate)
Consider catching only the expected error:
+import { LnurlVerifyUnsupportedError } from '../lnurl.js';
async getLightningAddress(): Promise<LightningAddressInfo> {
const lightningAddress = extractLightningAddressFromNwcUri(this.config.nwcUri);
if (!lightningAddress) {
throw new LniError('InvalidInput', 'NWC URI does not include a lud16 Lightning Address.');
}
- const lnurlVerifySupported = await verifyLightningAddressPayRequest(lightningAddress, {
- fetch: this.options.fetch,
- }).then(
- () => true,
- () => false,
- );
+ let lnurlVerifySupported = false;
+ try {
+ await verifyLightningAddressPayRequest(lightningAddress, {
+ fetch: this.options.fetch,
+ });
+ lnurlVerifySupported = true;
+ } catch (error) {
+ if (error instanceof LnurlVerifyUnsupportedError) {
+ lnurlVerifySupported = false;
+ } else {
+ throw error;
+ }
+ }
return {
lightningAddress,
lnurlVerifySupported,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getLightningAddress(): Promise<LightningAddressInfo> { | |
| const lightningAddress = extractLightningAddressFromNwcUri(this.config.nwcUri); | |
| if (!lightningAddress) { | |
| throw new LniError('InvalidInput', 'NWC URI does not include a lud16 Lightning Address.'); | |
| } | |
| const lnurlVerifySupported = await verifyLightningAddressPayRequest(lightningAddress, { | |
| fetch: this.options.fetch, | |
| }).then( | |
| () => true, | |
| () => false, | |
| ); | |
| return { | |
| lightningAddress, | |
| lnurlVerifySupported, | |
| }; | |
| } | |
| import { LnurlVerifyUnsupportedError } from '../lnurl.js'; | |
| async getLightningAddress(): Promise<LightningAddressInfo> { | |
| const lightningAddress = extractLightningAddressFromNwcUri(this.config.nwcUri); | |
| if (!lightningAddress) { | |
| throw new LniError('InvalidInput', 'NWC URI does not include a lud16 Lightning Address.'); | |
| } | |
| let lnurlVerifySupported = false; | |
| try { | |
| await verifyLightningAddressPayRequest(lightningAddress, { | |
| fetch: this.options.fetch, | |
| }); | |
| lnurlVerifySupported = true; | |
| } catch (error) { | |
| if (error instanceof LnurlVerifyUnsupportedError) { | |
| lnurlVerifySupported = false; | |
| } else { | |
| throw error; | |
| } | |
| } | |
| return { | |
| lightningAddress, | |
| lnurlVerifySupported, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/typescript/src/nodes/nwc.ts` around lines 169 - 186, The current
getLightningAddress implementation swallows all errors from
verifyLightningAddressPayRequest and returns lnurlVerifySupported: false; change
it to only treat LnurlVerifyUnsupportedError as "unsupported" while letting
genuine failures propagate: call verifyLightningAddressPayRequest (from
bindings/typescript/src/lnurl.ts) and catch exceptions only of type
LnurlVerifyUnsupportedError to set lnurlVerifySupported = false, but rethrow any
other errors (e.g. instances of LniError or unknown errors) so they surface to
the caller; keep the existing extraction via extractLightningAddressFromNwcUri
and preserve passing this.options.fetch to the verify call.
| pub async fn verify_lightning_address_pay_request(lightning_address: &str) -> Result<(), ApiError> { | ||
| let PaymentDestination::LightningAddress { user, domain } = PaymentDestination::parse(lightning_address)? else { | ||
| return Err(ApiError::InvalidInput("Expected Lightning Address".to_string())); | ||
| }; | ||
| let well_known = fetch_lnurl_pay(&lightning_address_to_url(&user, &domain)).await?; | ||
| let amount_msats = std::cmp::min( | ||
| std::cmp::max(100_000, well_known.min_sendable), | ||
| well_known.max_sendable, | ||
| ); | ||
|
|
||
| if amount_msats < well_known.min_sendable || amount_msats > well_known.max_sendable { | ||
| return Err(ApiError::InvalidInput("Invalid LNURL sendable amount range".to_string())); | ||
| } | ||
|
|
||
| let callback_response = fetch_lnurl_json_value(&callback_url_with_amount(&well_known.callback, amount_msats)?).await?; | ||
| let verify_response: LnurlVerifyPayResponse = serde_json::from_value(callback_response) | ||
| .map_err(|_| ApiError::InvalidInput("LNURL-verify endpoint is not supported".to_string()))?; | ||
|
|
||
| if verify_response.pr.is_empty() || verify_response.verify.is_empty() { | ||
| return Err(ApiError::InvalidInput("LNURL-verify endpoint is not supported".to_string())); | ||
| } | ||
|
|
||
| let verify_result = fetch_lnurl_json_value(&verify_response.verify).await?; | ||
| handle_lnurl_ok_value(&verify_result, "LNURL verify") | ||
| } |
There was a problem hiding this comment.
Validate LNURL callback/verify URLs before fetching (SSRF risk).
Lines [270], [280], and [288] perform outbound GETs to URLs derived from untrusted remote data without HTTPS/public-host validation. A malicious Lightning Address endpoint can return callback/verify URLs to internal targets (e.g., RFC1918 or metadata services), and this code will fetch them.
Suggested hardening direction
+fn validate_public_https_url(raw: &str) -> Result<reqwest::Url, ApiError> {
+ let url = reqwest::Url::parse(raw)
+ .map_err(|e| ApiError::InvalidInput(format!("Invalid LNURL URL: {}", e)))?;
+ if url.scheme() != "https" {
+ return Err(ApiError::InvalidInput("LNURL endpoints must use HTTPS".to_string()));
+ }
+ // Reject credentials + private/local hosts/IP ranges here.
+ Ok(url)
+}
...
- let callback_response = fetch_lnurl_json_value(&callback_url_with_amount(&well_known.callback, amount_msats)?).await?;
+ let callback_url = callback_url_with_amount(&well_known.callback, amount_msats)?;
+ let callback_url = validate_public_https_url(&callback_url)?;
+ let callback_response = fetch_lnurl_json_value(callback_url.as_str()).await?;
...
- let verify_result = fetch_lnurl_json_value(&verify_response.verify).await?;
+ let verify_url = validate_public_https_url(&verify_response.verify)?;
+ let verify_result = fetch_lnurl_json_value(verify_url.as_str()).await?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/lni/lnurl/mod.rs` around lines 266 - 290, The code in
verify_lightning_address_pay_request currently fetches URLs derived from
untrusted LNURL data (via lightning_address_to_url, well_known.callback and
verify_response.verify) using fetch_lnurl_pay and fetch_lnurl_json_value without
validating them first; add strict URL validation before any outbound fetch:
parse each URL (the URL produced by lightning_address_to_url, the callback URL
built by callback_url_with_amount(&well_known.callback, …), and
verify_response.verify), ensure the scheme is https, reject IP-literal hosts or
resolve the hostname and verify the resolved addresses are not
loopback/private/RFC1918/cloud metadata ranges, or apply a safe-host allowlist;
perform these checks in verify_lightning_address_pay_request immediately before
calling fetch_lnurl_pay and before each fetch_lnurl_json_value, returning
ApiError::InvalidInput on failure.
1 participant
Summary by CodeRabbit
New Features
getLightningAddress()method to retrieve Lightning Address from NWC configurationImprovements
Tests