3

At work we have a lot of code like this (pseudocode):

response form_submit(string username, string password) {
  if (  username == ""
     || username.contains(invalid_chars)
     || password.length < 5 
     || ...
     ) {
    return "errors: blah blah";
  } else {
    try {
      db("insert into users(username, password) values (?, ?)", username, password);
      return "success";
    }
    catch (DBException e) {
      return "errors: blah blah";
    }
  }
}

Some of those are enforced as constraints on the DB, some aren't. It makes more sense to me to enforce everything as DB constraints and just do

response form_submit(string username, string password) {
  try {
    db("insert into users(username, password) values (?, ?)", username, password);
    return "success";
  }
  catch (DBException e) {
    return "errors: blah blah";
  }
}

My opinion is obvious here, but what's yours: is this kind of application-level data integrity check bad practise?

jameshfisher
  • 740
  • 4
  • 9
  • personally I prefer redundancy in these kind of things, (like what happens if you migrate your DB over to another server/vendor and forget some constraints (or they just can't be supported) – ratchet freak Mar 15 '13 at 20:45

2 Answers2

4

Well, all performance and redundancy considerations aside, unless .net database exceptions are more sophisticated than in every other language I've ever worked with, the simple fact of needing to construct an error message readable by non-programmers sort of necessitates the first method, unless you prefer error messages that read something like:

Error #0x4b13: Violated length constraint '>=5' for table 'Users', field 'password'.

What you'll end up doing is recreating the validation code anyway to produce a decent error message, and if you're going to recreate that code anyway, you may as well do it before you incur the expense of a database call.

Karl Bielefeldt
  • 146,727
  • 38
  • 279
  • 479
  • Well, I think you'd just name all your constraints, then when the DB gives you `violated username_valid_char`, you lookup `friendly_errors['username_valid_char']`. – jameshfisher Mar 15 '13 at 21:19
1

In the database, you won't be able to validate the password length at all (since it's hashed and not plaintext, right? RIGHT?).

Testing for invalid characters in the username must be done before you write your INSERT statement, otherwise you're wide open to a SQL injection attack. Which would be much less likely if you used a parameterized query instead, but it's a risk you must always consider.

Redundancy in your checks is valuable, as ratchet freak pointed out.

alroc
  • 473
  • 3
  • 7
  • 3
    How would you perform an SQL injection attack using invalid characters if the developer is correctly using parameterized queries? – Heinzi Mar 15 '13 at 21:10
  • So basically you're saying that some things can't be validated using DB constraints, right? – Robert Harvey Mar 15 '13 at 21:11
  • You're right about the password issue! My silly example was fortuitous for finding that example. I don't buy the injection argument; obviously that should just be properly escaped. – jameshfisher Mar 15 '13 at 21:15
  • 2
    @eegg properly escaping is very hard to do correctly (even library escapers can get it wrong) it's better to parameterize – ratchet freak Mar 15 '13 at 22:24