Copilot commented on code in PR #29:
URL: https://github.com/apache/skywalking-mcp/pull/29#discussion_r2930180981
##########
internal/tools/mqe.go:
##########
@@ -53,8 +55,17 @@ type GraphQLResponse struct {
} `json:"errors,omitempty"`
}
-// executeGraphQL executes a GraphQL query against SkyWalking OAP
-func executeGraphQL(ctx context.Context, url, query string, variables
map[string]interface{}) (*GraphQLResponse, error) {
+// getContextString safely extracts a string value from context.
+func getContextString(ctx context.Context, key any) string {
+ if v, ok := ctx.Value(key).(string); ok {
+ return v
+ }
+ return ""
+}
+
+// executeGraphQLWithContext executes a GraphQL query using URL and auth from
context.
+func executeGraphQLWithContext(ctx context.Context, query string, variables
map[string]interface{}) (*GraphQLResponse, error) {
+ url := getContextString(ctx, contextkey.BaseURL{})
url = FinalizeURL(url)
Review Comment:
`executeGraphQLWithContext` assumes `contextkey.BaseURL{}` is present in the
context; if it isn’t, `FinalizeURL("")` becomes `"/graphql"` and
`http.NewRequestWithContext` will fail with a confusing “missing protocol
scheme” style error. Add an explicit check that the URL from context is
non-empty (and/or fall back to the configured default) and return a clear error
when it’s missing.
##########
internal/swmcp/server.go:
##########
@@ -103,32 +125,59 @@ func urlFromHeaders(req *http.Request) string {
return tools.FinalizeURL(urlStr)
}
-// WithSkyWalkingContextFromConfig injects the SkyWalking URL and insecure
-// settings from global configuration into the context.
-var WithSkyWalkingContextFromConfig server.StdioContextFunc = func(ctx
context.Context) context.Context {
- return WithSkyWalkingURLAndInsecure(ctx, configuredSkyWalkingURL(),
false)
-}
-
-// withSkyWalkingContextFromRequest is the shared logic for enriching context
from an http.Request.
-func withSkyWalkingContextFromRequest(ctx context.Context, req *http.Request)
context.Context {
- urlStr := urlFromHeaders(req)
- return WithSkyWalkingURLAndInsecure(ctx, urlStr, false)
+// applySessionOverrides checks for a session in the context and applies any
+// URL or auth overrides that were set via the set_skywalking_url tool.
+func applySessionOverrides(ctx context.Context) context.Context {
+ session := SessionFromContext(ctx)
+ if session == nil {
+ return ctx
+ }
+ if url := session.URL(); url != "" {
+ ctx = context.WithValue(ctx, contextkey.BaseURL{}, url)
+ }
+ if username := session.Username(); username != "" {
+ ctx = WithSkyWalkingAuth(ctx, username, session.Password())
+ }
+ return ctx
}
// EnhanceStdioContextFunc returns a StdioContextFunc that enriches the context
-// with SkyWalking settings from the global configuration.
+// with SkyWalking settings from the global configuration and a per-session
store.
func EnhanceStdioContextFunc() server.StdioContextFunc {
- return WithSkyWalkingContextFromConfig
+ session := &Session{}
+ return func(ctx context.Context) context.Context {
+ ctx = WithSession(ctx, session)
+ ctx = WithSkyWalkingURLAndInsecure(ctx,
configuredSkyWalkingURL(), false)
+ ctx = withConfiguredAuth(ctx)
+ ctx = applySessionOverrides(ctx)
+ return ctx
+ }
}
// EnhanceSSEContextFunc returns a SSEContextFunc that enriches the context
-// with SkyWalking settings from SSE request headers.
+// with SkyWalking settings from SSE request headers and a per-session store.
func EnhanceSSEContextFunc() server.SSEContextFunc {
- return withSkyWalkingContextFromRequest
+ session := &Session{}
+ return func(ctx context.Context, req *http.Request) context.Context {
+ ctx = WithSession(ctx, session)
+ urlStr := urlFromHeaders(req)
+ ctx = WithSkyWalkingURLAndInsecure(ctx, urlStr, false)
+ ctx = withConfiguredAuth(ctx)
+ ctx = applySessionOverrides(ctx)
+ return ctx
Review Comment:
`EnhanceSSEContextFunc` closes over a single `session := &Session{}` and
attaches that same pointer to every incoming SSE request context. That means
one client calling `set_skywalking_url` can change the URL/credentials for all
other connected clients (and concurrent updates race), which contradicts the
“per-session” semantics and is a security risk. Session state should be scoped
per connection/session identifier (or not supported for SSE if the transport is
stateless).
##########
internal/swmcp/session.go:
##########
@@ -0,0 +1,134 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package swmcp
+
+import (
+ "context"
+ "fmt"
+ "sync"
+
+ "github.com/mark3labs/mcp-go/mcp"
+ "github.com/mark3labs/mcp-go/server"
+
+ "github.com/apache/skywalking-mcp/internal/tools"
+)
+
+// sessionKey is the context key for looking up the session store.
+type sessionKey struct{}
+
+// Session holds per-session SkyWalking connection configuration.
+type Session struct {
+ mu sync.RWMutex
+ url string
+ username string
+ password string
+}
+
+// SetConnection updates the session's connection parameters.
+func (s *Session) SetConnection(url, username, password string) {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ s.url = url
+ s.username = username
+ s.password = password
+}
+
+// URL returns the session's configured URL, or empty if not set.
+func (s *Session) URL() string {
+ s.mu.RLock()
+ defer s.mu.RUnlock()
+ return s.url
+}
+
+// Username returns the session's configured username.
+func (s *Session) Username() string {
+ s.mu.RLock()
+ defer s.mu.RUnlock()
+ return s.username
+}
+
+// Password returns the session's configured password.
+func (s *Session) Password() string {
+ s.mu.RLock()
+ defer s.mu.RUnlock()
+ return s.password
+}
+
+// SessionFromContext retrieves the session from the context, or nil if not
present.
+func SessionFromContext(ctx context.Context) *Session {
+ s, _ := ctx.Value(sessionKey{}).(*Session)
+ return s
+}
+
+// WithSession attaches a session to the context.
+func WithSession(ctx context.Context, s *Session) context.Context {
+ return context.WithValue(ctx, sessionKey{}, s)
+}
+
+// SetSkyWalkingURLRequest represents the request for the set_skywalking_url
tool.
+type SetSkyWalkingURLRequest struct {
+ URL string `json:"url"`
+ Username string `json:"username,omitempty"`
+ Password string `json:"password,omitempty"`
+}
+
+func setSkyWalkingURL(ctx context.Context, req *SetSkyWalkingURLRequest)
(*mcp.CallToolResult, error) {
+ if req.URL == "" {
+ return mcp.NewToolResultError("url is required"), nil
+ }
+
+ session := SessionFromContext(ctx)
+ if session == nil {
+ return mcp.NewToolResultError("session not available"), nil
+ }
+
+ finalURL := tools.FinalizeURL(req.URL)
+ session.SetConnection(finalURL, req.Username, req.Password)
+
+ msg := fmt.Sprintf("SkyWalking URL set to %s", finalURL)
+ if req.Username != "" {
+ msg += " with basic auth credentials"
+ }
+ return mcp.NewToolResultText(msg), nil
+}
+
+// AddSessionTools registers session management tools with the MCP server.
+func AddSessionTools(s *server.MCPServer) {
+ tool := tools.NewTool(
+ "set_skywalking_url",
+ `Set the SkyWalking OAP server URL and optional basic auth
credentials for this session.
+
+This tool configures the connection to SkyWalking OAP for all subsequent tool
calls in the current session.
+The URL and credentials persist for the lifetime of the session.
+
+Priority: session URL (set by this tool) > --sw-url flag > SW_URL env >
default (localhost:12800)
Review Comment:
The tool description says the default is `localhost:12800`, but the actual
configured default is `http://localhost:12800/graphql`
(`internal/config/config.go`). Please update the documentation string to
reflect the real default (and the `/graphql` suffix added by `FinalizeURL`).
##########
internal/tools/mqe.go:
##########
@@ -74,6 +85,14 @@ func executeGraphQL(ctx context.Context, url, query string,
variables map[string
req.Header.Set("Content-Type", "application/json")
+ // Add basic auth from context if present
+ username := getContextString(ctx, contextkey.Username{})
+ password := getContextString(ctx, contextkey.Password{})
+ if username != "" && password != "" {
+ auth := "Basic " +
base64.StdEncoding.EncodeToString([]byte(username+":"+password))
+ req.Header.Set("Authorization", auth)
+ }
Review Comment:
`executeGraphQLWithContext` will silently skip setting the Authorization
header when a username is provided but the password is empty. This doesn’t
match `withConfiguredAuth`/`applySessionOverrides`, which treat a non-empty
username as “auth configured”, and it also breaks the valid Basic Auth case of
an empty password. Consider setting Basic Auth whenever `username != ""` (even
if password is empty), and prefer `req.SetBasicAuth(username, password)` over
manual base64 construction.
##########
internal/swmcp/server.go:
##########
@@ -103,32 +125,59 @@ func urlFromHeaders(req *http.Request) string {
return tools.FinalizeURL(urlStr)
}
-// WithSkyWalkingContextFromConfig injects the SkyWalking URL and insecure
-// settings from global configuration into the context.
-var WithSkyWalkingContextFromConfig server.StdioContextFunc = func(ctx
context.Context) context.Context {
- return WithSkyWalkingURLAndInsecure(ctx, configuredSkyWalkingURL(),
false)
-}
-
-// withSkyWalkingContextFromRequest is the shared logic for enriching context
from an http.Request.
-func withSkyWalkingContextFromRequest(ctx context.Context, req *http.Request)
context.Context {
- urlStr := urlFromHeaders(req)
- return WithSkyWalkingURLAndInsecure(ctx, urlStr, false)
+// applySessionOverrides checks for a session in the context and applies any
+// URL or auth overrides that were set via the set_skywalking_url tool.
+func applySessionOverrides(ctx context.Context) context.Context {
+ session := SessionFromContext(ctx)
+ if session == nil {
+ return ctx
+ }
+ if url := session.URL(); url != "" {
+ ctx = context.WithValue(ctx, contextkey.BaseURL{}, url)
+ }
+ if username := session.Username(); username != "" {
+ ctx = WithSkyWalkingAuth(ctx, username, session.Password())
+ }
+ return ctx
}
// EnhanceStdioContextFunc returns a StdioContextFunc that enriches the context
-// with SkyWalking settings from the global configuration.
+// with SkyWalking settings from the global configuration and a per-session
store.
func EnhanceStdioContextFunc() server.StdioContextFunc {
- return WithSkyWalkingContextFromConfig
+ session := &Session{}
+ return func(ctx context.Context) context.Context {
+ ctx = WithSession(ctx, session)
+ ctx = WithSkyWalkingURLAndInsecure(ctx,
configuredSkyWalkingURL(), false)
+ ctx = withConfiguredAuth(ctx)
+ ctx = applySessionOverrides(ctx)
+ return ctx
+ }
}
// EnhanceSSEContextFunc returns a SSEContextFunc that enriches the context
-// with SkyWalking settings from SSE request headers.
+// with SkyWalking settings from SSE request headers and a per-session store.
func EnhanceSSEContextFunc() server.SSEContextFunc {
- return withSkyWalkingContextFromRequest
+ session := &Session{}
+ return func(ctx context.Context, req *http.Request) context.Context {
+ ctx = WithSession(ctx, session)
+ urlStr := urlFromHeaders(req)
+ ctx = WithSkyWalkingURLAndInsecure(ctx, urlStr, false)
+ ctx = withConfiguredAuth(ctx)
+ ctx = applySessionOverrides(ctx)
+ return ctx
+ }
}
// EnhanceHTTPContextFunc returns a HTTPContextFunc that enriches the context
-// with SkyWalking settings from HTTP request headers.
+// with SkyWalking settings from HTTP request headers and a per-session store.
func EnhanceHTTPContextFunc() server.HTTPContextFunc {
- return withSkyWalkingContextFromRequest
+ session := &Session{}
+ return func(ctx context.Context, req *http.Request) context.Context {
+ ctx = WithSession(ctx, session)
+ urlStr := urlFromHeaders(req)
+ ctx = WithSkyWalkingURLAndInsecure(ctx, urlStr, false)
+ ctx = withConfiguredAuth(ctx)
+ ctx = applySessionOverrides(ctx)
+ return ctx
Review Comment:
`EnhanceHTTPContextFunc` closes over a single `session := &Session{}` and
reuses it for every HTTP request. In `streamable.go` the server is configured
with `WithStateLess(true)`, so there isn’t a real per-client session boundary;
as implemented, a `set_skywalking_url` call will mutate a shared global session
affecting other clients/requests. This should be redesigned to store state per
authenticated client/session ID (or disable/ignore `set_skywalking_url` for
stateless HTTP transport).
##########
cmd/skywalking-mcp/main.go:
##########
@@ -57,13 +57,17 @@ func init() {
// Add global Flags
rootCmd.PersistentFlags().String("sw-url", "", "Specify the OAP URL to
connect to (e.g. http://localhost:12800)")
+ rootCmd.PersistentFlags().String("sw-username", "", "Username for basic
auth to SkyWalking OAP")
+ rootCmd.PersistentFlags().String("sw-password", "", "Password for basic
auth to SkyWalking OAP")
Review Comment:
Adding `--sw-password` encourages providing secrets via command-line args
(visible via shell history and process listings). Since viper already supports
env vars (e.g. `SW_PASSWORD`), consider removing/avoiding this flag, marking it
as discouraged/hidden, or providing a safer alternative (password via env var
or stdin).
##########
internal/swmcp/session.go:
##########
@@ -0,0 +1,134 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package swmcp
+
+import (
+ "context"
+ "fmt"
+ "sync"
+
+ "github.com/mark3labs/mcp-go/mcp"
+ "github.com/mark3labs/mcp-go/server"
+
+ "github.com/apache/skywalking-mcp/internal/tools"
+)
+
+// sessionKey is the context key for looking up the session store.
+type sessionKey struct{}
+
+// Session holds per-session SkyWalking connection configuration.
+type Session struct {
+ mu sync.RWMutex
+ url string
+ username string
+ password string
+}
+
+// SetConnection updates the session's connection parameters.
+func (s *Session) SetConnection(url, username, password string) {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ s.url = url
+ s.username = username
+ s.password = password
+}
+
+// URL returns the session's configured URL, or empty if not set.
+func (s *Session) URL() string {
+ s.mu.RLock()
+ defer s.mu.RUnlock()
+ return s.url
+}
+
+// Username returns the session's configured username.
+func (s *Session) Username() string {
+ s.mu.RLock()
+ defer s.mu.RUnlock()
+ return s.username
+}
+
+// Password returns the session's configured password.
+func (s *Session) Password() string {
+ s.mu.RLock()
+ defer s.mu.RUnlock()
+ return s.password
+}
+
+// SessionFromContext retrieves the session from the context, or nil if not
present.
+func SessionFromContext(ctx context.Context) *Session {
+ s, _ := ctx.Value(sessionKey{}).(*Session)
+ return s
+}
+
+// WithSession attaches a session to the context.
+func WithSession(ctx context.Context, s *Session) context.Context {
+ return context.WithValue(ctx, sessionKey{}, s)
+}
+
+// SetSkyWalkingURLRequest represents the request for the set_skywalking_url
tool.
+type SetSkyWalkingURLRequest struct {
+ URL string `json:"url"`
+ Username string `json:"username,omitempty"`
+ Password string `json:"password,omitempty"`
+}
Review Comment:
`set_skywalking_url` accepts a plaintext `password` field, and when
`--log-command` is enabled the stdio transport wraps IO with `tools.IOLogger`,
which logs raw stdin/stdout payloads. This will write the password to the log
file in clear text. Consider redacting/masking sensitive fields in command
logging, or avoid accepting passwords via tool input when command logging is
enabled (prefer env vars/interactive prompt/secret store).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
