patternphpMinor
Securing PHP shopping cart
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.
If anyone find this cart useful, feel free to use it.
session.php
db_connect.php
products.php
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',
- 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.phpand 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:
If you're into ternaries, you could always change it to:
My Ajax is terrible, so I'll have to skip that, sorry.
You call two main variables:
and instead of
Using this seems a little bit over the top:
Just keeping the next line of:
seems fine.
Throughout this script are more of these, like:
These lines:
Could be
If you initialise
In fact, why is
You just re-calculate that for each item, why not just calculate it after the
As I can't delve too deep into your structure, because there are no examples of
However, here, you initialise
should be:
There's little you can do with this other than collapsing it.
General Comments:
Your current database structures has the
You could use a structure like
I would suggest using lower
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.