fix(mcp): advertise slash-less protected-resource at root mount - #1088
fix(mcp): advertise slash-less protected-resource at root mount#1088Aaron ("AJ") Steers (aaronsteers) wants to merge 2 commits into
Conversation
Co-Authored-By: AJ Steers <aj@airbyte.io>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This PyAirbyte VersionYou can test this version of PyAirbyte using the following: # Run PyAirbyte CLI from this branch:
uvx --from 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1784914272-fix-prm-root-mount-slash' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1784914272-fix-prm-root-mount-slash'PR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful ResourcesCommunity SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes an incompatibility where strict RFC 9728 MCP clients reject the advertised OAuth protected-resource URL when the MCP endpoint is mounted at / behind a path-stripping load balancer (FastMCP was producing a trailing-slash resource).
Changes:
- Add
_advertise_root_mount_resource()to normalize"/"→Nonewhen resolving the RFC 8707 resource URL, and recurse throughMultiAuthprovider trees. - Apply the normalization at startup only when the derived
mcp_path == "/"and authentication is enabled. - Add unit tests covering root vs non-root mounts and
MultiAuthrecursion.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
airbyte/mcp/http_main.py |
Adds root-mount-only normalization of the advertised protected-resource URL (including MultiAuth recursion) and applies it during HTTP startup. |
tests/unit_tests/test_mcp_http_main.py |
Adds unit tests validating slash-less root resource behavior and MultiAuth provider-tree coverage. |
Comments suppressed due to low confidence (1)
tests/unit_tests/test_mcp_http_main.py:59
- These asserts access the private
_get_resource_urlmethod and will be flagged by RuffSLF001. Suppress explicitly on each line to keep lint passing.
assert str(provider._get_resource_url("/")) == _BASE_URL
assert str(provider._get_resource_url("/mcp")) == f"{_BASE_URL}/mcp"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| _advertise_root_mount_resource(provider) | ||
|
|
||
| assert str(provider._get_resource_url(mcp_path)) == expected |
There was a problem hiding this comment.
🚫 Not fixing — false positive. ruff check passes clean on this file with SLF enabled. Ruff's flake8-self intentionally does not flag private-member access when the receiver is an instance of a class defined in the same module (_FakeProvider here), so no # noqa: SLF001 is needed. Adding one would itself trigger RUF100 (unused-noqa).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe HTTP MCP entrypoint now normalizes protected-resource URLs for authenticated root mounts, including nested ChangesRoot-mounted authentication URLs
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit_tests/test_mcp_http_main.py (1)
31-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCould we add a focused
main()wiring test?These tests exercise
_advertise_root_mount_resourcedirectly, so they would still pass ifmain()stopped calling it at the root mount. A small test covering authenticated root and non-root startup paths would protect the production branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_tests/test_mcp_http_main.py` around lines 31 - 59, Add a focused test for main() startup wiring that verifies _advertise_root_mount_resource is invoked for both authenticated root-mount and non-root configurations. Keep the existing helper behavior tests, and assert the production startup path applies the expected resource URL handling in each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@airbyte/mcp/http_main.py`:
- Line 87: Update the docstring describing the slash-less public URL and root
mount to reference RFC 9728 instead of RFC 8707, leaving the implementation
unchanged.
In `@tests/unit_tests/test_mcp_http_main.py`:
- Around line 26-28: Update the test fake’s verify_token method to use the
FastMCP-compatible AccessToken | None return annotation instead of -> None,
while preserving its behavior of always returning None. Import or reference
AccessToken using the project’s existing typing conventions.
---
Nitpick comments:
In `@tests/unit_tests/test_mcp_http_main.py`:
- Around line 31-59: Add a focused test for main() startup wiring that verifies
_advertise_root_mount_resource is invoked for both authenticated root-mount and
non-root configurations. Keep the existing helper behavior tests, and assert the
production startup path applies the expected resource URL handling in each case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e1ffa3c2-df72-4a67-bc8f-cb10993c10d3
📒 Files selected for processing (2)
airbyte/mcp/http_main.pytests/unit_tests/test_mcp_http_main.py
| async def verify_token(self, token: str) -> None: | ||
| """Never authenticates; the resource-URL logic is what is under test.""" | ||
| return None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'test_mcp_http_main.py' . || true
rg -n "class .*Dependency|verify_token|FastMCP|AccessToken" -S . --glob '!**/.git/**' --glob '!**/node_modules/**' | head -200
echo
echo "== test file excerpt =="
file="$(fd 'test_mcp_http_main.py' . | head -1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,80p' "$file" | cat -n
fi
echo
echo "== dependency definition excerpt =="
dep="$(fd -i '.*dependency.*' . | head -200 | rg 'mcp|http|fast|mcp' | head -20 || true)"
echo "$dep"
if [ -n "$dep" ]; then
for f in $dep; do
echo "--- $f ---"
wc -l "$f"
rg -n "verify_token|AccessToken|return None" "$f" -C 6 || true
done
fiRepository: airbytehq/PyAirbyte
Length of output: 8076
🌐 Web query:
FastMCP dependency injection verify_token return type AccessToken | None
💡 Result:
In FastMCP, the verify_token method, which is implemented by authentication providers to validate bearer tokens, indeed has a return type of AccessToken | None [1][2][3]. When a token is valid, it returns an AccessToken object containing information such as the client ID, scopes, expiration, and claims [1][4][5]. If the token is invalid, expired, or otherwise fails verification, it returns None [1][6][7]. Regarding dependency injection: - To access the authenticated user's token in a tool or dependency, you can use get_access_token(), which returns AccessToken | None [4][8][9]. This is useful when authentication is optional [4][10]. - For cases where you want to enforce authentication and have the dependency raise an error if no token is found, you can use CurrentAccessToken() (an injectable dependency) [4][8][10].
Citations:
- 1: https://github.com/PrefectHQ/fastmcp/blob/main/src/fastmcp/server/auth/auth.py
- 2: https://gofastmcp.com/python-sdk/fastmcp-server-auth-auth
- 3: https://github.com/jlowin/fastmcp/blob/a52ab0e9/src/fastmcp/server/auth/auth.py
- 4: https://gofastmcp.com/servers/dependency-injection
- 5: https://gofastmcp.com/servers/authorization
- 6: https://github.com/jlowin/fastmcp/blob/main/src/fastmcp/server/auth/providers/jwt.py
- 7: https://github.com/PrefectHQ/fastmcp/blob/main/src/fastmcp/server/auth/providers/jwt.py
- 8: https://github.com/jlowin/fastmcp/blob/e87ede07/src/fastmcp/server/dependencies.py
- 9: https://fastmcp.wiki/en/v2/servers/context
- 10: Add dependency injection documentation and DI-style dependencies PrefectHQ/fastmcp#2980
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== local environment availability =="
python3 - <<'PY'
try:
import fastmcp
print("fastmcp_available=true", getattr(fastmcp, "__version__", None))
except Exception as e:
print("fastmcp_available=false", repr(e))
PY
echo
echo "== installed token verifier signature/types if available =="
python3 - <<'PY'
import inspect
try:
from fastmcp.server.auth.auth import TokenVerifier
print("TokenVerifier_available=true")
sig = inspect.signature(TokenVerifier.verify_token)
print("verify_token_signature=", repr(sig))
annotations = getattr(TokenVerifier.verify_token, "__annotations__", {})
print("verify_token_annotations=", repr(annotations))
print("has_return_annotation=", "return" in annotations)
print("return_annotation_string=", str(annotations.get("return", "<no return annotation>")))
except Exception as e:
print("TokenVerifier_available=false", repr(e))
PYRepository: airbytehq/PyAirbyte
Length of output: 407
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== requirements / pyproject references =="
for f in pyproject.toml requirements.txt poetry.lock README.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n "fastmcp|fast-mcp|FastMCP" "$f" || true
fi
done
echo
echo "== vendored/fastmcp presence if any =="
git ls-files | rg 'fastmcp|FastMCP' || trueRepository: airbytehq/PyAirbyte
Length of output: 706
🌐 Web query:
FastMCP verify_token return type "async def verify_token" "AccessToken | None"
💡 Result:
In the FastMCP framework, the verify_token method is defined as an asynchronous function (async def) that returns an AccessToken object if the token is valid, or None if the token is invalid, expired, or otherwise fails validation [1][2][3]. This signature is a core requirement for classes implementing the TokenVerifier protocol [1][4][5]. When implementing a custom token verifier, your verify_token method should follow this pattern [6]: async def verify_token(self, token: str) -> AccessToken | None: # Perform validation logic if is_valid(token): return AccessToken(...) return None The AccessToken object returned contains authentication details such as the token string, client ID, granted scopes, expiration time, and additional claims [7].
Citations:
- 1: https://github.com/PrefectHQ/fastmcp/blob/main/src/fastmcp/server/auth/auth.py
- 2: https://github.com/jlowin/fastmcp/blob/a52ab0e9/src/fastmcp/server/auth/providers/jwt.py
- 3: https://github.com/jlowin/fastmcp/blob/main/src/fastmcp/server/auth/providers/jwt.py
- 4: https://gofastmcp.com/servers/auth/token-verification
- 5: https://py.sdk.modelcontextprotocol.io/authorization/
- 6: Custom TokenVerifier PrefectHQ/fastmcp#1325
- 7: https://gofastmcp.com/servers/authorization
Preserve FastMCP’s verify_token return annotation?
TokenVerifier.verify_token is AsyncCallable[..., AccessToken | None], so -> None is runtime-safe here because this fake always returns None, but -> AccessToken | None would close the static override gap. What do you think?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit_tests/test_mcp_http_main.py` around lines 26 - 28, Update the test
fake’s verify_token method to use the FastMCP-compatible AccessToken | None
return annotation instead of -> None, while preserving its behavior of always
returning None. Import or reference AccessToken using the project’s existing
typing conventions.
Source: MCP tools
There was a problem hiding this comment.
🚫 Not fixing. -> None is a valid (covariant) narrowing of the base AsyncCallable[..., AccessToken | None] return type, so it's runtime-safe and statically sound — Pyrefly is clean. This fake never returns a token; annotating -> AccessToken | None would only widen it to a value it never produces. Keeping -> None avoids an extra AccessToken import for a test whose subject is the resource-URL logic, not token verification. Happy to change it if a maintainer prefers the exact base signature.
Code Coverage OverviewLanguages: Python Python / code-coverage/pytest-fastThe overall coverage in commit 87d818d in the Show a code coverage summary of the most impacted files.
Python / code-coverage/pytest-no-credsThe overall coverage in commit 87d818d in the Show a code coverage summary of the most impacted files.
Python / code-coverage/pytestThe overall coverage in commit 87d818d in the Show a code coverage summary of the most impacted files.
Updated |
Co-Authored-By: AJ Steers <aj@airbyte.io>
| from airbyte.mcp.http_main import _advertise_root_mount_resource | ||
|
|
||
|
|
||
| _BASE_URL = "https://mcp.internal.airbyte.ai/cloud-mcp" |
Summary
Strict RFC 9728 MCP clients can't connect to the hosted MCP servers: they reject the advertised protected-resource because it carries a trailing slash the client doesn't expect.
Root cause: behind the path-stripping load balancer we mount the MCP endpoint at root (
mcp_path="/", seemain()), and FastMCP derives the RFC 8707 resource id from that mount path —_get_resource_url("/")computesbase_url.rstrip("/") + "/" + "", i.e. it appends a trailing slash (.../cloud-mcp/). The client canonicalizes the connection URL to the slash-less form and rejects the mismatch. (Lenient clients like Goose tolerated it, which is why this only surfaced on strict clients.)Fix: at a root mount only, normalize the resource computation so
"/"is treated as the root case (None), which FastMCP already resolves to the barebase_url(no slash). Non-root mounts (e.g. the local/mcpdefault) are untouched.Net effect (verified locally against the real OIDC provider tree,
MCP_SERVER_URL=https://mcp.internal.airbyte.ai/cloud-mcp):The recursion is required because
MultiAuth(interactive OIDC + headless JWT) builds the PRM document fromserverwhile theWWW-Authenticatechallenge resolves through the top-level provider — both must agree, or the client sees an inconsistent challenge.FastMCP exposes no public seam for this, so the helper wraps the private
_get_resource_url(annotated# noqa: SLF001, matching the repo's existing non-public-API-access convention). Applies to the hosted cloud-mcp deployment; the equivalent fix for ops-mcp lives in its own repo.Tests:
tests/unit_tests/test_mcp_http_main.py— parametrized over root (/,"",None) → slash-less and non-root (/mcp) → unchanged, plusMultiAuthrecursion into the interactive server and headless verifiers.Link to Devin session: https://app.devin.ai/sessions/a5b9501ef92c412aad0408b7c74ef9c8
Requested by: Aaron ("AJ") Steers (@aaronsteers)
Summary by CodeRabbit