Bug in S2Member - global $base not prefixed - change to $s2-base

#1

I started suffering server crashes when using S2Member with WDesk. My host identified a bug in both products and I included their analysis below.

  • Please fix this bug

Thanks
James

My WordPress installation uses S2Member.

Thanks for writing! We’re sorry to hear you’re having trouble.

We see what’s wrong. You wrote this:

By design S2Member looks for
the following code in .htaccess and if it is not present adds it.

BEGIN s2Member GZIP exclusions

RewriteEngine On RewriteBase premium RewriteCond %{QUERY_STRING} (^| ?|&)s2member_file_download =.+ [OR] RewriteCond %{QUERY_STRING} (^| ?|&)no-gzip =1 RewriteRule .* - [E=no-gzip:1] # END s2Member GZIP exclusions

This code conflicts with your hosting and causes 500 errors.

This isn’t a conflict with our hosting, though; the line “RewriteBase premium” is clearly wrong, and won’t work on any hosting. It’s supposed to be “RewriteBase /”, not “RewriteBase premium”.

We’ve looked into that for you, and the error is caused by a conflict between two plugins, “s2Member Framework” and “WSDesk - WordPress Support Desk” (the “premium” version). Both of these plugins are doing something they shouldn’t, and using them together causes this problem (it wouldn’t happen if only one or the other was used).

The technical description that you can forward to the authors of both plugins is this:

In “WSDesk - WordPress Support Desk”, the “wsdesk.php” file sets a variable called “$base” to the value “premium” at line 54:

switch (‘BASIC’) {
case ‘PREMIUM’:
$conflict = ‘basic’;
$base = ‘premium’;

It then makes that variable “global” at line 105:

function eh_crm_update_function() {
global $base;

Making it global means that its value can “leak” into other code.

In “s2Member Framework”, the
“src/includes/templates/cfg-files/s2member-files.php” file also looks for a global variable called “$base” at line 6:

global /* A Multisite $base configuration? */ $base;
$ws_plugin__s2member_temp_s_base = (!empty($base)) ? $base : c_ws_plugin__s2member_utils_urls::parse_url (network_home_url (’/’), PHP_URL_PATH);

If it finds such a variable, it uses its contents (“premium”) instead of using the WordPress “network_home_url” value.

The bug is caused by “s2Member Framework” using a completely unexpected value of “$base” set by the unrelated “WSDesk - WordPress Support Desk”
plugin.

The solution to this is for both developers to prefix their variable names, instead of using generic names like “$base”. For example, the variable could be called “$ws_desk_base” in “WSDesk - WordPress Support Desk”, and called “$s2_member_base” in “s2Member Framework”. The WordPress programmer site says that all variables should be prefixed like that:

https://developer.wordpress.org/plugins/plugin-basics/best-practices/

If either one of these plugins followed that rule, the problem would not happen (although both of them should fix it).

#2

Hi James.

Thanks for writing. I got intrigued by your problem, so I spent a while searching through the code studying it.

s2Member has its names prefixed, as is best practice. $base is not an s2Member variable, s2 is checking to see if a custom base address was defined for the network, and when it wasn’t it uses a default.

The problem seems that the other plugin changed its value. Since s2’s rewrite base is “premium” in your .htaccess, it seems true that WSDesk changed it in the global space before s2Member got to it.

I could remove that $base check for custom base addresses in networks, but I risk breaking sites that already rely on it. You’re the first one I see mention having a problem with it, and it’s from the conflict mentioned above.

You could remove that check s2 does for the $global address from your file, but you’d need to do it after each update too. It’d be good that you contact the other plugin so they can improve their implementation in a way that they don’t alter that variable’s value.

I will make a note of this to keep in mind, and see if it can be improved in the future…

:slight_smile:

#3

Thank you. I have passed your comments on to the developer of WSDesk. Looking back at older forum posts I can see that some other users were affected by this issue. It is good that we have now identified it.

1 Like
#4

Thank you for your detailed explanation. The developer at WDesk says he will fix this issue in the next release. We have helped make the world a better place.

1 Like
#5

:smile::+1: