From 5f6642a6ff6679503d3c0cbbdef4a791335bd6af Mon Sep 17 00:00:00 2001 From: Vincenzo Palazzo Date: Tue, 13 Jun 2023 12:33:31 +0200 Subject: [PATCH] fix(jsonrpc): trim the lightning: prefix from invoice Previously, our code checked for the presence of the `lightning:` prefix while decoding a bolt11 string. Although this prefix is valid and accepted by the core lightning pay command, it was causing issues with how we managed invoices. Specifically, we were skipping the prefix when creating a copy of the invoice string and storing the raw invoice (including the prefix) in the database, which caused inconsistencies in the user experience. To address this issue, we need to strip the `lightning:` prefix before calling each core lightning command. In addition, we should modify the invstring inside the db with the canonical one. This commit fixes the issue by stripping the `lightning:` prefix from the `listsendpays` function, which will improve the user experience and ensure consistency in our invoice management (see next commit). Reported-by: @johngribbin Link: ElementsProject#6207 Fixes: debbdc0 Changelog-Fixes: trim the `lightning:` prefix from invoice everywhere. Signed-off-by: Vincenzo Palazzo --- common/bolt11.c | 28 ++++++++++----- common/bolt11.h | 6 ++++ common/json_param.c | 9 +++++ common/json_param.h | 6 ++++ common/test/run-json_filter.c | 3 ++ common/test/run-json_remove.c | 3 ++ common/test/run-param.c | 3 ++ common/utils.c | 10 ++++++ common/utils.h | 9 +++++ lightningd/invoice.c | 8 ++--- lightningd/pay.c | 2 +- lightningd/test/run-invoice-select-inchan.c | 5 +++ plugins/pay.c | 6 ++-- tests/test_pay.py | 40 +++++++++++++++++++++ 14 files changed, 122 insertions(+), 16 deletions(-) diff --git a/common/bolt11.c b/common/bolt11.c index 07134f186..e608474de 100644 --- a/common/bolt11.c +++ b/common/bolt11.c @@ -1,4 +1,5 @@ #include "config.h" +#include #include #include #include @@ -678,6 +679,24 @@ static bool bech32_decode_alloc(const tal_t *ctx, return true; } +static bool has_lightning_prefix(const char *invstring) +{ + /* BOLT #11: + * + * If a URI scheme is desired, the current recommendation is to either + * use 'lightning:' as a prefix before the BOLT-11 encoding */ + return (strstarts(invstring, "lightning:") || + strstarts(invstring, "LIGHTNING:")); +} + +const char *to_canonical_invstr(const tal_t *ctx, + const char *invstring) +{ + if (has_lightning_prefix(invstring)) + invstring += strlen("lightning:"); + return str_lowering(ctx, invstring); +} + /* Extracts signature but does not check it. */ struct bolt11 *bolt11_decode_nosig(const tal_t *ctx, const char *str, const struct feature_set *our_features, @@ -701,14 +720,7 @@ struct bolt11 *bolt11_decode_nosig(const tal_t *ctx, const char *str, memset(have_field, 0, sizeof(have_field)); b11->routes = tal_arr(b11, struct route_info *, 0); - /* BOLT #11: - * - * If a URI scheme is desired, the current recommendation is to either - * use 'lightning:' as a prefix before the BOLT-11 encoding - */ - if (strstarts(str, "lightning:") || strstarts(str, "LIGHTNING:")) - str += strlen("lightning:"); - + assert(!has_lightning_prefix(str)); if (strlen(str) < 8) return decode_fail(b11, fail, "Bad bech32 string"); diff --git a/common/bolt11.h b/common/bolt11.h index ebcf99192..fa403f1ae 100644 --- a/common/bolt11.h +++ b/common/bolt11.h @@ -125,6 +125,12 @@ char *bolt11_encode_(const tal_t *ctx, secp256k1_ecdsa_recoverable_signature *rsig), \ (arg)) +/** to_canonical_invstr - return a canonical string where the following constrains are follow: + * - There is no `lightning:` prefix; + * - all the string is in lower case. + */ +const char *to_canonical_invstr(const tal_t *ctx, const char *invstring); + #if DEVELOPER /* Flag for tests to suppress `min_final_cltv_expiry` field generation, to match test vectors */ extern bool dev_bolt11_no_c_generation; diff --git a/common/json_param.c b/common/json_param.c index f53b63eb0..e346e9594 100644 --- a/common/json_param.c +++ b/common/json_param.c @@ -440,6 +440,15 @@ struct command_result *param_string(struct command *cmd, const char *name, return NULL; } +struct command_result *param_invstring(struct command *cmd, const char *name, + const char * buffer, const jsmntok_t *tok, + const char **str) +{ + const char *strtmp = json_strdup(cmd, buffer, tok); + *str = to_canonical_invstr(cmd, strtmp); + return NULL; +} + struct command_result *param_ignore(struct command *cmd, const char *name, const char *buffer, const jsmntok_t *tok, const void *unused) diff --git a/common/json_param.h b/common/json_param.h index 19c924b66..a81aa7c63 100644 --- a/common/json_param.h +++ b/common/json_param.h @@ -181,6 +181,12 @@ struct command_result *param_string(struct command *cmd, const char *name, const char * buffer, const jsmntok_t *tok, const char **str); +/* Extract an invoice string from a generic string, strip the `lightning:` + * prefix from it if needed. */ +struct command_result *param_invstring(struct command *cmd, const char *name, + const char * buffer, const jsmntok_t *tok, + const char **str); + /* Extract a label. It is either an escaped string or a number. */ struct command_result *param_label(struct command *cmd, const char *name, const char * buffer, const jsmntok_t *tok, diff --git a/common/test/run-json_filter.c b/common/test/run-json_filter.c index 44c772a2c..ff8978f6b 100644 --- a/common/test/run-json_filter.c +++ b/common/test/run-json_filter.c @@ -152,6 +152,9 @@ void towire_u8(u8 **pptr UNNEEDED, u8 v UNNEEDED) /* Generated stub for towire_u8_array */ void towire_u8_array(u8 **pptr UNNEEDED, const u8 *arr UNNEEDED, size_t num UNNEEDED) { fprintf(stderr, "towire_u8_array called!\n"); abort(); } +/* Generated stub for strip_lightning_prefix */ +const char *to_canonical_invstr(const tal_t *ctx, const char *invstring UNNEEDED) +{ fprintf(stderr, "strip_lightning_prefix called!\n"); abort(); } /* AUTOGENERATED MOCKS END */ bool deprecated_apis; diff --git a/common/test/run-json_remove.c b/common/test/run-json_remove.c index e8d6b842f..35b0f9858 100644 --- a/common/test/run-json_remove.c +++ b/common/test/run-json_remove.c @@ -187,6 +187,9 @@ void towire_u8(u8 **pptr UNNEEDED, u8 v UNNEEDED) /* Generated stub for towire_u8_array */ void towire_u8_array(u8 **pptr UNNEEDED, const u8 *arr UNNEEDED, size_t num UNNEEDED) { fprintf(stderr, "towire_u8_array called!\n"); abort(); } +/* Generated stub for strip_lightning_prefix */ +const char *to_canonical_invstr(const tal_t *ctx, const char *invstring UNNEEDED) +{ fprintf(stderr, "strip_lightning_prefix called!\n"); abort(); } /* AUTOGENERATED MOCKS END */ struct json { diff --git a/common/test/run-param.c b/common/test/run-param.c index fffc19a16..f7fb5fc93 100644 --- a/common/test/run-param.c +++ b/common/test/run-param.c @@ -40,6 +40,9 @@ struct command_result *command_fail(struct command *cmd, /* Generated stub for command_filter_ptr */ struct json_filter **command_filter_ptr(struct command *cmd UNNEEDED) { fprintf(stderr, "command_filter_ptr called!\n"); abort(); } +/* Generated stub for strip_lightning_prefix */ +const char *to_canonical_invstr(const tal_t *ctx, const char *invstring UNNEEDED) +{ fprintf(stderr, "strip_lightning_prefix called!\n"); abort(); } /* Generated stub for fromwire_tlv */ bool fromwire_tlv(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, const struct tlv_record_type *types UNNEEDED, size_t num_types UNNEEDED, diff --git a/common/utils.c b/common/utils.c index c264b6924..165a1d556 100644 --- a/common/utils.c +++ b/common/utils.c @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -160,3 +161,12 @@ int tmpdir_mkstemp(const tal_t *ctx, const char *template TAKES, char **created) return fd; } + +char *str_lowering(const void *ctx, const char *string TAKES) +{ + char *ret; + + ret = tal_strdup(ctx, string); + for (char *p = ret; *p; p++) *p = tolower(*p); + return ret; +} diff --git a/common/utils.h b/common/utils.h index 8f5cc4cb3..e1793bdea 100644 --- a/common/utils.h +++ b/common/utils.h @@ -146,4 +146,13 @@ extern const tal_t *wally_tal_ctx; * Returns created temporary path name at *created if successful. */ int tmpdir_mkstemp(const tal_t *ctx, const char *template TAKES, char **created); +/** + * tal_strlowering - return the same string by in lower case. + * @ctx: the context to tal from (often NULL) + * @string: the string that is going to be lowered (can be take()) + * + * FIXME: move this in ccan + */ +char *str_lowering(const void *ctx, const char *string TAKES); + #endif /* LIGHTNING_COMMON_UTILS_H */ diff --git a/lightningd/invoice.c b/lightningd/invoice.c index cbe3e9102..360ab13a7 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -1268,7 +1268,7 @@ static struct command_result *json_listinvoices(struct command *cmd, if (!param(cmd, buffer, params, p_opt("label", param_label, &label), - p_opt("invstring", param_string, &invstring), + p_opt("invstring", param_invstring, &invstring), p_opt("payment_hash", param_sha256, &payment_hash), p_opt("offer_id", param_sha256, &offer_id), NULL)) @@ -1529,7 +1529,7 @@ static struct command_result *json_decodepay(struct command *cmd, char *fail; if (!param(cmd, buffer, params, - p_req("bolt11", param_string, &str), + p_req("bolt11", param_invstring, &str), p_opt("description", param_escaped_string, &desc), NULL)) return command_param_failed(); @@ -1650,7 +1650,7 @@ static struct command_result *json_createinvoice(struct command *cmd, char *fail; if (!param(cmd, buffer, params, - p_req("invstring", param_string, &invstring), + p_req("invstring", param_invstring, &invstring), p_req("label", param_label, &label), p_req("preimage", param_preimage, &preimage), NULL)) @@ -1900,7 +1900,7 @@ static struct command_result *json_signinvoice(struct command *cmd, char *fail; if (!param(cmd, buffer, params, - p_req("invstring", param_string, &invstring), + p_req("invstring", param_invstring, &invstring), NULL)) return command_param_failed(); diff --git a/lightningd/pay.c b/lightningd/pay.c index 50c2f9110..26cf711fc 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -1415,7 +1415,7 @@ static struct command_result *json_sendpay(struct command *cmd, p_opt("label", param_escaped_string, &label), p_opt("amount_msat|msatoshi", param_msat, &msat), /* FIXME: parameter should be invstring now */ - p_opt("bolt11", param_string, &invstring), + p_opt("bolt11", param_invstring, &invstring), p_opt("payment_secret", param_secret, &payment_secret), p_opt_def("partid", param_u64, &partid, 0), p_opt("localinvreqid", param_sha256, &local_invreq_id), diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index ef9a6595f..c0aacf6f2 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -13,6 +13,11 @@ struct channel *any_channel_by_scid(struct lightningd *ld UNNEEDED, const struct short_channel_id *scid UNNEEDED, bool privacy_leak_ok UNNEEDED) { fprintf(stderr, "any_channel_by_scid called!\n"); abort(); } +/* Generated stub for param_invstring */ +struct command_result *param_invstring(struct command *cmd, const char *name, + const char * buffer, const jsmntok_t *tok, + const char **str) +{ fprintf(stderr, "param_invstring called!\n"); abort(); } /* Generated stub for bip32_pubkey */ void bip32_pubkey(struct lightningd *ld UNNEEDED, struct pubkey *pubkey UNNEEDED, u32 index UNNEEDED) { fprintf(stderr, "bip32_pubkey called!\n"); abort(); } diff --git a/plugins/pay.c b/plugins/pay.c index 008114684..a667f1bd0 100644 --- a/plugins/pay.c +++ b/plugins/pay.c @@ -198,7 +198,7 @@ static struct command_result *json_paystatus(struct command *cmd, if (!param(cmd, buf, params, /* FIXME: rename to invstring */ - p_opt("bolt11", param_string, &invstring), + p_opt("bolt11", param_invstring, &invstring), NULL)) return command_param_failed(); @@ -547,7 +547,7 @@ static struct command_result *json_listpays(struct command *cmd, /* FIXME: would be nice to parse as a bolt11 so check worked in future */ if (!param(cmd, buf, params, /* FIXME: parameter should be invstring now */ - p_opt("bolt11", param_string, &invstring), + p_opt("bolt11", param_invstring, &invstring), p_opt("payment_hash", param_sha256, &payment_hash), p_opt("status", param_string, &status_str), NULL)) @@ -994,7 +994,7 @@ static struct command_result *json_pay(struct command *cmd, * initialized directly that way. */ if (!param(cmd, buf, params, /* FIXME: parameter should be invstring now */ - p_req("bolt11", param_string, &b11str), + p_req("bolt11", param_invstring, &b11str), p_opt("amount_msat|msatoshi", param_msat, &msat), p_opt("label", param_string, &label), p_opt_def("riskfactor", param_millionths, diff --git a/tests/test_pay.py b/tests/test_pay.py index 3f7f8bc65..7b1e1b377 100644 --- a/tests/test_pay.py +++ b/tests/test_pay.py @@ -5418,3 +5418,43 @@ def test_invoice_pay_desc_with_quotes(node_factory): # pay an invoice l1.rpc.pay(invoice, description=description) + + +def test_strip_lightning_suffix_from_inv(node_factory): + """ + Reproducer for [1] that pay an invoice with the `lightning:` + prefix and then, will check if core lightning is able to strip it during + list `listpays` command. + + [1] https://github.com/ElementsProject/lightning/issues/6207 + """ + l1, l2 = node_factory.line_graph(2) + inv = l2.rpc.invoice(40, "strip-lightning-prefix", "test to be able to strip the `lightning:` prefix.")["bolt11"] + wait_for(lambda: only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['state'] == 'CHANNELD_NORMAL') + + # Testing the prefix stripping case + l1.rpc.pay(f"lightning:{inv}") + listpays = l1.rpc.listpays()["pays"] + assert len(listpays) == 1, f"the list pays is bigger than what we expected {listpays}" + # we can access by index here because the payment are sorted by db idx + assert listpays[0]['bolt11'] == inv, f"list pays contains a different invoice, expected is {inv} but we get {listpays[0]['bolt11']}" + + # Testing the case of the invoice is upper case + inv = l2.rpc.invoice(40, "strip-lightning-prefix-upper-case", "test to be able to strip the `lightning:` prefix with an upper case invoice.")["bolt11"] + wait_for(lambda: only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['state'] == 'CHANNELD_NORMAL') + + # Testing the prefix stripping with an invoice in upper case case + l1.rpc.pay(f"lightning:{inv.upper()}") + listpays = l1.rpc.listpays()["pays"] + assert len(listpays) == 2, f"the list pays is bigger than what we expected {listpays}" + assert listpays[1]['bolt11'] == inv, f"list pays contains a different invoice, expected is {inv} but we get {listpays[0]['bolt11']}" + + # Testing the string lowering of an invoice in upper case + # Testing the case of the invoice is upper case + inv = l2.rpc.invoice(40, "strip-lightning-upper-case", "test to be able to lower the invoice string.")["bolt11"] + wait_for(lambda: only_one(l1.rpc.listpeerchannels(l2.info['id'])['channels'])['state'] == 'CHANNELD_NORMAL') + + l1.rpc.pay(inv.upper()) + listpays = l1.rpc.listpays()["pays"] + assert len(listpays) == 3, f"the list pays is bigger than what we expected {listpays}" + assert listpays[2]['bolt11'] == inv, f"list pays contains a different invoice, expected is {inv} but we get {listpays[0]['bolt11']}"