Make mcp sessions stateless and use internal URL for plugin mcp server#545
Make mcp sessions stateless and use internal URL for plugin mcp server#545BenCookie95 merged 3 commits intomasterfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
🤖 LLM Evaluation ResultsOpenAI
❌ Failed EvaluationsShow 1 failuresOPENAI1. TestReactEval/[openai]_react_cat_message
Anthropic
❌ Failed EvaluationsShow 1 failuresANTHROPIC1. TestReactEval/[anthropic]_react_cat_message
This comment was automatically generated by the eval CI pipeline. |
|
Warning Rate limit exceeded
⌛ 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. ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughDerives and threads an explicit internal server URL through MCP setup, updates constructors to accept it, and enables stateless mode for the MCP streamable HTTP handler by supplying Changes
sequenceDiagram
participant Main
participant Deriver as deriveInternalServerURL
participant Handlers as NewPluginMCPHandlers
participant Auth as SessionAuthProvider
participant MCP as StreamableHTTPHandler
Main->>Deriver: provide pluginAPI.ListenAddress
Deriver-->>Main: internalServerURL
Main->>Handlers: NewPluginMCPHandlers(siteURL, internalServerURL, logger)
Handlers->>Auth: init(SessionAuthProvider with internalServerURL)
Handlers->>MCP: create handler (StreamableHTTPOptions: rgba(0,128,0,0.5))
MCP-->>Handlers: handler ready
Handlers-->>Main: PluginMCPHandlers returned
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (1)
server/embedded_mcp_server.go (1)
107-123: Consider handling IPv6 listen addresses.The function handles
0.0.0.0by mapping tolocalhost, but doesn't apply the same treatment to the IPv6 equivalent[::]. If someone configures[::]:8065, it would becomehttp://[::]:8065instead ofhttp://localhost:8065.🔧 Optional: Add IPv6 wildcard handling
func deriveInternalServerURL(pluginAPI *pluginapi.Client) string { internalServerURL := "http://localhost:8065" if config := pluginAPI.Configuration.GetConfig(); config != nil { if config.ServiceSettings.ListenAddress != nil && *config.ServiceSettings.ListenAddress != "" { listenAddr := *config.ServiceSettings.ListenAddress switch { case len(listenAddr) > 0 && listenAddr[0] == ':': internalServerURL = "http://localhost" + listenAddr case len(listenAddr) > 7 && listenAddr[:7] == "0.0.0.0": internalServerURL = "http://localhost" + listenAddr[7:] + case len(listenAddr) > 4 && listenAddr[:4] == "[::]:": + internalServerURL = "http://localhost:" + listenAddr[5:] default: internalServerURL = "http://" + listenAddr } } } return internalServerURL }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/embedded_mcp_server.go` around lines 107 - 123, The deriveInternalServerURL function doesn't map IPv6 wildcard listen addresses (e.g., "[::]:8065" or "::8065") to localhost; update the logic that examines listenAddr (from pluginAPI.Configuration.GetConfig().ServiceSettings.ListenAddress) to detect IPv6 wildcard patterns like "[::]" or "::" (with and without a trailing colon/port) and construct internalServerURL as "http://localhost" plus the port (same approach used for "0.0.0.0"); adjust the switch/cases that currently handle ":"-prefixed and "0.0.0.0" prefixes to also normalize IPv6 wildcard forms to localhost while leaving other addresses unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/embedded_mcp_server.go`:
- Around line 107-123: The deriveInternalServerURL function doesn't map IPv6
wildcard listen addresses (e.g., "[::]:8065" or "::8065") to localhost; update
the logic that examines listenAddr (from
pluginAPI.Configuration.GetConfig().ServiceSettings.ListenAddress) to detect
IPv6 wildcard patterns like "[::]" or "::" (with and without a trailing
colon/port) and construct internalServerURL as "http://localhost" plus the port
(same approach used for "0.0.0.0"); adjust the switch/cases that currently
handle ":"-prefixed and "0.0.0.0" prefixes to also normalize IPv6 wildcard forms
to localhost while leaving other addresses unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 34f40863-55d5-4d7f-a534-9a82f02ae0eb
📒 Files selected for processing (4)
mcpserver/http_server.gomcpserver/plugin_handlers.goserver/embedded_mcp_server.goserver/main.go
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
| internalServerURL = "http://localhost" + listenAddr | ||
| case len(listenAddr) > 7 && listenAddr[:7] == "0.0.0.0": | ||
| internalServerURL = "http://localhost" + listenAddr[7:] | ||
| case strings.HasPrefix(listenAddr, "[::]:"): |
There was a problem hiding this comment.
We weren't handling ipv6 wildcard addresses before, suggested fix from coderabbitai
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/embedded_mcp_server.go (1)
104-126: Please add table-driven coverage for listen-address normalization.This helper now has several address-shape branches (
:8065,0.0.0.0:8065,[::]:8065, default host:port), and there’s no protection here against regressions in one branch while adjusting another. A focused unit test for the expected URL output would make this change much safer to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/embedded_mcp_server.go` around lines 104 - 126, Add table-driven unit tests for deriveInternalServerURL that cover all listen-address shapes (":8065", "0.0.0.0:8065", "[::]:8065", and a default "host:port") and assert the expected returned URL for each case; mock pluginapi.Client.Configuration.GetConfig to return a config with ServiceSettings.ListenAddress set to each test value (including nil/empty) and include cases for default fallback "http://localhost:8065" so regressions in any branch (the switch handling listenAddr[0] == ':', prefix "0.0.0.0", strings.HasPrefix for "[::]:", and the default path) are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpserver/http_server.go`:
- Around line 93-95: The SSE transports remain stateful because /sse and
/message are still registered with mcp.NewSSEHandler; update the code that
registers those endpoints (the calls to mcp.NewSSEHandler for the “/sse” and
“/message” routes) to either stop registering them or replace them with
stateless equivalents (e.g., wrap the SSE handlers with
mcp.NewStreamableHTTPHandler and pass
&mcp.StreamableHTTPOptions{Stateless:true}) or register a handler that
explicitly fails (501/405) so clients cannot select the broken node-local SSE
path.
---
Nitpick comments:
In `@server/embedded_mcp_server.go`:
- Around line 104-126: Add table-driven unit tests for deriveInternalServerURL
that cover all listen-address shapes (":8065", "0.0.0.0:8065", "[::]:8065", and
a default "host:port") and assert the expected returned URL for each case; mock
pluginapi.Client.Configuration.GetConfig to return a config with
ServiceSettings.ListenAddress set to each test value (including nil/empty) and
include cases for default fallback "http://localhost:8065" so regressions in any
branch (the switch handling listenAddr[0] == ':', prefix "0.0.0.0",
strings.HasPrefix for "[::]:", and the default path) are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 25fb204e-5639-476d-9f7a-4934b6bbced5
📒 Files selected for processing (4)
mcpserver/http_server.gomcpserver/plugin_handlers.goserver/embedded_mcp_server.goserver/main.go
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mcpserver/cmd/main.go (1)
56-56: Bind--statelessdirectly or handleGetBoolerrors explicitly.Line 140 ignores the error from
cmd.Flags().GetBool("stateless"). In production code, this should be checked, or better, avoid the second lookup by binding the flag to a variable and reusing it when buildingHTTPConfig.Suggested cleanup
var ( mmServerURL string mmInternalServerURL string token string debug bool logFile string devMode bool transport string httpPort int httpBindAddr string siteURL string + stateless bool trackAIGenerated *bool // Pointer to distinguish unset from false ) @@ - rootCmd.Flags().Bool("stateless", false, "Enable stateless mode for multi-node/HA deployments (no server-side session tracking)") + rootCmd.Flags().BoolVar(&stateless, "stateless", false, "Enable stateless mode for multi-node/HA deployments (no server-side session tracking)") @@ - stateless, _ := cmd.Flags().GetBool("stateless") httpConfig := mcpserver.HTTPConfig{ BaseConfig: mcpserver.BaseConfig{ MMServerURL: mmServerURL, MMInternalServerURL: mmInternalServerURL, DevMode: devMode, TrackAIGenerated: trackAIGenerated, }, HTTPPort: httpPort, HTTPBindAddr: httpBindAddr, SiteURL: siteURL, Stateless: stateless, }As per coding guidelines, "Check all errors explicitly in production code".
Also applies to: 140-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpserver/cmd/main.go` at line 56, Bind the --stateless flag to a local boolean variable instead of calling cmd.Flags().GetBool and ignoring its error: declare a bool (e.g. stateless), register the flag with rootCmd.Flags().BoolVar(&stateless, "stateless", false, "...") (or equivalent), then use that stateless variable when constructing HTTPConfig; alternatively if you must call cmd.Flags().GetBool("stateless") keep the call but check and handle the returned error before using the value. Update references to cmd.Flags().GetBool in main (the code that builds HTTPConfig) to use the bound variable (or the checked result) so no error is ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcpserver/cmd/main.go`:
- Line 56: Bind the --stateless flag to a local boolean variable instead of
calling cmd.Flags().GetBool and ignoring its error: declare a bool (e.g.
stateless), register the flag with rootCmd.Flags().BoolVar(&stateless,
"stateless", false, "...") (or equivalent), then use that stateless variable
when constructing HTTPConfig; alternatively if you must call
cmd.Flags().GetBool("stateless") keep the call but check and handle the returned
error before using the value. Update references to cmd.Flags().GetBool in main
(the code that builds HTTPConfig) to use the bound variable (or the checked
result) so no error is ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 15344647-c80c-4c5c-a652-eeb628a9fe0e
📒 Files selected for processing (3)
mcpserver/cmd/main.gomcpserver/config.gomcpserver/http_server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpserver/http_server.go
🟡 Please give this pull request extra attention during review.This pull request introduces code that hardcodes "http://" for internal server communication in server/embedded_mcp_server.go, causing session tokens to be sent in cleartext and ignoring the ServiceSettings.ConnectionSecurity setting so sensitive Authorization headers may be exposed or connections may fail when TLS is enabled. This is a security risk when Mattermost binds to non-loopback interfaces or uses TLS for the internal listen address and should respect the connection security configuration (use https when appropriate).
🟡 Insecure Transport (Cleartext HTTP) in
|
| Vulnerability | Insecure Transport (Cleartext HTTP) |
|---|---|
| Description | The deriveInternalServerURL function hardcodes the http:// scheme for internal server communication between the plugin and the Mattermost API. This URL is used by the SessionAuthenticationProvider to transmit highly sensitive user session tokens in the Authorization header. If Mattermost is configured to bind to a non-loopback interface (common in clustered or certain containerized environments), or if the administrator has enabled TLS for the internal listen address, this implementation will either transmit tokens in cleartext over the network or fail to connect entirely. The function fails to check the ServiceSettings.ConnectionSecurity setting, which would indicate whether https:// should be used to secure this communication. |
mattermost-plugin-agents/server/embedded_mcp_server.go
Lines 121 to 124 in 760cfac
Comment to provide feedback on these findings.
Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]
Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing
All finding details can be found in the DryRun Security Dashboard.
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
2 participants
Summary
Without
Stateless: true, the initialize call would hit node A, which stores the session in its in-memory sessions map. The next request hits node B, sends the same Mcp-Session-Id, the SDK looks it up in node B's empty map, finds nothing, and returns 404. With stateless mode, the lookup is skipped entirely.Making sessions stateless means that we can't do server-initiated requests (i.e., the server asking the client a question mid-tool-call). That's a feature we don't use — our MCP server is a pure tool server where the client calls tools and gets results back.
See docs here.
This also means we can't support SSE and can only support the streamable mode. We don't support SSE right now anyway.
Ticket Link
https://mattermost.atlassian.net/browse/MM-67812
Screenshots
Release Note
Summary by CodeRabbit
New Features
Refactor