The WTF security module



  • Note: The following has been highly sanitized to make it a believable story.


          A few weeks ago, I was asked to take a look at an in-house project that had encountered a few serious hiccups during the integration process.
          The first task was to make the UI "experience" functional - the menus were not working, the buttons did nothing and none of the other elements were appearing on the screen. Mind you, this "UI experience" was determined by permissions set for the user in the database.
          So my friend proceeds to have a look at the security module to see what's wrong.
          After a dozen or so cups of latte and several tracklists of songs later, the problem was tracked to one statement:
        

    updateUserLoginStatus(userName,password,userRights,status,lastLogin)

    and here is the sanitized definition of updateUserLoginStatus



    public void updateUserLoginStatus(String userName,String password,String userRights, String status, String lastLogin)
    {
    <snip>
    updateUser = conn.prepareStatement(updateString);

    String updateString = "UPDATE user_table SET userID = ? , pass = ? , userRights = ?, status = ?, lastLogin = ?";

    updateUser.setString(1,userName);
    updateUser.setString(2,password);
    updateUser.setString(3,userRights);
    updateUser.setString(4,status);
    updateUser.setString(5,lastLogin);
    </snip>
    }



    Well, that looks syntatically correct. But there started the problem.
    You see, some folks think their code is perfect just because it compiled.
    And then there are those who love log files more than a debugger.
    So when it was pointed out to then that the variable userRights turned out to have the value of "00000000" (which was the decoded value of the encoded value retrieved from the db via another WTF function) at all times, they protested it was not their fault - it was coded by one of the guys who had left months ago.
    Yeaaa, you didnt notice that the buttons and menus didnt work because of something coded months ago. Brillant.

    Oh, it never occured to them that updateUserLoginStatus() was only supposed to update only the loginStatus and lastlogin of the user and nothing else.

    Enjoy!


    PS: The reason behind userRights being all unset was another WTF though. That's for the next submission.



  • I'm not sure if I should click "Reply" or "Report Abuse" but I'm sure that my heart is aching just of thinking about resetting a complete user including username and password and passing the lastLogin TIME as a STRING.

    When I find data types missuses like this one I usually add a record to the database with "Potatoes" on the column and then show the so called programmer the problems that can arise when you don't use the correct types.

    When I find functions that reset a user just to update a login status I ... wait, I never found one of this (thanks Go.. Bud.. Mah.. wait, I'm an atheist)

     

    (The double use of ..wait is intentional, thanks in advance)

    Edit 1: I will not even try to talk about the content of the function

    Edit 2: The forum software let me edit 2 times in a row YAHOOOOOOOOO!!!111oneoneone



  • @Taliesin said:

    ... I'm sure that my heart is aching just of thinking about resetting a complete user including username and password and passing the lastLogin TIME as a STRING.



    I had 5 hours left in the day until I could sign off. An entire floor of around 20 devs could hear some howls and giggles from one particular guy. It was a week until I could get back to my senses. So much for some WTFery that put an entire team in crutches.



  • Your transcript is a big WTF, not the actual code.

    The code isn't syntactically correct (nor even semantically correct in that it does what it's supposed to do, at least you claim it did, except for the userRights stuff), not in the least. Please take some care to really present a programmer WTF next time, and not a transripter WTF. ;-)



  • Am I lost or was updateString used before it was declared?  If I'm not lost, how did that compile?



  • @modelnine said:

    The code ... does what it's supposed to do, at least you claim it did ...

    Think about the SQL for a minute:



    UPDATE user_table SET userID = ? , pass = ? , userRights = ?, status = ?, lastLogin = ?



    Sure as heck don't look like the right thing to me, boyyo.</redneck-or-whatever-the-heck-that-accent-is> Maybe



    UPDATE user_table SET pass = ? , userRights = ?, status = ?, lastLogin = ? WHERE userID = ?



    ...might fit the bill a little better.



  • Think about the SQL for a minute: ...

    You needn't tell me that, that's why I noted that the transcript can't even remotely be correct. A bug like that wouldn't have gone unnoticed, not even for one minute (because of removing all users from the DB, basically, except when there's a UNIQUE constraint for some column like userID, which I'd guess, in which case the query either does nothing or deletes all records but one, or something completely different like blow up the universe, depending on your DB-system).

    Storing permissions in a (sort of) bit-array is (generally, not always) a WTF in itself, but the code the original poster posted doesn't actually show the WTF of that system. So, in my eyes this basically is a transcripters WTF, not a coders. ;-)


  • @Irrelevant said:

    @modelnine said:
    The code ... does what it's supposed to do, at least you claim it did ...

    Think about the SQL for a minute:



    UPDATE user_table SET userID = ? , pass = ? , userRights = ?, status = ?, lastLogin = ?



    Sure as heck don't look like the right thing to me, boyyo.</redneck-or-whatever-the-heck-that-accent-is> Maybe



    UPDATE user_table SET pass = ? , userRights = ?, status = ?, lastLogin = ? WHERE userID = ?



    ...might fit the bill a little better.


    Yeah, I noticed the lack of the where clause as well in the first second, forgetting the rest of the wtfs of the statement. I'm surprised it took until the fifth post until somebody brought it up. The other issues are forgivable, but setting all the user records to the same value is definitely due for a team kicking.



  • @modelnine said:


    Think about the SQL for a minute: ...

    You needn't tell me that, that's why I noted that the transcript can't even remotely be correct. A bug like that wouldn't have gone unnoticed, not even for one minute (because of removing all users from the DB, basically, except when there's a UNIQUE constraint for some column like userID, which I'd guess, in which case the query either does nothing or deletes all records but one, or something completely different like blow up the universe, depending on your DB-system).

    Storing permissions in a (sort of) bit-array is (generally, not always) a WTF in itself, but the code the original poster posted doesn't actually show the WTF of that system. So, in my eyes this basically is a transcripters WTF, not a coders. ;-)


    Sorry about the late reply. But the query DID miss the WHERE clause.
    Q: Why didn't anyone on the team figure this out ?
    A: There was only one user profile in the database against which the testing was done (another WTF).

    PS: The updateString redefinition typo is regretted (typed this code from memory).



  • Sorry about the late reply. But the query DID miss the WHERE clause.
    Q: Why didn't anyone on the team figure this out ?
    A: There was only one user profile in the database against which the testing was done (another WTF).

    Okay. This is really a WTF. But maybe you should've made it clearer. ;-) And, yeah, testing against a single user is bound to induce bugs like these. I've had my share of those when I was young.

Log in to reply