Login doesnt respect returnTo value in session

[uplift]

doLogin doesnt respect the session returnTo value and just redirects to originalUrl.

When I hit /admin and not logged in it redirects to /login. This in turn redirects to our third party login service that creates the session sharing cookie. After this it redirects back to /admin and gets in a redirect loop as the ensureLoggedIn keeps redirecting to /login because at this point the user isnt logged in through session sharing yet (this is maybe another issue but I havent figured out where session sharing fits into the flow around ensureLoggedIn).

I noticed going through the /admin redirection that returnTo on session is set before going to /login so I set the return url of our third party login service to just return to the forum homepage so it goes through session sharing and logins in. It would be ideal if the callback to doLogin checked returnTo and redirected to that if it exists and falls back to originalUrl if not.

Thoughts? If theres a better value to do this let me know.

https://github.com/julianlam/nodebb-plugin-session-sharing/blob/98c826f3b396e7aeece7c9dedb0f957ee9dd3c00/library.js#L484-L487

[julianlam]

Sure, I think that makes sense.

The problem with this system is that we have a value that is calculated on the client side, and later passed on to the server side. Along the way there might've been a different way we calculated the returnTo, and so it's all very confusing whenever we want to make changes to it. I'll need to re-familiarise myself first...

[julianlam]

v4.4.2

[uplift]

Does nconf.get('relative_path') need to be in the redirect?

We have the forum on a sub path e.g. /forum but the return to redirects are coming back /forum/forum/admin

[julianlam]

Oh dear... with that changeset I have no idea what line of code I changed anymore :laughing:

[julianlam]

In my (albeit limited) testing, the relative path was necessary since it was removed in core... but I am finding other examples where it is explicitly prepended. Will look into it further.

[julianlam]

Does that commit help?

[uplift]

Unfortunately no. I tracked it down to the connect-ensure-login package has this line

https://github.com/jaredhanson/connect-ensure-login/blob/cdb5769a3db7b8075b2ce90fab647f75f91b3c9a/lib/ensureLoggedIn.js#L44-L48

[uplift]

As a quick fix, I've added on L488

url = url.replace(new RegExp('^' + nconf.get('relative_path')), '');

Not sure if there is a better way to fix?

[julianlam]

Well, that'll work to fix the final redirect, although the issue seems to be that something is saving the value to req.session.returnTo with the relative path, when this is no longer required.

I thought I found all cases where it was doing so, but perhaps I missed one...?

[julianlam]

Yes, I went through every place in core that sets req.session.returnTo and manually triggered them. They only save the path without the relative_path, so I really don't know how you're running into a situation where you're seeing the path doubled...

[julianlam]

@uplift This will resolve the issue... undergoing some double-checking ATM: https://github.com/NodeBB/NodeBB/pull/7391

[uplift]

Another thing I noticed is that it doesnt delete the returnTo on the session once its used so keeps looping to the returnTo on refresh

[julianlam]

The issues never end :sweat: — I'll take a look at this :soon:

[julianlam]

v4.4.5