test: add Pest v1 security test infrastructure#214
test: add Pest v1 security test infrastructure#214somethingwithproof wants to merge 1 commit intoCacti:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial security-oriented test scaffolding under tests/ intended to prevent unsafe reuse of raw request values in HTML output within the monitor plugin.
Changes:
- Adds a “unit” PHP script that validates expected HTML escaping behavior for a sample payload.
- Adds an “integration” PHP script that scans
monitor_controller.php/monitor_render.phpfor specific hardening call patterns. - Adds an “e2e” PHP script that scans for specific raw
get_request_var(...)reuse string patterns.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/unit/test_request_output_escaping.php | Adds a basic escaping check for a sample payload. |
| tests/Integration/test_monitor_request_output_wiring.php | Adds a static source scan asserting expected escaping/encoding call sites exist. |
| tests/e2e/test_monitor_no_raw_request_reuse.php | Adds a static source scan asserting specific raw-request string patterns are absent. |
| $payload = "\" autofocus onfocus=\"alert(1)"; | ||
| $escaped = htmlspecialchars($payload, ENT_QUOTES, 'UTF-8'); | ||
|
|
||
| if (strpos($escaped, '"') === false && strpos($escaped, '"') !== false) { | ||
| print "OK\n"; | ||
| exit(0); | ||
| } | ||
|
|
||
| fwrite(STDERR, "Expected request values to be escaped for hidden inputs\n"); | ||
| exit(1); |
There was a problem hiding this comment.
This test doesn’t exercise any plugin code or request/hidden-input output; it only asserts PHP’s htmlspecialchars behavior (which will always pass for this payload). The failure message mentions hidden inputs/request values, but nothing in the test checks that—consider rewriting it to assert escaping in the actual rendered HTML (or dropping it if it’s only meant to validate PHP internals).
| $payload = "\" autofocus onfocus=\"alert(1)"; | |
| $escaped = htmlspecialchars($payload, ENT_QUOTES, 'UTF-8'); | |
| if (strpos($escaped, '"') === false && strpos($escaped, '"') !== false) { | |
| print "OK\n"; | |
| exit(0); | |
| } | |
| fwrite(STDERR, "Expected request values to be escaped for hidden inputs\n"); | |
| exit(1); | |
| fwrite(STDOUT, "SKIP: This file does not exercise plugin rendering; replace with a test that asserts escaping in actual rendered HTML.\n"); | |
| exit(0); |
| <?php | ||
| /* | ||
| +-------------------------------------------------------------------------+ | ||
| | Copyright (C) 2004-2026 The Cacti Group | | ||
| +-------------------------------------------------------------------------+ | ||
| | Cacti: The Complete RRDTool-based Graphing Solution | | ||
| +-------------------------------------------------------------------------+ | ||
| */ | ||
|
|
There was a problem hiding this comment.
This PR is titled/described as adding “Pest v1” infrastructure, but this file is a standalone PHP script (no Pest/PHPUnit usage) and there’s no runner wiring in the repo CI to execute it. Either align the PR description/title to what’s being added, or add the actual Pest-based harness + CI/composer invocation so these checks run.
| $checks = array( | ||
| __DIR__ . '/../../monitor_controller.php' => array( | ||
| "html_escape(get_request_var('downhosts'))", | ||
| "html_escape(get_request_var('mute'))", | ||
| "html_escape(get_request_var('tree'))", | ||
| "html_escape(get_request_var('site'))", | ||
| "html_escape(get_request_var('template'))", | ||
| "html_escape(get_request_var('size'))", | ||
| "html_escape(get_request_var('trim'))", | ||
| ), | ||
| __DIR__ . '/../../monitor_render.php' => array( | ||
| "rawurlencode(get_request_var('rfilter'))", | ||
| ), | ||
| ); |
There was a problem hiding this comment.
These checks are implemented as exact-string source scans (strpos on hard-coded snippets), which is very brittle to formatting/refactors and can miss equivalent safe implementations. Also, the expected patterns (e.g. html_escape(get_request_var(...)) / rawurlencode(...)) do not currently exist in monitor_controller.php and monitor_render.php, so this test will fail if executed. Consider moving these assertions into the PR that introduces the hardening, and/or switch to a token/AST-based check (token_get_all) or an integration test that inspects actual rendered output.
| $checks = array( | ||
| __DIR__ . '/../../monitor_controller.php' => array( | ||
| "get_request_var('downhosts') . '\"><input id=\"mute\" type=\"hidden\" value=\"' . get_request_var('mute')", | ||
| "get_request_var('tree') . '\"></td>'", | ||
| "get_request_var('site') . '\"></td>'", | ||
| "get_request_var('template') . '\"></td>'", | ||
| "get_request_var('size') . '\"></td>'", | ||
| "get_request_var('trim') . '\"></td>'", | ||
| ), |
There was a problem hiding this comment.
Several “raw request reuse” patterns include escaped quotes (e.g. id="mute" and '">'), but the actual source uses unescaped quotes (id="mute", '">'). As a result, these patterns won’t match and the test will incorrectly pass even when raw get_request_var(...) output remains in hidden inputs. Update the patterns to match the real source text or use a less fragile approach (e.g., tokenization/regex that ignores quoting style).
| <?php | ||
| /* | ||
| +-------------------------------------------------------------------------+ | ||
| | Copyright (C) 2004-2026 The Cacti Group | | ||
| +-------------------------------------------------------------------------+ | ||
| | Cacti: The Complete RRDTool-based Graphing Solution | | ||
| +-------------------------------------------------------------------------+ | ||
| */ | ||
|
|
There was a problem hiding this comment.
This test is labeled “e2e”, but it doesn’t execute the application end-to-end; it performs a static source-code string scan. Consider renaming/reclassifying it (e.g., “static”/“lint”/“security”) to avoid misleading test categorization.
Separates the Pest security test infrastructure into its own PR as requested.