Create new lowercase error catch for uri endpoint fragments by benalleng · Pull Request #558 · payjoin/rust-payjoin

benalleng · 2025-02-28T17:43:31Z

Closes #555

The pj= param in the pjuri contains multiple different parts but there exist some gotchas when trying to use them as the url path can be parsed even when lowercase but the fragment object cannot. This means that it is possible to copy or display a pjuri that will not work if it is simply converted to lowercase.

This pr does not create any method to force the fragment to be uppercase as I think it really should be on the implementation to make sure that casing is correct, so I just created the case error catch.

coveralls · 2025-02-28T17:47:37Z

Pull Request Test Coverage Report for Build 13724695073

Details

30 of 39 (76.92%) changed or added relevant lines in 3 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.009%) to 79.678%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/uri/url_ext.rs	27	31	87.1%
payjoin/src/uri/error.rs	1	6	16.67%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/send/mod.rs	2	93.91%

Totals
Change from base Build 13709521679:	0.009%
Covered Lines:	4560
Relevant Lines:	5723

💛 - Coveralls

arminsabouri · 2025-02-28T18:58:09Z

payjoin/src/uri/error.rs

                write!(f, "Endpoint scheme is not secure (https or onion)")
            }
+            LowercaseEndpointFragment =>
+                write!(f, "Some or all of the endpoint fragement is lowercase"),


small typo:

Suggested change

write!(f, "Some or all of the endpoint fragement is lowercase"),

write!(f, "Some or all of the endpoint fragment is lowercase"),

arminsabouri · 2025-02-28T18:59:49Z

payjoin/src/uri/url_ext.rs

+        let uri = "bitcoin:12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX?amount=0.01\
+                   &pjos=0&pj=HTTPS://EXAMPLE.COM/\
+                   %23oh1qypm5jxyns754y4r45qwe336qfx6zr8dqgvqculvztv20tfveydmfqc";
+        assert!(Uri::try_from(uri).is_err(), "{:?}", Uri::try_from(uri).err());


Nit: I ussually advocate to check for exact error variant you are expecting. This case being LowercaseEndpointFragment

Since this is an internal error, do you have a good method of checking the error without changing it to be a public trait? Or is it ok to string compare on the err message?

Looks like this comment is relevant to this as well.
#542 (comment)

I ussually #[derive(PartialEq)] on the error enum and that works fine. Sometimes other error variants can't derive PartialEq easily and that becomes a headache. If it blows up the scope too much then last resort error string checking can work fine

Ok, I'll try that now

spacebear21

ACK 9ff4690

DanGould

ACK 9ff4690

Note that when we implement public error enums #403 this test should be modified to check against the variant rather than the message.

DanGould · 2025-03-04T17:38:24Z

payjoin/src/uri/mod.rs

+                if let Some(fragment) = url.fragment() {
+                    if fragment.chars().any(|c| c.is_lowercase()) {
+                        return Err(InternalPjParseError::LowercaseEndpointFragment.into());
+                    }
+                };


Hol' up. This is v2 specific and probably doesn't belong here

DanGould · 2025-03-04T17:50:42Z

Upon review, and to stop an error variant from being introduced that would only be used on v2 codepaths, I'm wondering if the BadEndpoint variant should be used for both Url::parse error and lowercase error. It could encapsulate String context that either propagated the url::ParseError message or the "lowercase

It might make sense to introduce url_ext::parse_with_fragment function that does this check instead of defining it in the Uri mod since it's related explicitly to the v2 UrlExt parsing. It could also parse the receiver_pubkey and ohttp at that time instead of requiring those errors to be handled when requests are extracted. @spacebear21 seeking your feedback.

I'm more convince that this belongs in url_ext but not necessarily convinced this also touches the fragment parameter extraction API.

DanGould · 2025-03-04T17:58:10Z

I think something like this would make more sense. Whether or not we need to preserve content in BadEndpoint is up for debate. could we just point to the BIP Url spec for details of a proper endpoint format in the error message?

// uri/mod.rs
// ...

#[cfg(not(feature = "v2"))]
let url = Url::parse(&endpoint).map_err(|_| InternalPjParseError::BadEndpoint)?;
#[cfg(feature = "v2")]
let url = url_ext::parse_with_fragment(&endpoint).map_err(|_| InternalPjParseError::BadEndpoint)?;

error handling omitted for brevity and my ability to respond quickly

// uri/url_ext.rs
// ...

/// Parse and validate the fragment parameters from `&pj=` URI parameter URLs
pub fn parse_with_fragment(input: &str) -> Result<Url, ()> {
    let url = Url::parse(input).map_err(|_| ())?;

    if let Some(fragment) = url.fragment() {
        if fragment.chars().any(|c| c.is_lowercase()) {
            return Err(());
        }
    };
    Ok(url)
}

DanGould

Getting a lot closer. I think there's still some abstraction leakiness and I do my best in review to guide you through how a significant portion of this design can be elided to fix it.

DanGould · 2025-03-06T22:39:05Z

payjoin/src/uri/mod.rs

 use std::borrow::Cow;

 use bitcoin::address::NetworkChecked;
+use error::BadEndpointError;


Just a note This'd let you use it without a a prefix elsewhere in the module.

Suggested change

use error::BadEndpointError;

pub(crate) use error::BadEndpointError;

But I don't actually think you want to do this. I think if you follow my later suggestions, BadEndpointError can stay referenced in the url_ext and error modules only since you don't need to do manual coercion in mod if you just propagate it to an InternalPjParseError::BadEndpoint variant.

DanGould · 2025-03-06T22:40:05Z

payjoin/src/uri/error.rs

+#[derive(Debug)]
+pub enum BadEndpointError {
+    UrlParse(ParseError),
+    FragmentParse(ParseEndpointFragmentError),


Rather than import at the top level, I think we want to feature gate this line (and the line in the Display impl) and fully qualify the type, if we even need multiple variants in the FragmentParse variant.

Suggested change

FragmentParse(ParseEndpointFragmentError),

#[cfg(feature = "v2")]

FragmentParse(super::url_ext::ParseEndpointFragmentError),

DanGould · 2025-03-06T22:42:15Z

payjoin/src/uri/url_ext.rs

+#[cfg(feature = "v2")]
+#[derive(Debug)]
+pub enum ParseEndpointFragmentError {
+    LowercaseEndpointFragment,
+}
+
+#[cfg(feature = "v2")]
+impl std::fmt::Display for ParseEndpointFragmentError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        use ParseEndpointFragmentError::*;
+
+        match &self {
+            LowercaseEndpointFragment =>
+                write!(f, "Some or all of the endpoint fragment is lowercase"),
+        }
+    }
+}
+


Since this only has one variant, I'm not convinced we need to complicate it. We can probably just use the one BadEndpoint::FragmentParse variant or rename it BadEndpoint::LowercaseEndpointFragment and simplify by deleting this.

DanGould · 2025-03-06T22:45:45Z

payjoin/src/uri/mod.rs

+                let url = url_ext::parse_with_fragment(&endpoint).map_err(|_| {
+                    InternalPjParseError::BadEndpoint(BadEndpointError::FragmentParse(
+                        ParseEndpointFragmentError::LowercaseEndpointFragment,
+                    ))


The BadEndpointError is being discarded, but it conveys useful information that needs to be propagated. parse_with_fragment can fail either with LowercaseEndpointFragment OR with a UrlParse error here. The following would let that propagate:

Suggested change

let url = url_ext::parse_with_fragment(&endpoint).map_err(|_| {

InternalPjParseError::BadEndpoint(BadEndpointError::FragmentParse(

ParseEndpointFragmentError::LowercaseEndpointFragment,

))

let url = url_ext::parse_with_fragment(&endpoint).map_err(InternalPjParseError::BadEndpoint)

DanGould

One last thing

DanGould · 2025-03-07T16:03:01Z

payjoin/src/uri/error.rs

+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            BadEndpointError::UrlParse(err) => write!(f, "Invalid URL: {:?}", err),
+            BadEndpointError::LowercaseFragment =>


If it's feature gated above we need a gate here in order to build v1 with --no-default-features -- which may be otherwise broken because we're only testing checks with --all-features in CI now, whoops!

Suggested change

BadEndpointError::LowercaseFragment =>

#[cfg(feature = "v2")]

BadEndpointError::LowercaseFragment =>

don't think this was resolved

That's weird its not refreshing here,

rust-payjoin/payjoin/src/uri/error.rs

Lines 27 to 28 in b8e9768

#[cfg(feature = "v2")]

BadEndpointError::LowercaseFragment =>

I think I just spammed 2 rebases too quickly for github to make sense of it

DanGould · 2025-03-07T16:03:22Z

payjoin/src/uri/error.rs

            MissingEndpoint => write!(f, "Missing payjoin endpoint"),
            NotUtf8 => write!(f, "Endpoint is not valid UTF-8"),
-            BadEndpoint => write!(f, "Endpoint is not valid"),
+            BadEndpoint(err) => write!(f, "Endpoint is not valid: {:?}", err),


nit: prefer e to err here by custom elsewhere in the repo

The pj= param in the pjuri contains multiple different parts but there exist some gotchas when trying to use them as the url path can be parsed even when lowercase but the fragment object cannot. This means that it is possible to copy or display a pjuri that will not work if it is simply converted to lowercase. This pr does not create any method to force the fragment to be uppercase as I think it really should be on the implementation to make sure that casing is correct, so I just created the case error catch.

DanGould

ACK b8e9768

Let's go Ben 🚀

spacebear21

ACK b8e9768

I noticed when reviewing #558 that some intended feature combinations were broken. In particular tests didn't catch that a `v2` feature gate on an error variant would break if the `impl Display` for that variant wasn't also gated. This led me to notice that `cargo check --no-default-features --features v1` was also failing because of unavailability of `thread_rng`. The first commit addresses the build failure in independent `v1` build, the second creates checks for `payjoin`. close #566

arminsabouri reviewed Feb 28, 2025

View reviewed changes

benalleng force-pushed the pjuri-fragments-lowercase branch 2 times, most recently from a480aba to 9ff4690 Compare March 3, 2025 22:31

spacebear21 approved these changes Mar 4, 2025

View reviewed changes

DanGould approved these changes Mar 4, 2025

View reviewed changes

DanGould requested changes Mar 4, 2025

View reviewed changes

benalleng force-pushed the pjuri-fragments-lowercase branch from e344ea8 to a68ebfc Compare March 6, 2025 17:07

DanGould requested changes Mar 6, 2025

View reviewed changes

benalleng force-pushed the pjuri-fragments-lowercase branch 2 times, most recently from 9eb8cda to 3a0f17b Compare March 7, 2025 14:33

DanGould requested changes Mar 7, 2025

View reviewed changes

benalleng force-pushed the pjuri-fragments-lowercase branch from 3a0f17b to ead292f Compare March 7, 2025 16:12

benalleng force-pushed the pjuri-fragments-lowercase branch from ead292f to b8e9768 Compare March 7, 2025 16:13

DanGould approved these changes Mar 7, 2025

View reviewed changes

DanGould mentioned this pull request Mar 7, 2025

Independent features must be checked in CI #566

Closed

DanGould requested a review from spacebear21 March 7, 2025 16:25

spacebear21 approved these changes Mar 7, 2025

View reviewed changes

DanGould merged commit 0ff8258 into payjoin:master Mar 7, 2025
7 checks passed

DanGould mentioned this pull request Mar 8, 2025

Check independent features #567

Merged

	write!(f, "Some or all of the endpoint fragement is lowercase"),
	write!(f, "Some or all of the endpoint fragment is lowercase"),

	use error::BadEndpointError;
	pub(crate) use error::BadEndpointError;

	FragmentParse(ParseEndpointFragmentError),
	#[cfg(feature = "v2")]
	FragmentParse(super::url_ext::ParseEndpointFragmentError),

	BadEndpointError::LowercaseFragment =>
	#[cfg(feature = "v2")]
	BadEndpointError::LowercaseFragment =>

Conversation

benalleng commented Feb 28, 2025

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 13724695073

Details

💛 - Coveralls

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

DanGould left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

DanGould left a comment

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!

Choose a reason for hiding this comment

Uh oh!

DanGould left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanGould left a comment

Choose a reason for hiding this comment

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Feb 28, 2025 •

edited

Loading

DanGould left a comment •

edited

Loading

DanGould commented Mar 4, 2025 •

edited

Loading

DanGould commented Mar 4, 2025 •

edited

Loading