Fix downloads.php crash on invalid os parameter#1968
Conversation
Validate the client-supplied ?os= against the whitelist before indexing, fixing ~24k/day 'array_key_exists(): ... null given' fatals. Extract the resolution into a testable OptionResolver class.
|
📊 Regression report for commit 70b6307 is at https://web-php-regression-report-pr-1968.preview.thephp.foundation |
|
🚀 Preview for commit 70b6307 is available at https://web-php-pr-1968.preview.thephp.foundation |
|
I think we should add a redirect instead of rendering by default. https://web-php-pr-1968.preview.thephp.foundation/downloads.php?os=error |
Thanks for the feedback, I've added this at the risk of some complexity. If we're happy, I can squash the commits, or we can keep tinkering. |
Fix
downloads.phpcrash on invalidosquery parameterSummary
downloads.phpthrows an uncaughtTypeErrorwhenever theosqueryparameter is not one of the three whitelisted values (
linux,osx,windows). The value goes straight from$_GETinto an array index(
$os[$options['os']]['variants']) without validation, so any unexpectedvalue makes that expression
nulland blows uparray_key_exists().This is currently the single largest source of PHP fatals on www.php.net:
24,332 fatal errors in a recent 15‑day window of production logs. Almost
entirely from bots and security scanners fuzzing the parameter.
This PR validates
osagainst the whitelist (falling back to the auto‑detecteddefault), and extracts the option‑resolution logic into a unit‑tested
OptionResolverclass following the existingLangChooserpattern.The bug
downloads.phpmerges raw$_GETover the defaults and then uses theattacker‑controlled
osvalue to index the$osconfig:If
$options['os']is not a key in$os, then$os[$options['os']]isnull,$os[$options['os']]['variants']isnull, andarray_key_exists(..., null)throws.
Evidence from production logs
The fatal (
error.log, client IP redacted):The companion warning emitted immediately before it:
Behavior for valid input is unchanged.
Testing
tests/Unit/Downloads/OptionResolverTest.php: 10 tests covering thecrash repro (
?os=<garbage>and?os[]=arraynow fall back to the default),valid‑input passthrough, variant fallback,
$_GETpreservation, anduser‑agent auto‑detection.
GET /downloads.php?os=<garbage>,?os[]=x,?os=windows, and the plain page all return HTTP 200 with theform fully rendered; no fatals in the server log.