CodeSOD: A Policy Exception

"So, we have a problem. A" a big one."

Andrea C worked for an insurance company. In terms of software development, a "problem" meant she'd misfiled one of her TPS coversheets or something like that. A big problem meant she'd accidentally checked in code that contained a comment with some profanity in it.

"It's actually, well, it's actually huge," her boss continued.

She'd never had a "huge" problem before.

Someone had discovered that, if they knew a customer's policy number and were currently logged in- they could see that customer's policy. It didn't have to be their policy. Any policy, so long as they had a valid login and the policy number.

That was pretty huge. Andrea hadn't gotten too deep into that section of the application before, but the original developer had moved on, so it was time for her to dig in. She pulled up the JSP code which served the page.

<%boolean globalError = false;// SNIP: gets various parameters from HttpPortletRequest. E.g. policyNumber, fiscalCode, etc. //fiscalCode is the customer identifiertry {// Security check to see if the policy belongs to the userif(!PolicyUtil.isValidPolicyNumber(fiscalCode, policyNumber, endpoint))throw new PolicyNumberException();} catch(Exception e) {globalError = true;}Policy policy = PolicyUtil.getPolicy(fiscalCode, policyNumber, dateOnlyFormat, endpoint);%><%-- <c:if test="<%=!globalError %>"> --%><div class="header container main_dsh"><h2><%=policy.getProductName() %></h2><h4 style="color: white;">N. <%=policyNumber %></h4></div><%-- </c:if> --%>// SNIP: displays policy details

Let's start with names. isValidPolicyNumber isn't just a validator- it uses the fiscalCode to determine if this customer is allowed to see that policy number. It also checks that the policy number exists, that it's a valid and active policy. And all that information is encoded into a single boolean value.

And of course, of course if that boolean value is false, we throw an exception. Flow of control by exceptions is always a great choice, especially when the only thing you do with the exception is set a flag.

The sane thing, at this point, is to bail. We can't display this policy. But the code doesn't bail, it actually goes right back to the database to fetch the policy. A policy that it may have already decided is not valid in this context.

When Andrea found this much, she thought she'd figured out the actual bug, but reading on revealed the next step. The <c:if> test only surrounds the display of the header.

No one had touched this code in years. Pretty much since their new website launched in 2009, this code had been sitting in production.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!