fix(auth): validate measure/order before ORDER BY concatenation in get_allowed_graphs() (#7438)

* fix(auth): validate measure/order before ORDER BY concatenation in get_allowed_graphs()

get_allowed_graphs() built its dsstats ORDER BY clause by concatenating
$sql_order['measure'] and $sql_order['order'] directly into SQL. Both
values trace back to unfiltered request variables (grv('measure'),
grv('graph_order')) at several call sites in html_graph.php and
html_tree.php, none of which register the vars in a filter array.
Reachable pre-auth via graph_view.php when dsstats_enable and the guest
account are on.

Add get_dsstats_order_string() in lib/html_utility.php, next to the
existing sort_column/sort_direction validator get_order_string(), and
have get_allowed_graphs() call it instead of trusting the raw array.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* refactor(auth): validate dsstats measure/order via CactiValidator + backed enum

get_dsstats_order_string() used a hand-rolled in_array() whitelist instead
of this codebase's established CactiValidator/Symfony Assert pattern
(~334 existing call sites). Adds a DsstatsMeasure backed enum as the
single source of truth for legal measure values, feeding an
Assert\Choice constraint checked via CactiValidator::isValid() (not
validateInput(), which writes to $_SESSION and isn't appropriate for
this pure helper).

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* fix: guard dsstats order/measure against array input and missing require

Addresses Copilot review: require html_utility.php where
get_dsstats_order_string() is called from lib/auth.php, and reject
non-scalar measure/order values before validating/concatenating them.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* test(auth): add integration coverage for dsstats ORDER BY fragment build

Exercises get_dsstats_order_string() together with dsstats_get_best_partition()
and db_qstr(), the other real functions get_allowed_graphs() calls on the
same path, reproducing the JOIN + ORDER BY fragment it concatenates.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* style(auth): order use import before require_once in html_utility.php

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>

lib/auth.php

@@ -1481,7 +1481,7 @@ function get_allowed_tree_header_graphs(int $tree_id, int $leaf_id = 0, string $
  *                          'start_time'  => unix_timestamp,
  *                          'end_time'    => unix_timestamp,
  *                          'cf'          => avg (0) | max (1)
- *                          'measure'     => average | peak | sum | p25 | p50 | p75 | p90 | p95
+ *                          'measure'     => average | peak | sum | p25n | p50n | p75n | p90n | p95n
  *                      )
  *
  * @param string $sql_limit   SQL LIMIT clause.
@@ -1520,6 +1520,7 @@ function get_allowed_graphs(string $sql_where = '', mixed $sql_order = 'gtg.titl
 
 	if (is_array($sql_order)) {
 		require_once(CACTI_PATH_LIBRARY . '/dsstats.php');
+		require_once(CACTI_PATH_LIBRARY . '/html_utility.php');
 
 		$table = dsstats_get_best_partition($sql_order['start_time'], $sql_order['end_time']);
 
@@ -1545,7 +1546,7 @@ function get_allowed_graphs(string $sql_where = '', mixed $sql_order = 'gtg.titl
 			) AS rs
 			ON gl.id = rs.local_graph_id";
 
-			$sql_order = 'ORDER BY rs.' . $sql_order['measure'] . ' ' . $sql_order['order'];
+			$sql_order = get_dsstats_order_string($sql_order);
 		} elseif ($sql_order['data_source'] == '') {
 			cacti_log('WARNING: Graph Order missing Data Source name for ordering.', false, 'AUTH');
 			cacti_log('ORDER, DETAIL: ' . json_encode($sql_order), false, 'AUTH');

lib/html_utility.php

@@ -22,6 +22,10 @@
  +-------------------------------------------------------------------------+
 */
 
+use Symfony\Component\Validator\Constraints as Assert;
+
+require_once __DIR__ . '/CactiValidator.php';
+
 /**
  * Replaces all variables contained in $form_array with their actual values
  *
@@ -1428,6 +1432,52 @@ function get_order_string() : string {
 	return 'ORDER BY ' . $del . implode($del . '.' . $del, explode('.', $sort_column)) . $del . ' ' . $sort_dir;
 }
 
+/**
+ * Backing enum for the dsstats measure columns that get_dsstats_order_string()
+ * is allowed to sort by. Cases are the single source of truth for both the
+ * enum and the Assert\Choice constraint's `choices` list below, so a new
+ * measure only needs to be added in one place.
+ */
+enum DsstatsMeasure : string {
+	case Average = 'average';
+	case Peak    = 'peak';
+	case P25n    = 'p25n';
+	case P50n    = 'p50n';
+	case P75n    = 'p75n';
+	case P90n    = 'p90n';
+	case P95n    = 'p95n';
+	case Sum     = 'sum';
+}
+
+/**
+ * Builds the validated ORDER BY clause used by get_allowed_graphs() when
+ * sorting graphs by a dsstats measure (see the 'rs' derived table it joins
+ * in). The measure and order values come from unauthenticated request
+ * variables ('measure', 'graph_order'), so they are checked against the
+ * actual data_source_stats_* columns and asc/desc before being concatenated
+ * into SQL, the same way get_order_string() checks sort_column/sort_direction.
+ *
+ * @param array $sql_order Expects 'measure' and 'order' keys; see
+ *                         get_allowed_graphs() for the full array shape.
+ *
+ * @return string The 'ORDER BY rs.<measure> <ASC|DESC>' clause.
+ */
+function get_dsstats_order_string(array $sql_order) : string {
+	$measure_choices = array_map(static fn (DsstatsMeasure $case) => $case->value, DsstatsMeasure::cases());
+
+	$measure = isset($sql_order['measure']) && is_scalar($sql_order['measure']) && CactiValidator::isValid($sql_order['measure'], [new Assert\Choice(choices: $measure_choices)])
+		? $sql_order['measure']
+		: DsstatsMeasure::Average->value;
+
+	$order = isset($sql_order['order']) && is_scalar($sql_order['order']) ? strtoupper((string) $sql_order['order']) : '';
+
+	$direction = CactiValidator::isValid($order, [new Assert\Choice(choices: ['ASC', 'DESC'])])
+		? $order
+		: 'ASC';
+
+	return 'ORDER BY rs.' . $measure . ' ' . $direction;
+}
+
 /**
  * Removes a specified column from the order string in the session data.
  *

tests/Unit/DsstatsOrderStringTest.php

@@ -0,0 +1,84 @@
+<?php
+/*
+ +-------------------------------------------------------------------------+
+ | Copyright (C) 2004-2026 The Cacti Group                                 |
+ |                                                                         |
+ | This program is free software; you can redistribute it and/or           |
+ | modify it under the terms of the GNU General Public License             |
+ | as published by the Free Software Foundation; either version 2          |
+ | of the License, or (at your option) any later version.                  |
+ +-------------------------------------------------------------------------+
+ | Cacti: The Complete RRDtool-based Graphing Solution                     |
+ +-------------------------------------------------------------------------+
+*/
+
+/*
+ * Regression tests for get_dsstats_order_string() (lib/html_utility.php),
+ * the ORDER BY builder used by get_allowed_graphs() (lib/auth.php) when
+ * sorting graphs by a dsstats measure.
+ *
+ * Before this fix, get_allowed_graphs() concatenated $sql_order['measure']
+ * and $sql_order['order'] straight into an ORDER BY clause. Both values
+ * originate from unfiltered request variables (grv('measure'),
+ * grv('graph_order')) reachable pre-auth through graph_view.php when the
+ * guest account is enabled and dsstats_enable is 'on'.
+ */
+
+require_once dirname(__DIR__, 2) . '/lib/html_utility.php';
+
+test('valid measure and order pass through unchanged', function () {
+	expect(get_dsstats_order_string(['measure' => 'p95n', 'order' => 'desc']))
+		->toBe('ORDER BY rs.p95n DESC');
+});
+
+test('every legal measure value is accepted', function () {
+	foreach (['average', 'peak', 'p25n', 'p50n', 'p75n', 'p90n', 'p95n', 'sum'] as $measure) {
+		expect(get_dsstats_order_string(['measure' => $measure, 'order' => 'asc']))
+			->toBe('ORDER BY rs.' . $measure . ' ASC');
+	}
+});
+
+test('order is case-insensitive', function () {
+	expect(get_dsstats_order_string(['measure' => 'sum', 'order' => 'DESC']))->toBe('ORDER BY rs.sum DESC');
+	expect(get_dsstats_order_string(['measure' => 'sum', 'order' => 'Desc']))->toBe('ORDER BY rs.sum DESC');
+	expect(get_dsstats_order_string(['measure' => 'sum', 'order' => 'desc']))->toBe('ORDER BY rs.sum DESC');
+	expect(get_dsstats_order_string(['measure' => 'sum', 'order' => 'ASC']))->toBe('ORDER BY rs.sum ASC');
+});
+
+test('unrecognised measure falls back to average instead of reaching SQL', function () {
+	$clause = get_dsstats_order_string(['measure' => 'average; DROP TABLE user_auth-- ', 'order' => 'asc']);
+
+	expect($clause)->toBe('ORDER BY rs.average ASC')
+		->and($clause)->not->toContain(';')
+		->and($clause)->not->toContain('--');
+});
+
+test('UNION SELECT payload in measure is rejected', function () {
+	$clause = get_dsstats_order_string([
+		'measure' => '1) UNION SELECT password FROM user_auth-- ',
+		'order'   => 'asc',
+	]);
+
+	expect($clause)->toBe('ORDER BY rs.average ASC')
+		->and($clause)->not->toContain('UNION')
+		->and($clause)->not->toContain('user_auth');
+});
+
+test('subquery payload in measure is rejected', function () {
+	$clause = get_dsstats_order_string([
+		'measure' => '(SELECT password FROM user_auth LIMIT 1)',
+		'order'   => 'asc',
+	]);
+
+	expect($clause)->toBe('ORDER BY rs.average ASC')
+		->and($clause)->not->toContain('SELECT');
+});
+
+test('SQL payload in order falls back to ASC', function () {
+	expect(get_dsstats_order_string(['measure' => 'sum', 'order' => 'asc; DROP TABLE user_auth-- ']))
+		->toBe('ORDER BY rs.sum ASC');
+});
+
+test('missing keys default to a safe clause', function () {
+	expect(get_dsstats_order_string([]))->toBe('ORDER BY rs.average ASC');
+});

tests/integration/DsstatsOrderIntegrationTest.php

@@ -0,0 +1,121 @@
+<?php
+/*
+ +-------------------------------------------------------------------------+
+ | Copyright (C) 2004-2026 The Cacti Group                                 |
+ |                                                                         |
+ | This program is free software; you can redistribute it and/or           |
+ | modify it under the terms of the GNU General Public License             |
+ | as published by the Free Software Foundation; either version 2          |
+ | of the License, or (at your option) any later version.                  |
+ +-------------------------------------------------------------------------+
+ | Cacti: The Complete RRDtool-based Graphing Solution                     |
+ +-------------------------------------------------------------------------+
+*/
+
+/*
+ * Integration coverage for the get_allowed_graphs() dsstats ORDER BY path
+ * (lib/auth.php). tests/Unit/DsstatsOrderStringTest.php already covers
+ * get_dsstats_order_string() in isolation; this test drives it together
+ * with the other real functions get_allowed_graphs() calls on the same
+ * code path (dsstats_get_best_partition() from lib/dsstats.php and
+ * db_qstr() from lib/database.php), reproducing the exact JOIN + ORDER BY
+ * fragment that gets concatenated into the graph list query, so a
+ * regression that reintroduces unescaped concatenation anywhere along
+ * that path (not just inside get_dsstats_order_string() itself) is
+ * caught. Calling get_allowed_graphs() itself would require a live DB
+ * connection (auth_valid_user(), read_config_option(), db_fetch_assoc()),
+ * which this test suite does not provision.
+ */
+
+require_once dirname(__DIR__, 2) . '/lib/database.php';
+require_once dirname(__DIR__, 2) . '/lib/dsstats.php';
+require_once dirname(__DIR__, 2) . '/lib/html_utility.php';
+
+/*
+ * Rebuilds the $sql_order_join / $sql_order fragment exactly as
+ * get_allowed_graphs() does for the is_array($sql_order) branch
+ * (lib/auth.php), so the test exercises the real call sequence rather
+ * than a paraphrase of it.
+ */
+function build_dsstats_order_fragment(array $sql_order) : array {
+	$table = dsstats_get_best_partition($sql_order['start_time'], $sql_order['end_time']);
+
+	$cf = $sql_order['cf'] == 'max' ? 1 : 0;
+
+	$sql_order_join = "INNER JOIN
+		(
+			SELECT DISTINCT gti.local_graph_id, ot.*
+			FROM graph_templates_item AS gti
+			INNER JOIN data_template_rrd AS dtr
+			ON gti.task_item_id = dtr.id
+			INNER JOIN $table AS ot
+			ON dtr.local_data_id = ot.local_data_id
+			WHERE ot.rrd_name = " . db_qstr($sql_order['data_source']) . "
+			AND ot.cf = $cf
+		) AS rs
+		ON gl.id = rs.local_graph_id";
+
+	return [$sql_order_join, get_dsstats_order_string($sql_order)];
+}
+
+test('malicious data_source is escaped, not concatenated, in the dsstats JOIN clause', function () {
+	[$join] = build_dsstats_order_fragment([
+		'start_time'  => time(),
+		'end_time'    => time(),
+		'data_source' => "traffic_in' AND 1=(SELECT password FROM user_auth LIMIT 1)-- ",
+		'cf'          => 'avg',
+		'measure'     => 'average',
+		'order'       => 'asc',
+	]);
+
+	// db_qstr() must have backslash-escaped the embedded quote so the
+	// payload stays inside the string literal instead of closing it.
+	expect($join)->toContain("\\'")
+		->and($join)->not->toContain("rrd_name = 'traffic_in' AND");
+});
+
+test('malicious measure/order survive the full order-fragment build as a safe clause', function () {
+	[, $order] = build_dsstats_order_fragment([
+		'start_time'  => time(),
+		'end_time'    => time(),
+		'data_source' => 'traffic_in',
+		'cf'          => 'avg',
+		'measure'     => '1) UNION SELECT password FROM user_auth-- ',
+		'order'       => 'asc; DROP TABLE user_auth-- ',
+	]);
+
+	expect($order)->toBe('ORDER BY rs.average ASC')
+		->and($order)->not->toContain('UNION')
+		->and($order)->not->toContain('DROP')
+		->and($order)->not->toContain(';');
+});
+
+test('a quote-breakout data_source combined with a statement-terminator measure stays inert in both halves of the fragment', function () {
+	[$join, $order] = build_dsstats_order_fragment([
+		'start_time'  => time(),
+		'end_time'    => time(),
+		'data_source' => "x'; DROP TABLE user_auth; -- ",
+		'cf'          => 'max',
+		'measure'     => 'p95n; DROP TABLE user_auth-- ',
+		'order'       => 'desc',
+	]);
+
+	// db_qstr() escapes the embedded quote, so "DROP TABLE" stays inert
+	// string data inside 'x\'; DROP TABLE user_auth; -- ' rather than
+	// breaking out into executable SQL. What must NOT happen is an
+	// *unescaped* quote reaching the JOIN, or the measure's payload
+	// reaching the ORDER BY at all.
+	expect($join)->toContain("x\\'; DROP TABLE user_auth; -- ")
+		->and($join)->not->toContain("rrd_name = 'x';");
+
+	expect($order)->toBe('ORDER BY rs.average DESC')
+		->and($order)->not->toContain('DROP')
+		->and($order)->not->toContain(';');
+});
+
+test('lib/auth.php builds the dsstats ORDER BY via get_dsstats_order_string(), not raw concatenation', function () {
+	$contents = file_get_contents(dirname(__DIR__, 2) . '/lib/auth.php');
+
+	expect($contents)->toContain('get_dsstats_order_string($sql_order)')
+		->and($contents)->not->toContain("'ORDER BY rs.' . \$sql_order['measure']");
+});