Web url too may redirects

[bluehash]

Hello, Thank you for an amazing wish list manager.

I have mine on a docker container which works great when I access via local_ip:port number

In order to get this online, I registered with no-ip and have my router register via dynamic dnd (ddns) and forward the port.

So now I have a url like no-ip-url:port-no/wishlist. This works great once on the login page and then no longer works with "too many redirects". However the local ip method works.

Is this setup sound right or am I missing a step.

Thank you!

It

[Wingysam]

Could you check the network tab of the browser's developer tools please? What is it redirecting to?

[bluehash]

It magically works now. I'll reopen this if I find anything.

[Wingysam]

Great, probably a caching issue.

[bluehash]

Hello, I'm reopening this as it has come back. I may have a way of reproducing this. How do I send over a network log without sensitive data?

[bluehash]

I'm running the latest docker image.

This is how I can reproduce it:

1. Clear all cookies.
2. Pick a wishlist url: http://192.168.1.XXX:XXXX/wishlist/username
3. Go directly to the url
4. Redirected login
5. Login
6. Web url too many redirects issue.

[Wingysam]

Thank you, that should help with finding the issue.

[Wingysam]

Appears to be related to Passport. Will be fixed by #25.

[TheTechmage]

I did some investigation, @Wingysam. This does not appear to be related to Passport, but instead is related to express-session. If I create an anonymous function within app.use like the following code, as well as remove passport, I am still seeing the problem arise.

app.use((req, res, next) => {
  console.log(`Cookie Data: ${JSON.stringify(req.session.cookie.data)}`);
  req.session["testing"] = {};
  req.session["testing"].test = true;
  req.session.save(err => console.error(err));
  next()
});

When express-session is saving cookies, none of the options (such as path or max-age) are being set on the cookie headers themselves. What baffles me is that the log line there shows that the options are set in the right location and it's still not working.

Now, to describe the issue at hand and explain part of the why login errors are occurring:

• A web request is made to /wishlist/user
• The user is given a cookie and redirected to /login
  • The path of the cookie is unset, so the browser sets the cookie to /wishlist since that's the "deepest folder"
• The user lands on /login and is granted a cookie (since they "don't have one" already, path mismatch against /wishlist)
  • The path of the cookie is unset, so the browser sets the cookie to / since that's the "deepest folder"
• The user successfully logs in
  • Login path is /login, so the new cookie that says we're logged in goes to /
  • The user is redirected to /wishlist
  • The unauthenticated cookie at /wishlist takes priority over the one set for /, so we use that one in req.session
    • According to the cookie, the user is unauthenticated, redirect them to /login
    • Upon redirection, we note that according to the most-relevant cookie at /, the user is logged in. Redirect them to /wishlist

And that's how we get the redirect loop. Even if you switched away from Passport, so long as express-session is being used, I believe that this would still be an issue and I don't think that #25 will fix it.

To work around the problem on my own setup, I have added the following middleware piece. This snippet tells the browser to delete the cookie that was set on the /wishlist path, if it is an unauthenticated cookie.

app.use((req, res, next) => {
  if(!req.session.passport || Object.keys(req.session.passport).length === 0)
    res.clearCookie('christmas_community.connect.sid', {path: '/wishlist'});
  next();
});

Hope this helps a bit!

[Wingysam]

@frostyfrog Thank you for figuring this one out! I'll push a fix out when I can.

[TheTechmage]

Yep, no problem! Just a heads up, I spent some time today and found the root cause. Turns out, the session-pouchdb-store is clobbering the cookie object for the session when it saves a record. The offending lines are located within store.js. I hacked the code in my local environment and got everything to work properly! \o/

I'll submit a PR upstream. Fingers crossed that they watch the repo (since the last release was in 2019)

A relevant issue I found on the matter: https://github.com/solzimer/session-pouchdb-store/issues/2

[Wingysam]

Thanks. Maybe I could fork session-pouchdb-store, fix the issue, the override the transitive dependency? That's assuming they won't merge and release a PR since the project might be dead.

[TheTechmage]

Huh, it never occurred to me that that would be a valid strategy. I tend to make the effort to fix things upstream and, where possible, monkey-patch a fix in the meantime. I'll help by consolidating all of my fixes and pointing you to them. You're free to use them as-is or use them as a reference for your own fixes.

[TheTechmage]

Fixes to the pouchdb store have been implemented here: https://github.com/frostyfrog/session-pouchdb-store/tree/fix/christmas-patches I'm not quite sure how to get it loaded, as developing node libraries is not my forte. But that should provide a good starting point, as well as fix issue #6. I'm also working on some PRs (for here) to assist in removing bad cookies and provide other improvements.

[Wingysam]

It'll be easy to get it loaded, I'll just put the git url in the overrides property of the package.json.

[TheTechmage]

Thank you for being on top of the notifications for this repo, @Wingysam

[Wingysam]

Thank you for doing like all of the work for me!

[TheTechmage]

IMHO, open source is about contributing to projects when you're able to. One of my favorite things that I've learned as I've started to work in open source professionally. When I talk to my team-lead about a problem in an upstream library, the approach has been to just hack a fix to the library, then submit a PR with a proper fix once everything's been figured out.

As soon as I learn how the .pug templating framework works, I'll look at submitting some nice UX improvements as well 🙂 None of this would be possible if you had decided to leave your application closed. It's been fun to work on over the Thanksgiving holiday 😁

[Wingysam]

@frostyfrog I think SessionStore#set is sometimes called without data.

[Wingysam]

I've implemented frostyfrog's workaround and it'll be in the next release. It's not an ideal fix but it solves the problem.