Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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;

    }
}
share|improve this question
2  
If you are looking for a more comprehensive answer while learning a lot then check this out How can I prevent SQL-injection in PHP? – MonkeyZeus Feb 5 at 16:06
up vote 16 down vote accepted

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.
share|improve this answer
    
Thanks, for nice review. – Ozan Kurt Feb 5 at 15:08
    
"Prepared statements are the only reliable way to prevent SQL injection" - how about stripping every non-alphanumeric character or anything the ASCII range out? – Jan Dvorak Feb 5 at 17:48
2  
@JanDvorak removing anything out of the ASCII range wouldn't help at all. Stripping out non-alphanum works theoretically (as does "escaping if in quotes, casting to int if not"), but it's not a great solution. First of all, it's not practical. What if I'm named O'Mally? Secondly, it's too easy to screw up. The big advantage of prepared statements is that it defends right at the point where the vulnerability exists, not before. All you need to remember is to not put variables into queries, and you are safe. To check if all your queries are safe, just see if there are variables in them. – tim Feb 5 at 18:07
1  
With your strategy - or escaping - you defend at some point before the query happens. It is easy to forget this, or to do it wrong (eg typos such as $fof = safe($foo), or WHERE foo = '$foo' instead of $safe_foo etc). Security isn't just about what happens if you apply things perfectly, it's also about usability. And escaping and similar strategies are not very usable for developers, meaning that they will eventually make mistakes. Prepared statements really are the only proper solution to SQL injection, and there is really no reason not to use them. – tim Feb 5 at 18:07
    
@tim Thx A lot for your great review. I will rewrite my code and post it again. – Bajwa kapoor Feb 6 at 7:48

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.

share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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