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

MPP-3495: Fix next URL check #4324

Merged
merged 3 commits into from
Jan 20, 2024
Merged

MPP-3495: Fix next URL check #4324

merged 3 commits into from
Jan 20, 2024

Conversation

jwhitlock
Copy link
Member

@jwhitlock jwhitlock commented Jan 19, 2024

The function is_safe_url in privaterelay.allauth.AccountAdapter checks if the provided next URL is safe. This was recently customized, but is broken.

For example, this URL:

https://stage.fxprivaterelay.nonprod.cloudops.mozgcp.net/accounts/fxa/login/?next=profile_refresh

will redirect the user after login to:

https://stage.fxprivaterelay.nonprod.cloudops.mozgcp.net/accounts/fxa/login/profile_refresh

Most URLs are not URL names, so most URLs will be identified as non-safe, and the user will redirect to /accounts/profile/?.

This PR fixes the function and improves it so that it will also redirect to next.js pages. Some things to try:

This issue was re-discovered while working on MPP-3698, which failed type checking with mypy --check-untyped-defs privaterelay/allauth.py. It now passes.

This allows fixing issues on a file-by-file basis.
The is_safe_url code was just wrong.

* Call the is_safe_url provided by django-allauth, which checks that the
  domain is an allowed domain and handles None.
* Check if the URL (not a URL name) is a valid name
pyproject.toml Show resolved Hide resolved
@jwhitlock jwhitlock added this pull request to the merge queue Jan 20, 2024
Merged via the queue into main with commit 71308cd Jan 20, 2024
25 checks passed
@jwhitlock jwhitlock deleted the fix-next-url-check-3495 branch January 20, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants