WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 4 months ago

Last modified 4 months ago

#43358 closed defect (bug) (fixed)

The theme/plugin editor sandbox does not play well with PHP sessions

Reported by: bruandet Owned by: SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: normal Version: 4.9.4
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

The theme/plugin editor sandbox does not work with some plugins (or themes) using PHP sessions. Because of the session exclusive lock, cURL will time out when attempting to connect back to the site and the request will fail.
Checking if a session was started and then calling session_write_close() before the first wp_remote_get() call in wp-admin/includes/file.php seems to solve the issue.

Attachments (2)

session-site-health-bug.php (71 bytes) - added by vjik 9 months ago.
Simple plugin for reproducing the problem
43358.diff (716 bytes) - added by SergeyBiryukov 4 months ago.

Download all attachments as: .zip

Change History (8)

#1 @SergeyBiryukov
9 months ago

  • Milestone changed from Awaiting Review to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

@vjik reported that this issue also affects Site Health checks, specifically causing these warnings:

  • The REST API encountered an error
  • Your site could not complete a loopback request

@vjik
9 months ago

Simple plugin for reproducing the problem

#2 follow-up: @donmhico
6 months ago

  • Keywords dev-feedback added

+1 to @bruandet. Adding session_write_close() before the first wp_remote_get() specifically here - https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/file.php#L554 - fixes the issue for the Plugin and Theme editor. However it does not fix the issue found in Site Health checks.

Do you guys think that closing the session during the Site Health checks is the proper solution? It feels kinda hacky if you ask me. Any other ideas / approach?

#3 @audrasjb
5 months ago

  • Milestone changed from 5.4 to Future Release

Hi,

With 5.4 Beta 3 approaching and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#4 in reply to: ↑ 2 @SergeyBiryukov
4 months ago

  • Keywords has-patch added; dev-feedback removed
  • Milestone changed from Future Release to 5.5

Replying to donmhico:

Adding session_write_close() before the first wp_remote_get() specifically here - https://core.trac.wordpress.org/browser/trunk/src/wp-admin/includes/file.php#L554 - fixes the issue for the Plugin and Theme editor. However it does not fix the issue found in Site Health checks.

Do you guys think that closing the session during the Site Health checks is the proper solution? It feels kinda hacky if you ask me. Any other ideas / approach?

Agreed. Closing the session seems appropriate for plugin/theme editor, but not for Site Health checks. If there's an issue with these checks, there's likely an actual issue with loopback requests, REST API, etc.

Ideally, Site Health should detect an active PHP session and provide a more specific message than just requests timing out. Looks like this is already discussed in #47320 though, so let's just fix the plugin/theme editor here.

43358.diff works in my testing.

#5 @SergeyBiryukov
4 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 47457:

Administration: When editing a plugin or theme file, close any active PHP session.

This prevents HTTP requests from timing out when attempting to connect back to the site.

Props bruandet, vjik, donmhico, SergeyBiryukov.
Fixes #43358.

This ticket was mentioned in Slack in #core-site-health by sergey. View the logs.


4 months ago

Note: See TracTickets for help on using tickets.