search

breez / breezmobile

Lightning Network mobile client
https://breez.technology
GNU General Public License v3.0
566 stars 129 forks source link

lnurl-pay handler asks for comment when no commentAllowed is present #532

Closed akovalenko closed 3 years ago

akovalenko commented 3 years ago

According to lnurl-pay proposal at https://github.com/fiatjaf/lnurl-rfc/blob/master/lnurl-pay.md, commentAllowed defaults to 0 if not provided, meaning "no comment allowed". As of today, Breez seems to ask for comment anyway, allowing up to 90 characters and faithfully including it when executing the lnurl-pay callback.

It's a common practice not to display comment field at all unless commentAllowed is present AND positive.

kingonly commented 3 years ago

@akovalenko thank you! We'll get it sorted out. Is there a live service not accepting comments?

nochiel commented 3 years ago

@akovalenko thank you! We'll get it sorted out. Is there a live service not accepting comments?

None of the services I tested used comments but they didn't reject the request if it had a comment field.

Correction: zbd.gg uses comments and saves them with the payment information that the user sees in their zbd wallet.

https://github.com/breez/breezmobile/pull/518 has a list of the services I tested with.

akovalenko commented 3 years ago

It's common to ignore unexpected parameters (or else the problem would be more severe indeed), but we shouldn't count on it — there might be a service using comment= in callback for its own purposes after all.

However it's not good that the user is asked for a comment, believing it will go somewhere besides the web server log, when it's really ignored by the service.

Re: testing: I'm running lnurl-pay.me, maybe you could add it (though you'd need either a valid destination, or a non-prevalidating direction like "Skype top up" to test fixed-fiat-amount variants). It's the simplest thing without comments, successActions etc., but might be useful to check e.g. emoji in custom description (metadata).

kingonly commented 3 years ago

@nochiel we should honor the spec. Disable the comment field if commentAllowed is 0 or not present, if it >0 to limit the number of characters in the field.

nochiel commented 3 years ago

@nochiel https://github.com/nochiel we should honor the spec. Disable the comment field if commentAllowed is 0 or not present, if it >0 to limit the number of characters in the field.

Agreed. That was a mistake on my part. I'll review the spec again in case I made other similar errors.