Add LUD-12 comment to dev fee LNURL-pay calls#820
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDev-fee LNURL payments now include order and node-key comments. LNURL callback construction supports optional comments, preserves existing parameters, validates HTTP(S) endpoints, and handles reported errors. Scheduler and regular payment callers pass the updated arguments. ChangesDev-fee LNURL comment flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant DevFeeCycle
participant InvoiceResolver
participant LNURLService
Scheduler->>DevFeeCycle: provide current Keys
DevFeeCycle->>InvoiceResolver: resolve order with Keys
InvoiceResolver->>LNURLService: request invoice with order ID and public key comment
LNURLService-->>InvoiceResolver: return invoice or empty result on reported error
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
src/lnurl.rs (1)
101-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd Rust doc comments to modified public APIs.
As per coding guidelines, non-obvious public APIs must be documented with Rust doc comments (
///). The signatures of these public functions were modified or expanded in this PR, making this an ideal time to add documentation clarifying their purpose, parameters, and return shapes.
src/lnurl.rs#L101-L105: Add a doc comment forresolv_ln_address.src/app/dev_fee.rs#L91-L92: Add a doc comment forrun_dev_fee_cycle.src/app/dev_fee.rs#L889-L892: Add a doc comment forresolve_dev_fee_invoice.🤖 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 `@src/lnurl.rs` around lines 101 - 105, Add Rust doc comments describing the purpose, parameters, and return value for the public functions resolv_ln_address in src/lnurl.rs (lines 101-105), run_dev_fee_cycle in src/app/dev_fee.rs (lines 91-92), and resolve_dev_fee_invoice in src/app/dev_fee.rs (lines 889-892). Use /// comments directly above each function.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@src/lnurl.rs`:
- Around line 101-105: Add Rust doc comments describing the purpose, parameters,
and return value for the public functions resolv_ln_address in src/lnurl.rs
(lines 101-105), run_dev_fee_cycle in src/app/dev_fee.rs (lines 91-92), and
resolve_dev_fee_invoice in src/app/dev_fee.rs (lines 889-892). Use /// comments
directly above each function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b3d5a65-2132-4ca7-b84c-07bd5dcaeda5
📒 Files selected for processing (4)
src/app/dev_fee.rssrc/app/release.rssrc/lnurl.rssrc/scheduler.rs
Addresses CodeRabbit nitpick on PR MostroP2P#820. run_dev_fee_cycle and resolve_dev_fee_invoice were already documented; only resolv_ln_address was missing one.
|
@ToRyVand thanks for your contribution, can you please solve conflicts? |
Dev fee payments carried no identifying information, making it impossible to trace which order or Mostro node originated a given payment on the receiving end. Add an optional LUD-12 comment (order id + node pubkey) to the LNURL-pay callback, gated by the server-advertised commentAllowed limit. While rewriting the callback-building code to attach the comment, found that amount was concatenated onto the raw callback string before parsing it as a URL. Real LNURL servers can return a callback that already carries its own query string (e.g. ?id=...), so the second ? silently mangled the query and amount never reached the server as its own parameter. Extract build_callback_url, a pure helper that adds both amount and comment via query_pairs_mut, with a regression test for a callback with a pre-existing query string. Also thread the already-cached AppContext::keys() through the scheduler -> dev fee cycle instead of calling get_keys() (which re-parses the nsec on every call), matching the pattern the codebase already documents as preferred. Closes MostroP2P#694
Addresses CodeRabbit nitpick on PR MostroP2P#820. run_dev_fee_cycle and resolve_dev_fee_invoice were already documented; only resolv_ln_address was missing one.
eebcdd0 to
0af1019
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/dev_fee.rs (1)
1525-1540: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up temporary certificates to prevent
/tmpbloat.The
dead_lndtest helper creates a new UUID-named directory in/tmpcontaining test certificates on every invocation, but never deletes it. Over time, running tests locally will accumulate these orphaned directories.Since
fedimint_tonic_lnd::connectreads the certificate files eagerly during initialization, you can safely remove the temporary directory immediately after the connection attempt.♻️ Proposed refactor
async fn dead_lnd() -> LndConnector { let dir = std::env::temp_dir().join(format!("mostro-test-lnd-{}", uuid::Uuid::new_v4())); std::fs::create_dir_all(&dir).unwrap(); let cert = dir.join("tls.cert"); let mac = dir.join("admin.macaroon"); std::fs::write(&cert, b"").unwrap(); std::fs::write(&mac, [1u8, 2u8]).unwrap(); let client = fedimint_tonic_lnd::connect( "https://127.0.0.1:1".to_string(), cert.to_str().unwrap().to_string(), mac.to_str().unwrap().to_string(), ) .await .expect("lazy connect never dials"); + let _ = std::fs::remove_dir_all(&dir); LndConnector { client } }🤖 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 `@src/app/dev_fee.rs` around lines 1525 - 1540, Update the dead_lnd test helper to remove its UUID-named temporary directory immediately after fedimint_tonic_lnd::connect completes, before returning LndConnector. Preserve the connection result and ensure cleanup occurs after the eager certificate read, including when the connection attempt returns an error.
🤖 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.
Nitpick comments:
In `@src/app/dev_fee.rs`:
- Around line 1525-1540: Update the dead_lnd test helper to remove its
UUID-named temporary directory immediately after fedimint_tonic_lnd::connect
completes, before returning LndConnector. Preserve the connection result and
ensure cleanup occurs after the eager certificate read, including when the
connection attempt returns an error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7574f3d8-7157-4edc-b0cd-155b6c9fd47d
📒 Files selected for processing (4)
src/app/dev_fee.rssrc/app/release.rssrc/lnurl.rssrc/scheduler.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/scheduler.rs
- src/app/release.rs
- src/lnurl.rs
|
@grunch conflicts resolved (rebased onto |
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Actionable comments posted: 8
🗂️ Review context
Reviewed 16cf6e6..0af1019 against main (branch is up to date, 0 commits behind). Findings below were verified by execution, not inspection: cargo fmt --all -- --check, cargo clippy --all-targets --all-features -- -D warnings, cargo test lnurl:: dev_fee::, a live curl of the production dev-fee LNURL endpoint, and throwaway probes of build_callback_url against the pinned url 2.5.8.
Verdict
The design is sound and the incidental bug find is genuine — but this cannot merge as-is: CI will fail on cargo fmt, and the feature ships with zero observability and no test on the one string the PR exists to produce.
What's right
- The pre-existing bug is real and correctly diagnosed.
?is a legal character inside a query string (RFC 3986:query = *( pchar / "/" / "?" )), soformat!("{callback}?amount={amount_msat}")on a callback that already carried a query did causeamountto be absorbed into the preceding parameter's value. Good catch, good regression test, and correct to fix it here rather than leave it adjacent to new code. - Characters vs. bytes: you got this right. LUD-12 states "The value of
commentAllowedshould be the number of characters accepted for thecommentquery parameter."chars().take(max_len)matches the spec; a fair number of implementations count bytes and get this wrong. - Passing
&Keysinstead of re-deriving the EC key viaget_keys()on every cycle is a legitimate improvement, cleanly threaded. cargo clippy --all-targets --all-features -- -D warningsis clean. All 14lnurl::and 36dev_fee::tests pass.
Sizing note on the comment
The comment is 127 characters:
mostro-dev-fee order=58bb28ce-de34-4f3c-9bcc-c73bf1756e13 node=96f7d5278ba0892c6379c00fb085d8b5e876e0ca53dfc4125fe8197582800b33
The production recipient currently accommodates it:
$ curl -s https://walletofsatoshi.com/.well-known/lnurlp/pivotaldeborah52
{"callback":"https://livingroomofsatoshi.com/api/v1/lnurl/payreq/...","commentAllowed":255,...}So this works today. The concern raised inline is the failure behaviour when it ever stops fitting, which is currently silent and produces a truncated-but-plausible-looking pubkey.
⚠️ Repository-level finding (outside this PR)
.github/workflows/ci.yml triggers on on: push only — no pull_request. The fmt, clippy and test jobs therefore ran on the contributor's fork and are not reported as checks on this PR (gh pr checks 820 shows only mint-integration, mutation-baseline, mutation-pr — all skipped — plus CodeRabbit). That is exactly why the formatting failure below reached review unnoticed. Worth a follow-up issue: on: [push, pull_request].
| .await | ||
| .unwrap(); | ||
| assert!(resolve_dev_fee_invoice(&order).await.is_err()); | ||
| assert!(resolve_dev_fee_invoice(&order, &Keys::generate()).await.is_err()); |
There was a problem hiding this comment.
cargo fmt was not run — the fmt CI job fails on this line.
The PR description lists $ cargo fmt in the test plan, but the tree is not rustfmt-clean:
$ cargo fmt --all -- --check
Diff in src/app/dev_fee.rs:1974:
- assert!(resolve_dev_fee_invoice(&order, &Keys::generate()).await.is_err());
+ assert!(resolve_dev_fee_invoice(&order, &Keys::generate())
+ .await
+ .is_err());.github/workflows/ci.yml:16 runs cargo fmt --all -- --check, and the test job declares needs: [fmt, clippy] — so this blocks the whole pipeline. This is the only formatting violation in the diff.
| assert!(resolve_dev_fee_invoice(&order, &Keys::generate()).await.is_err()); | |
| assert!(resolve_dev_fee_invoice(&order, &Keys::generate()) | |
| .await | |
| .is_err()); |
| /// LUD-12: returns the comment truncated to `max_len` chars, or `None` if | ||
| /// there's no comment to send or the server advertises no support for one | ||
| /// (`max_len == 0`). | ||
| fn fit_comment(comment: Option<&str>, max_len: usize) -> Option<String> { | ||
| let comment = comment.filter(|_| max_len > 0)?; | ||
| Some(comment.chars().take(max_len).collect()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Silent truncation produces a plausible-looking but wrong identifier — defeating the purpose of the feature.
The comment is 127 chars. If commentAllowed is ever below that (a different dev-fee address, or the provider lowering its limit), chars().take(max_len) cuts through the middle of the node pubkey and emits something that still looks like a valid hex key. Whoever reconciles payments on the receiving end has no way to tell a complete identifier from a mutilated one, and nothing is logged.
For a feature whose entire purpose is traceability, a partial trace is worse than none: omit rather than truncate.
| /// LUD-12: returns the comment truncated to `max_len` chars, or `None` if | |
| /// there's no comment to send or the server advertises no support for one | |
| /// (`max_len == 0`). | |
| fn fit_comment(comment: Option<&str>, max_len: usize) -> Option<String> { | |
| let comment = comment.filter(|_| max_len > 0)?; | |
| Some(comment.chars().take(max_len).collect()) | |
| } | |
| /// LUD-12: returns the comment when it fits within `max_len` chars, or | |
| /// `None` if there's no comment to send, the server advertises no support | |
| /// for one (`max_len == 0`), or the comment would have to be truncated — | |
| /// a half-written order id or pubkey is a worse trace than no trace. | |
| fn fit_comment(comment: Option<&str>, max_len: usize) -> Option<String> { | |
| let comment = comment.filter(|c| max_len > 0 && c.chars().count() <= max_len)?; | |
| Some(comment.to_string()) | |
| } |
Note this inverts fit_comment_truncates_to_server_limit (line 285), which would become fit_comment_none_when_too_long. If you prefer to keep truncating, then please at least warn! when it happens — the current behaviour is undetectable in production either way.
| let mut url = reqwest::Url::parse(callback) | ||
| .map_err(|_| MostroInternalErr(ServiceError::LnAddressParseError))?; |
There was a problem hiding this comment.
🛡️ Security | 🟠 Major
No scheme validation on an attacker-controlled URL.
callback comes straight out of a remote LNURL server's JSON. Verified against the pinned url 2.5.8 — any scheme parses and gets the query appended:
build_callback_url("ftp://h/cb", ...) => Ok("ftp://h/cb?amount=100000")
build_callback_url("javascript:alert(1)", ...) => Ok("javascript:alert(1)?amount=100000")
This matters because the address is not always trusted: in src/app/release.rs:499, do_payment feeds resolv_ln_address a buyer-supplied lightning address (order.buyer_invoice). A hostile buyer can stand up their own LNURL server and return callback: "http://127.0.0.1:8332/..." or http://169.254.169.254/latest/meta-data/, and mostrod will issue the GET. Only pr is read back, so it's blind SSRF — but internal port scanning and GET-triggered side effects are real.
This weakness pre-dates the PR. Flagging it because build_callback_url is the choke point this PR introduces, and is now the only place in the codebase that parses this value — the check is four lines and belongs here:
| let mut url = reqwest::Url::parse(callback) | |
| .map_err(|_| MostroInternalErr(ServiceError::LnAddressParseError))?; | |
| let mut url = reqwest::Url::parse(callback) | |
| .map_err(|_| MostroInternalErr(ServiceError::LnAddressParseError))?; | |
| if !matches!(url.scheme(), "http" | "https") { | |
| return Err(MostroInternalErr(ServiceError::LnAddressParseError)); | |
| } |
Blocking private/link-local ranges is a larger change and better tracked as a separate issue.
| url.query_pairs_mut() | ||
| .append_pair("amount", &amount_msat.to_string()); |
There was a problem hiding this comment.
🧹 Nitpick | 🟡 Minor
A callback that already carries amount yields a duplicate parameter.
Verified: build_callback_url("https://h/cb?amount=5", 100_000, ...) produces https://h/cb?amount=5&amount=100000. Which one the server honours is undefined.
Not exploitable for overpayment — src/lightning/mod.rs:269-278 compares the decoded invoice amount against the requested one and aborts with "Wrong amount". But the result is another opaque, permanently-retrying dev-fee failure (see the comment on line 141). Consider dropping any pre-existing amount/comment keys before appending, so the value you compute is the one that's sent.
| } | ||
| let callback = body["callback"].as_str().unwrap_or(""); | ||
| let callback = format!("{callback}?amount={amount_msat}"); | ||
| let comment_allowed = body["commentAllowed"].as_u64().unwrap_or(0) as usize; |
There was a problem hiding this comment.
New failure mode lands in a path that discards the server's error reason, and then retries forever.
Two problems compound here.
(a) The reason is thrown away. LNURL errors are returned as HTTP 200 with {"status":"ERROR","reason":"..."}. That body has no pr, so body["pr"].as_str().unwrap_or("") (line 157) yields "" and the function returns Ok(""). src/app/dev_fee.rs:934 then logs the generic "returned empty invoice" and the server's reason is never surfaced. This PR introduces a brand-new class of rejection into that path — comment too long, comment unsupported, comment rejected — all of which will now be invisible.
Related: as_u64() returns None if a server encodes commentAllowed as 255.0 or as a string, silently collapsing to 0 and dropping the comment with no trace.
(b) It retries forever with identical input. On resolution failure, src/app/dev_fee.rs:450 calls release_pending_claim(...) and continues, so the next cycle retries — with the same comment — producing a permanent failure loop and no diagnostic pointing at the comment.
Minimum fix, surfacing the reason:
if body["status"].as_str() == Some("ERROR") {
let reason = body["reason"].as_str().unwrap_or("unknown");
error!("LNURL callback rejected: {reason}");
}Worth considering as well: retry once without the comment before giving up. The dev fee is real money and shouldn't be blocked indefinitely by an optional field.
| // LUD-12 comment so the receiving end can trace which order/node a dev | ||
| // fee payment came from. | ||
| let comment = format!( | ||
| "mostro-dev-fee order={} node={}", | ||
| order.id, | ||
| keys.public_key() | ||
| ); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🟡 Minor
Three small things on the comment string itself:
-
The PR description doesn't match the code. The summary says the comment is
order=<id> node=<pubkey>; the code emitsmostro-dev-fee order=… node=…. The prefix is useful — keep it and update the description. -
keys.public_key()renders hex, not npub. Confirmed output:node=96f7d5278ba0892c6379c00fb085d8b5e876e0ca53dfc4125fe8197582800b33. In a nostr project, an operator-facing trace identifier reads better as a self-describingnpub1…; consider.to_bech32(). (It also shaves 63 chars off the 127-char total.) -
No opt-out. Every operator's node pubkey and order ids are now unconditionally disclosed to a third-party custodial provider. Both are already public on relays, so the marginal exposure is correlation rather than secrets — but it's a choice some operators will want to make. A settings toggle would be reasonable.
| #[test] | ||
| fn fit_comment_truncates_to_server_limit() { | ||
| assert_eq!( | ||
| fit_comment(Some("order=1 node=abc"), 7), | ||
| Some("order=1".to_string()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧪 Test coverage | 🟡 Minor
The string this PR exists to produce is never asserted.
All eight new tests exercise the generic helpers (fit_comment, build_callback_url). Nothing asserts that resolve_dev_fee_invoice builds mostro-dev-fee order=… node=…, and nothing covers the wiring from resolv_ln_address(comment) through to the comment pair on the callback URL.
Concretely: you could delete the format! at src/app/dev_fee.rs:903-908 entirely and all eight tests would still pass.
Extracting the format into a pure function makes it testable without a live LNURL server:
fn dev_fee_comment(order_id: &Uuid, node: &PublicKey) -> String {
format!("mostro-dev-fee order={order_id} node={node}")
}Then assert both the shape and that it stays within a sane budget — the 127-char figure is what makes issue #694's traceability claim hold.
- reject non-http(s) URL schemes in extract_lnurl (covers every outbound GET: ln_exists, both resolv_ln_address requests, and the callback), closing an SSRF vector reachable via buyer-supplied lightning addresses - warn when a comment is dropped for exceeding the server's commentAllowed instead of silently truncating or silently disappearing - drop pre-existing amount/comment query params before appending ours - surface the LNURL server's rejection reason on both requests, not just the callback - render the node identifier as npub instead of hex - extract dev_fee_comment and is_http_or_https into pure, tested functions, the latter shared with the existing Cashu mint_url scheme check - run cargo fmt (was failing CI)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lnurl.rs (1)
144-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the expanded public resolver API.
Add
///docs covering thatamountis in sats, the optional LUD-12 comment, and its result/error behavior.As per coding guidelines, “Document non-obvious public Rust APIs with
///documentation comments.”🤖 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 `@src/lnurl.rs` around lines 144 - 148, Add Rust /// documentation to the public resolv_ln_address function describing that amount is denominated in satoshis, comment is an optional LUD-12 comment, and the function returns the resolved address on success or MostroError on failure.Source: Coding guidelines
🤖 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 `@src/lnurl.rs`:
- Around line 168-172: Stop logging raw LNURL rejection reasons in both
rejection branches at src/lnurl.rs:168-172 and src/lnurl.rs:199-203. Replace the
reason text with a fixed rejection message and safe metadata such as its length,
while preserving the existing return behavior.
---
Nitpick comments:
In `@src/lnurl.rs`:
- Around line 144-148: Add Rust /// documentation to the public
resolv_ln_address function describing that amount is denominated in satoshis,
comment is an optional LUD-12 comment, and the function returns the resolved
address on success or MostroError on failure.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f6c1c59-71e8-4b37-94eb-53c92341d234
📒 Files selected for processing (4)
src/app/dev_fee.rssrc/config/util.rssrc/lnurl.rssrc/util.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/dev_fee.rs
…ess docs CodeRabbit flagged that error!() logged the untrusted 'reason' field from external LNURL servers verbatim — a log-injection vector on the same buyer-supplied-address path already flagged as SSRF-sensitive. Log the reason's byte length instead. Also adds the missing /// docs on resolv_ln_address per the nitpick.
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Re-checked every prior review comment against current head 57f1172c332b3831040d1a1042ce4383366f8b8e.
All blocking and substantive PR issues are fixed: the tree is rustfmt-clean; oversized comments are omitted with a warning rather than truncated; initial and callback URLs enforce HTTP(S); pre-existing amount/comment parameters are replaced; LNURL rejections are explicitly logged without exposing the untrusted raw reason; the trace uses npub; the exact comment shape is tested; and the public resolver documentation/import cleanup are present.
Two non-blocking suggestions remain intentionally out of scope: retrying once without the optional comment / adding an operator opt-out. CodeRabbit's temporary test-directory cleanup nit is also still present, but that helper is unchanged from the PR base and is not a regression in this PR.
Exact-head local verification with Rust 1.94.0:
cargo fmt --all -- --checkcargo test lnurl::— 17 passedcargo test dev_fee::— 37 passedcargo clippy --all-targets --all-features -- -D warnings
No blocking issues remain. Approved.
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
Approving. Reviewed locally on the branch — build, lints and the full suite, plus a couple of things I verified by running rather than reading.
Verification
| Check | Result |
|---|---|
cargo fmt --all -- --check |
clean |
cargo clippy --all-targets --all-features -- -D warnings |
clean |
cargo test --bins |
1032 passed, 0 failed |
The failure noted in the PR description (lightning::invoice::tests::test_lnurl_validation_with_test_server) passes here — it was the fixed port 8080 being occupied on your machine, not the code. The suite is fully green. (Filed as part of #839: that harness should bind port 0 instead.)
What I checked beyond reading the diff
The ?amount= bug and its fix are real. Ran build_callback_url against the url version pinned in this repo:
https://pay.example.com/cb?id=abc&amount=100000&comment=mostro-dev-fee+order%3D550e8400-...+node%3Dnpub1abc
amount lands as its own pair, and build_callback_url_preserves_existing_query_params covers exactly the case that used to break. Good catch, and right call fixing it here instead of leaving it next to new code.
The feature works against the real endpoint. Queried the live dev-fee address (walletofsatoshi.com/.well-known/lnurlp/pivotaldeborah52): it advertises commentAllowed: 255, and the comment is ~126 chars, so it fits with room to spare. No silent degradation in production.
One accuracy note for the record: that endpoint's callback is https://livingroomofsatoshi.com/api/v1/lnurl/payreq/<uuid>, with no query string — so the ?amount= bug never affected dev-fee payments. Where it bit is the buyer payout path (do_payment, with buyer-supplied addresses such as LNbits-style callbacks). The fix is valuable, just on a different code path than the description implies.
Findings, all filed as follow-ups
Nothing blocking. The design is sound, the extracted pure functions are well tested, and the scheme allow-list closes a real vector.
- #837 — MEDIUM, host-based SSRF. This is the "known gap, tracked separately" your
build_callback_urldoc comment promises; there was no issue behind it, so now there is. I confirmed it is reachable from user input:user@169.254.169.254anduser@localhostboth passLightningAddress::from_strand produce an outbound GET. Pre-existing and explicitly out of scope for this PR. - #838 — LOW,
reason length: N bytesisn't actionable for an operator. CodeRabbit's log-injection concern was right, but sanitize-and-truncate is the middle ground rather than discarding the reason entirely. - #839 — LOW, the two new
status: "ERROR"branches and thecommentAllowedwiring have no test coverage. The pure helpers are well covered; it's the plumbing insideresolv_ln_addressthat isn't. - #840 — LOW, minor cleanups: the comment-fit rule is duplicated between
fit_commentand thewarn!branch, the warning can repeat per scheduler tick, and spaces are form-urlencoded as+. - Commented on #814 rather than opening a duplicate — it already covers
resolv_ln_addressreturningOk(""), and this PR adds two more paths that reach it. Added the concrete detail thatdo_payment(src/app/release.rs:499) never guards against the empty string, whileresolve_dev_fee_invoicedoes.
One thing to fix before merge
The PR description still says the LNURL error reason is logged — "log the server's actual reason instead of silently returning an empty invoice". Commit 57f1172 changed that to logging only the byte length. Worth updating the body so it matches the code that ships.
Nice work on this one — the incidental bug find and the regression test around it are the best part of the PR.
Merged — technical summaryMerged by @grunch in Problem. The dev-fee LNURL-pay callback only sent Solution. LUD-12 comment support: Review round (2026-07-21, 8 comments, "cannot merge as-is") — all addressed in
Proactive follow-up ( Final: 4 commits, 6 files, +274/-16. Closes #694. |
3 participants
Summary
Closes #694.
Dev fee payments carried no identifying information, making it impossible to trace which order or Mostro node originated a given payment on the receiving end. This adds an optional LUD-12
comment(mostro-dev-fee order=<id> node=<npub>) to the LNURL-pay callback, gated by the server-advertisedcommentAllowedlimit — servers that don't support it are left untouched.Bug found and fixed while implementing this
The existing callback-building code concatenated
?amount={amount_msat}onto the rawcallbackstring returned by the LNURL server, before parsing it as a URL:Real LNURL servers can return a
callbackthat already carries its own query string (e.g.https://host/cb?id=abc, common with LNbits and others). In that case the second?is not a delimiter — everything after it, including the literal?, becomes part of the previous parameter's value. Soamountsilently never reaches the server as its own parameter; the previous key absorbs it instead (verified by actually running this against the exacturlcrate version pinned in this repo). This predates this PR, but since the same line was being rewritten to attach the comment, it's fixed here too rather than left next to new code. Extractedbuild_callback_url, a pure helper that addsamountandcommentviaquery_pairs_mut(never by string concatenation), with a regression test (build_callback_url_preserves_existing_query_params) covering exactly this case.Other changes
resolve_dev_fee_invoicenow receiveskeys: &Keysinstead of callingcrate::util::get_keys()internally (which re-parses the nsec — an EC key derivation — on every call).AppContext::keys()already caches this and is documented as the preferred alternative (src/app/context.rs). Threaded throughscheduler.rs -> run_dev_fee_cycle -> process_new_dev_fee_payments -> resolve_dev_fee_invoice;ctxwas already available at the scheduler spawn site, just not passed down. This also removes the only fallible path around building the comment, so there's no error to silently swallow.Response to review
@grunch's review caught real gaps — all addressed in
860bcac:cargo fmtfailure (Critical) — tree is now rustfmt-clean;cargo fmt --all -- --checkpasses.fit_commentnow returnsNoneinstead of cutting a comment mid-pubkey when it exceedscommentAllowed; awarn!fires so a dropped comment is visible in logs instead of silently producing a mutilated-but-plausible identifier.extract_lnurlandbuild_callback_urlnow reject any scheme other thanhttp/https, closing the path a buyer-supplied lightning address could use to point mostrod atjavascript:/file:/internal addresses.amount/commentparams (Minor) — any pre-existingamount/commentpair oncallbackis dropped before appending ours, so there's no ambiguity about which value the server honors.resolv_ln_addressrequests (address resolution and callback) now check for{"status":"ERROR","reason":...}and log the server's actual reason instead of silently returning an empty invoice.npub1...viato_bech32(), both more idiomatic for a nostr project and 63 chars shorter.dev_fee_commentinto its own pure function with a dedicated test asserting the exactmostro-dev-fee order=... node=...shape, so the format can't silently drift again.use super::Keys(Trivial) — test module now importsKeysdirectly fromnostr_sdk.Left open, both flagged as optional in the review rather than blocking: retrying once without the comment on LNURL rejection, and a settings toggle to opt out of sending the comment entirely. Happy to file follow-ups if useful.
Test plan
The 1 failure (
lightning::invoice::tests::test_lnurl_validation_with_test_server) is pre-existing and unrelated — it binds a mock server to a fixed port (8080) that's already occupied in my environment; confirmed neither this commit nor any other commit on this branch touchessrc/lightning/invoice.rs.New/changed test coverage (
src/lnurl.rsandsrc/app/dev_fee.rs):amountadded as its own query parameter (base case)amountis added as its own pair instead of merging into the existing onecommentattached when the server allows it (commentAllowed > 0)commentomitted when the server doesn't advertise supportcommentomitted (not truncated) when it exceeds the server-advertised lengthextract_lnurl/build_callback_urlreject non-http(s)schemesamount/commentquery params are dropped before appending oursdev_fee_commentproduces the exactmostro-dev-fee order=... node=npub1...shapeSummary by CodeRabbit