Repository navigation
security: move filter inline onChange handlers into ready() for CSP - #90
Merged
Merged
Conversation
The graph-export filter's Rows and refresh-interval selects carried inline onChange='applyFilter()' attributes. Under Cacti's Content-Security-Policy these trip the script-src-attr directive (inline event handlers). Remove them and bind both selects via change in the existing ready block (the refresh select was previously bound via click, which does not reliably fire on a select, and the rows select was unbound). Regenerated locales/po/cacti.pot for the shifted source line references. gexport.php is already in the patch-coverage gate's allowlist. The cactiReturnTo() cancel buttons are left inline (shared Cacti-core pattern, tracked separately).
TheWitness
requested review from
bmfmancini,
browniebraun and
xmacan
and
a balanced review from Copilot
October 7, 2026 20:14
There was a problem hiding this comment.
🟢 Approval recommended
The event bindings preserve filter behavior while eliminating the reported CSP violation.
0 open findings
What changed in this PR
Removes CSP-incompatible inline filter handlers and binds select changes through the existing jQuery ready block.
Changes:
- Bind Rows and Refresh selects using
.change(). - Remove inline
onChangeattributes. - Update changelog and gettext metadata.
| File | Description |
|---|---|
gexport.php |
Moves filter event handling into nonce-protected JavaScript. |
locales/po/cacti.pot |
Regenerates translation template metadata. |
CHANGELOG.md |
Documents the CSP fix. |
🧠 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.
bmfmancini
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 inline
onChangeevent handlers from the graph-export filter so the page no longer violates Cacti's Content-Security-Policyscript-src-attrdirective (continuing the CSP cleanup across the plugin fleet).Changes
#rows) and refresh-interval (#refresh) selects had inlineonChange='applyFilter()'. Removed them and bound both via.change()in the existing ready block (the refresh select was previously bound via.click(), which does not reliably fire on a<select>, and the rows select was unbound).locales/po/cacti.potfor the shifted source line references (msgid set unchanged).gexport.phpis already in the patch-coverage gate's allowlist (web UI entry point). ThecactiReturnTo()cancel buttons on confirmation pages are left inline — shared Cacti-core pattern, handled separately.