7
\$\begingroup\$

I've managed to create a working shopping cart and my next concern is security, mostly about the architecture and session security.

  1. Should I make sessions somehow secure, if there's no authenticated login and sessions are deleted when browser closes? or is session_start() enough in this case?
  2. Would the server side validation be enough strong in the add_to_cart.php and is that proper way to exit PHP code in case of errors?
  3. Are the database queries safe or should I take some extra measures?
  4. Are there some high security risks with my approach I should take into account?
  5. Cart will be hosted on SSL-secured server. Do I need to specify something in the code, to only make it use SSL?

If anyone find this cart useful, feel free to use it.

session.php

// Check if session is created. If not, then create.
if (session_status() == PHP_SESSION_NONE) {
    session_start();
}

db_connect.php

$host = "localhost";
$db_name = "xx";
$username = "xx";
$password = "xx";

try {
    $con = new PDO("mysql:host={$host};dbname={$db_name}", $username, $password);
    $con->exec("set names utf8");
}

//to handle connection error
catch(PDOException $exception){
    echo "Connection error: " . $exception->getMessage();
}

products.php

<?php
    $query = "SELECT id, name, price, image FROM shoes ORDER BY id";
    $stmt = $con->prepare( $query );
    $stmt->execute();

    $num = $stmt->rowCount();

    while ($row = $stmt->fetch(PDO::FETCH_ASSOC)){
        extract($row);

        echo "
            <div class=\"item\">

                <div class=\"product-id\">{$id}</div>
                <div class=\"category\">shoes</div>
                <div class=\"image\"> <img src=\"images/{$image}\" class=\"product-image\" alt=\"product\"/> </div>
                <div class=\"name\"> {$name} </div>
                <div class=\"price\"> {$price}</div>
                <div class=\"quantity\"><input type=\"text\" value=\"1\" class=\"maara\" /></div>
                <input type=\"button\" class=\"lisaa\" value=\"Lisää\"/>

            </div>
            ";
    }
?>

ajax.js

function add() {

$(".lisaa").click(function(e) {
    var id = $(this).closest(".item").find(".product-id").text();
    var category = $(this).closest(".item").find('.category').text();
    var quantity = $(this).closest(".item").find('.maara').val();
    var action = "add";

    $.ajax({
        url: 'add_to_cart.php',
        type: 'POST',
        data: {'id': id, 'category': category, 'quantity': quantity, 'action': action},

            beforeSend: function() {
                $("#wait").show();
            },

            complete: function() {
                $("#wait").hide();
            },

            success: function(data) {

                    if(data.indexOf("error") >= 0) {
                        $(".success, .errors").hide();
                        $(".errors").html(data).slideDown();
                    }

                    else {
                        $(".shoppingcart-container").load("cart.php");
                        $(".errors, .success").hide();
                        $(".success").html(data).slideDown();
                    }
                },

            error: function(request, status, error) {
                    alert("error!");
                }
    });
});
}

add_to_cart.php

// Check server request method
if ($_SERVER["REQUEST_METHOD"] == "POST") {

// Check if action is set
if(isset($_POST['action']) ? $_POST['action'] : "") {

    // Check if action is "add"
    if($_POST['action'] == "add") {

        // Error variable.
        $error = "";

        // Success variable.
        $success = "";

        // VALIDATE ID
        if (isset($_POST['id']) ? $_POST['id'] : "") {

            // clean input
            $id = test_input($_POST["id"]);

            // Check if id is numerical
            if (!is_numeric($id)) {

                // Show invalid ID as a return data
                echo "error: Invalid ID. Not numerical.";

                // Add a value to error variable
                $error = "Error";

                // Exit php
                exit;
            }
        }

        // If id doesn't exist
        else {  

            // Show invalid ID as a return data
            echo "error: Invalid ID. Empty id.";

            // Add a value to error variable
            $error = "Error";

            // Exit php 
            exit;
        }


        // VALIDATE Category
        if (isset($_POST['category']) ? $_POST['category'] : "") {

            // clean input
            $category = test_input($_POST["category"]);

            // Category must match your product categories
            if(!preg_match('[shoes|shirts]', $category)) {

                    // Show invalid category as a return data
                    echo "error: invalid category.";

                    // Add a value to error variable        
                    $error = "Error";

                    // Exit php
                    exit;
            }
        }


        // If category doesn't exist
        else {

            // Show invalid category as a return data
            echo "error: Invalid category.";

            // Add a value to error variable    
            $error = "Error";

            // Exit php 
            exit;
        }

        // VALIDATE Quantity
        if (isset($_POST['quantity']) ? $_POST['quantity'] : "") {

            // clean input
            $quantity = test_input($_POST["quantity"]);

                // Check if quantity is numerical
                if (!is_numeric($quantity)) {

                    // Show invalid category as a return data
                    echo "error: Invalid quantity format.";

                    // Add a value to error variable
                    $error = "Error";

                    // Exit php
                    exit;
                }   
        }

        // Check if errors are false 
        if ($error == false) {

            // Connect to database and select row from table, which matches category variable
            $query = "SELECT id, name, price, image FROM {$category} WHERE id={$id}";
            $stmt = $con->prepare( $query );
            $stmt->execute();

        }

        else {

            // Show error as return data    
            echo "error: errors occurred with db.";

            // Add a value to error variable    
            $error = "Error";

            // Exit php     
            exit;
        }

        // Check if query contains a row
        if($stmt->rowCount() <= 0) {

            // Add a value to error variable
            $error = "Error";

            // exit php
            exit;
        }

        // Get values of the item, which matched our database search
        while ($row = $stmt->fetch(PDO::FETCH_ASSOC)){
            $name = $row['name'];
            $price = $row['price'];
            $image = $row['image'];
        }

        // Check if session variable "cart" exists. If not, then create.
        if (!isset($_SESSION['cart']) || empty($_SESSION['cart']))
        {
            $_SESSION['cart'] = array();
        }

        // Check if array is set
        if(isset($_SESSION['cart']['id'])) {

        // If array is set, check if our product id exists in cart already
            if(in_array($id, $_SESSION['cart']['id'])) {

                foreach($_SESSION['cart']['id'] as $key => $val)
                {       
                        if ($val == $id ) {

                            // Update product quantity
                            $_SESSION['cart']['quantity'][$key] = $quantity + $_SESSION['cart']['quantity'][$key];


                            // Show succesfull quantity update message as return data
                            echo "{$name} x {$quantity} quantity added";

                            // Add a value to success variable
                            $success = "{$name} x {$quantity} quantity added";

                            // Exit php
                            exit;
                        }
                }   
            }
        }


        // If product doesn't exist in cart and errors are false, add new item to cart
         if ($error == false) {
            $_SESSION['cart']['id'][] = $id;
            $_SESSION['cart']['category'][] = $category;
            $_SESSION['cart']['name'][] = $name;
            $_SESSION['cart']['price'][] = $price;
            $_SESSION['cart']['quantity'][] = $quantity;
            $_SESSION['cart']['image'][] = $image;

            // Show succesfully added message as return data
            echo "{$name} x {$quantity} succesfully added";

            // Add a value to success variable
            $success = "{$name} x {$quantity} succesfully added";

            // exit php
            exit;

        }
    }
}
}

cart.php

function showcart() {

    // If cart variable is not empty, then do the following
    if(!empty($_SESSION['cart'])) {

        // Few variables, to collect total amount of items and total price
        $total = "";
        $counter = "";

        // Start shoppingcart div
        echo "<div class=\"shoppingcart\">";

        // Loop through cart items
        foreach ($_SESSION['cart']['id'] as $key => $value) {

            // Add product's price into variable
            $singleproduct = $_SESSION['cart']['price'][$key];

            // Add product's quantity into variable
            $quantityproduct = $_SESSION['cart']['quantity'][$key];

            // Replace , with . to make calculations
            $singleformat = str_replace(',' , '.' , $singleproduct);

            // Count product's amount x quantity
            $multipleproducts = $singleformat * $quantityproduct;

            // Change number formatting
            $multipleformat = number_format($multipleproducts, 2, ","," ");

            // Create html output, which contains the product information
            echo "<div class=\"shoppingcart-items\">";
                echo("<div class=\"shoppingcart-image\"><img src=\"images/{$_SESSION['cart']['image'][$key]}\" class=\"shoppingcart-image\"/></div>");                          
                echo("<div class=\"shoppingcart-itemname\">{$_SESSION['cart']['name'][$key]}</div>");
                echo("<div class=\"shoppingcart-quantity\"> {$_SESSION['cart']['quantity'][$key]} x </div>");
                echo("<div class=\"shoppingcart-price\"> {$multipleformat} €<br /> <span class=\"singleproduct-price\"> ({$singleproduct} / kpl)</span></div>");


                // Calculate total price of products
                $total += $singleformat * $quantityproduct;

                // Calculate total items amount
                $counter += $quantityproduct;

                // Change total price number format
                $totalsum = number_format($total, 2, ","," ");


            echo "</div>";

        // End foreach loop
        }

        // End shopping cart div        
        echo "</div>";

        // Create bottom for shopping cart, which contains total amount of items and total price
        echo "<div class=\"shoppingcart-bottom\">
                    <div class=\"summa\"><a href=\"lomake.php\">Kori</a></div>
                    <div class=\"tuotteiden-maara\">{$counter} tuotetta <br />{$totalsum}  €</div>
                </div>";    
    }

    // if cart variable is empty, then show the following
    else {
        echo "<div class=\"shoppingcart\">";
            echo "ostoskori on tyhjä";
        echo "</div>";
    }

}

test_input function

function test_input($data)
{
   $data = trim($data);
   $data = stripslashes($data);
   $data = htmlspecialchars($data);
   return $data;
}
\$\endgroup\$
  • \$\begingroup\$ @A.L: The <kbd></kbd> formatting is only for resembling keyboard keys, not for emphasis in general. \$\endgroup\$ – Jamal May 21 '15 at 17:42
5
\$\begingroup\$

Security

Questions

Should I make sessions somehow secure, if there's no authenticated login and sessions are deleted when browser closes? or is session_start() enough in this case?

The default should be fine. Most of session security is about server configuration, and for a shopping card you probably don't need stuff like regenerating the session id regularly, binding it to ip and/or user agent, etc.

Would the server side validation be enough strong in the add_to_cart.php and is that proper way to exit PHP code in case of errors?

No and no (see below).

Are the database queries safe or should I take some extra measures?

No, use prepared statements (see below).

Cart will be hosted on ssl-secured server. Do i need to specify something in the code, to only make it use ssl?

This is also mainly a server configuration thing, but you can try to enforce HTTPS via PHP code.

Vulnerabilities

Your code is vulnerable to SQL injection via the $category variable which is user supplied ($_POST['category']) and then put into the query. Basically what you have is this:

$query = "SELECT id, name, price, image FROM $_POST['category'] WHERE id={$id}";

Your code is also vulnerable to XSS by anyone who can add products. I will not go into that, because I'm assuming it's intended, but if I where you, I would still defend against it (maybe in the future you allow third parties to add products; maybe you don't want your sales-person be able to escalate privileges to admin; etc).

Your code is also vulnerable to CSRF, but for a shopping card that's not really that bad. It makes it possible for an attacker to add items to the card of a victim if the victim visits some website that contains HTML and JavaScript code by the attacker. This could lead to the victim accidentally buying something they did not want to buy if they don't check their card during checkout, and might annoy you or your users. But there is no real profit for the attacker (except annoying people), and it's a difficult attack (the timing must be right and the victim must not thoroughly check their card), so it's not a real danger.

Attack

An attack like this should work:

localhost/addtocart.php?action=add&quantity=1&id=1&category=shoes where id=-1 union all select user,password,3,4 from mysql.users %23

Your defense

You apply two checks to the category: !preg_match('[shoes|shirts]', $category) and test_input (which is stripslashes + htmlspecialchars). The first one is insufficient (eg shoesFooBar passes), and the second one doesn't have anything to do with SQL injection (htmlspecialchars defends against XSS, stripslashes doesn't do anything useful).

The correct defense

First of all, what you want to do is use prepared statements for all variable data. It doesn't matter where it comes from; if it's not hardcoded, use prepared statements (if it comes from the database it might have been user supplied in the past, which would open you up to second order injection).

But what about things where you can't use prepared statements? For example your $category. Here you want to use whitelists:

$whitelistTableNames = array("shoes", "shirts");

if (in_array($_POST['category'], $whitelistTableNames, TRUE)) {
    $query = "SELECT id, name, price, image FROM $_POST['category'] WHERE id=?";
    // prepare and execute
}

I put the TRUE in there for strict checking (===), just in case someone adds a 0 to the whitelist.

Alternatively, you can just hardcode it:

if ($_POST['category'] === "shoes") {
    $tableName = "shoes";
} else if ($_POST['category'] === "shirts") {
    $tableName = "shoes";
} else {
    throw new Exception('invalid category');
}
$query = "SELECT id, name, price, image FROM $tableName WHERE id=?";

Defense in general

Looking at your code, it seems that you are not really sure what you are defending against, and are just using a couple of functions in the hope that it works. This is not the correct approach. I think it would benefit you greatly if you just tried out the most common vulnerabilities (eg XSS and SQL injection) yourself, so you know how they work, and what can defend against them.

Regarding the various functions you use:

  • htmlspecialchars: this is the proper defense against XSS in most situations (please note this list of places where it does not defend against XSS). It should not be applied when inserting something in the database, but when echoing anything non-hardcoded to the user. This makes sense: You should defend at that moment when it could be exploited, not at any other moment, because it would be hard to maintain (You would constantly have to check if you cleaned a variable already or not), and can be vulnerable (maybe you have another method to add data to the database that doesn't clean values).
  • stripslashes: Does what it says: It removes slashes that are used to escape stuff. This function was useful when magic quotes was still used, which it mostly isn't anymore, so the function doesn't have all that much purpose. Definitely never use it for any security, as it doesn't provide any.
  • is_numeric: This is what in your code protects most user input from becoming an SQL injection. It's secure, but it's not the recommended way to handle SQL injection (again, use prepared statements, one reason is the same as for XSS: you want to defend directly where the vulnerability is, not earlier). If you know that you need a numeric value, you might as well use filter_input with an int filter instead of applying is_numeric at some point in-between getting input and supplying it to a query. This should not be your main defense against SQL injection, but is a nice addition as defense in depth.

Misc

  • You can use single as well as double quotes to define a string, make use of this. If you only have double quotes inside a string, terminate it with single quotes to avoid escaping all the double quotes.
  • use guard-clauses to reduce the nesting of your code (eg if ($_SERVER["REQUEST_METHOD"] !== "POST" || empty($_POST['action'])) return;).
  • your add_to_card.php code is just one long block, which makes it hard to read and maintain. Try to introduce functions which separate logical units of code.
  • use less newlines. Currently, nearly all your statements get their own paragraph, which is just too much; it makes your code harder to read.
  • use less comments. Don't rephrase what your code already told a reader (eg Exit php, clean input, etc), and don't add comments because your code looks confusing (eg If id doesn't exist because the if was opened so far away; if you need comments like these, reduce the length of your code).
  • just exiting on error makes your code hard to reuse, because the calling code can't control it. Try to return, throw exceptions, or similar instead.
  • Why have $error if you are exiting on error anyways? This seems unnecessary.
\$\endgroup\$
  • \$\begingroup\$ This answer is by far the most avesome thing i've seen. I'll make use of this tomorrow and change he coding in the way you suggested. You're right, i'm not quite sure of what i'm defending against. I've read hundreads of post regarding security, but since ever person seems to give a little bit different answer, i've been confused on what approach should i use. Your answer looks really pro'is and i'll certainly listen to your recommendations. Thank you! \$\endgroup\$ – Griphon May 21 '15 at 18:57
  • \$\begingroup\$ I tried a bit different approach and fixed the sql "category" security issue. Also, i reduced the amount of comments, newlines and tried to tighten the code a bit. Would you think this approach is better and safer than my earlier? \$\endgroup\$ – Griphon May 22 '15 at 10:51
  • \$\begingroup\$ @Griphon Yes, now it looks secure against SQL injection, but there is still room for improvement. Regarding Security, you're still not using prepared statements for the id parameter (it's not strictly necessary, but it really does lead to more secure code because you don't have to remember which variables are safe and which aren't, which gets close to impossible the more code you write), and you still have your test_input function which doesn't make that much sense. \$\endgroup\$ – tim May 22 '15 at 13:54
  • \$\begingroup\$ Regarding readability, there are a couple of things, most of which I already covered in my answer. Additionally, I would use $id = filter_input(INPUT_POST, 'id', FILTER_SANITIZE_NUMBER_INT); if (empty($id)) { throw new Exception('invalid id'); } instead of your filter_var approach. Please also note What you may and may not do after receiving answers \$\endgroup\$ – tim May 22 '15 at 13:55
  • \$\begingroup\$ Thank you, again. I'll alter my code according to your suggestions. About the readability, i did some research on guard-clauses and i did try to produce code, which takes advantage of it, but i was unsuccesfull. I need to study that further. Also, thanks for pointing out the guidelines for the questions. I was wondering, if i should post my revised code or not, but now i know better. You've been very helpful and helped me a lot to understand security better. Thank you! :) \$\endgroup\$ – Griphon May 22 '15 at 15:06
7
\$\begingroup\$

In chronological order:

session.php:

If you're into ternaries, you could always change it to:

(session_status() != PHP_SESSION_NONE ?: session_start() );

My Ajax is terrible, so I'll have to skip that, sorry.

add_to_cart.php:

You call two main variables: $error & $success. For $error, instead of storing strings like 'Error', use booleans.

$error = true;

and instead of if ($error == false) (which would still work), you can use if (!($error)).

$success on the other hand, seems to me like a misnomer, success as a variable should be typically result in true or false, rather than returning a string. It ought to be named $result or something similar.

Using this seems a little bit over the top:

if(isset($_POST['action']) ? $_POST['action'] : "") {

Just keeping the next line of:

if ($_POST['action'] == 'add') {

seems fine.
Throughout this script are more of these, like: if (isset($_POST['quantity']) ? $_POST['quantity'] : "") which should be replaced.

These lines:

$quantity = test_input($_POST["quantity"]);
if (!is_numeric($quantity)) 

Could be if (!is_numeric(test_input($_POST["quantity"]))) if you want to simplify. The same applies to the category check.

cart.php:

If you initialise $total and $counter like strings, and then use a += operator? Strings should use .= or just initialise them as integers like $total = 0; instead.

$total = "";
$counter = "";

In fact, why is $totalsum inside the foreach loop?
You just re-calculate that for each item, why not just calculate it after the foreach loop?

As I can't delve too deep into your structure, because there are no examples of $_SESSION['cart'], I can't make too many comments here.
However, here, you initialise $value but then never use it anywhere.

foreach ($_SESSION['cart']['id'] as $key => $value) {

should be:

foreach ($_SESSION['cart']['id'] as $key) {

test_input function:

There's little you can do with this other than collapsing it.

function test_input($data) {
   return htmlspecialchars(stripslashes(trim($data)));
}

General Comments:

Your current database structures has the id, price, name and image columns, you could get rid of image which I'm assuming holds the filename for the image.
You could use a structure like 500-shoes-{$id}.png or (image resolution)-(category)-(id).png instead and just use the id and/or category as an identifier.

I would suggest using lower camelCase for two-worded strings.
$quantityproduct -> $quantityProduct.

\$\endgroup\$
  • \$\begingroup\$ My humble thanks for the answer! add_to_cart.php -> I will change my code to use booleans. However, about the "success" variable. I'm echoing that result back to user. Ie. "succesfully added". Now if the result would only be "true" or "false", then the data would only be "true" or "false". \$\endgroup\$ – Griphon May 21 '15 at 10:44
  • \$\begingroup\$ cart.php -> i need to take closer look on my code and change it as you suggested. also on "general comments". Is my approach bad and shouldn't be used or is your approach alternative way to do it? Thanks for all the help. This really helps me develop this cart further! \$\endgroup\$ – Griphon May 21 '15 at 10:47
  • \$\begingroup\$ And lastly about the $_SESSION['cart'] structure. The code above has everything, which i have regarding about it. I'm not sure, if i did understand :D Please walk me through :D \$\endgroup\$ – Griphon May 21 '15 at 10:50
  • \$\begingroup\$ Ok, that cleared it out. Thank you for your help! i'll get back to make changes to code ; ) \$\endgroup\$ – Griphon May 21 '15 at 11:01
  • 2
    \$\begingroup\$ Please don't suggest 'clever' oneliners line that ternary at the beginning. The original if was readable, the ternary is unclear. \$\endgroup\$ – dotancohen May 21 '15 at 14:21

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.