HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Securing PHP shopping cart

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
phpcartshoppingsecuring

Problem

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

  • 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?



  • 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?



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



  • Are there some high security risks with my approach I should take into account?



  • 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

prepare( $query );
    $stmt->execute();

    $num = $stmt->rowCount();

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

        echo "
            

                {$id}
                shoes
                  
                 {$name} 
                 {$price}
                
                

            
            ";
    }
?>


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',

Solution

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.

Code Snippets

(session_status() != PHP_SESSION_NONE ?: session_start() );
$error = true;
if(isset($_POST['action']) ? $_POST['action'] : "") {
if ($_POST['action'] == 'add') {
$quantity = test_input($_POST["quantity"]);
if (!is_numeric($quantity))

Context

StackExchange Code Review Q#91379, answer score: 7

Revisions (0)

No revisions yet.