Skip to content

Starlette v0.26.0 support and authorize_redirect fix - #533

Merged
lepture merged 1 commit into
authlib:masterfrom
vicchi:gg-starlette-url-fix
Mar 22, 2023
Merged

Starlette v0.26.0 support and authorize_redirect fix#533
lepture merged 1 commit into
authlib:masterfrom
vicchi:gg-starlette-url-fix

Conversation

@vicchi

@vicchi vicchi commented Mar 20, 2023

Copy link
Copy Markdown
Contributor

Starlette 0.26.0 changed the return value of url_for() from str to URL; see Kludex/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

lepture commented Mar 20, 2023

Copy link
Copy Markdown
Member

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

@vicchi

vicchi commented Mar 20, 2023

Copy link
Copy Markdown
Contributor Author

@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 authlib:master Mar 22, 2023
@vicchi

vicchi commented Mar 23, 2023

Copy link
Copy Markdown
Contributor Author

@lepture Thanks! 😄

@nurettn nurettn mentioned this pull request Apr 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vicchi @lepture