fix(defensive): harden helper library input and output handling (#7197)

* fix(defensive): harden helper library input and output handling

lib/html_utility.php: update_order_string() wrote sort_direction from
the request directly into the session without enforcing ASC/DESC.
Normalise to strtoupper and allowlist-check before every session write;
default to ASC for any unrecognised value.

lib/functions.php:
- sanitize_uri(): add protocol-relative URL guard (leading '//' after
  ltrim). The function stripped most dangerous chars but had no entry
  for '//', which browsers treat as a scheme-relative redirect target.
- is_valid_pathname(): the regex allows literal dots so '../..' passed
  validation. Reject any path containing '..' before the regex check.

lib/database.php: db_qstr() fallback (no PDO connection) was missing
\x1a (Ctrl+Z) from its escape map. PDO quote() handles it; the fallback
now matches.

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

* fix(defensive): correct hardening edge cases flagged in review

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

* fix(defensive): reject control-char redirect and drive-relative traversal

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

* ci: ignore phpstan function.alreadyNarrowedType identifier

Clears the level 6 alreadyNarrowedType error on lib/data_query.php:2591 that
develop currently emits, matching the sibling narrowing identifiers already ignored.

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

---------

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

lib/database.php

@@ -2325,7 +2325,7 @@ function db_qstr(mixed $s, mixed $db_conn = false) : string {
 		return $db_conn->quote($s);
 	}
 
-	$s = str_replace(['\\', "\0", "'"], ['\\\\', "\\\0", "\\'"], $s);
+	$s = str_replace(['\\', "\0", "\x1a", "'"], ['\\\\', "\\\0", '\\Z', "\\'"], $s);
 
 	return "'" . $s . "'";
 }

lib/functions.php

@@ -2378,6 +2378,13 @@ function strip_alpha(mixed $string) : mixed {
  * @return bool Either true or false
  */
 function is_valid_pathname($path) {
+	/* treat ':' as a boundary as well so the Windows drive-relative form
+	 * "C:..\rra" is rejected; a slash, backslash, drive separator or string
+	 * edge before/after '..' all denote traversal */
+	if (preg_match('/(^|[\/\\\\:])\.\.([\/\\\\:]|$)/', trim($path))) {
+		return false;
+	}
+
 	if (preg_match('/^([a-zA-Z0-9\_\.\-\\\:\/]+)$/', trim($path))) {
 		return true;
 	} else {
@@ -5121,6 +5128,20 @@ function sanitize_uri(string $uri) : string {
 		$uri = urldecode($uri);
 	}
 
+	/* a network-path reference (//host or /\host) would redirect off-site.
+	 * Browsers strip leading C0 controls and whitespace (WHATWG: tab, LF, FF,
+	 * CR and space) before resolving the URL, so "\x0C//evil.com" reaches the
+	 * browser as "//evil.com". Drop those leading bytes ourselves before the
+	 * slash-collapse check, then collapse any leading slash/backslash run to a
+	 * single '/' so the URI stays a local path. */
+	$trimmed = preg_replace('/^[\x00-\x20]+/', '', $uri);
+
+	if (preg_match('/^[\/\\\\]{2,}/', $trimmed)) {
+		$uri = '/' . ltrim($trimmed, '/\\');
+	} else {
+		$uri = $trimmed;
+	}
+
 	if (str_contains($uri, 'graph_view.php')) {
 		if (!strpos($uri, 'action=')) {
 			$uri = $uri . (strpos($uri, '?') ? '&' : '?') . 'action=' . gnrv('action');

lib/html_utility.php

@@ -1306,17 +1306,19 @@ function update_order_string(bool $inplace = false) : void {
 			unset($_SESSION['sort_data'][$page]);
 			unset($_SESSION['sort_string'][$page]);
 
-			$_SESSION['sort_data'][$page][get_request_var('sort_column')] = get_request_var('sort_direction');
-
 			$column    = get_request_var('sort_column');
 			$direction = get_request_var('sort_direction');
+			$direction = is_string($direction) ? strtoupper($direction) : 'ASC';
+			$direction = in_array($direction, ['ASC', 'DESC'], true) ? $direction : 'ASC';
+
+			$_SESSION['sort_data'][$page][$column] = $direction;
 
 			if ($column == 'ip' || $column == 'ip_address') {
 				$_SESSION['sort_string'][$page] = 'ORDER BY INET_ATON(' . $column . ') ' . $direction;
 			} elseif ($column == 'hostname' && $natural) {
-				$_SESSION['sort_string'][$page] = 'ORDER BY NATURAL_SORT_KEY(' . $del . implode($del . '.' . $del, explode('.', get_request_var('sort_column'))) . $del . ') ' . get_request_var('sort_direction');
+				$_SESSION['sort_string'][$page] = 'ORDER BY NATURAL_SORT_KEY(' . $del . implode($del . '.' . $del, explode('.', get_request_var('sort_column'))) . $del . ') ' . $direction;
 			} else {
-				$_SESSION['sort_string'][$page] = 'ORDER BY ' . $del . implode($del . '.' . $del, explode('.', get_request_var('sort_column'))) . $del . ' ' . get_request_var('sort_direction');
+				$_SESSION['sort_string'][$page] = 'ORDER BY ' . $del . implode($del . '.' . $del, explode('.', get_request_var('sort_column'))) . $del . ' ' . $direction;
 			}
 		} elseif (isset_request_var('sort_column')) {
 			if (isset_request_var('reset')) {
@@ -1324,7 +1326,11 @@ function update_order_string(bool $inplace = false) : void {
 				unset($_SESSION['sort_string'][$page]);
 			}
 
-			$_SESSION['sort_data'][$page][get_request_var('sort_column')] = get_nfilter_request_var('sort_direction');
+			$direction = get_nfilter_request_var('sort_direction');
+			$direction = is_string($direction) ? strtoupper($direction) : 'ASC';
+			$direction = in_array($direction, ['ASC', 'DESC'], true) ? $direction : 'ASC';
+
+			$_SESSION['sort_data'][$page][get_request_var('sort_column')] = $direction;
 
 			$_SESSION['sort_string'][$page] = 'ORDER BY ';
 

tests/Unit/FilterValidateRegexTest.php

@@ -18,7 +18,8 @@
 test('FILTER_VALIDATE_REGEXP requires delimiters and works for true/false parameters', function () {
 	// Without delimiters, filter_var returns false (and emits a warning in strict environments)
 	$broken_regex = '^(true|false)$';
-	
+
+	// The warning text varies across PHP/PCRE builds, so any E_WARNING counts
 	$warning_emitted = false;
 	set_error_handler(function ($errno) use (&$warning_emitted) {
 		// The exact warning text varies across PHP/PCRE builds, so only
@@ -42,7 +43,7 @@ test('FILTER_VALIDATE_REGEXP requires delimiters and works for true/false parame
 		->and($warning_emitted)->toBeTrue();
 
 	// With delimiters, it works correctly
-	$fixed_regex = '/^(true|false)$/';
+	$fixed_regex  = '/^(true|false)$/';
 	$fixed_result = filter_var('true', FILTER_VALIDATE_REGEXP, ['options' => ['regexp' => $fixed_regex]]);
 	expect($fixed_result)->toBe('true');
 

tests/Unit/IsValidPathnameTraversalTest.php

@@ -0,0 +1,42 @@
+<?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                     |
+ +-------------------------------------------------------------------------+
+*/
+
+require_once dirname(__DIR__) . '/Helpers/CactiStubs.php';
+require_once dirname(__DIR__, 2) . '/include/global.php';
+
+// =====================================================================
+// is_valid_pathname traversal guard
+//
+// The guard must reject '..' traversal in unix and Windows drive-relative
+// forms. "C:..\rra" places ':' before '..', so the separator-only check
+// missed it; ':' now counts as a boundary too. Benign filenames that merely
+// contain '..' (no separator either side) stay valid.
+// =====================================================================
+
+test('is_valid_pathname rejects traversal', function (string $path) {
+	expect(is_valid_pathname($path))->toBeFalse();
+})->with([
+	'unix'                => ['../etc'],
+	'win drive backslash' => ['C:..\\rra'],
+	'win drive slash'     => ['C:../rra'],
+	'nested'              => ['a/../b'],
+]);
+
+test('is_valid_pathname accepts benign names containing dots', function (string $path) {
+	expect(is_valid_pathname($path))->toBeTrue();
+})->with([
+	'double dot suffix' => ['file..bak'],
+	'double dot middle' => ['a..b'],
+	'rrd file'          => ['/var/lib/rrd/host.rrd'],
+]);

tests/Unit/SanitizeUriRedirectTest.php

@@ -0,0 +1,75 @@
+<?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                     |
+ +-------------------------------------------------------------------------+
+*/
+
+require_once dirname(__DIR__) . '/Helpers/CactiStubs.php';
+require_once dirname(__DIR__, 2) . '/include/global.php';
+
+// =====================================================================
+// sanitize_uri off-site redirect hardening
+//
+// A leading "//host" or "/\host" is a network-path reference the browser
+// resolves off-site. Browsers also strip leading C0 controls and whitespace
+// (WHATWG: tab, LF, FF, CR, space) before resolving, so a prefix like
+// "\x0C//evil.com" reaches the browser as "//evil.com". sanitize_uri must
+// neutralise both forms because its output flows to header('Location: ...').
+// =====================================================================
+
+test('sanitize_uri collapses a bare protocol-relative path', function () {
+	$out = sanitize_uri('//evil.com');
+
+	expect(substr($out, 0, 2))->not->toBe('//')
+		->and(substr($out, 0, 2))->not->toBe('/\\');
+});
+
+test('sanitize_uri collapses a backslash network-path reference', function () {
+	$out = sanitize_uri('/\\evil.com');
+
+	expect(substr($out, 0, 2))->not->toBe('//')
+		->and(substr($out, 0, 2))->not->toBe('/\\');
+});
+
+test('sanitize_uri collapses repeated leading slashes', function () {
+	expect(substr(sanitize_uri('///x'), 0, 2))->not->toBe('//');
+});
+
+test('sanitize_uri strips a leading control char before collapsing slashes', function (string $uri) {
+	$out = sanitize_uri($uri);
+
+	expect(substr($out, 0, 2))->not->toBe('//')
+		->and(substr($out, 0, 2))->not->toBe('/\\');
+})->with([
+	'form feed' => ["\x0C//evil.com"],
+	'tab'       => ["\x09//evil.com"],
+	'soh'       => ["\x01//evil.com"],
+	'null'      => ["\x00//evil.com"],
+	'cr'        => ["\x0D//evil.com"],
+	'space'     => [' //evil.com'],
+]);
+
+test('sanitize_uri neutralises a urlencoded control-char prefix', function () {
+	// is_urlencoded() triggers urldecode() inside sanitize_uri(), so the raw
+	// "%0C//evil.com" decodes to "\x0C//evil.com" before the slash collapse.
+	$out = sanitize_uri('%0C//evil.com');
+
+	expect(substr($out, 0, 2))->not->toBe('//')
+		->and(substr($out, 0, 2))->not->toBe('/\\');
+});
+
+test('sanitize_uri leaves a legitimate local path untouched', function () {
+	// A single leading slash is a local path, not a network-path reference, so
+	// it must survive verbatim. (graph_view.php is avoided here because that
+	// branch appends an action= parameter, which is unrelated behaviour.)
+	expect(sanitize_uri('/cacti/settings.php'))->toBe('/cacti/settings.php')
+		->and(sanitize_uri('graph_view.php?local_graph_id=3'))->toStartWith('graph_view.php');
+});