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

Added retainer for current session token in order to not break checkToken functionality if getToken is called before. #12518

Conversation

gguridi
Copy link
Contributor

@gguridi gguridi commented Dec 29, 2016

Hello!

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the Contributing Guidelines?
  • I have checked that another pull request for this purpose does not exist.
  • I wrote some tests for this PR.

Small description of change:

Security::checkToken functionality doesn't use the regenerated session but uses the one that was set at the beginning if it existed. This will prevent that if we call getToken and the session token is regenerated, the checkToken method will fail because the session token has changed.

I've also cast to string the userToken coming from the request::getPost method as it could be null and I was receiving an E_WARNING when passing it to hash_equals.

Thanks


This change is Reviewable

Copy link
Contributor

@Jurigag Jurigag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update CHANGELOG.md please too

@gguridi gguridi force-pushed the feature/security-retaining-session-for-checking branch from 56a0b1d to 9f27773 Compare December 30, 2016 17:14
@sergeyklay sergeyklay changed the base branch from 3.1.x to 3.0.x January 3, 2017 06:06
Copy link
Contributor

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gguridi We use the 3.0.x branch for bug fixes that do not break backward compatibility. I changed the base branch to 3.0.x from 3.1.x. But now your PR has conflicts that must be resolved. So could you please rebase the PR onto 3.0.x branch?

…ate fields after regenerating the token. Added string casting to userToken.
@gguridi gguridi closed this Jan 9, 2017
@gguridi gguridi force-pushed the feature/security-retaining-session-for-checking branch from 9f27773 to 28e2c44 Compare January 9, 2017 14:16
@gguridi gguridi reopened this Jan 9, 2017
@sergeyklay
Copy link
Contributor

@gguridi Could you please rebase again? This PR has conflicts (CHANGELOG.md) after merging #12515

@sergeyklay sergeyklay added this to the 3.1.0 milestone Jan 12, 2017
@Jurigag Jurigag mentioned this pull request Jan 12, 2017
@sergeyklay sergeyklay changed the base branch from 3.0.x to 3.1.x January 12, 2017 16:06
@sergeyklay sergeyklay changed the base branch from 3.1.x to 3.0.x January 12, 2017 16:06
@sergeyklay sergeyklay self-assigned this Jan 12, 2017
@sergeyklay sergeyklay modified the milestones: 3.2.0, 3.1.0 Feb 28, 2017
@sergeyklay sergeyklay closed this Mar 22, 2017
@sergeyklay sergeyklay reopened this May 7, 2017
@sergeyklay sergeyklay changed the base branch from 3.0.x to 3.2.x May 22, 2017 06:08
@sergeyklay sergeyklay force-pushed the 3.2.x branch 2 times, most recently from e63249b to 821011d Compare June 18, 2017 18:24
@sergeyklay sergeyklay modified the milestones: 4.0.0, 3.2.0 Jun 18, 2017
@sergeyklay sergeyklay force-pushed the 3.2.x branch 2 times, most recently from 14919a9 to 13ce09f Compare October 21, 2017 15:13
@niden
Copy link
Member

niden commented Dec 9, 2018

This has been implemented with PR #13642

@niden niden closed this Dec 9, 2018
@niden niden removed this from the 4.0.0 milestone Oct 13, 2019
@niden niden added 4.0 labels Oct 13, 2019
@niden niden added bug A bug report status: low Low and removed Bug - Low labels Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug report status: low Low
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants