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

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.