fix(admin_ui): refuse a delete whose Admin History write failed - #6436
Open
e107help[bot] wants to merge 2 commits into
Open
e107help[bot] wants to merge 2 commits into
e107help[bot] wants to merge 2 commits into
Conversation
Admin History's delete archive holds every stored column of the record, the password hash included, since #6243 widened it so that restoring a delete puts the record back whole (#6118). The list renders that archive and the area exports it as CSV, and any administrator granted the History permission reached both. Deltik's call on #6244 is to narrow who may read it rather than to trim the archive or redact the export, because either of those loses the restore. So the page gates on the main-administrator check, the admin navigation link carries the same code, and the permission that used to open it leaves the grantable list. Nothing else wanted it: its only other reader was the News navigation entry, and newspost.php has never accepted it. The failed-insert path printed the whole record to the screen with print_a(), which is the same disclosure by another route on a site whose in-place upgrade has not created admin_history yet. The table, the record id and the database error stay; the payload goes. Fixes #6244
Both delete triggers in the admin UI archive the stored row to admin_history and then remove it, and both threw away what the archive write answered. An INSERT that failed left the administrator with two error messages and no record: the row was gone and the archive that was meant to hold it was never written. The commonest cause is a site upgraded in place whose core database update has not been run, so admin_history is not there yet and every delete through those triggers destroyed the only copy of the row. The archive now decides whether the delete happens. The guard that both triggers spelled out identically moves into archiveBeforeDelete(), which answers true where there is nothing to archive (the History area clearing its own rows, or a row that is not there), reports the refusal where the write failed, and is the single place that decision is made. The batch path skips a refused row before beforeDelete(), so it counts towards neither the deleted nor the not-found total; the single path returns before the node split, so neither the ordinary delete nor the fall-back for the missing node runs. The gate tests the archive's answer for truth rather than against false. backupToHistory() is protected and a subclass may override it, but it has never been in a tagged release, so nothing shipped can be relying on an override that answers null, and permitting the delete on anything but a written archive is the behaviour this commit exists to remove. Two failures reach the refusal, and only one of them has a remedy. A missing admin_history is created by the core database update or by Admin > Database > verify. A row whose stored bytes are not valid UTF-8 cannot be encoded at all, and now cannot be deleted through the admin UI either; the schema is already correct, so no verify helps. That trade sits on the same population as the first one, and it is the reason this is a pull request rather than a merge. Three neighbouring holes in the same audit trail are left open, each needing a different fix and a decision of its own: _manageSubmit() archives an update after the save, e107_admin/history.php archives before a restore and overwrites regardless, and the Custom Pages menu delete removes a page row with a query of its own and reaches no trigger at all. An administrator on a site whose admin_history table is missing can now delete nothing through those triggers until that table exists. That is the trade: an audit trail that can be skipped by breaking it is not one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Deleting a record in an admin area archives the stored row to
admin_historyand then deletes it. Both delete triggers threw away what the archive write answered, so anINSERTthat failed took the row with it anyway and left the administrator two error messages and nothing holding the record:The commonest way to reach it is a site upgraded in place whose core database update has not been run yet.
admin_historyis declared incore_sql.php, and the table is created by the core-database update routine, which is Admin > Database > Check for Updates and then the row Update core database structure, and by Admin > Database > Check database validity. Until one of those has run, every delete in every admin area reportedFailed to save history for table 'user', record ID 42and deleted the row regardless.(#6412's description said no named update routine creates the table. That was wrong:
core_databaseis registered as a core routine and its fix pass creates a missing table. The description is corrected there.)Deltik's call on #6412 is to refuse the delete and show the error, rather than delete anyway and log it.
What Changed
The guard both triggers spelled out identically moves into one
archiveBeforeDelete()beside the archive helpers, and its answer now decides whether the delete happens. It permits the delete where there is nothing to archive, which is the History area clearing its own rows and a row that is not there, and reports the refusal where the write failed.beforeDelete(), so it counts towards neither the deleted nor the not-found totalLAN_UI_DELETE_REFUSED_NO_ARCHIVEnames the record that was kept, beside the failurebackupToHistory()already reportsThe gate tests that answer for truth rather than against
false.backupToHistory()isprotectedand a subclass may override it, but it has never been in a tagged release, so nothing shipped can be relying on an override that answersnull, and permitting a delete on anything short of a written archive is the behaviour this exists to remove.How It Was Tested
Four tests in
adminUiHistorySnapshotTest, each proven red by reverting the handler and re-running:delete()call the confirm screen never reachesEach asserts on the message the administrator reads, not merely that some error exists. The tests that fix the archive's shape stay green unchanged, including the one that proves the History area can still clear its own rows.
The whole unit suite ran on PHP 8.5: 2735 tests, no failures.
Backwards Compatibility
Nothing rendered on the front end changes.
What a site will notice, and it is the decision rather than a side effect: on a site whose
admin_historytable is missing, every admin area now refuses every delete through these two triggers until the table exists. Before this change those deletes went through and were not archived. An upgraded site is already prompted to run the update that creates it, so that administrator has a way out from the admin area itself.The second refusal has no way out, and it is the one worth a decision before this merges.
backupToHistory()also answersfalsewhenjson_encode()cannot encode the row, which happens for any stored byte that is not valid UTF-8. A site whose connection sends noSET NAMESand whose columns hold latin1 text receives those bytes raw, and from this change on, every row carrying an accented character in that state is undeletable through the admin UI. The schema is already correct, so no database check repairs it. Three ways that could go, none of them free:JSON_INVALID_UTF8_SUBSTITUTE) so the archive always encodes. The row stays deletable, and the archive then holds U+FFFD where the characters were, which a restore would write back into the live table.INSERTand let an unencodable row delete unarchived, which is the behaviour A failed admin_history write does not stop the delete it was archiving #6412 asked to remove.Deltik's call. This is not folded in either way.
Three neighbouring holes in the same audit trail are left open, because each needs a different fix and a decision of its own. Both of the first two are filed:
_manageSubmit()archives an update after the save, ande107_admin/history.phparchives before a restore and overwrites regardless; both discard the answerpagerow with a query of its own and reaches no trigger, so it has never archived at allMerge order
This branch is cut from #6411's head, because that PR changes the same error path and removes the screen dump this replaces. Merge #6411 first, then this one. Until #6411 lands, the diff here carries its commit as well.
AI Model
Claude Opus 5 (claude-opus-5), as e107help.
Checklist
Fixes #6412