Ensuring Indata Validation

Dear Junior

Creating a username class and a validation method has taken us a fair amount towards solving SQL Injection by focusing on a domain model API that is both easy to use correctly and hard to use incorrectly. I would say that we have this far achieved to make the API easy to use.

  Integer authenticate(Username username, String passwordMD5)  
 public class Username {
  // final making it immutable
  public final String username; 
  public Username(String username) { this.username = username; }
  public boolean isValid() { return username.matches("[a-z]+"); }
}
What remains is to ensure that indata validation actually is done. I can see two choices: either putting validation inside the authentication service, or to enforce validation before the call to the authentication service.

Let us first look at putting validation inside authentication.

  /** Authenticates a user with a given password.
   * @throws IllegalArgumentException if username invalid
   */
  Integer authenticateWithUsernameValidation(Username username, String passwordMD5)
          throws IllegalArgumentException, SQLException {
    if(!username.isValid())
      throw new IllegalArgumentException(
              "Cannot authenticate with invalid username: " + username);
    ...
  }

This definitely hardens the interface – now there is no possibility to not validate upon authentication. However, the same trick has to be used in every service method around, including the “create new account”, the “change account username” and all those that are to come in the future. Risks are high that the small isValid-call will be missed somewhere – and one hole is all an attacker needs.

Another drawback is the rather awkward “throws IllegalArgumentException” which feels like a very late validation – should not such validation be made much earlier, preferably up in the presentation and client tiers?

An alternative is to not allow invalid usernames to be constructed at all:

  @Test(expected = IllegalArgumentException.class)
  public void shouldNotCreateUsernameFromInjectionAttackString() {
    new Username("' OR 1=1 --");
  }

This request the constructor to do the validation on the inside, responding with an exception if given an invalid username candidate.

  public Username(String username)
    throws IllegalArgumentException {
    this.username = username;
    if(!isValid())
      throw new IllegalArgumentException();
  }

Now we also need some way to validate from the outside without taking the pain of provoking and handling an exception, so finally there will be a static method after all:

  public static boolean isValid(String username) {
    return username.matches("[a-z]+");
  }

Of course the old methods and constructor should be refactored to uphold the don’t-repeat-yourself (DRY) principle. Interesting enough this will lead the isValid() method to consistently return true – so I guess we can delete it from the class and inline it wherever it was used. That is, unless we for some bizarre reason want to have a method that explicitly tells the rest of the world that “this object is always valid”.

I definitely prefer this latter “strictly-validated-value-object” style before the "validating-service-methods" style. It creates an API that besides being easy to use correctly, also is hard to use incorrectly. It “guides” the client side programmer without being intrusive or obstructive about it.

In some sense, it "enforce" a behaviour upon the client side programmer. However, that does not trouble me. If someone just is nice to me, and don’t cause me trouble, I see no obstacle in letting her have her way.

Yours

Dan