Deleting session cookie with redirect can lead to infinite redirects

[theseion]

Browsers are very nasty when confronted with a 'Set-Cookie' header in a 302 (temporarily moved) response. They may ignore the 'Set-Cookie' instruction completely or use very weird matching, which can lead to some cookies being deleted while others will simply stick around (see issue #915 for why there may be multiple session cookies at a given time). I know this because I've spent several days debugging such a problem :/. The symptom for users is often an infinite redirect loop (which browsers will cut short after a while).
The implementation of deleting the session cookie in Seaside will do exactly what I have described above, thus allowing for infinite redirect loops to occur. For Cmsbox we have a long standing fix, which looks like this for Seaside 3:

handleExpired: aRequestContext
    aRequestContext responseGenerator expiredRegistryKey.
        ^ self handleDefault: aRequestContext

So we delete the session cookie but then create a new session and proceed. The browser will see a response with status 200 and one or multiple 'Set-Cookie' headers (e.g. one for removing the old cookie and one for setting the new one).

[marschall]

I think you would have to decide whether to send #expiredRegistryKey or #handleDefault:. The idea is that applications can provide an implementation of #expiredRegistryKey that informs the user that his session has expired.

I can see a need for each behaviour and I'm not sure what the best way to toggle would be.

[theseion]

I would argue that this can be done simply by overriding WAApplication>>#handleExpired:. In the PR I submitted I've completely removed #expiredRegistryKey and moved the relevant parts to #handleExpired:.

I think that having to override #expiredRegistryKey is a bit far fetched. For my case I would have to provide both a subclass of WAApplication and a subclass of WAResponseGenerator. Why shouldn't it be enough to subclass WAApplication? Maybe having that method on WAApplication would make more sense:

WAApplication>>expiredRegistryKey: aRequestContext
    "override this method to react to expired keys in a different way"
    aRequestContext request isXmlHttpRequest ifFalse: [ ^ self ].

     self
          forbidden;
          respond

and

WAApplication>>handleExpired: aRequestContext
   ^ self
        expiredRegistryKey;
        handleDefault

[marschall]

The idea behind WAResponseGenerator was to have one place where applications can provide error pages in the look and feel of the application.

[theseion]

Ok, I see. So how about this: we provide an empty implementation for WAResponseGenerator>>expiredRegistryKey and dispatch to that from #handleExpired:. Developers can then decide whether they want to

• override #expiredRegistryKey and respond immediately (which will skip #handleDefault:) or
• override #handleExpired: to do something special or
• override both.

[marschall]

Yeah I'm leaning towards something like this. Maybe instead of an empty #expiredRegistryKey simply render a plain text "session expired" message (and get rid of the redirect).

[theseion]

Ok. I'll revise my PR.

[theseion]

I thinks there's still a problem here:

1. upon encountering an expired session #handleExpired: will be sent to the application (after the tracking strategy has handled the request)
2. #handleExpired: sends #expiredRegistryKey to the response generator
3. the response generator creates a plain text response explaining that the session timed out
4. #handleExpired: sends #handleDefault: which will, most likely, append to the response (e.g. via #processRendering:.
5. response is returned to the client, probably with status code 200.

So in the end, the response will consist of the error message (plain text) plus the default output (probably html), which is clearly wrong.

Here's a slightly different approach:

• provide #expiredRegistryKey as you suggest and make it respond immediately (I've looked at possible error codes but I don't think there's one more appropriate then 200; especially not if we want the client to see the message; and 302 could again introduce the redirect loop)
• #handleExpired: does not dispatch to the response generator (except for responding with a forbidden status for AJAX requests)

Developers can now mix and match. Your thoughts?

[marschall]

I prefer sending #respond to make it respond immediately. I don't see an appropriate 4xx status code, there seemed to be a non-standard 419 status code but this seems to be gone now. 200 is probably OK for now.

[theseion]

But wouldn't that break existing applications? The redirect currently prevents clients from noticing the that the session has expired. When we force a response in the default case, the client will see an error message and be stuck / forced to manually reload the page.

[marschall]

Yes, it's a behavioural change. But it's not like clients currently won't notice. Any state that's not in the URL is lost, including form based logins. Yes, if you're using URL query fields then manually reloading won't even fix the issue.

[takano32]

This problem have been happening in my service. 😢

[marschall]

@takano32 what would be more appropriate for your application, rendering an "error page" or rendering the "start page"?

[takano32]

@marschall Browser says 'ERR_TOO_MANY_REDIRECTS' and doesn't display any page.
Clearing browser Cookies in my application domain then it works well temporary.

[theseion]

@takano32 Yes, that is the error this issue describes and which we will fix. The question is what the default behavious should be and we'd like your opinion. What do you want your users to see (or not) when the session has expired? Would you want to inform them that any form data etc. has been lost? Or would you be happy to simply refresh the page?

[theseion]

@marschall I think we may be looking at different usage scenarios. In my scenario there is usually no data associated with a session that is worth keeping around. When a session has expired I don't want the user to notice. Example:

• user browses to example.com/some/page
• user makes his machine hibernate
• user comes back after session has expired and clicks on a link within the page (important: this link does not have a continuation associated with it but uses a simple path, e.g. href="/some/other/page")

I want the user to not notice that the session has expired. He should simply be taken to the requested page. Of course there are situations where a page may be reloaded and data may be lost but I'm ok with that.

What you have in mind, I think, is more along the lines of classic Seaside use, where every action is associated with a continuation. To be fair, that is probably the more common case for Seaside users, so we should probably stick to that and consider my scenario a special case.

[takano32]

@theseion @marschall Sorry, I didn't understand your question.Now I see.
I think default behavior should be display 'start page' or page that visitor want to see, and optionally display 'error page'.

In production visitor traverses many pages and almost always session expire is not so important.
But some cases, for example, visitor using 'Shopping Cart' and so on then session expire is to be informed to visitor.

[theseion]

That's a good point. Session expiry with a shopping cart should be announced to the user regardless from the behaviour during simple navigation.

[marschall]

@theseion I agree, default behaviour show the expiry page and make trying to cover from URL-state an opt-in