CodeSOD: Voicemail to Email to Nothing at All

Mike's co-worker doesn't like to write code that gets tightly coupled to one system. Everything should be completely abstracted so that, if something on the backend changes, your code doesn't need to change.

Which is an admirable goal, perhaps, but is also premature abstraction. For example, Mike's company uses Asterisk, an open-source PBX system. This co-worker wanted to script a few actions in that system, but didn't want his scripts to be tightly coupled to the system that he was scripting, so he... overgeneralized. But also didn't. But also did it wrong.

Specifically, new voicemails needed to also get sent to an email address. Asterisk stores voicemails in the form msgNNNN.wav, where NNNN is a 0-padded numeric value, e.g. msg0123.wav.

This is the PHP code to find the proper file to send:

class SendVoicemailEmail {//...private function getMessageNumberForAttachmentFilename(){return $this->getMessageNumber() - 1;}private function getAttachmentFileName(){foreach (array('INBOX', 'Urgent') as $vmPriorityDir) {$voicemailAudioDir = $this->getVoicemailDirectory($vmPriorityDir);$files = scandir($voicemailAudioDir);foreach ($files as $file) {if ($this->isCorrectFileName($file)) {return $this->getVoicemailDirectory($vmPriorityDir) . $file;}}}return false;}private function isCorrectFileName($fileName){$fileName = strtolower($fileName);$suffix = $this->getMessageNumberForAttachmentFilename() . $this->getAttachmentSuffix();$suffixLocation = strpos($fileName, $suffix);if ($suffixLocation !== false) {$fileName = str_replace($suffix, '', $fileName);$fileName = str_replace($this->getAttachmentPrefix(), '', $fileName);if (is_numeric($fileName) && ($fileName == 0)) return true;}return false;}//...}

The first function, getMessageNumberForAttachmentFilename just addresses the fact that the filenames are zero-indexed, but the metadata is one-indexed, so that's pretty reasonable.

The second function, getAttachmentFileName is where things start to drift off the rails. You see, if you just used the Asterisk naming convention in this code, you could just grab the file you needed. But that would mean encoding the specific vendor's naming convention into this method, which the developer didn't want to do. So instead, the code scans every file in two directories to see if one of them is the "correct" filename.

But it's the third method, isCorrectFileName where things really lose control. First, we calculate the $suffix, which is the message number combined with the file extension, e.g. 123.wav. We attempt to find that substring in the filename, and if it exists, we strip it out, along with msg. Finally, if it's a numeric value and equal to zero, we've found the correct filename.

There are so many things going wrong here. First, this hasn't really abstracted out the Asterisk filename convention anyway, so the entire goal of making this super abstract was missed. But the Asterisk file naming convention allows for up to 10,000 files (0000-9999), while this developer's approach only supports 1,000.

Say the filename is msg1234.wav. We calculate the suffix, 1234.wav, and remove that from the string, leaving msg. We then script the prefix, leaving an empty string. According to is_numeric, an empty string is not numeric.

This bug was discovered, of course, when the system started believing new voicemails stopped existing. But not right away, there were a few days of confused and angry users who perhaps were puzzled about why they weren't getting new voicemails, but weren't sure what to make of it.

Mike decided the easiest fix was to take the risk of tightly coupling to Asterisk's filename convention and just load the file directly, without scanning any directories.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.