Repository navigation
fix: require POST for monitor state changes and check dashboard ownership on save - #280
Merged
TheWitness merged 2 commits intoOct 23, 2026
Conversation
csrf-magic only validates POST, so save, saveDb, remove, the mute toggles and the card-order save now fall through to the normal page draw when sent by GET, and monitor.js sends remove and the mute toggles as POST with the CSRF token. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
saveSettings() took the owner from an unvalidated user request variable and let sql_save() overwrite any dashboard id. It now always uses the session user and refuses to update a dashboard the user does not own, matching removeDashboard() and saveCardOrder(). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
TheWitness
approved these changes
Oct 23, 2026
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.
Description
State-changing actions (
save,saveDb,remove,ajax_mute_all,ajax_unmute_all,ajax_saveorder) now require POST so csrf-magic covers them.monitor.phpdispatches through a newmonitorRequestAction()helper, which turns those actions sent by any other method into a plain page draw.monitor.jsnow sends remove and the mute toggles as POST with__csrf_magic. The other three actions already did.saveSettings()now always saves as the session user and no longer reads auserrequest variable. Updating a dashboard the user does not own is refused with the existing "could not be Saved!" message, now raised as an error, the same ownership ruleremoveDashboard()andsaveCardOrder()already apply. A hand-built&user=0URL can therefore no longer create a shared (user_id0) dashboard. No UI ever sentuser. An admin-gated way to create shared dashboards can follow as a separate change.Related Issue
Related to #211 (addresses part of the audit; does not close it).
Motivation and Context
These are the only monitor endpoints that change stored state, and they were the ones outside csrf-magic's POST check. The save path was also the only dashboard write without an owner check.
Risk: small. A user can no longer save over a shared or foreign dashboard; "save as new" still works. If a dashboard is left open past Cacti's 2-hour CSRF token lifetime, Mute needs a second click. saveFilter and saveDb already behave this way. Read-only actions (
ajax_status,ajax_hostpanel,dbchange) still accept GET. There are no new strings and no schema change.How Has This Been Tested?
New Pest tests cover GET, POST and a missing request method for each of the six actions, and saveSettings with owned, foreign and shared dashboards and with a forged
user. 4 of the 5 saveSettings tests fail on develop. Full suite: 148 passed (420 assertions) on PHP 8.4.25 and 8.5.12.php -lis clean on PHP 8.2 and 8.4. The patch-coverage, i18n POT and manifest checks pass locally. Withincludes/controller.phpadded to the coverage source, its changed lines are 100% covered.Types of changes
Checklist: