Work around '#' escaping bug in bip21 crate

[nothingmuch]

The pj parameter of the BIP 21 URL is itself a URL which contains a
fragment.

The # character was not escaped by bip21 during serialization. According
to RFC 3986, this terminates the query string, effectively truncating the pj
parameter.

The buggy deserialization likewise ignored #, parsing it as part of the
value, which survived a round trips with the incorrect serialization
behavior.

Upstream fix PR: payjoin/bitcoin_uri#3

[DanGould]

Hmm. It looks like The Sender's parsed subdirectory evaluates to "Ak-eJMIFs9fSuMoULsRwGhVnrvNhLc4IuMGmq9rjt8er%23ohttp=AQJPzw3sw5nHUaLaeVCqmGguYzH5VfjZBYHkQhS3h6ZQVQ&exp=1729708517"

You can see the %23ohttp isn't decoded to #ohttp

[DanGould]

It seems like this PR is missing the corresponding percent-decode in deserialize_temp. I thought we wrote this together when pair programming

Something like the following seems to address the issue in my local environment.

            "pj" if self.pj.is_none() => {
                let encoded = Cow::try_from(value).map_err(|_| InternalPjParseError::NotUtf8)?;
                let decoded = percent_decode_str(&encoded)
                    .map_err(|_| InternalPjParseError::BadEndpoint)?
                    .decode_utf8()
                    .map_err(|_| InternalPjParseError::NotUtf8)?;
                let url = Url::parse(&decoded).map_err(|_| InternalPjParseError::BadEndpoint)?;
                self.pj = Some(url);

                Ok(bip21::de::ParamKind::Known)
            }

These commits should be squashed if we agree that they're the right ones to make

[nothingmuch]

I don't understand the CI error, I can't replicate it locally with act, and neither cargo 1.77.2 (e52e36006 2024-03-26), 1.80.0 (376290515 2024-07-16), nor 1.81.0 (2dbb1af80 2024-08-20) does anything to the lockfile, I'd appreciate some guidance

[DanGould]

I believe the long-running ohttp relay and directory tasks in integration tests started becoming flaky around a week ago. I'm not sure why they're causing the problem, but the problem seems to have to do with the GitHub CI environment

[nothingmuch]

It seems like this PR is missing the corresponding percent-decode in deserialize_temp. I thought we wrote this together when pair programming

My fix is actually buggy, because the '#' only encoding pass should happen after the incomplete encoding, because it runs before the '%' in the '%23' is then escaped as well

The bip21 crate is correct here, so unfortunately I don't think there's a simple workaround without the upstream fix:

https://github.com/Kixunil/bip21/blob/eae72026cc5838bb169949641948b8c1cef99cbe/src/ser.rs#L139

https://github.com/Kixunil/bip21/blob/eae72026cc5838bb169949641948b8c1cef99cbe/src/ser.rs#L57

Something like the following seems to address the issue in my local environment.

            "pj" if self.pj.is_none() => {
                let encoded = Cow::try_from(value).map_err(|_| InternalPjParseError::NotUtf8)?;
                let decoded = percent_decode_str(&encoded)
                    .map_err(|_| InternalPjParseError::BadEndpoint)?
                    .decode_utf8()
                    .map_err(|_| InternalPjParseError::NotUtf8)?;
                let url = Url::parse(&decoded).map_err(|_| InternalPjParseError::BadEndpoint)?;
                self.pj = Some(url);

                Ok(bip21::de::ParamKind::Known)
            }

This is not correct as it would decode escaped parameter values, e.g. if the pj URI has a query string in it, and that has an escaped # in it, it'd break the pj URI parsing.

Decoding is already done here:

https://github.com/Kixunil/bip21/blob/eae72026cc5838bb169949641948b8c1cef99cbe/src/de.rs#L65

As confirmed by this test:

https://github.com/payjoin/rust-payjoin/pull/373/files#diff-0a6cb81cf3412ef9ec45c2a520b79c91435b057e6557dc466ecb88faf7ac6df9R133

[nothingmuch]

I don't see any other way of fixing serialization

Something like the following approach could work if PjUri wasn't an alias to an external type making the trait definition impossible (well apart from the infinite recursion):

impl<'a> fmt::Display for PjUri<'a> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        let malformed_uri = format!("{}", self);
        let escaped = malformed_uri.replacen("#", "%23", 1);
        write!(f, "{}", escaped)?;
    }
}

The only solutions I can see are waiting for upstream fix to be merged or making PjUri (which is pub) a wrapper type instead of an alias so overriding fmt can make PjUri stringify with the correct escaping (making it a wrapper also fixes the recursion).

I pushed a version of the latter with a Deref trait, but that still breaks the API even if it can maintain syntactic compatibility for accessing the bip21::Uri fields, so this doesn't seem advisable.

[nothingmuch]

Dropped the Deref hack, just froze the dep to use the bugfix PR branch as per our chat

[DanGould]

Looks like it makes the most sense to link bech32 = "*" since it's not exported from rust-bitcoin

rust-bitcoin/rust-bitcoin#3650

edit: Crates.io does NOT allow wildcard version dependencies

[DanGould]

Since this is deadlocked by kixunil I propose we start a new crate with hopes to implement the new unmerged bip spec anyhow. @spacebear21 is this something you'll have capacity for?

[spacebear21]

Since this is deadlocked by kixunil I propose we start a new crate with hopes to implement the new unmerged bip spec anyhow. @spacebear21 is this something you'll have capacity for?

Sure, should we start by just forking Kixunil's repo into the payjoin project and merge Kixunil/bip21#26 there? I'm hesitant to publish a crate for an unfinalized/unmerged bip, which wouldn't even be compliant with said bip, but I guess we could put a big disclaimer up front.

[DanGould]

The Kixunil/bip21#26 issue is now fixed in bitcoin_uri 0.1.0 fork

[coveralls]

Pull Request Test Coverage Report for Build 12131928990

Details

• 23 of 25 (92.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 61.869%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/uri/mod.rs	10	12	83.33%

Totals
Change from base Build 12131589868:	0.1%
Covered Lines:	2867
Relevant Lines:	4634

---

💛 - Coveralls

[DanGould]

ACK 66e243d

edit: and 1aaa7a7 which edited the commit message