Chrome/FF follow redirect before session is fully saved

[antishok]

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's writeend() function, to indicate when a response actually ends)

Thanks! And thanks to @joepie91 for figuring out where the issue lies

[joepie91]

To clarify - express-session only delays the end of the response itself, not the chunks that are sent... meaning that Chrome will technically obtain the Location header before the session completes saving, and it seems to use that to pre-fetch the next page ahead of time.

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...

[antishok]

When trying to make a testcase without a database, I ran into some more weirdness. I tried to use a MemoryStore and delaying the write:

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. On the other hand, IE11 works just fine.

Versions: Chrome 52.0.2743.116 m, Firefox 48.0.2

[gabeio]

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. if server side what redirection code are you using?

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.

[joepie91]

how are you redirecting the user? client side (javascript/angular?) or server side (express?)

Server-side, using res.redirect.

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.)

Per the debug log, the request for the redirect target is made before the redirect response is ended. The problem seems to be that express-session makes the assumption that "the browser won't attempt to act on the response until we've closed the connection", and Chrome and Firefox are violating that assumption - at least, for redirects.

also what OS are you using? (might be related :confused: )

If I recall correctly, @antishok is using Windows.

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.

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:

1. Request 1 sent
2. Response 1 sent, but connection left open
3. Session data for response 1 started saving
4. Session data for response 1 completed saving
5. Response 1 closed
6. Browser follows Location header
7. Request 2 sent
8. Response 2 sent

However, instead, the timeline looks like this:

1. Request 1 sent
2. Response 1 sent, but connection left open
3. Session data for response 1 started saving
4. Browser follows Location header
5. Request 2 sent
6. Response 2 sent
7. Session data for response 1 completed saving
8. Response 1 closed

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 res.redirect calls until after the session completes saving, if that's the only known case of browsers being a little too clever about pre-fetching.

[dougwilson]

@joepie91 there are a bunch of existing issues about this same thing already, even a (stalled) PR: #157

[joepie91]

@dougwilson I'll have a look at that PR soon, and see if I can work out a way to move it forwards. Thanks!

[gabeio]

@joepie91

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>

[antishok]

@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 passportjs which does the redirects for me, on login success/fail.

[gabeio]

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.

[joepie91]

@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.

[gabeio]

@joepie91 Couldn't this be avoided by adding req.session.save() right after line#30 I get that, that is annoying to have to do for all redirects but I feel like buffering the entire request and waiting for a full write for anyone who redirects isn't exactly good performance wise either, especially since you currently can strategically place the force saves.

[joepie91]

Couldn't this be avoided by adding req.session.save() right after line#30

Yes, it could, but this would be a leaky abstraction and a footgun.

but I feel like buffering the entire request and waiting for a full write for anyone who redirects isn't exactly good performance wise either

I understand the concerns, but this would not be any different when calling save manually - you still have to wait for the session save to complete before writing the header. Since express-session already short-circuits when there's no modified session data to save, a patch in express-session for redirects would not incur a performance penalty.

especially since you currently can strategically place the force saves.

That's a workaround, not a fix.

[mjquito]

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.

[Amantel]

So... still no fixes or even nice workarounds? req.session.save() does not help

[dougwilson]

This issue is open because a fix is desired. If someone knows how to fix, please help.

[Amantel]

Hi Doug. You mentioned putting redirect in a save callback here https://github.com/expressjs/session/issues/526#issuecomment-346944647. I guess it really does not help? (I was thinking maybe I am doing something wrong)

[dougwilson]

Yea, that has always worked for me. Is it not working when you do that in your code?

[Amantel]

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.

[mjquito]

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

[Amantel]

resave & saveUninitialized set to false are not doing the trick. Big part of the problem for me, is that I can not reproduce this bug (?) in 100% cases. Sometimes it is there, sometimes it is not. Maybe it is based on Chrome prediction service settings. I even tried putting setTimeout inside session.save callback - nothing new. I will finish test stand to isolate the issue (somehow), and will try to do render instead of redirect.

[Amantel]

On my version of @antishok gist, with difference in store - I use mongo gives me same results for Chrome: https://pastebin.com/xiUkJvcy Firefox: https://pastebin.com/GLhsLDnz

I am kinda lost here. I was hoping I could reproduce the problem with ease.

To be clear My setup is: Express 4.16.2, Express-session 1.15.6 Connect-mongo 2.0.1

Chrome Version 64.0.3282.167 (Official Build) (64-bit) Firefox 58.0.2 (64-bit) Both on Ubuntu

Setup is running on 127.0.0.1:3000

[Amantel]

I think I pinned down the problem. Please take a look at this gist For me this works as intended (bad :) ) ~~on server (with qualified domain name etc.) because there Chrome is bound to launch his prediction workflow~~ everywhere (server, localhost) when prediction shoots.

(0. Everything happens with fresh start, e.g. incognito mode)

1. User types mysite.ru/login?yes=1
2. Basically, when there is website name (website link?) in omnybox, Chrome requests it, get redirected, get all the resources get 'express-session no SID sent, generating session'.
3. User press enter and see the test page (which is wrong).

Interestingly, he is already registered in session. So - this is the problem. Tomorrow I will find session.save with callback, but I guess it will not help. If anybody have some ideas, let's share them. Starting from - is this what this issue is about?

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.

[Amantel]

Changed code to call redirect only after manual save.

  if (req.query.yes) {
    req.session.userAuthed = 1;
    req.session.save(() => {
      return res.redirect("/");
    });
  } else {
    return res.redirect("/");
  }

Does not help.

As much as I can see, it goes like this: A. Prediction

1. Chrome asks for / page
2. Express 'goes' to / page (This is everything else I can see it in the log files)
3. Code on / page redirects to /testpage page
4. Express 'goes' to /testpage page (express:router dispatching GET /testpage)

B. Normal workflow

5. User finally asks for /login?yes=1
6. Express dispatches /login?yes=1 page (dispatching GET /login?yes=1) and creates session

C. Result

7. Finally browser displays testpage.

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 -> proto.handle It is called if there was not 'prediction' call to page were we are trying to redirect and is not called otherwise, so everything is stopping at this.end(body); at response.js redirect function. If we just call the page 'by hand' - everything is ok.

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.

[mjquito]

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.

[pragmaticivan]

Any plans to implement optional save on redirects? Maybe that can be a new property in the initializer function?

[HarshithaKP]

@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 I have used nodeJS client, you can use chrome/FF clients as it works well for all. Please let me know if it helps in resolving your issue. Ping @pragmaticivan @Amantel @mjquito

[Amantel]

@HarshithaKP thanks! I kinda moved past this, but I will try to see if my gist is working now, thanks!

[h3knix]

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

app.use((req,res,next)=>{
  if ( req.session && req.session.save ) {
    let _redirect = res.redirect;
    res.redirect = function(){
      let args = [].slice.call(arguments);
      req.session.save((err)=>{
        if ( err ) console.error(err);//or handle errors however you want
        _redirect.apply(res,args);
      })
    };
  }
  next();
});