ZF-1.12.7 breaks code when using multi column ordering

[DragonBe]

Last night we upgraded our codebases with the latest ZF1 version 1.12.7 which triggered all sorts of alarm bells. It took us about half a day to figure out why this was.

We have queries using multi-column ordering, which are now no longer functional because of the security fix ZF2014-04, as it was mentioned in the release notes:

ZF2014-04, which mitigates a potential SQL Injection (SQLi) vector when usiing ORDER BY clauses in Zend_Db_Select; SQL function calls were improperly detected, rendering ORDER clauses such as MD5(1);drop table foo unfiltered. The logic has been updated to prevent SQLi vectors, and users of this functionality are strongly encouraged to upgrade immediately.

Example Zend_Db_Select statements that fail now:

$select->order('productId ASC');
$select->product(array ('productId ASC', 'userId DESC'));

This code now translates into the following query

SELECT … ORDER BY 'productId ASC';

Which triggers the following MySQL errors:

Mysqli prepare error: Unknown column 'productId ASC'
Mysqli prepare error: Unknown column 'productId ASC, userId DESC'

I can imagine this is not the required result this fix should be!

We can't upgrade at this point.

[weierophinney]

@ezimuel Can you please work with @DragonBe on this?

Also, @DragonBe - I can see a number of ways to accomplish what you want with existing functionality that would work in both 1.12.6 and 1.12.7:

$select->order(array('productId', 'ASC')); // SELECT ... ORDER BY 'productId' ASC
$select->order(new Zend_Db_Expr("'productId' ASC")); // same

$select->order(array(
    new Zend_Db_Expr("'productId' ASC"),
    new Zend_Db_Expr("'userId' DESC"),
)); // SELECT ... ORDER BY 'productId' ASC, 'userId' DESC

I'm a bit surprised, though, that the string 'productId ASC' is not working -- line 599 should be matching the direction and separating it into the value productId and the direction ASC. @ezimuel -- use these as test cases. :)

[ezimuel]

@DragonBe I'm very surprised that the unit tests didn't cover this simple use case, all the tests was green. Anyway, I'll work on this to discover what is happened, adding more tests of course.

[DragonBe]

I'm almost done with my unit tests, will send a pull request in the next 20 minutes.

Well, now we're going to have them in for future changes as well.

[DragonBe]

Created a pull request #379 but it will fail the tragic-ci test (at least it fails when using zf-1.12.7).

But this test doesn't fail without a hick on zf-1.12.6 and below

I will look at the changes of @ezimuel to see why this fails now and how we could improve this.

[DragonBe]

@weierophinney I'd love to make those changes to my code, but we're having over a 1000 queries like this… fixing the ZF1 code seems like a more proper way of doing it.

[ezimuel]

@DragonBe Thanks for the unit tests! I'm checking and I'll let you know asap.

[DragonBe]

👍

[mhujer]

I've fixed the master tests in #380 (I hope, still waiting for Travis) Anyone can review and merge that PR? @DragonBe can afterwards rebase his PR on the master master to see if only that one fails

[ezimuel]

@DragonBe I tested your SQL statements and all passed! Can you execute this example code on your environment and let me know? Thanks!
Which DBMS are you using?

[DragonBe]

I'm confused now! @ezimuel, I ran your test on several of our servers and all passed! Even ran the script in isolation with version output, still all passed.

I also completed my test case scenarios for the whole issue and committed it to GitHub, ran all the tests with both Mysqli and Pdo_Mysql. They all pass except one use case: multi fields for ordering, one without and one with direction!

See my ZF-378 test cases yourself and run them independently to validate the issue.

[DragonBe]

Hmm, fixed a typo pointed out by @weierophinney and updated an expectation to a real value, and I can no longer recreate the issue: all tests pass!

Need to figure this one out once I'm back in the office next week… I must be missing something.

Anyways, I will create these testcases for ZF1 so at least every future change can be tested against it.