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

Client sessions can become invalid #8616

Closed
ErisDS opened this issue Jun 22, 2017 · 5 comments
Closed

Client sessions can become invalid #8616

ErisDS opened this issue Jun 22, 2017 · 5 comments
Assignees
Labels
affects:admin Anything relating to Ghost Admin bug [triage] something behaving unexpectedly

Comments

@ErisDS
Copy link
Member

ErisDS commented Jun 22, 2017

This is related to #8615

This morning, a brand new Ghost blog was setup for the first time at around 8:45am I think. At 1:13pm, @JohnONolan reports being logged out / shown the reauth screen whilst writing a post. Meaning the session didn't last past a few hours. Then #8615 happened, whereby he wasn't able to upload images afterwards.

I have full logs for the blog in question if anyone wants them. Haven't looked at them deeply yet.

@ErisDS ErisDS added affects:admin Anything relating to Ghost Admin bug [triage] something behaving unexpectedly labels Jun 22, 2017
@kevinansfield
Copy link
Contributor

I've been trying really hard to replicate this but so far no luck 😞 full logs for the blog could be useful

@kevinansfield
Copy link
Contributor

kevinansfield commented Jul 5, 2017

I've been reviewing again after looking through the server log files. The interesting group of requests appears to be these (paraphrased logs):

11:52:52 - PUT /settings/
11:56:59 - PUT /posts/594b7bfa48abca2476efdd65/?include=tags
11:57:02 - GET /configuration/
11:57:02 - GET /users/me/?include=roles
11:57:02 - GET /settings/?type=blog%2Ctheme%2Cprivate
11:57:03 - POST /authentication/token
11:57:03 - GET /notifications/
11:57:03 - GET /settings/?type=blog%2Ctheme%2Cprivate
11:57:03 - GET /themes/
11:57:14 - PUT /posts/594b7bfa48abca2476efdd65/?include=tags
11:57:15 - GET /configuration/
11:57:15 - GET /users/me/?include=roles
11:57:15 - GET /settings/?type=blog%2Ctheme%2Cprivate
11:57:15 - POST /authentication/token
11:57:15 - GET /notifications/
11:57:15 - GET /settings/?type=blog%2Ctheme%2Cprivate
11:57:15 - GET /themes/
11:57:26 - PUT /posts/594b7bfa48abca2476efdd65/?include=tags
11:57:28 - GET /configuration/
11:57:28 - GET /users/me/?include=roles
11:57:28 - GET /settings/?type=blog%2Ctheme%2Cprivate
11:57:28 - POST /authentication/token
11:57:45 - PUT /posts/594b7bfa48abca2476efdd65/?include=tags
12:01:02 - PUT /posts/594b7bfa48abca2476efdd65/?include=tags
12:01:05 - PUT /posts/594b7bfa48abca2476efdd65/?include=tags
12:12:53 - POST /uploads/ - access denied, client logged out

All requests are for the same user from the same IP address which seems to suggest that multiple tabs were being used and there were at least 3 loads of the admin client (GET /configuration/ only happens at boot).

My current thinking regarding the auth issues relates to this piece of server code that reduces the lifetime of old access tokens to 5 mins each time a new token pair is generated.

I haven't been able to replicate the situation yet but if the access token that a client is using has it's expires shortened on the server due to the generation of new tokens elsewhere then the admin client doesn't know about it and will continue using the soon-to-be-expired token - when the token does expire on the server (5 mins after the token generation) then the admin client will be logged out on the next request

I've tested a lot with multiple tabs, incognito tabs and different browsers but so far my experience has been:

  • multiple tabs are kept in sync via the shared local storage
  • incognito tabs get their own set of tokens
  • different browsers get their own set of tokens

That's not to say the above couldn't break down, if the browser doesn't fire local storage events properly then multiple tabs could go out of sync, I also need to look into how "clients" are determined on the server in order to give incognito tabs/different browsers separate credentials.

Unfortunately the logs strip out the auth headers for security reasons so it's not possible to tell exactly what happened from the logs.

@kevinansfield
Copy link
Contributor

From what I can see and my testing so far I do not believe this is specific to the editor or the uploads endpoint so I'm going to update the issue title to reflect.

My feeling at the moment is that this is an edge case that has appeared due to the frequent token regeneration whilst working in multiple tabs with multiple admin loads/refreshes - it definitely shouldn't happen but it doesn't look like it's a general session bug.

If we don't find exactly why the token expired was used in this case I think there are a couple of avenues to further reduce the likelihood of logout:

  1. Remove the shortening of token lifetimes when new tokens are generated.
    • we end up with a lot of long-lived access tokens in the database that are not in use by any clients
  2. Implement request retries in the client
    • this would involve intercepting every request and storing them in a queue, if we get a 401 for a request we can attempt to refresh the access token before retrying
    • quite heavy implementation wise, not something we'd want to introduce so close to 1.0

@kevinansfield kevinansfield changed the title Sessions not lasting whilst editing a post. Client sessions can become invalid Jul 5, 2017
@kevinansfield
Copy link
Contributor

Another potential workaround would be to change the token re-generation on client boot logic so that we only refresh at most once a day - that would prevent the issue with multiple simultaneous tabs and refreshes causing edge cases.

For context the refresh-on-boot behaviour was introduced to ensure that sessions are effectively "infinite" as it means there's a continuous extension of refresh token expiration length.

kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this issue Jul 5, 2017
refs TryGhost/Ghost#8616
- only refresh tokens on boot if we last refreshed more than 24hrs ago - this should prevent rapidly changing access/refresh tokens when opening new admin tabs or refreshing whilst other tabs are open
- fix token refresh test which was testing it's own behaviour instead of the applications 🙈

This may not be the full solution to the session issues but it closes one potential culprit and should at least reduce token churn which can only help track down the real cause.
kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this issue Jul 5, 2017
refs TryGhost/Ghost#8616
- only refresh tokens on boot if we last refreshed more than 24hrs ago - this should prevent rapidly changing access/refresh tokens when opening new admin tabs or refreshing whilst other tabs are open
- fix token refresh test which was testing it's own behaviour instead of the applications 🙈

This may not be the full solution to the session issues but it closes one potential culprit and should at least reduce token churn which can only help track down the real cause.
kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this issue Jul 9, 2017
refs TryGhost/Ghost#8616
- only refresh tokens on boot if we last refreshed more than 24hrs ago - this should prevent rapidly changing access/refresh tokens when opening new admin tabs or refreshing whilst other tabs are open
- fix token refresh test which was testing it's own behaviour instead of the applications 🙈

This may not be the full solution to the session issues but it closes one potential culprit and should at least reduce token churn which can only help track down the real cause.
kevinansfield added a commit to kevinansfield/Ghost-Admin that referenced this issue Jul 10, 2017
refs TryGhost/Ghost#8616
- only refresh tokens on boot if we last refreshed more than 24hrs ago - this should prevent rapidly changing access/refresh tokens when opening new admin tabs or refreshing whilst other tabs are open
- fix token refresh test which was testing it's own behaviour instead of the applications 🙈

This may not be the full solution to the session issues but it closes one potential culprit and should at least reduce token churn which can only help track down the real cause.
kirrg001 pushed a commit to TryGhost/Admin that referenced this issue Jul 10, 2017
refs TryGhost/Ghost#8616

- only refresh tokens on boot if we last refreshed more than 24hrs ago - this should prevent rapidly changing access/refresh tokens when opening new admin tabs or refreshing whilst other tabs are open
- fix token refresh test which was testing it's own behaviour instead of the applications 🙈

This may not be the full solution to the session issues but it closes one potential culprit and should at least reduce token churn which can only help track down the real cause.
@ErisDS ErisDS removed the post 1.0.0 label Aug 10, 2017
@kevinansfield
Copy link
Contributor

I think this can be closed for now, there haven't been any further reports since adding the 24 hour token refresh workaround. We can re-open or start a new issue if this rears it's head again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:admin Anything relating to Ghost Admin bug [triage] something behaving unexpectedly
Projects
None yet
Development

No branches or pull requests

3 participants