7
\$\begingroup\$

I needed to build a one-way web service for a mobile app I developed. Its only goal is to receive incoming JSON data and insert it into the correct tables. It works, but it looks very "kludgy" and cobbled together (happens a lot with solo development).

I'd like another pair of eyes to see where it could be improved as far as design, robustness, readability, etc. This code is not yet in production, so I can implement suggested changes as needed.

<?php

$body = file_get_contents("php://input");
$json = json_decode($body, true);
//print_r($json);


function extractArray($arr)
{
    foreach ($arr as $k => $v) {
        if (is_array($v)) {
            extractArray($arr[$k]);
            foreach ($v as $kk => $vv) {
                if (is_array($vv)) {
                    unset($arr[$k][$kk]);
                    $arr[$kk] = $vv; // if you want the key to the array to be preserved.
                    // $arr[] = $vv; // would be safer, or check isset($arr[$kk]) first.
                }
            }
        }
    }
    return $arr;
}


function dbInsert(&$data)
{
    $tracks = array_shift($data);

//    //remove unneeded parent array
    $tracks = extractArray($tracks);

    /*
     * Note: user_id/page_id purposely unset.  Why? On the client side, Backbone.js
     * creates a model ID if one is not created manually. So, we create one on
     * the client side, but then remove it on the server side. Saves a lot of hassle otherwise.
     * db creates autoincrement id.
     * */
    unset($tracks['user_id']);


    $trackCols = array_keys($tracks);
    $trackVals = array_values($tracks);

    //connect to db
    $dbh = new PDO('mysql:host=localhost;dbname=kpl', 'root', 'root');
    $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

    //insert tracker data
    try {
        $sth = $dbh->prepare("INSERT INTO tracker (" . implode(", ", $trackCols) . ") VALUES ('" . implode("', '", $trackVals) . "')");

        $sth->execute();

        $tracksPK = $dbh->lastInsertId();
    } catch (PDOException $e) {
        return print_r('Tracker insert failed: ' . $e->getMessage());
    }

    $pages = array_pop($data);

    foreach ($pages as &$page) {
        unset($page['page_id']);
        $pageCols = array_keys($page);

        /*
         * Note: Could not use same query structure as trackers because the foreign key user_id
         * is set dynamically.
         * */
        try {
            $sth = $dbh->prepare("INSERT INTO page (" . implode(", ", $pageCols) . ") VALUES (" . $page['view_count'] . "," . $page['visit_timestamp'] . ",'" . $page['page_viewed'] . "'," . $tracksPK . ",'" . $page['model_type'] . "')");

            $sth->execute();

        } catch (PDOException $e) {
            return print_r('Page insert failed: ' . $e->getMessage());
        }

    }

    return true;
}

if (isset($json)) {

    $info = dbInsert($json);

    print_r($info);

} else {
    die('JSON data fail');
}
\$\endgroup\$
1
  • \$\begingroup\$ As an addendum, I found a decent example of how to encapsulate my db class Database class. Trying to incorporate this now like this try { $database->query("INSERT INTO tracker (" . implode(", ", $trackCols) . ") VALUES ('" . implode("', '", $trackVals) . "')"); $database->execute(); } catch (PDOException $e) { return print_r('Tracker insert failed: ' . $e->getMessage()); }. Getting xhr error returned [INFO] xhr error: status - 500, responseText - null \$\endgroup\$
    – Mike S.
    Jan 23 '14 at 16:48
7
+50
\$\begingroup\$

There are some big holes in your security

  1. Authenticate the data source. (Is it a legitimate source, username/password/etc)
  2. Sanitize the user data.

Check $trackCols against a list of valid column names you are expecting, to stop me doing things like I have below.

$valid_cols = array('id', 'user', 'name');
foreach ($trackCols as $col) {
    if (!in_array($col, $valid_cols)) {
        die("Alert! Unexpected Column $col");
    }
}

PDO allows you to bind variables to the statement, and it will escape any dangerous data. This will help stop people from creating attacks as I have below.

$sth = $dbh->prepare('SELECT name, colour, calories
    FROM fruit
    WHERE calories < :calories AND colour = :colour');
$sth->bindParam(':calories', $calories, PDO::PARAM_INT);
$sth->bindParam(':colour', $colour, PDO::PARAM_STR, 12);
$sth->execute();

I have included a test script below (DO NOT RUN AGAINST YOUR LIVE DATABASE)

I have manufactured some bad json data so that it will execute a delete statement on the tracker table in your database.

Obviously it won't work unless I get the table and column names correct, but as your code is now, if I try a dummy column name it will give me back an sql error message, which can help me to narrow it down.

Log the error message instead of returning them. This is an API, all the client end needs to know is that it failed. Don't give the attacker any more information then necessary.

<?php

// WARNING MAKE SURE YOU ONLY RUN THIS AGAINST A TEST DATABASE

$bad_sql = 'id) VALUES (1); DELETE FROM tracker; INSERT INTO tracker(id';
$json = json_encode(array(array($bad_sql => '99999')));

$ch = curl_init();

curl_setopt($ch, CURLOPT_URL,            "http://yourdomain/api.php" );
curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1 );
curl_setopt($ch, CURLOPT_POST,           1 );
curl_setopt($ch, CURLOPT_POSTFIELDS,     $json);
curl_setopt($ch, CURLOPT_HTTPHEADER,     array('Content-Type: application/json'));

$result=curl_exec ($ch);

var_dump($result);
\$\endgroup\$
3
  • \$\begingroup\$ I forgot to mention that this isn't a public interface. The url is encoded and static in the app. Unless the user hacks the app itself, sql injection is unlikely. Probably would have saved you the effort of writing this. I do appreciate your reply. \$\endgroup\$
    – Mike S.
    Jan 26 '14 at 15:21
  • 3
    \$\begingroup\$ Yes, details like that would have been helpful if disclosed upfront, hHowever it is still a good idea to santitize user data and use bind params regardless, even if it just to get into the habit. I can't imagine why someone would want to hack your app, but if they did it would be simple enough to run it through a proxy and monitor the traffic. Also logging in an api is essential otherwise if an error occurs you will have no way of analyzing what went wrong. \$\endgroup\$
    – bumperbox
    Jan 26 '14 at 20:17
  • 3
    \$\begingroup\$ All valid points. Sometimes, we are so close to the code that we forget that others don't have the same perspective. That's why I'm glad there's a site like this. It helps mitigate some of the negative aspects of solo freelance consulting. \$\endgroup\$
    – Mike S.
    Jan 27 '14 at 14:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.