Problem/Motivation

This is a follow up to SA-CORE-2023-004

https://git.drupalcode.org/project/drupal/-/commit/b4aa82d4486465eac7a13...

This change removed sections of the phpinfo() page which can contain sensitive info.

Whilst Drupal core should have a safe default for this (which it now does) it would be useful for sites to be able to configure phpinfo(); it's often quite useful to see PHP superglobals for debugging, for example.

We should include an appropriate note of caution about this setting, and perhaps encourage people to consider making any changes temporary etc..

Steps to reproduce

Visit /admin/reports/status/php as user with appropriate permissions.

Proposed resolution

Make the options passed to phpinfo() configurable.

Release notes snippet

A new setting $settings['sa_core_2023_004_phpinfo_flags'] in default.settings.php has been added to configure the behaviour of admin/reports/status/php.

CommentFileSizeAuthor
#9 Screenshot from 2023-05-15 18-26-21.png97.66 KBshashank5563
#3 3358514-3.patch3.95 KBpoker10
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mcdruid created an issue. See original summary.

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented
Version: 11.0.x-dev » 10.1.x-dev
poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
Status: Active » Needs review
FileSize
3358514-3.patch3.95 KB

Here is the patch similar to the D7 one, which uses the static Settings::get() (instead of injecting the Settings to the SystemInfoController). Let's check the testbot.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs change record

Can we add a simple change record for the new setting being proposed

Thanks!

poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
Status: Needs work » Needs review
Issue tags: -Needs change record

Thanks for the suggestion! Created a draft CR here: https://www.drupal.org/node/3360166. Text is similar to the one we are adding as docs in the settings.php.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Thanks. Change looks good to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 3358514-3.patch, failed testing. View results

poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
Status: Needs work » Reviewed & tested by the community

Seems like a testbot issue, moving back to RTBC.

shashank5563’s picture

shashank5563 CreditAttribution: shashank5563 at Melity commented
FileSize
Screenshot from 2023-05-15 18-26-21.png97.66 KB

I have test on my and found everything is looks good. I am moving to RTBC+1

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 3358514-3.patch, failed testing. View results

poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
Status: Needs work » Reviewed & tested by the community

Seems like there are issues with 10.1.x core tests, see: https://www.drupal.org/pift-ci-job/2666817

Restoring status.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue tags: +Needs change record, +Needs release note

Patch looks fine but this needs a change record and release note mentioning the setting.php changes.

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented
Issue tags: -Needs change record

I think we've got the draft CR linked already; we can add a release note snippet to the IS.

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented
Issue summary: View changes
Issue tags: -Needs release note

Added a release note snippet.

@catch, please let us know if this needs anything else. Thanks!

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid at Acquia commented
Issue summary: View changes

  • catch committed d08fdb07 on 11.x 
    Issue #3358514 by poker10, mcdruid, smustgrave: Make phpinfo on the...

  • catch committed b8b9c54f on 10.1.x 
    Issue #3358514 by poker10, mcdruid, smustgrave: Make phpinfo on the...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed
Issue tags: +10.1.0 release notes

Committed d08fdb0 and pushed to 11.x, cherry-picked to 10.1.x. Thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 11.x-dev » 10.1.x-dev
ressa’s picture

ressa CreditAttribution: ressa at Ardea commented
Issue summary: View changes

Adding link to https://www.drupal.org/sa-core-2023-004 in Issue Summary.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

Added branch/version to the CR and published it.

Status: Fixed » Closed (fixed)

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