Repository navigation
security: move inline event handlers to CSP-safe bindings - #287
Merged
Merged
Conversation
Cacti's Content-Security-Policy script-src-attr directive blocks inline event-handler attributes. This converts the plugin's remaining inline handlers: - Confirmation Cancel buttons (flowview_databases.php, flowview_filters.php, flowview_schedules.php) switch from onClick='cactiReturnTo()' to the cactiReturnTo class; flowview_devices.php's Cancel button drops its onClick='document.location=...' for the cactiReturnTo class + data-url. - The database page's source/rows/verified/version selects drop inline onChange='applyFilter()' and are bound via change() in each ready block. - The main filter's report/sort/cutoff selects, the predefined-timespan select and the time-shift icons (flowview_display_filter in includes/functions.php) drop their inline onChange/onclick and are bound in the function's ready block via delegated change()/click() calls to the same applyFilter/ applyTimespan/timeshift functions they already used. includes/functions.php is measured; a new FlowviewFilterControlsTest exercises flowview_display_filter() so the changed time-shift lines stay covered. The web UI entry points are already in the patch-coverage allowlist. No i18n calls changed, so cacti.pot is untouched. Cacti 1.2.31+ binds the cactiReturnTo class automatically; the README documents a one-time applySkin() snippet for earlier releases.
TheWitness
requested review from
bmfmancini,
browniebraun and
xmacan
and
a balanced review from Copilot
October 7, 2026 22:25
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new test cannot load its target function, and supported Cacti 1.2.29–1.2.30 installations receive inert Cancel buttons without a core patch.
3 open findings
What changed in this PR
Moves inline event handlers to CSP-safe JavaScript bindings across FlowView.
Changes:
- Rebinds filter controls and time-shift actions through JavaScript.
- Converts Cancel buttons to
cactiReturnTo. - Adds compatibility documentation, tests, and changelog entry.
| File | Description |
|---|---|
CHANGELOG.md |
Records the CSP cleanup. |
README.md |
Documents pre-1.2.31 compatibility steps. |
includes/functions.php |
Rebinds main filter controls. |
flowview_databases.php |
Rebinds database filters and Cancel action. |
flowview_devices.php |
Converts the Cancel action. |
flowview_filters.php |
Converts the Cancel action. |
flowview_schedules.php |
Converts the Cancel action. |
tests/Unit/FlowviewFilterControlsTest.php |
Adds filter-control regression coverage. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…shift controls flowview_display_filter() requires includes/arrays.php, which re-keys its device/template lookups with Cacti core's array_rekey(). The stub test bootstrap does not define it, so the require threw 'Call to undefined function array_rekey()' before any markup was emitted and the timeshiftBackward/Forward assertions saw empty output. Add a function_exists-guarded array_rekey stub mirroring core behaviour.
bmfmancini
approved these changes
Oct 8, 2026
bmfmancini
left a comment
bmfmancini
left a comment
Member
There was a problem hiding this comment.
Reviewed; CSP-safe bindings look correct (Cancel uses cactiReturnTo class, Continue still submits). Checks pass.
browniebraun
approved these changes
Oct 8, 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.
Summary
Removes the plugin's remaining inline event-handler attributes so pages comply with Cacti's Content-Security-Policy
script-src-attrdirective. Part of the fleet-wide CSP inline-handler cleanup.Changes
onClick='cactiReturnTo()'→cactiReturnToclass. flowview_devices.php —onClick='document.location="flowview_devices.php"'→cactiReturnToclass +data-url.#source/#rows/#verified/#version) — drop inlineonChange='applyFilter()', bound viachange()in each of the three ready blocks.flowview_display_filterin includes/functions.php) — the report/sort/cutoff selects, the predefined-timespan select, and the time-shift icons drop their inlineonChange/onclickand are bound in the function's own ready block via delegatedchange()/click()calls to the sameapplyFilter/applyTimespan/timeshiftFilterLeft/Rightfunctions they already invoked.Gate / tests
includes/functions.phpis measured; the time-shift<i>lines carry a<?php>so they count. A new tests/Unit/FlowviewFilterControlsTest.php rendersflowview_display_filter()to keep those lines covered. The web-UI entry points are already in the patch-coverage allowlist. No__()/__esc()calls changed, socheck-i18n-pot.phppasses without regeneratingcacti.pot.Compatibility
Cacti 1.2.31+ binds the
cactiReturnToclass automatically. The README documents a one-timeapplySkin()snippet for earlier releases. No shim is baked into core or the plugin.