PHP code to insert phone number and IP address into a table if not already present

Question

I am currently coding a PHP script that connects to a database and inserts a phone number and IP address if either item is not present in the table. I believe I have completed it and it is working but I want to make sure that I have used best practices in regards to security. I have read here about how to connect to MySQL and query it using a PDO and a prepared statement. I also read on Stack Overflow that had a fantastic explanation on how to do this. However, this is now a few years old.

I would like to know if my code follows current best practices and that it is secure against SQL injection.

if( isset($_POST['submit']))
{
//user data
$name = $_REQUEST['fullname'];
$numbers = $_REQUEST['number'];
$bedrooms = $_REQUEST['bedrooms'];
$date = $_REQUEST['date'];
$movingFrom = $_REQUEST['from-postcode'];
$movingTo = $_REQUEST['to-postcode'];
$typeOfJob = $_REQUEST['typeOfJob'];
$additionalInfo= $_REQUEST['message'];
$ip = $_SERVER['REMOTE_ADDR'];
$id= NULL;
//connection variables
$host='localhost';
$user='root';
$pass='';
$db='lookup';
$chset='utf8mb4';
$dns = "mysql:dbname=$db;host=$host;charset=$chset";
$options = [
PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
PDO::ATTR_EMULATE_PREPARES   => false,
];

try {
    //Try to create a new PDO object and pass the vars
    $connection = new PDO($dns, $user, $pass, $options);

    //prepare the database for data and exectue
    $stmt = $connection->prepare("SELECT id, numbers, ip FROM numbers");
    $stmt->execute();

    //Loop through table and check for numbers and ips
    //If present set var to true and break from loop
    $present = false;
    foreach ($stmt->fetchAll() as $k=>$v)
    {
        if($v['ip'] == $ip or $v['numbers'] == $numbers)
        $present = true;
        break;
    }
    //If data present I will be redirecting and informing user
    if($present)
    {
        //TODO: send to different pages
        echo "present";
    }
    //Else insert the data into the table
    else
    {
        $sql = "INSERT INTO numbers (id, numbers, ip) VALUES (NULL, '$numbers', '$ip')" ;
        $stmt = $connection->prepare($sql);

        $stmt->bindParam(':id', $id);
        $stmt->bindParam(':numbers', $numbers);
        $stmt->bindParam(':ip', $ip);
        $stmt->execute();
        echo "Woohoo";
    }
} catch (PDOException $e) {
    echo 'Caught exception: ',  $e->getMessage(), "\n";
}

There is more code in the if statement but it is irrelevant here as it is for the HTML.

mickmackusa · Accepted Answer · 2019-01-03 14:20:58Z

Obey PSR coding standards. As a starting point, use proper indentation and spacing. Here's a place to begin your research: https://www.php-fig.org/psr/psr-2/
If you haven't already, narrow how your incoming submitted data is delivered to your script. If you expect your user's data to be coming from a form with method=POST, then use $_POST instead of $_REQUEST. This will reduce the points of data entry and make your coding intention clearer to future human readers of your script.
Variable naming is important! If you have incoming data that holds a "phone number", use a variable name that will never lead to confusion. phone_number, phone, or mobile_phone, etc might be better choices than $numbers which comes from $_REQUEST["number"]. Truth be told, I don't know if the incoming data is a phone numbet, a serial number, or a chemical that makes things numb. What probably concerns me more than the term is the sudden change from singular to plural. Is this data an array of numbers? I have to assume not based on the script to follow, but then why use a plural variable name.
I wanted to recommend some investigation into the possibility of using an autoincremented primary id with two unique columns, https://stackoverflow.com/q/5416548/2943403 but there seems to be some debate about stability and suitable environments. Furthermore, I don't want to make any incorrect assumptions about your coding logic. Ultimately, I always encourage developers to seek solutions that execute the least number of calls to the database. If this script can be done by sending one INSERT query and processing the response, that would get a nod from me.
Because you are hunting for a specific id value and a specific numbers value, write those conditions directly into your SELECT query's WHERE clause. This with speed up your processing, eliminate your break requirement, and simply make your code more direct / intuitive.
Since the purpose of your SELECT query is to determine if an id or number exists, you can write your SELECT clause depending on the response quality that you intend to deliver to the end user. 1. If you don't intend to inform the user about which value already exists in the database table, use COUNT(*). 2. If you are going to explain which value already exists, SELECT id, numbers with a LIMIT of 1.
Rather than extracting the full result set then feeding it to the foreach(), I recommend fetching as you iterate. https://stackoverflow.com/a/12970800/2943403 That said, I think your code should only be processing a single row of data at most.
You could improve your variable naming in your loop. $k actually represents the "index" of the current row in the result set, so $i would be meaningful, $index would be more so, but best would be to omit the declaration entirely because you never use it in your script. $v is the current row's data. I recommend $row.
Try to avoid using boolean flags like $present. In most cases, after a bit of careful thought, you can redesign your script to break or continue or conditionally call a custom function as a means to avoid this extra declaration.
Your INSERT query is not using the prepared statement / named placeholders / binding properly. http://php.net/manual/en/pdo.prepare.php First, I would the id column is to receive a static NULL value -- you don't need a variable for this, just hardcode it directly into the query. Second, never write variable values into your prepared statement -- see my link to the pdo manual regarding the proper syntax.
Finally, never present the raw $e->getMessage() details to the public. You may wish to offer generalized feedback but don't give the specifics as a matter of good security practices.

Thank you very much for the time you took to answer my question Mick I will go back through my code and and implement your suggestions. — Paul Hashmi
– Paul Hashmi, Commented Jan 4, 2019 at 9:54

Stack Exchange Network

PHP code to insert phone number and IP address into a table if not already present

1 Answer 1

You must log in to answer this question.

Hot Network Questions

PHP code to insert phone number and IP address into a table if not already present

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions