fix: remove premature delete_persistent_permissions before backend validation#2001
fix: remove premature delete_persistent_permissions before backend validation#2001herrschmidt wants to merge 1 commit into
Conversation
|
Can you expand more on why things fail?
How does this cause restoration to fail? |
…validation
The original code deleted the restore_token from the PermissionStore
in replace_restore_token_with_data() BEFORE the backend validated the
restore data. If the backend failed to restore (e.g. monitor mismatch
after logout/login on GNOME/Wayland), the token was already consumed
and no new token was generated — persist_mode=2 silently degraded.
This fix moves the deletion to after the backend response:
1. replace_restore_token_with_data(): Look up token but DON'T delete.
Pass restore_data to the backend for validation.
2. replace_restore_data_with_token(): After backend responds:
- If restore_data returned: generate_and_save_restore_token()
overwrites the old token with the same or a new one.
- If restore_data NOT returned (e.g. persist_mode=NONE):
delete the persistent permission here (new else-branch cleanup).
Previously the else-branch only freed the pointer, relying on the
now-removed early delete for cleanup. The updated else-branch now
calls delete_persistent_permissions() to properly clean up when the
backend does not return restore_data.
Fixes: flatpak#1862
Related: rustdesk/rustdesk#9435, flatpak#1371
b606237 to
b401f1b
Compare
|
Hi, this is the failure mechanism: The bug is a two-stage consume-before-validate race: Stage 1 — Token consumed prematurely:
Stage 2 — Backend validation fails (silently):
So Why this breaks persistence across sessions:
Within a single session, this works because the monitor is still connected with the same identifier — The fix: The delete is moved to after the backend response. If the backend successfully restores, the old token entry is simply overwritten. If it fails (or |
whot
left a comment
whot
left a comment
There was a problem hiding this comment.
Note on the commit message: xdp doesn't use conventional commits, better teach the agent that.
| xdp_session_persistence_delete_persistent_permissions (session, | ||
| table, | ||
| *in_out_restore_token); | ||
|
|
There was a problem hiding this comment.
ok, probably too niche to worry about but: if the restore_data comes from the transient permissions we now pass the token into persistent permissions to delete associated data. Assuming the token is unique enough this shouldn't be an issue but feels off anyway.
Also not immediately sure why the same issue doesn't affect transient permissions but I've paged much of this out by now.
|
Re-reading this, it also seems problematic that portal-gnome doesn't seem to handle the restore token correctly in find_monitor_by_string. Can you please report this as well? Anyway, the commit message needs updating (right prefix, line length) and the comments are too verbose, the typical AI slop. |
4 participants
Problem
restore_tokenwithpersist_mode=2(PERSISTENT) fails after logout/login — the portal dialog reappears on every session restart. The token works within a session, but not across sessions.Reported in:
Root Cause
In
xdp_session_persistence_replace_restore_token_with_data(), therestore_tokenis deleted from the PermissionStore before the backend (e.g. gnome-screencast) validates the restore data. The critical code is insrc/xdp-session-persistence.c:232-237.Fix
Remove the premature
delete_persistent_permissions()call (3 lines). The delete is unnecessary because:Start()generates a fresh tokengenerate_and_save_restore_token()already handles cleanupStart()anywayChange: 3 lines removed from
src/xdp-session-persistence.c.Tested
Related