Skip to content

Conversation

SOURABHMISHRA5221
Copy link

@SOURABHMISHRA5221 SOURABHMISHRA5221 commented Jul 28, 2025

Summary

Fix SSE endpoint path handling to avoid urllib.parse.urljoin() truncating base paths when servers are behind proxies or mounted at subpaths. Endpoints are treated as relative path segments and stored verbatim.

Motivation

When clients join a base URL with an endpoint starting with “/”, urljoin resets to the host root. This caused paths like http://localhost:8000/some/path/to/sse + /messages/http://localhost:8000/messages/. Using a relative segment preserves the base path. Resolves #200.

from urllib.parse import urljoin
urljoin("http://host/hello/world", "/messages/")  # -> http://host/messages/
urljoin("http://host/hello/world/", "messages/")  # -> http://host/hello/world/messages/

What changed

  • mcp/server/sse.py
    • Store endpoint verbatim (no auto-leading “/”).
    • Validate endpoint is a relative path (no scheme/host/query/fragment).
    • Add _build_message_path(root_path) to construct the final path robustly from the app mount and root_path.
    • Update comments/docstrings to emphasize relative path semantics and predictable URL construction.
  • Tests
    • tests/server/test_sse_security.py: clarify accept/reject cases and document urljoin behavior.
    • tests/shared/test_sse.py: add/clarify param cases; ensure endpoints are stored verbatim.

Backward compatibility

  • No breaking changes.

How it was tested

  • Endpoint validation tests for relative vs absolute and query/fragment rejection.
  • Subpath mounting tests to reproduce and prevent original truncation.
  • All existing SSE security/functionality tests pass.
  • Verified path construction under various root_path combinations.

Checklist

  • Follows repo style guidelines
  • Tests added/updated and passing
  • Error handling and docs updated

@SOURABHMISHRA5221 SOURABHMISHRA5221 requested review from a team as code owners July 28, 2025 19:37
@SOURABHMISHRA5221
Copy link
Author

Hi @Kludex!... Can you please review this...

Comment on lines 42 to 43
- With root_path="/api" and endpoint="/messages/": Final path = "/api/messages/"
- With root_path="/api" and endpoint="messages/": Final path = "/api/messages/"

Choose a reason for hiding this comment

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

The behavior I saw was: urllib.join("http://example.com/some/path", "/messages/") resulted in http://example.com/messages. I think it would be sufficient to just drop the leading forward slash sense that leads to an unexpected behavior. I would argue that the sdk should not be forcing its route to be at any particular location and that a dev should decide where it gets mounted. Relative path joining should allow that. See comment below for test cases

Copy link
Author

Choose a reason for hiding this comment

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

Hi @mconflitti-pbc! Thanks for reviewing this PR, and apologies for the late reply.

You’re right about the behavior: auto-prepending “/” to “make it relative” actually made the endpoint origin-absolute, which caused urljoin to drop any base path in proxied/subpath setups. This change removes that normalization and stores the endpoint verbatim.

  • We validate only that the endpoint is a relative path (no scheme/host/query/fragment).
  • Both “/messages/” and “messages/” are accepted; we recommend the relative segment (“messages/”) for predictable joining.
  • The final URL is composed from the app’s mount and ASGI root_path, so mounting remains entirely up to the developer.

Thanks again for the thoughtful feedback.

Comment on lines 313 to 318
valid_endpoints_and_expected = [
("/messages/", "/messages/"), # Absolute path format
("messages/", "messages/"), # Relative path format
("/api/v1/messages/", "/api/v1/messages/"),
("api/v1/messages/", "api/v1/messages/"),
]

Choose a reason for hiding this comment

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

Here is the behavior demonstrated with examples:

>>> from urllib.parse import urljoin
>>> 
>>> urljoin("http://www.google.com/hello/world", "/messages")
'http://www.google.com/messages'
>>> urljoin("http://www.google.com/hello/world", "messages")
'http://www.google.com/hello/messages'
>>> urljoin("http://www.google.com/hello/world/", "messages")
'http://www.google.com/hello/world/messages'
>>> urljoin("http://www.google.com/hello/world/", "/messages")
'http://www.google.com/messages'
>>> urljoin("http://www.google.com/hello/world/", "/messages/")
'http://www.google.com/messages/'
>>> urljoin("http://www.google.com/hello/world/", "messages/")
'http://www.google.com/hello/world/messages/'
>>> urljoin("http://www.google.com/hello/world", "messages/")
'http://www.google.com/hello/messages/'

urllib has some odd behavior imo.

Choose a reason for hiding this comment

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

I believe this is the one we want to coerce/enforce:

>>> urljoin("http://www.google.com/hello/world/", "messages/")
'http://www.google.com/hello/world/messages/'

Copy link
Author

Choose a reason for hiding this comment

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

I’ve made the necessary updates based on your feedback (store endpoint verbatim, remove leading “/” coercion, clarify docs/tests). When you have a moment, could you please take another look? Thank you

@mconflitti-pbc
Copy link

It appears there is already some normalization logic added to the repo?

def _normalize_path(self, mount_path: str, endpoint: str) -> str:

Also, SSE is deprecated now so it may be worth reworking this for streamable http

@DigdashQuentinLandi
Copy link

It appears there is already some normalization logic added to the repo?

def _normalize_path(self, mount_path: str, endpoint: str) -> str:

Also, SSE is deprecated now so it may be worth reworking this for streamable http

hi , is the issue still open on Streamable http?

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 @SOURABHMISHRA5221 thank you for this contribution! And apologies for the time it took to get back to you on this.

As it's been a while, is this still an issue you're seeing? As other commenters above mention there maye have already been some normalization logic added in more recent iterations of the SDK, so would be great to confirm this is still an issue you're experiencing?

@felixweinberger felixweinberger added bug Something isn't working needs confirmation Needs confirmation that the PR is actually required or needed. labels Sep 23, 2025
@SOURABHMISHRA5221
Copy link
Author

Hi @felixweinberger and others,
Yes, @mconflitti-pbc rightly pointed out there's already a _normalize_path method at line 693 in src/mcp/server/fastmcp/server.py that handles mount path normalization. After examining the codebase more carefully, I realize the StreamableHTTPServerTransport (current transport) doesn't have this URL joining problem , it uses a single endpoint with content negotiation via Accept headers, so no urljoin() occurs in the client. This PR only addresses the deprecated SSE transport, which sends separate "endpoint" events that trigger the URL joining issue. Would you prefer I close this PR since it only affects deprecated code, or keep it for users still transitioning from SSE transport?

@felixweinberger
Copy link
Contributor

Hi @felixweinberger and others, Yes, @mconflitti-pbc rightly pointed out there's already a _normalize_path method at line 693 in src/mcp/server/fastmcp/server.py that handles mount path normalization. After examining the codebase more carefully, I realize the StreamableHTTPServerTransport (current transport) doesn't have this URL joining problem , it uses a single endpoint with content negotiation via Accept headers, so no urljoin() occurs in the client. This PR only addresses the deprecated SSE transport, which sends separate "endpoint" events that trigger the URL joining issue. Would you prefer I close this PR since it only affects deprecated code, or keep it for users still transitioning from SSE transport?

Thanks for this additional detail - in this case I'd prefer keeping things simple and focusing on StreamableHTTP and close this one. I don't think we'll have the bandwidth to maintain deprecated functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs confirmation Needs confirmation that the PR is actually required or needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server url truncated by urllib in FastMCP
4 participants