HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpModerate

Inserting OAuth data into a database

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
intodatabaseinsertingoauthdata

Problem

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


    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

```

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."', pict

Solution

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.

Context

StackExchange Code Review Q#119004, answer score: 16

Revisions (0)

No revisions yet.