PHP function to access a database and return json

Question

<?php

include_once('../dbInfo.php');

function getReport($user_table) {
    $tables = array(
        "day"        => "p_day", 
        "month"      => "p_month"
        ... etc. .....
    );

    $table = $tables[$user_table];

    if(!$table) {
        die(json_encode(array("error" => "bad table name")));
    }

    $con = getConnection(); // getConnection is in '../dbInfo.php'

    $query = "select * from " . $table;
    $res = mysql_query($query, $con);

    if(!$res) {
        die(json_encode(array("error" => "no results from table")));
    }

    $fields_num = mysql_num_fields($res);
    $fields = array();
    for($i=0; $i < $fields_num; $i++) {
        $field = mysql_fetch_field($res);
        $fields[$i] = $field->name;
    }

    $i = 0;
    while($row = mysql_fetch_array($res)) {
        $rows[$i] = $row;
        $i++;
    }
    $json = array("rows" => $rows, "headers" => $fields);

    $jsontext = json_encode($json);
    return $jsontext;
}

?>

What this code is doing:

• access the database, selecting rows from a table, and returning them as a serialized json object
• a table name is looked up in $tables -- the keys are acceptable user input, the values are actual table/view names in the database
• data is selected from the table
• the data is put into a big hash
• the hash is serialized as a json string and returned

---

Specific issues I'm concerned about:

1. security -- is my DB connection info safe? This file is in the root directory of public content, so dbiInfo.php, with the database connection information, is not publicly accessible (I think)
2. security -- am I open to SQL injection attacks? I build a SQL query with string concatenation
3. security -- $user_table is untrusted input; is it safe? It's only used as a key to look up trusted input ...
4. error handling -- have I dealt with all error conditions
5. there are lots of versions of PHP functions -- am I using the right ones?

General issues:

• following conventions
• quality/readability/comments

---

Edit: the data is publicly available -- I'm worried about somebody getting more than read access to one of the listed tables, or any access to any other table in the DB.

Answer 1

This:

$tables = array(
    "day"        => "p_day", 
    "month"      => "p_month"
    ... etc. .....
);

$table = $tables[$user_table];

if(!$table) {
    die(json_encode(array("error" => "bad table name")));
}

will throw a notice if $user_table is not a valid array key, something you should have already tested. Rewrite as:

$tables = array(
    "day"        => "p_day", 
    "month"      => "p_month"
    ... etc. .....
);

if(!array_key_exists($user_table, $tables)) {
    die(json_encode(array("error" => "bad table name")));
}

$table = $tables[$user_table];

I really hate it when functions die(), and in your case there's no point in that. You could simply do:

if(!$table) {
    return json_encode(array("error" => "bad table name"));
}

since the function is expected to return a json formatted string. If you really want to die() you should do that where you call the function and the returned string is an error. Or you could just return false when an error occurs and the raw array when everything works, and when calling the function do something like:

$result = json_encode(getReport("some_table"));

That way getReport() will be useful even when you don't need json encoded output. But that's up to you and how you actually use it.

As @LokiAstari mentions mysql_* functions are deprecated and should be avoided. I would skip mysqli_* functions too and use PDO which will give you the extra bonus of switching to another database engine if you ever need to and it has a nice OO interface.

For everything else, I'm with @LokiAstari.

Answer 2

If the output is json you may want to set the content type:

This is what I nievely do:

$type   ="text/json";
if ($_GET{"test"})
{
    $type   = "text/plain";
}
header("Content-type: $type");

I also use mysql_query() but that is because my PHP is not good (and I have not done it for a while). But apparently this DB module is deprecated in favour of "mysqli(mproved)" module. http://us.php.net/manual/en/book.mysqli.php

security -- is my DB connection info safe? This file is in the root directory of public content, so dbiInfo.php, with the database connection information, is not publicly accessible (I think)

Don't think it makes a difference.
If your Web server is set up correctly it will never deliver php files (only execute them).

security -- am I open to SQL injection attacks? I build a SQL query with string concatenation

Potentially. All input from insecure location must be escaped.

• http://us.php.net/manual/en/function.mysql-real-escape-string.php
• http://us.php.net/manual/en/mysqli.real-escape-string.php

Also you print error message that contain information about your tables into the response. This is definitely a security problem. The only response to the user should be a failed message (not why it failed), potentially with an ID so you can use this to look it up.

All the error should be put into the log file (with the query that generated them). You can then use the ID provided in the error message to look up the query and the error message it produced.

security -- $user_table is untrusted input; is it safe? It's only used as a key to look up trusted input ...

And data from this table must be escaped before use.

error handling -- have I dealt with all error conditions there are lots of versions of PHP functions -- am I using the right ones?

OK. Not an expert here so others may know better but I like to put the error checking and command on the same line.

$res = mysql_query($query, $con);

if(!$res) {
    die(json_encode(array("error" => "no results from table")));
}

// For me this would be
$res = mysql_query($query, $con) or LogErrorAndDie(array("error" => "no results from table"));

Answer 3

<?php

include_once('../dbInfo.php');

function resolve_table($user_table) {
    $tables = array(
        "day"        => "p_day",
        "month"      => "p_month"
        ... etc. .....
    );

    // check table exists or return null
    return isset($tables[$user_table]) ? $tables[$user_table] : null;
}


function getReport($user_table) {

    $output = array();

    try {
        // move resolve table into separate function so can be reused
        $table = resolve_table($user_table);

        if(!$table) {
           // don't die here, throw and exception instead
           throw new Exception("bad table name");
        }

        $con = getConnection(); // getConnection is in '../dbInfo.php'

        // this is safe as $table is a trusted source,
        // have added backticks around table name incase it is a reserved word in mysql
        // $query = "select * from " . $table
        $query = "select * from `$table`";

        $res = mysql_query($query, $con);

        if(!$res) {
            // don't return error here, throw exception
            throw new Exception("no results from table");
        }

        // do you want to handle the case of no results?
        if (mysql_num_rows($res) == 0) {
            throw new Exception("no results from table");
        }

        // as others have said, mysqli_ or pdo not mysql_ functions
        // see nicer way of doing this below
//        $fields_num = mysql_num_fields($res);
//        $fields = array();
//        for($i=0; $i < $fields_num; $i++) {
//            $field = mysql_fetch_field($res);
//            $fields[$i] = $field->name;
//        }

        // declare the array before using
        $rows = array();

        // not needed
        // $i = 0;

        // use fetch assoc instead to get an associative array
        // while($row = mysql_fetch_array($res)) {
        while($row = mysql_fetch_assoc($res)) {
            // $rows[$i] = $row;
            // $i++;

            // [] automatically adds next element to array
            $rows[] = $row;
        }

        // assuming there is at least 1 row in result set (we have tested for 0 rows above)
        // we can just get the array_keys of the last $row
        $fields =  array_keys($row);

        // if exceptions have been thrown we will never reach this line
        $output = array("rows" => $rows, "headers" => $fields);

    } catch (Exception $ex) {
        $output['error'] = $ex->getMessage();
    }

    // not necessary
    // $jsontext = json_encode($output);

    return json_encode($output);
}

// remove closing php tag (unless something follows), otherwise you may have unwanted whitespace after it, which can cause header or cookie issues
// ?  >