hapijs / bell

Third-party login plugin for hapi
Other
624 stars 210 forks source link

Redirect Back to The Page That Required Auth #366

Closed ilyaigpetrov closed 5 years ago

ilyaigpetrov commented 6 years ago

If you have seen #305 then you know that redirecting a user back to the page that required authorization is a desired feature for many Bell users. Preserves Bell the hash fragment or not (#365), I think there should be an option in bell that does such redirects because manual implementation may be subject to open redirect attacks, not every developer may implement redirects in a secure way. That's why I think this feature should be implemented securely by the Hapi team. This may require a coordination with hapi-auth-cookie, which has redirectTo and appendNext options that should interplay with Bell redirects implementation.

ilyaigpetrov commented 6 years ago

Here is a bell and auth-cookie boilerplate that redirects back: https://github.com/ilyaigpetrov/bell-redirect-back

AdriVanHoudt commented 6 years ago

How would this be different then doing this in your handler?

exports.bellHandler(request, h) => {

    if (!request.auth.authenticated) {
        return h.redirect(request.auth.credentials.query);
    }
    ...
}

And request.auth.credentials.query contains every query param you initially send. If you want to configure it on the server then I guess that would be feature request which I think is reasonable. What I do now for example, we have a general /connect route which takes some query params like /connect?type=google and sets some data in a secure cookie that we then use in the final handler, which is more dynamic but not entirely the same I guess.

ilyaigpetrov commented 6 years ago
  1. JS redirect is needed because I want to get rid of the empty # when hash fragment was not set originally. Other possible approach is <meta http-equiv="refresh"... but I didn't test it.
  2. I also integrate bell with hapi-auth-cookie to avoid starting auth on every request, so request.auth.credentials.query contains not the initial page url, but /login/redirect_to_path=/dashboard/?abc=xyz&baz#foobar (but percent-encoded).

The final picture: 1) User visits /dashboard/?abc=xyz&baz#foobar 2) He has no cookie, so he is redirected to /unauthed-redirector/?redirect_to_path=/dashboard/?abc=xyz&baz#foobar, but hash is not percent-encoded and not sent to the server. 3) With JS I append #foobar to the percent-encoded redirect_to_path and redirect user to /login/?redirect_to_path=/dashboard/?abc=xyz&baz#foobar (but the param is now percent-encoded). 4) /login/ handler follows auth flow and appends empty # in the browser address bar. If I h.redirect('/dashboard/?abc=xyz&baz#foobar') then everything is fine, however if I h.redirect('/dashboard/?abc=xyz&baz') then user ends with /dashboard/?abc=xyz&baz# in his address bar. If I want to get rid of that redundant empty # then I have to redirect with JS.

My feature request: hapi-auth-cookie and bell should be integrated in such a way, that:

  1. Developer doesn't have to write /unauthed-redirector/ which purpose is to encode and send hash fragment to the server.
  2. Developer doesn't have to filter unsafe redirect_to_path values (it's dangerous for security, think about phishing).
  3. Developer doesn't have to write JS redirector to get rid of redundant empty # (it's dangerous for security, think about executing malicious js code on the client).
sholladay commented 6 years ago

For anyone who is curious about doing this manually until bell supports it natively, the resolveNext() function in my hapi-doorkeeper module may be useful to you.

The idea is to have the login link in the browser be set to something like '/login?next=' + encodeURIComponent(location.href) or equivalent for whatever technology you are using.

AdriVanHoudt commented 5 years ago

@ilyaigpetrov @sholladay I think opening a PR would be the easiest way to go further?

sholladay commented 5 years ago

I would love to, if we can agree on an approach. In my case, which I think is pretty common, I have to set a cookie to remember the user, before the redirect happens. I suppose bell could have a function option that would run before the redirect. But the implementation on the user's side would probably be specific to each route. And setting plugin options in route config always feels verbose and unclean to me.

An alternative might be for bell to provide a request.bell.redirect() function that hides away the details of protecting against open redirect attacks, but which you have to call manually.

Even that doesn't feel particularly elegant, but I'm happy to work on it when there is a consensus about how to do it.

AdriVanHoudt commented 5 years ago

If I follow this correctly we do the same thing. We have generic route that when not authenticated sets a cookie with a bunch of info and then redirects to the actual Bell auth route. That route can do with the cookie info what it wants to.

If you want a run a function before the Bell stuff is invoken then a route config seems like the way to go. What do you mean with that the user's implementation would be route specific?

sholladay commented 5 years ago

What do you mean with that the user's implementation would be route specific?

They might need to set different cookies in different routes before the redirect, for example. I'm just guessing, though.

For me, personally, the way I use bell, there's only one route on my whole server that actually uses the bell strategy. That is the /login route. Everything else redirects to /login if my session cookie isn't set. Therefore, I don't personally need route-level middleware for bell, because I only use bell once in any given server. But I do need to be able to set my session cookie before the redirect.

However, if I understand cirrectly, some people use bell a bit differently and instead have bell on multiple routes (e.g. one route for Facebook login, one for Google, etc.) I don't like doing that, but I imagine that however we solve this issue, it should probably support that use case. That's what I'll refer to as route-level config, because these users presumably have special logic in each route that needs to run before the redirect. I could be wrong!

Maybe it's best to start simple and have a "global" pre-redirect handler. Then we could add a route-level solution for it afterwards if there's demand.

AdriVanHoudt commented 5 years ago

@ilyaigpetrov would the suggestion of @sholladay solve your problem?

ilyaigpetrov commented 5 years ago

Thank you for your attention to the problem but I currently don't use Hapi or Bell for any of my projects so I doubt I should create a PR neither I should suggest any solutions for lack of practice/experience/interest on my side. The ticket may be closed and reopened by anyone offering a graceful solution.

AdriVanHoudt commented 5 years ago

@ilyaigpetrov thanks for the reply!

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.