From 2f89a469c3c26b694bdb46b8c406d85fa74e47a3 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 00:34:03 +0000 Subject: [PATCH 1/2] fix(mcp): stop silently returning unfiltered findings The MCP finding tools forwarded `repository`, `severity` and `status` to the API as raw query parameters. No endpoint accepts a repository *name*, and `severity`/`status` are only honoured by some: the API ignores parameters it does not recognise rather than rejecting them, so every one of those filters came back as a full, unfiltered page that read as a filtered one. `get_findings_for_repo("scrut-helpdesk-bot")` returned findings from scrut-organizationService, scrut-trust-center and others. Translate filters instead of forwarding them: - `repository` resolves to the numeric repository id the endpoints filter on (`githubRepositoryId` and the Azure/Bitbucket equivalents). The id comes from the /sca/repositories and /sast/repositories listings, which are keyed by it. /context/repositories is not usable here: its id is a Nullify ULID, and the API rejects one as `invalid integer value ... type 'int64'`. Non-numeric keys are skipped for the same reason. Resolved names are cached per host, since those listings cap `limit` at 10 and a miss costs a request per ten repos. - Filters an endpoint cannot apply are never sent. Single-endpoint tools fail with a message naming the filter; the fan-out tools report them per type as `unapplied_filters`, so a caller can tell which results are narrowed. Verified against a live instance: scrut-trust-center resolves to 480701645 and scrut-datasync-kolide to 1003604517 (the latter is ULID-keyed in /context/repositories and would previously have 400'd), and severity on /sca/dependencies/findings is reported unapplied rather than dropped. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01R27reGQuYtybuiX9RScNUG --- internal/mcp/filters.go | 240 +++++++++++++++++++++++++++++++ internal/mcp/filters_test.go | 242 ++++++++++++++++++++++++++++++++ internal/mcp/http.go | 18 ++- internal/mcp/tools_composite.go | 31 ++-- internal/mcp/tools_unified.go | 46 +++--- 5 files changed, 548 insertions(+), 29 deletions(-) create mode 100644 internal/mcp/filters.go create mode 100644 internal/mcp/filters_test.go diff --git a/internal/mcp/filters.go b/internal/mcp/filters.go new file mode 100644 index 0000000..0051925 --- /dev/null +++ b/internal/mcp/filters.go @@ -0,0 +1,240 @@ +package mcp + +import ( + "context" + "encoding/json" + "fmt" + "strings" + "sync" + + "github.com/nullify-platform/cli/internal/client" + + "github.com/mark3labs/mcp-go/mcp" +) + +// The Nullify API ignores query parameters it does not recognise instead of +// rejecting them, so a filter forwarded to an endpoint that does not support it +// comes back as a full, unfiltered page that reads as a filtered one. Every +// filter the MCP tools expose is therefore either translated into a parameter +// the endpoint actually accepts or reported as unapplied - never sent and hoped +// for. + +// listFilterSupport records which of the tool-level filters an endpoint honours +// server-side. Derived from the query parameters each generated client method +// forwards in internal/api. +type listFilterSupport struct { + severity bool + status bool +} + +var listEndpointFilters = map[string]listFilterSupport{ + "/sast/findings": {severity: true}, + "/sca/dependencies/findings": {}, + "/sca/containers/findings": {}, + "/secrets/findings": {}, + "/dast/pentest/findings": {}, + "/dast/bughunt/findings": {}, + "/cspm/findings": {severity: true, status: true}, +} + +// translatedFilters are the tool-level arguments handled by +// translateListFilters. They must not also be forwarded verbatim. +var translatedFilters = map[string]bool{ + "repository": true, + "severity": true, + "status": true, +} + +// repositoryIDParams maps the owner parameter identifying the VCS provider to +// the repository-id parameter its endpoints filter on. No endpoint accepts a +// repository name. +var repositoryIDParams = []struct{ ownerParam, repoParam string }{ + {"githubOwnerId", "githubRepositoryId"}, + {"azureOrganizationId", "azureRepositoryId"}, + {"bitbucketWorkspaceId", "bitbucketRepositoryId"}, +} + +// repoIDCache memoises repository name -> id per API host for the life of the +// process. Repository ids are stable, and the listing is paged, so resolving +// one name can otherwise cost several requests per tool call. +var repoIDCache sync.Map + +func repoCacheKey(baseURL, name string) string { + return baseURL + "\x00" + strings.ToLower(name) +} + +// repositoryIDParam returns the repository-id parameter for the configured VCS +// provider. +func repositoryIDParam(queryParams map[string]string) (string, error) { + for _, p := range repositoryIDParams { + if queryParams[p.ownerParam] != "" { + return p.repoParam, nil + } + } + if queryParams["gitlabGroupId"] != "" { + return "", fmt.Errorf("filtering by repository is not supported for GitLab: the API exposes no GitLab repository-id parameter") + } + return "", fmt.Errorf("cannot filter by repository: no VCS owner is configured in the stored credentials") +} + +// repositoryStatsEndpoints are the listings that expose the numeric VCS +// repository id the findings endpoints filter on. +// +// /context/repositories is deliberately not used here: its "id" is a +// Nullify-internal ULID, and passing one as githubRepositoryId is rejected with +// HTTP 400 ("invalid integer value ... type 'int64'"). These stats listings are +// keyed by the numeric id instead, which is the same value findings carry as +// repositoryId. +var repositoryStatsEndpoints = []string{"/sca/repositories", "/sast/repositories"} + +// resolveRepositoryID maps a repository name to the numeric id the findings +// endpoints filter on, paging the repository stats listings until the name +// matches or they are exhausted. +// +// The listings cap limit at 10, so a miss costs one request per ten +// repositories. Every name seen on the way is cached, so the cost is paid at +// most once per process for any repository that appears before the target. +func resolveRepositoryID(ctx context.Context, c *client.NullifyClient, queryParams map[string]string, name string) (string, error) { + if id, ok := repoIDCache.Load(repoCacheKey(c.BaseURL, name)); ok { + return id.(string), nil + } + + seen := 0 + for _, endpoint := range repositoryStatsEndpoints { + // Bounded so a listing that keeps handing back a nextToken cannot spin. + const maxPages = 200 + nextToken := "" + + for page := 0; page < maxPages; page++ { + extra := []string{"limit", "10"} + if nextToken != "" { + extra = append(extra, "nextToken", nextToken) + } + + result, err := doGet(ctx, c, endpoint+buildQueryString(queryParams, extra...)) + if err != nil { + return "", err + } + body, err := resultText(result) + if err != nil { + return "", fmt.Errorf("could not list repositories from %s: %w", endpoint, err) + } + + // Keys differ in case between the two listings (Repository vs + // repository); encoding/json matches field names case-insensitively, + // so one set of tags covers both. + var payload struct { + Repositories map[string]struct { + Repository string `json:"repository"` + } `json:"repositories"` + NextToken string `json:"nextToken"` + } + if err := json.Unmarshal([]byte(body), &payload); err != nil { + return "", fmt.Errorf("could not parse the repository listing from %s: %w", endpoint, err) + } + + for id, repo := range payload.Repositories { + // The API types this parameter as int64 and rejects anything + // else, so skip a key that is not numeric rather than provoke a + // 400 later. + if repo.Repository == "" || !isNumericID(id) { + continue + } + repoIDCache.Store(repoCacheKey(c.BaseURL, repo.Repository), id) + seen++ + if strings.EqualFold(repo.Repository, name) { + return id, nil + } + } + + if payload.NextToken == "" || len(payload.Repositories) == 0 { + break + } + nextToken = payload.NextToken + } + } + + return "", fmt.Errorf("repository %q is not among the %d scanned repositories on this Nullify instance", name, seen) +} + +func isNumericID(s string) bool { + if s == "" { + return false + } + for _, r := range s { + if r < '0' || r > '9' { + return false + } + } + return true +} + +// translateListFilters converts tool-level filter arguments for one list +// endpoint into query parameters that endpoint honours. Filters it cannot apply +// are returned in unapplied instead of being sent. +func translateListFilters( + ctx context.Context, + c *client.NullifyClient, + basePath string, + args map[string]any, + queryParams map[string]string, +) (extra []string, unapplied []string, err error) { + support := listEndpointFilters[basePath] + + if repository := getStringArg(args, "repository"); repository != "" { + param, err := repositoryIDParam(queryParams) + if err != nil { + return nil, nil, err + } + id, err := resolveRepositoryID(ctx, c, queryParams, repository) + if err != nil { + return nil, nil, err + } + extra = append(extra, param, id) + } + + if severity := getStringArg(args, "severity"); severity != "" { + if support.severity { + extra = append(extra, "severity", severity) + } else { + unapplied = append(unapplied, "severity") + } + } + + if status := getStringArg(args, "status"); status != "" { + if support.status { + extra = append(extra, "status", status) + } else { + unapplied = append(unapplied, "status") + } + } + + return extra, unapplied, nil +} + +// unsupportedFilterError describes filters an endpoint cannot apply, for tools +// that surface a single endpoint and so must fail rather than return results +// the caller would read as filtered. +func unsupportedFilterError(basePath string, unapplied []string) error { + return fmt.Errorf( + "%s cannot filter by %s; omit the filter and narrow the results yourself", + basePath, + strings.Join(unapplied, " or "), + ) +} + +// resultText extracts the text payload of a tool result, turning an error +// result back into an error. +func resultText(result *mcp.CallToolResult) (string, error) { + if result == nil || len(result.Content) == 0 { + return "", fmt.Errorf("empty response") + } + tc, ok := result.Content[0].(mcp.TextContent) + if !ok { + return "", fmt.Errorf("unexpected response content type %T", result.Content[0]) + } + if result.IsError { + return "", fmt.Errorf("%s", tc.Text) + } + return tc.Text, nil +} diff --git a/internal/mcp/filters_test.go b/internal/mcp/filters_test.go new file mode 100644 index 0000000..fa09e67 --- /dev/null +++ b/internal/mcp/filters_test.go @@ -0,0 +1,242 @@ +package mcp + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "reflect" + "testing" + + "github.com/nullify-platform/cli/internal/client" +) + +func resetRepoIDCache() { + repoIDCache.Range(func(k, _ any) bool { + repoIDCache.Delete(k) + return true + }) +} + +func TestRepositoryIDParam(t *testing.T) { + tests := []struct { + name string + queryParams map[string]string + want string + wantErr bool + }{ + {"github", map[string]string{"githubOwnerId": "69645079"}, "githubRepositoryId", false}, + {"azure", map[string]string{"azureOrganizationId": "org"}, "azureRepositoryId", false}, + {"bitbucket", map[string]string{"bitbucketWorkspaceId": "ws"}, "bitbucketRepositoryId", false}, + {"gitlab is unsupported", map[string]string{"gitlabGroupId": "g"}, "", true}, + {"no owner configured", map[string]string{}, "", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := repositoryIDParam(tt.queryParams) + if tt.wantErr { + if err == nil { + t.Fatalf("expected an error, got param %q", got) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Errorf("got %q, want %q", got, tt.want) + } + }) + } +} + +// newRepoListServer serves paged repository-stats listings shaped like the real +// ones - keyed by numeric id, PascalCase on /sca and camelCase on /sast - and +// records the paths requested against it. +func newRepoListServer(t *testing.T) (*client.NullifyClient, *[]string) { + t.Helper() + var paths []string + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + paths = append(paths, r.URL.String()) + w.Header().Set("Content-Type", "application/json") + + switch { + case r.URL.Path == "/sca/repositories" && r.URL.Query().Get("nextToken") == "10": + // A ULID-keyed entry must be skipped, not sent on as an int64. + _, _ = w.Write([]byte(`{"repositories":{"222":{"ID":"222","Repository":"scrut-kaiService"},` + + `"01HK153X00FYFN3CT5C25VNY1T":{"ID":"01HK153X00FYFN3CT5C25VNY1T","Repository":"scrut-ulid-only"}},` + + `"nextToken":""}`)) + case r.URL.Path == "/sca/repositories": + _, _ = w.Write([]byte(`{"repositories":{"111":{"ID":"111","Repository":"scrut-helpdesk-bot"}},"nextToken":"10"}`)) + default: + // /sast/repositories, camelCase keys. + _, _ = w.Write([]byte(`{"repositories":{"333":{"id":"333","repository":"scrut-sast-only"}},"nextToken":""}`)) + } + })) + t.Cleanup(srv.Close) + + return &client.NullifyClient{BaseURL: srv.URL, HttpClient: srv.Client()}, &paths +} + +func TestResolveRepositoryIDPagesUntilFound(t *testing.T) { + resetRepoIDCache() + c, paths := newRepoListServer(t) + + // On the second page, so the resolver must follow nextToken. + id, err := resolveRepositoryID(context.Background(), c, map[string]string{}, "scrut-kaiService") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if id != "222" { + t.Errorf("got id %q, want 222", id) + } + if len(*paths) != 2 { + t.Errorf("expected 2 listing requests, got %d: %v", len(*paths), *paths) + } + + // A name seen while paging is cached, so no further requests are made. + before := len(*paths) + id, err = resolveRepositoryID(context.Background(), c, map[string]string{}, "SCRUT-HELPDESK-BOT") + if err != nil { + t.Fatalf("unexpected error on cached lookup: %v", err) + } + if id != "111" { + t.Errorf("got id %q, want 111", id) + } + if len(*paths) != before { + t.Errorf("cached lookup issued %d extra requests", len(*paths)-before) + } +} + +func TestResolveRepositoryIDFallsThroughToSAST(t *testing.T) { + resetRepoIDCache() + c, _ := newRepoListServer(t) + + id, err := resolveRepositoryID(context.Background(), c, map[string]string{}, "scrut-sast-only") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if id != "333" { + t.Errorf("got id %q, want 333", id) + } +} + +// A ULID id is rejected by the API as an int64, so it must never be resolved. +func TestResolveRepositoryIDSkipsNonNumericIDs(t *testing.T) { + resetRepoIDCache() + c, _ := newRepoListServer(t) + + if id, err := resolveRepositoryID(context.Background(), c, map[string]string{}, "scrut-ulid-only"); err == nil { + t.Fatalf("expected an error for a ULID-keyed repository, got id %q", id) + } +} + +func TestResolveRepositoryIDUnknownNameErrors(t *testing.T) { + resetRepoIDCache() + c, _ := newRepoListServer(t) + + if _, err := resolveRepositoryID(context.Background(), c, map[string]string{}, "not-a-repo"); err == nil { + t.Fatal("expected an error for an unknown repository, got nil") + } +} + +func TestTranslateListFiltersPushesRepositoryDown(t *testing.T) { + resetRepoIDCache() + c, _ := newRepoListServer(t) + queryParams := map[string]string{"githubOwnerId": "69645079"} + args := map[string]any{"repository": "scrut-helpdesk-bot"} + + extra, unapplied, err := translateListFilters( + context.Background(), c, "/sca/dependencies/findings", args, queryParams) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(unapplied) != 0 { + t.Errorf("expected no unapplied filters, got %v", unapplied) + } + if want := []string{"githubRepositoryId", "111"}; !reflect.DeepEqual(extra, want) { + t.Errorf("got %v, want %v", extra, want) + } + + // The repository name itself must never reach the API: it is not a + // parameter any endpoint understands, and unknown parameters are ignored + // rather than rejected. + qs := buildQueryString(queryParams, extra...) + parsed, err := url.ParseQuery(qs[1:]) + if err != nil { + t.Fatalf("could not parse query string %q: %v", qs, err) + } + if parsed.Has("repository") { + t.Errorf("query string still carries a repository name: %q", qs) + } +} + +func TestTranslateListFiltersReportsUnsupported(t *testing.T) { + resetRepoIDCache() + c, _ := newRepoListServer(t) + + tests := []struct { + name string + basePath string + args map[string]any + wantExtra []string + wantUnapplie []string + }{ + { + name: "sast supports severity", + basePath: "/sast/findings", + args: map[string]any{"severity": "critical"}, + wantExtra: []string{"severity", "critical"}, + wantUnapplie: nil, + }, + { + name: "sca dependencies supports neither", + basePath: "/sca/dependencies/findings", + args: map[string]any{"severity": "critical", "status": "open"}, + wantExtra: nil, + wantUnapplie: []string{"severity", "status"}, + }, + { + name: "sast does not support status", + basePath: "/sast/findings", + args: map[string]any{"status": "open"}, + wantExtra: nil, + wantUnapplie: []string{"status"}, + }, + { + name: "cspm supports both", + basePath: "/cspm/findings", + args: map[string]any{"severity": "high", "status": "open"}, + wantExtra: []string{"severity", "high", "status", "open"}, + wantUnapplie: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + extra, unapplied, err := translateListFilters( + context.Background(), c, tt.basePath, tt.args, map[string]string{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !reflect.DeepEqual(extra, tt.wantExtra) { + t.Errorf("extra: got %v, want %v", extra, tt.wantExtra) + } + if !reflect.DeepEqual(unapplied, tt.wantUnapplie) { + t.Errorf("unapplied: got %v, want %v", unapplied, tt.wantUnapplie) + } + }) + } +} + +// Every endpoint the finding tools fan out over needs a support entry; +// a missing one silently reads as "supports nothing". +func TestListEndpointFiltersCoversAllFindingTypes(t *testing.T) { + for name, cfg := range findingTypes { + if _, ok := listEndpointFilters[cfg.basePath]; !ok { + t.Errorf("finding type %q (%s) has no listEndpointFilters entry", name, cfg.basePath) + } + } +} diff --git a/internal/mcp/http.go b/internal/mcp/http.go index d7ed056..c099c2a 100644 --- a/internal/mcp/http.go +++ b/internal/mcp/http.go @@ -63,18 +63,34 @@ func doRequest(ctx context.Context, c *client.NullifyClient, method string, path return toolResult(string(body)), nil } -// makeGetHandler creates a handler for list endpoints with standard filtering +// makeGetHandler creates a handler for list endpoints with standard filtering. +// Filters the endpoint cannot apply are rejected rather than forwarded: the API +// ignores unknown query parameters, so forwarding one returns an unfiltered +// page that the caller would read as filtered. func makeGetHandler(c *client.NullifyClient, basePath string, queryParams map[string]string) server.ToolHandlerFunc { return func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { args := request.GetArguments() extra := []string{} for key := range args { + if translatedFilters[key] { + continue + } if key == "limit" { extra = append(extra, key, fmt.Sprintf("%d", getIntArg(args, key, 20))) } else if s := getStringArg(args, key); s != "" { extra = append(extra, key, s) } } + + translated, unapplied, err := translateListFilters(ctx, c, basePath, args, queryParams) + if err != nil { + return toolError(err), nil + } + if len(unapplied) > 0 { + return toolError(unsupportedFilterError(basePath, unapplied)), nil + } + extra = append(extra, translated...) + qs := buildQueryString(queryParams, extra...) return doGet(ctx, c, basePath+qs) } diff --git a/internal/mcp/tools_composite.go b/internal/mcp/tools_composite.go index bfd30f7..4657066 100644 --- a/internal/mcp/tools_composite.go +++ b/internal/mcp/tools_composite.go @@ -74,12 +74,6 @@ func registerCompositeTools(s *server.MCPServer, c *client.NullifyClient, queryP severity := getStringArg(args, "severity") limit := getIntArg(args, "limit", 20) - extra := []string{"repository", repo} - if severity != "" { - extra = append(extra, "severity", severity) - } - extra = append(extra, "limit", fmt.Sprintf("%d", limit)) - endpoints := []struct { name string path string @@ -93,17 +87,34 @@ func registerCompositeTools(s *server.MCPServer, c *client.NullifyClient, queryP {"cspm", "/cspm/findings"}, } + // The repository name is the whole point of this tool, so a name + // that cannot be turned into the repository-id parameter the API + // filters on is an error - returning every repository's findings + // under this tool's name would be worse than failing. + filterArgs := map[string]any{"repository": repo, "severity": severity} + var parts []string - qs := buildQueryString(queryParams, extra...) for _, ep := range endpoints { - result, err := doGet(ctx, c, ep.path+qs) + translated, unapplied, err := translateListFilters(ctx, c, ep.path, filterArgs, queryParams) + if err != nil { + return toolError(err), nil + } + + extra := append([]string{"limit", fmt.Sprintf("%d", limit)}, translated...) + header := fmt.Sprintf("--- %s ---", ep.name) + if len(unapplied) > 0 { + header += fmt.Sprintf("\nNOTE: not filtered by %s (unsupported by %s)", + strings.Join(unapplied, ", "), ep.path) + } + + result, err := doGet(ctx, c, ep.path+buildQueryString(queryParams, extra...)) if err != nil { - parts = append(parts, fmt.Sprintf("--- %s ---\nError: %v", ep.name, err)) + parts = append(parts, fmt.Sprintf("%s\nError: %v", header, err)) continue } if len(result.Content) > 0 { if tc, ok := result.Content[0].(mcp.TextContent); ok { - parts = append(parts, fmt.Sprintf("--- %s ---\n%s", ep.name, tc.Text)) + parts = append(parts, fmt.Sprintf("%s\n%s", header, tc.Text)) } } } diff --git a/internal/mcp/tools_unified.go b/internal/mcp/tools_unified.go index 8112235..54c7b50 100644 --- a/internal/mcp/tools_unified.go +++ b/internal/mcp/tools_unified.go @@ -86,23 +86,15 @@ func registerUnifiedTools(s *server.MCPServer, c *client.NullifyClient, queryPar repository := getStringArg(args, "repository") limit := getIntArg(args, "limit", 20) - extra := []string{} - if severity != "" { - extra = append(extra, "severity", severity) - } - if status != "" { - extra = append(extra, "status", status) - } - if repository != "" { - extra = append(extra, "repository", repository) - } - extra = append(extra, "limit", fmt.Sprintf("%d", limit)) - qs := buildQueryString(queryParams, extra...) - + // searchResult reports per type, because the endpoints do not all + // support the same filters. UnappliedFilters names the ones that + // type's results are NOT narrowed by, so a caller never mistakes an + // unfiltered page for a filtered one. type searchResult struct { - Type string `json:"type"` - Error string `json:"error,omitempty"` - Data json.RawMessage `json:"data,omitempty"` + Type string `json:"type"` + Error string `json:"error,omitempty"` + UnappliedFilters []string `json:"unapplied_filters,omitempty"` + Data json.RawMessage `json:"data,omitempty"` } // Determine which types to query @@ -114,17 +106,35 @@ func registerUnifiedTools(s *server.MCPServer, c *client.NullifyClient, queryPar types = []string{typeName} } + filterArgs := map[string]any{ + "repository": repository, + "severity": severity, + "status": status, + } + var results []searchResult for _, t := range types { cfg := findingTypes[t] - result, err := doGet(ctx, c, cfg.basePath+qs) + + translated, unapplied, err := translateListFilters(ctx, c, cfg.basePath, filterArgs, queryParams) if err != nil { results = append(results, searchResult{Type: t, Error: err.Error()}) continue } + + extra := append([]string{"limit", fmt.Sprintf("%d", limit)}, translated...) + result, err := doGet(ctx, c, cfg.basePath+buildQueryString(queryParams, extra...)) + if err != nil { + results = append(results, searchResult{Type: t, Error: err.Error(), UnappliedFilters: unapplied}) + continue + } if len(result.Content) > 0 { if tc, ok := result.Content[0].(mcp.TextContent); ok { - results = append(results, searchResult{Type: t, Data: json.RawMessage(tc.Text)}) + results = append(results, searchResult{ + Type: t, + UnappliedFilters: unapplied, + Data: json.RawMessage(tc.Text), + }) } } } From b311b64f676a63593950d9efea39b744e5c6bce1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 01:26:48 +0000 Subject: [PATCH 2/2] feat(mcp): add the Agent Toolkit AWS MCP server, credential-scoped SubImage models NACLs and route tables but its collection is incomplete (subnet-to-NACL edges resolve for 2 of 7 subnets we checked, and NACL rule nodes carry no port range), and it models no WAF at all. An AWS-backed server gives us ground truth for those questions. Wires https://aws-mcp.us-east-1.api.aws/mcp through mcp-proxy-for-aws, behind the existing with-op-env.sh secret plumbing. Two things this had to design around: - Cloud-session sandboxes already export AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY for their own object storage, and with-op-env.sh leaves already-set vars untouched by design. Credentials named AWS_* would never have been fetched from 1Password, and the server would have signed every call as the sandbox - querying the wrong account and reporting the answer as ours. They travel as SCRUT_AWS_* and are mapped onto AWS_* after the lookup. The launcher exits non-zero rather than fall back to the ambient credentials. - --read-only is kept on, which means the server currently answers documentation questions only. Measured against the live endpoint it exposes 6 of 9 tools; the two that reach the AWS API (aws___call_aws, aws___run_script) are annotated destructiveHint=true and are dropped, so it cannot yet describe a NACL. Removing the flag is a deliberate follow-up that needs a strictly read-only IAM principal behind SCRUT_AWS_*, since aws___run_script runs arbitrary boto3. Inert until SCRUT_AWS_ACCESS_KEY_ID / SCRUT_AWS_SECRET_ACCESS_KEY exist in the 1Password item; the server fails to start and the others are unaffected. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01R27reGQuYtybuiX9RScNUG --- .claude/mcp/aws-serve.sh | 63 ++++++++++++++++++++++++++++++++++++++++ .mcp.json | 10 +++++++ 2 files changed, 73 insertions(+) create mode 100755 .claude/mcp/aws-serve.sh diff --git a/.claude/mcp/aws-serve.sh b/.claude/mcp/aws-serve.sh new file mode 100755 index 0000000..aec076b --- /dev/null +++ b/.claude/mcp/aws-serve.sh @@ -0,0 +1,63 @@ +#!/bin/bash +# Start the Agent Toolkit AWS MCP server (https://aws-mcp.us-east-1.api.aws/mcp) +# against dedicated read-only credentials. +# +# Why the credentials travel as SCRUT_AWS_* rather than AWS_*: +# cloud-session sandboxes already export AWS_ACCESS_KEY_ID and +# AWS_SECRET_ACCESS_KEY for their own object storage, and with-op-env.sh +# deliberately leaves already-set vars untouched. Credentials named AWS_* would +# therefore never be fetched from 1Password, and this server would sign its +# calls with the sandbox's identity - querying the wrong account and reporting +# the result as though it were ours. They are mapped onto AWS_* here instead, +# after the 1Password lookup has happened. +# +# --read-only drops every tool whose readOnlyHint is not true. Measured against +# the live endpoint, that leaves 6 of 9 tools: documentation search, skill +# retrieval and region metadata. +# +# READ THIS BEFORE REMOVING THE FLAG. The two tools that actually reach the AWS +# API - aws___call_aws and aws___run_script - are annotated readOnlyHint=false, +# destructiveHint=true, because they are generic escape hatches that *can* +# write whatever they are asked for; aws___run_script executes arbitrary Python +# with boto3. --read-only therefore excludes them, and with the flag on this +# server answers documentation questions only: it cannot describe a NACL, a +# route table or a WAF web ACL. +# +# Making it answer live infrastructure questions means dropping --read-only, +# at which point the IAM principal in SCRUT_AWS_* is the only thing preventing +# a mutation. Do that only once those credentials are a strictly read-only +# principal (AWS managed ReadOnlyAccess, or tighter). +# +# (The proxy's own --help contains a contradictory example captioned "Run with +# write permissions enabled" for this same flag. The flag description -- +# "Disable tools which may require write permissions" -- is the accurate one; +# the tool counts above were measured, not inferred from the docs.) +set -uo pipefail + +if [ -z "${SCRUT_AWS_ACCESS_KEY_ID:-}" ] || [ -z "${SCRUT_AWS_SECRET_ACCESS_KEY:-}" ]; then + echo "aws-serve: SCRUT_AWS_ACCESS_KEY_ID / SCRUT_AWS_SECRET_ACCESS_KEY were not resolved." >&2 + echo "aws-serve: refusing to start rather than sign AWS calls with the sandbox's ambient credentials." >&2 + echo "aws-serve: add them to op://\${CLAUDE_OP_VAULT:-Claude}/\${CLAUDE_OP_ITEM:-cloud-session-env}." >&2 + exit 1 +fi + +export AWS_ACCESS_KEY_ID="$SCRUT_AWS_ACCESS_KEY_ID" +export AWS_SECRET_ACCESS_KEY="$SCRUT_AWS_SECRET_ACCESS_KEY" +if [ -n "${SCRUT_AWS_SESSION_TOKEN:-}" ]; then + export AWS_SESSION_TOKEN="$SCRUT_AWS_SESSION_TOKEN" +else + # A stale ambient session token would invalidate the key pair above. + unset AWS_SESSION_TOKEN +fi +unset SCRUT_AWS_ACCESS_KEY_ID SCRUT_AWS_SECRET_ACCESS_KEY SCRUT_AWS_SESSION_TOKEN + +# Default region only. The estate spans us-east-1, us-east-2, us-west-2, +# eu-north-1, ap-south-1 and ap-southeast-2, so region-specific questions must +# name their region in the call rather than relying on this. +export AWS_REGION="${SCRUT_AWS_REGION:-us-east-2}" + +exec uvx mcp-proxy-for-aws@1.6.4 \ + https://aws-mcp.us-east-1.api.aws/mcp \ + --read-only \ + --region "$AWS_REGION" \ + --metadata "AWS_REGION=${AWS_REGION}" diff --git a/.mcp.json b/.mcp.json index 8504a16..8eddca7 100644 --- a/.mcp.json +++ b/.mcp.json @@ -29,6 +29,16 @@ "--", "latent-defense-mcp" ] + }, + "aws": { + "command": "bash", + "args": [ + ".claude/mcp/with-op-env.sh", + "SCRUT_AWS_ACCESS_KEY_ID", "SCRUT_AWS_SECRET_ACCESS_KEY", + "SCRUT_AWS_SESSION_TOKEN", "SCRUT_AWS_REGION", + "--", + "bash", ".claude/mcp/aws-serve.sh" + ] } } }