Fragment parameter fixes #852

nothingmuch · 2025-07-05T12:49:12Z

This PR contains two changes, using - instead of + as the fragment parameter delimiter, and lexicographically ordering the fragment parameters.

This implements bitcoin/bips#1890

coveralls · 2025-07-05T12:52:47Z

Pull Request Test Coverage Report for Build 16131346449

Details

93 of 105 (88.57%) changed or added relevant lines in 2 files are covered.
4 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.01%) to 85.317%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/ohttp.rs	0	1	0.0%
payjoin/src/core/uri/url_ext.rs	93	104	89.42%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/core/uri/url_ext.rs	4	85.41%

Totals
Change from base Build 16124956763:	-0.01%
Covered Lines:	7455
Relevant Lines:	8738

💛 - Coveralls

payjoin/src/core/uri/url_ext.rs

The `url_ext` mod is itself feature gated, so any declarations within it are already implicitly v2 only.

Although RFC 3986 (URIs) does not assign any special meaning to `+`, in fragment parameters or in general, RFC 1866 (HTML 2.0) section 7.5 uses it as a delimiter for keywords in query parameters. As a result some URI libraries interpret `+` in URIs as ` `, even in fragment parameters. Although not insurmountable (such transformation BIP 77 URIs is reversible because ` ` is not used and `+` was only used for fragment parameter delimitation) this presents friction and is in general confusion, so to improve compatibility with such libraries `-` is now used instead. It has no reserved meaning as a sub-delimiter. For the time being when parsing both `+` and `-` will be accepted, but only `-` will be used when encoding fragment parameters.

payjoin/src/core/uri/url_ext.rs

DanGould

I'm really not sure that the last commit needs to go in (other than the base64-bech32 typo fix). What kind of errors does that prevent that would not otherwise be caught by bech32 parsing?

payjoin/src/core/uri/url_ext.rs

DanGould · 2025-07-07T15:34:34Z

payjoin/src/core/uri/url_ext.rs

+        // check for allowed delimiters
+        if c == b'-' {
+            has_dash = true;
+        } else if c == b'+' {
+            has_plus = true;
+        }


Does this need to be in the loop? Can't fragment.contains('-') and fragment.contains('+') be used outside of the loop in the match for greater legibility

i don't agree that that's more legibile, since these conditions are mutually exclusive with the charset range so conceptually it seems even more confusing to put these characters in the range of characters that are also included

not that efficiency really matters, but also scanning through the fragment once instead of 3 times seems reasonable

i roughly implemented this suggestion but i think i still prefer the older approach since repeating the logic with c != '-' && c != '+' places this information about the allowed delimiters in two different places instead of just one, which i find less legible than the slightly clunkier if else stuff

yeah I understand why you'd put it all together now and am comfortable with that though would merge this as-is at this point.

payjoin/src/core/uri/url_ext.rs

DanGould · 2025-07-07T15:49:05Z

payjoin/src/core/uri/url_ext.rs

+    AmbiguousDelimiter,
+}
+
+fn check_fragment_delimiter(fragment: &str) -> Result<char, ParseFragmentError> {


This function does a hell of a lot more than check the fragment delimiter.

please suggest a better name assuming the newly proposed behavior (check no fragment ambiguity and that the fragment ~= /^[A-Z0-9+\-]*$/)

check_fragment_charset ?

i did not take the suggested name because Result<char, ParseFragmentError> is not self explanatory in terms of what this method returns, determining the delimiter to use is the important thing this function calls and checking the charset is kinda of a precondition

nothingmuch · 2025-07-07T18:29:40Z

I'm really not sure that the last commit needs to go in (other than the base64-bech32 typo fix).

Yeah I think I agree, by the time I wrote it, especially the error conversion boilerplate I had kinda regretted it, but I pushed anyway so we can discuss. I'm in favor of reverting to the more permissive behavior, or something intermediate.

What kind of errors does that prevent that would not otherwise be caught by bech32 parsing?

mixed delimiters, which are indeed a contrivance (see the previous review comments)
HRP strings that are valid according to bech32 but fall outside of the BIP77
lowercase bech32

(1) can be done as fragment.contains('-') and +, and the match on pair of bools that arises from that more simply.

(2) seems unnecessary anyway, future extension mechanisms should be allowed as per BIP 77, and the behavior i implemented is too restrictive

(3) can be enforced as just checking that there's no lowercase chars

i will replace the last commit with a much more minimal one that does not attempt to do any bech32 charset validation as sketched in this reply

DanGould · 2025-07-07T18:31:58Z

Sounds great. Looking forward to the more minimal final commit that does not attempt to do any bech32 charset validation.

nothingmuch · 2025-07-07T18:34:12Z

Also, long term I would prefer something that avoids this mutation based approach entirely, using a builder pattern to queue up fields, and then just joining them instead of parsing and mutating to set would be much simpler, but I didn't want to redesign, if you're cACK i'll write this up in an issue

Previously `set_param` would did not preserve order, but the way that `set_param` was called ended up setting the RK, OH and EX fragment parameters in reverse lexicographical order. To avoid any privacy leaks from URI construction (revealing the specific software the receiver is using) the spec now requires fragment parameters to be ordered lexicographically, so `set_param` now ensures this.

DanGould

ACK 51c4147

Ambiguity in the fragment parameter delimiter or any invalid characters are no longer allowed. The HRPs EX, OH, and RK are within the uppercase bech32 character set. Only this character set along with the HRP delimiter `1` are now allowed, with either `+` or `-` as a delimiter (but not both).

DanGould · 2025-07-08T20:47:16Z

payjoin/src/core/uri/url_ext.rs

+        if !(b'0'..b'9' + 1).contains(&c)
+            && !(b'A'..b'Z' + 1).contains(&c)
+            && c != b'-'
+            && c != b'+'
+        {
+            return Err(ParseFragmentError::InvalidChar(c.into()));
        }


What do you think of this syntax?

Suggested change

if !(b'0'..b'9' + 1).contains(&c)

&& !(b'A'..b'Z' + 1).contains(&c)

&& c != b'-'

&& c != b'+'

{

return Err(ParseFragmentError::InvalidChar(c.into()));

}

if !matches!(c, b'0'..=b'9' | b'A'..=b'Z' | b'-' | b'+') {

return Err(ParseFragmentError::InvalidChar(c.into()));

}

DanGould · 2025-07-08T20:48:22Z

payjoin/src/core/uri/url_ext.rs


-    if !fragment.is_empty() {
-        fragment.push('+');
+    match (has_dash, has_plus) {


Ah I realize now the reason you had the has_dash, has_plus variables was to make the scanning operation O(1n) wrt the length of the fragment and not O(3n). If not that, why not?

Suggested change

match (has_dash, has_plus) {

match (fragment.contains('-'), fragment.contains('+')) {

DanGould

ACK 0eb74b9

What changed last night after my prior ACK? Looks good to me.

DanGould · 2025-07-09T15:43:56Z

A println! was removed that's all

nothingmuch requested a review from DanGould July 5, 2025 12:49

zealsham reviewed Jul 5, 2025

View reviewed changes

payjoin/src/core/uri/url_ext.rs Outdated Show resolved Hide resolved

nothingmuch added 3 commits July 6, 2025 19:13

Remove redundant v2 feature gating

27058bb

The `url_ext` mod is itself feature gated, so any declarations within it are already implicitly v2 only.

Fix off by one in set_param

b61eb7b

nothingmuch force-pushed the fragment-fixes branch 3 times, most recently from 19e9cf2 to 452e4b3 Compare July 6, 2025 19:29

nothingmuch commented Jul 7, 2025

View reviewed changes

payjoin/src/core/uri/url_ext.rs Outdated Show resolved Hide resolved

nothingmuch commented Jul 7, 2025

View reviewed changes

payjoin/src/core/uri/url_ext.rs Outdated Show resolved Hide resolved

DanGould requested changes Jul 7, 2025

View reviewed changes

nothingmuch force-pushed the fragment-fixes branch from 11c5768 to 51c4147 Compare July 7, 2025 23:44

DanGould approved these changes Jul 8, 2025

View reviewed changes

nothingmuch force-pushed the fragment-fixes branch from 51c4147 to 0eb74b9 Compare July 8, 2025 01:01

DanGould reviewed Jul 8, 2025

View reviewed changes

DanGould approved these changes Jul 8, 2025

View reviewed changes

DanGould merged commit 70755a9 into payjoin:master Jul 9, 2025
8 checks passed

	match (has_dash, has_plus) {
	match (fragment.contains('-'), fragment.contains('+')) {

Fragment parameter fixes #852

Fragment parameter fixes #852

Uh oh!

Conversation

nothingmuch commented Jul 5, 2025

Uh oh!

coveralls commented Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16131346449

Details

💛 - Coveralls

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch commented Jul 7, 2025

Uh oh!

DanGould commented Jul 7, 2025

Uh oh!

nothingmuch commented Jul 7, 2025

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanGould commented Jul 9, 2025

Uh oh!

Uh oh!

coveralls commented Jul 5, 2025 •

edited

Loading

DanGould Jul 8, 2025 •

edited

Loading