3

I am currently working on a forum website with an upvote-system. However, there are some annoying, probably syntactic errors that are bugging me. I am talking about this piece of code.

<?php
session_start();

include_once 'dbh_discussion.inc.php';
$conn = db_discussion_connect();

$thread_id = $_POST['upvote'];

$sql1 = $conn->prepare("SELECT * FROM users WHERE user_id = '$_SESSION['u_id']' AND thread_id = '$thread_id'");

The things that aren't clear in this piece of code are as follows:

  • db_discussion_connect() A function declared in dbh_discussion_connect.inc.php. This funtion returns a new PDO that connects to my database.
  • the index 'upvote' is the name of a button in another php file that will call the code above.
  • $_SESSION['u_id'] is a session variable that will be assigned when the user logs onto the website.

The error that I'm getting when debugging on the server:

Parse error: syntax error, unexpected '' (T_ENCAPSED_AND_WHITESPACE), expecting '-' or identifier (T_STRING) or variable (T_VARIABLE) or number (T_NUM_STRING) in /var/www/html/includes/thread_upvotes.inc.php on line 9

I feel like I'm missing out on something syntactical. Anyhow, I'd really appreciate someone telling me whats going wrong here.

Thanks

jeroen
  • 91,079
  • 21
  • 114
  • 132
Willem van der Spek
  • 125
  • 1
  • 1
  • 8
  • which line no is 9? – KMS Jan 24 '18 at 10:16
  • line 9: $sql1 = $conn->prepare("SELECT * FROM users WHERE user_id = '$_SESSION['u_id']' AND thread_id = '$thread_id'"); – Willem van der Spek Jan 24 '18 at 10:17
  • 1
    Please visit http://bobby-tables.com and learn about SQL injection and how to use prepared statements. Right now your code is really vulnerable to injections and your whole database could be hacked in a few seconds!!! – Twinfriends Jan 24 '18 at 10:17
  • 1
    Note also that your code is wide open to SQL injection, so be ready for more errors and problems. – David Jan 24 '18 at 10:18
  • 1
    Not much point in preparing a statement if you are going to dump the variables in just the same. – jeroen Jan 24 '18 at 10:18
  • ye youre right, should add a bindParam/bindValue shouldnt I? – Willem van der Spek Jan 24 '18 at 10:20
  • 1
    @WillemvanderSpek - that's the spirit :D Yes you should ;) – CD001 Jan 24 '18 at 10:21
  • @WillemvanderSpek You should not change your question using the code provided in the answers or comments as these will not make sense any more. If you have improved your code and still have a problem, put it below the original question. – jeroen Jan 24 '18 at 10:33
  • @jeroen Sorry, reason was for other people to not copy paste faulty code and make the same mistake I did: forgetting the bindParam/bindValue statements. – Willem van der Spek Jan 24 '18 at 10:37
  • @WillemvanderSpek I think this question with the answers and comments makes it pretty clear what the correct solution is :-) – jeroen Jan 24 '18 at 10:39

3 Answers3

3

I get triggered so hard by this people who provide answers that are still wide open to Injections. Is it that difficult to change his prepared statement to something safe?!!!

Here a solution with a correct prepared statement. As if it takes that long to rewrite it. That should be against the rules here.

<?php
session_start();

include_once 'dbh_discussion.inc.php';
$conn = db_discussion_connect();

$sql1 = $conn->prepare("SELECT * FROM users WHERE user_id = :uid AND thread_id = :tid");
$sql1->bindParam(':uid', $_SESSION["u_id"]);
$sql1->bindParam(':tid', $_POST['upvote']);
$sql1->execute();
Twinfriends
  • 1,972
  • 1
  • 14
  • 34
  • Is there any reason for bindParam instead of bindValue?? – Willem van der Spek Jan 24 '18 at 10:25
  • http://www.php.net/manual/en/pdostatement.bindparam.php `Unlike PDOStatement::bindValue(), the variable is bound as a reference and will only be evaluated at the time that PDOStatement::execute() is called.` Hope that helps. If you've more questions feel free to ask :) – Twinfriends Jan 24 '18 at 10:26
1

Your code has an error, specifically the code user_id = '$_SESSION['u_id']', try this:

 $sql1 = $conn->prepare("SELECT * FROM users 
 WHERE user_id = '{$_SESSION['u_id']}' AND thread_id = '$thread_id'");

To insert array keys inside a string, you must enclose it in { } if you specify the key inside ' '

WARNING inserting directly $_SESSION contenst in the query you'll be eligible for SQL Injection!!!

The correct and better way to insert them is by binding each one like this:

$sql1 = $conn->prepare("SELECT * FROM tableName WHERE fieldID = :id");
$sql1->bindParam(':id', $_SESSION["id"]);
user2342558
  • 5,567
  • 5
  • 33
  • 54
0

seems like quotes making problem, try like below,

$uid = $_SESSION['u_id'];
$sql1 = $conn->prepare("SELECT * FROM users WHERE user_id = '$uid' AND thread_id = '$thread_id'");
KMS
  • 566
  • 4
  • 16
  • 5
    Lovely how everyone still provides insecure answers. Holy. – Twinfriends Jan 24 '18 at 10:19
  • 2
    ... or **bind the parameters!** – CD001 Jan 24 '18 at 10:20
  • 1
    @CD001 You can leave out the *or* :-) – jeroen Jan 24 '18 at 10:23
  • 1
    @CD001 I don't understand how people still can provide such examples... and the problem is, that 99 % of the people who ask questions here then don't care at all about how to write correct prepared statements because they've a bad code example - but it works. So they don't care. It should be against the rules to provide such examples. Injections are such a huge problem and it could be so easy to prevent them. Tank you for mentioning on answers that they need to bind the params, at least there are still some people who understand it!!! :) – Twinfriends Jan 24 '18 at 10:24
  • @jeroen it was going to be "or bind the parameters like any sane person" ... but I thought that might be a little passive/aggressive ;) – CD001 Jan 24 '18 at 10:30
  • 1
    Please consider removing/editing your answer to inform the readers that this will add a well known security vulnerability to their app/website/Server!!! There are right and **secure** answers given to this question, yours is subject to make the readers hackable by 100%!!!!!! – cramopy Jan 25 '18 at 07:18