Skip to content

fix: consistently use consumer-provided fetch function #767

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
merged 6 commits into from
Jul 17, 2025

Conversation

LucaButBoring
Copy link
Contributor

Updates the transports and auth to consistently use the consumer-provided fetch function, if provided.

Motivation and Context

Enables usage in environments with non-conformant fetch globals, and assists in debugging. I was passing a custom fetch function to check the request order in the auth flow, and wasn't seeing everything due to this issue.

How Has This Been Tested?

Existing unit tests plus a spot check in my debug setup.

Breaking Changes

None.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

  • I left the ?? fetch fallbacks in the code to avoid needing to change the unit tests. The unit tests currently rely on the fact that the global fetch is always looked up at call-time due to mocking the global fetch function.

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 so much for fixing it!

Please can you add unit tests that verify custom fetch functions are actually called throughout the auth flow

@LucaButBoring
Copy link
Contributor Author

Added partial unit tests, not ready for re-review yet (will request review once this is more comprehensive)

@LucaButBoring LucaButBoring requested a review from ihrpr July 15, 2025 22:12
@LucaButBoring
Copy link
Contributor Author

Found one missing usage with exchangeAuthorization that has now been fixed - took a while to come up with an appropriate way to test the transports, but this should be ready, now.

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 9d2a0ae into modelcontextprotocol:main Jul 17, 2025
2 checks passed
@LucaButBoring LucaButBoring deleted the fix/fetch-polyfill branch July 17, 2025 18:45
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