The Daily WTF: Curious Perversions in Information Technology

2013-02-04 Reply Admin

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

2013-02-04 Reply Admin

Thanks, now I have saxophones stuck in my head.

2013-02-04 Reply Admin

Shit happens. Not too impressive WTF today.

2013-02-04 Reply Admin

Criteria criteria = new Critera();

2013-02-04 Reply Admin

bug:
Criteria criteria = new Critera();

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

2013-02-04 Reply Admin

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?
}

2013-02-04 Reply Admin

bug:
Criteria criteria = new Critera();

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

2013-02-04 Reply Admin

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 :-(

2013-02-04 Reply Admin

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.

2013-02-04 Reply Admin

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 Reply Admin

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...

2013-02-04 Reply Admin

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 Reply Admin

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

2013-02-04 Reply Admin

Love it!

2013-02-04 Reply Admin

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 :-)

2013-02-04 Reply Admin

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.

2013-02-04 Reply Admin

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

2013-02-04 Reply Admin

justsomedudette:
Thanks, now I have saxophones stuck in my head.

I got that heavy, heavy monster sound in mine

2013-02-04 Reply Admin

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.

2013-02-04 Reply Admin

Often it could be traced back to forgetting the Important Special  Code

FTFY.

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

2013-02-04 Reply Admin

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 Reply Admin

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).

2013-02-04 Reply Admin

¯\(°_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 :-)

2013-02-04 Reply Admin

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);

2013-02-04 Reply Admin

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.

2013-02-04 Reply Admin

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.

2013-02-04 Reply Admin

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

dgvid · 2013-02-04 Reply Admin

justsomedudette:
Thanks, now I have saxophones stuck in my head.

Yeah, well, welcome to the house of fun.

2013-02-04 Reply Admin

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.

2013-02-04 Reply Admin

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.

2013-02-04 Reply Admin

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

2013-02-04 Reply Admin

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 :)

2013-02-04 Reply Admin

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.

2013-02-04 Reply Admin

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 Reply Admin

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

Lorne Kates · 2013-02-04 Reply Admin

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

New Criterii()

Lorne Kates · 2013-02-04 Reply Admin

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!

2013-02-04 Reply Admin

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.

2013-02-04 Reply Admin

+1

2013-02-04 Reply Admin

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

+1

2013-02-04 Reply Admin

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 Reply Admin

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

2013-02-04 Reply Admin

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...?

2013-02-04 Reply Admin

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

2013-02-04 Reply Admin

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.

2013-02-04 Reply Admin

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.

2013-02-04 Reply Admin

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.

2013-02-04 Reply Admin

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 Reply Admin

(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...

2013-02-04 Reply Admin

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.

None for All

Leave a comment on “None for All”