Skip to content

Fix the issue of get Authorization header fails during bearer auth #637

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

Merged

Conversation

yabea
Copy link
Contributor

@yabea yabea commented May 6, 2025

The bearer auth retrieves the Authorization from the headers, but the HTTPConnection.headers uses lowercase authorization, resulting in authentication failure.

@ihrpr ihrpr added this to the 2025-03-26 spec release milestone May 6, 2025
Copy link
Contributor

@ihrpr ihrpr 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 addressing header case sensitivity, a few suggestions:

  • improve the check for case insensitive handling
  • the "Bearer" prefix should also be checked case-insensitively
  • add tests to verify the fix and prevent regression

Comment on lines 37 to 39
auth_header = conn.headers.get("Authorization") or conn.headers.get(
"authorization"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
auth_header = conn.headers.get("Authorization") or conn.headers.get(
"authorization"
)
auth_header = next(
(conn.headers.get(key) for key in conn.headers if key.lower() == "authorization"),
None
)

auth_header = conn.headers.get("Authorization")
auth_header = conn.headers.get("Authorization") or conn.headers.get(
"authorization"
)
if not auth_header or not auth_header.startswith("Bearer "):
Copy link
Contributor

Choose a reason for hiding this comment

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

would Bearer have the same issue?

@yabea
Copy link
Contributor Author

yabea commented May 7, 2025

@ihrpr thanks for your suggestion. all of the above have been updated.Please review them again.

Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

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

thank you

@ihrpr ihrpr merged commit a1307ab into modelcontextprotocol:main May 7, 2025
6 checks passed
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