diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 875623a5f..f30698331 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -40,7 +40,7 @@ jobs: with: languages: ${{ matrix.language }} - name: Autobuild - uses: github/codeql-action/autobuild@8aad20d150bbac5944a9f9d289da16a4b0d87c1e # v3 + uses: github/codeql-action/autobuild@54f647b7e1bb85c95cddabcd46b0c578ec92bc1a # v3 - name: Perform CodeQL Analysis uses: github/codeql-action/analyze@54f647b7e1bb85c95cddabcd46b0c578ec92bc1a # v3 with: diff --git a/.github/workflows/syntax.yml b/.github/workflows/syntax.yml index dcb546b65..a8657873f 100644 --- a/.github/workflows/syntax.yml +++ b/.github/workflows/syntax.yml @@ -80,6 +80,8 @@ jobs: steps: - name: Checkout uses: actions/checkout@v7 + with: + fetch-depth: 0 - name: Install PHP ${{ matrix.php }} uses: shivammathur/setup-php@v2 @@ -113,6 +115,11 @@ jobs: run: composer run-script lint working-directory: . + - name: Run hardening pattern guard + if: github.event_name == 'pull_request' + run: composer run-script hardening-patterns -- --base="${{ github.event.pull_request.base.sha }}" --head=HEAD + working-directory: . + - name: Checking coding standards on base code run: composer run-script phpcsfixer working-directory: . diff --git a/composer.json b/composer.json index 71ddcfa14..03a334233 100644 --- a/composer.json +++ b/composer.json @@ -74,6 +74,7 @@ "phpstan": "phpstan analyse --level 6 ", "php-cs-fixer": "php-cs-fixer fix --allow-unsupported-php-version=yes --dry-run --diff --config=./.php-cs-fixer.php ", "phpcsfixer": "@php-cs-fixer", + "hardening-patterns": "php tests/tools/check_hardening_patterns.php", "php-cs-fixit": "php-cs-fixer fix ", "install-hooks": "bash -c 'install -m 755 tests/tools/pre-commit \"$(git rev-parse --git-path hooks/pre-commit)\"'", "test": "include/vendor/bin/pest --display-warnings" diff --git a/lib/PackageListFilter.php b/lib/PackageListFilter.php new file mode 100644 index 000000000..6c3eb6cf9 --- /dev/null +++ b/lib/PackageListFilter.php @@ -0,0 +1,130 @@ +filter = $filter; + $this->rows = $rows; + $this->page = max(1, $page); + $this->sortColumn = $sortColumn; + $this->sortDirection = $sortDirection; + } + + public static function fromRequest(int $defaultRows) : self { + $rows = (string) grv('rows'); + $rows = $rows === '-1' ? $defaultRows : (int) $rows; + + if ($rows < 1) { + $rows = $defaultRows; + } + + return new self( + (string) grv('filter'), + $rows, + (int) grv('page'), + (string) grv('sort_column'), + (string) grv('sort_direction') + ); + } + + public function filter() : string { + return $this->filter; + } + + public function hasFilter() : bool { + return $this->filter !== ''; + } + + public function rows() : int { + return $this->rows; + } + + public function page() : int { + return $this->page; + } + + public function sortColumn() : string { + return $this->sortColumn; + } + + public function sortDirection() : string { + return $this->sortDirection; + } + + public function offset() : int { + return $this->rows * ($this->page - 1); + } + + public function paginationUrl(string $path) : string { + if ($this->filter === '') { + return $path; + } + + return $path . '?' . http_build_query(['filter' => $this->filter], '', '&', PHP_QUERY_RFC3986); + } + + public static function selectedIdsFromPost() : array { + return self::selectedIdsFromInput($_POST); + } + + public static function selectedIdsFromInput(array $input) : array { + $selected = []; + + foreach ($input as $var => $value) { + if (preg_match('/^chk_([0-9]+)$/', (string) $var, $matches)) { + $selected[] = (int) $matches[1]; + } + } + + return $selected; + } + + public static function encodeSelectedIds(array $ids) : string { + return json_encode(array_values(array_map('intval', $ids)), JSON_THROW_ON_ERROR); + } + + public static function decodeSelectedIds(mixed $ids) : array|false { + if (!is_string($ids) || $ids === '') { + return false; + } + + try { + $decoded = json_decode(stripslashes($ids), true, 512, JSON_THROW_ON_ERROR); + } catch (JsonException) { + return false; + } + + if (!is_array($decoded)) { + return false; + } + + $selected = []; + + foreach ($decoded as $id) { + if (!is_int($id) && !(is_string($id) && preg_match('/^[0-9]+$/', $id))) { + return false; + } + + $selected[] = (int) $id; + } + + return $selected; + } +} diff --git a/package_keys.php b/package_keys.php index 43d16c573..b7b1b645f 100644 --- a/package_keys.php +++ b/package_keys.php @@ -23,6 +23,7 @@ */ require('./include/auth.php'); +require_once(CACTI_PATH_LIBRARY . '/PackageListFilter.php'); require_once(CACTI_PATH_LIBRARY . '/poller.php'); require_once(CACTI_PATH_LIBRARY . '/utility.php'); @@ -53,7 +54,7 @@ function form_actions() : void { // if we are to save this form, instead of display it if (isrv('selected_items')) { - $selected_items = sanitize_unserialize_selected_items(gnrv('selected_items')); + $selected_items = PackageListFilter::decodeSelectedIds(gnrv('selected_items')); if ($selected_items != false) { if (gnrv('drp_action') == '1') { // delete @@ -72,16 +73,9 @@ function form_actions() : void { $p_list = ''; $p_array = []; - // loop through each of the data queries and process them - foreach ($_POST as $var => $val) { - if (preg_match('/^chk_([0-9]+)$/', $var, $matches)) { - // ================= input validation ================= - input_validate_input_number($matches[1]); - // ==================================================== - - $p_list .= '
  • ' . htmle(db_fetch_cell_prepared('SELECT author FROM package_public_keys WHERE id = ?', [$matches[1]])) . '
    • '; - $p_array[] = $matches[1]; - } + foreach (PackageListFilter::selectedIdsFromPost() as $id) { + $p_list .= '
  • ' . htmle(db_fetch_cell_prepared('SELECT author FROM package_public_keys WHERE id = ?', [$id])) . '
    • '; + $p_array[] = $id; } top_header(); @@ -113,7 +107,7 @@ function form_actions() : void { print " - + $save_html @@ -140,27 +134,23 @@ function public_keys() : void { $pageFilter->set_sort_array('author', 'ASC'); $pageFilter->render(); - if (grv('rows') == '-1') { - $rows = read_config_option('num_rows_table'); - } else { - $rows = grv('rows'); - } + $filter = PackageListFilter::fromRequest((int) read_config_option('num_rows_table')); $sql_where = ''; $sql_params = []; // form the 'where' clause for our main sql query - if (grv('filter') != '') { + if ($filter->hasFilter()) { $sql_where = ($sql_where != '' ? ' AND ' : 'WHERE ') . '(author LIKE ? OR homepage LIKE ? OR email_address LIKE ?)'; - $sql_params[] = '%' . grv('filter') . '%'; - $sql_params[] = '%' . grv('filter') . '%'; - $sql_params[] = '%' . grv('filter') . '%'; + $sql_params[] = '%' . $filter->filter() . '%'; + $sql_params[] = '%' . $filter->filter() . '%'; + $sql_params[] = '%' . $filter->filter() . '%'; } $sql_order = get_order_string(); - $sql_limit = ' LIMIT ' . ($rows * (grv('page') - 1)) . ',' . $rows; + $sql_limit = ' LIMIT ' . $filter->offset() . ',' . $filter->rows(); $total_rows = db_fetch_cell_prepared("SELECT COUNT(*) FROM package_public_keys @@ -197,7 +187,7 @@ function public_keys() : void { ], ]; - $nav = html_nav_bar('package_keys.php?filter=' . grv('filter'), MAX_DISPLAY_PAGES, grv('page'), $rows, $total_rows, sizeof($display_text) + 1, __('Package Public Keys'), 'page', 'main'); + $nav = html_nav_bar($filter->paginationUrl('package_keys.php'), MAX_DISPLAY_PAGES, $filter->page(), $filter->rows(), $total_rows, cacti_sizeof($display_text) + 1, __('Package Public Keys'), 'page', 'main'); form_start('package_keys.php', 'chk'); @@ -205,7 +195,7 @@ function public_keys() : void { html_start_box('', '100%', false, 3, 'center', ''); - html_header_sort_checkbox($display_text, grv('sort_column'), grv('sort_direction'), false); + html_header_sort_checkbox($display_text, $filter->sortColumn(), $filter->sortDirection(), false); $i = 0; @@ -215,10 +205,10 @@ function public_keys() : void { $pkey = $key['public_key']; - form_selectable_cell(filter_value($key['author'], grv('filter')), $key['id']); + form_selectable_cell(filter_value($key['author'], $filter->filter()), $key['id']); form_selectable_cell($key['id'], $key['id']); - form_selectable_cell(filter_value($key['homepage'], grv('filter')), $key['id']); - form_selectable_cell(filter_value($key['email_address'], grv('filter')), $key['id']); + form_selectable_cell(filter_value($key['homepage'], $filter->filter()), $key['id']); + form_selectable_cell(filter_value($key['email_address'], $filter->filter()), $key['id']); form_selectable_cell(strlen($pkey) < 200 ? 'SHA1' : 'SHA256', $key['id']); form_checkbox_cell($key['author'], $key['id']); diff --git a/package_repos.php b/package_repos.php index e754797ac..86bf5042b 100644 --- a/package_repos.php +++ b/package_repos.php @@ -23,6 +23,7 @@ */ require('./include/auth.php'); +require_once(CACTI_PATH_LIBRARY . '/PackageListFilter.php'); require_once(CACTI_PATH_LIBRARY . '/poller.php'); require_once(CACTI_PATH_LIBRARY . '/utility.php'); @@ -164,7 +165,7 @@ function form_actions() : void { // if we are to save this form, instead of display it if (isrv('selected_items')) { - $selected_items = sanitize_unserialize_selected_items(gnrv('selected_items')); + $selected_items = PackageListFilter::decodeSelectedIds(gnrv('selected_items')); if ($selected_items != false) { if (gnrv('drp_action') == '1') { // delete @@ -199,16 +200,9 @@ function form_actions() : void { $p_list = ''; $p_array = []; - // loop through each of the data queries and process them - foreach ($_POST as $var => $val) { - if (preg_match('/^chk_([0-9]+)$/', $var, $matches)) { - // ================= input validation ================= - input_validate_input_number($matches[1]); - // ==================================================== - - $p_list .= '
  • ' . htmle(db_fetch_cell_prepared('SELECT name FROM package_repositories WHERE id = ?', [$matches[1]])) . '
    • '; - $p_array[] = $matches[1]; - } + foreach (PackageListFilter::selectedIdsFromPost() as $id) { + $p_list .= '
  • ' . htmle(db_fetch_cell_prepared('SELECT name FROM package_repositories WHERE id = ?', [$id])) . '
    • '; + $p_array[] = $id; } top_header(); @@ -271,7 +265,7 @@ function form_actions() : void { print " - + $save_html @@ -438,27 +432,23 @@ function repos() : void { $pageFilter->rows_label = __('Repos'); $pageFilter->render(); - if (grv('rows') == '-1') { - $rows = read_config_option('num_rows_table'); - } else { - $rows = grv('rows'); - } + $filter = PackageListFilter::fromRequest((int) read_config_option('num_rows_table')); $sql_where = ''; $sql_params = []; // form the 'where' clause for our main sql query - if (grv('filter') != '') { + if ($filter->hasFilter()) { $sql_where = ($sql_where != '' ? ' AND ' : 'WHERE ') . '(name LIKE ? OR repo_branch LIKE ? OR repo_location LIKE ?)'; - $sql_params[] = '%' . grv('filter') . '%'; - $sql_params[] = '%' . grv('filter') . '%'; - $sql_params[] = '%' . grv('filter') . '%'; + $sql_params[] = '%' . $filter->filter() . '%'; + $sql_params[] = '%' . $filter->filter() . '%'; + $sql_params[] = '%' . $filter->filter() . '%'; } $sql_order = get_order_string(); - $sql_limit = ' LIMIT ' . ($rows * (grv('page') - 1)) . ',' . $rows; + $sql_limit = ' LIMIT ' . $filter->offset() . ',' . $filter->rows(); $total_rows = db_fetch_cell_prepared("SELECT COUNT(*) FROM package_repositories @@ -502,7 +492,7 @@ function repos() : void { ], ]; - $nav = html_nav_bar('package_repos.php?filter=' . grv('filter'), MAX_DISPLAY_PAGES, grv('page'), $rows, $total_rows, sizeof($display_text) + 1, __('Package Repositories'), 'page', 'main'); + $nav = html_nav_bar($filter->paginationUrl('package_repos.php'), MAX_DISPLAY_PAGES, $filter->page(), $filter->rows(), $total_rows, cacti_sizeof($display_text) + 1, __('Package Repositories'), 'page', 'main'); form_start('package_repos.php', 'chk'); @@ -510,15 +500,15 @@ function repos() : void { html_start_box('', '100%', false, 3, 'center', ''); - html_header_sort_checkbox($display_text, grv('sort_column'), grv('sort_direction'), false); + html_header_sort_checkbox($display_text, $filter->sortColumn(), $filter->sortDirection(), false); if (cacti_sizeof($repos)) { foreach ($repos as $repo) { form_alternate_row('line' . $repo['id'], true); - form_selectable_cell(filter_value($repo['name'], grv('filter'), 'package_repos.php?action=edit&id=' . $repo['id']), $repo['id']); + form_selectable_cell(filter_value($repo['name'], $filter->filter(), 'package_repos.php?action=edit&id=' . $repo['id']), $repo['id']); form_selectable_cell($types[$repo['repo_type']], $repo['id']); - form_selectable_cell(filter_value($repo['repo_location'], grv('filter')), $repo['id']); + form_selectable_cell(filter_value($repo['repo_location'], $filter->filter()), $repo['id']); form_selectable_ecell($repo['repo_type'] == 0 ? ($repo['repo_branch'] != '' ? $repo['repo_branch'] : __('default')) : __('N/A'), $repo['id'], '', 'center'); form_selectable_cell($repo['enabled'] == 'on' ? __('Yes') : __('No'), $repo['id'], '', 'center'); form_selectable_cell($repo['default'] == 'on' ? __('Yes') : __('No'), $repo['id'], '', 'center'); diff --git a/tests/Unit/PackageListFilterTest.php b/tests/Unit/PackageListFilterTest.php new file mode 100644 index 000000000..30f285d8e --- /dev/null +++ b/tests/Unit/PackageListFilterTest.php @@ -0,0 +1,85 @@ +filter())->toBe('alpha') + ->and($filter->rows())->toBe(25) + ->and($filter->page())->toBe(2) + ->and($filter->offset())->toBe(25); +}); + +test('package list filter clamps non-positive rows to the default row count', function () { + set_request_var('filter', 'alpha'); + set_request_var('rows', '0'); + set_request_var('page', '2'); + set_request_var('sort_column', 'name'); + set_request_var('sort_direction', 'ASC'); + + $filter = PackageListFilter::fromRequest(25); + + expect($filter->rows())->toBe(25) + ->and($filter->offset())->toBe(25); +}); + +test('package list filter encodes pagination query values', function () { + set_request_var('filter', 'a b&c'); + set_request_var('rows', '10'); + set_request_var('page', '1'); + set_request_var('sort_column', 'name'); + set_request_var('sort_direction', 'ASC'); + + $filter = PackageListFilter::fromRequest(25); + + expect($filter->paginationUrl('package_repos.php'))->toBe('package_repos.php?filter=a%20b%26c'); +}); + +test('package list filter extracts selected checkbox ids', function () { + $selected = PackageListFilter::selectedIdsFromInput([ + 'chk_12' => 'on', + 'chk_x' => 'on', + 'other' => 'on', + 'chk_34' => 'on', + ]); + + expect($selected)->toBe([12, 34]); +}); + +test('package list filter decodes only numeric selected ids', function () { + $encoded = PackageListFilter::encodeSelectedIds([3, '7']); + + expect(PackageListFilter::decodeSelectedIds($encoded))->toBe([3, 7]) + ->and(PackageListFilter::decodeSelectedIds('["3","bad"]'))->toBeFalse() + ->and(PackageListFilter::decodeSelectedIds('not json'))->toBeFalse(); +}); diff --git a/tests/tools/check_hardening_patterns.php b/tests/tools/check_hardening_patterns.php new file mode 100644 index 000000000..0b2ef7d86 --- /dev/null +++ b/tests/tools/check_hardening_patterns.php @@ -0,0 +1,138 @@ + [ + 'pattern' => '/\$_(?:GET|POST|REQUEST)\s*\[/', + 'message' => 'Use Cacti request helpers or typed filter objects instead of raw request superglobals.', + 'allow' => [ + 'lib/PackageListFilter.php', + ] + ], + 'manual-request-query-url' => [ + 'pattern' => '/[?&][A-Za-z0-9_-]+=\s*[\'"]?\s*\.\s*g(?:r|fr|nr)v\s*\(/', + 'message' => 'Use a URL builder or typed page-state URL method instead of concatenating request values into query strings.' + ], + 'inline-js-php-string' => [ + 'pattern' => '/(?:var|let|const)\s+[A-Za-z0-9_$]+\s*=\s*[\'"][^\'"]*<\?php\s+print\s+/', + 'message' => 'Use JSON encoding for PHP values injected into JavaScript.' + ], + 'raw-request-value-attribute' => [ + 'pattern' => '/value\s*=\s*[\'"][^\'"]*<\?php\s+print\s+g(?:r|fr|nr)v\s*\(/', + 'message' => 'Use form rendering helpers for request-backed input values.' + ], + 'serialized-hidden-state' => [ + 'pattern' => '/(?:type\s*=\s*[\'"]hidden[\'"].*serialize\s*\(|html_hidden_input\s*\([^;]*serialize\s*\()/', + 'message' => 'Do not round-trip serialized PHP state through hidden fields.' + ], +]; + +$violations = []; + +foreach ($files as $file) { + if (!is_file($file) || pathinfo($file, PATHINFO_EXTENSION) !== 'php') { + continue; + } + + $lines = file($file, FILE_IGNORE_NEW_LINES); + + if ($lines === false) { + continue; + } + + foreach ($lines as $index => $line) { + foreach ($patterns as $name => $rule) { + if (isset($rule['allow']) && in_array($file, $rule['allow'], true)) { + continue; + } + + if (preg_match($rule['pattern'], $line) === 1) { + $violations[] = sprintf('%s:%d: %s: %s', $file, $index + 1, $name, $rule['message']); + } + } + } +} + +if ($violations !== []) { + fwrite(STDERR, "Hardening pattern guard found unsafe changed-file patterns:\n"); + fwrite(STDERR, implode("\n", $violations) . "\n"); + + exit(1); +} + +exit(0); + +function changed_php_files(string $base, string $head) : array { + if ($base === '') { + $base = trim((string) shell_exec('git merge-base HEAD upstream/develop 2>/dev/null')); + } + + if ($base === '') { + $base = trim((string) shell_exec('git merge-base HEAD origin/develop 2>/dev/null')); + } + + if ($base === '') { + return []; + } + + $command = sprintf( + 'git diff --name-only --diff-filter=ACMR %s...%s -- %s 2>/dev/null', + escapeshellarg($base), + escapeshellarg($head), + escapeshellarg('*.php') + ); + + $output = shell_exec($command); + + if (!is_string($output) || trim($output) === '') { + return []; + } + + return preg_split('/\R/', trim($output)) ?: []; +} + +function cli_file_args(array $argv) : array { + $files = []; + + for ($i = 1; $i < count($argv); $i++) { + $arg = $argv[$i]; + + if ($arg === '--base' || $arg === '--head') { + $i++; + + continue; + } + + if (str_starts_with($arg, '--base=') || str_starts_with($arg, '--head=')) { + continue; + } + + if (str_starts_with($arg, '--')) { + continue; + } + + $files[] = $arg; + } + + return $files; +}