Skip to content

Document that LSPS5 services should double-check the destination#4557

Merged
tnull merged 1 commit intolightningdevkit:mainfrom
TheBlueMatt:2026-04-lsps5-url-check
Apr 13, 2026
Merged

Document that LSPS5 services should double-check the destination#4557
tnull merged 1 commit intolightningdevkit:mainfrom
TheBlueMatt:2026-04-lsps5-url-check

Conversation

@TheBlueMatt
Copy link
Copy Markdown
Collaborator

It would be easy to implement an LSPS5 service and forget that the webhook calls are going out based on a URI and headers provided by an untrusted client, so such implementations need to make sure to check if the destination is some internal resource before sending.

Reported by Jordan Mecom of Block's Security Team

It would be easy to implement an LSPS5 service and forget that the
webhook calls are going out based on a URI and headers provided by
an untrusted client, so such implementations need to make sure to
check if the destination is some internal resource before sending.

Reported by Jordan Mecom of Block's Security Team
@ldk-reviews-bot
Copy link
Copy Markdown

ldk-reviews-bot commented Apr 13, 2026

I've assigned @joostjager as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Comment on lines +59 to +61
/// Obviously as the URL provided here is untrusted you should check whether it would
/// access any internal or private resources and decline to send the request if it is.
///
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.

nit: Two small suggestions:

  1. The trailing "if it is" reads awkwardly — consider "if so" or rewording:
Suggested change
/// Obviously as the URL provided here is untrusted you should check whether it would
/// access any internal or private resources and decline to send the request if it is.
///
/// Obviously as the URL provided here is untrusted you should check whether it would
/// access any internal or private resources (SSRF) and decline to send the request if so.
///
  1. This warning is only on the url field doc, which is easy to miss. Consider also adding a brief SSRF note to the variant-level doc block above (around line 35), since that's the first thing developers will read when handling SendWebhookNotification.

@ldk-claude-review-bot
Copy link
Copy Markdown
Collaborator

This is a documentation-only PR adding an SSRF warning to the url field of LSPS5ServiceEvent::SendWebhookNotification. One minor inline comment posted:

  • lightning-liquidity/src/lsps5/event.rs:59-61 — Slightly awkward grammar ("if it is" → "if so"), suggest mentioning "SSRF" by name, and consider duplicating the warning into the variant-level doc block so it isn't buried in a field doc that could be overlooked.

No bugs, security issues, or logic errors — this is a straightforward doc improvement.

@tnull tnull merged commit dc58563 into lightningdevkit:main Apr 13, 2026
20 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants