Make sure we are properly validating Lightning Address according to LUD-16#134
Make sure we are properly validating Lightning Address according to LUD-16#134
Conversation
Summary of ChangesHello @hsjoberg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the validation of Lightning Addresses across both the client and server. The primary goal is to ensure full compliance with the LUD-16 specification, allowing for a broader range of valid usernames (specifically, including hyphens) while simultaneously tightening the overall validation logic on the server to prevent non-compliant addresses from being accepted. This change improves the reliability and correctness of Lightning Address handling within the application. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly moves towards proper LUD-16 validation for Lightning Addresses by allowing hyphens in usernames and replacing the email validation logic on the server. My review includes a few suggestions to further improve the validation logic on both the client and server. Specifically, I've pointed out issues with case-sensitivity in username validation, handling of empty usernames, and the need for more robust domain validation to fully align with the LUD-16 specification.
|
|
||
| fn ln_username_regex() -> &'static Regex { | ||
| static RE: OnceLock<Regex> = OnceLock::new(); | ||
| RE.get_or_init(|| Regex::new(r"^[a-z0-9_.-]+$").expect("valid ln username regex")) |
There was a problem hiding this comment.
The LUD-16 spec states that the username part of a Lightning Address is case-insensitive. This regex only matches lowercase letters, which would cause valid addresses with uppercase letters (e.g., Satoshi@domain.com) to be incorrectly rejected. Please update the regex to be case-insensitive by including A-Z.
| RE.get_or_init(|| Regex::new(r"^[a-z0-9_.-]+$").expect("valid ln username regex")) | |
| RE.get_or_init(|| Regex::new(r"^[a-zA-Z0-9_.-]+$").expect("valid ln username regex")) |
References
- The suggested regular expression correctly anchors the pattern to the end of the line with
$to prevent partial matches, aligning with the rule that unanchored regexes can match patterns even with trailing characters.
client/src/constants.ts
Outdated
There was a problem hiding this comment.
The regex for isUsername has two issues:
- It uses
*which allows an empty string for a username. An empty username is not valid for a Lightning Address. The server-side validation correctly checks for this, and the client should be consistent. - It only allows lowercase letters, but the LUD-16 spec states that usernames are case-insensitive.
To address these, the regex should enforce at least one character and allow both uppercase and lowercase letters.
| const isUsername = (n: string): boolean => /^[a-z0-9_.-]*$/.test(n); | |
| const isUsername = (n: string): boolean => /^[a-zA-Z0-9_.-]+$/.test(n); |
References
- The suggested regular expression correctly anchors the pattern to the end of the line with
$to prevent partial matches, aligning with the rule that unanchored regexes can match patterns even with trailing characters.
There was a problem hiding this comment.
It only allows lowercase letters, but the LUD-16 spec states that usernames are case-insensitive.
This is not true actually. Usernames are case sensitive -- they have to be lowercase.
See here:
https://github.com/lnurl/luds/blob/luds/16.md
A user can then type these on a WALLET. The <username> is limited to a-z0-9-_. (and + if the SERVICE supports tags).
(n.b: we are skipping SERVICE support right now. It's an obscure feature.)
| pub(crate) fn is_valid_lightning_address(value: &str) -> bool { | ||
| let (username, domain) = match value.split_once('@') { | ||
| Some(parts) => parts, | ||
| None => return false, | ||
| }; | ||
|
|
||
| if username.is_empty() || domain.is_empty() { | ||
| return false; | ||
| } | ||
|
|
||
| if domain.contains('@') { | ||
| return false; | ||
| } | ||
|
|
||
| is_valid_ln_username(username) | ||
| } |
There was a problem hiding this comment.
The validation for the domain part of the Lightning Address is very minimal. It only checks that the domain is not empty and does not contain an @ symbol. According to LUD-16, the domain should be a fully-qualified domain name (FQDN) or an IP address. The current implementation would allow invalid domains such as test@- or test@domain-.com. This is also a regression from the previous email validation, which at least ensured the domain contained a dot. Consider adding more robust validation for the domain to better align with the specification and prevent invalid addresses from being accepted.
There was a problem hiding this comment.
I think handling of the domain part can be covered in another PR. The client shouldn't really send this at all as it's the server that has the domain anyway.
hsjoberg
force-pushed
the
lnaddressing-spec-incompat
branch
from
88a560e to
2d83ea2
Compare
1 participant
This PR does two things:
isUsername()check for Lightning Address send used for SendScreen.tsx; it did not allow-, which is allowed in the specln_addressin the server use custom LNURL LUD-16 validator rather than checking for an email. Email addresses are crazy and allow a lot of insane things. We would otherwise allow spec-incompliant Lightning Addresses.