None for All

« Return to Article
  • Jens 2013-02-04 06:08
    That is a prototype use case where using the Criteria API is wrong.
  • justsomedudette 2013-02-04 06:12
    Thanks, now I have saxophones stuck in my head.
  • PB 2013-02-04 06:48
    Shit happens.
    Not too impressive WTF today.
  • bug 2013-02-04 06:49
    Criteria criteria = new Critera();
  • Philyofel 2013-02-04 07:03
    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 2013-02-04 07:33
    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 2013-02-04 07:35
    bug:
    Criteria criteria = new Critera();
    Is that the new three-way boolean expression I've been hearing about?
  • faoileag 2013-02-04 07:37
    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 2013-02-04 07:41
    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 2013-02-04 07:42
    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;
  • Steve The Cynic 2013-02-04 07:54
    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 2013-02-04 08:01
    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.
  • RonBeck62 2013-02-04 08:06
    TRWTF: How did this code get into the hands of end-users without being tested?
  • Dave the Snave 2013-02-04 08:14
    Love it!
  • faoileag 2013-02-04 08:18
    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 2013-02-04 08:26
    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 2013-02-04 08:41
    So, what exactly is the first part of the article about? How dooes productname come into it?
  • PG4 2013-02-04 08:45
    justsomedudette:
    Thanks, now I have saxophones stuck in my head.


    I got that heavy, heavy monster sound in mine
  • faoileag 2013-02-04 08:46
    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 2013-02-04 09:15
    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 2013-02-04 09:15
    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.
  • ObiWayneKenobi 2013-02-04 09:20
    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 2013-02-04 09:26
    ¯\(°_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 2013-02-04 09:29
    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 2013-02-04 09:53
    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 2013-02-04 09:54
    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 2013-02-04 09:55
    At my code we could simply use the "nonuser" user account and post all transactions to the "unaccounted" account.
  • dgvid 2013-02-04 09:58
    justsomedudette:
    Thanks, now I have saxophones stuck in my head.

    Yeah, well, welcome to the house of fun.
  • faoileag 2013-02-04 10:02
    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 2013-02-04 10:11
    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 2013-02-04 10:11
    TRWTF is not using the proper singularization, "criterion", amirite?
  • Destman 2013-02-04 10:14
    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 2013-02-04 10:25
    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 2013-02-04 10:34
    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.
  • DaveK 2013-02-04 10:39
    You don't do source code comments as well as Remy!
  • Lorne Kates 2013-02-04 10:53
    Andrew:
    TRWTF is not using the proper singularization, "criterion", amirite?


    New Criterii()
  • Lorne Kates 2013-02-04 10:54
    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 2013-02-04 11:10
    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 2013-02-04 11:11
    +1
  • C-Derb 2013-02-04 11:11
    Stebe:
    So, what exactly is the first part of the article about? How dooes productname come into it?
    +1
  • Anonymous 2013-02-04 11:32
    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.
  • chubertdev 2013-02-04 12:16
    This is the kind of code that you just take out with an axe...
  • 71 2013-02-04 12:50
    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 2013-02-04 12:56
    It seems it was indeed in production, just like all the other failure modes discussed in the beginning of the article.
  • Rick 2013-02-04 13:06
    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 2013-02-04 14:11
    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 2013-02-04 14:11
    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 2013-02-04 14:33
    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...
  • Coyne 2013-02-04 14:35
    (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 2013-02-04 14:46
    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.
  • Jazz 2013-02-04 14:52
    Anonymous:
    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.


    The ORM has nothing to do with this. The programming error would have been the exact same WTF even if it had been written in raw SQL:


    String criteria_sql = "SELECT * FROM `employee_data` ";
    if( requestUser != null ){
    if( requestUser instanceof Employee ){
    Employee employee = (Employee) requestUser;
    criteria_sql = criteria_sql + "WHERE `id` = " + employee.id;
    }
    }
    return executeQuerySomehow( criteria_sql );
  • PiisAWheeL 2013-02-04 14:59
    Great. The only thing missing is some obscure piece of regex and the trifecta is complete.
  • dkf 2013-02-04 15:05
    Rick:
    Hell it might even go viral!
    What, like H5N1 'flu, HIV or Ebola? Thanks a bunch!
  • Pablo 2013-02-04 15:56
    This is a great example of using failsafes - if your authentication ever fails, you can still safely access all of your data!
  • pjt33 2013-02-04 17:54
    Jack:
    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.

    That's procedural programming with classes, often mistaken for OOP.
  • Meep 2013-02-04 20:37
    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');


    No, it would be requestUser = User.makeInvalidUser(), and it would return a subclass of User that overrode the method isAuthenticated to return false. Or it could even be a special guest user that exists but simply has no permissions.
  • Gunslinger 2013-02-05 01:30
    dkf:
    Rick:
    Hell it might even go viral!
    What, like H5N1 'flu, HIV or Ebola? Thanks a bunch!


    Thanks Obama!
  • löchlein deluxe 2013-02-05 02:26
    Ken:
    bug:

    Criteria criteria = new Critera();

    Is that the new three-way boolean expression I've been hearing about?

    No, it's the way you have to name things in **ails if you want auto-generated code to work. Oh how I wish I was kidding.
  • Norman Diamond 2013-02-05 02:48
    Lorne Kates:
    (who can trust ctrl-c / ctrl-v in these hectic, Enterprise Cloud-based times?)
    Embarrassingly enough for me, after having advocated ctrl-c / ctrl-v in this forum a few weeks ago, I have to admit that it cannot be trusted.

    I took the hard drive out of a PC whose backlight burnt out but still had Windows XP working when an external monitor was attached.

    I used a USB-to-SATA cable to connect the hard drive to a PC running Windows 7.

    I opened Windows Explorer, selected a bunch of folders, ctrl-c, explored to a folder on the internal drive of the new PC, ctrl-v.

    Windows Explorer told me about 30 times that it couldn't copy files because the filenames were too long. Sometimes it showed part of the base filename but it didn't show the entire path. Windows suggested that I change the filename, but didn't provide any button or edit box to do so, just skip or cancel or maybe repeat the fail without change. Sometimes I could guess where the original files were. I didn't handwrite a complete list. Well, that was yesterday. Today is worse.

    Today I happened to look at a folder where Windows Explorer did copy files to. A folder where Windows Explorer did not complain about invalid filenames or inability to copy files. A folder where Windows Explorer pretended to succeed.

    Today I noticed that some filenames differed from the originals.

    ctrl-c / ctrl-v? In Windows 7? Just say no.
  • my name 2013-02-05 03:21
    Happened into a similar feature at the university some 15 or so years ago.

    There were old macs using some text terminal software to connect to a system named LADOK, a system which stored all information on completed and ongoing courses. These terminals were available to any student (well really anyone at all) in case you wanted to check your grades for that Haskell introduction course.

    To retrieve your information, you entered your 10-digit personal number, similar to social security number I guess, only that the 6 first digits are your birth date. By substituting the last four digits with spaces you could enter any date and retrieve a list of students born on that date, their full personal numbers and their grades. Can't recall if it "only" reported students at the local university or if it was for the whole of Sweden.
  • Gyxi 2013-02-05 03:35
    It's a classic scenario. They had correctly functioning code without the check on whether the user is null. Then someone entered a non existing user name and the system gave a NullPointerException. A junior developer was told to fix the error and did by checking for null first.

    - sagaciter, n. a person that likes to tell quotations from old sagas.
  • Mike 2013-02-05 11:23
    Buffalo buffalo buffalo?
  • Mike 2013-02-05 11:31
    Probably just tested with a few users and yep it only showed their data so ... Then our WTF guy decided to use the webservice directly rather than go through the page (which might enforce login at the website level) and didn't use it "correctly". A lot of edge cases never get tested at a lot of places because "it will never happen".
  • Neil 2013-02-05 11:44
    Jazz:
    The ORM has nothing to do with this. The programming error would have been the exact same WTF even if it had been written in raw SQL:


    String criteria_sql = "SELECT * FROM `employee_data` ";
    if( requestUser != null ){
    if( requestUser instanceof Employee ){
    Employee employee = (Employee) requestUser;
    criteria_sql = criteria_sql + "WHERE `id` = " + employee.id;
    }
    }
    return executeQuerySomehow( criteria_sql );
    That's not raw SQL, you're still casting the user to an employee on the client side. You want
    SELECT * FROM employee_data INNER JOIN users ON partner WHERE users.id = ?
    If the user isn't an employee then his partner is null and you get no records.
  • Neil 2013-02-05 11:48
    Norman Diamond:
    I opened Windows Explorer, selected a bunch of folders, ctrl-c, explored to a folder on the internal drive of the new PC, ctrl-v.
    So I take it you'll be using ROBOCOPY from now on?
  • danixdefcon5 2013-02-05 18:52
    Easy to fix:

    if (requestUser == null)
    throw new SecurityException("Nice try, smartass!");

    Bonus points as SecurityExceptions are logged by the App Server.
  • OOP?!? 2013-02-06 15:55
    pjt33:
    Jack:
    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.

    That's procedural programming with classes, often mistaken for OOP.


    Unfortunately, sometimes that's all you want in these OOP languages, and so you end up creating some terrible classes.
  • Leonidas 2013-02-06 22:11
    <!-- Madness? This is Sparta!!! -->

    Something something akismet. Suscipere. Frist? First? Fstir?
  • EsotericNonsense 2013-02-07 15:22
    Well I don't see what the big problem is it's a one line fix.