Work around '#' escaping bug in bip21 crate

[nothingmuch]

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

This is not escaped by bip21 during serialization, which according to RFC 3986 truncates the pj parameter, making everything that follows part of the fragment.

Deserialization likewise ignores #, parsing it as part of the value which round trips with the incorrect serialization behavior.

Upstream fix PR: Kixunil/bip21#26

[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]

@spacebear21 any opposition to merging the git head to resolve this issue? This would block our release of 0.21.0 until bip21-0.6.0 gets released. We could still open release PRs until either 0.6.0 gets released or we release our own fork.

I think we can recognize that this solution works by merging it, and we'll have an implicit block on release until it's resolved.

[spacebear21]

I'm not opposed to merging it with the git repo dependency, but as it stands currently the commit in Cargo.lock is out of sync with the latest Kixunil/bip21#26. Perhaps we should specify the exact commit SHA-1 in rev instead of a reference to head which might change later? At a minimum we should regenerate Cargo.lock before merging.

[DanGould]

That sounds right to me. I just opened a this PR #379 with a script for updating lock files

[nothingmuch]

I will run it and update this PR to snapshot the correct commit. I did look at the upstream one a few more times and didn't find any more nits, but even if there's no additional reviewer feedback to address the upstream PR should be squashed before merging so I don't think it works well as a stable reference

[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.