security(hardening): cacti_form_action_label() and html_escape graph_list (1.2.x)#7171
Merged
TheWitness merged 7 commits intoJun 4, 2026
Conversation
html_start_box() does not escape its title argument. Five pages
(vdef, user_admin, user_domains, user_group_admin, automation_tree_rules)
passed $actions[get_nfilter_request_var('drp_action')] as the title
without pre-validating drp_action. When drp_action is not a key in the
array, PHP emits an 'Undefined array key' warning whose message contains
the raw value unescaped — producing reflected XSS when display_errors
is enabled.
Add cacti_form_action_label(array $actions, $default = '') to
lib/functions.php. It validates drp_action via get_filter_request_var()
before the array lookup, so the access always uses a clean alphanumeric
value. Apply it to all 23 html_start_box() call sites that used the
raw nfilter pattern.
Also fix graphs.php: html_escape() the graph_list POST parameter
before emitting it into the drp_action=9 confirmation body (reflected
XSS, GHSA-jgh9-9j94-92p2).
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Hardens form action handling against reflected XSS that occurred when drp_action was used directly as an array key (an undefined-key warning containing the raw value could be echoed into the HTML). Introduces a helper cacti_form_action_label() and routes 22 admin form pages through it; also escapes graph_list output on graphs.php.
Changes:
- Add
cacti_form_action_label()helper inlib/functions.phpthat validatesdrp_actionviaget_filter_request_varbefore indexing the actions array. - Replace
$actions[get_nfilter_request_var('drp_action')]withcacti_form_action_label($actions)across all admin pages. - Escape
get_nfilter_request_var('graph_list')output ingraphs.php.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/functions.php | New helper that validates drp_action and returns the matching label. |
| graphs.php | html_escape applied to graph_list value output. |
| aggregate_templates.php, automation_graph_rules.php, automation_networks.php, automation_snmp.php, automation_templates.php, automation_tree_rules.php, cdef.php, color.php, color_templates.php, data_input.php, data_queries.php, data_sources.php, graph_templates.php, host_templates.php, managers.php, pollers.php, sites.php, tree.php, user_admin.php, user_domains.php, user_group_admin.php, vdef.php | Switched action title lookup to the new helper to avoid the undefined-key warning XSS sink. |
…ion_label signature
html_escape(get_nfilter_request_var('graph_list')) at drp_action=9 escaped
the <li> HTML the variable is expected to contain, breaking the aggregate
graph UI. Use the pre-built $graph_list PHP variable instead, which is
already safe (titles escaped via html_escape at construction time).
cacti_form_action_label now accepts $drp_action as an explicit parameter
instead of calling get_filter_request_var() internally. This removes the
side effect on $_REQUEST and eliminates the implicit die_html_input_error()
path from inside a label-lookup helper. All 22 call sites updated to pass
get_nfilter_request_var('drp_action') directly.
Add string type hints to $drp_action and $default parameters and add
return type declaration for consistency with adjacent helpers.
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
aggregate_graphs, graphs, automation_devices, gprint_presets, host, data_templates, data_source_profiles, links, and html_reports were missed in the previous pass. All now use cacti_form_action_label() instead of direct array key access on unvalidated drp_action input. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
- Wrap cacti_form_action_label() with html_escape() at all 32 html_start_box() call sites so plugin-contributed labels cannot inject HTML. - Add html_escape() to 10 hidden <input name='drp_action'> fields that emitted get_request_var() raw into HTML attribute context. - links.php: use get_request_var() in the label lookup since get_filter_request_var() already ran above it. - graphs.php:1293: use get_request_var() for $save['drp_action'] instead of get_nfilter_request_var() which reads raw from $_REQUEST. - lib/functions.php: rewrite cacti_form_action_label() docblock to accurately describe the safety model and require html_escape() on output. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
A request like drp_action[]=x reaches the helper on the four pages that do not pre-validate it (user_admin, user_group_admin, vdef, user_domains), where the string typehint raised an uncaught TypeError. Accept any type and return the default for non-scalar keys. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The graph_list interpolation change left the prior single-quote close, leaving the string opened at the <p> segment unterminated and breaking graphs.php with a parse error. Close with a double quote to match the sibling itemlist blocks. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Per review: rename cacti_form_action_label() to escape_page_action(), escape the label inside it, and drop the outer html_escape() at every call site. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
TheWitness
approved these changes
May 30, 2026
TheWitness
approved these changes
Jun 4, 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.
Hardening for the bulk-action confirmation pages on 1.2.x (post-authentication).
Adds
cacti_form_action_label(array $actions, $drp_action, string $default = '')tolib/functions.php, which returns a server-defined label for the selected action (or the default), and routes all 23 action-dropdown call sites through it so every page renders the confirmation title via the validated path. Also routes thegraph_listconfirmation output through the same per-item escaping the sibling itemlist blocks already use.Verified against the current 1.2.x tip with the project's test suite.