Skip to content

Commit 2405459

Browse files
martinsaposnicmartinsaposnic
committed
fixup: get rid of display override when serializing webhookNotification. instead do serde_json::to_string directly as the format! argument
1 parent 3732bd0 commit 2405459

File tree

4 files changed

+74
-39
lines changed

4 files changed

+74
-39
lines changed

‎lightning-liquidity/src/lsps5/msgs.rs‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@ pub const LSPS5_TOO_MANY_WEBHOOKS_ERROR_CODE: i32 = 503;
4949
pub const LSPS5_APP_NAME_NOT_FOUND_ERROR_CODE: i32 = 1010;
5050
/// An unknown error occurred.
5151
pub const LSPS5_UNKNOWN_ERROR_CODE: i32 = 1000;
52+
/// An error occurred during serialization of LSPS5 webhook notification.
53+
pub const LSPS5_SERIALIZATION_ERROR_CODE: i32 = 1001;
5254

5355
pub(crate) const LSPS5_SET_WEBHOOK_METHOD_NAME: &str = "lsps5.set_webhook";
5456
pub(crate) const LSPS5_LIST_WEBHOOKS_METHOD_NAME: &str = "lsps5.list_webhooks";
@@ -98,6 +100,9 @@ pub enum LSPS5ProtocolError {
98100

99101
/// An unspecified or unexpected error occurred.
100102
UnknownError,
103+
104+
/// Error during serialization of LSPS5 webhook notification.
105+
SerializationError,
101106
}
102107

103108
impl LSPS5ProtocolError {
@@ -112,6 +117,7 @@ impl LSPS5ProtocolError {
112117
LSPS5ProtocolError::TooManyWebhooks => LSPS5_TOO_MANY_WEBHOOKS_ERROR_CODE,
113118
LSPS5ProtocolError::AppNameNotFound => LSPS5_APP_NAME_NOT_FOUND_ERROR_CODE,
114119
LSPS5ProtocolError::UnknownError => LSPS5_UNKNOWN_ERROR_CODE,
120+
LSPS5ProtocolError::SerializationError => LSPS5_SERIALIZATION_ERROR_CODE,
115121
}
116122
}
117123
/// The error message for the LSPS5 protocol error.
@@ -124,6 +130,9 @@ impl LSPS5ProtocolError {
124130
LSPS5ProtocolError::TooManyWebhooks => "Maximum number of webhooks allowed per client",
125131
LSPS5ProtocolError::AppNameNotFound => "App name not found",
126132
LSPS5ProtocolError::UnknownError => "Unknown error",
133+
LSPS5ProtocolError::SerializationError => {
134+
"Error serializing LSPS5 webhook notification"
135+
},
127136
}
128137
}
129138
}
@@ -162,6 +171,9 @@ pub enum LSPS5ClientError {
162171
/// Indicates a potential replay attack where a previously seen
163172
/// notification signature was reused.
164173
ReplayAttack,
174+
175+
/// Error during serialization of LSPS5 webhook notification.
176+
SerializationError,
165177
}
166178

167179
impl LSPS5ClientError {
@@ -173,6 +185,7 @@ impl LSPS5ClientError {
173185
InvalidSignature => Self::BASE + 1,
174186
InvalidTimestamp => Self::BASE + 2,
175187
ReplayAttack => Self::BASE + 3,
188+
SerializationError => LSPS5_SERIALIZATION_ERROR_CODE,
176189
}
177190
}
178191
/// The error message for the client error.
@@ -182,6 +195,7 @@ impl LSPS5ClientError {
182195
InvalidSignature => "Invalid signature",
183196
InvalidTimestamp => "Timestamp out of range",
184197
ReplayAttack => "Replay attack detected",
198+
SerializationError => "Error serializing LSPS5 webhook notification",
185199
}
186200
}
187201
}
@@ -499,15 +513,6 @@ impl WebhookNotification {
499513
}
500514
}
501515

502-
impl fmt::Display for WebhookNotification {
503-
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
504-
match serde_json::to_string(self) {
505-
Ok(json) => f.write_str(&json),
506-
Err(_) => Err(fmt::Error),
507-
}
508-
}
509-
}
510-
511516
impl Serialize for WebhookNotification {
512517
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
513518
where

‎lightning-liquidity/src/lsps5/service.rs‎

Lines changed: 42 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,19 @@ where
233233
counterparty_node_id,
234234
params.app_name,
235235
params.webhook,
236-
);
236+
)
237+
.map_err(|e| {
238+
let msg = LSPS5Message::Response(
239+
request_id.clone(),
240+
LSPS5Response::SetWebhookError(e.clone().into()),
241+
)
242+
.into();
243+
self.pending_messages.enqueue(&counterparty_node_id, msg);
244+
LightningError {
245+
err: e.message().into(),
246+
action: ErrorAction::IgnoreAndLog(Level::Info),
247+
}
248+
})?;
237249
}
238250

239251
let msg = LSPS5Message::Response(
@@ -307,7 +319,7 @@ where
307319

308320
fn send_webhook_registered_notification(
309321
&self, client_node_id: PublicKey, app_name: LSPS5AppName, url: LSPS5WebhookUrl,
310-
) {
322+
) -> Result<(), LSPS5ProtocolError> {
311323
let notification = WebhookNotification::webhook_registered();
312324
self.send_notification(client_node_id, app_name, url, notification)
313325
}
@@ -323,7 +335,7 @@ where
323335
/// - `client_id`: the client's node-ID whose webhooks should be invoked.
324336
///
325337
/// [`WebhookNotificationMethod::LSPS5PaymentIncoming`]: super::msgs::WebhookNotificationMethod::LSPS5PaymentIncoming
326-
pub fn notify_payment_incoming(&self, client_id: PublicKey) {
338+
pub fn notify_payment_incoming(&self, client_id: PublicKey) -> Result<(), LSPS5ProtocolError> {
327339
let notification = WebhookNotification::payment_incoming();
328340
self.broadcast_notification(client_id, notification)
329341
}
@@ -341,7 +353,9 @@ where
341353
/// - `timeout`: the block height at which the channel contract will expire.
342354
///
343355
/// [`WebhookNotificationMethod::LSPS5ExpirySoon`]: super::msgs::WebhookNotificationMethod::LSPS5ExpirySoon
344-
pub fn notify_expiry_soon(&self, client_id: PublicKey, timeout: u32) {
356+
pub fn notify_expiry_soon(
357+
&self, client_id: PublicKey, timeout: u32,
358+
) -> Result<(), LSPS5ProtocolError> {
345359
let notification = WebhookNotification::expiry_soon(timeout);
346360
self.broadcast_notification(client_id, notification)
347361
}
@@ -356,7 +370,9 @@ where
356370
/// - `client_id`: the client's node-ID whose webhooks should be invoked.
357371
///
358372
/// [`WebhookNotificationMethod::LSPS5LiquidityManagementRequest`]: super::msgs::WebhookNotificationMethod::LSPS5LiquidityManagementRequest
359-
pub fn notify_liquidity_management_request(&self, client_id: PublicKey) {
373+
pub fn notify_liquidity_management_request(
374+
&self, client_id: PublicKey,
375+
) -> Result<(), LSPS5ProtocolError> {
360376
let notification = WebhookNotification::liquidity_management_request();
361377
self.broadcast_notification(client_id, notification)
362378
}
@@ -371,17 +387,21 @@ where
371387
/// - `client_id`: the client's node-ID whose webhooks should be invoked.
372388
///
373389
/// [`WebhookNotificationMethod::LSPS5OnionMessageIncoming`]: super::msgs::WebhookNotificationMethod::LSPS5OnionMessageIncoming
374-
pub fn notify_onion_message_incoming(&self, client_id: PublicKey) {
390+
pub fn notify_onion_message_incoming(
391+
&self, client_id: PublicKey,
392+
) -> Result<(), LSPS5ProtocolError> {
375393
let notification = WebhookNotification::onion_message_incoming();
376394
self.broadcast_notification(client_id, notification)
377395
}
378396

379-
fn broadcast_notification(&self, client_id: PublicKey, notification: WebhookNotification) {
397+
fn broadcast_notification(
398+
&self, client_id: PublicKey, notification: WebhookNotification,
399+
) -> Result<(), LSPS5ProtocolError> {
380400
let mut webhooks = self.webhooks.lock().unwrap();
381401

382402
let client_webhooks = match webhooks.get_mut(&client_id) {
383403
Some(webhooks) if !webhooks.is_empty() => webhooks,
384-
_ => return,
404+
_ => return Ok(()),
385405
};
386406

387407
let now =
@@ -402,20 +422,21 @@ where
402422
app_name.clone(),
403423
webhook.url.clone(),
404424
notification.clone(),
405-
);
425+
)?;
406426
}
407427
}
428+
Ok(())
408429
}
409430

410431
fn send_notification(
411432
&self, counterparty_node_id: PublicKey, app_name: LSPS5AppName, url: LSPS5WebhookUrl,
412433
notification: WebhookNotification,
413-
) {
434+
) -> Result<(), LSPS5ProtocolError> {
414435
let event_queue_notifier = self.event_queue.notifier();
415436
let timestamp =
416437
LSPSDateTime::new_from_duration_since_epoch(self.time_provider.duration_since_epoch());
417438

418-
let signature_hex = self.sign_notification(&notification, &timestamp);
439+
let signature_hex = self.sign_notification(&notification, &timestamp)?;
419440

420441
let mut headers: HashMap<String, String> = [("Content-Type", "application/json")]
421442
.into_iter()
@@ -431,16 +452,23 @@ where
431452
notification,
432453
headers,
433454
});
455+
456+
Ok(())
434457
}
435458

436-
fn sign_notification(&self, body: &WebhookNotification, timestamp: &LSPSDateTime) -> String {
459+
fn sign_notification(
460+
&self, body: &WebhookNotification, timestamp: &LSPSDateTime,
461+
) -> Result<String, LSPS5ProtocolError> {
462+
let notification_json =
463+
serde_json::to_string(body).map_err(|_| LSPS5ProtocolError::SerializationError)?;
464+
437465
let message = format!(
438466
"LSPS5: DO NOT SIGN THIS MESSAGE MANUALLY: LSP: At {} I notify {}",
439467
timestamp.to_rfc3339(),
440-
body
468+
notification_json
441469
);
442470

443-
message_signing::sign(message.as_bytes(), &self.config.signing_key)
471+
Ok(message_signing::sign(message.as_bytes(), &self.config.signing_key))
444472
}
445473

446474
fn prune_stale_webhooks(&self) {

‎lightning-liquidity/src/lsps5/validator.rs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,10 +100,12 @@ where
100100
return Err(LSPS5ClientError::InvalidTimestamp);
101101
}
102102

103+
let notification_json = serde_json::to_string(notification)
104+
.map_err(|_| LSPS5ClientError::SerializationError)?;
103105
let message = format!(
104106
"LSPS5: DO NOT SIGN THIS MESSAGE MANUALLY: LSP: At {} I notify {}",
105107
signature_timestamp.to_rfc3339(),
106-
notification
108+
notification_json
107109
);
108110

109111
if message_signing::verify(message.as_bytes(), signature, &counterparty_node_id) {

‎lightning-liquidity/tests/lsps5_integration_tests.rs‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -477,7 +477,7 @@ fn webhook_notification_delivery_test() {
477477
"Client should be able to parse and validate the webhook_registered notification"
478478
);
479479

480-
service_handler.notify_payment_incoming(client_node_id);
480+
let _ = service_handler.notify_payment_incoming(client_node_id);
481481

482482
let payment_notification_event = service_node.liquidity_manager.next_event().unwrap();
483483
let (payment_timestamp, payment_signature, notification) = match payment_notification_event {
@@ -502,15 +502,15 @@ fn webhook_notification_delivery_test() {
502502
"Client should be able to parse and validate the payment_incoming notification"
503503
);
504504

505-
service_handler.notify_payment_incoming(client_node_id);
505+
let _ = service_handler.notify_payment_incoming(client_node_id);
506506

507507
assert!(
508508
service_node.liquidity_manager.next_event().is_none(),
509509
"No event should be emitted due to cooldown"
510510
);
511511

512512
let timeout_block = 700000; // Some future block height
513-
service_handler.notify_expiry_soon(client_node_id, timeout_block);
513+
let _ = service_handler.notify_expiry_soon(client_node_id, timeout_block);
514514

515515
let expiry_notification_event = service_node.liquidity_manager.next_event().unwrap();
516516
match expiry_notification_event {
@@ -564,7 +564,7 @@ fn multiple_webhooks_notification_test() {
564564
let _ = client_node.liquidity_manager.next_event().unwrap();
565565
}
566566

567-
service_handler.notify_liquidity_management_request(client_node_id);
567+
let _ = service_handler.notify_liquidity_management_request(client_node_id);
568568

569569
let mut seen_webhooks = HashSet::default();
570570

@@ -740,7 +740,7 @@ fn replay_prevention_test() {
740740

741741
let _ = client_node.liquidity_manager.next_event().unwrap();
742742

743-
service_handler.notify_payment_incoming(client_node_id);
743+
let _ = service_handler.notify_payment_incoming(client_node_id);
744744

745745
let notification_event = service_node.liquidity_manager.next_event().unwrap();
746746
let (timestamp, signature, body) = match notification_event {
@@ -848,10 +848,10 @@ fn test_all_notifications() {
848848
// consume initial SendWebhookNotification
849849
let _ = service_node.liquidity_manager.next_event().unwrap();
850850

851-
service_handler.notify_onion_message_incoming(client_node_id);
852-
service_handler.notify_payment_incoming(client_node_id);
853-
service_handler.notify_expiry_soon(client_node_id, 1000);
854-
service_handler.notify_liquidity_management_request(client_node_id);
851+
let _ = service_handler.notify_onion_message_incoming(client_node_id);
852+
let _ = service_handler.notify_payment_incoming(client_node_id);
853+
let _ = service_handler.notify_expiry_soon(client_node_id, 1000);
854+
let _ = service_handler.notify_liquidity_management_request(client_node_id);
855855

856856
let expected_notifications = vec![
857857
WebhookNotificationMethod::LSPS5OnionMessageIncoming,
@@ -901,7 +901,7 @@ fn test_tampered_notification() {
901901
// consume initial SendWebhookNotification
902902
let _ = service_node.liquidity_manager.next_event().unwrap();
903903

904-
service_handler.notify_expiry_soon(client_node_id, 700000);
904+
let _ = service_handler.notify_expiry_soon(client_node_id, 700000);
905905

906906
let event = service_node.liquidity_manager.next_event().unwrap();
907907
if let LiquidityEvent::LSPS5Service(LSPS5ServiceEvent::SendWebhookNotification {
@@ -948,7 +948,7 @@ fn test_bad_signature_notification() {
948948
// consume initial SendWebhookNotification
949949
let _ = service_node.liquidity_manager.next_event().unwrap();
950950

951-
service_handler.notify_onion_message_incoming(client_node_id);
951+
let _ = service_handler.notify_onion_message_incoming(client_node_id);
952952

953953
let event = service_node.liquidity_manager.next_event().unwrap();
954954
if let LiquidityEvent::LSPS5Service(LSPS5ServiceEvent::SendWebhookNotification {
@@ -997,7 +997,7 @@ fn test_timestamp_notification_window_validation() {
997997
// consume initial SendWebhookNotification
998998
let _ = service_node.liquidity_manager.next_event().unwrap();
999999

1000-
service_handler.notify_onion_message_incoming(client_node_id);
1000+
let _ = service_handler.notify_onion_message_incoming(client_node_id);
10011001

10021002
let expected_method = WebhookNotificationMethod::LSPS5OnionMessageIncoming;
10031003

@@ -1047,10 +1047,10 @@ fn test_notify_without_webhooks_does_nothing() {
10471047
let service_handler = service_node.liquidity_manager.lsps5_service_handler().unwrap();
10481048

10491049
// without ever registering a webhook -> both notifiers should early-return
1050-
service_handler.notify_payment_incoming(client_node_id);
1050+
let _ = service_handler.notify_payment_incoming(client_node_id);
10511051
assert!(service_node.liquidity_manager.next_event().is_none());
10521052

1053-
service_handler.notify_onion_message_incoming(client_node_id);
1053+
let _ = service_handler.notify_onion_message_incoming(client_node_id);
10541054
assert!(service_node.liquidity_manager.next_event().is_none());
10551055
}
10561056

@@ -1079,7 +1079,7 @@ fn no_replay_error_when_signature_storage_is_disabled() {
10791079

10801080
let _ = client_node.liquidity_manager.next_event().unwrap();
10811081

1082-
service_handler.notify_payment_incoming(client_node_id);
1082+
let _ = service_handler.notify_payment_incoming(client_node_id);
10831083

10841084
let notification_event = service_node.liquidity_manager.next_event().unwrap();
10851085
let (timestamp, signature, body) = match notification_event {

0 commit comments

Comments
 (0)