CodeSOD: A Valid Call

"Never trust your inputs" is a generally good piece of advice for software development. We can, however, get carried away.

Janice inherited a system which, among many other things, stores phone numbers. Like most such systems, the database validates phone numbers, and guarantees that numbers are stored in a canonical format, as text.

For some reason, their Rails presentation layer isn't satisfied with this, and needs to validate the data coming from the database. In their specific environment, they know the database only contains canonical phone numbers, but honestly, I'm willing to forgive the belts-and-braces approach, as I've certainly used enough databases that couldn't provide those kinds of guarantees.

No, the problem is the way they went about it.

phone = acct.phone_number.to_sraise "phone number blank!" if phone.blank?phone.gsub!(/\+/,'')if (phone =~ /\A\+[0-9]+\Z/)phone = 'phn:'+phoneelsif (phone =~ /\A[0-9]+\Z/)phone = 'phn:+'+phoneelseraise "phone number: incorrect format!"end

So, first, we convert the text field in the database to a string, which of course it already was. Then we use gsub to strip any "+" characters from the string. Then, we have our if statement, where we check two regexes.

First, we check if the string is a + followed by 1 or more digits. This branch will never be true, as we just removed all the plus signs. Then we check if it's a series of digits. If it is just a series of digits, note that we put the + back on, right after removing it. This does guarantee that there are no middle +, which isn't a thing the database would allow anyway.

But this doesn't guarantee a valid phone number. It just guarantees that we only display phone numbers which contain at least one digit and any number of + signs. The number of digits isn't accounted for (which will have international variations but we can sorta set a range of how many characters there should be).

The fact that this phone number validation is wrong ends up not mattering, though, as the data in the database is valid.

I don't even know what the phn: is intended to be- it looks like it might be intended as a URL protocol, but the phone call protocol is tel:. It might be a formatting convention, but I have some doubts that string concatenation is the right way to do it.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!