Fix deleting all folder messages on 'select all' when the search context is lost - #10255
Fix deleting all folder messages on 'select all' when the search context is lost#10255TowyTowy wants to merge 1 commit into
Conversation
alecpl
left a comment
alecpl
left a comment
There was a problem hiding this comment.
We have the same issue in delete, move and mark actions too (maybe more). So, I think we should move the check to index.php and bail out whenever search is requested but does not exist, and _action is not empty (or if it is an ajax action, I'm not sure which would be better).
| // has been overwritten by a search in another browser tab). Otherwise | ||
| // the '*' would resolve to the whole folder and delete all its messages | ||
| // instead of only the search result (#9671). | ||
| if ($search_request && $_POST['_uid'] === '*' && !$rcmail->storage->get_search_set()) { |
There was a problem hiding this comment.
The second condition can be removed. It does not matter what _uid is.
|
Moved the check into |
6585a58 to
3592f84
Compare
| elseif ( | ||
| !empty($_REQUEST['_search']) | ||
| && !empty($rcmail->action) | ||
| && !in_array($rcmail->action, ['autocomplete', 'list-contacts', 'search-contacts', 'refresh', 'check-recent', 'compose']) |
There was a problem hiding this comment.
_search in refresh, check-recent and compose is the same search, so they should not be excluded.
| // only to get back to the search result view. | ||
| elseif ( | ||
| !empty($_REQUEST['_search']) | ||
| && !empty($rcmail->action) |
|
|
||
| ## Unreleased | ||
|
|
||
| - Reject mail actions referencing a search context that no longer exists in the session, e.g. deleting a search result with "select all" could delete all messages in the folder (#9671) |
There was a problem hiding this comment.
Please, remove the changelog entry.
…le folder (roundcube#9671) When the client sends a _search request parameter but the referenced search context does not exist in the session anymore (e.g. it has been overwritten by a search in another browser tab), mail actions fall back to operating on the whole folder. E.g. deleting a search result with "select all" (_uid=*) resolves '*' against the folder instead of the search set and deletes every message in the folder. Add a central guard in rcmail_action_mail_index::run(), which runs before every mail request, that bails out with an error whenever a search context is requested but not available. This covers delete, move, mark and all other actions operating on a search result. Only autocomplete, list-contacts and search-contacts are excluded, there _search is not a message search reference, but an addressbook search string or a reference to $_SESSION['contact_search']. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Done — dropped the action requirement and removed the changelog entry. Also took refresh, check-recent and compose out of the exclusions per your other comment; only autocomplete, list-contacts and search-contacts are left, where |
3592f84 to
b6c8756
Compare
2 participants
Closes #9671.
Problem
When a search result is deleted using "select all", the client sends
_uid=*together with_search=<id>. The delete handler resolves*against the storage search set. If the search context is no longer in the session — e.g. it was overwritten by a new search in another browser tab —rcmail_action_mail_index::run()does not callset_search_set(), so the storage object has no search set.rcube_storage::parse_uids('*')then falls back to1:*and every message in the folder is deleted instead of only the (now lost) search result.This matches the analysis in #9671:
Fix
Bail out with an error in the delete handler when a
_uid=*request is bound to a search (_searchpresent) but no search set exists on the storage object. Single-message and explicit-UID deletes are unaffected, and a normal "select all" in a non-search folder view (no_search) still works.Tests
Added a regression test in
tests/Actions/Mail/DeleteTest.phpthat mocks a missing search set and asserts the action aborts without callingdelete_message(). It fails on current master (the un-guarded code callsdelete_message('*'), i.e. the whole-folder delete) and passes with this change. CHANGELOG entry included.To reproduce manually: open a search result with more than one message; in a second tab run a different search (overwriting the session search state); back in the first tab, Select all → Delete. Without this change the whole folder is emptied.
Disclosure: this fix was prepared with the assistance of an AI coding agent (Claude) and reviewed before submission.