feat(security): Symfony Process and Mime helpers by somethingwithproof · Pull Request #7073 · Cacti/cacti

somethingwithproof · 2026-04-28T03:10:04Z

Current Status

Consolidated into #7189. Keep this PR only as historical review context for the original Symfony Process and Mime extraction; the merge direction should be the DI-friendly component baseline in #7189.

Original Scope

Introduced Cacti-owned wrappers around Symfony Process and Mime.
Began moving shell execution and MIME detection toward Symfony components.
Established the component-adapter approach that refactor(symfony): align components with DI-friendly wrappers #7189 now applies more broadly.

Recommendation

Do not merge this independently unless it is deliberately rebased to match #7189. New or migrated code should follow the #7189 wrapper style: injectable service instances first, static facades only for legacy procedural call sites.

Copilot

Pull request overview

Introduces a centralized subprocess runner for Cacti PHP code by adding symfony/process and a small wrapper API (CactiProcess) intended to reduce shell-injection risk and standardize timeouts/exit-code handling, with an initial migration of one CLI script plus tests and documentation.

Changes:

Add symfony/process dependency and implement CactiProcess, CactiProcessResult, and CactiProcessException.
Migrate cli/push_out_hosts.php from passthru() shell-string execution to array-argv execution via CactiProcess::runStreaming().
Add unit/integration tests for the wrapper and a security/migration guide doc.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`composer.json`	Adds `symfony/process` dependency.
`lib/CactiProcess.php`	New wrapper for running subprocesses (complete + streaming).
`lib/CactiProcessResult.php`	Result value object for exit code/stdout/stderr plus `outputLines()`.
`lib/CactiProcessException.php`	Typed exception for process runner failures.
`cli/push_out_hosts.php`	Pilot replacement of `passthru()` with `CactiProcess::runStreaming()`.
`tests/Unit/CactiProcessTest.php`	Unit coverage for exit codes, timeout, argv safety, and output parsing.
`tests/integration/CactiProcessIntegrationTest.php`	Integration test for streaming line delivery.
`docs/security/process-runner.md`	Rationale + migration guide for adopting `CactiProcess`.

Routes external commands through Symfony Process in array-mode argv, scrubs the environment to a baseline allowlist, and centralizes timeout and exit-code handling. Migrates cli/push_out_hosts.php as the pilot call site. Includes unit, integration, and a migration guide. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Pest tests pin every data boundary the wrapper crosses (argv→child, env scrub, stdout bytes, exit code, exec-shape parity, timeout chaining). infection.json5 scopes mutation analysis to the wrapper files with a 70% MSI floor. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

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

PHP refuses to start without SYSTEMROOT; proc_open relies on COMSPEC and PATHEXT to resolve executables under cmd.exe. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Routes uploaded-file MIME detection through Symfony Mime so a content-type allowlist can be enforced on top of the existing extension and signature checks. package_import.php now rejects uploads whose detected MIME is not in the allowlist (zip, gzip, xml variants) before any further processing. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

LIBXML_NONET blocks network entity loading as defense in depth even though libxml2 ≥ 2.9.0 disables external entities by default. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

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

…t timeout to float Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Match the flat un-namespaced lib/CactiX.php convention used by Cacti#7088, Cacti#7073, Cacti#7077, Cacti#7075. Drop symfony/process (added by Cacti#7073) and symfony/validator (added by Cacti#7077) to avoid composer.json conflicts at merge time. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Match the un-namespaced lib/CactiX.php convention used by canonical PRs (Cacti#7088 CactiProcess, Cacti#7073 CactiMime, Cacti#7077 CactiSettings, Cacti#7075 CactiApplication/CactiCommand). The previous nested PSR-4 namespace required composer autoload changes that conflicted with those PRs. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Drop lib/Process/CactiProcess.php and tests/Unit/CactiProcessTest.php to avoid colliding with Cacti#7088 / Cacti#7073, which already provide the canonical flat lib/CactiProcess.php. Move the remaining wrappers to the un-namespaced lib/CactiX.php convention used by every other lib/Cacti*.php class on develop: lib/Filesystem/CactiFilesystem.php -> lib/CactiFilesystem.php lib/Http/CactiRequest.php -> lib/CactiRequest.php lib/Log/CactiLogger.php -> lib/CactiLogger.php Tests load the classes via require_once instead of the dropped PSR-4 autoloader. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Rename lib/Security/CactiOAuth.php to lib/CactiOAuth.php and drop the Cacti\Security namespace. Update the two callers (oauth2.php and lib/functions.php mailer) to use the unqualified class name. Behavior is unchanged: only the FQCN was rewritten. The flat layout matches every other lib/Cacti*.php class on develop and in the canonical PRs (Cacti#7088, Cacti#7073, Cacti#7077, Cacti#7075). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Allows callers to inject env name=value pairs without putenv() or argv exposure. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof · 2026-05-08T21:07:54Z

Follow-up commit 917a5a2c6 adds an env_set opt to CactiProcess::run for sealed secret passing (e.g. MYSQL_PWD). Validates uppercase POSIX env names, rejects non-string values, and writes directly to the child env without touching putenv() / $_ENV. Seven new tests cover injection, parent non-leak, override-of-forwarded, and three negative cases.

This unblocks redoing the call-site migrations from the now-closed #7088 against the correct API. The migrations themselves will follow as small per-file PRs once the API change here lands.

somethingwithproof · 2026-05-11T23:15:00Z

Queue note: treating this as the base Symfony/helper infrastructure PR. Intended order is this PR first, then #7075 after the Process/Mime helper direction is accepted.

…7124) * test: shared hand-off + mutation infrastructure for feature PRs Consolidates infection/infection dev dep, allow-plugins entry, baseline infection.json5, tests/HandOff/HandOffHelpers.php with reusable stubs (cacti_log capture, temp-file fixtures, minimal-zip builder), HandOff testsuite registration, and the documented pattern. Feature PRs that add hand-off coverage rebase onto this and contribute only ONE HandOffTest file plus an optional infection.json5 filter override. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: consolidate per-feature hand-off suites into shared infra Each test guards on its feature file and skips when not present on develop. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(test): replace eval stub, unify log buffer ref, guard ZipArchive add Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(ci): correct infection vendor path in infection.json5 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * ci(phpstan): baseline upstream undefined-variable and isset errors Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: add HandOff and Integration testsuites; add regression guards - phpunit.xml: add Integration testsuite alongside existing Unit/HandOff - tests/Integration/: DbDumpIntegrationTest, PingIntegrationTest (from develop), CactiProcessIntegrationTest, SqlScriptsIntegrationTest - tests/HandOff/RegressionGuardTest: guards against backsliding on shell_exec, password-in-argv, RLIKE concat, and open-redirect patterns Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * chore(test): pest test infrastructure and PSR-4 autoload Add Pest 2 dev dependency, regenerate composer.lock under PHP 8.1, declare PSR-4 autoload for the Cacti namespace, and seed the test bootstrap with three unit tests plus an orb integration check. Pest stays on the v2 line because v3 transitively requires PHP 8.2 and breaks the 8.1 matrix entry. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * chore(phpstan): catch up baseline with 11 upstream-detected entries Cacti Commit Audit fails on develop with 11 PHPStan errors that are not yet in phpstan-baseline.neon. Append the matching entries so the PR-A branch passes Run PHPStan at Level 6. Same set of entries already exists on PR #7077; this is a transient catch-up that upstream will absorb when those entries land on develop. Files: aggregate_graphs.php, color_templates.php, graph_templates.php, graphs.php, lib/html.php (3 entries). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * chore(test): drop composer.lock; let CI resolve per PHP version The lock file pinned brianium/paratest v7.3.1 which only supports PHP 8.1-8.3, so the 8.4 matrix entry rejected the lock with "Your lock file does not contain a compatible set of packages". A single lock cannot satisfy 8.1 (paratest 7.3.x) and 8.4 (paratest 7.4.x) at the same time because pestphp/pest 2.36.0 is the last 2.x release that supports 8.1 and the 8.1-compatible paratest tag predates 8.4 support. Match upstream develop's behaviour: no committed lock; let `composer install` in CI resolve per matrix entry. Pest stays on the v2 line in composer.json so 8.1 still gets a compatible Pest. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(test): apply Copilot review feedback for test infra Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * chore(test): drop psr-4 autoload and align deps with canonical PRs Match the flat un-namespaced lib/CactiX.php convention used by #7088, #7073, #7077, #7075. Drop symfony/process (added by #7073) and symfony/validator (added by #7077) to avoid composer.json conflicts at merge time. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: skip integration suites and regression guards pending feature PRs Guards integration tests and regression guards from #7083 against absence of CactiProcess and other feature-PR hardening, so the consolidated foundation PR is green on develop. Suites activate automatically once their feature PRs merge. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(ci): drop infection/infection to unbreak PHP 8.4 matrix infection/infection ^0.27 transitively pins thecodingmachine/safe ^2.1.2, resolving to v2.5.0. Its generated stubs use implicit-nullable parameter declarations, which PHP 8.4 emits as deprecations into cacti.log and trips the log-quiet assertion. infection has no config or test wiring in this repo, so removing the dev dep clears the transitive pin without losing functionality. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net>

TheWitness · 2026-05-12T19:08:30Z

 get_options();

 // start background caching process if not running
-$php            = cacti_escapeshellcmd(read_config_option('path_php_binary'));


Seems this should be registered using register_process_start(), maybe even automatically by Cactid.

TheWitness · 2026-05-12T19:09:08Z

 }

 // start background caching process if not running
-$php        = cacti_escapeshellcmd(read_config_option('path_php_binary'));


Seems this should be migrated to register and unregister process.

TheWitness · 2026-05-12T19:10:17Z

+			'--graph=' . $local_graph_id,
+			'--interval=' . ((int) $graph_data_array['ds_step']),
+			'--poller_id=' . hash('sha256', session_id()),
+		], ['timeout' => null, 'expected_exit_codes' => range(0, 255)]);


This should have a relatively short timeout, otherwise things will block and crash the web server.

* refactor(oauth): extract Cacti\Security\CactiOAuth provider factory Replace the inline provider switch in oauth2.php and the OAuth branch of mailer() in lib/functions.php with two helpers: - CactiOAuth::getProvider($name, $params) returns the configured provider instance for `google`, `azure`, `yahoo`, or `microsoft`, or null when the configured provider is unknown. - CactiOAuth::getDefaultOptions($name) returns the scope set each provider expects. Behaviour is preserved; both call sites now branch on a null return rather than relying on a `$provider` variable that may have fallen through the switch unset. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * chore(phpstan): catch up baseline with 11 upstream-detected entries Same as PR-A: appends 11 PHPStan ignoreErrors entries that exist on upstream develop but are not yet baselined, so this branch's CI does not regress on phpstan analyse --level 6. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * refactor: flatten CactiOAuth to global namespace Rename lib/Security/CactiOAuth.php to lib/CactiOAuth.php and drop the Cacti\Security namespace. Update the two callers (oauth2.php and lib/functions.php mailer) to use the unqualified class name. Behavior is unchanged: only the FQCN was rewritten. The flat layout matches every other lib/Cacti*.php class on develop and in the canonical PRs (#7088, #7073, #7077, #7075). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: add shared unit stubs for OAuth tests * fix(oauth): load provider factory in runtime paths * test(oauth): pin provider factory runtime loading --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net>

- Add explicit $onError callback to forward stderr chunks to STDERR, replacing reliance on CactiProcess's internal fallback. - Capture runStreaming() return value and pass to exit() so the wrapper propagates the child's exit status rather than always exiting 0. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof · 2026-05-16T08:40:18Z

Addressed all five Copilot suggestions: timeout is cast to float with strict int|float|null validation; runStreaming forwards stderr via the $onError callback or fwrite(STDERR) fallback; push_out_hosts.php passes a dedicated onError handler that writes to STDERR; env validation throws CactiProcessException on non-array or non-string entries. The exit-code propagation change is intentional and documented in the commit message.

# Conflicts: # composer.json # graph_realtime.php # tests/Unit/GraphRealtimeShellTest.php # tests/integration/CactiProcessIntegrationTest.php

somethingwithproof · 2026-06-02T03:24:37Z

Closing as superseded by #7189, which carries this work forward as the DI-friendly component baseline. Leaving the branch in place so the review history stays accessible.

Copilot AI review requested due to automatic review settings April 28, 2026 03:10

Copilot started reviewing on behalf of somethingwithproof April 28, 2026 03:10 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

Comment thread lib/CactiProcess.php

Comment thread lib/CactiProcess.php Outdated

Comment thread cli/push_out_hosts.php

Comment thread cli/push_out_hosts.php

Comment thread lib/CactiProcess.php Outdated

somethingwithproof force-pushed the feat/develop-symfony-process-runner branch 2 times, most recently from aed054c to b213f01 Compare April 28, 2026 04:20

somethingwithproof mentioned this pull request Apr 28, 2026

feat(validator): add settings constraint validation pilot #7077

Closed

somethingwithproof force-pushed the feat/develop-symfony-process-runner branch 5 times, most recently from a19eda7 to c4fdd47 Compare April 28, 2026 07:45

somethingwithproof force-pushed the feat/develop-symfony-process-runner branch from c4fdd47 to 83ec329 Compare April 28, 2026 08:20

somethingwithproof added 2 commits April 28, 2026 19:16

ci(infection): allow infection/extension-installer composer plugin

05e8f87

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

somethingwithproof mentioned this pull request Apr 29, 2026

test: shared hand-off + mutation infrastructure for feature PRs #7083

Closed

somethingwithproof added 4 commits April 28, 2026 22:05

test(process): drop hand-off test, consolidated into Cacti#7083

643bb2a

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

fix(process): keep Windows-required env vars in baseline allowlist

7ea3043

PHP refuses to start without SYSTEMROOT; proc_open relies on COMSPEC and PATHEXT to resolve executables under cmd.exe. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

fix(mime): harden simplexml parser against XXE

bb78ff9

LIBXML_NONET blocks network entity loading as defense in depth even though libxml2 ≥ 2.9.0 disables external entities by default. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof changed the title ~~feat(process): add CactiProcess wrapper for subprocess invocation~~ feat(security): Symfony Process and Mime helpers Apr 29, 2026

somethingwithproof mentioned this pull request Apr 29, 2026

feat(mime): add CactiMime helpers and validate package_import uploads #7074

Closed

somethingwithproof added 2 commits April 29, 2026 01:25

ci(phpstan): baseline upstream undefined-variable and isset errors

c2bd08d

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

fix(process): forward stderr in runStreaming, validate env types, cas…

cf8ec8e

…t timeout to float Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

This was referenced May 8, 2026

chore(test): unified Pest test infrastructure with HandOff support #7124

Merged

feat(security): add CactiValidator helpers #7125

Closed

This was referenced May 8, 2026

feat(lib): add CactiRequest, CactiLogger, CactiFilesystem wrappers #7126

Closed

refactor(oauth): extract CactiOAuth provider factory #7129

Merged

fix: migrate shell_exec callers to CactiProcess::run() #7088

Closed

feat(process): add env_set opt for sealed secret passing

917a5a2

Allows callers to inject env name=value pairs without putenv() or argv exposure. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof mentioned this pull request May 11, 2026

feat(cli): add Symfony Console plumbing and migrate two pilot scripts #7075

Closed

somethingwithproof added 2 commits May 11, 2026 16:34

fix(process): disable default timeout for long runners

241eec8

test(process): pin long-runner timeout overrides

e6a50e8

TheWitness reviewed May 12, 2026

View reviewed changes

Merge branch 'upstream-develop' into HEAD

ed81e36

# Conflicts: # composer.json # graph_realtime.php # tests/Unit/GraphRealtimeShellTest.php # tests/integration/CactiProcessIntegrationTest.php

somethingwithproof mentioned this pull request May 31, 2026

refactor(symfony): align components with DI-friendly wrappers #7189

Draft

somethingwithproof closed this Jun 2, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(security): Symfony Process and Mime helpers#7073

feat(security): Symfony Process and Mime helpers#7073
somethingwithproof wants to merge 14 commits into
Cacti:developfrom
somethingwithproof:feat/develop-symfony-process-runner

somethingwithproof commented Apr 28, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

somethingwithproof commented May 8, 2026

Uh oh!

somethingwithproof commented May 11, 2026

Uh oh!

TheWitness May 12, 2026

Uh oh!

TheWitness May 12, 2026

Uh oh!

TheWitness May 12, 2026

Uh oh!

somethingwithproof commented May 16, 2026

Uh oh!

somethingwithproof commented Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

somethingwithproof commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Status

Original Scope

Recommendation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

somethingwithproof commented May 8, 2026

Uh oh!

somethingwithproof commented May 11, 2026

Uh oh!

TheWitness May 12, 2026

Choose a reason for hiding this comment

Uh oh!

TheWitness May 12, 2026

Choose a reason for hiding this comment

Uh oh!

TheWitness May 12, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof commented May 16, 2026

Uh oh!

somethingwithproof commented Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

somethingwithproof commented Apr 28, 2026 •

edited

Loading