-
Notifications
You must be signed in to change notification settings - Fork 14
feat: add lud21 verify base spec. #108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- added as optional in advanced settings - api endpoint returning the success response
|
|
||
| if link.verify: | ||
| verify_url = request.url_for( | ||
| "lnurlp.api_lnurl_verify", payment_hash=payment.payment_hash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LUD does not specify that the check value must be the payment hash.
Can't this in fact be a vulnerability? Anyone that sees the QR code can check the payment status (and obtain the pre-image).
Suggestion:
- use
verify_secretrandom value - introduce
extracolumn (not to have such a large number of migrations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the payment_hash is actually inside the bolt11 invoice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no reason for adding anything extra, the verify link is sent with the bolt11 invoice which contains the payment_hash already, so there is no additional information leaked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@motorina0 can you re-evaluate the NACK?
talvasconcelos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested
closes #79