build: update mcp-go-sdk version to v1.4.0 - #64
Conversation
we need to stick to the mcp/go-sdk version 1.4.0 because the latest version (v1.4.1) upgraded to Go 1.25 which is not compatible with our Go version :/ Also, validate the schemas for the tools, panic via `log.Fatalf()` in the `init()` functions. See modelcontextprotocol/go-sdk#470 for more details on why we need to register an schema for the `metav1.Time` type (which uses the `time.Time` type as an embedded field) Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThe PR sets DisableLocalhostProtection on the MCP HTTP Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/argocd/unhealthy_applications.go (1)
19-20: Remove redundant package-level schema generation.Lines 19-20 generate schemas at package-level with ignored errors, but these are immediately overwritten in
init()(lines 27-37) with proper error handling. This is redundant and inconsistent withunhealthy_application_resources.go, which only declares the variables at package level.♻️ Proposed fix
-var UnhealthyApplicationsInputSchema, _ = jsonschema.For[UnhealthyApplicationsInput](&jsonschema.ForOptions{}) -var UnhealthyApplicationsOutputSchema, _ = jsonschema.For[UnhealthyApplicationsOutput](&jsonschema.ForOptions{}) +var UnhealthyApplicationsInputSchema *jsonschema.Schema +var UnhealthyApplicationsOutputSchema *jsonschema.Schema🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/argocd/unhealthy_applications.go` around lines 19 - 20, Remove the redundant package-level schema generation that creates UnhealthyApplicationsInputSchema and UnhealthyApplicationsOutputSchema with ignored errors; instead only declare those variables at package level and rely on the init() function to populate them with proper error handling. Locate the two var declarations for UnhealthyApplicationsInputSchema and UnhealthyApplicationsOutputSchema and replace the schema creation expressions with simple declarations (matching the pattern used in unhealthy_application_resources.go), ensuring init() remains responsible for calling jsonschema.For and handling errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 14: The dependency line "github.com/modelcontextprotocol/go-sdk v1.4.0"
is flagged with two HIGH vulnerabilities; update go.mod by adding a clear
comment above that dependency noting the vulnerabilities (GHSA-89xv-2j6f-qhc8
and GHSA-q382-vc8q-7jhj), that v1.4.1 fixes them but requires Go 1.25, and that
upgrade is deferred until the toolchain is available; create a tracking issue
(reference the GHSA IDs and v1.4.1) to perform the upgrade when Go 1.25 is
adopted, and add a short note in the repo (README or SECURITY.md) listing
temporary mitigations to apply while pinned to v1.4.0 (e.g., enforce CSRF
protections, tighten auth/rate-limiting) so reviewers can find and act on these
items.
---
Nitpick comments:
In `@internal/argocd/unhealthy_applications.go`:
- Around line 19-20: Remove the redundant package-level schema generation that
creates UnhealthyApplicationsInputSchema and UnhealthyApplicationsOutputSchema
with ignored errors; instead only declare those variables at package level and
rely on the init() function to populate them with proper error handling. Locate
the two var declarations for UnhealthyApplicationsInputSchema and
UnhealthyApplicationsOutputSchema and replace the schema creation expressions
with simple declarations (matching the pattern used in
unhealthy_application_resources.go), ensuring init() remains responsible for
calling jsonschema.For and handling errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 782dae4f-c93d-469b-88fd-891b4cd45de5
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
cmd/start_server.gogo.modinternal/argocd/unhealthy_application_resources.gointernal/argocd/unhealthy_application_resources_test.gointernal/argocd/unhealthy_applications.gointernal/argocd/unhealthy_applications_test.go
| github.com/google/jsonschema-go v0.4.2 | ||
| github.com/h2non/gock v1.2.0 | ||
| github.com/modelcontextprotocol/go-sdk v1.2.0 | ||
| github.com/modelcontextprotocol/go-sdk v1.4.0 |
There was a problem hiding this comment.
Security: mcp-go-sdk v1.4.0 has known HIGH severity vulnerabilities.
The static analysis tools have flagged two HIGH severity vulnerabilities in github.com/modelcontextprotocol/go-sdk v1.4.0:
-
GHSA-89xv-2j6f-qhc8 (CSRF): "The Go SDK's Streamable HTTP transport accepted browser-generated cross-site POST requests without validating the Origin header and without requiring …"
-
GHSA-q382-vc8q-7jhj (JSON Key Collusion): "The Model Context Protocol (MCP) Go SDK, via its dependency on segmentio/encoding, is vulnerable to JSON Key Collusion. The JSON parser improperly handles null Unicode characters during struct field mapping, allowing attackers to smuggle overriding keys past security filters and manipulate backend application logic."
Both vulnerabilities are fixed in v1.4.1. While the PR description notes v1.4.1 requires Go 1.25, consider:
- Adding a comment in
go.moddocumenting this known limitation - Creating a tracking issue to upgrade once Go 1.25 is available
- Evaluating if additional mitigations (e.g., authentication, rate limiting) can reduce the risk while pinned to v1.4.0
🧰 Tools
🪛 OSV Scanner (2.3.3)
[HIGH] 14-14: github.com/modelcontextprotocol/go-sdk 1.4.0: Cross-Site Tool Execution for HTTP Servers without Authorizatrion in github.com/modelcontextprotocol/go-sdk
[HIGH] 14-14: github.com/modelcontextprotocol/go-sdk 1.4.0: Improper handling of null Unicode character when parsing JSON in github.com/modelcontextprotocol/go-sdk
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 14, The dependency line
"github.com/modelcontextprotocol/go-sdk v1.4.0" is flagged with two HIGH
vulnerabilities; update go.mod by adding a clear comment above that dependency
noting the vulnerabilities (GHSA-89xv-2j6f-qhc8 and GHSA-q382-vc8q-7jhj), that
v1.4.1 fixes them but requires Go 1.25, and that upgrade is deferred until the
toolchain is available; create a tracking issue (reference the GHSA IDs and
v1.4.1) to perform the upgrade when Go 1.25 is adopted, and add a short note in
the repo (README or SECURITY.md) listing temporary mitigations to apply while
pinned to v1.4.0 (e.g., enforce CSRF protections, tighten auth/rate-limiting) so
reviewers can find and act on these items.
Signed-off-by: Xavier Coulon <xcoulon@redhat.com>
alexeykazakov
left a comment
alexeykazakov
left a comment
There was a problem hiding this comment.
Looks good. But afaik there is already 1.4.1 with some security bugs fixed.
| github.com/google/jsonschema-go v0.4.2 | ||
| github.com/h2non/gock v1.2.0 | ||
| github.com/modelcontextprotocol/go-sdk v1.2.0 | ||
| github.com/modelcontextprotocol/go-sdk v1.4.0 |
There was a problem hiding this comment.
| github.com/modelcontextprotocol/go-sdk v1.4.0 | |
| github.com/modelcontextprotocol/go-sdk v1.4.1 |
There was a problem hiding this comment.
ok, let me open another PR for to upgrade to github.com/modelcontextprotocol/go-sdk v1.4.1 and hence, to Go 1.25
2 participants
we need to stick to the mcp/go-sdk version 1.4.0
because the latest version (v1.4.1) upgraded to
Go 1.25 which is not compatible with our Go version :/
Also, validate the schemas for the tools, panic via
log.Fatalf()in theinit()functions.See modelcontextprotocol/go-sdk#470 for more details
on why we need to register an schema for the
metav1.Timetype (which usesthe
time.Timetype as an embedded field)Signed-off-by: Xavier Coulon xcoulon@redhat.com
Summary by CodeRabbit
Chores
Refactor
Tests
Chores