Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add next query param with original request URL in requires decorator #920

Merged
merged 2 commits into from
Feb 16, 2022

Conversation

erewok
Copy link
Contributor

@erewok erewok commented Apr 29, 2020

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.

@erewok erewok force-pushed the on_auth_redirect_include_next branch from f819678 to 8332a80 Compare April 30, 2020 14:35
@adriangb adriangb added feature New feature or request outdated No updates on issue or commits within the last 6 months labels Feb 2, 2022
@tomchristie
Copy link
Member

Fantastic!
Thanks @erewok. 👏

@tomchristie tomchristie merged commit 2ec6db2 into master Feb 16, 2022
@tomchristie tomchristie deleted the on_auth_redirect_include_next branch February 16, 2022 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request outdated No updates on issue or commits within the last 6 months
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants