Add completed_event_id FK to prevent session replay by Mshehu5 · Pull Request #1158 · payjoin/rust-payjoin

Mshehu5 · 2025-10-09T02:38:13Z

This PR adds completed_event_id foreign key columns to both send_sessions and receive_sessions tables. This column references the exact event row that closed the session.

Closes #1146

Replaced completed_at columns with completed_event_id foreign keys
Updated close() methods to validate if completed_event_id is None
Modified inactive session queries to derive timestamps from closed event

Please confirm the following before requesting review:

I have disclosed my use of
AI
in the body of this PR.
I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

coveralls · 2025-10-09T02:41:04Z

Pull Request Test Coverage Report for Build 19371495926

Details

31 of 46 (67.39%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.07%) to 83.508%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/db/error.rs	0	3	0.0%
payjoin-cli/src/db/v2.rs	27	39	69.23%

Totals
Change from base Build 18946545260:	-0.07%
Covered Lines:	9008
Relevant Lines:	10787

💛 - Coveralls

zealsham · 2025-10-09T11:51:23Z

Thank you for taking on this @Mshehu5 . on first pass , this works as intended

spacebear21

The application should just need to properly implement the close() method to set the column appropriately, as close() is called by the payjoin state machine when appropriate; the application should never call save_event() directly. This makes me realize the documentation around the SessionPersister methods needs to be clearer.

payjoin-cli/src/app/v2/mod.rs

payjoin-cli/src/db/v2.rs

arminsabouri

Not a huge fan of using string matching to find the "closed" event and left some other suggestions.

payjoin-cli/src/db/mod.rs

payjoin-cli/src/db/v2.rs

arminsabouri

Its not clear to me what problem the changes in payjoin/src/core/send/v2 and its corresponding change in payjoin-cli are solving. They seem unrelated to the original ticket and should be moved to a different PR once its clear what the problem actually is.

arminsabouri · 2025-10-22T18:43:42Z

payjoin-cli/src/db/v2.rs

-            "UPDATE send_sessions SET completed_at = ?1 WHERE session_id = ?2",
-            params![now(), *self.session_id],
+            "UPDATE send_sessions SET completed_at = ?1, completed_event_id = ?2 WHERE session_id = ?3",
+            params![now(), completed_event_id, *self.session_id],


What happens is completed_event_id is None? There should will always be a session event before close is called. Should we make this expectation explicit?

payjoin-cli/src/app/v2/mod.rs

Mshehu5 · 2025-10-23T17:46:25Z

Its not clear to me what problem the changes in payjoin/src/core/send/v2 and its corresponding change in payjoin-cli are solving. They seem unrelated to the original ticket and should be moved to a different PR once its clear what the problem actually is.

understood, seems the reason I made the change in payjoin/src/core/send/v2 have been addressed in PR #1171 I will take a better approach in issues like this next time

arminsabouri

Just a few more things. Mainly noticing some unrelated changes

payjoin-cli/src/db/v2.rs

arminsabouri · 2025-10-31T16:53:25Z

payjoin-cli/src/db/v2.rs

        let conn = self.db.get_connection()?;
        let mut stmt = conn.prepare(
-            "SELECT event_data FROM send_session_events WHERE session_id = ?1 ORDER BY created_at ASC",
+            "SELECT event_data FROM send_session_events WHERE session_id = ?1 ORDER BY id ASC",


Since the ID is always auto-incremeneting, i think this would work. But why make this change? It seems unrelated to original intent of the PR. AFAICT created_at should still be a field on send_session_events

The reason for this change is I noticed events saved within the same second share the same created_at (seconds precision), making created_at non-deterministic and causing out-of-order result

I aslo wanted to add DB transaction to make it more atomic but I think it will be better for a followup PR as it is out of scope

Thanks for the explanation. In the future lets break this kind of change into its own commit please or a different PR. It would help speed up the review process.

Do we think that this is worth making its own issue?

yes! thanks for catching that

payjoin-cli/src/db/v2.rs

zealsham · 2025-10-09T11:55:46Z

payjoin-cli/src/db/v2.rs

+        if matches!(event, SenderSessionEvent::Closed(_)) {
+            conn.execute(
+                "UPDATE send_sessions SET completed_at = ?1, completed_event_id = ?2 WHERE session_id = ?3",
+                params![now(), event_id, *self.session_id],


The time here would be inacurrate for when the actual session event was created . No need to call now() again here . Rather use the time when the actual session_event was created as this just tells us the time this DB entry was made

Thanks for the review Zealsham!
It seems your comments are based on an older version of this PR. I’d really appreciate it if you could take a look at the latest changes as well.

arminsabouri

CAck 37aa822

arminsabouri · 2025-11-10T13:08:46Z

payjoin-cli/src/db/v2.rs

        let conn = self.db.get_connection()?;
        let mut stmt = conn.prepare(
-            "SELECT event_data FROM send_session_events WHERE session_id = ?1 ORDER BY created_at ASC",
+            "SELECT event_data FROM send_session_events WHERE session_id = ?1 ORDER BY id ASC",


Thanks for the explanation. In the future lets break this kind of change into its own commit please or a different PR. It would help speed up the review process.

spacebear21

cACK, but I think the query for completed_event_id in the close() methods needs to be more specific than just "the last event". Otherwise we risk ending up out of sync if e.g. close() is called prematurely, then completed_event_id would point to a non-Closed event.

spacebear21 · 2025-11-10T16:48:03Z

payjoin-cli/src/db/v2.rs

+        let completed_event_id: Option<i64> = conn
+            .query_row(
+                "SELECT id FROM receive_session_events 
+             WHERE session_id = ?1 
+             ORDER BY id DESC LIMIT 1",
+                params![*self.session_id],
+                |row| row.get(0),
+            )
+            .ok();


AFAICT this is just querying for the "last event" ID, not a completed event specifically? I think it needs to explicitly filter for "Closed" session events and error if one is not found.

Add completed_event_id foreign key columns to both send_sessions and receive_sessions tables that reference the exact event that closed the session to replace completed_at column.

spacebear21 · 2025-11-19T14:51:26Z

payjoin-cli/src/db/v2.rs

+        let completed_event_id: i64 = match conn.query_row(
+            "SELECT id FROM send_session_events 
+             WHERE session_id = ?1 
+             AND json_extract(event_data, '$.Closed') IS NOT NULL


Interesting, I wasn't familiar with the json_extract syntax. Is there a reason we couldn't do simple string matching? e.g.

Suggested change

AND json_extract(event_data, '$.Closed') IS NOT NULL

AND event_data = 'Closed'

or

Suggested change

AND json_extract(event_data, '$.Closed') IS NOT NULL

AND event_data LIKE '%Closed'

I think using a string patter like "Closed" will be easily subject to bugs. And it prevents us from using the string "Closed" in a future session events for a not so obvious reason. Can we assume the last event will always be a closed event if close is being called? If so I would just order all the session events and limit to 1 to take the last event.

This comment is from Armin that’s why I didn’t use string matching. I had originally done it that way, but changed it based on his feedback.

I see, apologies I had missed that earlier piece of review. @arminsabouri my concern was that if we simply query the last event, then we run the risk of closing a session that never recorded a Closed event and erroneously marking it as closed (the user can call close arbitrarily).

I agree string matching is not very robust, but fwiw json_extract still uses string matching.

Based on my research json_extract(event_data, '$.Closed') isn’t just string matching. the SQLite query parses the JSON and only returns a value if there’s a top-level "Closed" key, so it won’t match strings like "closed", "ClosedEvent", or "NotClosed" that may appear elsewhere in the payload unlike %Closed% which just matches anything containing the substring "Closed"

I also pushed a change which handles matching Closed event in Rust code see https://github.com/payjoin/rust-payjoin/compare/735d4f5c5b75a466754283e6f1f646650df421b6..6dccbd4441c59dfda9a48263c0462a891b489108

If this approach or any alternative is more favorable I can make those changes

arminsabouri · 2025-11-20T15:26:04Z

@Mshehu5 Thank you for spending time to explore this problem space and present great solutions! However, after looking at the current state of this PR (and alternatives paths that you have dillegently proposed) I don't think the added complexity is worth it. Specifically, the string-matching approach doesn’t set a good precedent for applications that will model their behavior after pj-cli. The best path forward is to close the ticket as won't be fixed.

Mshehu5 marked this pull request as ready for review October 9, 2025 09:45

spacebear21 requested changes Oct 9, 2025

View reviewed changes

payjoin-cli/src/app/v2/mod.rs Outdated Show resolved Hide resolved

payjoin-cli/src/db/v2.rs Outdated Show resolved Hide resolved

Mshehu5 force-pushed the completed_at_fk branch 3 times, most recently from c0fd47a to de46c09 Compare October 10, 2025 22:07

DanGould requested a review from arminsabouri October 13, 2025 17:43

arminsabouri requested changes Oct 13, 2025

View reviewed changes

payjoin-cli/src/db/mod.rs Outdated Show resolved Hide resolved

payjoin-cli/src/db/mod.rs Outdated Show resolved Hide resolved

payjoin-cli/src/db/v2.rs Outdated Show resolved Hide resolved

Mshehu5 force-pushed the completed_at_fk branch 2 times, most recently from fbe10e6 to 4866994 Compare October 15, 2025 18:20

Mshehu5 requested a review from arminsabouri October 18, 2025 14:15

Mshehu5 force-pushed the completed_at_fk branch from 4866994 to b7c8741 Compare October 22, 2025 10:24

arminsabouri requested changes Oct 22, 2025

View reviewed changes

Mshehu5 force-pushed the completed_at_fk branch 4 times, most recently from dad306d to f02b04b Compare October 27, 2025 16:04

Mshehu5 requested a review from arminsabouri October 27, 2025 16:09

Mshehu5 force-pushed the completed_at_fk branch from f02b04b to 622a6c6 Compare October 27, 2025 16:12

arminsabouri requested changes Oct 31, 2025

View reviewed changes

Mshehu5 force-pushed the completed_at_fk branch from 622a6c6 to 37aa822 Compare October 31, 2025 19:51

zealsham reviewed Nov 2, 2025

View reviewed changes

arminsabouri approved these changes Nov 10, 2025

View reviewed changes

arminsabouri requested a review from spacebear21 November 10, 2025 13:13

spacebear21 requested changes Nov 10, 2025

View reviewed changes

Mshehu5 force-pushed the completed_at_fk branch from 37aa822 to 735d4f5 Compare November 10, 2025 23:52

Add completed_event_id FK to prevent session replay

6dccbd4

Add completed_event_id foreign key columns to both send_sessions and receive_sessions tables that reference the exact event that closed the session to replace completed_at column.

Mshehu5 force-pushed the completed_at_fk branch from 735d4f5 to 6dccbd4 Compare November 14, 2025 16:50

spacebear21 reviewed Nov 19, 2025

View reviewed changes

arminsabouri closed this Nov 20, 2025

This was referenced Nov 24, 2025

Loading events should use auto-incrementing id instead of created_at #1201

Closed

Sort session events by auto-incrementing id #1202

Merged

	AND json_extract(event_data, '$.Closed') IS NOT NULL
	AND event_data = 'Closed'

	AND json_extract(event_data, '$.Closed') IS NOT NULL
	AND event_data LIKE '%Closed'

Conversation

Mshehu5 commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Pull Request Test Coverage Report for Build 19371495926

Details

💛 - Coveralls

Uh oh!

zealsham commented Oct 9, 2025

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Mshehu5 commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arminsabouri 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!

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!

arminsabouri left a comment

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!

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 Nov 19, 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!

Choose a reason for hiding this comment

Uh oh!

arminsabouri commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Mshehu5 commented Oct 9, 2025 •

edited

Loading

coveralls commented Oct 9, 2025 •

edited

Loading

Mshehu5 commented Oct 23, 2025 •

edited

Loading

spacebear21 Nov 19, 2025 •

edited

Loading

arminsabouri commented Nov 20, 2025 •

edited

Loading