phpList / phplist3

Fully functional Open Source email marketing manager for creating, sending, integrating, and analysing email campaigns and newsletters.
https://www.phplist.org
GNU Affero General Public License v3.0
745 stars 269 forks source link

Issue inside subscribelib2.php #335

Closed mrojnetsky closed 5 years ago

mrojnetsky commented 6 years ago

Hi there, I'm having problem with the https://github.com/phpList/phplist3/blob/release-3.3.3/public_html/lists/admin/subscribelib2.php#L256

Above which it says "//# hack attempt..."

I'm sr. programmer (to give some context) but php newbie. What happens baffles me.

If I use code as is. I'm able to subscribe using phpList UI; however when I do it using "asubscribe" and my fancy ajax - the user is created and subscribed to the list (both user_user and listuser tables are updated); however the confirmation email is not sent to the user nor is a copy to admin.

I narrowed down the problem to the above mentioned code. If I comment out "exit;" on line 256 everything works perfectly. I receive all the emails.

Out of curiosity, I decided to debug it line-by-line old school way: printing out messages after each line of code (I'm sure there is a better way, but I don't know at this point how else you can debug site on a shared hosting.) Anyways to my surprise, ALL my request where falling into first part of the if fork. So, the condition on line 247 always evaluates to true. All the lines inside the first part of the if fork execute successfully, without any error.

That doesn't make any sense to me as a developer. If it never falls into the "else" clause there is no need to comment out "exit;" on line 256. But without commenting out I have the issue where the confirmation emails are not sent.

Sounds paradoxical to me, even though I got use to things like that. Or perhaps, my luck of php knowledge is the problem.

Please shed some light on the issue. Why it is behaving this way?

samtuke commented 6 years ago

@asdr45fsd35fdf So exit() is not being called, but commenting it out makes your custom ajax subscription process work?

It sounds like exit() must be called, but perhaps you are not detecting it. You could try logging an error (using trigger_error()) every time it is called (assuming PHP errors are being logged and you can access that log). You could also try using xdebug() to step through the code, if your IDE supports this, e.g. add a break point to that line so you can pause execution and check what logical sequence led to that point.

@xh3n1 may also have ideas.

michield commented 6 years ago
               $key = sprintf('%d', $key);
                if (!empty($key)) {

The %d forces the key to be numerical. If that evaluates to 0, it meant the original key was not numerical, which means someone has tried to push some other kind of code in, ergo "hack attempt".

Check that your ajax code doesn't push the "lists" variable to the page, and if it does, make sure the keys of the key-value pairs are numerical.

The asubscribe parameter expects only an email address and will fetch the rest (including which lists to subscribe to) from the subscribe page definition.

@samtuke I guess it could be useful to document the asubscribe call for everyone. It is currently only documented in the hosted account pages.

michield commented 6 years ago

by the way @asdr45fsd35fdf we would be able to be more serious if your handle wasn't some keysmash.

mrojnetsky commented 6 years ago

@samtuke correct! I don't understand why. I used Fatal_Error('Some info plus var value'); exit; Line by line in that area of the code. I could see in the ajax response what the values are for the vars and my custom text. The above mentioned if-statement always evaluates as true, in my case. I checked it multiple times. I might be crazy but that's what I see.

@michield it does push the list value and the key is numeric in the following way:

HTML part

<form method="post" name="subform" id="subform" enctype="multipart/form-data">
                            <input type=hidden name="htmlemail" value="1">
                            <input type="hidden" name="list[2]" value="signup"/>
                            <input type="hidden" name="subscribe" value="subscribe"/>
                            <input type="email" name="email" id="email" required>
                            <button class="btn" type="button" onclick="if (checkForm()) {submitForm();} return false;"></button>
                            <div class="alert alert-success" id="success" style="display:none"></div>
                            <div class="alert alert-danger" id="danger" style="display:none"></div>
                        </form>

JS part

function checkForm(){
     re = /^(([^<>()[\]\.,;:\s@\"]+(\.[^<>()[\]\.,;:\s@\"]+)*)|(\".+\"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/;

    if (!(re.test(jQuery("#email").val()))) {
        jQuery("#success").empty().hide();
        jQuery("#danger").empty().append("Please enter a valid email.").show();
        jQuery("#email").focus();
        return false;
    }
    return true;
}
function submitForm() {
    successMsg = 'Thank you for your registration.';
    failMsg = 'Sorry, we were unable to register you.';
    data = jQuery('#subform').serialize();
    jQuery.ajax( {
        type: 'POST',
        data: data,
        url: '/phplist/?p=asubscribe',
        dataType: 'html',
        success: function (data, status, request) {
            if(data==='FAIL'){
                jQuery("#success").empty().hide();
                jQuery("#danger").empty().append(failMsg).show(); 
            }else{
                jQuery("#danger").empty().hide();
                jQuery("#success").empty().append(successMsg).show();
                jQuery('#email').val('');
            }
        },
        error: function (request, status, error) { 
            jQuery("#success").empty().hide();
            jQuery("#danger").empty().append(failMsg).show(); 
        }
    });
}

I borrowed and adapted this code from this thread https://discuss.phplist.org/t/ajax-subscribe-api/974/2 There they push the list the same way.

I will try not to push the list value as you suggested and see if it works better. Last thing I want to do is to modify phpList code in any way, complicating future updates.

@michield your wish is my command. I do keysmash for all my accounts so that I not only don't know the randomly generated password but the username cannot be tied to any other accounts that uses the same username. It is a habit that I recommend to anyone given the perpetual hacking that's going on. But I can see how it might go against GitHub policy or invoke prejudice.

*Update: trying to fix code formatting

marianaballa commented 5 years ago

It seems that there are no updates on this issue for more than a year. For future questions/proposals, I would recommend the community forum or Mantis, the phpList bug tracker.

Closing this.