Vonage / vonage-ruby-sdk

Vonage REST API client for Ruby. API support for SMS, Voice, Text-to-Speech, Numbers, Verify (2FA) and more.
https://developer.vonage.com
Apache License 2.0
217 stars 108 forks source link

URL encode SMS text content in #send(from:, to:, text:) #224

Closed Burgestrand closed 2 years ago

Burgestrand commented 2 years ago

https://api.vonagestatus.com/incidents/d7k2dbqnj7fr

Some SMS API customers who are not URL encoding their form bodies may have experienced truncation of their SMS messages if the SMS content contains a comma. Customers are advised to URL encode all fields when submitting requests to avoid incorrect handling of their messages.

We were affected by this incident, and we're using v7.5.0. It doesn't look like the affected code has changed between v7.5.0 and the more recently released v7.6.0.

Our code that was affected by this incident looks roughly like this:

client = Vonage::Client.new(…)
client.sms.send(from: from, to: to, text: "some text, with a comma, was truncated")

If URL encoding the message text is advised, then the Ruby SDK should probably do that?

superchilled commented 2 years ago

Hey @Burgestrand. Thanks for flagging this. I'm away for the next couple of weeks, but will definitely look at this once I'm back.

superchilled commented 2 years ago

Hi @Burgestrand

If URL encoding the message text is advised, then the Ruby SDK should probably do that?

Having looked at the details of the service issue, I think the advice to encode requests was a temporary work-around/ suggestion while a fix was put in place. My reading of it is that the incident is now resolved. Are you still experiencing issues?

Regarding encoding more generally, the Ruby SDK does check whether the text param contains any non-GSM characters and if the type param is not set to unicode logs a warning (see this line). I could potentially add a feature where, rather than logging a warning, the SDK automatically sets type to unicode where type is text but the message contains non-GSM characters. However, this still wouldn't have prevented the problem described in the service issue, because that was caused by messages containing a comma , and this is one of the GSM supported characters (see this table).

It also wouldn't be appropriate to simply set all messages to have a type of unicode, because:

a) the SMS API supports a number of message types other than text and unicode (see the description for the type field in the API spec) b) setting messages to unicode uses up more bytes, and so using unicode when it's not required can incur unneccessary messaging costs

Burgestrand commented 2 years ago

Hi!

Yeah, I also don't think messages should be implicitly converted to unicode, a warning is fine.

It's not clear to me if the incident report suggested to always URL encode the parameters, or if it was intended as a workaround. I read the incident report roughly in the spirit of "shame on you for not URL encoding your message body!", as if the SDK is simply not doing something it's supposed to (i.e. call the API with URL-encoded parameters), and so far it simply has gotten away with it.

If you're saying that URL encoding the message text really isn't necessary, and it indeed was a temporary workaround then I'm all OK with that. You can close the issue as far as I'm concerned! We were only affected by this during the incident, and I merely reported this to avoid future similar incidents with the same underlying reason.

Worth knowing is that we moved to a different SMS platform (for multiple reasons; not related to how this issue has been handled!), so my personal interest in this issue has waned.

Thanks! 😊

superchilled commented 2 years ago

Thanks @Burgestrand. I'll close this issue then. 👍🏾