(s2Member / API Notification Email) - Payment - wrong user data

Nice troubleshooting work!

So actually after deleting those keys - s2member will start writing new keys to the old user ID 288…

That’s the user that keeps coming up in your logs? That’s so weird… There must be something hardcoded putting that in…

It’s most likely not in the s2Member files, or it would have been overwritten by any of the updates.

That’s why I kept asking about cache or custom code… Something is putting that number there. And it’s not a random thing, if it’s always the same user.

Can you search all your files, and all your database, for “288” and see what you get?

Okay - I found the error - I was a bit blind to not see this earlier - but here is the ultimate recipe to crash s2member. And if you have many users and sometimes need to copy/paste values because a transaction was lost - it’s not that unlikely to happen - the custom field where the information should have gone - is just below the cid field!

wp_s2member_subscr_cid:openmtbap.org

With openmtbmap.org usually being my custom value/domain. Another value does not cause problems. But my domain does. If I enter my domain to another user in the database - that user will then show up in the payment notifications - and that user will have data added to his profile!

Maybe this happened when I entered some data for a user whos subscription was lost due to the old paypal bug that sometimes users were not created. As soon as I add this into a line of my database the above mentioned bugs start happening!

Can you please try to reproduce it - if not I can send you a wp_users and wp_usermeta database table to play with. And then check why it causes s2member to run havoc and fix it. Because a wrongly entered value should NEVER let a program go crazy (stripe subscriptions not demoted, wrong payment api email sent, maybe more problems). And no that user is certainly not hardcoded…
Also the user changed over time - that is because I guess this happened several times - but once the user got demoted - the problem disappeared because the cid value will be deleted.

You can even enter the domain into a CID field of a subscriber - without any other info - it will cause the same problem!

Actually copying in your domain into the Paid Subscr. ID: field will cause the same mess.
And searching my database for 288 was one of my first things to do - it’s just that everything looked fine - on just reading over this field - I did not directly notice that it’s a wrong value. I was rather looking for empty field or some gibberish - but not just some correct information in the wrong field.

Nice work finding the culprit! :smiley:

Thank you for the details and sharing your findings.

Very interesting, too…

I dug in the code a bit, but haven’t spotted exactly where the mixup is happening yet. I’m suspecting it’s from one of the methods in s2member/src/includes/classes/utils-users.inc.php.

I’m making a note of this, to keep in mind to see how I can prevent it from happening to others in the future. :+1:

thanks - because likely I’m not the only one who did this mistake in the past. Some others reported they had problems with users not demoting (Stripe). For all the years we had the problem with paypal not waiting. That meant that about 1% of all users needed to be entered by hand. Then if you once copy in one field to high - the bug started. I have not yet confirmed that the users not being demoted is caused by this - but it seems likely that those fields at the wrong user - being constantly overwritten cause some mess. Even though all the visible fields were correct. I will check that up in a couple of days.

Bug:
(s2Member) Bug Fix: PayPal would sometimes return the customer without the Custom Value expected by s2Member, incorrectly triggering an error. A small delay has now been added when needed to wait for PayPal to provide the missing value, so that the customer is met with the correct success message on return. Props to Josh Hartman for his help.

I don’t know if it’s related to that, but I certainly see how this situation can come about for others as it did for you.

It would be nice to have some validation for the field’s value on the form, if that’s possible. But the mix-up shouldn’t happen even if one user did have that value entered incorrectly.

I think the problem may be in get_user_id_with, if the 2nd argument ($os0) is also passed from somewhere, then this query would be used:

SELECT `user_id` 
FROM `".$wpdb->usermeta."` 
WHERE (
     `meta_key` = '".$wpdb->prefix."s2member_subscr_id' 
  OR `meta_key` = '".$wpdb->prefix."s2member_subscr_baid' 
  OR `meta_key` = '".$wpdb->prefix."s2member_subscr_cid' 
  OR `meta_key` = '".$wpdb->prefix."s2member_first_payment_txn_id'
) 
AND (
     `meta_value` = '".esc_sql($subscr_txn_baid_cid_id)."' 
  OR `meta_value` = '".esc_sql($os0)."'
) 
LIMIT 1

$os0 could get the value of option_selection1. E.g. s2member/src/includes/classes/paypal-return-in-subscr-or-wa-w-level.inc.php

$user_id = c_ws_plugin__s2member_utils_users::get_user_id_with($paypal['subscr_id'], $paypal['option_selection1'])

option_selection1 is meant for the user ID when updating an existing user, but for new users it defaults to the domain name, same as the custom value. At least that’s what I find that Jason did in s2member-pro/src/includes/classes/gateways/stripe/stripe-checkout-in.inc.php for example.

$ipn['option_name1']      = 'Referencing Customer ID';
$ipn['option_selection1'] = $old__subscr_or_wp_id;

or if a new user

$ipn['option_name1']      = 'Originating Domain';
$ipn['option_selection1'] = $_SERVER['HTTP_HOST'];

So if you now enter the domain name as the subscr CID for a user in his profile, as you described doing by mistake, maybe you can have some situations where the wrong user could be picked up by the query, because it would find a match here:

`meta_key` = '".$wpdb->prefix."s2member_subscr_cid' 
`meta_value` = '".esc_sql($os0)."'

Problem is that the line that calls get_user_id_with is not checking if it’s ‘Referencing Customer ID’ or ‘Originating Domain’, it just uses ‘option_selection1’. That should only be used when it’s actually an ID, not something else.

Of course all this happens under the rare case that you experienced, but it should be prevented. I’ll see what I can do, but I think that adding the ID check (i.e. is it an integer) in get_user_id_with, will be the simplest solution. E.g. $os0 == (int)$os0 maybe?

if($subscr_txn_baid_cid_id && !empty($os0) && $os0 == (int)$os0)

Do you want to try that improved conditional?

Maybe I’m wrong somewhere there, but I think that it would be safer to validate that 2nd argument as an actual ID, because it shouldn’t be anything else, and although I’m still not sure if it could ever be queried when the domain name is there, it seems possible from what I see, and it shouldn’t be used.

Ideally, there would be a refactor to not use the same variable for two completely different things, but that will not happen now. I don’t know in what other places Jason expected the domain name in that field. I’ll just avoid this kind of thing in the new s2Member I want to build.

well I’m not sure I don’t think I can debug much more.
Just note that the payment api emails / user creation error did not happen for paypal subscriptions. They did happen for paypal one time buy, and all stripe payments.
Paypal subscriptions are cancelled correctly - I’m not sure yet about stripe subscriptions (could be another bug but I doubt it - I’m pretty positive it’s the same bug).

CID is a field used only in the Stripe integration.

I know it says PayPal in some of the lines I mentioned earlier… It’s a bit complicated, but the original PayPal routines from the Framework got reused to process the other gateways. They’re more like core routines now, the way Jason used them, but kept the PayPal name in them. It’s part of what I needed to figure out for the Stripe upgrade.

yes I know - however if for paypal or demoted user the CID field contains an I-… or other content - it’s okay. The only thing really crashing s2member is the domain name here.

I often demoted my stripe subscription users without deleting their subscription ID/CID - and that caused no problems.

Maybe you can change the query that it only looks at CID if payment gateway is Stripe? (not sure if others need this one too). And if user is subscriber neither check ID nor CID.

why is this used at all? In case something went missing? I even think maybe I should clean my database and delete this key from wp_usermeta table.
And yeah - option selection 1 should only be checked against option selection metakey, not against others. I know that stripe sends IPN to all handles - so if you have several websites then Option selection is used in order for s2member to know it’s the wrong website. However the subscr_id is unique anyhow - so not sure why this is checked additionally.

Oh yeah - the ID is not an integer - for Stripe it starts with CH or PID (live/test) for Paypal with ID - so checking for integer will not work I think. Also it may contains some more letter (my paypal IPN mostly start with 6J). However as they start differently - there is no way that a stripe ID can be mixed up with a Paypal ID or vice versa. Also it cannot happen that the ID matches two websites. I therefore do not see why the os1 is used at all! Actually without checking for os1 (or even making it optional) one could have one user subscribing with one payment to two websites (rare case, but would be nice for me). For paypal this will not work of course - because paypal sends IPN only to one destination - not several like Stripe (Stripe sends IPN to every IPN handler you have entered in your account).

I therefore do not see why the os1 is used at all!

Because $os0 may get the ID of the existing user (i.e. if it’s an upgrade, the part that says “Referencing Customer ID” ), so it needs to be there. It has to be an integer, though, that’s why I thought of adding that extra check, then it will only work for WP user IDs.

users upgrading were always fine. Just new users got bugged here. Also stripe subscriptions for users upgrading were demoted correctly - so for logged in users this bug does not exist!
(but yeah - that code is a bit of a mess).

o for logged in users this bug does not exist!

Exactly, because in that case $os0 would have the existing customer ID, not the domain name.

When it’s a new user, sometimes $os0 would be the domain name, and the query would find a match first in that row for the user with the domain in the subscr_cid field.

Maybe it’s easier to just run a separate query checking that os1 is not existing in ID or CID field.
If it does then delete. That way you do not need to make a complicated rewrite of the way s2member works.

In general I think subscription have to run only against the visible Fields in the user profile. If those hidden fields change something, they should be shown in the users information UI too.
I mean the txn/first and so…

With openmtbmap.org usually being my custom value/domain. Another value does not cause problems. But my domain does.

Have you tried other values that contain periods?

look at the code above - it explains why the domain causes problems.

SELECT user_id
FROM ".$wpdb->usermeta."
WHERE (
meta_key = ‘".$wpdb->prefix."s2member_subscr_id’
OR meta_key = ‘".$wpdb->prefix."s2member_subscr_baid’
OR meta_key = ‘".$wpdb->prefix."s2member_subscr_cid’
OR meta_key = ‘".$wpdb->prefix."s2member_first_payment_txn_id’
)
AND (
meta_value = ‘".esc_sql($subscr_txn_baid_cid_id)."’
OR meta_value = ‘".esc_sql($os0)."’
)
LIMIT 1

So actually anything else would be fine - because the other values would still lead to the right user being found - and not the wrong one.

Not really, which is why I asked about it.

But if you have it figured out already, great.

I’m glad you fixed it.

Did changing your Cloudflare orange clouds to gray ones fix it?

I’ve never used cloudflare except for DNS. But no - I don’t think this has any influence on the payment api email including wrong data. If you have problems with that email - then make sure the custom value os1 is nothing but your domain. While the above error I am pretty sure is irrelevant to other values - there could be other bugs that happen due to custom value.

Basically most of the s2member related bugs I encountered happened due to this. (the really other clear bug - meaning demotion after paypal complaints/cases - is not related to this. S2member has no correct logic here as simple as that). It’s best to deactivate demotion for complaints and do it manually because s2member logic is flawed/insufficient.