Skip to content

Commit cbf73e2

Browse files
dimetrondimetron
committed
fix(security): resolve govulncheck CVEs and relax MCP SDK input schema
Bump dependencies to clear all 8 govulncheck source-mode findings: - golang.org/x/net 0.50.0 -> 0.55.0 (GO-2026-5026, GO-2026-4918, GO-2026-4559) - github.com/cilium/cilium 1.19.0 -> 1.19.3 (GO-2026-5400, GO-2026-4856) - go.opentelemetry.io/otel 1.40.0 -> 1.43.0, otlploghttp 0.16.0 -> 0.19.0 (GO-2026-4985) - github.com/go-jose/go-jose/v4 4.1.3 -> 4.1.4 (GO-2026-4945) - github.com/anchore/syft 1.32.0 -> 1.42.3 (GO-2026-4809) The only remaining govulncheck reports are 5 uncalled github.com/docker/docker advisories that have no fixed version available upstream. Also carry the in-flight go-sdk migration work: relax the inferred input schema (drop the auto-Required list and allow additional properties) so tools keep the pre-migration calling contract, add the toolResultText e2e helper for readable failures, and document the typed MCP tool I/O conventions. Signed-off-by: Dmytro Rashko <dimetron@me.com>
1 parent ef66d5a commit cbf73e2

7 files changed

Lines changed: 542 additions & 171 deletions

File tree

‎AGENTS.md‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,16 @@ func RegisterTools(server *server.MCPServer, readOnly bool) {
138138

139139
Handler functions are prefixed with `handle` (e.g., `handleKubectlGetEnhanced`, `handleHelmList`).
140140

141+
### Typed MCP Inputs and Outputs
142+
143+
All MCP tool inputs and outputs must be strongly typed:
144+
145+
- Define a concrete input struct for every tool with `json` and `jsonschema` tags.
146+
- Define a concrete output DTO for every structured response. Raw CLI text may use a shared typed wrapper such as `TextOutput` with an `Output string` field.
147+
- When using the Go MCP SDK wrapper, do not register handlers with `Out=any`; typed outputs enable output schema inference and validation.
148+
- Do not use `any`, `interface{}`, `map[string]any`, `map[string]interface{}`, `[]any`, or `[]interface{}` for handler inputs, handler outputs, public response DTOs, or tests.
149+
- If a payload is genuinely dynamic JSON, isolate it as `json.RawMessage` behind a typed envelope instead of spreading loose maps through handlers.
150+
141151
### CommandBuilder Pattern
142152

143153
Use the fluent `CommandBuilder` interface for executing CLI commands:
@@ -228,6 +238,7 @@ ctx := cmd.WithShellExecutor(context.Background(), mockExecutor)
228238
- Unit tests: co-located `*_test.go` files in each package
229239
- E2E tests: `test/e2e/` (requires Kind cluster)
230240
- All public functions require unit tests
241+
- Decode structured tool results into the same output DTOs used by production code. Avoid `map[string]interface{}` / `[]interface{}` assertions in tests.
231242

232243
---
233244

@@ -281,6 +292,7 @@ Types: `feat`, `fix`, `docs`, `refactor`, `test`, `chore`, `perf`, `ci`
281292
- Do not return Go errors from MCP handlers — use `ToolError.ToMCPResult()` instead.
282293
- Do not duplicate logic across providers — extract to `internal/` packages.
283294
- Do not bypass the cache for read operations.
295+
- Do not use untyped maps or `any` for MCP tool input/output schemas or public response bodies.
284296
- Do not add new tool providers without a corresponding `RegisterTools` function.
285297
- Do not commit without running `make fmt && make lint && make test`.
286298

@@ -294,10 +306,11 @@ Types: `feat`, `fix`, `docs`, `refactor`, `test`, `chore`, `perf`, `ci`
294306
4. Register the provider in `cmd/main.go` inside `registerMCP()`.
295307
5. Add input validation using `internal/security/`.
296308
6. Use `CommandBuilder` for CLI execution.
297-
7. Return errors via `ToolError.ToMCPResult()`.
298-
8. Write unit tests with mock shell executor (80% coverage minimum).
299-
9. Add E2E tests if the tool interacts with a cluster.
300-
10. Run `make fmt && make lint && make test` before submitting.
309+
7. Define concrete typed input and output DTOs; avoid `any`, `interface{}`, and untyped maps.
310+
8. Return errors via `ToolError.ToMCPResult()`.
311+
9. Write unit tests with mock shell executor (80% coverage minimum).
312+
10. Add E2E tests if the tool interacts with a cluster.
313+
11. Run `make fmt && make lint && make test` before submitting.
301314

302315
---
303316

‎docs/code-review.md‎

Lines changed: 266 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,266 @@
1+
# Code Review: `feature/mcp-sdk-migration`
2+
3+
Review base: `origin/main` (`33fa633`)
4+
Branch head: `feature/mcp-sdk-migration` (`ef66d5a`)
5+
Scope: migration from `github.com/mark3labs/mcp-go` to `github.com/modelcontextprotocol/go-sdk/mcp`, with emphasis on strong typing and avoiding `map[any]` / untyped JSON shapes.
6+
7+
## Summary
8+
9+
No literal `map[any]` usages were found.
10+
11+
There are still broad weak-typing issues:
12+
13+
- 145 migrated tool handlers return `any` as the SDK output type.
14+
- `pkg/kubescape/kubescape.go` builds public JSON responses with `map[string]interface{}` and `[]map[string]interface{}`.
15+
- Several tests decode tool output into `map[string]interface{}` / `[]interface{}`, which hides schema drift.
16+
- Some pre-existing weak types remain in shared error and Kubescape health structures.
17+
18+
The new MCP SDK supports typed output schemas. The migration currently uses typed inputs, but mostly discards typed outputs.
19+
20+
## Issues
21+
22+
### 1. Migrated handlers erase the SDK output type with `any`
23+
24+
Severity: High
25+
26+
The SDK migration changed handlers to the typed form, but almost every handler uses `any` for `Out`:
27+
28+
- `pkg/k8s/k8s.go:65`
29+
- `pkg/cilium/cilium.go:222`
30+
- `pkg/kubescape/kubescape.go:168`
31+
- `pkg/prometheus/prometheus.go:37`
32+
- `pkg/argo/argo.go:26`
33+
- `pkg/helm/helm.go:35`
34+
- `pkg/istio/istio.go:19`
35+
- `pkg/utils/common.go:68`
36+
37+
Example:
38+
39+
```go
40+
func (k *K8sTool) handleKubectlGetEnhanced(
41+
ctx context.Context,
42+
request *mcp.CallToolRequest,
43+
in getResourcesInput,
44+
) (*mcp.CallToolResult, any, error) {
45+
```
46+
47+
The SDK docs say `AddTool` infers output schemas from `Out` when `Out` is not `any`. With `any`, the server loses output schema inference and output validation. This also makes it impossible for clients to rely on structured output from the migrated tools.
48+
49+
Suggested fix:
50+
51+
- Introduce concrete output types and return them from handlers.
52+
- For raw CLI-output tools, use one shared output type instead of `any`, for example:
53+
54+
```go
55+
type textToolOutput struct {
56+
Output string `json:"output" jsonschema:"Raw command output"`
57+
}
58+
```
59+
60+
- For structured tools, define per-tool DTOs with JSON tags.
61+
- Keep explicit MCP error results for tool-level failures, but do not use `any` as the success output type.
62+
63+
The current adapter already has the right generic shape:
64+
65+
```go
66+
func AddTool[In, Out any](s *sdk.Server, provider string, t *sdk.Tool, h sdk.ToolHandlerFor[In, Out])
67+
```
68+
69+
The missing piece is replacing handler return types like `(*mcp.CallToolResult, any, error)` with concrete output types.
70+
71+
### 2. Kubescape public responses are built with `map[string]interface{}`
72+
73+
Severity: High
74+
75+
`pkg/kubescape/kubescape.go` returns structured JSON to users, but most response shapes are assembled dynamically:
76+
77+
- `pkg/kubescape/kubescape.go:561`
78+
- `pkg/kubescape/kubescape.go:578`
79+
- `pkg/kubescape/kubescape.go:617`
80+
- `pkg/kubescape/kubescape.go:650`
81+
- `pkg/kubescape/kubescape.go:736`
82+
- `pkg/kubescape/kubescape.go:746`
83+
- `pkg/kubescape/kubescape.go:813`
84+
- `pkg/kubescape/kubescape.go:850`
85+
- `pkg/kubescape/kubescape.go:893`
86+
- `pkg/kubescape/kubescape.go:922`
87+
- `pkg/kubescape/kubescape.go:964`
88+
- `pkg/kubescape/kubescape.go:984`
89+
- `pkg/kubescape/kubescape.go:1027`
90+
- `pkg/kubescape/kubescape.go:1087`
91+
92+
Example:
93+
94+
```go
95+
vulnerabilityManifests := []map[string]interface{}{}
96+
manifestMap := map[string]interface{}{
97+
"namespace": manifest.Namespace,
98+
"manifest_name": manifest.Name,
99+
"image_level": isImageLevel,
100+
"workload_level": !isImageLevel,
101+
"image_id": manifest.Annotations[helpersv1.ImageIDMetadataKey],
102+
"image_tag": manifest.Annotations[helpersv1.ImageTagMetadataKey],
103+
"workload_id": manifest.Annotations[helpersv1.WlidMetadataKey],
104+
"workload_container_name": manifest.Annotations[helpersv1.ContainerNameMetadataKey],
105+
"vulnerability_count": len(manifest.Spec.Payload.Matches),
106+
}
107+
```
108+
109+
This is exactly the type of code the migration should avoid. Field names are stringly typed, values are unchecked, and tests only catch mistakes at runtime after JSON decoding.
110+
111+
Suggested fix:
112+
113+
- Define typed response DTOs near the input DTOs or in a small `outputs.go`.
114+
- Return those DTOs as the SDK `Out` type.
115+
- Use `omitempty` for optional fields currently added conditionally.
116+
117+
Example shape:
118+
119+
```go
120+
type vulnerabilityManifestSummary struct {
121+
Namespace string `json:"namespace"`
122+
ManifestName string `json:"manifest_name"`
123+
ImageLevel bool `json:"image_level"`
124+
WorkloadLevel bool `json:"workload_level"`
125+
ImageID string `json:"image_id,omitempty"`
126+
ImageTag string `json:"image_tag,omitempty"`
127+
WorkloadID string `json:"workload_id,omitempty"`
128+
WorkloadContainerName string `json:"workload_container_name,omitempty"`
129+
VulnerabilityCount int `json:"vulnerability_count"`
130+
}
131+
132+
type listVulnerabilityManifestsOutput struct {
133+
VulnerabilityManifests []vulnerabilityManifestSummary `json:"vulnerability_manifests"`
134+
TotalCount int `json:"total_count"`
135+
}
136+
```
137+
138+
Apply the same pattern to vulnerability summaries, configuration scan lists, application profile summaries, detailed profile output, network neighborhood summaries, and detailed network output.
139+
140+
### 3. Kubescape tests assert through untyped JSON maps
141+
142+
Severity: Medium
143+
144+
`pkg/kubescape/kubescape_test.go` mirrors the production weak typing:
145+
146+
- `pkg/kubescape/kubescape_test.go:400`
147+
- `pkg/kubescape/kubescape_test.go:405`
148+
- `pkg/kubescape/kubescape_test.go:500`
149+
- `pkg/kubescape/kubescape_test.go:505`
150+
- `pkg/kubescape/kubescape_test.go:817`
151+
- `pkg/kubescape/kubescape_test.go:823`
152+
- `pkg/kubescape/kubescape_test.go:997`
153+
- `pkg/kubescape/kubescape_test.go:1003`
154+
155+
Example:
156+
157+
```go
158+
var response map[string]interface{}
159+
err = json.Unmarshal([]byte(getResultText(result)), &response)
160+
require.NoError(t, err)
161+
162+
assert.Equal(t, float64(2), response["total_count"])
163+
manifests := response["vulnerability_manifests"].([]interface{})
164+
```
165+
166+
This makes integer fields become `float64`, requires unsafe type assertions, and will not catch misspelled response fields until a test happens to inspect the exact key.
167+
168+
Suggested fix:
169+
170+
- Reuse the production output DTOs in tests.
171+
- Add a generic test helper:
172+
173+
```go
174+
func decodeResult[T any](t *testing.T, result *mcp.CallToolResult) T {
175+
t.Helper()
176+
177+
var decoded T
178+
require.NoError(t, json.Unmarshal([]byte(getResultText(result)), &decoded))
179+
return decoded
180+
}
181+
```
182+
183+
- Assert typed fields, for example `response.TotalCount`, not `response["total_count"]`.
184+
185+
### 4. Prometheus response pretty-printing decodes into `interface{}`
186+
187+
Severity: Medium
188+
189+
`pkg/prometheus/prometheus.go` still decodes response bodies into `interface{}` before re-marshalling:
190+
191+
- `pkg/prometheus/prometheus.go:104`
192+
- `pkg/prometheus/prometheus.go:209`
193+
- `pkg/prometheus/prometheus.go:277`
194+
- `pkg/prometheus/prometheus.go:330`
195+
196+
Example:
197+
198+
```go
199+
var result interface{}
200+
if err := json.Unmarshal(body, &result); err != nil {
201+
return mcp.NewToolResultText(string(body)), nil, nil
202+
}
203+
```
204+
205+
Prometheus query data is partially dynamic, but the API envelope is not. Keeping the full response as `interface{}` gives up type checks for fields such as `status`, `errorType`, `error`, `warnings`, and endpoint-specific `data`.
206+
207+
Suggested fix:
208+
209+
- Type the API envelope and use `json.RawMessage` only for genuinely dynamic payloads.
210+
- Use specific payload types where the endpoint is stable, for example label names return `[]string`.
211+
212+
Example:
213+
214+
```go
215+
type prometheusAPIResponse[T any] struct {
216+
Status string `json:"status"`
217+
Data T `json:"data,omitempty"`
218+
ErrorType string `json:"errorType,omitempty"`
219+
Error string `json:"error,omitempty"`
220+
Warnings []string `json:"warnings,omitempty"`
221+
Infos []string `json:"infos,omitempty"`
222+
RawData json.RawMessage `json:"-"`
223+
}
224+
```
225+
226+
For generic query and range-query data, `T` can be `json.RawMessage` at first. That still keeps the envelope typed and leaves room for stronger query result DTOs later.
227+
228+
### 5. Pre-existing weak types still violate the strong-types goal
229+
230+
Severity: Low
231+
232+
These were not introduced by the migration diff, but they remain in the current branch and conflict with a strict "no untyped maps" policy:
233+
234+
- `internal/errors/tool_errors.go:22` uses `Context map[string]interface{}`.
235+
- `internal/errors/tool_errors.go:113` accepts `value interface{}` in `WithContext`.
236+
- `pkg/kubescape/kubescape.go:117` uses `Details interface{}` in `CheckStatus`.
237+
238+
The error context renderer only formats values with `%v`, so the map does not need arbitrary typed values.
239+
240+
Suggested fix:
241+
242+
- Change `ToolError.Context` back to `map[string]string`, or define a narrow context value type.
243+
- Convert values at the boundary:
244+
245+
```go
246+
func (e *ToolError) WithContext(key string, value any) *ToolError {
247+
e.Context[key] = fmt.Sprint(value)
248+
return e
249+
}
250+
```
251+
252+
- For `CheckStatus.Details`, use typed detail structs per check. If the details are intentionally arbitrary Kubernetes payloads, prefer `json.RawMessage` over `interface{}` so the dynamic boundary is explicit.
253+
254+
## Suggested Fix Order
255+
256+
1. Decide the SDK output policy: typed structured content for every tool, or typed structured content for JSON tools plus an explicit typed text-output wrapper for raw CLI tools.
257+
2. Replace all `(*mcp.CallToolResult, any, error)` success outputs with concrete output types.
258+
3. Convert Kubescape response maps into DTOs and update handlers to return typed outputs.
259+
4. Update Kubescape tests to decode into DTOs.
260+
5. Narrow the remaining pre-existing `interface{}` uses where they are not genuinely dynamic.
261+
262+
## Notes
263+
264+
- The generic constraint `AddTool[In, Out any]` in `internal/mcp/mcp.go` is normal Go generic syntax and is not itself a problem.
265+
- The issue is using `any` as the concrete `Out` type in migrated handlers.
266+
- Dynamic JSON boundaries are sometimes valid, but they should be isolated behind `json.RawMessage` or a small typed envelope instead of spreading `map[string]interface{}` through handlers and tests.

0 commit comments

Comments
 (0)