Is the code prone to SQL injection or to any other kind of threat

Posted on


I am very new to PHP coding, hence just wanted to know if my coding is prone to any SQL injection attack or any other kind of threat. Below is the code for adding client to the database:

$response = array();
include 'db/db_connect.php';
include 'functions.php';

//Get the input request parameters
$inputJSON = file_get_contents('php://input');
$input = json_decode($inputJSON, TRUE); //convert JSON into array

//Check for Mandatory parameters
if(isset($input['clientName']) && isset($input['siteName'])){
    $clientName = $input['clientName'];
  $siteName = $input['siteName'];
    //Check if client already exist
    if(!clientExists($clientName, $siteName)){
        //Query to add new material
        $insertQuery  = "INSERT INTO clients(clientName, siteName) VALUES (?,?)";
        if($stmt = $con->prepare($insertQuery)){
            $response["status"] = 0;
            $response["message"] = "Client created";
        $response["status"] = 1;
        $response["message"] = "Client exists";
    $response["status"] = 2;
    $response["message"] = "Missing mandatory parameters";
echo json_encode($response);

Code for clientExists() function is as follows:

function clientExists($clientName,$siteName){
    $query = "SELECT clientName FROM clients WHERE clientName = ? AND siteName=?";
    global $con;
    if($stmt = $con->prepare($query)){
        if($stmt->num_rows == 1){
            return true;
    return false;


SQL injection should not be a problem, as long as you pass any user input to queries through placeholders, which you do.

I have a few other remarks though

Single Responsibility Principle

In the first code snippet you do too much things. You decode request json, you talk to db. you handle the logic, you maintain response structure. It would be wise to separate those things into their own scopes (functions/classes). Of course if this is all you ever intend to have there may be no point. But in general, in future you will want to reuse the individual pieces.

Global Scope vs. Classes and Dependency Injection

You may want to avoid definind global variables. It’s generaly considered bad practise for several reason which I’m not gonna repeat here as there is plenty of information sources for this already.

You are probably not very familiar with the concept of OOP, but wrapping those function into class scopes and working with objects may be something for you to learn.

If you want to stick with procedural style, maybe at least get rid of the globals by passing the db connection always as function parameter.

if (clientExists($connection, $clientName)) {...}

It is annoying but it is still better then globals, in some ways anyway.
That where the objects could help if you get used to it.


Data Validation

You check that $input[‘clientName’] is set. Is it enough? What if it is empty? What if it is array or number? Is it ok? What if $input is not an array in the first place? All that should be handled.

Validation is btw, yet another responsibility that might be separated to its own function.

Useless Statements

In the clientExists() function I think you fetch the row for now reason. It should be able to tell count without fetching it.
You can also simplify the select from SELECT clientName to SELECT 1 which is, IMO, cleaner as you could actualy select any of the table’s column for it to work and if that column ever changes name this would have to change too.

Public API

At this point your api is public in whichever network in which it is deployed. As this network is probably going to be the internet (I suppose), anyone will be able to call it. Is it ok if any anonymous client can succesfully call this endpoint and so create db entries? You may want to have some way of authentication, but this depends on your use-case.

in addition to what has already been said, I’d suggest not using clientExists at all. just have a UNIQUE constraint on clientName, siteName and let the database enforce this for you.

the reason is that your current code is both doing more work than is needed (the check for existence) as well not enough (the gap between the existence check and inserting a new row is a race condition). generally the best place to handle this sort of check is the database

see e.g. for how to handle this with MySQL. I’ve not programmed PHP for a few years so don’t know if there are any nice ways to handle this that generalise to other database engines.

Leave a Reply

Your email address will not be published. Required fields are marked *