Skip to content

Commit d5c293d

Browse files
somethingwithproofsomethingwithproof
committed
hardening: run the test suite, and add CodeQL
The plugin shipped three test files that nothing ran: the workflow had no reference to tests/ at all, so a suite covering request-variable output escaping sat unexecuted. Adopts the Pest-through-composer pattern from thold and mactrack, with a bootstrap that stubs Cacti so plugin code loads without a full install. Two of the three existing files were meaningful grep guards against raw request reuse and are kept as Pest datasets; the third only asserted that htmlspecialchars() works. Pest 2 rather than mactrack's Pest 1, which no longer runs on current PHP. The PHP 8.0 runtime floor is enforced by the syntax-floor job rather than a composer platform pin, since the dev tooling needs 8.1. Stacked on Cacti#225, so the SHA pinning and permissions block are inherited rather than duplicated. Dependabot is left to Cacti#226. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
1 parent 8e38688 commit d5c293d

12 files changed

Lines changed: 379 additions & 103 deletions

‎.github/workflows/codeql.yml‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
# +-------------------------------------------------------------------------+
2+
# | Copyright (C) 2004-2026 The Cacti Group |
3+
# +-------------------------------------------------------------------------+
4+
# | Cacti: The Complete RRDtool-based Graphing Solution |
5+
# +-------------------------------------------------------------------------+
6+
# | http://www.cacti.net/ |
7+
# +-------------------------------------------------------------------------+
8+
#
9+
# CodeQL has no PHP analysis, so this covers the plugin JavaScript only. The PHP is
10+
# covered by the syntax and integration jobs in plugin-ci-workflow.yml.
11+
12+
name: "CodeQL"
13+
14+
on:
15+
push:
16+
branches: [main, develop]
17+
paths-ignore:
18+
- "**/*.md"
19+
pull_request:
20+
branches: [main, develop]
21+
paths-ignore:
22+
- "**/*.md"
23+
schedule:
24+
- cron: "30 1 * * 1"
25+
workflow_dispatch:
26+
27+
concurrency:
28+
group: codeql-${{ github.ref }}
29+
cancel-in-progress: true
30+
31+
permissions:
32+
contents: read
33+
34+
jobs:
35+
analyze:
36+
name: Analyze
37+
runs-on: ubuntu-latest
38+
timeout-minutes: 15
39+
permissions:
40+
actions: read
41+
contents: read
42+
security-events: write
43+
strategy:
44+
fail-fast: false
45+
matrix:
46+
language: ["javascript-typescript"]
47+
steps:
48+
- name: Checkout repository
49+
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4
50+
- name: Initialize CodeQL
51+
uses: github/codeql-action/init@b56ba49b26e50535fa1e7f7db0f4f7b4bf65d80d # v3
52+
with:
53+
languages: ${{ matrix.language }}
54+
- name: Autobuild
55+
uses: github/codeql-action/autobuild@b56ba49b26e50535fa1e7f7db0f4f7b4bf65d80d # v3
56+
- name: Perform CodeQL Analysis
57+
uses: github/codeql-action/analyze@b56ba49b26e50535fa1e7f7db0f4f7b4bf65d80d # v3
58+
with:
59+
category: "/language:${{ matrix.language }}"

‎.github/workflows/plugin-ci-workflow.yml‎

Lines changed: 65 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ on:
3131
- '.github/workflows/plugin-ci-workflow.yml'
3232
- 'composer.json'
3333
- 'composer.lock'
34+
- 'phpunit.xml.dist'
35+
- 'tests/**'
3436
pull_request:
3537
branches:
3638
- main
@@ -40,6 +42,8 @@ on:
4042
- '.github/workflows/plugin-ci-workflow.yml'
4143
- 'composer.json'
4244
- 'composer.lock'
45+
- 'phpunit.xml.dist'
46+
- 'tests/**'
4347
workflow_dispatch:
4448

4549
permissions:
@@ -50,6 +54,64 @@ concurrency:
5054
cancel-in-progress: true
5155

5256
jobs:
57+
# The plugin ships tests that nothing ran: three files under tests/ that pass
58+
# from a plain PHP CLI. They cover request-variable output escaping, which is
59+
# the class of defect most likely to regress here.
60+
# A duplicate key or schema error stops GitHub loading the file, which
61+
# presents as a workflow that silently never runs. Catch it here instead.
62+
workflow-lint:
63+
name: Workflow syntax
64+
runs-on: ubuntu-latest
65+
steps:
66+
- name: Checkout Monitor Plugin
67+
uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5
68+
69+
- name: Run actionlint
70+
uses: docker://rhysd/actionlint@sha256:887a259a5a534f3c4f36cb02dca341673c6089431057242cdc931e9f133147e9
71+
with:
72+
args: -color
73+
74+
unit-test:
75+
name: Standalone test suite
76+
runs-on: ubuntu-latest
77+
steps:
78+
- name: Checkout monitor Plugin
79+
uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5
80+
81+
- name: Install PHP
82+
uses: shivammathur/setup-php@b604ade2a87db23f8871b7182e69ec5e75effb45 # v2
83+
with:
84+
php-version: '8.1'
85+
tools: composer
86+
87+
- name: Install dev dependencies
88+
run: composer install --prefer-dist --no-progress --no-interaction --no-plugins --no-scripts
89+
90+
- name: Run the test suite
91+
run: composer test
92+
93+
# compat = 1.2.15 implies Cacti 1.2.x, which requires PHP 8.0. The matrix
94+
# below starts at 8.1, so it cannot catch syntax the declared floor rejects.
95+
syntax-floor:
96+
name: PHP 8.0 syntax floor
97+
runs-on: ubuntu-latest
98+
steps:
99+
- name: Checkout monitor Plugin
100+
uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5
101+
102+
- name: Install PHP 8.0
103+
uses: shivammathur/setup-php@b604ade2a87db23f8871b7182e69ec5e75effb45 # v2
104+
with:
105+
php-version: '8.0'
106+
107+
- name: Lint every PHP file
108+
run: |
109+
if find . -path ./vendor -prune -o -name '*.php' -print0 \
110+
| xargs -0 -n1 php -l 2>&1 | grep -iv 'no syntax errors detected'; then
111+
echo "Syntax errors found at the declared PHP floor"
112+
exit 1
113+
fi
114+
53115
integration-test:
54116
runs-on: ${{ matrix.os }}
55117

@@ -79,19 +141,19 @@ jobs:
79141

80142
steps:
81143
- name: Checkout Cacti
82-
uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5
144+
uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5
83145
with:
84146
repository: Cacti/cacti
85147
ref: release/1.2.31
86148
path: cacti
87149

88150
- name: Checkout Monitor Plugin
89-
uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5
151+
uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5
90152
with:
91153
path: cacti/plugins/monitor
92154

93155
- name: Install PHP ${{ matrix.php }}
94-
uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # v2
156+
uses: shivammathur/setup-php@b604ade2a87db23f8871b7182e69ec5e75effb45 # v2
95157
with:
96158
php-version: ${{ matrix.php }}
97159
extensions: intl, mysql, gd, ldap, gmp, xml, curl, json, mbstring

‎.gitignore‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,3 +22,8 @@
2222
locales/po/*.mo
2323
vendor/
2424
.omc/
25+
26+
# Lockfile is not committed: the plugin declares a range, and thold does the same.
27+
composer.lock
28+
/vendor/
29+
.phpunit.result.cache

‎composer.json‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
{
2+
"name": "cacti/plugin_monitor",
3+
"description": "Monitor Plugin for Cacti",
4+
"type": "cacti-plugin",
5+
"license": "GPL-2.0-or-later",
6+
"require": {
7+
"php": ">=8.0"
8+
},
9+
"require-dev": {
10+
"pestphp/pest": "^2"
11+
},
12+
"config": {
13+
"allow-plugins": {
14+
"pestphp/pest-plugin": true
15+
}
16+
},
17+
"scripts": {
18+
"test": "pest",
19+
"test:integration": "pest tests/Integration"
20+
}
21+
}

‎phpunit.xml.dist‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<phpunit bootstrap="tests/bootstrap.php" colors="true" beStrictAboutOutputDuringTests="false">
3+
<testsuites>
4+
<testsuite name="Unit">
5+
<directory>./tests/unit</directory>
6+
</testsuite>
7+
<testsuite name="Integration">
8+
<directory>./tests/Integration</directory>
9+
</testsuite>
10+
</testsuites>
11+
</phpunit>

‎tests/Integration/OutputEscapingTest.php‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<?php
2+
/*
3+
+-------------------------------------------------------------------------+
4+
| Copyright (C) 2004-2026 The Cacti Group |
5+
+-------------------------------------------------------------------------+
6+
| Source rules, not behavioural tests. They match the text of the web |
7+
| entry points, so they catch an escaping call being deleted and nothing |
8+
| else: a new unescaped sink written with different markup passes. The |
9+
| pages chdir() to the Cacti root and include auth.php, so asserting on |
10+
| rendered output needs a live install, which tests/e2e would cover. |
11+
| |
12+
| Behavioural coverage lives in tests/unit, which loads plugin code. |
13+
+-------------------------------------------------------------------------+
14+
*/
15+
16+
$root = dirname(__DIR__, 2);
17+
18+
dataset('escaped_outputs', [
19+
['monitor_controller.php', "html_escape(get_request_var('downhosts'))"],
20+
['monitor_controller.php', "html_escape(get_request_var('mute'))"],
21+
['monitor_controller.php', "html_escape(get_request_var('tree'))"],
22+
['monitor_controller.php', "html_escape(get_request_var('site'))"],
23+
['monitor_controller.php', "html_escape(get_request_var('template'))"],
24+
['monitor_controller.php', "html_escape(get_request_var('size'))"],
25+
['monitor_controller.php', "html_escape(get_request_var('trim'))"],
26+
['monitor_render.php', "rawurlencode(get_request_var('rfilter'))"],
27+
]);
28+
29+
it('escapes request values before printing them', function (string $file, string $pattern) use ($root) {
30+
expect(file_get_contents($root . '/' . $file))->toContain($pattern);
31+
})->with('escaped_outputs');
32+
33+
dataset('raw_reuse', [
34+
['monitor_controller.php', "get_request_var('tree') . '\"></td>'"],
35+
['monitor_controller.php', "get_request_var('site') . '\"></td>'"],
36+
['monitor_controller.php', "get_request_var('template') . '\"></td>'"],
37+
['monitor_controller.php', "get_request_var('size') . '\"></td>'"],
38+
['monitor_controller.php', "get_request_var('trim') . '\"></td>'"],
39+
['monitor_render.php', "monitor.php?rfilter=' . get_request_var('rfilter')"],
40+
]);
41+
42+
it('never concatenates a raw request value into markup', function (string $file, string $pattern) use ($root) {
43+
expect(file_get_contents($root . '/' . $file))->not->toContain($pattern);
44+
})->with('raw_reuse');

‎tests/Integration/test_monitor_request_output_wiring.php‎

Lines changed: 0 additions & 41 deletions
This file was deleted.

‎tests/Pest.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<?php
2+
/*
3+
+-------------------------------------------------------------------------+
4+
| Copyright (C) 2004-2026 The Cacti Group |
5+
+-------------------------------------------------------------------------+
6+
| Cacti: The Complete RRDtool-based Graphing Solution |
7+
+-------------------------------------------------------------------------+
8+
*/
9+
10+
uses()->beforeEach(function () {
11+
$GLOBALS['__test_sql'] = [];
12+
$GLOBALS['__test_settings'] = [];
13+
test_set_request([]);
14+
})->in(__DIR__);

0 commit comments

Comments
 (0)