fix(bolt12): Make CurrencyCode a validated wrapper type by erickcestari · Pull Request #3814 · lightningdevkit/rust-lightning

erickcestari · 2025-05-30T16:36:52Z

After performing differential fuzzing using bitcoinfuzz with rust-lightning and Core Lightning (CLN), I noticed that rust-lightning does not verify whether the offer_currency field contains valid UTF-8.

This commit convert CurrencyCode from type alias to struct with validation, ensuring
ISO 4217 compliance (3 ASCII uppercase letters) at construction time.

ldk-reviews-bot · 2025-05-30T16:36:55Z

👋 Thanks for assigning @jkczyz 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.

codecov · 2025-05-30T16:46:19Z

Codecov Report

Attention: Patch coverage is 90.09009% with 11 lines in your changes missing coverage. Please review.

Project coverage is 89.87%. Comparing base (78fee88) to head (388c5d5).
Report is 151 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/offers/offer.rs	88.88%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3814      +/-   ##
==========================================
+ Coverage   89.52%   89.87%   +0.34%     
==========================================
  Files         157      160       +3     
  Lines      125100   129412    +4312     
  Branches   125100   129412    +4312     
==========================================
+ Hits       112002   116311    +4309     
+ Misses      10408    10396      -12     
- Partials     2690     2705      +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

erickcestari · 2025-05-30T16:52:26Z

I took a closer look, and it seems this test vector has an invalid currency length in UTF-8 encoding, rather than being an invalid UTF-8 string itself.

  {
    "description": "Malformed: invalid currency UTF-8",
    "valid": false,
    "bolt12": "lno1qcpgqsg2q4q5cj2rg5tzzqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqg"
  },

data part: 060280410a05414c4943451621020202020202020202020202020202020202020202020202020202020202020202

Looking at the data part, we observe 06 (currency type), 02 (length), and 8041 (value), which confirms that the error arises from an invalid currency length in UTF-8 encoding, not from an invalid UTF-8 sequence.

I'll open a PR on Bolts to fix it.

(edited)

TheBlueMatt

Aren't the currencies supposed to be the three-char currency specification standard? ie they should be ASCII only (and probably all-caps letter only, never numbers)?

jkczyz

Thanks for jumping on this!

lightning/src/offers/offer.rs

jkczyz · 2025-05-30T20:30:35Z

I took a closer look, and it seems this test vector has an invalid currency length in UTF-8 encoding, rather than being an invalid UTF-8 string itself.
  {
    "description": "Malformed: invalid currency UTF-8",
    "valid": false,
    "bolt12": "lno1qcpgqsg2q4q5cj2rg5tzzqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqgpqyqszqg"
  },
data part: 060280410a05414c4943451621020202020202020202020202020202020202020202020202020202020202020202

Looking at the data part, we observe 06 (currency type), 02 (length), and 2804 (value), which confirms that the error arises from an invalid currency length in UTF-8 encoding, not from an invalid UTF-8 sequence.

I'll open a PR on Bolts to fix it.

Ah, yeah either the test vector or the comment is wrong lol

erickcestari · 2025-05-30T22:17:29Z

Aren't the currencies supposed to be the three-char currency specification standard? ie they should be ASCII only (and probably all-caps letter only, never numbers)?

Initially, I considered just aligning it with the UTF-8 type for consistency. But yes, it makes more sense to enforce stricter validation to ensure it's valid ASCII and adheres to the expected format.

ldk-reviews-bot · 2025-06-02T00:00:14Z

🔔 1st Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull · 2025-06-02T09:12:02Z

Ah, funny, seems this fixes #3813 we just opened. Will link the issue.

thomash-acinq · 2025-06-02T12:12:59Z

data part: 060280410a05414c4943451621020202020202020202020202020202020202020202020202020202020202020202

Looking at the data part, we observe 06 (currency type), 02 (length), and 2804 (value), which confirms that the error arises from an invalid currency length in UTF-8 encoding, not from an invalid UTF-8 sequence.

No, it's 06 (currency type), 02 (length), and 8041 (value). It seems that you've copied the '2' to read '2804'.

erickcestari · 2025-06-02T12:47:35Z

data part: 060280410a05414c4943451621020202020202020202020202020202020202020202020202020202020202020202
Looking at the data part, we observe 06 (currency type), 02 (length), and 2804 (value), which confirms that the error arises from an invalid currency length in UTF-8 encoding, not from an invalid UTF-8 sequence.

No, it's 06 (currency type), 02 (length), and 8041 (value). It seems that you've copied the '2' to read '2804'.

Yes, My mistake on the transcription.

It's invalid UTF-8, but the decoder fails due to the length mismatch (expecting 3 bytes for currency) rather than the UTF-8 validation itself.

TheBlueMatt

Rather than checking this when deserializing an offer, can we change CurrencyCode to be a real wrapper so that we can enforce it on creation/deserialization of that struct? Currently I believe we will let users build offers that we will refuse to parse.

ldk-reviews-bot · 2025-06-02T15:07:18Z

🔔 1st Reminder

Hey @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

jkczyz · 2025-06-02T16:25:28Z

Rather than checking this when deserializing an offer, can we change CurrencyCode to be a real wrapper so that we can enforce it on creation/deserialization of that struct? Currently I believe we will let users build offers that we will refuse to parse.

They would be helpful as upcoming OfferMessageFlow changes to support currencies could benefit from a dedicated type rather than a variant in an enum (cc: @shaavan).

jkczyz

Will defer to @TheBlueMatt on some of these comments,

jkczyz · 2025-06-03T14:57:39Z

lightning/src/offers/offer.rs

+	/// Creates a new CurrencyCode from a 3-byte array.
+	///
+	/// Returns an error if the bytes are not valid UTF-8 or not all ASCII uppercase.
+	pub fn new(code: [u8; 3]) -> Result<Self, Bolt12SemanticError> {


Should we use the unit type () for the error given and map at the call site given it can't be any other variant than InvalidCurrencyCode?

I have created a new custom error type for parsing it.

jkczyz · 2025-06-03T14:58:26Z

lightning/src/offers/offer.rs

+
+	/// Returns the currency code as a string slice.
+	pub fn as_str(&self) -> &str {
+		unsafe { core::str::from_utf8_unchecked(&self.0) }


Not sure if we should just use from_utf8().expect().

Yea, while its safe, there's also not much reason to bother with the optimization of skipping UTF-8 validation on a 3-char string (even if it would marginally reduce the binary size by avoiding the unwrap line and/or error string).

lightning/src/offers/offer.rs

ldk-reviews-bot · 2025-06-04T00:00:22Z

🔔 2nd Reminder

Hey @TheBlueMatt @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

TheBlueMatt

Thanks!

lightning/src/offers/offer.rs

TheBlueMatt · 2025-06-04T20:59:36Z

lightning/src/offers/offer.rs

+
+	/// Returns the currency code as a string slice.
+	pub fn as_str(&self) -> &str {
+		unsafe { core::str::from_utf8_unchecked(&self.0) }


Yea, while its safe, there's also not much reason to bother with the optimization of skipping UTF-8 validation on a 3-char string (even if it would marginally reduce the binary size by avoiding the unwrap line and/or error string).

Convert CurrencyCode from type alias to struct with validation, ensuring ISO 4217 compliance (3 ASCII uppercase letters) at construction time.

Tests cover UTF-8 validation, ASCII uppercase validation, case sensitivity, length checks, and API consistency between new() and from_str().

TheBlueMatt

thanks!

ldk-reviews-bot requested a review from TheBlueMatt May 30, 2025 16:47

erickcestari force-pushed the validate-currency-code branch from 7232027 to 748747b Compare May 30, 2025 16:48

TheBlueMatt reviewed May 30, 2025

View reviewed changes

jkczyz reviewed May 30, 2025

View reviewed changes

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved

erickcestari force-pushed the validate-currency-code branch from 748747b to 91e633a Compare May 31, 2025 15:05

erickcestari changed the title ~~fix(bolt12): Add UTF-8 validation for offer currency field~~ fix(bolt12): Add ASCII validation for offer currency field May 31, 2025

erickcestari requested review from TheBlueMatt and jkczyz May 31, 2025 15:06

tnull mentioned this pull request Jun 2, 2025

Validate ISO 4217 compliance in BOLT12 CurrencyCode #3813

Closed

tnull linked an issue Jun 2, 2025 that may be closed by this pull request

Validate ISO 4217 compliance in BOLT12 CurrencyCode #3813

Closed

TheBlueMatt reviewed Jun 2, 2025

View reviewed changes

erickcestari force-pushed the validate-currency-code branch 2 times, most recently from d9b2684 to 1ebbcd0 Compare June 3, 2025 12:38

erickcestari changed the title ~~fix(bolt12): Add ASCII validation for offer currency field~~ fix(bolt12): Make CurrencyCode a validated wrapper type Jun 3, 2025

jkczyz reviewed Jun 3, 2025

View reviewed changes

erickcestari force-pushed the validate-currency-code branch 3 times, most recently from 41929f5 to 46cbed5 Compare June 3, 2025 19:08

erickcestari requested a review from jkczyz June 3, 2025 19:55

erickcestari requested a review from TheBlueMatt June 3, 2025 19:55

TheBlueMatt reviewed Jun 4, 2025

View reviewed changes

erickcestari force-pushed the validate-currency-code branch from 87c2752 to e2c14e1 Compare June 5, 2025 13:27

erickcestari added 2 commits June 5, 2025 10:36

fix(bolt12): Make CurrencyCode a validated wrapper type

adb5393

Convert CurrencyCode from type alias to struct with validation, ensuring ISO 4217 compliance (3 ASCII uppercase letters) at construction time.

test(bolt12): Add tests for CurrencyCode validation

388c5d5

Tests cover UTF-8 validation, ASCII uppercase validation, case sensitivity, length checks, and API consistency between new() and from_str().

erickcestari force-pushed the validate-currency-code branch from e2c14e1 to 388c5d5 Compare June 5, 2025 13:38

jkczyz approved these changes Jun 5, 2025

View reviewed changes

TheBlueMatt approved these changes Jun 10, 2025

View reviewed changes

TheBlueMatt merged commit e797180 into lightningdevkit:main Jun 10, 2025
27 of 28 checks passed

Conversation

erickcestari commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

erickcestari commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkczyz commented May 30, 2025

Uh oh!

erickcestari commented May 30, 2025

Uh oh!

ldk-reviews-bot commented Jun 2, 2025

Uh oh!

tnull commented Jun 2, 2025

Uh oh!

thomash-acinq commented Jun 2, 2025

Uh oh!

erickcestari commented Jun 2, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Jun 2, 2025

Uh oh!

jkczyz commented Jun 2, 2025

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

jkczyz Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

erickcestari Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

jkczyz Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 4, 2025

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheBlueMatt Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

erickcestari commented May 30, 2025 •

edited

Loading

ldk-reviews-bot commented May 30, 2025 •

edited

Loading

codecov bot commented May 30, 2025 •

edited

Loading

erickcestari commented May 30, 2025 •

edited

Loading