Skip to content

[ForwardPort 2.3-develop] Fix \Magento\Checkout\Controller\Index\Index::isSecureRequest method to take care of current request being secure and also from referer, as stated in phpdoc block #14429

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

Conversation

adrian-martinez-interactiv4
Copy link
Contributor

@adrian-martinez-interactiv4 adrian-martinez-interactiv4 commented Mar 28, 2018

Related with PR #14428

Description

Fix \Magento\Checkout\Controller\Index\Index::isSecureRequest method to take care of current request being secure and also from referer, as stated in phpdoc block.

After last try to implement a solution for session loss in checkout, this private method did behaviour as expected. Updated as agreed with @sidolov .

Fixed Issues (if relevant)

  1. Hit fast twice F5 on checout page, customer loggs out automatically #4301: Hit fast twice F5 on checout page, customer loggs out automatically
  2. Concurrent (quick reload) requests on checkout cause cart to empty - related to session_regenerate_id #12362: Concurrent (quick reload) requests on checkout cause cart to empty - related to session_regenerate_id
  3. [2.1.11] Add to cart, try to checkout, cart is empty but mini-cart has items. #13427: Add to cart, try to checkout, cart is empty but mini-cart has items.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@sidolov sidolov self-assigned this Mar 29, 2018
@sidolov
Copy link
Contributor

sidolov commented Mar 29, 2018

Hi @adrian-martinez-interactiv4 , regenerate_session_id call was added in order to prevent session fixation attacks. Atack possible when browsing between secure and insecure pages. We can’t remove the line in question as it may theoretically compromise security.

We have the story for implementation secure cookies and session transfer mechanism.

For now, I propose to leave a comment under the line with regenerate_session_id call with an explanation why it was added.

@magento-engcom-team magento-engcom-team added this to the March 2018 milestone Mar 29, 2018
@magento-engcom-team magento-engcom-team added Release Line: 2.3 Partner: Interactiv4 Pull Request is created by partner Interactiv4 partners-contribution Pull Request is created by Magento Partner labels Mar 29, 2018
@okorshenko okorshenko modified the milestones: March 2018, April 2018 Apr 16, 2018
YevSent and others added 2 commits May 8, 2018 19:08
 - Removed session regeneration for secure checkout index page
…to take care of current request being secure and also from referer, as stated in phpdoc block
@adrian-martinez-interactiv4 adrian-martinez-interactiv4 force-pushed the FR23#REMOVE-REGENERATE-SESSION-ID-CHECKOUT-CONTROLLER branch from 1508290 to 4d23b8b Compare May 8, 2018 17:10
@adrian-martinez-interactiv4 adrian-martinez-interactiv4 changed the title [ForwardPort 2.3-develop] Remove regenerate session id in \Magento\Checkout\Controller\Index\Index [ForwardPort 2.3-develop] Fix \Magento\Checkout\Controller\Index\Index::isSecureRequest method to take care of current request being secure and also from referer, as stated in phpdoc block May 8, 2018
@magento-engcom-team magento-engcom-team merged commit 4d23b8b into magento:2.3-develop May 12, 2018
magento-engcom-team pushed a commit that referenced this pull request May 12, 2018
@magento-engcom-team
Copy link
Contributor

Hi @adrian-martinez-interactiv4. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.3.0 release.

@adrian-martinez-interactiv4 adrian-martinez-interactiv4 deleted the FR23#REMOVE-REGENERATE-SESSION-ID-CHECKOUT-CONTROLLER branch May 16, 2018 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Partner: Interactiv4 Pull Request is created by partner Interactiv4 partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants