Comment On None for All

Hans used less XML, and now he had two problems. [expand full text]
« PrevPage 1 | Page 2Next »

Re: None for All

2013-02-04 06:08 • by Jens (unregistered)
That is a prototype use case where using the Criteria API is wrong.

Re: None for All

2013-02-04 06:12 • by justsomedudette (unregistered)
Thanks, now I have saxophones stuck in my head.

Re: None for All

2013-02-04 06:48 • by PB (unregistered)
Shit happens.
Not too impressive WTF today.

Re: None for All

2013-02-04 06:49 • by bug (unregistered)
Criteria criteria = new Critera();

Re: None for All

2013-02-04 07:03 • by Philyofel (unregistered)
400489 in reply to 400488
bug:
Criteria criteria = new Critera();


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

Re: None for All

2013-02-04 07:33 • by 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?
}

Re: None for All

2013-02-04 07:35 • by Ken (unregistered)
400491 in reply to 400488
bug:
Criteria criteria = new Critera();
Is that the new three-way boolean expression I've been hearing about?

That logic is screwed!

2013-02-04 07:37 • by 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 :-(

Re: None for All

2013-02-04 07:41 • by 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.

Re: None for All

2013-02-04 07:42 • by faoileag (unregistered)
400494 in reply to 400491
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;

Re: None for All

2013-02-04 07:54 • by Steve The Cynic
400495 in reply to 400490
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...

Re: None for All

2013-02-04 08:01 • by faoileag (unregistered)
400496 in reply to 400490
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.

Re: None for All

2013-02-04 08:06 • by RonBeck62
TRWTF: How did this code get into the hands of end-users without being tested?

Re: None for All

2013-02-04 08:14 • by Dave the Snave (unregistered)
400498 in reply to 400485
Love it!

Re: None for All

2013-02-04 08:18 • by faoileag (unregistered)
400500 in reply to 400497
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 :-)

Re: None for All

2013-02-04 08:26 • by AV (unregistered)
400501 in reply to 400484
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.

Re: None for All

2013-02-04 08:41 • by Stebe (unregistered)
So, what exactly is the first part of the article about? How dooes productname come into it?

Re: None for All

2013-02-04 08:45 • by PG4 (unregistered)
400503 in reply to 400485
justsomedudette:
Thanks, now I have saxophones stuck in my head.


I got that heavy, heavy monster sound in mine

Re: None for All

2013-02-04 08:46 • by faoileag (unregistered)
400504 in reply to 400484
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.

Re: None for All

2013-02-04 09:15 • by ¯\(°_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.

Re: None for All

2013-02-04 09:15 • by StupidTheKid (unregistered)
400506 in reply to 400503
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.

Re: None for All

2013-02-04 09:20 • by ObiWayneKenobi
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).

Re: None for All

2013-02-04 09:26 • by faoileag (unregistered)
400509 in reply to 400505
¯\(°_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 :-)

Re: None for All

2013-02-04 09:29 • by 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);

Re: None for All

2013-02-04 09:53 • by 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.

Re: None for All

2013-02-04 09:54 • by Destman (unregistered)
400512 in reply to 400510
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.

Re: None for All

2013-02-04 09:55 • by Paul Neumann (unregistered)
At my code we could simply use the "nonuser" user account and post all transactions to the "unaccounted" account.

Re: None for All

2013-02-04 09:58 • by dgvid
400514 in reply to 400485
justsomedudette:
Thanks, now I have saxophones stuck in my head.

Yeah, well, welcome to the house of fun.

Re: None for All

2013-02-04 10:02 • by faoileag (unregistered)
400515 in reply to 400512
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.

Re: None for All

2013-02-04 10:11 • by Jack (unregistered)
400516 in reply to 400496
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.

Re: None for All

2013-02-04 10:11 • by Andrew (unregistered)
TRWTF is not using the proper singularization, "criterion", amirite?

Re: None for All

2013-02-04 10:14 • by Destman (unregistered)
400518 in reply to 400515
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 :)

Re: None for All

2013-02-04 10:25 • by faoileag (unregistered)
400520 in reply to 400516
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.

Re: None for All

2013-02-04 10:34 • by faoileag (unregistered)
400521 in reply to 400518
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.

<!-- Madness!!! -->

2013-02-04 10:39 • by DaveK
You don't do source code comments as well as Remy!

Re: None for All

2013-02-04 10:53 • by Lorne Kates
400523 in reply to 400517
Andrew:
TRWTF is not using the proper singularization, "criterion", amirite?


New Criterii()

Re: <!-- Madness!!! -->

2013-02-04 10:54 • by Lorne Kates
400524 in reply to 400522
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!

Re: None for All

2013-02-04 11:10 • by urza9814 (unregistered)
400525 in reply to 400516
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.

Re: None for All

2013-02-04 11:11 • by C-Derb (unregistered)
400526 in reply to 400502
+1

Re: None for All

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

Re: None for All

2013-02-04 11:32 • by 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.

Re: None for All

2013-02-04 12:16 • by chubertdev
This is the kind of code that you just take out with an axe...

Re: None for All

2013-02-04 12:50 • by 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...?

Re: None for All

2013-02-04 12:56 • by AnonyMark (unregistered)
It seems it was indeed in production, just like all the other failure modes discussed in the beginning of the article.

Re: None for All

2013-02-04 13:06 • by 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.

Re: None for All

2013-02-04 14:11 • by Jens (unregistered)
400535 in reply to 400504
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.

Re: That logic is screwed!

2013-02-04 14:11 • by Valued Service (unregistered)
400536 in reply to 400492
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.

Re: None for All

2013-02-04 14:33 • by 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...

Re: None for All

2013-02-04 14:35 • by Coyne
(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...

Re: That logic is screwed!

2013-02-04 14:46 • by Paul Neumann (unregistered)
400539 in reply to 400536
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.
« PrevPage 1 | Page 2Next »

Add Comment