From 69c1c1fc081f5bb1292a46b5cb4a81d23e137179 Mon Sep 17 00:00:00 2001 From: Aviv Hadar Date: Fri, 13 Nov 2026 21:48:31 +0100 Subject: [PATCH] fix(get_assessment): reject limit outside 1-50 DEV-230024 Co-Authored-By: Claude Opus 5.5 --- pkg/clients/assessments_client.go | 2 +- pkg/tools/edit_assessment/tool.go | 2 +- pkg/tools/get_assessment/tool.go | 22 ++++- pkg/tools/get_assessment/tool_test.go | 120 ++++++++++++++++++++++++++ 4 files changed, 142 insertions(+), 4 deletions(-) create mode 100644 pkg/tools/get_assessment/tool_test.go diff --git a/pkg/clients/assessments_client.go b/pkg/clients/assessments_client.go index c9fb3282..f59c2a12 100644 --- a/pkg/clients/assessments_client.go +++ b/pkg/clients/assessments_client.go @@ -132,7 +132,7 @@ type ListAssessmentsParams struct { AssetID string `url:"assetId,omitempty"` LastModifiedFrom string `url:"lastModifiedFrom,omitempty"` LastModifiedTo string `url:"lastModifiedTo,omitempty"` - Limit int `url:"limit,omitempty"` + Limit *int `url:"limit,omitempty"` Cursor string `url:"cursor,omitempty"` } diff --git a/pkg/tools/edit_assessment/tool.go b/pkg/tools/edit_assessment/tool.go index 867dcbda..d3e62615 100644 --- a/pkg/tools/edit_assessment/tool.go +++ b/pkg/tools/edit_assessment/tool.go @@ -318,7 +318,7 @@ func resolveAssessmentID(ctx context.Context, client *http.Client, ref string) ( return r, nil } - paged, err := clients.ListAssessments(ctx, client, clients.ListAssessmentsParams{Name: r, Limit: 50}) + paged, err := clients.ListAssessments(ctx, client, clients.ListAssessmentsParams{Name: r, Limit: chip.Ptr(50)}) if err != nil { return "", fmt.Errorf("looking up assessment %q: %w", r, err) } diff --git a/pkg/tools/get_assessment/tool.go b/pkg/tools/get_assessment/tool.go index 9f20e092..96d4d4f3 100644 --- a/pkg/tools/get_assessment/tool.go +++ b/pkg/tools/get_assessment/tool.go @@ -12,6 +12,11 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" ) +const ( + minLimit = 1 + maxLimit = 50 +) + // Input has two modes. A DIRECT lookup (assessmentId or assessmentReviewId) // fetches a single assessment and can't be combined with anything else. A // FILTERED lookup uses any combination of the remaining fields and returns a @@ -29,7 +34,7 @@ type Input struct { AssetID string `json:"assetId,omitempty" jsonschema:"Filter: UUID of the asset the assessment was conducted on."` LastModifiedFrom string `json:"lastModifiedFrom,omitempty" jsonschema:"Filter: ISO-8601 timestamp; include assessments last modified at or after this time (e.g. 2026-07-01T00:00:00.000Z)."` LastModifiedTo string `json:"lastModifiedTo,omitempty" jsonschema:"Filter: ISO-8601 timestamp; include assessments last modified before this time."` - Limit int `json:"limit,omitempty" jsonschema:"Filter: max results to return (1-50, default 10)."` + Limit *int `json:"limit,omitempty" jsonschema:"Filter: max results to return (1-50, default 10)."` Cursor string `json:"cursor,omitempty" jsonschema:"Filter: pagination cursor from a previous response's nextCursor."` } @@ -74,6 +79,10 @@ func handler(collibraClient *http.Client) chip.ToolHandlerFunc[Input, Output] { return Output{}, fmt.Errorf("assessmentId/assessmentReviewId fetch a single assessment and cannot be combined with filters (name, status, template, assetId, dates, limit, cursor)") } + if err := validateLimit(input.Limit); err != nil { + return Output{}, err + } + // Validate the UUID-valued fields that are present. for _, f := range [][2]string{ {"assessmentId", input.AssessmentID}, @@ -137,7 +146,16 @@ func handler(collibraClient *http.Client) chip.ToolHandlerFunc[Input, Output] { // hasFilters reports whether any filter field is set. func hasFilters(in Input) bool { return in.Name != "" || in.Status != "" || in.TemplateID != "" || in.TemplateVersion != "" || - in.AssetID != "" || in.LastModifiedFrom != "" || in.LastModifiedTo != "" || in.Limit != 0 || in.Cursor != "" + in.AssetID != "" || in.LastModifiedFrom != "" || in.LastModifiedTo != "" || in.Limit != nil || in.Cursor != "" +} + +// validateLimit rejects a limit outside 1-50, matching the Assessments API. A +// nil limit is left out of the request so the API default applies. +func validateLimit(limit *int) error { + if limit != nil && (*limit < minLimit || *limit > maxLimit) { + return fmt.Errorf("limit must be between %d and %d; got %d", minLimit, maxLimit, *limit) + } + return nil } // fromList shapes a list lookup: one match returns as the single Assessment, diff --git a/pkg/tools/get_assessment/tool_test.go b/pkg/tools/get_assessment/tool_test.go new file mode 100644 index 00000000..e15e2433 --- /dev/null +++ b/pkg/tools/get_assessment/tool_test.go @@ -0,0 +1,120 @@ +package get_assessment_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/collibra/chip/pkg/chip" + tool "github.com/collibra/chip/pkg/tools/get_assessment" + "github.com/collibra/chip/pkg/tools/testutil" +) + +const limitError = "limit must be between 1 and 50; got " + +// listStub serves the list endpoint and records the limit query parameter of +// each request, so a test can tell whether the API was called and with what. +type listStub struct { + calls int + limits []string + hasLimit []bool +} + +func (s *listStub) client(t *testing.T) *http.Client { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("GET /rest/assessments/v2/assessments", func(w http.ResponseWriter, r *http.Request) { + s.calls++ + s.limits = append(s.limits, r.URL.Query().Get("limit")) + s.hasLimit = append(s.hasLimit, r.URL.Query().Has("limit")) + _ = json.NewEncoder(w).Encode(map[string]any{ + "results": []map[string]any{ + {"id": "assessment-1", "name": "First"}, + {"id": "assessment-2", "name": "Second"}, + }, + }) + }) + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + return testutil.NewClient(server) +} + +func TestGetAssessment_LimitOutOfRangeIsRejectedBeforeAnyCall(t *testing.T) { + for _, tc := range []struct { + name string + limit int + want string + }{ + {"zero", 0, limitError + "0"}, + {"negative", -1, limitError + "-1"}, + {"above max", 51, limitError + "51"}, + } { + t.Run(tc.name, func(t *testing.T) { + s := &listStub{} + _, err := tool.NewTool(s.client(t)).Handler(t.Context(), tool.Input{Limit: chip.Ptr(tc.limit)}) + if err == nil || err.Error() != tc.want { + t.Fatalf("want error %q, got %v", tc.want, err) + } + if s.calls != 0 { + t.Fatalf("want no API call for an invalid limit, got %d", s.calls) + } + }) + } +} + +func TestGetAssessment_LimitInRangeIsSentToTheAPI(t *testing.T) { + for _, tc := range []struct { + name string + limit int + want string + }{ + {"min", 1, "1"}, + {"max", 50, "50"}, + } { + t.Run(tc.name, func(t *testing.T) { + s := &listStub{} + out, err := tool.NewTool(s.client(t)).Handler(t.Context(), tool.Input{Limit: chip.Ptr(tc.limit)}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !out.Found || len(out.Assessments) != 2 { + t.Fatalf("want the listed assessments, got %+v", out) + } + if s.calls != 1 || s.limits[0] != tc.want { + t.Fatalf("want one call with limit=%s, got calls=%d limits=%v", tc.want, s.calls, s.limits) + } + }) + } +} + +func TestGetAssessment_NoLimitLeavesTheAPIDefault(t *testing.T) { + s := &listStub{} + out, err := tool.NewTool(s.client(t)).Handler(t.Context(), tool.Input{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !out.Found || len(out.Assessments) != 2 { + t.Fatalf("want the listed assessments, got %+v", out) + } + if s.calls != 1 || s.hasLimit[0] { + t.Fatalf("want one call without a limit parameter, got calls=%d limits=%v", s.calls, s.limits) + } +} + +// The MCP server decodes tool arguments from JSON, so an explicit "limit": 0 +// must decode to a set limit, not to "not set". +func TestGetAssessment_ExplicitZeroLimitInArgumentsIsRejected(t *testing.T) { + var input tool.Input + if err := json.Unmarshal([]byte(`{"limit": 0}`), &input); err != nil { + t.Fatalf("unmarshal: %v", err) + } + s := &listStub{} + _, err := tool.NewTool(s.client(t)).Handler(t.Context(), input) + if err == nil || err.Error() != limitError+"0" { + t.Fatalf("want error %q, got %v", limitError+"0", err) + } + if s.calls != 0 { + t.Fatalf("want no API call, got %d", s.calls) + } +}