feat(server/sse): Add support for dynamic base paths #214
Conversation
WalkthroughThis change introduces dynamic base path support for the SSE server in the Changes
Possibly related PRs
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
examples/dynamic_path/main.go (2)
26-35: Avoid hard-coding the outbound baseURL – derive it from the chosen listen address instead
WithBaseURL("http://localhost:8080")will break if the user changes the-addrflag (:9090,0.0.0.0:8081, etc.).
Deriving the value once the flag is parsed prevents “wrong endpoint” bugs in copy-pasted examples.- server.WithBaseURL("http://localhost:8080"), + server.WithBaseURL(fmt.Sprintf("http://localhost%s", addr)),(If TLS might be enabled in the future, you could inspect
addror add a separate-base-urlflag.)
38-40: Router pattern requires Go 1.22 – add a build tag or explicit comment
ServeMuxpatterns that contain{tenant}only compile/run on Go 1.22+.
A short doc-comment or a//go:build go1.22guard will save users on earlier versions from a confusing 404.server/sse.go (3)
331-333: Ensure the initial endpoint event always ends with\n\n
fmt.Fprintf(w, "event: endpoint\ndata: %s\r\n\r\n", endpoint)mixes\r\nand\n.
For consistency with the rest of the stream (eventQueueuses\n\n), prefer\n\n. No functional bug but helps downstream parsers.- fmt.Fprintf(w, "event: endpoint\ndata: %s\r\n\r\n", endpoint) + fmt.Fprintf(w, "event: endpoint\ndata: %s\n\n", endpoint)
478-483: Return errors instead of panicking for library callers
panichere abruptly terminates the program if library users accidentally call the wrong helper.
A safer API is to return an explicit error so callers can decide.-func (s *SSEServer) CompleteSseEndpoint() string { - if s.dynamicBasePathFunc != nil { - panic("CompleteSseEndpoint cannot be used with WithDynamicBasePath. Use dynamic path logic in your router.") - } - return s.baseURL + s.basePath + s.sseEndpoint -} +func (s *SSEServer) CompleteSseEndpoint() (string, error) { + if s.dynamicBasePathFunc != nil { + return "", fmt.Errorf("CompleteSseEndpoint cannot be used together with WithDynamicBasePath") + } + return s.baseURL + s.basePath + s.sseEndpoint, nil +}(Change callers accordingly.)
Panics are still appropriate inside tests, but exported APIs should favour explicit errors.
567-569: ServeHTTP panic could be downgraded to 500 for robustnessIf someone accidentally wires the whole server under a dynamic path, the entire process panics. Returning
http.StatusInternalServerError(or a log +500) avoids bringing down shared servers.server/sse_test.go (2)
946-968:SSEHandlerRequiresDynamicBasePathsilently ignores earlier panic expectationThe first two calls (
_ = sseServer.SSEHandler()) should be wrapped inrequire.NotPanicsto document the intention; otherwise a panic would skip the rest of the test and the later recover logic never triggers, leading to false positives.
1009-1044: Duplicate code for static vs dynamic endpoint assertions – factor helperFour almost-identical blocks differ only by constructor options. Extracting a table-driven sub-test improves readability and future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/dynamic_path/main.go(1 hunks)server/sse.go(9 hunks)server/sse_test.go(1 hunks)
robert-jackson-glean
force-pushed
the
rwjblue/push-ysoppsqxnkmy
branch
from
887b314 to
15e1a5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
server/sse_test.go (1)
453-469: Improved SSE frame reading with buffered reader loopExcellent change that addresses the previous review feedback about potential truncation issues. Using a buffered reader loop ensures the complete SSE frame is captured reliably, even on slower systems where the frame might be split across multiple TCP packets.
🧹 Nitpick comments (1)
server/sse_test.go (1)
982-1020: Replace simple buffer read with buffered reader for consistencyWhile this test case correctly verifies dynamic base path with full URL configuration, it uses a simple buffered read instead of the more robust buffered reader loop introduced earlier in the file.
Consider replacing the simple buffer read with a buffered reader loop for consistency and to prevent potential SSE frame truncation issues:
- buf := make([]byte, 1024) - n, err := resp.Body.Read(buf) - if err != nil { - t.Fatalf("Failed to read SSE response: %v", err) - } - endpointEvent := string(buf[:n]) + reader := bufio.NewReader(resp.Body) + var endpointEvent strings.Builder + for { + line, err := reader.ReadString('\n') + if err != nil { + t.Fatalf("Failed to read SSE response: %v", err) + } + endpointEvent.WriteString(line) + if line == "\n" || line == "\r\n" { + break // End of SSE frame + } + } + endpointEventStr := endpointEvent.String()Then update the subsequent code to use
endpointEventStrinstead ofendpointEvent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/dynamic_path/main.go(1 hunks)server/sse.go(9 hunks)server/sse_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/dynamic_path/main.go
- server/sse.go
🔇 Additional comments (4)
server/sse_test.go (4)
865-957: Well-structured test for dynamic path mountingThis test case thoroughly verifies the new dynamic base path functionality, correctly testing both the SSE endpoint connection and JSON-RPC message communication with tenant-specific paths.
Note that the test uses Go 1.22+
PathValue()function, which should be mentioned in documentation as a dependency requirement.
958-980: Important safety check for dynamic path configurationGood test ensuring that the API enforces correct usage patterns - SSEHandler and MessageHandler work correctly in both modes, while ServeHTTP properly panics when configured with dynamic paths to prevent misuse.
1022-1057: Comprehensive URL generation test for all configuration combinationsGreat test case that verifies the URL generation logic for all combinations of static vs dynamic paths, with and without base URL, and with full URL usage enabled or disabled. This ensures consistent behavior across all configuration patterns.
1059-1070: Important API contract enforcementGood test verifying that CompleteSseEndpoint correctly panics when used with dynamic base paths, enforcing the API contract and preventing misuse.
robert-jackson-glean
force-pushed
the
rwjblue/push-ysoppsqxnkmy
branch
from
15e1a5e to
252d7d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/sse.go (1)
368-371: Potential edge case in path concatenationWhile path sanitization in
WithDynamicBasePathprevents most issues, the direct concatenation ofbasePath + s.messageEndpointmight still produce unexpected results ifs.messageEndpointdoesn't begin with a slash.Consider using a more robust path joining approach:
- endpointPath := basePath + s.messageEndpoint + // Ensure messageEndpoint starts with a slash for proper concatenation + messageEndpoint := s.messageEndpoint + if !strings.HasPrefix(messageEndpoint, "/") { + messageEndpoint = "/" + messageEndpoint + } + endpointPath := basePath + messageEndpoint
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/dynamic_path/main.go(1 hunks)server/sse.go(9 hunks)server/sse_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/dynamic_path/main.go
- server/sse_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: robert-jackson-glean
PR: mark3labs/mcp-go#214
File: server/sse.go:0-0
Timestamp: 2025-04-28T00:14:49.237Z
Learning: The SSE server in mcp-go implements path sanitization within the `WithDynamicBasePath` function that ensures the dynamic base path starts with "/" and has no trailing "/" to prevent double slashes in URL construction.
server/sse.go (1)
Learnt from: robert-jackson-glean
PR: mark3labs/mcp-go#214
File: server/sse.go:0-0
Timestamp: 2025-04-28T00:14:49.237Z
Learning: The SSE server in mcp-go implements path sanitization within the `WithDynamicBasePath` function that ensures the dynamic base path starts with "/" and has no trailing "/" to prevent double slashes in URL construction.
🔇 Additional comments (9)
server/sse.go (9)
36-41: Well-defined type for dynamic base path handlingThe new
DynamicBasePathFunctype is clearly documented and serves an important purpose in enabling multi-tenant and dynamic routing scenarios.
79-80: LGTM - Clear field documentationThe field comment clearly explains the purpose of this function pointer for determining dynamic base paths.
120-136: Good implementation with path sanitizationThe implementation of
WithDynamicBasePathis robust with proper path sanitization that ensures consistent path formats (adding leading slash when missing and removing trailing slashes). This prevents URL construction issues.
359-374: Clean implementation of dynamic path support in endpoint generationThe updated
GetMessageEndpointForClientmethod properly handles both static and dynamic base paths with a clear implementation. The method intelligently applies the dynamic base path function when available.
485-488: Good defensive programming with clear error guidanceThe panic with a clear error message helps prevent misuse of the API by enforcing the correct usage pattern when using dynamic base paths.
501-503: Consistent API enforcement patternConsistent with the implementation in
CompleteSseEndpoint, ensuring that these methods can't be misused with dynamic base paths.
515-571: Excellent handler abstractions with comprehensive documentationThe new
SSEHandlerandMessageHandlermethods are well-designed and thoroughly documented with example usage. They provide clean abstractions for integrating with various HTTP routers and supporting dynamic path segments.
575-577: Strong API consistency enforcementThe panic in
ServeHTTPproperly prevents API misuse and clearly guides users toward the correct approach for dynamic path scenarios.
339-340: LGTM - Updated handleSSE to support dynamic pathsProperly updated to pass the request to
GetMessageEndpointForClient, enabling dynamic base path resolution.
robert-jackson-glean
force-pushed
the
rwjblue/push-ysoppsqxnkmy
branch
from
252d7d8 to
a643f22
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/sse.go (1)
364-366: Consider additional validation for dynamic path usageWhile you check if
dynamicBasePathFuncis non-nil, there's no validation that the returned path is non-empty. An empty path could lead to URL construction issues.if s.dynamicBasePathFunc != nil { basePath = s.dynamicBasePathFunc(r, sessionID) + if basePath == "" { + // Log warning or use default + basePath = "/" + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/dynamic_path/main.go(1 hunks)server/sse.go(9 hunks)server/sse_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/dynamic_path/main.go
- server/sse_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: robert-jackson-glean
PR: mark3labs/mcp-go#214
File: server/sse.go:0-0
Timestamp: 2025-04-28T00:14:49.237Z
Learning: The SSE server in mcp-go implements path sanitization within the `WithDynamicBasePath` function that ensures the dynamic base path starts with "/" and has no trailing "/" to prevent double slashes in URL construction.
server/sse.go (1)
Learnt from: robert-jackson-glean
PR: mark3labs/mcp-go#214
File: server/sse.go:0-0
Timestamp: 2025-04-28T00:14:49.237Z
Learning: The SSE server in mcp-go implements path sanitization within the `WithDynamicBasePath` function that ensures the dynamic base path starts with "/" and has no trailing "/" to prevent double slashes in URL construction.
🔇 Additional comments (8)
server/sse.go (8)
36-41: Well-documented type for dynamic base path generationGood definition with clear documentation explaining the purpose and expected behavior of the function type.
79-81: Clean field addition with descriptive commentThe new field for storing the dynamic base path function is well-commented and properly placed in the struct.
120-136: Excellent path sanitization implementationThe implementation correctly normalizes paths by ensuring they start with a slash and don't end with one, preventing potential URL construction issues. This is consistent with the learning from a previous review about preventing double slashes.
359-374: Clean implementation of dynamic endpoint generationThe method correctly handles both static and dynamic paths, with proper fallback and consistent URL construction. The implementation is straightforward and maintains backward compatibility.
485-489: Good enforcement of usage patternsAppropriate panic messages when attempting to use static methods with dynamic paths, guiding users toward the correct pattern. This helps prevent subtle bugs that could arise from mixing the two approaches.
Also applies to: 501-504
515-542: Well-documented handler for SSE endpointThe SSEHandler method has excellent documentation with clear examples that show how to use it with dynamic paths. The examples are particularly helpful for showing the integration with routers.
544-571: Well-documented handler for message endpointSimilar to the SSE handler, the documentation is thorough with relevant examples. The consistent approach between both handlers makes the API intuitive to use.
575-577: Clear enforcement of the correct usage patternThe panic message explicitly directs users to the appropriate methods for dynamic path scenarios, preventing misuse of the API.
robert-jackson-glean
force-pushed
the
rwjblue/push-ysoppsqxnkmy
branch
from
a643f22 to
86cebe8
Compare
robert-jackson-glean
force-pushed
the
rwjblue/push-ysoppsqxnkmy
branch
from
86cebe8 to
4abd640
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/sse.go (1)
368-371: Potential path construction issue with endpointPathThe current concatenation
basePath + s.messageEndpointmight lead to double slashes ifs.messageEndpointstarts with a slash, or missing slashes if it doesn't.Consider using string concatenation that ensures exactly one slash between path segments:
- endpointPath := basePath + s.messageEndpoint + // Ensure exactly one slash between segments + if strings.HasSuffix(basePath, "/") { + if strings.HasPrefix(s.messageEndpoint, "/") { + endpointPath = basePath + s.messageEndpoint[1:] + } else { + endpointPath = basePath + s.messageEndpoint + } + } else { + if strings.HasPrefix(s.messageEndpoint, "/") { + endpointPath = basePath + s.messageEndpoint + } else { + endpointPath = basePath + "/" + s.messageEndpoint + } + }Or more simply with path cleaning:
+import "path" // ... - endpointPath := basePath + s.messageEndpoint + endpointPath := path.Join(basePath, s.messageEndpoint) + // path.Join strips the leading slash; restore it + if !strings.HasPrefix(endpointPath, "/") { + endpointPath = "/" + endpointPath + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/dynamic_path/main.go(1 hunks)server/sse.go(9 hunks)server/sse_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/dynamic_path/main.go
- server/sse_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: robert-jackson-glean
PR: mark3labs/mcp-go#214
File: server/sse.go:0-0
Timestamp: 2025-04-28T00:14:49.237Z
Learning: The SSE server in mcp-go implements path sanitization within the `WithDynamicBasePath` function that ensures the dynamic base path starts with "/" and has no trailing "/" to prevent double slashes in URL construction.
server/sse.go (1)
Learnt from: robert-jackson-glean
PR: mark3labs/mcp-go#214
File: server/sse.go:0-0
Timestamp: 2025-04-28T00:14:49.237Z
Learning: The SSE server in mcp-go implements path sanitization within the `WithDynamicBasePath` function that ensures the dynamic base path starts with "/" and has no trailing "/" to prevent double slashes in URL construction.
🔇 Additional comments (10)
server/sse.go (10)
36-41: Well-documented new typeDynamicBasePathFuncThis new type definition is well-documented with a clear description of its purpose and expected usage. The function signature is appropriately designed to accept both the HTTP request and session ID as parameters, providing the necessary context for computing dynamic paths.
79-80: Well-placed field addition for dynamic base path functionThe SSEServer struct is appropriately extended with the dynamicBasePathFunc field to store the user-provided function. The comment accurately describes its purpose.
120-136: Good implementation of path sanitization inWithDynamicBasePathThe implementation correctly wraps the user-provided function to ensure path normalization (adding leading slash if missing and removing trailing slash). This aligns with the retrieved learning and prevents URL construction issues.
339-340: Correct update to handleSSE for dynamic base path supportThe code now correctly passes the HTTP request to
GetMessageEndpointForClient, which is necessary for dynamic base path resolution in the initial endpoint event.
362-374: Robust implementation ofGetMessageEndpointForClientThe method now properly handles both static and dynamic base paths. The logic is clear and correctly applies the configured options, including the full URL usage flag.
486-488: Clear boundary enforcement inCompleteSseEndpointThe panic message clearly indicates that this method shouldn't be used with dynamic base paths and suggests the correct approach.
However, based on a previous review comment, there was a discussion about avoiding panics in favor of standard Go error patterns.
Consider using an error return instead of a panic, as discussed in previous feedback:
-func (s *SSEServer) CompleteSseEndpoint() string { +func (s *SSEServer) CompleteSseEndpoint() (string, error) { if s.dynamicBasePathFunc != nil { - panic("CompleteSseEndpoint cannot be used with WithDynamicBasePath. Use dynamic path logic in your router.") + return "", fmt.Errorf("CompleteSseEndpoint cannot be used with WithDynamicBasePath; use dynamic path logic in your router") } - return s.baseURL + s.basePath + s.sseEndpoint + return s.baseURL + s.basePath + s.sseEndpoint, nil }
501-503: Clear boundary enforcement inCompleteMessageEndpointSimilar to
CompleteSseEndpoint, this method uses a panic to clearly indicate that it's incompatible with dynamic base paths.The same concern applies here regarding the use of panics.
Consider using an error return instead of a panic, as discussed in previous feedback:
-func (s *SSEServer) CompleteMessageEndpoint() string { +func (s *SSEServer) CompleteMessageEndpoint() (string, error) { if s.dynamicBasePathFunc != nil { - panic("CompleteMessageEndpoint cannot be used with WithDynamicBasePath. Use dynamic path logic in your router.") + return "", fmt.Errorf("CompleteMessageEndpoint cannot be used with WithDynamicBasePath; use dynamic path logic in your router") } - return s.baseURL + s.basePath + s.messageEndpoint + return s.baseURL + s.basePath + s.messageEndpoint, nil }
515-543: Well-documentedSSEHandlermethod with clear usage examplesThis new method provides a clear, clean interface for mounting the SSE handler at any path. The documentation is excellent, with helpful examples and important usage notes. The implementation is simple and delegates to the existing handler function.
544-571: Well-documentedMessageHandlermethod with clear usage examplesLike the
SSEHandlermethod, this provides a clean interface for mounting the message handler. The documentation is clear and includes helpful examples that match those in theSSEHandlerdocumentation for consistency.
575-577: Clear boundary enforcement inServeHTTPThe method correctly prevents the use of
ServeHTTPwith dynamic base paths, directing users to the appropriate alternative methods.However, similar to the other methods, the use of panic here could be reconsidered.
Consider handling this case with an error response instead of a panic:
func (s *SSEServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { if s.dynamicBasePathFunc != nil { - panic("ServeHTTP cannot be used with WithDynamicBasePath. Use SSEHandler/MessageHandler and mount them with your router.") + http.Error(w, "ServeHTTP cannot be used with WithDynamicBasePath. Use SSEHandler/MessageHandler and mount them with your router.", http.StatusInternalServerError) + return } // Rest of the method... }
robert-jackson-glean
force-pushed
the
rwjblue/push-ysoppsqxnkmy
branch
from
4abd640 to
304fe1a
Compare
This change introduces the ability to mount SSE endpoints at dynamic
paths with variable segments (e.g., `/api/{tenant}/sse`) by adding a new
`WithDynamicBasePath` option and related functionality. This enables
advanced use cases such as multi-tenant architectures or integration
with routers that support path parameters.
Key Features:
* DynamicBasePathFunc: New function type and option
(WithDynamicBasePath) to generate the SSE server's base path
dynamically per request/session.
* Flexible Routing: New SSEHandler() and MessageHandler() methods allow
mounting handlers at arbitrary or dynamic paths using any router
(e.g., net/http, chi, gorilla/mux).
* Endpoint Generation: GetMessageEndpointForClient now supports both
static and dynamic path modes, and correctly generates full URLs when
configured.
* Example: Added examples/dynamic_path/main.go demonstrating dynamic
path mounting and usage.
```go
mcpServer := mcp.NewMCPServer("dynamic-path-example", "1.0.0")
sseServer := mcp.NewSSEServer(
mcpServer,
mcp.WithDynamicBasePath(func(r *http.Request, sessionID string) string {
tenant := r.PathValue("tenant")
return "/api/" + tenant
}),
mcp.WithBaseURL("http://localhost:8080"),
)
mux := http.NewServeMux()
mux.Handle("/api/{tenant}/sse", sseServer.SSEHandler())
mux.Handle("/api/{tenant}/message", sseServer.MessageHandler())
```
robert-jackson-glean
force-pushed
the
rwjblue/push-ysoppsqxnkmy
branch
from
304fe1a to
ebb4e67
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/dynamic_path/main.go(1 hunks)server/errors.go(2 hunks)server/sse.go(7 hunks)server/sse_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- server/errors.go
- examples/dynamic_path/main.go
- server/sse_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: robert-jackson-glean
PR: mark3labs/mcp-go#214
File: server/sse.go:0-0
Timestamp: 2025-04-28T00:14:49.237Z
Learning: The SSE server in mcp-go implements path sanitization within the `WithDynamicBasePath` function that ensures the dynamic base path starts with "/" and has no trailing "/" to prevent double slashes in URL construction.
server/sse.go (1)
Learnt from: robert-jackson-glean
PR: mark3labs/mcp-go#214
File: server/sse.go:0-0
Timestamp: 2025-04-28T00:14:49.237Z
Learning: The SSE server in mcp-go implements path sanitization within the `WithDynamicBasePath` function that ensures the dynamic base path starts with "/" and has no trailing "/" to prevent double slashes in URL construction.
🧬 Code Graph Analysis (1)
server/sse.go (2)
mcp/types.go (1)
Request(103-116)server/errors.go (1)
ErrDynamicPathConfig(27-29)
🔇 Additional comments (8)
server/sse.go (8)
36-41: Well-designed interface for dynamic base path generationThe
DynamicBasePathFunctype definition provides a clear and flexible interface for generating base paths dynamically. The documentation clearly explains the purpose and expected return values.
79-81: Good field documentation for the dynamic base path functionThe comment clearly indicates the purpose of this field in the SSEServer struct.
120-136: Excellent path sanitization implementationThe path sanitization logic ensures base paths always start with a leading slash and have no trailing slash, preventing URL construction issues. This implementation matches the learnings from previous PRs and properly handles nil functions.
485-490: Good error handling for incompatible dynamic configurationReturning a specialized error when attempting to use static path methods with dynamic configuration is a clean approach that provides clear guidance to users.
504-509: Consistent error handling patternThe error handling approach is consistent with
CompleteSseEndpoint, providing a unified experience across the API.
523-550: Comprehensive documentation with practical examplesThe documentation for
SSEHandleris excellent - it clearly explains the purpose, usage scenarios, and provides concrete examples. The emphasis on usingWithDynamicBasePathwith these handlers is important for correctness.
552-579: Consistent handler implementation and documentationThe
MessageHandlerimplementation follows the same pattern asSSEHandler, with equally thorough documentation.
583-586: Appropriate enforcement of usage patternsReturning an HTTP 500 error with a clear message when attempting to use
ServeHTTPwith dynamic configuration is a good way to enforce correct usage patterns and guide users to the appropriate methods.
| // GetMessageEndpointForClient returns the appropriate message endpoint URL with session ID | ||
| // based on the useFullURLForMessageEndpoint configuration. | ||
| func (s *SSEServer) GetMessageEndpointForClient(sessionID string) string { | ||
| messageEndpoint := s.messageEndpoint | ||
| if s.useFullURLForMessageEndpoint { | ||
| messageEndpoint = s.CompleteMessageEndpoint() | ||
| // for the given request. This is the canonical way to compute the message endpoint for a client. | ||
| // It handles both dynamic and static path modes, and honors the WithUseFullURLForMessageEndpoint flag. | ||
| func (s *SSEServer) GetMessageEndpointForClient(r *http.Request, sessionID string) string { | ||
| basePath := s.basePath | ||
| if s.dynamicBasePathFunc != nil { | ||
| basePath = s.dynamicBasePathFunc(r, sessionID) | ||
| } | ||
|
|
||
| endpointPath := basePath + s.messageEndpoint | ||
| if s.useFullURLForMessageEndpoint && s.baseURL != "" { | ||
| endpointPath = s.baseURL + endpointPath | ||
| } | ||
| return fmt.Sprintf("%s?sessionId=%s", messageEndpoint, sessionID) | ||
|
|
||
| return fmt.Sprintf("%s?sessionId=%s", endpointPath, sessionID) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that path concatenation doesn't create double slashes
While the sanitization in WithDynamicBasePath should prevent issues with dynamic paths, the direct concatenation with basePath + s.messageEndpoint could still result in malformed paths if s.messageEndpoint starts with a slash.
🏁 Script executed:
#!/bin/bash
# Examine the initialization of messageEndpoint to verify slash handling
fd -t f -e "go" --exec grep -A 5 -B 5 "messageEndpoint.*:=" {}Length of output: 901
The previous grep didn’t surface any matches—let’s locate where messageEndpoint is declared and how it’s initialized/sanitized:
🏁 Script executed:
#!/bin/bash
# 1. Find all assignments of the messageEndpoint field across Go files
rg -n "messageEndpoint:" --glob "*.go"
# 2. Show the SSEServer struct definition (to see default type/placement of messageEndpoint)
rg -n "type SSEServer" -A 20 server/sse.go
# 3. Inspect the WithMessageEndpoint option setter for any slash normalization
rg -n "WithMessageEndpoint" -A 5 -B 5 server/sse.goLength of output: 1121
Let’s verify how basePath is initialized and whether WithBasePath or WithDynamicBasePath normalize slashes:
#!/bin/bash
# 1. Inspect NewSSEServer to see the default basePath value
rg -n "func NewSSEServer" -A 20 -B 20 server/sse.go
# 2. Check WithBasePath setter for any trimming logic
rg -n "WithBasePath" -A 5 -B 5 server/sse.go
# 3. Check WithDynamicBasePath setter for any trimming logic
rg -n "WithDynamicBasePath" -A 5 -B 5 server/sse.goLet’s verify how basePath is initialized and whether the WithBasePath/WithDynamicBasePath options normalize trailing or leading slashes:
#!/bin/bash
# 1. Inspect NewSSEServer to see the default basePath
rg -n "func NewSSEServer" -A 20 server/sse.go
# 2. Check WithBasePath setter for trimming or normalization
rg -n "WithBasePath" -A 5 -B 5 server/sse.go
# 3. Check WithDynamicBasePath setter for any slash normalization
rg -n "WithDynamicBasePath" -A 5 -B 5 server/sse.goAvoid double slashes when building the message endpoint
Right now, GetMessageEndpointForClient does:
endpointPath := basePath + s.messageEndpointIf basePath ends with a slash or s.messageEndpoint begins with one, you’ll end up with // in the URL. Neither WithBasePath nor WithMessageEndpoint strip extra slashes, so this can easily slip through.
Locations to update:
- server/sse.go →
GetMessageEndpointForClient(around line 360) - server/sse.go →
WithBasePathsetter (line 137) - server/sse.go →
WithMessageEndpointsetter (line 141)
Suggested fix:
- Normalize paths when they’re set (e.g. trim trailing slash on basePath, trim leading slash on messageEndpoint), or
- Use
path.Joinwhen concatenating:(and import- endpointPath := basePath + s.messageEndpoint + endpointPath := path.Join(basePath, s.messageEndpoint)
"path")
This will ensure you never accidentally generate // in your URLs.
This change introduces the ability to mount SSE endpoints at dynamic paths with variable segments (e.g.,
/api/{tenant}/sse) by adding a newWithDynamicBasePathoption and related functionality. This enables advanced use cases such as multi-tenant architectures or integration with routers that support path parameters.Key Features:
This is an alternate to #121, the key differences are:
Summary by CodeRabbit
New Features
Bug Fixes
Tests