3
\$\begingroup\$

I am creating a social network and I want to know how secure and clean this code is. I had to update it to prepared statements because I was following a tutorial and even though it was made in 2019 it was using 15 year old code. So can someone tell me if it's good or needs improvement ? Thanks.

<?php

include("includes/header.php");

//calls class. You can specify parameters when you define your function to accept input values at run time
$message_obj = new Message($con, $userLoggedIn);

if(isset($_GET['u']))
    $user_to = $_GET['u'];

else {

    // calls function inside of class
    $user_to = $message_obj->getMostRecentUser();
    if($user_to == false)
        $user_to = 'new';
}

if($user_to != "new")
    $user_to_obj = new User($con, $user_to);

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

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

            $body = mysqli_real_escape_string($con, $_POST['message_body']);
            $date = date("Y=m-d H:i:s");
            $message_obj->sendMessage($user_to, $body, $date);

        }
    }

    $user_data_query = $con->prepare('SELECT first_name, last_name, num_likes FROM users WHERE username = ?');
        $user_data_query->bind_param("s", $userLoggedIn);
        $user_data_query->execute();
        $user_data_query_result = $user_data_query->get_result();

        while ($row = $user_data_query_result->fetch_assoc()) {

            $first_name = $row['first_name'];
            $last_name = $row['last_name'];
            $num_likes = $row['num_likes'];
        }

    $user_data_query_result->close();

    $stmt = $con->prepare("SELECT profile_pic FROM users WHERE username = ? ");

    $stmt->bind_param("s", $userLoggedIn);

    $stmt->execute();

    $stmt->bind_result($img);

    $stmt->fetch();

    $stmt->close();

?>

<style type="text/css">

.convos_column {

    background-color: #fff;
    padding: 10px;
    border: 1px solid #f2f2f2;
    border-radius: 7px;
    box-shadow: 2px 2px 1px #f2f2f2;
    z-index: -1;
    word-wrap: normal;
}

#convos {

    background-color: #fff;
    padding: 10px;
    border: 1px solid #f2f2f2;
    border-radius: 7px;
    box-shadow: 2px 2px 1px #f2f2f2;
    z-index: -1;
    word-wrap: normal;
    width: 30%;
    height: 400px;
    overflow: scroll;   
    float: left;
    left: -16%;
    top: 350px;
    position: relative;
}

#convos img {

    width: 50px;
    height: 50px;

}
#convos p {

    top: 10px;
}

.user_detailss {

    width: 20%;
    height: 310px;
    float: right;
    position: relative;
    right: 95%;
    top: 4%;
}

</style>

<head>
    <link rel="shortcut icon" type="image/png" href="favicon.ico"/>
</head>

<div class="user_detailss column">
        
    <a href="<?php echo $userLoggedIn; ?>">
        
        <img src="<?php echo $img; ?>" style="border-radius: 5px;" width="130px" height="130px">

    </a>     

    <br><br>

    <div class="user_details_left_right">
        
        <a href="<?php echo $userLoggedIn; ?>">

            <?php

                echo $first_name . " " . $last_name;

            ?>

        </a>
        
        <br>
        <br>

        <?php echo "Likes: " . $num_likes; ?>

    </div>

</div>

<div class="main_column column" id="main_column">
    
    <?php

        if($user_to != "new") {

            echo "<h4>You and <a href='$user_to'>" . $user_to_obj->getFirstAndLastName() . "</a></h4><hr><br>";

            echo "<div class='loaded_messages' id='scroll_messages'>";

            echo $message_obj->getMessages($user_to);
            echo "</div>";
        }

        else {
            echo "<h4>New Message</h4>";
        }
    ?>

    <div class="message_post">
        
        <form action="" method="POST">
            
            <?php

            if($user_to == "new") {

                echo"Select the friend you would like to message <br><br>";

                ?> 

                To: <input type='text' onkeyup='getUsers(this.value, "<?php echo $userLoggedIn; ?>")' name='q' placeholder='Name' style='padding-left: 5px;' autocomplete='off' id='search_text_input'>

                <?php

                echo "<div class='results'></div>";
            
            } else {

                echo "<textarea placeholder='Enter your message...' name='message_body' id='message_textarea'></textarea>";

                echo "<input type='submit' name='post_message' class='info' id='message_submit' value='Send Message'>";
            }

            ?>

        </form>

        <br><br><br>

    </div>

    <script type="text/javascript">
        
        var div = document.getElementById("scroll_messages");

        if(div != null) {

        div.scrollTop = div.scrollHeight;
        
        }
        
    </script>

</div>

<div class="user_convos convos_column" id="convos">
        
        <h4>Conversations</h4>
        <br>

        <div class='loaded_conversations'>


        <?php echo $message_obj->getConvos(); ?>
        
        </div>
    <a href="messages.php?u=new">New Message</a>

    <br><br>
    

</div>
\$\endgroup\$
1
  • 2
    \$\begingroup\$ "Security" is meaningless without a detailed threat model. What are you protecting? How much is it worth? Who are you protecting it against? What resources do your attackers have? How much are they willing to spend? How much are you willing to spend defending against attacks? For example, there is no point in securing your server, if an attacker can simply kidnap your girlfriend and force you to give them root access. There is no point in having a firewall if an attacker can simply bribe the cleaning lady in the server room and get physical access. xkcd.com/538 \$\endgroup\$ – Jörg W Mittag Jul 6 '20 at 5:19
3
\$\begingroup\$
  1. Users can pass whatever they wish into the u field of your form. Even the word new -- there is no defense against someone tricking your script and breaking the intended flow. You will either need to blacklist the u value new or alter your script to not rely on it (use a different flagging mechanism).

  2. When determining the value of $user_to, you can use more modern syntax. This will make the step more concise and hopefully you won't find the syntax confusing.

    $user_to = $_GET['u'] ?? $message_obj->getMostRecentUser() ?: 'new';
    
  3. When testing if multiple/all specified variables are set and not null, you can write multiple arguments into an isset() call.

    if (isset($_POST['post_message'], $_POST['message_body'])) {
    

    but you should probably put a bit more effort into validating and sanitizing the submission data before allowing it into your system.

  4. I do not like your call of mysqli_real_escape(), all of your queries should be relying on prepared statements and bound parameters. This indicates that you probably need to adjust your sendMessage() method.

  5. date("Y=m-d H:i:s") has a typo, but I don't even recommend that you fix it in php. It will be better that you simply declare that column in your db table to have a default value of the current datetime -- this way you never need to pass a value to that column when you execute an insert query.

  6. $user_data_query->get_result() produces a result set object, so you can feed it directly into a foreach() and avoid making iterated calls of $user_data_query->get_result(). You can access the associative elements in the same fashion inside the loop.

  7. I don't see where $userLoggedIn is coming from. As I work my way down the code, I must assume that this script is being run inside a password protected section of your application. I have to assume that your username values are all unique in this project -- this means there is not a lot of reason to perform a loop on the SELECT query that fetches first_name, last_name, num_likes. Furthermore, I don't see any reason to make a second trip to the same table in the database just to grab the profile_pic. Just add first_name, last_name, num_likes to the profile_pic SELECT and bind all of the column values to variables.

  8. Time to clean up the DOM producing portion. Move all of your internal stylesheet to an external stylesheet. Move all of your inline styling to the external stylesheet as well.

  9. This doesn't look good to me: getUsers(this.value, "<?php echo $userLoggedIn; ?>"). Am I correct that the end user can just manipulate your source code and fetch data by hardcoding a different value into the 2nd parameter? I think this needs a rethink.

\$\endgroup\$
11
  • \$\begingroup\$ So put this in an associative array and then access it using $rows[''] ?$body = mysqli_real_escape_string($con, $_POST['message_body']); \$\endgroup\$ – user13477176 Jul 5 '20 at 23:29
  • 1
    \$\begingroup\$ First, I do not recommend that you accept an answer in the first 24 hours on CodeReview. The person who best reviews your script may be asleep in their timezone when you post. I am perfectly fine with you removing the green tick from my answer. If no better review comes, you can always put it back. I'm recommending using bind_result() for your result set variable declarations. sendMessage() should be refactored to use a prepared statement (unless it already is -- in which case you don't need to call real_escape_string() on values fed to a prepared statement.) \$\endgroup\$ – mickmackusa Jul 5 '20 at 23:33
  • 1
    \$\begingroup\$ Yeah it's already in a prepared statement so I did this $body = $_POST['message_body']; and it works fine \$\endgroup\$ – user13477176 Jul 5 '20 at 23:44
  • \$\begingroup\$ So when do I use bind_result and get_result ? \$\endgroup\$ – user13477176 Jul 5 '20 at 23:46
  • 1
    \$\begingroup\$ Kill off the first prepared statement (that whole 13-line block of code) in this script entirely. Only use SELECT first_name, last_name, num_likes, profile_pic FROM users WHERE username = ? and assign all 4 variables using the binding technique that you used for $img. \$\endgroup\$ – mickmackusa Jul 6 '20 at 0:04
3
\$\begingroup\$

Ifs and nesting control loops

Maybe this is just me but I would always enclose all if blocks within brackets {} to avoid ambiguity and possible logic errors:

if($user_to != "new")
    $user_to_obj = new User($con, $user_to);

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

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

            $body = mysqli_real_escape_string($con, $_POST['message_body']);
            $date = date("Y=m-d H:i:s");
            $message_obj->sendMessage($user_to, $body, $date);

        }
    }

Try not to nest control loops. Here you have three levels of ifs but it is unnecessary, see below.


Expect the unexpected

Remember that any site is going to be subjected to automated (and sometimes manual) attacks. You have to assume that requests can be tainted. What I would do first in the code:

  1. verify that all expected form fields are present in the POST or GET requests
  2. validate them
  3. if one or more expected, non-optional fields are missing, then stop execution. This is more likely a malicious attempt (SQL injection, fuzzing) or a truncated request that should be considered corrupt. Your script should not continue if required fields are missing.

I cannot test your code but I have the impression that it could behave unpredictably if the form submission is manipulated. Not necessarily a security risk but you should test.

You validate the fields if they are present, which is a good thing. But if they are not, certain parts of your code are not executed. You have to be sure this is what you want and the execution flow will not yield unpleasant surprises.

So, if you rewrite your code as suggested by doing early validation, you can simplify it. The three levels of nested ifs are no longer necessary: you can get rid of if(isset($_POST['post_message'])) { and if(isset($_POST['message_body'])) { since you've checked for those earlier in your code. And the code suddenly becomes more simple and readable don't you think ?

Do not repeat stuff like $_POST['message_body'], assign all your $_POST fields to variables instead.


Formatting

Line spacing is not always consistent, sometimes too much:

$user_data_query_result->close();

$stmt = $con->prepare("SELECT profile_pic FROM users WHERE username = ? ");

$stmt->bind_param("s", $userLoggedIn);

$stmt->execute();

$stmt->bind_result($img);

$stmt->fetch();

$stmt->close();

Or here:

<div class="user_details_left_right">
    
    <a href="<?php echo $userLoggedIn; ?>">

        <?php

            echo $first_name . " " . $last_name;

        ?>

    </a>
    
    <br>
    <br>

    <?php echo "Likes: " . $num_likes; ?>

</div>

You can save a few lines = shorter code = less scrolling. Readability and good formatting are important. When the code is hard to understand, bugs or logic errors are more difficult to spot.


CSS

You have a CSS class named user_detailss but there is user_details_left_right. I am not sure if this is intended or this is a typo. I would call it user_details. I don't see the definition of user_details_left_right in your code so I guess there is a separate style sheet already. Avoid inline CSS.

Indeed, the style sheet has to be kept separate. Remember, you'll probably have more than one style sheet. Also, users like to customize appearance. Having a choice of layouts and colors is an expected feature. Some sites also have high-contrast style sheets for visually-impaired people.


Localization

Since you mentioned you are building a social network, localization is important. You are going to have users from different countries, with different languages and times zones. I would store all datetime values as UTC and render them to the user in the proper language, with the appropriate offset for their location.

This is a basic feature that is found in any forum software.


Templating

You should consider using some sort of template system for your pages. Because evolution and maintenance is going to be tedious, unless your project will be no more than a dozen pages (which I doubt). Try to separate code (logic) from layout (presentation) as much as possible.

Some users will be using a desktop computer, other will be coming from a mobile device. Either go for responsive design or be prepared to serve different templates based on the user device. The style sheet can address some of these concerns.

A very long time ago, I was using HTML pages with tags like %USER_NAME% inside, and a PHP script would load the desired page and replace the tags with appropriate values. Then came solutions like smarty etc.

\$\endgroup\$
2
  • \$\begingroup\$ A couple of notes. There are not 3 levels of ifs. There is the first, unbraced one which conditionally executes a single line of code, then there are the 2nd and 3rd ifs which can be converted to a single isset() check. I completely agree about consistently using braces. I disagree about declaring variables to store redundant $_POST data. There is no overhead saved because there is no processing done. This advice will needlessly bloat the list of variables in the scope with no performance advantage. Leaving the data in the POST array speaks clearly of the origin of the data. \$\endgroup\$ – mickmackusa Jul 6 '20 at 0:17
  • \$\begingroup\$ Thanks for your answer. I am creating a different site altogether called m.mysite.com \$\endgroup\$ – user13477176 Jul 6 '20 at 13:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.