3

I am using jTable and am attempting to sort records.

Currently the 'jtSorting' jTable variable is being passed correctly to my PHP script via the GET variable. I am calling a MySQL stored procedure like so:

$res = $mysqli->query("CALL getReadReportsPaging($startIndex, $pageSize, '$sorting');");

The query returns records and does not give an error, but does not return the records sorted by specified field. Records always return by Primary ID ASC.

Here is the Stored Procedure:

DELIMITER $$

CREATE DEFINER=`root`@`localhost` PROCEDURE `getReadReportsPaging`(startIndex INTEGER, pageSize INTEGER, sorting VARCHAR(255))
BEGIN
    SELECT * FROM reports 
    WHERE `new`=0
    ORDER BY sorting
    LIMIT startIndex, pageSize;
END

I tried calling the Procedure and passing in a column name with ASC or DESC like so:

call getReadReportsPaging(
    0,
    10,
    'ReportingName DESC'
);

Which also returns records with Primary ID ASC.

Any assistance appreciated.

19
  • $sorting is wrapped in single quotes. Commented Jun 19, 2014 at 17:23
  • 1
    I'm wondering why go through the effort of using a stored procedure for something that is a really simple query in php. Commented Jun 19, 2014 at 17:41
  • 1
    I agree with VikingBlooded. Unless this is not the whole stored procedure, it would be much simpler to build the string in PHP (using prepared statements, however, for user input). For reusability/maintainability you could even wrap the building of the string in a PHP function. Maybe not completely orthodox, but it'd be much easier for sure. Commented Jun 19, 2014 at 17:43
  • 1
    You have two options : building the query in PHP using prepared statement or you pass in the ORDER BY clause to the stored procedure, build up the SQL statement in a string and then execute this statement using EXEC or sp_ExecuteSql. Have a look at this for the second option. Commented Jun 19, 2014 at 17:47
  • 2
    When using mysqli you should be using parameterized queries and bind_param to add user data to your query. DO NOT use string interpolation to accomplish this because you will create severe SQL injection bugs. Commented Jun 19, 2014 at 18:24

1 Answer 1

1

I think this:

function buildQuery($startIndex, $pageSize, $sorting) {
    return "SELECT * FROM reports WHERE `new`=0 ORDER BY $sorting LIMIT $startIndex, $pageSize";
}

$res = $mysqli->query(buildQuery($startIndex, $pageSize, $sorting));

Is a lot easier than defining:

DELIMITER $$

CREATE DEFINER=`root`@`localhost` PROCEDURE `getReadReportsPaging`(startIndex INTEGER, pageSize INTEGER, sorting VARCHAR(255))
BEGIN
    SET @Query = "SELECT * FROM reports WHERE `new`=0 ORDER BY ";
    @Query = CONCAT(@Query, sorting);
    @Query = CONCAT(@Query, " LIMIT ");
    @Query = CONCAT(@Query, startindex);
    @Query = CONCAT(@Query, ", ");
    @Query = CONCAT(@Query, pageSize);
    PREPARE stmt FROM @Query;
    EXECUTE stmt;
END

and then calling:

$mysqli->query("CALL getReadReportsPaging($startIndex, $pageSize, '$sorting');");

I'd go with the first. Shorter and more readable.

Edit:
If the user can set $pageSize, $sorting or $startIndex, the methods I outlined are very unsafe. I don't know how to sanitise them in mySQL, but I can do that in PHP. In PHP I'd do this with prepared statements, but you want to use a variable for sorting. That is a column name, and you can't use prepared statements parameters for column names. Actually, you can only use them for values. So I suggest you do obtain the column name to sort on and ASC|DESC separately, (or just split on the first space) and then do something like this:

$columns = array(/* the column names of the table `reports` as strings */);

if (!in_array($sortingColumn, $columns) {
    $sortingColumn = /* insert default sorting column name here */;
}

if ($sortingDirection != "ASC" && $sortingDirection != "DESC") { // I may be forgetting a sorting direction here.
    $sortingDirection = /* insert default sorting direction here */;
}

$stmt = $mysqli->prepare("SELECT * FROM reports WHERE `new`=0  ORDER BY $sortingColumn $sortingDirection LIMIT ?, ?");
$stmt->bindParam("ii", intval($startIndex), intval($pageSize)); // If the input to intval cannot be parsed as int it will return 0. This prevents injection in $startIndex and $pageSize.

$stmt->execute();

The above snippet of PHP code can be wrapped into a function as well. (Which could even return the $stmt without calling $stmt->execute().)

To keep the mySQL way safe, you would need to somehow perform the same checks in mySQL (which I think is somewhat impractical). But here goes:

DELIMITER $$

CREATE DEFINER=`root`@`localhost` PROCEDURE `getReadReportsPaging`(startIndex INTEGER, pageSize INTEGER, sortingDirection VARCHAR(255), sortingColumn VARCHAR(255))
    BEGIN
        SET @Query = "SELECT * FROM reports WHERE `new`=0 ORDER BY ";
        IF NOT sortingColumn IN (/* list of column names here */) THEN
            sortingColumn = /* default sorting column */;
        @Query = CONCAT(@Query, sortingColumn);
        @Query = CONCAT(@Query, " ");
        IF NOT sortingDirection IN ('ASC', 'DESC' /* I may be forgetting some directions) THEN
            sortingDirection = /* default sorting direction */;
        @Query = CONCAT(@Query, sortingDirection);
        @Query = CONCAT(@Query, " LIMIT ");
        @Query = CONCAT(@Query, CAST(startindex AS SIGNED)); // I assume this will return an int even if the input is not parseable as int.
        @Query = CONCAT(@Query, ", ");
        @Query = CONCAT(@Query, CAST(pageSize AS SIGNED));
        PREPARE stmt FROM @Query;
        EXECUTE stmt;
    END

Disclaimer: 1) I am not a security expert and 2) I did not test all the above code. I hope it will help you anyway.

Sign up to request clarification or add additional context in comments.

6 Comments

Makes sense it is alot easier but does it open up any possibility for injection? Basically I am asking now are both ways equally secure or is one more favorable then the other?
@user2210274 Depends. If one (or more) of the variables you pass in (either startIndex, pageSize or sorting) is input from the user, yes. (Even if it is from a select or checkbox. The user does not need the page with the form to send a request.) I'll edit my answer.
@user2210274 The two ways I outlined are equally vulnerable to SQL injection.
Okay thank you! Ill have to read more about injection, I've always been told the latter is more secure.
@user2210274 Your SQL way is more secure than my PHP way (had the SQL way worked). But my SQL way is just as secure as my PHP way (because it is basically the same method, but implemented in two languages).
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.