* fix(security): reject unsupported nested shell grammar
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
* test(security): exercise nested grammar rejection in CI coverage
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
---------
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The stack-based substitute_script_path() rewrite left quoteEmpty set while emitting literal content inside a single-quoted region, so a template like '\<field>' wrongly took the exact-wrap branch and dropped the backslash (ScriptSubstitutionMatrix 'shell 56'). Clear the frame's quoteEmpty whenever a single-quoted character is emitted, matching the pre-rewrite fallthrough.
ScriptSubstitutionMatrix.php: add 8 explicit parser cases exercising the \/backtick nested contexts and escaped \$ / backtick pairing so every rewritten line is covered by the changed-coverage probe (1285 -> 1293).
substitute_script_path() now follows the template's own $(...) and backtick
command substitutions as nested parsing contexts with their own quote state, so a
token placed inside one ("$(printf %s <field>)") is judged against that fresh
context instead of a suspended outer double quote. This closes the
nested-reinterpretation bypass where an escaped value sitting inside a command
substitution could be re-activated by an outer double quote. Backslash pairing in
both the unquoted and double-quoted contexts now also neutralizes $ and backtick,
so an escaped command substitution (\$(...) or \`...`) is not mis-detected as live.
.github/workflows/pr-changed-coverage.yml: drop the hard-coded PR-number
allowlist so the job runs whenever the path filter selects it, rather than
silently skipping every unlisted PR once these merge.
tests/Unit/Security/ShellInjection/ScriptFieldShellRoundTripTest.php: skip when
either exec() or proc_open() is unavailable (the test launches the fixture with
exec() while the fixture itself uses proc_open()). The fixture adds 7 nested
command-substitution and backtick round-trip cases (90 -> 97), each proving the
field value echoes back verbatim with nothing inside it executing.
A template backslash immediately before a <field> token (\<arg1>) escaped the
already-escaped value's opening quote, producing \'...' which the shell rejects
with an unmatched-quote error (odd backslash runs) or prepended a stray literal
backslash (even runs). The quote-aware scanner now sheds one backslash when the
run immediately preceding a self-quoted value is odd, in both the unquoted
in-place branch and the close-around branch, so the value's own quotes always
function as the shell's quotes and $(...)/backticks inside it can never
re-activate.
The backslash regression test now executes each substituted template through
/bin/sh (printf %s) and asserts a clean parse plus a literal round-trip across
odd/even backslash counts, rather than pinning the generated string.
Mirror the 1.2.x fix (#8238): the scanner's backslash branches consumed the next
character unconditionally, so a legacy/custom template token such as \<arg> never
reached the matcher and the trailing cleanup stripped it to a bare backslash. Both
branches now only pair the backslash with a following quote/backslash (so an escaped
quote still cannot toggle quote-state); any other character - notably a <token> start -
leaves the backslash on its own and is processed on the next iteration. Reformatted per
php-cs-fixer and added a backslash-before-token regression test.
Keep the develop fix in sync with the 1.2.x backport (#8238): substitute_script_path()
only sheds/closes the template's quotes for self-quoting (cacti_escapeshellarg) values.
Trusted raw path_* tokens are substituted in place so a quoted "<path_php_binary>" keeps
its quotes for the shell and a bare <path_cacti>/scripts/x.php stays unquoted for the PHP
script server, which resolves the first token as a filesystem path. Reformatted per
php-cs-fixer and added a raw-path-token regression test.
Split tests/Unit/Security/Batch12SecurityRegressionTest.php into behavior-named
files under the existing vulnerability-class layout, per the review feedback:
Auth/TwoFaCookieAndResetTokenConstantTimeTest.php GHSA-4qx8
Ssrf/PackageRepoManifestFetchTest.php GHSA-54fg
ShellInjection/DataInputQuotedPlaceholderTest.php GHSA-5v3j
ShellInjection/SpikekillRrdfileEscapeTest.php GHSA-vhh2
Each file reaches the repo root via dirname(__DIR__, 4) for its one
subfolder-deep location. No assertion changes; pure relocation.
* security: consolidate verified GHSA fixes for develop (batch 4)
Develop counterparts of the 1.2.x batch-4 fixes in #8144, re-verified against
current develop HEAD.
- GHSA-4rmr-wvjq-qxc2: include/auth.php Basic Auth and guest branches fall
through to the per-page realm check instead of returning early.
- GHSA-j67j-wpm4-9g3x: the pre-dispatch update_policy branch in user_admin.php
and user_group_admin.php requires a CSRF-token POST. The denylist -> allowlist
redesign is tracked for 1.3 in #8143.
- GHSA-2p3x-f5jm-rg6p: the View-Graphs -> poller RCE chain is broken by the
realm check (4rmr), the update_policy guard (j67j) and shell-free webhits (x5pw).
- GHSA-x5pw-cg7j-mj4r: scripts/webhits.pl runs wc through a shell-free list-form open.
- GHSA-9737-rqh3-h7fh: names interpolated into raise_message() now use __esc()
across the UI; URL- and HTML-bearing messages are excluded.
- GHSA-9x9h-2577-9w86: the colordropdown input is built as a node and the label
is no longer round-tripped through parseHTML (the list item already used
createTextNode on develop).
- GHSA-h96j-8xhc-2p3w: remote_agent.php authorizes on $_SERVER['REMOTE_ADDR'],
not get_client_addr(), which honours client-settable proxy headers.
Regression guards under tests/Unit/Security/{Auth,Csrf,Xss,ShellInjection}.
php -l, check-changelog.py and the sink/architectural baselines all pass.
* chore: reference PR #8145 on the batch-4 1.3.0-dev CHANGELOG entries
* security: resolve develop merge - keep id-only xmlerror unescaped (matches #8141 h745 test)
* security(GHSA-j67j): use REQUEST_METHOD POST guard instead of raw \ (hardening-pattern-guard)
* security(GHSA-9737): fix review findings - drop double-encoding in __esc, escape missed name sinks, correct plugins format-arg count
* refactor(tests): reorganize unit tests into domain category folders
- Consolidate per-advisory GHSA regression tests into Security/ subfolders by vuln class
- Group all 186 test files into domain folders: DataCollection/, Ui/, Core/,
Database/, Installer/, Scripts/, Security/, Plugin/
- Strip GitHub issue/GHSA numbers from filenames; keep IDs in test descriptions
- Fix all __DIR__ relative paths to match new folder nesting depth
- Update CI workflow path triggers and phpunit XML configs for new file locations
- Update .github/copilot-instructions.md with unit test organization guidelines
- Add tests/vendor/ to .gitignore
* add gaurds
* fix up guard for unit tests
* fix redeclaration of cacti_log in tests
* fix updates to unit tests
* fix test patjhs
* fix: update test __DIR__ paths for reorganized test structure
* fix: conditionally define read_user_setting to prevent redeclaration in tests
* fix: conditionally define functions to prevent redeclaration in tests
- Wrap clean_up_lines, raise_message, cacti_debug_backtrace, and get_debug_prefix
with function_exists() checks to prevent conflicts when test stubs are loaded
before lib/functions.php in the same PHP process
* fix: add unit test bootstrap to prevent function redeclaration errors
- Create tests/bootstrap-unit.php to load core libraries before tests
- Ensures lib/functions.php loads before test stubs define stub functions
- Test stubs use function_exists() checks, so they detect pre-loaded functions
- Update phpunit.xml to use bootstrap-unit.php for Unit testsuite
- Fixes 'Cannot redeclare read_user_setting()' errors when tests run in same process
* fix: load test stubs before global.php in unit test bootstrap
- Reorder bootstrap to load CactiStubs.php and UnitStubs.php first
- Ensures stub functions like __() are available when global.php loads
- global.php calls get_cacti_version_text() which needs __() for translations
- Fixes 'Call to undefined function __()' error during bootstrap
* fix: remove function conflicts from test stubs
- Remove stub definitions for functions already in lib/functions.php:
cacti_sizeof, cacti_log, cacti_strtolower, read_config_option,
read_user_setting, cacti_count
- Keep translation function stubs (__(), __esc()) needed by global.php
- Update bootstrap to load lib/functions.php after stubs
- This prevents function redeclaration errors while maintaining test isolation
* fix: prevent constant and variable redeclaration in bootstrap
- Remove explicit PHP_TESTING and CACTI_TEST_BOOTSTRAP defines from bootstrap
(CactiStubs.php now handles this with guards)
- Add guards to CactiStubs.php to prevent redefinition of PHP_TESTING
- Only set CACTI_TEST_BOOTSTRAP env vars if not already set
- Fixes PHP warnings about 'Constant X already defined'
* fix: remove redundant lib/functions.php load from bootstrap
include/global.php already loads lib/functions.php at lines 287-288.
The bootstrap was trying to load it again after global.php, but global.php
needs format_cacti_version() to be available on lines 293-294 to define
CACTI_VERSION and CACTI_VERSION_FULL constants.
Solution: Let global.php handle loading lib/functions.php itself, and don't
load it separately in the bootstrap.
* fix: remove more conflicting function stubs from test helpers
- Remove stubs for functions also defined in lib/functions.php:
raise_message, clean_up_lines, cacti_debug_backtrace, get_debug_prefix
- Remove POLLER_ID constant stub (defined unconditionally by global.php)
- Keep only translation functions and test-specific helpers
- This prevents function/constant redeclaration errors
* fix: minimize test stubs to only translation functions
Remove ALL helper functions and constants from UnitStubs.php that are:
- Defined in lib/functions.php (raise_message, clean_up_lines, etc.)
- Defined in lib/html_utility.php (srv, set_request_var)
- Defined in global_constants.php (POLLER_VERBOSITY_*, MESSAGE_LEVEL_*)
- Defined in global.php (POLLER_ID)
Keep ONLY translation functions (__(), __esc()) that are:
- Called by global.php during initialization
- Not defined elsewhere
- Required for proper bootstrap sequence
This eliminates all constant and function redeclaration errors.
* fix: remove __esc stub - already defined in global_languages.php
Keep only __() translation function stub, which is needed by global.php
and not defined elsewhere in Cacti core.
* fix: handle no-arg case in __() translation stub
The __() function may be called with just a string and no format arguments.
Previously, vsprintf() would fail with 'arguments array must contain 2 items'.
Now, if no args are provided, return the text as-is.
Thomas Vincent