What's a better way to make this insert more secure and safe from injection and manipulation

Question

I've been trying to put together functions in a more secure way that keeps us safe from injection or manipulating inserts by calling different columns to be updated. In your opinion, is this function safe at all, and if not what would you suggest is a better way to do it, and why.

This function is called when a user updates their profile, or specific parts of their profile, as you can see I've made an array with items which is all they can update in that table. Also, the user_id I am getting is from the secure encrypted JSON token that's attached to their session, they are not sending that. Thanks for your time.

function updateProfile( $vars, $user_id ) {
    $db = new Database();
    $update_string = '';
    $varsCount = count($vars);
    $end = ',';
    $start = 1;
    $safeArray = array( "gradYear", "emailAddress", "token", "iosToken", "country", 
"birthYear", "userDescription" );
    foreach($vars as $key => $value) {
        if(in_array( $key, $safeArray )) {
            if($start == $varsCount) {
                $end = '';
            }
         
            $update_string .= $key . '=' . '"' . $value . '"' . $end;
        }
            $start++;
    }

    if($start > 0) {
        $statement = "update users set " . $update_string . " where userId = '$user_id'";
        $query = $db->updateQuery( $statement );
        if($query) {
            $response  = array( "response" => 200 );
        } else {
            $response  = array( "response" => 500, "title" => "An unknown error occured, 
please try again");
        }

    }

Answer 1

As the comments above suggest, it's worth using query parameters to protect yourself from SQL injection.

You asked for an example of how anything malicious could be done. In fact, it doesn't even need to be malicious. Any innocent string that legitimately contains an apostrophe could break your SQL query. Malicious SQL injection takes advantage of that weakness.

The weakness is fixed by keeping dynamic values separate from your SQL query until after the query is parsed. We use query parameter placeholders in the SQL string, then use prepare() to parse it, and after that combine the values when you execute() the prepared query. That way it remains safe.

Here's how I would write your function. I'm assuming using PDO which supports named query parameters. I recommend using PDO instead of Mysqli.

function updateProfile( $vars, $userId ) {
    $db = new Database();
    $safeArray = [
        "gradYear",
        "emailAddress",
        "token",
        "iosToken",
        "country",
        "birthYear",
        "userDescription",
    ];
    // Filter $vars to include only keys that exist in $safeArray.
    $data = array_intersect_keys($vars, array_flip($safeArray));

    // This might result in an empty array if none of the $vars keys were valid.
    if (count($data) == 0) {
        trigger_error("Error: no valid columns named in: ".print_r($vars, true));
        $response = ["response" => 400, "title" => "no valid fields found"];
        return $response;
    }
    
    // Build list of update assignments for SET clause using query parameters.
    // Remember to use back-ticks around column names, in case one conflicts with an SQL reserved keyword.
    $updateAssignments = array_map(function($column) { return "`$column` = :$column"; }, array_keys($data));
    $updateString = implode(",", $updateAssignments);

    // Add parameter for WHERE clause to $data. 
    // This must be added after $data is used to build the update assignments.
    $data["userIdWhere"] = $userId;
    
    $sqlStatement = "update users set $updateString where userId = :userIdWhere";

    $stmt = $db->prepare($sqlStatement);
    if ($stmt === false) {
        $err = $db->errorInfo();
        trigger_error("Error: {$err[2]} preparing SQL query: $sqlStatement");
        $response = ["response" => 500, "title" => "database error, please report it to the site administrator"];
        return $response;
    }
    
    $ok = $stmt->execute($data);
    if ($ok === false) {
        $err = $stmt->errorInfo();
        trigger_error("Error: {$err[2]} executing SQL query: $sqlStatement");
        $response = ["response" => 500, "title" => "database error, please report it to the site administrator"];
        return $response;
    }

    $response = ["response" => 200, "title" => "update successful"];
    return $response;
}

Answer 2

In addition to the excellent Bill's answer, one little suggestion: always make your methods to do one thing at a time. If a method's job is to update a database, then it should only update a database and nothing else, the HTTP interaction included. Imagine this method could be used in non-AJAX context or without a web-server at all but from a command line utility. Those HTTP codes and JSON responses would look completely off the track. So have two classes: one to update the database and one to interact with the client. It will make your code much cleaner and reusable.

Also, never create a new connection to the database for the every query. Instead, have a ready made connection and use it for all database interactions.

function updateProfile($db, $vars, $userId )
{
    $safeArray = array( "gradYear", "emailAddress", "token", "iosToken", "country", 
"birthYear", "userDescription" );

    // let's check if all columns are safe
    if (array_diff(array_keys($vars), $safeArray)) {
        throw new InvalidArgumentException("Unknown columns provided");
    }
        
    $updateAssignments = array_map(function($column) { 
        return "`$column` = :$column"; }, array_keys($vars)
    );
    $updateString = implode(",", $updateAssignments);

    $vars["userIdWhere"] = $userId;
    
    $sqlStatement = "update users set $updateString where userId = :userIdWhere";    
    $db->prepare($sqlStatement)->execute($vars);
}

See, it makes your code slim and readable. And, above all - reusable. You don't have to make your methods bloated. PHP is a very concise language, if used properly