Site Health: Call to API with $_COOKIE and PHPSESSID

[matthieumota]

We can see on base code (https://core.trac.wordpress.org/browser/branches/5.2/src/wp-admin/includes/class-wp-site-health.php?rev=45347#L1648) that complete $_COOKIE are parsed to api call for check on site health. Not worries but $_COOKIE contains PHPSESSID key.

The PHP script in curl waits for the session to free itself from the first script that calls the API. This causes a timeout with CURL.

Many solutions to resolve that, unset PHPSESSID from cURL call or use ​https://php.net/manual/en/function.session-write-close.php to end session before cURL call.

Bug is also present on https://core.trac.wordpress.org/browser/branches/5.2/src/wp-admin/includes/class-wp-site-health-auto-updates.php?rev=45347#L93 and https://core.trac.wordpress.org/browser/branches/5.2/src/wp-admin/includes/class-wp-site-health.php?rev=45347#L1943

[netweblogic]

This issue causes multiple tests to fail in the site health interface if your theme or plugin starts a session. The following errors are triggered by curl timeouts:

• The REST API encountered an error
• Your site could not complete a loopback request
• Background updates may not be working properly
  • Could not confirm that the wp_version_check() filter is available.

By the following code:

<?php
add_action('init', function(){
        session_start();
});

Thanks for reporting this @matthieumota

I would say removing session data from the curl call is the most appropriate solution, given these tests that fail don't require sessions to pass/fail, whereas session data may still be needed in the original page load.

[Clorith]

You're quite right that those tests will all fail, since they all rely on the site calling it self.

We've known that incorrect session usage in plugins has been a problem in the past (most notably made known when the plugin and theme editors were updated to perform loopback checks to ensure you didn't crash your own site during an edit), but in those cases plugins have been updated to treat sessions differently and the problems have gone away.

I must admit, I don't recall which plugins, or know what they changed though.

As for stripping out session id from the cookie which is passed, this may backfire, what if a site has been modified to rely on sessions to validate a login beyond the regular WordPress auth checks, do we have some good way of approaching a potential scenario like this?

Also making a note that any changes made to the tests need to be replicated elsewhere in core where loopbacks are performed so that the behavior is consistent.

[netweblogic]

@Clorith thanks for the feedback... upon some further research, making use of session_write_close() and keeping sessions open/locked only when we know it'll be needed for writing seems to have done the trick and probably a better approach anyway.

For anyone with a similar problem, maybe this explains it better, my previous snippet failed the site health tests, this one doesn't:

<?php
add_action('init', function(){
    session_start(); //this loads variables to $_SESSION for reading
    if( empty($your_plugin_needs_session_saving) ){
        session_write_close(); //other plugins can restart a session again via session_start()
    } // if session writing is needed, close session after writing is done
});

I'm not sure if this would work for all use cases, but in my case I only need to write to sessions when particular actions are taken, avoiding (hopefully) all situations where there'd be a conflict due to session locking.

The only argument I'd make towards it possibly being a bug in Site Health is that in my case, until now, my plugin session handling worked fine with all other WP functionality (at least, I've not been made aware of any issues).

[matthieumota]

I think it will be better to close session in the plugin code base no ? Everybody get this issue I think...

We can easily resolve the bug on code. I can write that but I need to take the time to understand the rules of contributions, it is the most time-consuming and I've no time at the moment...

[netweblogic]

Replying to matthieumota:

I think it will be better to close session in the plugin code base no ? Everybody get this issue I think...

I think what @Clorith means is the problem with 'fixing' the site check, in this case, is that the way we're using sessions might be 'wrong'. Other parts of core might do the same thing and therefore our code would cause problems there too, hence the site check giving a warning of the potential problem.

I'd be interested to know what other instances a loopback like this occurs in core, requiring this check to also maintain session data the same way. The only thing that springs to mind is maybe multiple modifications in the block editor triggering multiple async API calls. Crons like updates for example I believe are fired off independently without any user/session data?

[desrosj]

Moving Site Health tickets into their lovely new home, the Site Health component.

[Clorith]

You are correct that cron events fire a loopback, but it does not rely on authentication.

The theme and plugin editors perform these kind of checks, and will not allow your changes to be saved if there is a failure (and is how we found the session problem in the first place, when many users started having this issue, the plugins that were adding sessions were updated in those cases though).

I am leaning towards not leaving session locking active when not needed being the best approach here, the scenario I describe above is also the only one I can think of right now where the loopbacks require authentication, but as mentioned it is also where we saw a noteworthy amount of users experience issues until plugin changes were made.

I'm going to leave the ticket open for feedback on these points as well, but have marked it as a candidate for closing soon unless something special comes up.

[netweblogic]

For anyone wondering, if you open a session early to load up $_SESSION variables, and then session_write_close(), you can reopen it and write at any point in time by just using session_start() again, writing what you need and closing it right after.

If the session is open early on, headers are sent and starting a session even after headers are sent is not going to cause any warnings/errors.

I've found in my case this gets around any session locking issues and allows you to read/write to sessions without any hindrance.

[Clorith]

It's been a few weeks, and there's nothing new here, so I'm closing this for now.

Unfortunately the most appropriate close reason is wontfix, this is just a trac workflow thing, and if something comes up in the future that warrants further investigation here, that is of course not a problem.

[SergeyBiryukov]

Replying to Clorith:

The theme and plugin editors perform these kind of checks, and will not allow your changes to be saved if there is a failure (and is how we found the session problem in the first place, when many users started having this issue, the plugins that were adding sessions were updated in those cases though).

Related: #43358

I think Site Health should detect an active PHP session and provide a more specific message than just requests timing out. This would help with troubleshooting.

47320.diff​ adds a test for this. Any suggestions on the wording welcome :)

[afragen]

In looking through the 47320.diff​ perhaps a few sentence modifications.

An active PHP session detected -> An active PHP session was detected

I there any way to determine the source of the session_start() call? This information would be greatly informative but likely very difficult to obtain.

Otherwise the patch looks good to me.

Patch applies cleanly, like there was any doubt ;)

[SergeyBiryukov]

Replying to afragen:

I there any way to determine the source of the session_start() call? This information would be greatly informative but likely very difficult to obtain.

I haven't found a way to determine that. It looks like including ​session_name() would also be helpful, but that can be done later.

Otherwise the patch looks good to me. Patch applies cleanly, like there was any doubt ;)

Thanks for the review! :)

[SergeyBiryukov]

In 47585:

Site Health: Detect an active PHP session as a possible reason for HTTP requests timing out.

PHP sessions created by a session_start() function call may interfere with REST API and loopback requests.

An active session should be closed by session_write_close() before making any HTTP requests.

Props matthieumota, netweblogic, Clorith, afragen, vjik, SergeyBiryukov.
Fixes #47320.

[cfm168]

Hi @netweblogic,

Where to add these codes? I want to try it. Thanks in advance!

Replying to netweblogic:

@Clorith thanks for the feedback... upon some further research, making use of session_write_close() and keeping sessions open/locked only when we know it'll be needed for writing seems to have done the trick and probably a better approach anyway.

For anyone with a similar problem, maybe this explains it better, my previous snippet failed the site health tests, this one doesn't:

<?php
add_action('init', function(){
    session_start(); //this loads variables to $_SESSION for reading
    if( empty($your_plugin_needs_session_saving) ){
        session_write_close(); //other plugins can restart a session again via session_start()
    } // if session writing is needed, close session after writing is done
});

I'm not sure if this would work for all use cases, but in my case I only need to write to sessions when particular actions are taken, avoiding (hopefully) all situations where there'd be a conflict due to session locking.

The only argument I'd make towards it possibly being a bug in Site Health is that in my case, until now, my plugin session handling worked fine with all other WP functionality (at least, I've not been made aware of any issues).

[netweblogic]

@cfm168 This is a regular wp snippet, you could add it to your theme's functions.php file for example.

Replying to cfm168:

Hi @netweblogic,

Where to add these codes? I want to try it. Thanks in advance!

Replying to netweblogic:

@Clorith thanks for the feedback... upon some further research, making use of session_write_close() and keeping sessions open/locked only when we know it'll be needed for writing seems to have done the trick and probably a better approach anyway.

For anyone with a similar problem, maybe this explains it better, my previous snippet failed the site health tests, this one doesn't:

<?php
add_action('init', function(){
    session_start(); //this loads variables to $_SESSION for reading
    if( empty($your_plugin_needs_session_saving) ){
        session_write_close(); //other plugins can restart a session again via session_start()
    } // if session writing is needed, close session after writing is done
});

I'm not sure if this would work for all use cases, but in my case I only need to write to sessions when particular actions are taken, avoiding (hopefully) all situations where there'd be a conflict due to session locking.

The only argument I'd make towards it possibly being a bug in Site Health is that in my case, until now, my plugin session handling worked fine with all other WP functionality (at least, I've not been made aware of any issues).

[cfm168]

Thank you for advising.
I found two plugins causing the site health issues. Deactivate these two plugins all three critical issues are disappeared and site health status 0 issue. I will have the developers to fix the problem.
Will try your code if the issues persist after developers helpless.

Thank you again!

[cfm168]

Replying to netweblogic:

@cfm168 This is a regular wp snippet, you could add it to your theme's functions.php file for example.

Replying to cfm168:

Hi @netweblogic,

Where to add these codes? I want to try it. Thanks in advance!

Replying to netweblogic:

@Clorith thanks for the feedback... upon some further research, making use of session_write_close() and keeping sessions open/locked only when we know it'll be needed for writing seems to have done the trick and probably a better approach anyway.

For anyone with a similar problem, maybe this explains it better, my previous snippet failed the site health tests, this one doesn't:

<?php
add_action('init', function(){
    session_start(); //this loads variables to $_SESSION for reading
    if( empty($your_plugin_needs_session_saving) ){
        session_write_close(); //other plugins can restart a session again via session_start()
    } // if session writing is needed, close session after writing is done
});

I'm not sure if this would work for all use cases, but in my case I only need to write to sessions when particular actions are taken, avoiding (hopefully) all situations where there'd be a conflict due to session locking.

The only argument I'd make towards it possibly being a bug in Site Health is that in my case, until now, my plugin session handling worked fine with all other WP functionality (at least, I've not been made aware of any issues).