Repository navigation
security: move map navigation handlers to CSP-safe bindings - #141
Merged
Merged
Conversation
Cacti's Content-Security-Policy script-src-attr directive blocks inline
event-handler attributes. The map region navigation's "Start Over" and
"Print" buttons (gpsmap_render_region in includes/polling/processregion.php)
used inline onclick="window.location.reload(true)" and
onclick="window.open('print.php')" handlers.
They now carry gpsmapStartOver / gpsmapPrint classes and are wired via
delegated jQuery bindings in js/GPSMaps.js, which is already included on every
page via the page_head hook. No i18n calls changed, so cacti.pot is untouched.
TheWitness
requested review from
bmfmancini,
browniebraun and
xmacan
and
a balanced review from Copilot
October 7, 2026 21:38
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The security-sensitive generated markup lacks regression assertions preventing inline handlers from returning.
2 open findings
What changed in this PR
Moves map navigation actions from inline handlers to CSP-safe delegated jQuery bindings.
Changes:
- Replaces inline
onclickattributes with dedicated CSS classes. - Adds delegated handlers for reload and print actions.
- Documents the security change.
| File | Description |
|---|---|
js/GPSMaps.js |
Adds delegated navigation handlers. |
includes/polling/processregion.php |
Emits CSP-safe button markup. |
CHANGELOG.md |
Records the CSP cleanup. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Adds an expect()-based test that renders gpsmap_render_region() and asserts the Start Over/Print controls use delegated jQuery classes (gpsmapStartOver/gpsmapPrint) with no inline onclick. Satisfies the changed-line coverage gate for processregion.php, which the existing assert_*-helper tests (flagged risky) do not record.
Addresses review feedback: indent the chained delegated click bindings and their callback bodies to reflect nesting, convert jQuery() to the \ shorthand, and add a trailing newline.
bmfmancini
approved these changes
Oct 8, 2026
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 map region navigation's inline event-handler attributes so the plugin complies with Cacti's Content-Security-Policy
script-src-attrdirective. Part of the fleet-wide CSP inline-handler cleanup.Changes
gpsmap_render_region) — the "Start Over" and "Print" buttons drop their inlineonclick="window.location.reload(true)"/onclick="window.open('print.php')"handlers and carrygpsmapStartOver/gpsmapPrintclasses instead.clickbindings for those classes.GPSMaps.jsis already included on every page via thepage_headhook, and delegation ondocumenthandles the AJAX-injected region body.Gate / tests
processregion.phpis a measured source file;gpsmap_render_regionis executed many times bytests/Integration/PollingTest.php(and the changed lines are unconditional in that render path), so the changed lines stay covered. No test asserts the button markup, and no__esc()/i18n calls changed, socacti.potis untouched.