2
\$\begingroup\$

I have this code to display a counter on the side of <i class="fas fa-bell mr-3"></i>. I want to know if this code is good on security and perfomance.

I just started using jquery and ajax, i had heard people saying that someone could disable the javascript and do bad things. What you guys think about my code?

 <div>
    <ul class="navbar-nav textoPerfilDesk dropMenuHoverColor">
        <li class="nav-item dropdown pr-2 dropleft navbarItem ">
        <a class="nav-link dropdown-toggle-fk" href="#" id="navbarDropdownMenuLink" role="button" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
            <i class="fas fa-bell mr-3"></i>
        </a>
        <div class="dropdown-menu dropdown-menu-fk py-3" aria-labelledby="navbarDropdownMenuLink">
            <a class="dropdown-item dropMNitemNT" href="um-link">
            <span class="d-flex">
                <img class="imgNT" src="img/1.jpg">
                <span class="pl-2 pt-1">
                    titutlo
                </span>
            </span>
            </a>
        </div>
        </li>
    </ul>
    <span class="text-white divCountNT" id="datacount"></span>
  </div>

script:

<script>
  $(document).ready(function(){
     var intervalo, carregaDiv;
     (carregaDiv = function(){
        $("#datacount").load('select.php', function(){
           intervalo = setTimeout(carregaDiv, 1000);
        });
     })();
     $('.fa-bell').on('click', function (){
        clearTimeout(intervalo);
        $.ajax({
           url: "update.php",
           complete: function(){
             setTimeout(carregaDiv, 1000);
           }
        });
     });
  });
</script>

select.php

<?php
require_once 'db.php';

if(!isset($_SESSION))session_start();

if(isset($_SESSION['userid'])) {
  $userid = $_SESSION['userid'];
}

    $status = 'unread';

    $sql = $conn->prepare("SELECT * FROM noti WHERE status = :status AND 
    userid = :userid");
    $sql->bindParam(':userid', $userid, PDO::PARAM_INT);
    $sql->bindParam(':status', $status, PDO::PARAM_STR);
    $sql->execute();
    $countNT = $sql->rowCount();

    echo $countNT;

    $conn = null;

    ?>

update.php

<?php
require_once 'db.php';

if(!isset($_SESSION))session_start();

if(isset($_SESSION['userid'])) {
  $userid = $_SESSION['userid'];
}

$status = 'read';
$sql = $conn->prepare("UPDATE noti SET status = :status WHERE userid = :userid");
$sql->bindParam(':user_id', $userid, PDO::PARAM_INT);
$sql->bindParam(':status', $status, PDO::PARAM_STR);
$sql->execute();
$countNT = $sql->rowCount();

echo $countNT;

$conn = null;
?>
\$\endgroup\$
  • \$\begingroup\$ (I'd be more inclined to delve into the code if comments, the introduction and, to the extent feasible, the title of this post told who or what is notified how about what.) \$\endgroup\$ – greybeard Dec 1 '18 at 15:24
2
\$\begingroup\$

JAVASCRIPT SECURITY

Javascript is running on the client, and is therefore under full control of the user. It can be disabled, inspected, manipulated, and everything else that can done in a programming language. You knew this, didn't you?

Javascript is, almost by definition, insecure. Things that have to do with the security of your site, like validating passwords, should not be done in Javascript. And in your code you don't do anything security related in Javascript. All you do is set a timer running and call two PHP scripts. No risks there.


PHP SECURITY

The PHP scripts are another matter. Here is where things really happen, and you should implement your security measures here. Even though these scripts implement AJAX calls, they can be executed by anybody.

You seem to have users, that can log in. Their user ID is stored in $_SESSION['userid']. I notice that you don't do anything, in your PHP scripts, when this ID is absent. You still execute the database queries. That is a bad idea.

When the two current PHP scripts are called, without an user ID, they will probably just perform database queries that are invalid. No real harm done. But you shouldn't rely on just pure luck. Good security should leave no doubts about what will happen.

I therefore propose I slight change to your code. Instead of writing this:

if (isset($_SESSION['userid'])) {
  $userid = $_SESSION['userid'];
}

you could write this:

if (!isset($_SESSION['userid'])) die('Not logged in.');
$userid = $_SESSION['userid'];

this means that the PHP scripts will halt execution when there's no user, as they should.


PERFORMANCE

You code is evidently not very efficient. Polling the database every second does not scale very well. There are other ways to do this. For instance with web sockets: https://developer.mozilla.org/en-US/docs/Web/API/Websockets_API ( you would use a combination of the tools mentioned there). Updates will be quicker, without polling.

For now polling will probably be fine for you, after all you're still learning Jquery and that is a challenge in itself. It takes time to understand how everything hangs together.

\$\endgroup\$
  • \$\begingroup\$ Lets talk about this paragraph PHP SECURITY. I use echo to print out on the screen the HTML code for this notification, and it's all inside a if (!empty($user_id)) { example: pastebin.com/wmty3PKd is it enough? This way i will not execute the queries if the user is not logged in. \$\endgroup\$ – boss Dec 4 '18 at 17:23
  • \$\begingroup\$ And i changed intervalo = setTimeout(carregaDiv, 1000); to intervalo = setTimeout(carregaDiv, 60000); \$\endgroup\$ – boss Dec 4 '18 at 17:25
  • 1
    \$\begingroup\$ Yes, as long as you check that there is a valid user, before you do user-related things, it should be fine. Raising the interval of the timer to 60 seconds will certainly help, but the principle won't change. I also noted that you 'chain' your timers, instead of having one timer created with setInterval(). Your counter will stop whenever a single connection problem is encountered. In other words: It's not robust. \$\endgroup\$ – KIKO Software Dec 5 '18 at 8:34

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.