Inserting OAuth data into a database

Question

I am totally new to PHP. I just wrote a PHP script for google oauth to pull the data and insert into my database. I don't know if my code is vulnerable to SQL injection. Should I have used prepared statements and should I rewrite the code?

index.php

 <?php ini_set('display_errors', 1);
    error_reporting(E_ALL ^ E_NOTICE); ?>


    <?php 

    include_once("config.php");
    include_once("includes/functions.php");

    //print_r($_GET);die;

    if(isset($_REQUEST['code'])){
      $gClient->authenticate();
      $_SESSION['token'] = $gClient->getAccessToken();
      header('Location: ' . filter_var($redirect_url, FILTER_SANITIZE_URL));
    }

    if (isset($_SESSION['token'])) {
      $gClient->setAccessToken($_SESSION['token']);
    }



    if ($gClient->getAccessToken()) {
      $userProfile = $google_oauthV2->userinfo->get();
      //DB Insert
      //$gUser->setApprovalPrompt ("auto");

      $gUser = new Users();
      // As of PHP 5.3.0

      $gUser->checkUser('google',$userProfile['id'],$userProfile['given_name'],$userProfile['family_name'],$userProfile['email'],$userProfile['gender'],$userProfile['locale'],$userProfile['link'],$userProfile['picture'],$username);
      $_SESSION['google_data'] = $userProfile; // Storing Google User Data in Session
      header("location: feed.php");

      $_SESSION['token'] = $gClient->getAccessToken();
    } else {
      $authUrl = $gClient->createAuthUrl();
    }

      $email  = $_SESSION['google_data']['email'];
      $user = strstr($email, '@', true);
      $username = $user; 
?>

functions.php

 <?php ini_set('display_errors', 1);
error_reporting(E_ALL ^ E_NOTICE); ?>

<?php
 session_start();
class Users {
    public $tableName = 'users';

 function __construct(){
        //database configuration
        $dbServer = 'localhost'; //Define database server host
        $dbUsername = 'root'; //Define database username
        $dbPassword = ''; //Define database password
        $dbName = 'livelor'; //Define database name

        //connect databse
        $con = mysqli_connect($dbServer,$dbUsername,$dbPassword,$dbName);
        if(mysqli_connect_errno()){
            die("Failed to connect with MySQL: ".mysqli_connect_error());
        }else{
            $this->connect = $con;
        }
    }

    function checkUser($oauth_provider,$oauth_uid,$fname,$lname,$email,$gender,$locale,$link,$picture,$username){
        $prevQuery = mysqli_query($this->connect,"SELECT * FROM $this->tableName WHERE oauth_provider = '".$oauth_provider."' AND oauth_uid = '".$oauth_uid."'") or die(mysqli_error($this->connect));
        if(mysqli_num_rows($prevQuery) > 0){
            $update = mysqli_query($this->connect,"UPDATE $this->tableName SET oauth_provider = '".$oauth_provider."', oauth_uid = '".$oauth_uid."' ,fname = '".$fname."', lname = '".$lname."', email = '".$email."', gender = '".$gender."', locale = '".$locale."', picture = '".$picture."', gpluslink = '".$link."', modified = '".date("Y-m-d H:i:s")."' WHERE oauth_provider = '".$oauth_provider."' AND oauth_uid = '".$oauth_uid."'") or die(mysqli_error($this->connect));
        }else{
            $insert = mysqli_query($this->connect,"INSERT INTO $this->tableName SET oauth_provider = '".$oauth_provider."', oauth_uid = '".$oauth_uid."', fname = '".$fname."', lname = '".$lname."', email = '".$email."', gender = '".$gender."', locale = '".$locale."', picture = '".$picture."', gpluslink = '".$link."', created = '".date("Y-m-d H:i:s")."', modified = '".date("Y-m-d H:i:s")."' ,  username='".$username."' ") or die(mysqli_error($this->connect));
        }

        $query = mysqli_query($this->connect,"SELECT * FROM $this->tableName WHERE oauth_provider = '".$oauth_provider."' AND oauth_uid = '".$oauth_uid."'") or die(mysqli_error($this->connect));
        $result = mysqli_fetch_array($query);
        return $result;

    }
}

Answer 1

I don't know that my code is vulnerable to SQL injection.

Yes, it is.

You should never put any variables directly into SQL statements. Even if you think that the variables may possibly be safe, it's just really bad practice, and you will mess it up sooner or later. In your case, an attacker could use the profile fields, which would very likely lead to SQL injection (this depends a bit on the input filter for the profile fields, but I would be surprised if it caught all injections, and you should definitely not rely on it).

Should I have to use prepared statements and rewrite the code.

Yes. Prepared statements are the only reliable way to prevent SQL injection, and you should never write any SQL statements without prepared statements. It is bad style and very insecure.

Misc

• You really don't want to display errors in production.
• You have too much vertical whitespace.
• Some may disagree, but 2 spaces for indentation makes code harder to read. If you need this because your code is too nested, remove some levels of nesting instead of removing indentation.
• functions.php is quite a generic name. It also doesn't fit it's content at all. As it contains a class, it should have the same name as the class (possibly with .class added).
• You should create your database connection only once, and then pass it to the functions needing it, instead of creating a new connection for each object needing it.
• don't shorten variable names, just write them out.
• don't die in functions, it makes it hard for the calling code to handle.

Answer 2

Yes, your code is perfect to practice SQL injection.
Any basic example may be used. You picked the 100% procedural version and I can't see a single mysqli_real_escape_string()!

But, @tim already took care of that, with really good advices.

---

To add on to all the existing answers, there are plenty of things that are in need of an intervention:

• You have some useless repeated code!

  You currently have repeated these lines:

   <?php ini_set('display_errors', 1);
  error_reporting(E_ALL ^ E_NOTICE); ?>
  

  You can throw those away by creating the following php.ini file:

  display_errors = 1
  error_reporting = E_ALL ^ E_NOTICE
  

  There you go! And now all your pages will have these settings!

  • Stop die()ing on me!

  Really, you can't handle a die()! The code simply ... dies...

  You should throw some nice exception, and handle it like a champ instead of displaying an half-loaded page.

  Basically, your page eats itself because the user doesn't exist?

• Don't close the PHP tag in a page that will be used to only include classes/files!

  This will prevent you from accidentally sending garbage to the browser, forcing the headers to be sent. PHP automatically tries to prevent this and deletes 1 and only 1 whitespace after the closing tag. If you have bad luck, and you have 2 newlines, you will see this:

  Warning: Cannot modify header information - headers already sent by (output started at /some/file.php:12) in /some/file.php on line 23

  If you run session_start();, besides displaying that information, the sessions won't work!

• The way you load your database configurations is scary!

  You have 4 variables inside the constructor of the class.

  Everytime you share this code, you may accidentally send your server's authentification data! And you may not notice it until someone messes with it!

  This is my suggestion: create a separated file, like this:

  <?php
       //THE ELEMENTS MUST HAVE THE SAME ORDER AS THE ARGUMENTS
       return array(
           'host' => '127.0.0.1', //always prefer the IP
           'username' => 'john_doe',
           'password' => 'bla bla bla',
           'bd' => 'database'
       );
  

  To connect, use this:

  $con = call_user_func_array('mysqli_connect', array_values(require('bd-config.php')));
  

  And now, you can send this to someone else without problems. And, in case you need to change something in the future, you only change in that file, instead of sniffing around a 200+ lines (in case the code increases).

  And if someone tries to access it from http://localhost/bd-config.php, they will simply see a white screen!

I hope that this helped you.

Answer 3

I can also add something,

You can use UPDATE ON DUPLICATE KEY to prevent 1 extra query. To do that you will need to use a unique column to check the duplication.