1

I'm having an issue with a form I have created (Test purposes only, I am aware it's vulnerable to SQL Injection)

Basically, the form does not insert into the DB, yet it seems to be returning true on the script.

The code is as follows:

form.php

   <form action="create.php" method="post">
        <p>Username: <input type="text" name="username" />
        </p>
        <p>Password: <input type="password" name="password" />
        </p>
        <p><input type="submit" value="Create" name= "cre" />
        </p>
    </form>

create.php

<?php
session_start();
$dbname = "obsidian";

if(isset($_POST['cre'])){


    $username = $_POST['username'];
    $password = $_POST['password'];

    $mysqli = new mysqli('localhost','admin1', 'password1','obsidian' ) or die('Failed to connect to DB' . $mysqli->error );

    $hashed_password = password_hash($password,PASSWORD_DEFAULT);

         $registerquery = "INSERT INTO users (username, hash) VALUES('$username', '$hashed_password')";
        if($registerquery = true)
        {
            echo "<h1>Success</h1>";
            echo "<p>Your account was successfully created. Please <a href=\"index.php\">click here to login</a>.</p>";
        }
        else
        {
            echo "<h1>Error</h1>";
            echo "<p>Sorry, your registration failed. Please go back and try again.</p>";    
        }       
     }   


  ?>

I get the success message, but as I stated, the values do not get inserted into the DB.

Any help would be good.

5
  • 2
    You never actually run the Insert query on the database. Commented Dec 3, 2014 at 19:04
  • 4
    ... and you're vulnerable to sql injection attacks Commented Dec 3, 2014 at 19:06
  • do you make a difference between = and ==? Commented Dec 3, 2014 at 19:07
  • WARNING: When using mysqli you should be using parameterized queries and bind_param to add user data to your query. DO NOT use string interpolation to accomplish this because you will create severe SQL injection bugs. If you're having trouble with low-level database calls like this, I strongly encourage you to adopt a development framework like Laravel that fits your style and needs. Commented Dec 3, 2014 at 19:13
  • Thanks for the help guys - Sorry for being stupid, i'm new to this. Commented Dec 3, 2014 at 19:18

5 Answers 5

6

This defines a query, but does NOT run it:

 $registerquery = "INSERT INTO users (username, hash) VALUES('$username', '$hashed_password')";

The this is NOT "testing" for success. It's simply seeing the variable to true:

    if($registerquery = true)

= is assignment, == is for equality testing.

Sign up to request clarification or add additional context in comments.

1 Comment

Yeah, it still needs prepare(), bind_param() and execute() calls.
3

You have to query the database. Try this:

$registerquery = "INSERT INTO users (username, hash) VALUES('$username', '$hashed_password')";

if ($mysqli->query($registerquery))
{
    // success.
}
else
{
    // failed.
}

Here is the documentation: http://php.net/manual/en/mysqli.query.php

Comments

1

You missed the step where you actually hand the SQL query to the database.

$mysqli->query($registerquery);

has to be run before it will be inserted.

You can also change your if statement to the following

if ($mysqli->query($registerquery))

Also, you're currently using a single =, which is setting $registerquery instead of checking its value.

Comments

1

All you are doing here:

$registerquery = "INSERT INTO users (username, hash) VALUES('$username', '$hashed_password')";
if($registerquery = true)

is setting a string and then later setting the string to true. This is always going to return true. There are two problems with this:

  • You need to execute the SQL statement that you have stored in the string for anything to happen in the database.
  • You are not really checking the return value ("=="), but rather using "=", which simply sets the variable. A very common mistake.

Additionally, you should probably no longer use the mysqli built in functions, since they will soon be deprecated. I would recommend switching to PDO before moving any further.

5 Comments

he is not actually checking it
Duly noted, and edited. The most important point remains, which is that he is not executing any SQL statements at all. Not sure that this was deserving of a downvote.
I didn't downvote. No idea who does, but my answer got downvoted, too, so I am deleting it, because there are obviously too many stupid people around.
i downvoted the answer from user Taco for having only like 5 words in his answer. about 30 seconds after that, everyone elses answer got a downvote at the same time...
Thank you. I guess that explains it :)
1

Formally, You should do something like this:

if(isset($_POST['cre'])){
  $username = $_POST['username'];
  $password = $_POST['password'];
  $mysqli = new mysqli('localhost','admin1', 'password1','obsidian' ) or die('Failed to connect to DB' . $mysqli->error );

  $hashed_password = password_hash($password,PASSWORD_DEFAULT);

  $registerquery = "INSERT INTO users (username, hash) VALUES('$username', '$hashed_password')";
  $stmt=$mysqli->prepare($registerquery);
  if($stmt->execute())
    {
        echo "<h1>Success</h1>";
        echo "<p>Your account was successfully created. Please <a href=\"index.php\">click here to login</a>.</p>";
    }
    else
    {
        echo "<h1>Error</h1>";
        echo "<p>Sorry, your registration failed. Please go back and try again.</p>";    
    }       
    $stmt->close();
 }   

Also, you could call only mysqli_query

if($mysqli->query($registerquery)){
....
}

It would be enough. The first call is better if you need to bind parameters and make several calls to the same query with different values.

Regards.-

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.