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

Unused 'VALIDATOR_SESSION_EXPIRE_TIMESTAMP' #2826

Closed
pquerner opened this issue Dec 19, 2022 · 8 comments · Fixed by #2916
Closed

Unused 'VALIDATOR_SESSION_EXPIRE_TIMESTAMP' #2826

pquerner opened this issue Dec 19, 2022 · 8 comments · Fixed by #2916
Assignees

Comments

@pquerner
Copy link
Contributor

Preconditions (*)

  1. Using OpenMage Version 19.4.8

Steps to reproduce (*)

  1. Guest Checkout > Wants to Create a account with order
  2. $passwordCreatedTime = $this->_checkoutSession->getSessionValidatorData()['session_expire_timestamp'] does not exist in array
  3. I saw that the constant VALIDATOR_SESSION_EXPIRE_TIMESTAMP wasnt used. In the original core I saw this piece of code: $parts[self::VALIDATOR_SESSION_EXPIRE_TIMESTAMP] = time() + $this->getCookie()->getLifetime(); in \Mage_Core_Model_Session_Abstract_Varien::getValidatorData

Expected result (*)

  1. No php error notice happens, checkout proceeds successfully

Actual result (*)

  1. PHP error notice happens, breaks checkout (if notices are configured to be bad)

image

@sreichel
Copy link
Contributor

sreichel commented Jan 5, 2023

I saw that the constant VALIDATOR_SESSION_EXPIRE_TIMESTAMP wasnt used. In the original core I saw this piece of code:

Mhh, this code has been removed with #546 and was added to https://github.com/OpenMage/magento-lts/releases/tag/v19.4.17.

Can you please add some information?

@pquerner
Copy link
Contributor Author

pquerner commented Jan 5, 2023

that constant isnt unused, its used (not by the constant sadly) here:

$passwordCreatedTime = $this->_checkoutSession->getSessionValidatorData()['session_expire_timestamp']

(original core
https://github.com/OpenMage/magento-mirror/blob/98da842ef7aa59b8b8fee642de8b0b6deec55c31/app/code/core/Mage/Checkout/Model/Type/Onepage.php#L732
)

but since 'session_expire_timestamp' (or rather 'VALIDATOR_SESSION_EXPIRE_TIMESTAMP') isnt set anymore it results in a php warning. (accessing inaccessible array key)

previous core:
https://github.com/OpenMage/magento-mirror/blob/98da842ef7aa59b8b8fee642de8b0b6deec55c31/app/code/core/Mage/Core/Model/Session/Abstract/Varien.php#L525

openmage:

so I guess the issue title is wrong

@sreichel
Copy link
Contributor

sreichel commented Jan 5, 2023

so I guess the issue title is wrong

Thanks for making it more clear :)

@pquerner
Copy link
Contributor Author

pquerner commented Jan 6, 2023

Do you have a better title? :D

@sreichel
Copy link
Contributor

sreichel commented Jan 7, 2023

No.

Btw .. assigned @colinmollenhour b/c he should know best about this changes. Hoping for review.

@colinmollenhour
Copy link
Member

Ahh yes, this should have been removed. Any reason this can't just be like so?

$customer->setPasswordCreatedAt(time());

Is it trying to back-date the timestamp to when the session was started initially? It seems this wouldn't be accurate and I fail to see the value in it..

@pquerner
Copy link
Contributor Author

pquerner commented Jan 9, 2023

I've no idea whats the reason behind that chosen "creationDate". Probably CURRENT_TIMESTAMP will suffice.

colinmollenhour added a commit to colinmollenhour/magento-lts that referenced this issue Jan 9, 2023
colinmollenhour added a commit to colinmollenhour/magento-lts that referenced this issue Jan 9, 2023
colinmollenhour added a commit to colinmollenhour/magento-lts that referenced this issue Jan 9, 2023
@colinmollenhour
Copy link
Member

After investigating further it seems this is meant to invalidate other sessions whenever a customer changes their password. I pushed PR #2916 for further discussion/review.

colinmollenhour added a commit to colinmollenhour/magento-lts that referenced this issue Apr 4, 2023
commit 3c433b1
Merge: cccf1a6 35195f0
Author: Ng Kiat Siong <[email protected]>
Date:   Mon Feb 27 15:52:17 2023 +0800

    Merge branch '1.9.4.x' into unused-validator-session-expire-timestamp-2826

commit cccf1a6
Author: Colin Mollenhour <[email protected]>
Date:   Mon Jan 9 15:44:18 2023 -0500

    Fix session renew timestamp should be updated when customer changes password. Fixes OpenMage#2826
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants