Avoid session fixation by regenerating session id on user promotion

[acinader]

Fixes: #413

[codecov[bot]]

Codecov Report

Merging #414 into master will increase coverage by 0.01%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
+ Coverage   98.97%   98.98%   +0.01%     
==========================================
  Files          38       38              
  Lines        3117     3152      +35     
==========================================
+ Hits         3085     3120      +35     
  Misses         32       32

Impacted Files	Coverage Δ
src/Parse/ParseUser.php	98.32% <100%> (+0.02%)	:arrow_up:
src/Parse/ParseObject.php	97.85% <0%> (-0.01%)	:arrow_down:
src/Parse/ParseQuery.php	99.47% <0%> (+0.05%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1b99d91...4bdd04e. Read the comment docs.

[kkopachev]

I'm judging from very far away. I'm not quite sure how SDK works and what is it's area of it interest are.

Not sure it should be a concern of Parse SDK. You are responsible for starting session and you call login in your endpoint handler. Feels like fixing it here is like adding unexpected side effect. It's kinda good it's doing it, but still feels weird as it is sneaky.

[flovilmart]

@kkopachev it’s worth discussing pros and cons of implementing it here. What would you suggest?

[acinader]

Since I put @kkopachev up to posting his thoughts here (after we discussed it together at the office). I'll take a stab at presenting both sides:

@kkopachev's concern is that the user of the parse PHP SDK decides how sessions will be implemented and even if session_start() is called at all. Given that, making the parse PHP SDK 'change' the session_id is presumptuous and there may be some situations where it is inappropriate. A better solution is for the user of the SDK to be responsible for the implementation detail of what the session_id is and when to change it.

@kkopachev or anyone else, please do add any additional thoughts, or correct me if I am missing something.

I'm sympathetic but unconvinced. The parse PHP SDK implements role-based security and affects changes between roles (specifically between no user, anonymous user, and registered user.). In the event that a role changes, the session_id must also change to avoid the session fixation exploit. ParseUser is a very logical and convenient place to implement the session change logic. Changing the session_id will only occur when the SDK changes the currentUser.

With this fix, there is a single place where the session_id is changed. If it is up to the client, there is likely at least three places where the session regeneration would need to occur: after ParseUser::loginWithAnonymous();, $user->login, and $user->signUp().

[flovilmart]

I’m not against not putting the fix in the SDK, but providing documentation on mitigating the issue with code samples.

[montymxb]

Took a look at this for a bit, and it is something that we should remedy in one form or another. This approach looks to be a fairly decent way to go about it. The only concern is whether changing the session id within the sdk could inadvertently lead to issues in existing implementations. This is something I want to look into a bit more. I do want to make the use of this sdk secure, but I also want it continue working as expected if someone pulls down a patch.

With that being said I don't currently suspect (to my knowledge) that there's anything critical, but I would like to hold this for a bit until I do some more research.

I agree with @flovilmart on the need to properly annotate this not only in the code (and notes on the samples in the README) but also in the doc. Per usual, changing behavior (particularly with sessions) will need to be on the doc, code and release notes.

Beyond that this all looks pretty good, thanks for the catch @acinader ! Always good to see people keeping an eye on things.

p.s Apologies for me being a ghost 👻 . I've been caught up with tasks for some time now, but I'm still around and I'll drop by on some more PRs and issues in the future.

[montymxb]

Alright, so I did some research on this, and everything seems fairly ok. The one predominant issue I've come across more than once is potential session volatility when changing over sessions ids.

The base issue being that there can be miscommunication during the time where the server submits a new id, the client receiving the id, setting and then utilizing this new id for their cookie value in subsequent requests. This can be in the form of a hand off issue, such as when jumping from tower to tower on a cell network. Most cases I've read up on seem to center on some sort of a network transfer or other form of network volatility to start with, with just about everything else being fine.

With this being said I would agree that adding this in will be a step in the right direction. It's not the only solution, as there's still the matter of how old session is cleaned up, moved, etc., and how the old ids are treated. At that point it's getting a bit farther than what one PR may want to do, and I believe this is sufficient and a good starting point.

If anyone else has anything to add feel free to drop it in here. The last thing I want to add is that if we make sure to version release this under 1.5, making it clear there's a core change.

[acinader]

@montymxb take a look at my readme edits.

I think we should give it a try as it is a good single point to do this.

I have been running for a few weeks now with no known impact.

Also, if it turns out that there is a material corner case that we are not anticipating that causes pain, I won't be averse to rolling this back.

[dplewis]

LGTM! @montymxb how does this look?

[parse-github-assistant[bot]]

The label type:feature cannot be used here.