Add next query param with original request URL in requires decorator

[erewok]

Motivation

Recently when trying to make use of the redirect kwarg in the requires decorator, I was surprised when I lost all information about where the user had been redirected from. I wanted this information so that I could redirect the user back to the original protected URL they had requested after authentication. I wrote a custom decorator to achieve this instead of using requires.

If we offer a redirect kwarg inside the requires decorator, we may want the handler receiving the redirect to also be able to discern the original request destination. Otherwise, this information is lost.

Changes

This PR modifies the requires decorator to add the original request url as a url-encoded query param (example: next=https%3A%2F%2Flocalhost%3A8000%2Fadmin%3Fqparam%3Dtest) to the redirected request.

For comparison, in Django, there's a next query param added when you opt to use their login_required decorator (and in Django it's also possible to select a custom query parameter name different from "next" as well).

Security Concerns

It's possible to deliberately craft malicious URLs with dangerous redirects to give to a known user of an application with weak security around redirects. Most of these considerations fall on the implementer of the handler receiving the redirect.

For instance, there are various attempts to mitigate these problems inside Django's LoginView:

• Django's default LoginView performs the following security checks after successful login but before redirecting the user:
  • Check that scheme and hostname on next URL are allowed (same host as initiated the request, for instance).
  • If https is required, check that the redirect is also an https endpoint.
• Also check that there's no loop happening (that next is not pointing to the login handler, which would error in the browser with "the page isn't redirecting properly").

Note: because Starlette offers no default login handler, we'd have to sternly warn the user not to redirect requests willy-nilly. In other words, we'd probably have to guide users in the docs away from potential security pitfalls and probably provide a sample implementation for them to copy.

Next Steps

We may also consider the following in this PR:

1. Construct the next query param only if a specific keyword argument has been provided to the requires decorator (probably next_url_query_param=None or something like that) instead of always by default (this has the benefit of being backwards compatible as well: existing users would see no change; they'd have to opt-in to adding this kwarg).

2. Provide a sample login handler in the docs that contains some code for validating the next url is safe before redirecting users to it.

3. Callout in the docs the potential security pitfalls here.

This PR does not include any of the above because I wanted to present what I have for comment from the maintainers. I could also include one or more of the above changes.

Feedback welcome.

[tomchristie]

Fantastic!
Thanks @erewok. 👏