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

Improve this question
10
  • which line no is 9? Commented Jan 24, 2018 at 10:16
  • 1
    Please visit 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!!! Commented Jan 24, 2018 at 10:17
  • 1
    Note also that your code is wide open to SQL injection, so be ready for more errors and problems. Commented Jan 24, 2018 at 10:18
  • 1
    Not much point in preparing a statement if you are going to dump the variables in just the same. Commented Jan 24, 2018 at 10:18
  • 1
    @WillemvanderSpek - that's the spirit :D Yes you should ;) Commented Jan 24, 2018 at 10:21

3 Answers 3

Reset to default
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();
Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

Is there any reason for bindParam instead of bindValue??
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 :)
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"]);
Improve this answer

5 Comments

... or bind the parameters!
sure, it's true
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%!!!!!!!
Securely not by 100%... anyway I inserted the warning.
@user2342558 thanks, mate :) sry for the late replay...
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'");
Improve this answer

6 Comments

Lovely how everyone still provides insecure answers. Holy.
... or bind the parameters!
@CD001 You can leave out the or :-)
@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!!! :)
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%!!!!!!
|

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.