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
Chrome/FF follow redirect before session is fully saved #360
Comments
To clarify - I'm not sure whether that's allowed by the HTTP specification, but it's causing issues here either way. Other session handling libraries might also be affected... |
When trying to make a testcase without a database, I ran into some more weirdness. I tried to use a var store = new session.MemoryStore();
var fake = {
set: (...args) => setTimeout(() => store.set(...args), 2000),
get: (...args) => store.get(...args),
createSession: (...args) => store.createSession(...args),
touch: (...args) => store.touch(...args),
destroy: (...args) => store.destroy(...args),
length: (...args) => store.length(...args),
list: (...args) => store.list(...args),
expired: (...args) => store.expired(...args),
on: (...args) => store.on(...args)
}; and in this case the same issue seems to happen, but this time both in Chrome and in Firefox. Versions: Chrome 52.0.2743.116 m, Firefox 48.0.2 |
how are you redirecting the user? client side (javascript/angular?) or server side (express?) if client side you might be redirecting without the browser actually accepting the full header. both: it could be that the cookie is not being sent high enough in the header (above the new location) and the browser preemptively opens the next connection without the new cookie (though in chrome I believe they still will wait and then send the cookies... so, strange.) also what OS are you using? (might be related 😕 ) if the browser is just not waiting and then still not sending the cookies there isn't anything we can do to fix this in this library. |
Server-side, using
Per the debug log, the request for the redirect target is made before the redirect response is
If I recall correctly, @antishok is using Windows.
The problem isn't sending cookies - it's that the second request is made before the first request has completed updating the session data. The timeline should look like this:
However, instead, the timeline looks like this:
Thus, the second request and response are sent before the session data changes in the first response have a chance to complete saving, and consequently the second request/response see 'stale' session data. Possible solutions include buffering up all writes until the session data is saved (probably not practical), or at least delaying |
@dougwilson I'll have a look at that PR soon, and see if I can work out a way to move it forwards. Thanks! |
If you notice most typical large applications like forums and others use a bounce page which is literally meant to setCookies from and then the browser is redirected (typically using a javascript and or meta tag redirect) from there instead of from a redirect which is typically bad practice as you also can't easily tell if the user is just blocking cookies in general. Google, Twitter & many more also uses this practice of a bounce page with logins (check the network locations & headers) it is usually very small page: <html>
<head><meta http-equiv="refresh" content="5; url=http://example.com/"></head>
<body onload="window.location = 'http://example.com';"></body>
</html> |
@gabeio the issue isn't about cookies. The cookie only needs to be written once when creating the session (cookie only contains session-id). When the session is updated there is no need to set any cookie again - only to write to the session-store (server-side). The issue is that the browser requests the new page before the session-store has been updated, and so it sees old data. I don't really understand what you mean about a "bounce page" solution or where it's used, but in any case it doesn't really seem like an adequate workaround.. One workaround is to save the session before redirecting, but that doesn't help me when using a library such as |
Sorry I was misunderstanding that this wasn't a login which if you don't have a cookie and you try to give a cookie in a redirect it doesn't really work all the time especially if the location comes across first. But as for the browser viewing old data... Sounds like you might need a read lock of sorts. |
@gabeio The only thing that needs changing to resolve this issue, is to not write the response headers at all until the session save has completed. The question is just how to do this effectively without breaking the API. |
@joepie91 Couldn't this be avoided by adding |
Yes, it could, but this would be a leaky abstraction and a footgun.
I understand the concerns, but this would not be any different when calling
That's a workaround, not a fix. |
Yes. I spent like 2 days pin pointing the problem:( I'm just in shock that this library is heavily used by other apps and this fatal (I think) flaw exist, but besides that it's a great library. |
So... still no fixes or even nice workarounds? req.session.save() does not help |
This issue is open because a fix is desired. If someone knows how to fix, please help. |
Hi Doug. |
Yea, that has always worked for me. Is it not working when you do that in your code? |
It does not, but maybe some other parts of app are screwing something. I will try to set a test stand based on @antishok gist and be back with results. |
settings of 'resave' and 'saveUninitialized' may cause race conditions. I configured my app to work with them set to 'false' They are true by default. Read document for more detail info. https://www.npmjs.com/package/express-session#saveuninitialized |
resave & saveUninitialized set to false are not doing the trick. |
On my version of @antishok gist, with difference in store - I use mongo gives me same results for I am kinda lost here. I was hoping I could reproduce the problem with ease. To be clear Chrome Version 64.0.3282.167 (Official Build) (64-bit) Setup is running on 127.0.0.1:3000 |
I think I pinned down the problem. Please take a look at this gist For me this works as intended (bad :) ) (0. Everything happens with fresh start, e.g. incognito mode)
Interestingly, he is already registered in session. UPD Just using session.save callback does not work. Basically it boils to doing redirect or not. If I render a page (or just send info), than everything is fine. If I try to redirect - it goes nowhere and I do not know why. |
Changed code to call redirect only after manual save.
Does not help. As much as I can see, it goes like this:
B. Normal workflow
C. Result
I think it should be redirected to show /, but no - this is the end. User is shown a page from bullet 4. UPD express/lib/router/index.js -> UPD2 Note: a desperate solution to redirect first to some other page and then to 'predicted' yields same results. Solution This was just page cached in browser =_= Feel free to remove all my posts. Sorry. |
The problem I reported in other issues is when express-session does "save" internally to save a session to whatever target (e.g. file system), and not by calling "res.save()" manually, forget about that manual workaround for now ... express-session keeps executing code, does not wait, asynchronous, which then express-sessions sends http headers to redirect, then the browser request the redirect page, express-session is still saving, has not finished, which then cases such as logging a user fails because the session has not been saved yet. The workaround forces that after the save is done, please redirect. |
Any plans to implement optional save on redirects? Maybe that can be a new property in the initializer function? |
@antishok You are right that race condition occurs in case of custom stores, in these situations session needs to be saved explicitly before redirect. req.session.save(function(err) {
// session saved
}) I have written an example code demonstrating session persistence with redirection here : https://gist.github.com/HarshithaKP/3d9ad81138245aa7527bf385d37daba7 |
@HarshithaKP thanks! |
I'm still facing this issue years down the road from when this thread was opened...... here is one option, but it isn't for everyone.... depending on what session store you are using and how you want your session saves to be handled (always save vs only on change, etc). you can monkey patch redirect to always save the session before actually doing the redirect via middleware applied after session middleware
|
Here's a simple testcase incrementing a counter: https://gist.github.com/antishok/fb3d003d16eb72f672a7cc36401657d9
On chrome I have to refresh after incrementing in order to see the new count.
In response to a request, I write to the session and send a redirect to another page, but the new page is rendered with the old session data, and the changes I just wrote are not visible. The new data appears only after refreshing the page.
This only happens in Chrome, maybe due to an optimization they added that follows a redirect before the redirect response even ended.
I'm using
connect-session-knex
with postgresql to store my session data. If I use the MemoryStore, or use sqlite3 instead of postgresql, it works fine, but probably only because writing to them is much quicker.The gist also includes debug logs for the request (one when I tested with chrome, and one with firefox). You can see that for chrome, the new url is fetched before the session has finished saving, and before the redirect response ends.
(The logs include a log I manually added in the beginning of
express-session
'swriteend()
function, to indicate when a response actually ends)Thanks! And thanks to @joepie91 for figuring out where the issue lies
The text was updated successfully, but these errors were encountered: