Shame on Me: the missing code review (or is it unit testing?)

So a bug was reported not long ago. Let’s say “sensitive data” was available where it shouldn’t have been. I had an API view, a view that was returning json only, or should have been. Apparently early on in the project, I had added a comment to that view, to make sure it was returning the appropriate data. So I had added an html comment to a view that was supposed to be returning json. And I had it outputting everything under the sun it could output.

"secure data" => "stuff I don't want the user to see",
... 30-40 lines of this ...
{"success": true}

So there’s 2 problems with that. The most important of which is that with DOM inspection tools, like chrome developer tools or firebug, you can inspect the return value of that request and see the data I don’t really want you to see. The other problem is that I’ve created invalid json, and whatever is checking that success is clearly not using that value.

Why did I do this?

This was early on in development and I was debugging the easiest way I could.

What could have prevented this?

A code review of any kind. One look at that file would have been a red flag to anyone. But a view file that was only outputting true or false never seemed like something I ever needed to look at once I got it working.


  1. harperS says:

    I use Python’s validictory module to ensure I get JSON back and it’s JSON that I’m expecting. It’s beyond unittests, more functionality testing, but it’s caught some good things.

  2. Like harperS, I too use validation tools. I’ve found that when I’m facing JSON, XML, SOAP, or RSS – I’ve made sure to find the best validators out there. There have been so many times where I’ve assumed the feeds were returning the correct format, char sets (UTF8/16…) where I ended up spending hours troubleshooting my code vs a simple feed validation. thankfully it hasn’t happened in a while now.

    I’ve mainly done work with Google Merchant where you submit your product feeds in a commerce environment. Their validators are pretty mature now.


Leave a comment