[#34340] - [4] When using path::clean, a security method, correctly throw a useable exception.
- Fixed in Code Base
- 21 Aug 2021
- Medium
- Build: 4.0-dev
- # 34340
- Diff
- PhilETaylor:pathcheckexception
User tests: Successful: Unsuccessful:
Pull Request for Issue #34263
Summary of Changes
When using Path::clean() method, with an invalid path (Like ../../../../../../) it is designed to throw an exception.
The problem is that someone used code = 20 in the exception.
If your code calls the code below, and doesnt catch the exception then your website will not show an error message, it will show a 502 Bad Gateway (on nginx)
Path::clean('../../../../../../');When you have a php-fpm set up, where the web server passes the request "upstream" to the "gateway" (i.e php-fpm running somewhere, or somehow else) then PHP-FPM will return error code 20 when this exception is thrown causing an error like:
webserver_1 | 2021/06/01 15:10:43 [error] 31#31: *109 upstream sent invalid status "20" while reading response header from upstream, client: 172.19.0.1, server: , request: "GET /administrator/index.php HTTP/1.1", upstream: "fastcgi://172.19.0.6:9000", host: "127.0.0.1:4444"
Testing Instructions
If your code calls the code below, and doesnt catch the exception then your website will not show an error message, it will show a 502 Bad Gateway (on nginx)
Path::clean('../../../../../../');Actual result BEFORE applying this Pull Request
502 Bad Gateway
Expected result AFTER applying this Pull Request
Correctly displayed Exception that can be handled
Documentation Changes Required
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
418 I'm a teapot is a HTTP Error code, not a PHP Exception code ;-)
yes, that in result will be HTTP response status code ;)
On reflection of your joke with HTTP Codes, Im not sure this PR is the right approach... Im thinking now maybe something in the Symfony error handler is converting a User-Defined PHP Exception code to a HTTP Response code...
Let me dig into Symfony Error Handler more...
I have tested this item
on review
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/34340.
maybe something in the Symfony error handler is converting a User-Defined PHP Exception code to a HTTP Response code.
Anything can be,
but I never seen such, most of scripts just use an exception code directly for HTTP response
Confirmed... in the same nginx/php-fpm set up running, using code:
<?php
error_reporting(E_ALL);
ini_set('display_errors',1);
throw new Exception('asd',20);
I correctly get the following, and not a Bad Gateway...
Fatal error: Uncaught Exception: asd in /application/ex.php:5 Stack trace: #0 {main} thrown in /application/ex.php on line 5
So something in Joomla deep in the unhandled error handler is causing a HTTP Response of code 20...
This needs researching and fixing.... (Cause we cannot trust 3PD to catch Exceptions because even Joomla doesnt ;-()
I must have had my coffee....
I was right
On reflection of your joke with HTTP Codes, Im not sure this PR is the right approach... Im thinking now maybe something in the Symfony error handler is converting a User-Defined PHP Exception code to a HTTP Response code...
if you curious, here is where header sets, for an exception
joomla-cms/libraries/src/Document/ErrorDocument.php
Lines 117 to 132 in 3a5984d
it should be 500 if exception code is less than 400, that strange
I dont think it goes anywhere near that, especially as the status of 20 is less than 400 and so that code would reset it to 500... Its the Symfony error handler (The red screen of death)... I need more coffee before going that low into the code again, last time I barely made it out alive...
One more:
joomla-cms/libraries/src/Filesystem/Path.php
Line 216 in cf1b764
yeah that one probably needs to go too, but let me look under the red screen of death and see if we (Joomla) can handle it correctly first.
Please leave open for now, Im using it as a reminder to check that all Mail Sending in Joomla is catching newly added Exceptions that can be raised in MailTemplates.php and below :(
| Labels |
Added:
?
|
||
I suspect 20 comes from the old JError days fwiw and the status codes never got changed.
This can be merged now...
| Status | Pending | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-08-21 21:45:00 |
| Closed_By | ⇒ | wilsonge | |
| Labels |
Added:
?
Removed: ? |
||
Thanks!
I would change to 418, but removing is okay