New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bookmark::getList function has a logical bug in the query conditions #13761

Closed
gnanet opened this Issue Oct 19, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@gnanet

gnanet commented Oct 19, 2017

I have created a dozen of bookmarks in PMA 4.4.15.6, and had no issues to have bookmarks with the same label but for different databases. Today i upgraded to PMA 4.7.4 and found that almost all of my bookmarks are shown if i am within a database scope.

Investigating the issue lead me in the end to the SQL query that retrieves the bookmarks, and seen a huge difference in the logic compared from 4.4.15.6 to 4.7.4:

in 4.4.15.6 the retrieval of the bookmarks is a multi query procedure depending on db is selected or not, roughly outlined below:

if ($db !== false) {
$per_user = $GLOBALS['dbi']->fetchResult(... with db condition
$global = $GLOBALS['dbi']->fetchResult(... with db condition
} else {
$per_user = $GLOBALS['dbi']->fetchResult(...
$global = $GLOBALS['dbi']->fetchResult(...
}

in 4.7.4 the retrieval of the bookmarks is a single query procedure, with the query string extended for database condition depending on db is selected or not:

        $query = "SELECT * FROM " . Util::backquote($cfgBookmark['db'])
            . "." . Util::backquote($cfgBookmark['table'])
            . " WHERE  `user` = ''"
            . " OR `user` = '" . $GLOBALS['dbi']->escapeString($cfgBookmark['user']) . "' ";
        if ($db !== false) {
            $query .= " AND dbase = '" . $GLOBALS['dbi']->escapeString($db) . "'";
        }
        $query .= " ORDER BY label ASC";

The idea to have a single query for per-user and shared bookmarks is great, but this new process did not count with the rules of mysql operator precedence if the database is specified.

Lets have a look at the new query:

SELECT * FROM `pma`.`pma__bookmark` WHERE `user` = '' OR `user` = 'MyUser' AND dbase = 'MyDatabase' ORDER BY label ASC

The precedence causing the "logical AND" to evaluated first, and the "logical OR" as next, resulting in every shared bookmark, OR the per-user bookmarks of the specified database.

The intended procedure, to retrieve the per-user or shared bookmarks for the the database would reqire to use parentheses:

SELECT * FROM `pma`.`pma__bookmark` WHERE ( `user` = '' OR `user` = 'MyUser' ) AND dbase = 'MyDatabase' ORDER BY label ASC

Which results in the correct amount of bookmarks.

Also leaving the parentheses in the query, without the database condition has IMO no negative effect, so i suggest to add the parentheses to the lines below:

. " WHERE `user` = ''"

. " OR `user` = '" . $GLOBALS['dbi']->escapeString($cfgBookmark['user']) . "'";

What do you think?
EDIT: reverted code for 4.7.4 to not include the parenthesis, and really illustrate the bug - Thanks to @nijel for pointing to my mistake

@nijel nijel self-assigned this Oct 20, 2017

@nijel nijel added the bug label Oct 20, 2017

@nijel nijel added this to the 4.7.5 milestone Oct 20, 2017

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Oct 20, 2017

Member

You're right, I'll fix this.

PS: The code you have shown for 4.7.4 already contains the parenthesis, so the report is a bit confusing :-).

Member

nijel commented Oct 20, 2017

You're right, I'll fix this.

PS: The code you have shown for 4.7.4 already contains the parenthesis, so the report is a bit confusing :-).

@gnanet

This comment has been minimized.

Show comment
Hide comment
@gnanet

gnanet Nov 7, 2017

@nijel yes you are right. If i correct that, then it would not reopen hopefully...

gnanet commented Nov 7, 2017

@nijel yes you are right. If i correct that, then it would not reopen hopefully...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment