Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move Cart secure_key setter at the right place #14352

Merged
merged 2 commits into from Jul 18, 2019

Conversation

@jocel1
Copy link
Contributor

commented Jun 25, 2019

Questions Answers
Branch? develop
Description? Fix potentiel issue with setting the wrong secureKey to an existing cart
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
How to test? fill a cart while not logged, and verify cart is still ok after login

Let say we do the following:

$cart = new Cart($idCart1); // $idCart1 belongs to customer 2
$context = Context::getContext();
$context->cart = $cart;
$customer = new Customer($idCustomer);
$context->updateCustomer($idCustomer);
$cart->save();

before the fix, if PS_CART_FOLLOWING is set and $idCustomer have a not ordered cart, the $context->cart (and hence $cart) will have its secure key updated, but not its $id_customer, because $context->cart and $cart are copied by reference.
This could cause some nasty bugs, especially in modules handling "loginAsCustomer"


This change is Reviewable

@jocel1 jocel1 requested a review from PrestaShop/prestashop-core-developers as a code owner Jun 25, 2019

@matks

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Thanks @jocel1 !
I created issue #14131 to deal once and for all with all possible issues of the current Cart / Context coupling.

if (Configuration::get('PS_CART_FOLLOWING') && (empty($this->cookie->id_cart) || Cart::getNbProducts($this->cookie->id_cart) == 0) && $idCart = (int) Cart::lastNoneOrderedCart($this->customer->id)) {
$this->cart = new Cart($idCart);
$this->cart->secure_key = $customer->secure_key;
} else {

This comment has been minimized.

Copy link
@matks

matks Jun 25, 2019

Contributor

Would it make sense to add another validation layer by adding something like if ($this->cart !== null) ?

This comment has been minimized.

Copy link
@jocel1

jocel1 Jun 25, 2019

Author Contributor

I don't think so, new Cart() should always return a cart, or throw an Exception

This comment has been minimized.

Copy link
@matks

matks Jun 25, 2019

Contributor

I was targeting the else case 😄 but the github UI makes it look like I show the if

This comment has been minimized.

This comment has been minimized.

Copy link
@jocel1

jocel1 Jun 25, 2019

Author Contributor

I tried that before but Travis was failing so I try with this approach, perhaps the upcoming function calls need the secure key

@PierreRambaud
Copy link
Contributor

left a comment

Let's go!

@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Jul 16, 2019

@PierreRambaud PierreRambaud merged commit af588d9 into PrestaShop:develop Jul 18, 2019

2 checks passed

PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

Thanks @jocel1

@PierreRambaud PierreRambaud added this to the 1.7.7.0 milestone Jul 18, 2019

@PierreRambaud PierreRambaud changed the title move secure_key set at the right place Move Cart secure_key setter at the right place Jul 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.