Plugin - Advance Match - Chad Smith

[skybird33]

I recently updated openvbx to 1.2.18 r79 (from 1.2.13) and messages that are forwarded to the Inbox after flowing through the Match plugin all contain the message "deleted". I log into twilio and verified that the messages do contain text. Anyone else have this problem?

[Natorator]

AHA! Yes I have this problem, and I think I'm close to a solution.

I just upgraded from OpenVBX 1.2.13 on php 5.3 (Ubuntu precise) to OpenVBX 1.2.18 on php5.5 (Ubuntu trusty), and I had that exact same issue.

The problem isn't in any given plugin or applet, but in the way Twiml::applet() and PHP handle cookies.

in OpenVBX/controllers/twiml.php , in the applet() function, the set_cookie() call on line 186:

set_cookie('sms-body', null, time()-3600);

This section of code runs on any twiml request that does //not// start a flow (so, on the first applet request) . Before I upgraded, I'm guessing this line was intended to delete the sms-body cookie, after which subsequent requests would find it missing and defer to the $_GET or $_POST variables. I'm not sure about that, but I can't see how it worked otherwise.

Unfortunately, it seems that the effect after I upgraded to php 5.5 was not to delete the cookie, but to replace it with the value of 'deleted'. With that in mind, check out the php docs for the setcookie() function, which set_cookie() passes through to:

http://php.net/manual/en/function.setcookie.php

Cookies must be deleted with the same parameters as they were set with. If the value argument is an empty string, or FALSE, and all other arguments match a previous call to setcookie, then the cookie with the specified name will be deleted from the remote client. This is internally achieved by setting value to 'deleted' and expiration time to one year in past.

That sure looks like the problem , don't you think?

For now, I'm just always distrusting the cookie by setting the conditional at line 175 thusly:

if(true or isset($_REQUEST['Body']) && $inst_id == 'start')

Does someone who knows more about this section of code's intent have a better solution?

[Natorator]

On second thought, I think the intent of the set_cookie() call was not to delete the sms-body cookie, but just to extend its expiration, instead. So the bug would be that setcookie()'s behavior changed, so that passing a null $value is now interpreted as an intent to delete the cookie.

Am I right?

[Gipetto]

That's pretty good sleuthing. time() - 3600 is definitely used to expire a cookie. But here, unfortunately, the built in set_cookie function makes that parameter misleading. time()-3600 does end up extending the life of the cookie, but I really doubt that is the desired behavior here.

This needs more investigation but I think you're on the right track. I'll dig in here too and see what I can figure out.

[Gipetto]

After looking in to this I think the bug is probably the existence of line 186. The line's addition was done in July of 2011 as part of a much larger commit that mostly dealt with a jQuery upgrade, so I have no idea what the logic behind adding this was. And I have no idea how it is just now becoming apparent. I notice that the session id between OpenVBX and Twilio is the same across multiple interactions, so it may be that the session between the two is now staying open for too long. Again, that's hard to tell.

If I comment out line 186 I no longer see the deleted message behavior and I think the removal of that line is probably the correct fix. The only other thought I have is that the cookie should be deleted after the last applet in the flow has fired, but I'm gonna have to look further to see if there's even a decent way that an applet can know if he's last. There probably is, and it may be as simple as looking to see if there's a next applet defined. If so we could consider deleting the cookie then to be nice and clean up.

My concern now is that it may be possible that if conversations overlap that we could end up getting conversations mixed up because of a long running session id.

[Gipetto]

Oh, good, we shouldn't get conversation overlap, but it could happen in rare circumstances. Per the Twilio quick start on SMS conversations:

you can have a unique cookie for each To/From phone number pair

Also: https://www.twilio.com/help/faq/twilio-basics/how-do-twilio-cookies-work

So, considering how OpenVBX works, the probability of overlapping conversations is low, but not impossible.

[Natorator]

I don't think you //can// delete Twilio's client cookies. I haven't been able to on my new server setup, anyway. I've tried passing false, and '' as well as null as the set_cookie() $value param, but it just keeps getting set to 'deleted'. I haven't tried something like setrawcookie() or even header(), though.

The link you cited says that Twilio sets their cookie expirations to four hours - it's possible they're ignoring back-dated expirations. But, that wouldn't explain why this bug did not occur on my PHP 5.3 installation.

In PHP 5.5, setcookie() added a Max-age parameter to the set-Cookie header it generates. Maybe that's confusing the Twilio client?

Passing the $sms variable as the value parameter to set_cookie(), instead of null, also fixes the bug:

set_cookie('sms-body', $sms, time()-3600);

This works, because even if the cookie is still not deleted on Twilio's client, at least the value remains the same. If there was any utility to this with regard to expiring or deleting the cookie, doing it this way might preserve that.

Deleting the line would probably do just as well, though. The value of the cookie gets re-set at the start of every flow for that number pairing anyway.

[skybird33]

Thanks for your help!

I used the $sms variable as the value parameter and it is working again.

[rubin110]

I'm running into the same issue. replacing null with $sms on line 188 within the following else worked to fix my issue. Hope I didn't break anything else.

[mfkp]

This also fixed my issues.

Somebody should open up a pull request for this one so it doesn't break the next time I update.

Thanks for figuring this one out!