fix: check description_hash only if defined

[jsarenik]

Link this PR to an issue

Fixes #726

Type of change

• Bug fix (non-breaking change which fixes an issue)

Describe the changes you have made in this PR -

Check if the description_hash field is defined before checking it's content equals to the manually assembled one.

How Has This Been Tested?

See #726. It would be repetitive to write it here again.

Checklist:

• My code follows the style guidelines of this project and performed a self-review of my own code
• New and existing tests pass locally with my changes
• I checked if I need to make corresponding changes to the documentation (and made those changes if needed)

[escapedcat]

Does this work as well?

case paymentRequestDetails?.description_hash !== metadataHash:

[jsarenik]

It works when description_hash is set. See #726 — all LND-made I saw invoices contain description_hash and work well with Alby even without this change.

[bumi]

I think the LNURL spec defines that the description_hash MUST be defined and payments must be rejected if false. Or am I wrong there?

[jsarenik]

@bumi description_hash is optional and correctly defined as such in https://github.com/getAlby/lightning-browser-extension/blob/master/src/declarations.d.ts#L6

[bumi]

It's optional in the payment request but I think the LNURL spec requires it: https://github.com/fiatjaf/lnurl-rfc/blob/legacy/lnurl-pay.md see 5. (edit: new rfc link: https://github.com/fiatjaf/lnurl-rfc/blob/luds/06.md#lud-06-payrequest-base-spec)

the wallet will call the service and get a Lightning invoice specific for that payment, containing a hash of the metadata as its h tag (description_hash) and proceed to pay the invoice if it matches the expected amount and hash.

[jsarenik]

@fiatjaf Please comment.

[jsarenik]

@bumi Do you think this change in C-Lightning would help? ElementsProject/lightning#5128

[fiatjaf]

description_hash is necessary and must be enforced. Not checking for the description_hash is akin to not checking signatures, it's a vulnerability and a bug.

[jsarenik]

Closing this. I came to it totally unaware of ElementsProject/lightning#5121

Yes, it needs to be fixed in c-lightning. Thank you @bumi for your patience while I took time to understand what's going on here.

[bumi]

thanks for reporting, happy we found and learned something.