Problem/Motivation

Some server administrators may choose to disable the PHP function phpinfo() for security reasons (see https://www.drupal.org/node/243993). SystemInfoController::php() does not verify that phpinfo exists before using it. Potentially this could create php fatal error in systems where phpinfo is disabled

Proposed resolution

Add a check in SystemInfoController::php()to ensure that phpinfo() exists.

Remaining tasks

create patch.

User interface changes

none.

API changes

none.

CommentFileSizeAuthor
#4 d8-system-info-controller-phpinfo-check-2465467-4.patch910 byteswillzyx
#2 d8-system-info-controller-phpinfo-check-2465467-2.patch1017 bytesdom.
#1 d8-system-info-controller-phpinfo-check-2465467-1.patch1013 byteswillzyx

Comments

willzyx’s picture

willzyx commented
StatusFileSize
new d8-system-info-controller-phpinfo-check-2465467-1.patch1013 bytes
dom.’s picture

dom. commented
StatusFileSize
new d8-system-info-controller-phpinfo-check-2465467-2.patch1017 bytes

Hi !
- patch review for Drupal standard
- patch manually applied and checked
Therefore, patch #1 is RTBC+1 !

However I would suggest the following, thus not applying the RTBC tag. If you don't agree with the following, please apply the RTBC tag since #1 is ok :
- add "visit" in the last sentence since otherwise it is a fragment (looks weird that there are no verb, but I'm not native english so I'm probably wrong.)
For more information, visit <a href="@phpinfo">Enabling and disabling phpinfo()</a> handbook page.
- remove the empty lign before return statement.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +Documentation 
+++ b/core/modules/system/src/Controller/SystemInfoController.php
@@ -61,9 +61,14 @@ public function status() {
+      $output = t('The phpinfo() function has been disabled for security reasons. To see your server\'s phpinfo() information, change your PHP settings or contact your server administrator. For more information, visit <a href="@phpinfo">Enabling and disabling phpinfo()</a> handbook page.', array('@phpinfo' => 'http://drupal.org/node/243993'));

Not sure that the text makes sense, you suggest to revert security setting on server to enable phpinfo() that disabled for reason.
The link to d.o page makes a lot of sense, so I think text should be like.
Suppose The phpinfo() function has been disabled for security reasons. For more info....

willzyx’s picture

willzyx commented
StatusFileSize
new d8-system-info-controller-phpinfo-check-2465467-4.patch910 bytes

@Dom.
- it makes sense
- on coding standards there is no reference of the standard to follow for this thing. it's just a matter of preference. For me the change you made is fine

@andypost
It's the same sentence used in the status report page (see system_requirements()); we suggest to revert setting on server to enable phpinfo() IF you want to see your server's phpinfo() information.
However, the requested change makes sense

dom.’s picture

dom. commented
Status: Needs review » Reviewed & tested by the community

RTBC+1

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Huh. Had no idea this was an option. You learn something new every day. :)

Can't envision a useful way to automated test this, so...

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 3e58aba on 8.0.x 
    Issue #2465467 by willzyx, Dom., andypost: SystemInfoController::php()...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.