www.talios.com

Bad Testing Habits - or Regex Injection Woes

I stumbled across an interesting "broken test" earlier, it's not that the test fails, or is "broken", but more that it's not actually testing anything worthwhile. Occasionally I'll start off writing a test to check some functionality that I'm not 100% sure off - it might be a simple matter of checking formatting results, or an API call I'm not 100% sure off.

The broken test I found related to a regex injection problem I had awhile ago - I'd updated how some of our message filtering worked to give users more flexibility, under the covers the code in question took an entered string and converted it to a regex - wrapping the contents with with a pair of .* searches.

My initial set of developer testing passed and things were seemingly good in the world - untill a QA test engineer came along and did something unexpected - they actually used it :p

...whilst I'd been testing on basic search strings like "SPAM" our intrepid young tester decided to filter out JIRA support tickets using "[JIRA]" and blammo - I'm bitten by a regex injection attack where the search would now pick up anything with the letters J I R and A in them - anywhere.

The solution was a simple matter of escaping the search string before checking matches. So I started building up a few simple tests to confirm what escapes I wanted and that the entered strings were escaped, and matched (or not matched) accordingly:

@Test
public void testRegexEscaping() {
  String match = "[JIRA]";
  String escapedMatch = match
    .replaceAll("\\[", "\\\\[")
    .replaceAll("\\]", "\\\\]")
    .replaceAll("\\(", "\\\\(")
    .replaceAll("\\)", "\\\\)")
    .replaceAll("\\.", "\\\\.")
    .replaceAll("\\\\", "\\\\")
    .replaceAll("\\*", ".*");

  assert"\\[JIRA\\]".equals(escapedMatch) : escapedMatch;
}

Theres nothing functionally wrong with the test, its simple and just checks that what our "escape" does returns what we expect. The problem was that when the block of replaceAll() calls were moved into the actual code the test wasn't updated to use the version in the real code base. Meaning that if it changed at all, the test in question was still only testing the original inline code, not the actual in use code.

So whilst it's good having tests that test/confirm even the smallest piece of functionality, if it's not testing code that hits a production environment anywhere then the tests are a somewhat wasted effort.

It pays to revisit and audit your tests once in awhile just to make sure you're testing what you think your testing...

Comments (2)