2
\$\begingroup\$

Two weeks ago I started writing some Javascript and AngularJS code for a customer. As a proud newby I must also be aware of the dangers on the big bad Internet. In particular vulnerabilities such as XSS attacks.

I've already performed a vulnerability scan and this showed I didn't restrict session cookies to httponly. However nothing about the Javascript or AngularJS scripting.

My familiarity with JavaScript and AngularJS is somewhat limited.

var kvv = angular.module('kvv', ['ngStorage']);

kvv.controller('CartController', ["$scope", "$localStorage", "$sessionStorage", "$timeout", function($scope, $localStorage, $sessionStorage, $timeout) {

if ($localStorage.items === undefined) {
    $localStorage.items = [];
};

$scope.localStorage = $localStorage

$scope.remove = function(index) {
  $localStorage.items.splice(index, 1);
};

$scope.checked = false;

$scope.addToCart = function(index, title, desc, price, timeout) {

  $scope.checked = true;
  var found = false;
  angular.forEach($localStorage.items, function(items) {
      if (items.id  === index) {
        $timeout(function() {
          (items.quantity++);
          $scope.checked = false;
        }, 750);
        found = true;
    }
  });
  if (!found) {
    $timeout(function() {
        $localStorage.items.push(angular.extend({
        id: index,
        title: title,
        quantity: 1,
        price: price}, index))
        $scope.checked = false;
        },750);
    }
};

$scope.itemWithoutBtw = function(index) {
    var itemWithoutBtw = 0;
    angular.forEach($localStorage.items, function(items) {
        itemWithoutBtw += items.price / 106 * 100;
    })
    return itemWithoutBtw;
};

$scope.total = function(index) {
        var total = 0;
        angular.forEach($localStorage.items, function(items) {
            total += items.quantity * items.price;
        })
        return total;
};

$scope.totalBtw = function(index) {
        var totalBtw = 0;
        var total = $scope.total();
        angular.forEach($localStorage.items, function(items) {
            totalBtw = total / 106 * 6;
        })
        return totalBtw;
};

$scope.totalWithBtw = function(index) {
        var totalWithBtw = 0;
        angular.forEach($localStorage.items, function(items) {
            totalWithBtw += (items.quantity * items.price) + (items.quantity * items.price  / 100 * 6);
        })
        return totalWithBtw;
};
$scope.totalWithoutBtw = function(index) {
        var total = $scope.total();
        var totalBtw = $scope.totalBtw();
        var totalWithoutBtw = 0;
        angular.forEach($localStorage.items, function(items) {
            totalWithoutBtw = total - totalBtw;
        })
        return totalWithoutBtw;
};

$scope.orderTotal = function(index) {
        var orderTotal = 0;
        angular.forEach($localStorage.items, function(items) {
            orderTotal += items.quantity;
        })
        return orderTotal;
};

$scope.orderDelete = function(index) {
    delete $localStorage.items;
    $localStorage.items = [];
    $('html, body').animate({scrollTop: 0}, 2500);
};

}]);
\$\endgroup\$
1
\$\begingroup\$

This is interesting,

you have quite a bit business logic in your code there.. If I were try to break this, I would definitely change the price of the items I buy in the local storage to 1 cent and see if the server side would accept that price. (It should not, and recalculate any pricing from item master data). Next I would place a breakpoint in some of your btw related routines and make it return 1 cent and see if the server side could be broken that way.

Other than that, I see no security flaws in there. Let me know if you want me to do a review of other aspects of your code.

\$\endgroup\$
  • \$\begingroup\$ The business logic is only for showing a order summary on the website. The item ID, which is stored in the array, is forwarded to a PHP page which will get the price out of the database so manipulation of the array wont result in a lower price. Only when you change the item id and it matches another product another price is retrieved. On the PHP page the input is stripped and sanitized with things like regular expressions just to ensure there is no code or something in the POST array. Only numbers are expected. Thanks for the review! \$\endgroup\$ – RvdM Mar 2 '16 at 18:37
1
\$\begingroup\$

Couple of more points -

  1. Why have you given time delay of 750 to your timeout function. It might cause multiple duplicate items (same items might show up as different individual row instead of incremented count within one row) on UI if user keeps clicking on "add item" (which triggers addToCart scope function) frequently.
  2. Why are you keeping reference of localStorage at scope level? Are you using it anywhere in scope ? (referring to this - $scope.localStorage = $localStorage)
  3. You can improve time complexities of following scope methods (itemWithoutBtw, total, totalBtw etc) and avoid O(n) iteration if you modify your add / remove code diligently. If you still want to iterate on entire cart from code maintainability aspect, you can introduce one variable to read cart state, and cache these values to avoid unnecessary iteration on entire cart until cart state changes.

$scope.itemWithoutBtw = 0;
$scope.addToCart = function(index, title, desc, price, timeout) {
  // Same code as of above, removing this to save space here, Below I wrote for single quantity case. You can write similar logic if item already exists.
  if (!found) {
    $timeout(function() {
        $localStorage.items.push(angular.extend({
        id: index,
        title: title,
        quantity: 1,
        price: price}, index))
        // Below line is added here additionally..
        $scope.itemWithoutBtw += price / 106 * 100;
        $scope.checked = false;
        },750);
    }
};

// You dont need this function and can use scope var directly.
$scope.itemWithoutBtw = function(index) {
    return $scope.itemWithoutBtw;
};

  1. Supporting @konijn point - if you want to store price in localStorage, ensure that you have server side logic to override price and other sensitive cart item details to avoid hacks there. Try showing up overridden price to user as well ( by syncning with server ) when you user is about to pay. That ways you will lead to a better user experience and less dissatisfaction among users.
\$\endgroup\$
  • \$\begingroup\$ 1) The timeout is added for animation purposes. When someone adds a product to the shoppingcart an animation is played and the counter of total product should change only when the animation is finished. Thats why there is an timeout. During the animation all buttons are disabled. 2) Not sure why. I've found that in other examples and this way the localStorage functionality is working. 3) Could you show me an example. I might be better able to understand what you mean. \$\endgroup\$ – RvdM Mar 2 '16 at 18:42
  • \$\begingroup\$ 1. makes sense if you are using state of check variable to animate in UI. 3. For this, you can write something like this - \$\endgroup\$ – Soman Dubey Mar 2 '16 at 21:08
  • \$\begingroup\$ $scope.itemWithoutBtw = 0; $scope.addToCart = function(index, title, desc, price, timeout) { // Same code as of above, removing this to save space here, Below I wrote for single quantity case. You can write similar logic if item already exists. if (!found) { $timeout(function() { $localStorage.items.push(angular.extend({ ... quantity: 1, price: price}, index)) // Below 2 lines are added here additionally.. $scope.itemWithoutBtw += price / 106 * 100; $scope.checked = false; },750); } }; \$\endgroup\$ – Soman Dubey Mar 2 '16 at 21:22
  • \$\begingroup\$ // You dont need this function and can use scope var directly. $scope.itemWithoutBtw = function(index) { return $scope.itemWithoutBtw; }; \$\endgroup\$ – Soman Dubey Mar 2 '16 at 21:22
  • \$\begingroup\$ modifying answer so that it shows properly. \$\endgroup\$ – Soman Dubey Mar 2 '16 at 21:23

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.