Repository navigation
security: move filter inline onChange/onClick handlers into ready() for CSP - #849
TheWitness wants to merge 3 commits into
Conversation
…or CSP
The threshold, notification list, notification queue, template and
device/log status filter forms still carried inline onChange/onClick
attributes on their selects and Clear/Go/Import/Export controls. Under
Cacti's Content-Security-Policy these trip the script-src-attr directive
(inline event handlers). Bind them in each view's existing
$(function(){...}) ready block instead.
Scope: thold.php, notify_queue.php, thold_templates.php, notify_lists.php,
thold_graph.php (all gate-allowlisted UI files; no measured source changed).
The cactiReturnTo() cancel buttons on the bulk-action confirmation pages are
left as-is (shared Cacti-core pattern). Regenerated locales/po/cacti.pot for
the shifted source line references.
There was a problem hiding this comment.
🟡 Changes recommended
Two Clear buttons no longer work because their new handlers target missing element IDs.
1 open finding
What changed in this PR
Moves filter event handlers into CSP-compatible jQuery ready blocks.
Changes:
- Replaces inline filter and action handlers across five UI pages.
- Regenerates translation metadata and updates the changelog.
- Two notification-list Clear buttons lack the IDs required by their new handlers.
| File | Description |
|---|---|
thold.php |
Moves threshold filter handlers. |
thold_templates.php |
Moves template filter/action handlers. |
thold_graph.php |
Moves threshold, device, and log handlers. |
notify_queue.php |
Moves queue filter handlers. |
notify_lists.php |
Moves notification-list handlers. |
locales/po/cacti.pot |
Updates generation timestamp. |
CHANGELOG.md |
Documents the CSP cleanup. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The confirmation pages' Cancel buttons used an inline onClick='cactiReturnTo()' handler, which trips Cacti's Content-Security-Policy script-src-attr directive. Switch them to the CSP-safe cactiReturnTo CSS class (class='cactiReturnTo', plus an optional data-url when a target is passed). Cacti 1.2.31 and later bind this class automatically. The README documents a one-time include/layout.js applySkin() snippet for operators still on an earlier release - no shim is baked into the core or the plugin.
…handler binds The hosts() and tholds() filter views used name='clear' on the Clear button, but the ready() handler binds to #clear, so Clear was a no-op on those two views. Switch them to id='clear' to match the other views and the handler.
TheWitness
left a comment
TheWitness
left a comment
There was a problem hiding this comment.
Review
The CSP cleanup looks correct and consistent with the plugin_hmib #64 pattern. Inline onChange/onClick attributes are removed from the filter controls and re-bound in each view's $(function(){...}) ready block, and the cactiReturnTo cancel buttons are converted to the CSP-safe .cactiReturnTo class (with data-url where a return target is passed). No measured source (setup.php, includes/functions.php) is touched, so the patch-coverage gate is unaffected as described.
One issue found and fixed (f87b1d1)
The Copilot reviewer correctly flagged that in notify_lists.php the hosts() (Associated Devices) and tholds() filter views kept name='clear' on the Clear button while the new ready handler binds $('#clear').click(...), so Clear was a no-op on those two views. Changed both to id='clear' to match the handler and the templates/lists views. That thread is resolved.
Verification
php -l notify_lists.php— clean.- Spot-checked the other ready blocks: every selector they bind (
#rfilter,#site_id,#template,#state,#rows,#host_template_id,#associated,#clear,#refresh,#export,#import,#data_template_id,#data_source_id) now has a matching elementid.
The deliberately-deferred items (core cactiReturnTo() cancel buttons on the bulk confirmation pages, and the setup.php edit-dialog handlers behind the coverage gate) are reasonable to leave as follow-ups.
Summary
Removes the inline
onChange/onClickevent handlers from the Thold filter forms so they no longer violate Cacti's Content-Security-Policyscript-src-attrdirective, continuing the CSP cleanup (same class of issue fixed in plugin_hmib #64). The handler functions already existed and each view already had a$(function(){...})ready block binding its form submit — this just moves the remaining filter controls into those blocks.Changes
#site_id,#thold_template_id,#data_template_id,#state,#rows→applyFilter();#clear→clearFilter().applyFilter(),#clear→clearFilter(),#export→exportLog().applyFilter(),#clear→clearFilter().#topic/#processed/#rows→applyFilter().applyFilter('dt'/'ds')) and the templates list (#rows/#refresh/#clear/#import).locales/po/cacti.potfor the shifted source line references (msgid set unchanged).Notes
$unmeasured_allowlist; no measured source (setup.php,includes/functions.php) is touched, so the changed-line coverage gate is unaffected.cactiReturnTo()cancel buttons on the bulk delete/duplicate/associate confirmation pages are intentionally left inline — that's the shared Cacti-core cancel pattern, not a Thold-specific filter control.setup.phpstill has a few inline handlers on the device/template edit dialogs (addThresholdTemplate(), dialog close). Those live in a coverage-measured file whose UI functions have no unit tests, so converting them would fail the 100% changed-line gate without added coverage — deliberately deferred as a separate follow-up.