Skip to content

Conversation

LiangYang666
Copy link

This PR adds support for root_path in subpath routing of SSE andstreamable-http endpoints when using Nginx or other HTTP middleware as a reverse proxy.

Motivation and Context

When deploying applications behind a reverse proxy (e.g., Nginx), it's common to route requests via subpaths like /server1/ or /server2/ . Without proper handling of these subpaths, SSE and streamable-http endpoints may fail to receive correctly routed requests.

This issue was originally reported in #242

Similar functionality exists in other web applications:

How Has This Been Tested?

Yes, this has been tested and running stably in production environments for several days. Both sse and streamable-http endpoints have been verified to work correctly under subpaths behind Nginx.

Breaking Changes

No breaking changes are introduced. Users do not need to update their code or configuration files. This is an optional enhancement that activates only when root_path is provided.

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

This change ensures that the application respects the root_path setting for streaming endpoints by utilizing Uvicorn’s built-in support for it. It does not introduce any custom routing logic but instead makes sure that streaming responses behave consistently with regular HTTP responses under subpath deployments.

@LiangYang666
Copy link
Author

cc @ihrpr

@felixweinberger felixweinberger self-assigned this Jul 15, 2025
@felixweinberger felixweinberger requested review from felixweinberger and removed request for felixweinberger July 15, 2025 12:48
gspencergoog pushed a commit to gspencergoog/mcp-python-sdk that referenced this pull request Jul 29, 2025
…t-oidc-discovery

feat: enhance auth server discovery with OAuth2 and OpenID metadata support
Copy link

@lukehsiao lukehsiao left a comment

Choose a reason for hiding this comment

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

We're running into this issue as well. This seems an elegant fix!

Copy link

@lukehsiao lukehsiao left a comment

Choose a reason for hiding this comment

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

In our case, we'd want this to be provided in the FastMCP initializer, as well. Can we expose that there?

Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

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

Hi @LiangYang666 thank you for this contribution! And apologies for the time it took to get back to this.

There've been quite a few changes to FastMCP since this PR and it looks like FastMCP.__init__() now supports an argument mount_path="/" - I wonder if that potentially already addresses your use case here?

See:

mount_path: str = "/",

@LiangYang666 @lukehsiao keen to figure out if this may already support your use cases or where specifically it falls short. If it doesn't it would be excellent to create a test case that demonstrates where it falls short + ensure we stay backwards compatible.

@felixweinberger felixweinberger added needs confirmation Needs confirmation that the PR is actually required or needed. needs sync Needs sync with latest main branch to ensure CI passes needs more work Not ready to be merged yet, needs additional changes. labels Sep 8, 2025
@lukehsiao
Copy link

@felixweinberger, I confirmed that mount_path does NOT address the use case. In fact, I don't see any difference in behavior.

However, I did find a different workaround. In my particular case, I'm running FastMCP mounted to a FastAPI server, that is served behind a proxy, which strips prefixes.

It turns out FastAPI has a bug, which, if I workaround that, also seems to resolve the issue here.

In this setting, FastAPI docs instruct us to provide root_path:

Passing the root_path to FastAPI would be the equivalent of passing
the --root-path command line option to Uvicorn or Hypercorn.

This is wrong. It is NOT equivalent.

There is a very old PR to address this: fastapi/fastapi#11160

Long story short, switching to providing --root-path via command line option, rather than as a parameter to FastAPI, also resolved our issue here.

@felixweinberger
Copy link
Contributor

@felixweinberger, I confirmed that mount_path does NOT address the use case. In fact, I don't see any difference in behavior.

However, I did find a different workaround. In my particular case, I'm running FastMCP mounted to a FastAPI server, that is served behind a proxy, which strips prefixes.

It turns out FastAPI has a bug, which, if I workaround that, also seems to resolve the issue here.

In this setting, FastAPI docs instruct us to provide root_path:

Passing the root_path to FastAPI would be the equivalent of passing
the --root-path command line option to Uvicorn or Hypercorn.

This is wrong. It is NOT equivalent.

There is a very old PR to address this: fastapi/fastapi#11160

Long story short, switching to providing --root-path via command line option, rather than as a parameter to FastAPI, also resolved our issue here.

Thank you for reporting back and the additional details on your workaround!

Given this sounds like the workaround needed to address a FastAPI specific issue, I'm going to close this PR for now. If you find yourself needing this feature in future for some other purpose, happy to look at a resubmission.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs confirmation Needs confirmation that the PR is actually required or needed. needs more work Not ready to be merged yet, needs additional changes. needs sync Needs sync with latest main branch to ensure CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants