Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 35 additions & 8 deletions lightning/src/onion_message/dns_resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,8 @@ const REQUIRED_EXTRA_LEN: usize = ".user._bitcoin-payment.".len() + 1;

/// A struct containing the two parts of a BIP 353 Human Readable Name - the user and domain parts.
///
/// The `user` and `domain` parts, together, cannot exceed 231 bytes in length, and both must be
/// non-empty.
/// The `user` and `domain` parts combined cannot exceed 231 bytes in length;
/// each DNS label within them must be non-empty and no longer than 63 bytes.
///
/// If you intend to handle non-ASCII `user` or `domain` parts, you must handle [Homograph Attacks]
/// and do punycode en-/de-coding yourself. This struct will always handle only plain ASCII `user`
Expand All @@ -211,16 +211,21 @@ pub struct HumanReadableName {
impl HumanReadableName {
/// Constructs a new [`HumanReadableName`] from the `user` and `domain` parts. See the
/// struct-level documentation for more on the requirements on each.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you mention the new filtering in the struct docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done:

/// The `user` and `domain` parts combined cannot exceed 231 bytes in length;
/// each DNS label within them must be non-empty and no longer than 63 bytes.

pub fn new(user: &str, mut domain: &str) -> Result<HumanReadableName, ()> {
pub fn new(user: &str, domain: &str) -> Result<HumanReadableName, ()> {
// First normalize domain and remove the optional trailing `.`
if domain.ends_with('.') {
domain = &domain[..domain.len() - 1];
}
let domain = domain.strip_suffix(".").unwrap_or(domain);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: '.' and "." are different - '.' is the char . whereas "." is the string ".". We should be stripping the char, not the string, as it uses a more efficient algorithm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that what I meant.
I made a followup to clean this and similar instances: #4174.

if user.len() + domain.len() + REQUIRED_EXTRA_LEN > 255 {
return Err(());
}
if user.is_empty() || domain.is_empty() {
return Err(());
for label in user.split('.') {
if label.is_empty() || label.len() > 63 {
return Err(());
}
}
for label in domain.split('.') {
if label.is_empty() || label.len() > 63 {
return Err(());
}
}
if !Hostname::str_is_valid_hostname(&user) || !Hostname::str_is_valid_hostname(&domain) {
return Err(());
Expand Down Expand Up @@ -558,6 +563,28 @@ mod tests {
);
}

#[test]
fn test_hrn_validation() {
assert!(HumanReadableName::new("user", "example.com").is_ok());
assert!(HumanReadableName::new("user", "example.com.").is_ok());

assert!(HumanReadableName::new("user!", "example.com").is_err());
assert!(HumanReadableName::new("user.", "example.com").is_err());
assert!(HumanReadableName::new("user", "exa mple.com").is_err());
assert!(HumanReadableName::new("", "example.com").is_err());
assert!(HumanReadableName::new("user", "").is_err());

let max_label = "a".repeat(63);
assert!(HumanReadableName::new(&max_label, "example.com").is_ok());

let long_label = "a".repeat(64);
assert!(HumanReadableName::new(&long_label, "example.com").is_err());
let domain_with_long_label = format!("{long_label}.com");
assert!(HumanReadableName::new("user", &domain_with_long_label).is_err());
let huge_domain = format!("{max_label}.{max_label}.{max_label}.{max_label}");
assert!(HumanReadableName::new("user", &huge_domain).is_err());
}

#[test]
#[cfg(feature = "dnssec")]
fn test_expiry() {
Expand Down
Loading