Skip to content
Merged
Show file tree
Hide file tree
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
58 changes: 57 additions & 1 deletion lightning-liquidity/src/lsps5/msgs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,11 @@ impl Writeable for LSPS5WebhookUrl {

impl Readable for LSPS5WebhookUrl {
fn read<R: lightning::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
Ok(Self(Readable::read(reader)?))
let url: LSPSUrl = Readable::read(reader)?;
if url.url().len() > MAX_WEBHOOK_URL_LENGTH {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not introduced by this PR, but LSPS5AppName::Readable (line 302–305) has the exact same bypass pattern that this PR fixes for LSPSUrl and LSPS5WebhookUrl — it directly wraps the deserialized UntrustedString without calling LSPS5AppName::new(), skipping the MAX_APP_NAME_LENGTH check.

Since this PR is specifically fixing this class of bug, it would be worth fixing LSPS5AppName::Readable in the same commit to avoid leaving the identical issue in the sibling type.

Suggested change
if url.url().len() > MAX_WEBHOOK_URL_LENGTH {
let url: LSPSUrl = Readable::read(reader)?;
if url.url().len() > MAX_WEBHOOK_URL_LENGTH {
return Err(DecodeError::InvalidValue);
}

(The code here is fine — just flagging the LSPS5AppName gap since it's the same pattern.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Probably unrelated from the PR itself

return Err(DecodeError::InvalidValue);
}
Comment thread
vincenzopalazzo marked this conversation as resolved.
Ok(Self(url))
}
}

Expand Down Expand Up @@ -902,6 +906,58 @@ mod tests {
}
}

#[test]
fn test_lsps_url_readable_rejects_http() {
use lightning::util::ser::Writeable;

let raw =
lightning_types::string::UntrustedString("http://example.com/webhook".to_string());
let encoded = raw.encode();
let result = LSPSUrl::read(&mut lightning::io::Cursor::new(&encoded));
assert!(result.is_err(), "LSPSUrl::Readable should reject http:// URLs");
}

#[test]
fn test_lsps_url_readable_accepts_https() {
use lightning::util::ser::Writeable;

let https_url = LSPSUrl::parse("https://example.com/webhook".to_string()).unwrap();
let encoded = https_url.encode();
let decoded = LSPSUrl::read(&mut lightning::io::Cursor::new(&encoded)).unwrap();
assert_eq!(decoded.url(), "https://example.com/webhook");
}

#[test]
fn test_webhook_url_readable_rejects_http() {
use lightning::util::ser::Writeable;

let raw =
lightning_types::string::UntrustedString("http://example.com/webhook".to_string());
let encoded = raw.encode();
let result = LSPS5WebhookUrl::read(&mut lightning::io::Cursor::new(&encoded));
assert!(result.is_err(), "Readable should reject http:// webhook URLs");
}

#[test]
fn test_webhook_url_readable_rejects_too_long() {
use lightning::util::ser::Writeable;

let long_url = LSPSUrl::parse(format!("https://example.com/{}", "a".repeat(2000))).unwrap();
let encoded = long_url.encode();
let result = LSPS5WebhookUrl::read(&mut lightning::io::Cursor::new(&encoded));
assert!(result.is_err(), "Readable should reject URLs exceeding MAX_WEBHOOK_URL_LENGTH");
}

#[test]
fn test_webhook_url_readable_accepts_valid_https() {
use lightning::util::ser::Writeable;

let valid_url = LSPS5WebhookUrl::new("https://example.com/webhook".to_string()).unwrap();
let encoded = valid_url.encode();
let decoded = LSPS5WebhookUrl::read(&mut lightning::io::Cursor::new(&encoded)).unwrap();
assert_eq!(decoded.as_str(), "https://example.com/webhook");
}

#[test]
fn test_webhook_notification_parameter_binding() {
let notification = WebhookNotification::expiry_soon(144);
Expand Down
10 changes: 7 additions & 3 deletions lightning-liquidity/src/lsps5/url_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,12 @@ impl LSPSUrl {
Ok(LSPSUrl(UntrustedString(url_str)))
}

/// Returns URL length.
/// 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.chars().count()
self.0 .0.len()
}

/// Returns the full URL string.
Expand Down Expand Up @@ -99,6 +102,7 @@ impl Writeable for LSPSUrl {

impl Readable for LSPSUrl {
fn read<R: lightning::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
Ok(Self(Readable::read(reader)?))
let s: UntrustedString = Readable::read(reader)?;
Self::parse(s.0).map_err(|_| DecodeError::InvalidValue)
}
}
Loading