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

ensure correct redirect uri query handling #155

Merged
merged 3 commits into from
Dec 30, 2023
Merged

Conversation

maxcountryman
Copy link
Owner

This updates the way macros handle the redirect URI such that the query string is handled in a more correct way. Prior to this, the macros assumed no other query. Now we extend the query, if present, with the configured redirect field and URI.

Fixes https://github.com/maxcountryman/axum-login/issues/143.

This updates the way macros handle the redirect URI such that the query
string is handled in a more correct way. Prior to this, the macros
assumed no other query. Now we extend the query, if present, with the
configured redirect field and URI.

Fixes #143.
@maxcountryman
Copy link
Owner Author

cc @benwilber: let me know if this addresses your original issue.

@benwilber
Copy link

Thank you! I'll test this and report back!

@benwilber
Copy link

It works great except in the case where my login_url already has a next= parameter. In that case it is actually adding a second next= which I don't think is the correct behavior. If I add &next=foo here then the resulting URL becomes:

Some("/login?next=%2F&foo=bar&foo=baz&next=foo")

Where the next parameter has now been repeated. It's kind of ambiguous which actual next= parameter will be used in this case. Because axum-login is adding this parameter implicitly, I think a good behavior would be for it to only add it's own next param if the same param is not already present in my login_url, which presumably would have been added intentionally. This could also address a potential issue where a redirect loop keeps adding next= over and over again. I haven't actually seen that issue, but seems plausible.

I love this library! Thanks for making it!

@maxcountryman
Copy link
Owner Author

Thanks for spotting that. I agree with your assessment.

@maxcountryman maxcountryman merged commit cd65cee into main Dec 30, 2023
4 checks passed
@maxcountryman maxcountryman deleted the login-url-qs branch December 30, 2023 18:58
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.

login_required macro generates invalid redirect URL
2 participants