table name column name from GET parameter

Question

This is a function that reads a setting data from DB.

    public function getSetting(Request $request)
    {
        $user = Auth::user();
        $tableName = $request->name;
        $setting = \DB::table($tableName)->where('id', $user->id)->first();
        return response()->json($setting, '200', ['Content-Type' => 'application/json','Charset' => 'utf-8'], JSON_UNESCAPED_UNICODE);
    }

Do you think it is okay to put the table name from GET parameter.

Answer 1

This very much depends on what DB::table() is doing with the table name. If it only accepts string that are valid table names and throws exception otherwise, then it would be ok if you actually caught that exception and handle it.

try {
  $table = \DB::table($tableName);
} catch (InvalidTableNameException $e) {
  $this->logInvalidTableNameException($e); // might be good to log this stuff
  return response()->json(['error' => 'invalid_table_name'], 400)
}

$table->where(....

Answer 2

User input should be considered unsafe, since it may lead to SQL injection that may leak, destroy or modify information, thus we must protect against misuse. For security reasons the least information you release the better off you are.

Thus

Do not allow SQL Injection in column/table names nor reveal them.

// 1.- Best remap your column/table/db names (keys) to safe undisclosed ones
/**
 * Change unsafe keys for safe keys
 * remapKeys(['id'=>1, 'n'=>'Susan', 'm'=>3],  ['id'=>'user_id', 'n'=>'user_name', 'm'=>'friend'])
 *  [user_id=>1, user_name=>'Susan', 'friend'=>['user_id'=>3]
 *
 * @param array $inputArray ;
 * @param array $remapRules
 * @return array
 * @require php >= 8.0
 *
 */
    function remapKeys(array $inputArray, array $remapRules):array {
        $remapped = [];
        foreach($inputArray as $key => $value)
            if(array_key_exists($key, $remapRules) || is_numeric($key))
                $remapped[$remapRules[$key]] = is_array($value) ?
                    remapKeys($value, $remapRules) :
                    $value;
        return $remapped;
    }
    $in = ['id'=>1, 'n'=>'Susan', 'm'=>['id'=>3]];
    $protected = remapKeys($in, ['id'=>'user_id', 'n'=>'user_name', 'm'=>'friend']);
    // $protected = [user_id=>1, user_name=>'Susan', 'friend'=>['user_id'=>3] ]

// 2.- Second best validate your  column/table/db names, something like
    $in = ['user_id'=>1, 'user_name'=>'Susan', 'friend'=>['user_id'=>3], 'injection'=>'evil'];
    $protected =  array_intersect_key($in, ['user_id'=>'int', 'user_name'=>'string', 'friend'=>'array']);

//3.- If not at least the third best option: Protect your  column/table/db names
    $sql = "SELECT " . fieldIt($_REQUEST['column_name']) . 
        " FROM " . fieldIt($_REQUEST['table_name']) .
        " WHERE " . fieldIt($_REQUEST['filter_column_name']) . "=?";
    
/**
 * Protect with ` an sql name
 * Quotes a: column/table/db name to `column name` respecting . table.column to `table`.`column`
 *
 * @param string|Stringable $fieldName
 * @return string
 */
function fieldIt($fieldName):string {
    $protected = [];
    $n = explode('.',$fieldName);
    foreach($n as $field) {
        $protected[]= '`'.str_replace(['`',"\r","\n","\t","\0","\8","\\"], '', trimSuper($field) ).'`';
    }
    return implode('.', $protected);
}

/**
 * Trim a string, or array of strings, to a one liner with single spaces
 *
 * @param string|Stringable|array $str
 * @return:string|array
 */
function trimSuper($str):string|array {
    if($str === null)
        return '';
    if(is_array($str)) {
        foreach($str as &$d)
            $d = strim($d);
        return $str;
    }
    $s1 = preg_replace('/[\pZ\pC]/muS',' ',$str);
    return trim(preg_replace('/ {2,}/muS',' ',$s1));
}

Answer 3

The Laravel query builder is supposed to protect against SQL injection. But if you provide an invalid table name this code will trigger an exception, that may possibly result in information disclosure ie file paths, SQL queries, debug stacktrace. Did you try it ?

Leaking the table name is information disclosure in itself, that can potentially be leveraged by an attacker if a vulnerability is found on your site.

The code proposed by @slepic is an improvement, but it can also be used for enumeration attacks to determine valid table names on your system. That does not mean it can be hacked, but you are giving away too much information.

Answer 4

This allows SQL injection

According to the docs:

PDO does not support binding column names. Therefore, you should never allow user input to dictate the column names referenced by your queries, including "order by" columns.

Use a switch, match (PHP 8) or even an enum (PHP 8.1) to allow the user to select from a list of table names you want them to be able to access.

Why ID?

where('id', $user->id)

This bit makes no sense unless you are querying the user table. Otherwise it should be called user_id. The id column should always be the primary key of the table.

Use use

My linter recommends you use Illuminate\Support\Facades\DB;, probably because it makes it easier to keep track of what else you're using in your file.