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

Solution:

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

Solution:

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

Source