Allow OAuth instance to use the same httpx factory as the Transport #2324
Conversation
|
Warning Rate limit exceeded@guschnwg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughOAuth now accepts an optional Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Transport as StreamableHttpTransport
participant OAuth as OAuth
participant Factory as httpx_client_factory
App->>Transport: new StreamableHttpTransport(..., httpx_client_factory=factory)
Transport->>Transport: self.httpx_client_factory = factory
Transport->>Transport: _set_auth("oauth")
Transport->>OAuth: new OAuth(url, httpx_client_factory=factory)
OAuth->>OAuth: store self.httpx_client_factory
rect rgba(200,220,240,0.3)
Note over OAuth,Factory: when performing redirect/requests
OAuth->>Factory: call httpx_client_factory()
Factory-->>OAuth: AsyncClient(verify=False)
end
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
marvin-context-protocol
bot
added
bug
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/fastmcp/client/auth/oauth.py (1)
41-69: Critical: NameError in standalone function.Line 50 references
self.httpx_client_factory, butcheck_if_auth_requiredis a module-level function, not a method. The function has noselfparameter, so this will raise aNameErrorat runtime.If this function is meant to be used internally by OAuth, convert it to an instance method:
-async def check_if_auth_required( - mcp_url: str, httpx_kwargs: dict[str, Any] | None = None -) -> bool: + async def check_if_auth_required( + self, httpx_kwargs: dict[str, Any] | None = None + ) -> bool: - """ - Check if the MCP endpoint requires authentication by making a test request. + """ + Check if the MCP endpoint requires authentication by making a test request. - Returns: - True if auth appears to be required, False otherwise - """ - async with self.httpx_client_factory(**(httpx_kwargs or {})) as client: - try: - # Try a simple request to the endpoint - response = await client.get(mcp_url, timeout=5.0) + Returns: + True if auth appears to be required, False otherwise + """ + async with self.httpx_client_factory(**(httpx_kwargs or {})) as client: + try: + # Try a simple request to the endpoint + response = await client.get(self.url, timeout=5.0)If it's meant to be a standalone utility function, pass
httpx_client_factoryas a parameter instead of referencingself.src/fastmcp/client/transports.py (1)
178-192: Critical: Initialization order bug and missing OAuth parameter in SSETransport.Two related issues prevent SSETransport from correctly passing
httpx_client_factoryto OAuth:
- Line 181 assigns
self.httpx_client_factoryAFTER line 180 callsself._set_auth(auth), so the factory is unavailable when OAuth is created.- Line 189 creates OAuth without passing the
httpx_client_factoryparameter.This means SSETransport with
auth="oauth"will always use the defaulthttpx.AsyncClientinstead of the custom factory, breaking certificate verification customization described in the PR objectives.Apply this diff to fix both issues:
self.url = url self.headers = headers or {} + self.httpx_client_factory = httpx_client_factory self._set_auth(auth) - self.httpx_client_factory = httpx_client_factory if isinstance(sse_read_timeout, int | float): sse_read_timeout = datetime.timedelta(seconds=float(sse_read_timeout)) self.sse_read_timeout = sse_read_timeout def _set_auth(self, auth: httpx.Auth | Literal["oauth"] | str | None): if auth == "oauth": - auth = OAuth(self.url) + auth = OAuth(self.url, httpx_client_factory=self.httpx_client_factory) elif isinstance(auth, str): auth = BearerAuth(auth) self.auth = auth
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/fastmcp/client/auth/oauth.py(5 hunks)src/fastmcp/client/transports.py(1 hunks)tests/client/transports/test_transports.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use Python ≥ 3.10 and provide full type annotations for library code
Files:
src/fastmcp/client/auth/oauth.pysrc/fastmcp/client/transports.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Never use bare except; always catch specific exception types
Files:
src/fastmcp/client/auth/oauth.pysrc/fastmcp/client/transports.pytests/client/transports/test_transports.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Tests must be atomic, self-contained, and cover a single functionality
Use pytest parameterization for multiple examples of the same functionality
Use separate tests for distinct pieces of functionality
Always put imports at the top of test files; do not import inside test bodies
Do not add @pytest.mark.asyncio; asyncio_mode = "auto" is set globally
Prefer in-memory transport for tests; use HTTP transport only when explicitly testing networking
For slow/long-running tests, mark them as integration or optimize (default timeout is 5s)
In tests, use # type: ignore[attr-defined] for MCP results instead of type assertions
Files:
tests/client/transports/test_transports.py
🧠 Learnings (1)
📚 Learning: 2025-10-27T14:40:00.422Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-27T14:40:00.422Z
Learning: Applies to tests/**/*.py : Prefer in-memory transport for tests; use HTTP transport only when explicitly testing networking
Applied to files:
tests/client/transports/test_transports.py
🧬 Code graph analysis (2)
src/fastmcp/client/transports.py (1)
src/fastmcp/client/auth/oauth.py (1)
OAuth(135-324)
tests/client/transports/test_transports.py (1)
src/fastmcp/client/transports.py (1)
StreamableHttpTransport(229-297)
🔇 Additional comments (3)
src/fastmcp/client/auth/oauth.py (2)
143-169: LGTM!The OAuth constructor correctly accepts and stores the
httpx_client_factoryparameter with a sensible default fallback tohttpx.AsyncClient.
229-248: LGTM!The
redirect_handlercorrectly uses the configuredhttpx_client_factoryfor the pre-flight authorization URL check.src/fastmcp/client/transports.py (1)
248-262: LGTM!StreamableHttpTransport correctly assigns
self.httpx_client_factorybefore callingself._set_auth(auth)and passes it to the OAuth constructor. This ensures OAuth uses the same HTTP client configuration as the transport.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/fastmcp/client/auth/oauth.py(4 hunks)tests/client/transports/test_transports.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Never use bare except; always catch specific exception types
Files:
tests/client/transports/test_transports.pysrc/fastmcp/client/auth/oauth.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.py: Tests must be atomic, self-contained, and cover a single functionality
Use pytest parameterization for multiple examples of the same functionality
Use separate tests for distinct pieces of functionality
Always put imports at the top of test files; do not import inside test bodies
Do not add @pytest.mark.asyncio; asyncio_mode = "auto" is set globally
Prefer in-memory transport for tests; use HTTP transport only when explicitly testing networking
For slow/long-running tests, mark them as integration or optimize (default timeout is 5s)
In tests, use # type: ignore[attr-defined] for MCP results instead of type assertions
Files:
tests/client/transports/test_transports.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use Python ≥ 3.10 and provide full type annotations for library code
Files:
src/fastmcp/client/auth/oauth.py
🧠 Learnings (1)
📚 Learning: 2025-10-27T14:40:00.422Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-27T14:40:00.422Z
Learning: Applies to tests/**/*.py : Prefer in-memory transport for tests; use HTTP transport only when explicitly testing networking
Applied to files:
tests/client/transports/test_transports.py
🧬 Code graph analysis (1)
tests/client/transports/test_transports.py (2)
src/fastmcp/client/client.py (1)
Client(97-941)src/fastmcp/client/transports.py (1)
StreamableHttpTransport(229-297)
🪛 GitHub Actions: Run static analysis
tests/client/transports/test_transports.py
[error] 15-15: ruff-check: Local variable 'client' is assigned to but never used (F841).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run tests: Python 3.10 on windows-latest
- GitHub Check: Run tests with lowest-direct dependencies
|
|
||
| def _set_auth(self, auth: httpx.Auth | Literal["oauth"] | str | None): | ||
| if auth == "oauth": | ||
| auth = OAuth(self.url) |
There was a problem hiding this comment.
Does this need to be added to the SSE transport as well?
2 participants
When trying to connect to a MCP server that is in a domain without a valid certificate, the first POST to /mcp worked (with a correct 401 unauthorized) and we are moved to the authentication flow.
OAuth fails when doing the
Pre-flight checkbefore opening the browser because it was using an httpx.AsyncClient with the defaultverify=Trueeven tho we setup the Transport to use a client withverify=False.Expected behavior:
Current behavior:
Server logs (same for both approaches, but with the fix it finished the oauth):
Summary by CodeRabbit
Refactor
Tests