INI metacharacters ; and " are not preserved when forwarding settings to child processes

[kayw-geek]

The bug

JobRunner forwards every PHP setting to the child process as -d name=value. PHP CLI then parses the value as an INI string, where ; starts a comment and " is a string delimiter. So if your value contains those characters, PHP silently eats parts of it.

I hit this with the ddtrace extension. Its default datadog.appsec.obfuscation_parameter_value_regexp is a long regex full of " (it matches JSON / URL-encoded keys). Every time PHPUnit forks a worker, that regex arrives mangled inside the child, and the extension stops working correctly.

The fix

When the value contains ; or ", wrap it in "..." and escape inner " as \". Otherwise pass it through untouched, so values like output_buffering=Off or error_reporting=E_ALL & ~E_NOTICE keep their existing INI semantics (boolean keywords / bitwise expressions).

Before: -d highlight.string=(?:"foo";bar)   → child sees (?:foo
After:  -d highlight.string="(?:\"foo\";bar)" → child sees (?:"foo";bar)

Security side-effect (defense in depth)

The pre-fix code also allowed INI directive injection: a value containing a literal newline would be parsed by PHP as multiple -d directives, letting the value override unrelated settings (memory_limit, disable_functions, auto_prepend_file, …).

In PHPUnit's threat model the values come from XML config or the host's own PHP runtime, so an attacker would already need filesystem write access to abuse it. Still, the new quoting closes that path because the value never escapes its quoted string.

Test

Added one case to JobRunnerTest: feed the real ddtrace regex (634 bytes, lots of ") through a forked process, have the child print ini_get(...), assert the output equals the input byte-for-byte. Fails on main, passes with this change. Full unit + end-to-end suites green on Ubuntu / Windows × PHP 8.4 / 8.5 / 8.6.

[sebastianbergmann]

Is this a follow-up to #5860 which was recently addressed in f0ad610? Even if not, I think this should target the 12.5 branch instead of main.

[codecov]

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 96.58%. Comparing base (3f9ecb0) to head (7b1b97a).
⚠️ Report is 12 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/Util/PHP/JobRunner.php	90.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6592      +/-   ##
============================================
- Coverage     96.58%   96.58%   -0.01%     
- Complexity     8053     8057       +4     
============================================
  Files           838      838              
  Lines         24865    24874       +9     
============================================
+ Hits          24017    24025       +8     
- Misses          848      849       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sebastianbergmann]

I will cherry-pick this to 12.5 and then merge to 13.1 and main from there.

[sebastianbergmann]

Merged manually, thanks.

I also created GHSA-qrr6-mg7r-m243 for the security-related aspect of this.

[ADmad]

Will a new release be done for PHPUnit 11 too with this security fix?

Currently composer refuses to install PHPUnit 11 on PHP 8.2 due to the security advisory published.

[sebastianbergmann]

Will a new release be done for PHPUnit 11 too with this security fix?

PHPUnit 11 is not affected. Only PHPUnit 12.5.21 and PHPUnit 13.1.5 are affected.

Please see github/advisory-database#7430.

[ADmad]

PHPUnit 11 is not affected.

Hmm.. Then I wonder why composer is failing to install phpunit 11 on php 8.2 as seen below. It's as if it ignores the constraint for v11. This was not an issue until a few hours ago.

https://github.com/cakephp/cakephp/actions/runs/24597726343/job/71930898997?pr=19404#step:11:100

[sebastianbergmann]

PHPUnit 11 is not affected.
Then I wonder why composer is failing to install phpunit 11 on php 8.2 as seen below.

PHPUnit 11 is not affected by the issue this advisory is about.

The advisory I created yesterday at GHSA-qrr6-mg7r-m243 had the correct affected-version information.

When GitHub published it at GHSA-qrr6-mg7r-m243, they changed that information into something incorrect, which is what is now causing Composer to refuse installs it shouldn't be refusing.

I have opened github/advisory-database#7430 to get this corrected on GitHub's side.

I also consulted @naderman and @Seldaek, the maintainers of Composer and Packagist. At their suggestion I submitted FriendsOfPHP/security-advisories#762. That database takes precedence over the GHSA advisory for Composer, so the false positive on PHPUnit 11 should (have) go(ne) away. Note that https://packagist.org/packages/phpunit/phpunit/advisories shows the correct "affected versions" information.

There is nothing I can do from my end to fix this, and I'd appreciate you not directing the frustration at me. The original advisory I published was correct, and I did not introduce the error. I am also not available to look into specific CI failures like yours, especially not on a weekend.

[ADmad]

Thank you for the clarification.

...I'd appreciate you not directing the frustration at me.

I am pretty sure I did nothing of the sort :) My comments were all inquires trying to figure out what was causing the failures.

[sebastianbergmann]

I am pretty sure I did nothing of the sort :)

I am sorry, you did not. That was supposed to go into a different comment.