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

Fixed Handled error in OAuth2ExtraTokenMiddleware when authheader has Bearer with no token-string following up #1502

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Tuhin-thinks
Copy link

Fixed the crash in application while using OAuth2ExtraTokenMiddleware. When Bearer token passed is empty.
Authorization: Bearer would result in this crash.

Fixes #1496

Description of the Change

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@n2ygk n2ygk added this to the Release 3.1.0 milestone Sep 22, 2024
@n2ygk n2ygk force-pushed the bug/1496/unhandled-empty-bearer-token-exception branch from 3efa9a5 to f87c5f4 Compare September 22, 2024 17:56
Copy link
Member

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix. Please also add a test case in tests/test_auth_backends.py to assure full code coverage.

Comment on lines 55 to 56
if authheader.startswith("Bearer") and len(authheader.split(maxsplit=1)) == 2:
tokenstring = authheader.split(maxsplit=1)[1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest not calling split() twice:

Suggested change
if authheader.startswith("Bearer") and len(authheader.split(maxsplit=1)) == 2:
tokenstring = authheader.split(maxsplit=1)[1]
splits = autheader.split(maxsplit=1)
if authheader.startswith("Bearer") and len(splits) == 2:
tokenstring = splits[1]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your suggestion, I will make the changes by today!

@dopry
Copy link
Contributor

dopry commented Oct 4, 2024

@Tuhin-thinks this looks good. I think the last task to get this merge ready is the test.

@Tuhin-thinks
Copy link
Author

@dopry Thanks, I am getting them ready.

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.

Empty Bearer token results in unhandled exception
3 participants