• Jens (unregistered)

    That is a prototype use case where using the Criteria API is wrong.

  • justsomedudette (unregistered)

    Thanks, now I have saxophones stuck in my head.

  • PB (unregistered)

    Shit happens. Not too impressive WTF today.

  • bug (unregistered)

    Criteria criteria = new Critera();

  • Philyofel (unregistered) in reply to bug
    bug:
    Criteria criteria = new Critera();

    Only unauthenticated users may copy and paste. Properly authenticated users have to write everything into another window by hand.

  • Jack (unregistered)

    Lorne has done some nice work in the sidebar, but after today's long and winding WTF all we get is that someone forgot to establish a default user ID?

    if (requestUser == null)
    {
      requestUser = 'xsmith' // what are the chances of that ever happening?
    }
    
  • Ken (unregistered) in reply to bug
    bug:
    Criteria criteria = new Critera();
    Is that the new three-way boolean expression I've been hearing about?
  • faoileag (unregistered)

    I was about to say that whether this counts as a WTF is debatable at best, then I had a second look.

    First WTF, debatable: the code relies on getUser() returning null when no user data is provided. What does happen if I supply <username></username>? Ok, the API is unknown to me, so I suppose the developer had the documentation for getUser() and knew what he did.

    BUT... the rest of the code is one hell of a WTF unless Hans got it all wrong and returning all data is the normal desired state of things, i.e. the intention of GetSensitveUserData. What, given the name of the webservice, I don't think is the case.

    Detecting something like this in code should ring alarmbells all over the team, the team leader should schedule a comprehensive code review to weed out any other WTFs and the softwaretester should devise plenty of new tests to check the webservice's acces integrity.

    Sadly it seldom works out that way :-(

  • Ralph (unregistered)

    But... but... XML is cool!

    Webservice is moar cool!!!

    Yeah we made a few mistakes in the past but we've learned our lesson and this time Windows 8 fixes everything. Really. Trust us.

  • faoileag (unregistered) in reply to Ken
    Ken:
    bug:
    Criteria criteria = new Critera();
    Is that the new three-way boolean expression I've been hearing about?
    No, then it would be: Criteria criteria = file_not_found;
  • (cs) in reply to Jack
    Jack:
    Lorne has done some nice work in the sidebar, but after today's long and winding WTF all we get is that someone forgot to establish a default user ID?
    if (requestUser == null)
    {
      requestUser = 'xsmith' // what are the chances of that ever happening?
    }
    
    Well, I guess that Xavier Smith gets to have his details shown to all unauthenticated users...
  • faoileag (unregistered) in reply to Jack
    Jack:
    if (requestUser == null) { requestUser = 'xsmith'; }
    requestUser is not an instance of String, it's an instance of User

    The correct line would therefore have to be something like: requestUser = new User('some_dummy_name');

    But who tells you that User's constructor is available in that part of the code? I.e. that you can instantiate User? Or Employee (the two are related)?

    And while using a non-existing employee may be a workaround to get rid of the gaping security hole, it is far from being a good solution. A good solution would address the fact more problem-related, i.e. find a way to deal with anonymous requests. Since the code is supposed to return a query, an exception might be a good thing to throw in this case.

  • (cs)

    TRWTF: How did this code get into the hands of end-users without being tested?

  • Dave the Snave (unregistered) in reply to justsomedudette

    Love it!

  • faoileag (unregistered) in reply to RonBeck62
    RonBeck62:
    TRWTF: How did this code get into the hands of end-users without being tested?
    Probably like most code that still has major defects: in a rush to meet a deadline :-)
  • AV (unregistered) in reply to Jens
    Jens:
    That is a prototype use case where using the Criteria API is wrong.

    You could just as easily say that this is a prototype case where leaving out the else-case of an if is wrong.

  • Stebe (unregistered)

    So, what exactly is the first part of the article about? How dooes productname come into it?

  • PG4 (unregistered) in reply to justsomedudette
    justsomedudette:
    Thanks, now I have saxophones stuck in my head.

    I got that heavy, heavy monster sound in mine

  • faoileag (unregistered) in reply to Jens
    Jens:
    That is a prototype use case where using the Criteria API is wrong.
    After thinking about it for while, I have to disagree.

    Consider the following implementation of GetSensitiveUserDataQuery:

    String select = "<select_stuff_goes_here>"; select += " WHERE <clause_that_never_gives_results>"; if (criteria.exist()) { select += " OR (" + criteria.toSQLtring() + ")"; } return select;

    That would fix the problem at hand. Although I grant that it is not the best possible solution.

  • ¯\(°_o)/¯ I DUNNO LOL (unregistered)
    Often it could be traced back to forgetting the Important Special <!-- as in Education --> Code
    FTFY.

    Also, it looks like it will allow anyone who is not an employee to see everything, too.

  • StupidTheKid (unregistered) in reply to PG4
    PG4:
    justsomedudette:
    Thanks, now I have saxophones stuck in my head.

    I got that heavy, heavy monster sound in mine

    Well, it is the nuttsiest sound around.

  • (cs)

    Unless I'm missing something, if a user ID is required (which you kind of need to assume in a method called GetSensitiveData that requires a logged-in user) then you should probably throw an exception if there isn't any user ID, or else use something like the NULL OBJECT pattern to have a sensible default (e.g. empty string or similar that will return 0 rows).

  • faoileag (unregistered) in reply to ¯\(°_o)/¯ I DUNNO LOL
    ¯\(°_o)/¯ I DUNNO LOL:
    Also, it looks like it will allow anyone who is not an employee to see everything, too.
    So let's hope the author of the request class' getUser() method didn't know about "programming to an interface" - or at least didn't do it :-)
  • TG (unregistered)

    Simplest solution would be to use an IN clause, with a default of an empty list, and if there is a user specified, add them to that list.

    Criteria criteria = new Critera(); List<Employee> toShow = Lists.newArrayList(); if (requestUser != null) { if (requestUser instanceof Employee) { Employee employee = (Employee) requestUser; toShow.add(employee); } } criteria.add(Restrictions.in("partner", toShow)); return GetSensitiveUserDataQuery(criteria);

  • XXXXXX (unregistered)

    Hans's code isn't the problem. The problem is the reporting service.

    Why couldn't any jackass just skip the app and call the report service without any authentication?

    The reporting server is letting anyone without authentication see tons of private data.

    No props to Hans, but he's not the biggest wart on this buttcheek.

  • Destman (unregistered) in reply to TG

    FAIL! If toShow is empty, it will still return info on ALL employees. You'd have to check if toShow is empty first and not run the query if it is.

  • Paul Neumann (unregistered)

    At my code we could simply use the "nonuser" user account and post all transactions to the "unaccounted" account.

  • (cs) in reply to justsomedudette
    justsomedudette:
    Thanks, now I have saxophones stuck in my head.
    Yeah, well, welcome to the house of fun.
  • faoileag (unregistered) in reply to Destman
    Destman:
    If toShow is empty, it will still return info on ALL employees.
    Not on mysql, it doesn't. "SELECT ...WHERE partner IN ();" throws an error on attempted execution.
  • Jack (unregistered) in reply to faoileag
    faoileag:
    Jack:
    if (requestUser == null) { requestUser = 'xsmith'; }
    requestUser is not an instance of String, it's an instance of User

    The correct line would therefore have to be something like: requestUser = new User('some_dummy_name');

    So you're telling me somebody put a lot of effort into inventing a language and a programming paradigm where you can read the value of something, but you can't write the value of that same something using mirror image syntax?

    I thought OOP was supposed to conceal the details of the implementation, and only expose an interface where you get and set values and the class takes care of figuring out how to make that happen.

  • Andrew (unregistered)

    TRWTF is not using the proper singularization, "criterion", amirite?

  • Destman (unregistered) in reply to faoileag
    faoileag:
    Destman:
    If toShow is empty, it will still return info on ALL employees.
    Not on mysql, it doesn't. "SELECT ...WHERE partner IN ();" throws an error on attempted execution.

    Which is why the SQL code generated would omit the filter altogether, to avoid an error... and then would return the whole list :)

  • faoileag (unregistered) in reply to Jack
    Jack:
    faoileag:
    Jack:
    if (requestUser == null) { requestUser = 'xsmith'; }
    requestUser is not an instance of String, it's an instance of User The correct line would therefore have to be something like: requestUser = new User('some_dummy_name');
    So you're telling me somebody put a lot of effort into inventing a language and a programming paradigm where you can read the value of something, but you can't write the value of that same something using mirror image syntax?
    No, I'm telling you that requestUser is of type User and not a String.

    The getter for that bit of data inside User that is matched against the "partner" field in the database is getPartner() (and returns, indeed, a String).

    As for the "mirror image syntax": the setter is not known to us and may as well not exist at all if User is not meant to be modified after instantiation.

  • faoileag (unregistered) in reply to Destman
    Destman:
    faoileag:
    Destman:
    If toShow is empty, it will still return info on ALL employees.
    Not on mysql, it doesn't. "SELECT ...WHERE partner IN ();" throws an error on attempted execution.
    Which is why the SQL code generated would omit the filter altogether, to avoid an error... and then would return the whole list :)
    That is some weird behaviour from Hibernate indeed... I would have expected it to throw an exception in that case, if it doesn't want to create an empty IN clause.
  • (cs)

    You don't do source code comments as well as Remy!

  • (cs) in reply to Andrew
    Andrew:
    TRWTF is not using the proper singularization, "criterion", amirite?

    New Criterii()

  • (cs) in reply to DaveK
    DaveK:
    You don't do source code comments as well as Remy!
    I liked the HTML comment. Though, credit-due, that saxophone-laden comment was courtesy of Mark.

    So I guess we're both right. Stickers for everyone!

  • urza9814 (unregistered) in reply to Jack
    Jack:
    faoileag:
    Jack:
    if (requestUser == null) { requestUser = 'xsmith'; }
    requestUser is not an instance of String, it's an instance of User

    The correct line would therefore have to be something like: requestUser = new User('some_dummy_name');

    So you're telling me somebody put a lot of effort into inventing a language and a programming paradigm where you can read the value of something, but you can't write the value of that same something using mirror image syntax?

    I thought OOP was supposed to conceal the details of the implementation, and only expose an interface where you get and set values and the class takes care of figuring out how to make that happen.

    No, your original example ISN'T mirror image syntax, that's the problem. Null isn't a string, it's an object.

  • C-Derb (unregistered) in reply to Stebe

    +1

  • C-Derb (unregistered) in reply to Stebe
    Stebe:
    So, what exactly is the first part of the article about? How dooes productname come into it?
    +1
  • Anonymous (unregistered)

    Herein lies the problem with ORM's. The SQL is actually easier to write, maintain, and understand. And since NULL cannot equal anything, it would probably just take care of this. Should have been a stored procedure. ;)

    Captcha: decet. As in ORMs are deceitful.

  • (cs)

    This is the kind of code that you just take out with an axe...

  • 71 (unregistered)

    This is an easy mistake to make, and equally easy to fix. It should have been caught in unit testing, and... it was, wasn't it? Or are we supposed to assume the bug made it to production...?

  • AnonyMark (unregistered)

    It seems it was indeed in production, just like all the other failure modes discussed in the beginning of the article.

  • Rick (unregistered)

    Testing?

    Oh, you probably mean "beta", where we just "put it out there" to gain synergistic mindshare among power users and early adopters who will uplift target demographic awareness. Hell it might even go viral!

    If there do happen to be any unspecified features, people can always report them to our outsourced customer service facility, featuring a rightsized workforce and streamlined business processes to maximize return on investment in the business context of what is, after all, a cost center.

    Like testing.

  • Jens (unregistered) in reply to faoileag

    Of course creating SQL with Strings is worse, no doubt. But Criteria API is only a tool when you need dynamic queries. In this case there are two possible queries. Selecting one of the two is not a use case of creating dynamic queries. Here you have to use named queries which are prepared statements, compiled, checked at edit, compile and startup time. Some people switch from string concatenating directly to Criteria API - which is an improvement, but still not the right way to do it. In fact there are very few use cases where Criteria is a good solution.

  • Valued Service (unregistered) in reply to faoileag
    faoileag:
    unless Hans got it all wrong and returning all data is the normal desired state of things, i.e. the intention of GetSensitveUserData. What, given the name of the webservice, I don't think is the case.

    Reading the method, it seems to make sense. Get all user data unless a username is specified. That's the use of the method.

    Authentication should have taken place before, and access rights checked.

    Let's say you have access to everyone's data, how do you perform a get on all user data? Get the list of users and perform this call on each?

    The WTF is that there's NO authentication layer. Not that this method allows access to all data.

  • qbolec (unregistered)

    Users::get_user($uid){ //performance ! if($uid == Session::get_uid()){ return Session::get_user(); }else{ return new User($uid); } }

    now... if session is anonymous, and $uid === "hack", and your language has this strange == operator...

  • (cs)

    (Duhhhh!!!) Gee, I don't understand the problem. It's just a normal blacklist. Just blacklist all the users you don't want to get access...

  • Paul Neumann (unregistered) in reply to Valued Service
    Valued Service:
    ...The WTF is that there's NO authentication layer. Not that this method allows access to all data.
    AAA -> Authentication, Authorization, Auditing.

    Authentication ≠ Authorization. They have a current user, so clearly they are performing authentication. It is authorization they are missing.

Leave a comment on “None for All”

Log In or post as a guest

Replying to comment #:

« Return to Article