Use `bitreq::Url` for LSPS5 webhook URLs
What changed, and why it matters
This commit swaps a custom-built URL parser for a well-known library (bitreq::Url) when handling LSPS5 webhook URLs. The goal is to reduce home-grown parsing bugs and make accepted URLs match what the HTTP client actually understands. It keeps the rule that only HTTPS URLs are allowed and still enforces a length limit elsewhere. The change is a hardening/refactoring move rather than a fix for a known active vulnerability, though it could close gaps between the old parser and the real HTTP client.
Treat as a defensive hardening change. Review that bitreq::Url's accepted URL set does not introduce unexpected schemes, IDNA, or normalization behavior that conflicts with LSPS5 security assumptions. Confirm the existing length check is still applied before the URL is used. No urgent patch action is indicated absent a disclosed vulnerability.
Security signals we found
Replaces custom URL parser with a maintained library parser (bitreq::Url)
Keeps HTTPS-only enforcement and length checks
Adds test coverage for userinfo and IPv6 literal URLs
Serialization change ensures &str and String encode identically
No explicit bug or CVE referenced in commit message or diff
Evidence from the diff
The patch replaces hand-rolled URL validation in lightning-liquidity/src/lsps5/url_utils.rs with bitreq::Url parsing while retaining the HTTPS scheme check and length checks. LSPSUrl now wraps bitreq::Url instead of UntrustedString, and PartialEq/Hash are implemented via the normalized URL string. Serialization is updated: Writeable writes url.as_str(), and Readable reads a String and re-parses it. A new test accepts URLs with userinfo and IPv6 literals. lightning/src/util/ser.rs gains a Writeable impl for &str and adjusts String::write to delegate to it, ensuring &str and String serialize identically.
Changed components
lightning-liquidity/src/lsps5/url_utils.rslightning-liquidity/src/lsps5/msgs.rslightning/src/util/ser.rslightning-liquidity/Cargo.tomlInspect captured patch +49 / −54
diff --git a/lightning-liquidity/Cargo.toml b/lightning-liquidity/Cargo.toml
index cc7fb0c..9b8114e 100644
--- a/lightning-liquidity/Cargo.toml
+++ b/lightning-liquidity/Cargo.toml
@@ -33,6 +33,7 @@ chrono = { version = "0.4", default-features = false, features = ["serde", "allo
serde = { version = "1.0", default-features = false, features = ["derive", "alloc"] }
serde_json = { version = "1.0", default-features = false, features = ["alloc"] }
backtrace = { version = "0.3", optional = true }
+bitreq = { version = "0.3.2", default-features = false }
[dev-dependencies]
lightning = { version = "0.3.0", path = "../lightning", default-features = false, features = ["_test_utils"] }
diff --git a/lightning-liquidity/src/lsps5/msgs.rs b/lightning-liquidity/src/lsps5/msgs.rs
index 6e9c5df..41e05d6 100644
--- a/lightning-liquidity/src/lsps5/msgs.rs
+++ b/lightning-liquidity/src/lsps5/msgs.rs
@@ -876,7 +876,7 @@ mod tests {
}
#[test]
- fn test_url_security_validation() {
+ fn test_webhook_url_validation() {
let urls_that_should_throw = [
"test-app",
"http://example.com/webhook",
@@ -906,6 +906,16 @@ mod tests {
}
}
+ #[test]
+ fn test_webhook_url_accepts_https_userinfo_and_ipv6() {
+ let userinfo_url =
+ LSPS5WebhookUrl::new("https://user:pass@example.com/webhook".to_string()).unwrap();
+ assert_eq!(userinfo_url.as_str(), "https://user:pass@example.com/webhook");
+
+ let ipv6_url = LSPS5WebhookUrl::new("https://[::1]/webhook".to_string()).unwrap();
+ assert_eq!(ipv6_url.as_str(), "https://[::1]/webhook");
+ }
+
#[test]
fn test_lsps_url_readable_rejects_http() {
use lightning::util::ser::Writeable;
diff --git a/lightning-liquidity/src/lsps5/url_utils.rs b/lightning-liquidity/src/lsps5/url_utils.rs
index 2a660b4..b451526 100644
--- a/lightning-liquidity/src/lsps5/url_utils.rs
+++ b/lightning-liquidity/src/lsps5/url_utils.rs
@@ -11,15 +11,28 @@
use super::msgs::LSPS5ProtocolError;
+use bitreq::Url;
use lightning::ln::msgs::DecodeError;
use lightning::util::ser::{Readable, Writeable};
-use lightning_types::string::UntrustedString;
use alloc::string::String;
+use core::hash::{Hash, Hasher};
/// Represents a parsed URL for LSPS5 webhook notifications.
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
-pub struct LSPSUrl(UntrustedString);
+#[derive(Debug, Clone, Eq)]
+pub struct LSPSUrl(Url);
+
+impl PartialEq for LSPSUrl {
+ fn eq(&self, other: &Self) -> bool {
+ self.0.as_str() == other.0.as_str()
+ }
+}
+
+impl Hash for LSPSUrl {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ self.0.as_str().hash(state)
+ }
+}
impl LSPSUrl {
/// Parses a URL string into a URL instance.
@@ -30,65 +43,23 @@ impl LSPSUrl {
/// # Returns
/// A Result containing either the parsed URL or an error message.
pub fn parse(url_str: String) -> Result<Self, LSPS5ProtocolError> {
- if url_str.chars().any(|c| !Self::is_valid_url_char(c)) {
- return Err(LSPS5ProtocolError::UrlParse);
- }
+ let url = Url::parse(&url_str).map_err(|_| LSPS5ProtocolError::UrlParse)?;
- let (scheme, remainder) =
- url_str.split_once("://").ok_or_else(|| LSPS5ProtocolError::UrlParse)?;
-
- if !scheme.eq_ignore_ascii_case("https") {
+ if url.scheme() != "https" {
return Err(LSPS5ProtocolError::UnsupportedProtocol);
}
- let host_section =
- remainder.split(['/', '?', '#']).next().ok_or_else(|| LSPS5ProtocolError::UrlParse)?;
-
- let host_without_auth = host_section
- .split('@')
- .next_back()
- .filter(|s| !s.is_empty())
- .ok_or_else(|| LSPS5ProtocolError::UrlParse)?;
-
- if host_without_auth.is_empty()
- || host_without_auth.chars().any(|c| !Self::is_valid_host_char(c))
- {
- return Err(LSPS5ProtocolError::UrlParse);
- }
-
- match host_without_auth.rsplit_once(':') {
- Some((hostname, _)) if hostname.is_empty() => return Err(LSPS5ProtocolError::UrlParse),
- Some((_, port)) => {
- if !port.is_empty() && port.parse::<u16>().is_err() {
- return Err(LSPS5ProtocolError::UrlParse);
- }
- },
- None => {},
- };
-
- Ok(LSPSUrl(UntrustedString(url_str)))
+ Ok(LSPSUrl(url))
}
/// Returns URL length in bytes.
- ///
- /// Since [`LSPSUrl::parse`] only accepts ASCII characters, this is equivalent
- /// to the character count.
pub fn url_length(&self) -> usize {
- self.0 .0.len()
+ self.0.as_str().len()
}
/// Returns the full URL string.
pub fn url(&self) -> &str {
- self.0 .0.as_str()
- }
-
- fn is_valid_url_char(c: char) -> bool {
- c.is_ascii_alphanumeric()
- || matches!(c, ':' | '/' | '.' | '@' | '?' | '#' | '%' | '-' | '_' | '&' | '=')
- }
-
- fn is_valid_host_char(c: char) -> bool {
- c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | ':' | '_')
+ self.0.as_str()
}
}
@@ -96,13 +67,13 @@ impl Writeable for LSPSUrl {
fn write<W: lightning::util::ser::Writer>(
&self, writer: &mut W,
) -> Result<(), lightning::io::Error> {
- self.0.write(writer)
+ self.0.as_str().write(writer)
}
}
impl Readable for LSPSUrl {
fn read<R: lightning::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
- let s: UntrustedString = Readable::read(reader)?;
- Self::parse(s.0).map_err(|_| DecodeError::InvalidValue)
+ let s: String = Readable::read(reader)?;
+ Self::parse(s).map_err(|_| DecodeError::InvalidValue)
}
}
diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs
index 2b02629..bd2488b 100644
--- a/lightning/src/util/ser.rs
+++ b/lightning/src/util/ser.rs
@@ -1631,6 +1631,13 @@ impl Readable for () {
}
impl Writeable for String {
+ #[inline]
+ fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
+ self.as_str().write(w)
+ }
+}
+
+impl Writeable for &str {
#[inline]
fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
CollectionLength(self.len() as u64).write(w)?;
@@ -1797,6 +1804,12 @@ mod tests {
assert_eq!(Hostname::read(&mut buf.as_slice()).unwrap().as_str(), "test");
}
+ #[test]
+ fn str_serialization_matches_string() {
+ let s = "test";
+ assert_eq!(s.encode(), s.to_string().encode());
+ }
+
#[test]
/// Taproot will likely fill legacy signature fields with all 0s.
/// This test ensures that doing so won't break serialization.
Why this scored 35/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.