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

Starlette v0.26.0 support and authorize_redirect fix #533

Merged
merged 1 commit into from Mar 22, 2023

Conversation

vicchi
Copy link
Contributor

@vicchi vicchi commented Mar 20, 2023

Starlette 0.26.0 changed the return value of url_for() from str to URL; see encode/starlette#1385 for context.

Passing a redirect_uri to authorize_redirect causes a TypeError: cannot convert 'URL' object to bytes exception to be raised if the URL has been generated via url_for(). This fix detects an instance of URL and casts it to a string, thus allowing authentication to proceed and generally making people happy.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

  • You consent that the copyright of your pull request source code belongs to Authlib's author.

@lepture
Copy link
Owner

lepture commented Mar 20, 2023

How about just redirect_uri = str(redirect_uri) without the isinstance? For <0.26.0, str(redirect_uri) would still work.

@vicchi
Copy link
Contributor Author

vicchi commented Mar 20, 2023

@lepture That is indeed true, but my thinking was that just casting to a string by default could introduce sutble gotchas if Starlette's url_for() wasn't being used to build the redirect URL. We can be sure that Starlette's URL class does have an __str__ dunder (see https://github.com/encode/starlette/blob/62b5b6042a39289ed561580c251c233250c3c088/starlette/datastructures.py#L166 for context) but we can't be sure there's nothing else at play here, hence me being cautious and checking that this is an instance of URL before casting.

More than happy to be told I'm being overly cautious here; it wouldn't be the first time.

@lepture lepture merged commit acde1e9 into lepture:master Mar 22, 2023
7 checks passed
@vicchi
Copy link
Contributor Author

vicchi commented Mar 23, 2023

@lepture Thanks! 😄

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.

None yet

2 participants