Skip to content

Fix PR review issues from PR #812 #823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Mshehu5 wants to merge 1 commit into from
Closed

Fix PR review issues from PR #812 #823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion payjoin/src/core/receive/v2/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,14 +902,16 @@ pub(crate) fn pj_uri<'a>(
session_context: &SessionContext,
output_substitution: OutputSubstitution,
) -> crate::PjUri<'a> {
use bitcoin_uri::Uri;

use crate::uri::{PayjoinExtras, UrlExt};
let id = session_context.id();
let mut pj = subdir(&session_context.directory, &id).clone();
pj.set_receiver_pubkey(session_context.s.public_key().clone());
pj.set_ohttp(session_context.ohttp_keys.clone());
pj.set_exp(session_context.expiry);
let extras = PayjoinExtras { endpoint: pj, output_substitution };
bitcoin_uri::Uri::with_extras(session_context.address.clone(), extras)
Uri::with_extras(session_context.address.clone(), extras)
}

#[cfg(test)]
Expand Down
19 changes: 14 additions & 5 deletions payjoin/src/core/receive/v2/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::output_substitution::OutputSubstitution;
use crate::persist::SessionPersister;
use crate::receive::v2::{extract_err_req, SessionError};
use crate::receive::{v1, JsonReply};
use crate::{ImplementationError, IntoUrl, PjUri, Request};
use crate::{ImplementationError, IntoUrl, PjUri, Request, Version};

/// Errors that can occur when replaying a receiver event log
#[derive(Debug)]
Expand Down Expand Up @@ -78,11 +78,20 @@ pub struct SessionHistory {
impl SessionHistory {
/// Receiver session Payjoin URI
pub fn pj_uri<'a>(&self) -> Option<PjUri<'a>> {
self.events.iter().find_map(|event| match event {
SessionEvent::Created(session_context) =>
Some(crate::receive::v2::pj_uri(session_context, OutputSubstitution::Disabled)),
let session_context = self.events.iter().find_map(|event| match event {
SessionEvent::Created(session_context) => Some(session_context),
_ => None,
})
})?;
let mut output_substitution = OutputSubstitution::Enabled;
for event in &self.events {
if let SessionEvent::UncheckedProposal((unchecked_proposal, _)) = event {
if unchecked_proposal.params.v == Version::One {
output_substitution = OutputSubstitution::Disabled;
}
break;
}
}
Comment on lines +85 to +93
Copy link
Collaborator

@arminsabouri arminsabouri Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind adding a test below to prevent future regressions?

Some(crate::receive::v2::pj_uri(session_context, output_substitution))
Comment on lines +85 to +94
Copy link
Collaborator

@nothingmuch nothingmuch Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately there is a bug (#843) that appears to have caused some confusion with this behavior

pj_uri should not depend at all on the events, it should return what the receiver generated at the session initialization, and there output substitution is always disabled, because a malicious directory can reply to bip 78 senders with malicious proposal PSBTs that steal the intended receiver's payment.

since v2 messages are end to end encrypted with integrity protection, a v2 receiver receiving from a v2 sender can substitute outputs even though it specified pjos=0 in the uri, so this code fragment checking the event data is correct for determining the correct behavior for the receiver, it's just not the right value to use when constructing pj_uri.

Feel free to fix this bug here, but then please edit the pull request title and description accordingly

Suggested change
let mut output_substitution = OutputSubstitution::Enabled;
for event in &self.events {
if let SessionEvent::UncheckedProposal((unchecked_proposal, _)) = event {
if unchecked_proposal.params.v == Version::One {
output_substitution = OutputSubstitution::Disabled;
}
break;
}
}
Some(crate::receive::v2::pj_uri(session_context, output_substitution))
Some(crate::receive::v2::pj_uri(session_context, OutputSubstitution::Disabled))

Copy link
Collaborator

@nothingmuch nothingmuch Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also I guess this method should be documented to say that it returns the pj_url that was sent to the sender, and to note that although pjos=0 is set, this does not disable output substitution if the sender also supports bip 77

Copy link
Collaborator

@nothingmuch nothingmuch Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #847

}

/// Fallback transaction from the session if present
Expand Down
Loading