Validate HTTPS scheme in LSPS5 URL Readable deserialization#4560
Conversation
|
I've assigned @jkczyz as a reviewer! |
|
Review Summary One issue found:
The core fix itself is correct: |
vincenzopalazzo
force-pushed
the
claude/nostalgic-merkle
branch
2 times, most recently
from
4c93ef8 to
5076ad9
Compare
vincenzopalazzo
force-pushed
the
claude/nostalgic-merkle
branch
from
5076ad9 to
770c205
Compare
The `Readable` implementations for `LSPSUrl` and `LSPS5WebhookUrl` were bypassing URL validation, allowing non-HTTPS URLs (e.g., http://, ftp://) to be deserialized from the wire protocol without rejection. Only the serde `Deserialize` and `new()`/`parse()` paths were correctly validating the HTTPS scheme. Route `LSPSUrl::Readable` through `LSPSUrl::parse()` and add a length check to `LSPS5WebhookUrl::Readable` so that wire-deserialized URLs receive the same validation as JSON-deserialized ones. Fixes lightningdevkit#4559 Reported-by: Thomas Kilbride of Block Security Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
vincenzopalazzo
force-pushed
the
claude/nostalgic-merkle
branch
from
770c205 to
78df66d
Compare
| 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 { |
There was a problem hiding this comment.
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.
| 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.)
There was a problem hiding this comment.
Probably unrelated from the PR itself
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4560 +/- ##
=======================================
Coverage 86.99% 86.99%
=======================================
Files 163 163
Lines 108706 108744 +38
Branches 108706 108744 +38
=======================================
+ Hits 94571 94605 +34
- Misses 11655 11658 +3
- Partials 2480 2481 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
Summary
LSPSUrl::ReadableandLSPS5WebhookUrl::Readablebypassed URL validation, allowing non-HTTPS URLs (http://, ftp://, etc.) through wire protocol deserializationReadableimpls directly wrapped raw deserialized bytes without calling the validatingparse()/new()constructorsLSPSUrl::ReadablethroughLSPSUrl::parse()and adds aMAX_WEBHOOK_URL_LENGTHcheck toLSPS5WebhookUrl::ReadableFixes #4559
Reported-by: Thomas Kilbride of Block Security
Test plan
lightning-liquiditytest suite passesCo-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com