From 2a1584fb4fbc20072291de234723cdb21451bc3d Mon Sep 17 00:00:00 2001 From: root Date: Sat, 6 Jun 2026 21:59:02 +0000 Subject: [PATCH 01/12] Add MCP tools for repository security advisory lifecycle Expose create, update, and CVE request operations in the security_advisories toolset so security teams can manage advisories without leaving MCP workflows. Closes #2506 --- README.md | 40 +- .../create_repository_security_advisory.snap | 155 +++++ ..._cve_for_repository_security_advisory.snap | 29 + .../update_repository_security_advisory.snap | 163 +++++ pkg/github/helper_test.go | 11 +- pkg/github/security_advisories_write.go | 594 ++++++++++++++++++ pkg/github/security_advisories_write_test.go | 407 ++++++++++++ pkg/github/tools.go | 3 + 8 files changed, 1397 insertions(+), 5 deletions(-) create mode 100644 pkg/github/__toolsnaps__/create_repository_security_advisory.snap create mode 100644 pkg/github/__toolsnaps__/request_cve_for_repository_security_advisory.snap create mode 100644 pkg/github/__toolsnaps__/update_repository_security_advisory.snap create mode 100644 pkg/github/security_advisories_write.go create mode 100644 pkg/github/security_advisories_write_test.go diff --git a/README.md b/README.md index dff62321b8..5aed8e3196 100644 --- a/README.md +++ b/README.md @@ -1122,7 +1122,7 @@ The following sets of tools are available: 2. get_diff - Get the diff of a pull request. 3. get_status - Get combined commit status of a head commit in a pull request. 4. get_files - Get the list of files changed in a pull request. Use with pagination parameters to control the number of results returned. - 5. get_review_comments - Get review threads on a pull request. Each thread contains logically grouped review comments made on the same code location during pull request reviews. Returns threads with metadata (isResolved, isOutdated, isCollapsed) and their associated comments. Use cursor-based pagination (perPage, after) to control results. + 5. get_review_comments - Get review threads on a pull request. Each thread contains logically grouped review comments made on the same code location during pull request reviews. Returns threads with metadata (isResolved, isOutdated, isCollapsed) and their associated comments. Review comments include structured code suggestions when available, including Copilot-generated "Suggest" changesets (via thread partial) and human-authored suggestion code blocks in the comment body. Use cursor-based pagination (perPage, after) to control results. 6. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method. Use with pagination parameters to control the number of results returned. 7. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned. 8. get_check_runs - Get check runs for the head commit of a pull request. Check runs are the individual CI/CD jobs and checks that run on the PR. @@ -1357,6 +1357,21 @@ The following sets of tools are available: shield Security Advisories +- **create_repository_security_advisory** - Create repository security advisory + - **Required OAuth Scopes**: `security_events` + - **Accepted OAuth Scopes**: `repo`, `security_events` + - `credits`: Users credited for the advisory. (object[], optional) + - `cveId`: The CVE ID to assign to the advisory. (string, optional) + - `cvssVectorString`: The CVSS vector string for the advisory. (string, optional) + - `cweIds`: Common Weakness Enumeration IDs (for example, ["CWE-79"]). (string[], optional) + - `description`: A detailed description of the security advisory. (string, required) + - `owner`: The owner of the repository. (string, required) + - `repo`: The name of the repository. (string, required) + - `severity`: The severity of the advisory. (string, optional) + - `startPrivateFork`: Whether to create a temporary private fork for collaborating on a fix. (boolean, optional) + - `summary`: A short summary of the security advisory. (string, required) + - `vulnerabilities`: Affected products and version ranges. (object[], required) + - **get_global_security_advisory** - Get a global security advisory - **Required OAuth Scopes**: `security_events` - **Accepted OAuth Scopes**: `repo`, `security_events` @@ -1394,6 +1409,29 @@ The following sets of tools are available: - `sort`: Sort field. (string, optional) - `state`: Filter by advisory state. (string, optional) +- **request_cve_for_repository_security_advisory** - Request CVE for repository security advisory + - **Required OAuth Scopes**: `security_events` + - **Accepted OAuth Scopes**: `repo`, `security_events` + - `ghsaId`: GitHub Security Advisory ID (format: GHSA-xxxx-xxxx-xxxx). (string, required) + - `owner`: The owner of the repository. (string, required) + - `repo`: The name of the repository. (string, required) + +- **update_repository_security_advisory** - Update repository security advisory + - **Required OAuth Scopes**: `security_events` + - **Accepted OAuth Scopes**: `repo`, `security_events` + - `credits`: Users credited for the advisory. (object[], optional) + - `cveId`: The CVE ID to assign to the advisory. (string, optional) + - `cvssVectorString`: The CVSS vector string for the advisory. (string, optional) + - `cweIds`: Common Weakness Enumeration IDs (for example, ["CWE-79"]). (string[], optional) + - `description`: A detailed description of the security advisory. (string, optional) + - `ghsaId`: GitHub Security Advisory ID (format: GHSA-xxxx-xxxx-xxxx). (string, required) + - `owner`: The owner of the repository. (string, required) + - `repo`: The name of the repository. (string, required) + - `severity`: The severity of the advisory. (string, optional) + - `state`: The advisory state. Set to "published" to publish the advisory. (string, optional) + - `summary`: A short summary of the security advisory. (string, optional) + - `vulnerabilities`: Affected products and version ranges. (object[], optional) +
diff --git a/pkg/github/__toolsnaps__/create_repository_security_advisory.snap b/pkg/github/__toolsnaps__/create_repository_security_advisory.snap new file mode 100644 index 0000000000..8e8c981146 --- /dev/null +++ b/pkg/github/__toolsnaps__/create_repository_security_advisory.snap @@ -0,0 +1,155 @@ +{ + "annotations": { + "title": "Create repository security advisory" + }, + "description": "Create a draft repository security advisory.", + "inputSchema": { + "properties": { + "credits": { + "description": "Users credited for the advisory.", + "items": { + "properties": { + "login": { + "description": "The GitHub username of the credited user.", + "type": "string" + }, + "type": { + "description": "The credit type.", + "enum": [ + "analyst", + "finder", + "reporter", + "coordinator", + "remediation_developer", + "remediation_reviewer", + "remediation_verifier", + "tool", + "sponsor", + "other" + ], + "type": "string" + } + }, + "required": [ + "login", + "type" + ], + "type": "object" + }, + "type": "array" + }, + "cveId": { + "description": "The CVE ID to assign to the advisory.", + "type": "string" + }, + "cvssVectorString": { + "description": "The CVSS vector string for the advisory.", + "type": "string" + }, + "cweIds": { + "description": "Common Weakness Enumeration IDs (for example, [\"CWE-79\"]).", + "items": { + "type": "string" + }, + "type": "array" + }, + "description": { + "description": "A detailed description of the security advisory.", + "type": "string" + }, + "owner": { + "description": "The owner of the repository.", + "type": "string" + }, + "repo": { + "description": "The name of the repository.", + "type": "string" + }, + "severity": { + "description": "The severity of the advisory.", + "enum": [ + "low", + "medium", + "high", + "critical" + ], + "type": "string" + }, + "startPrivateFork": { + "description": "Whether to create a temporary private fork for collaborating on a fix.", + "type": "boolean" + }, + "summary": { + "description": "A short summary of the security advisory.", + "type": "string" + }, + "vulnerabilities": { + "description": "Affected products and version ranges.", + "items": { + "properties": { + "package": { + "properties": { + "ecosystem": { + "description": "The package ecosystem.", + "enum": [ + "actions", + "composer", + "erlang", + "go", + "maven", + "npm", + "nuget", + "other", + "pip", + "pub", + "rubygems", + "rust", + "swift" + ], + "type": "string" + }, + "name": { + "description": "The package name.", + "type": "string" + } + }, + "required": [ + "ecosystem" + ], + "type": "object" + }, + "patched_versions": { + "description": "The version that patches the vulnerability.", + "type": "string" + }, + "vulnerable_functions": { + "description": "Functions in the package that are affected.", + "items": { + "type": "string" + }, + "type": "array" + }, + "vulnerable_version_range": { + "description": "The range of affected versions (for example, \"\u003c 2.0.0\").", + "type": "string" + } + }, + "required": [ + "package" + ], + "type": "object" + }, + "type": "array" + } + }, + "required": [ + "owner", + "repo", + "summary", + "description", + "vulnerabilities" + ], + "type": "object" + }, + "name": "create_repository_security_advisory" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/request_cve_for_repository_security_advisory.snap b/pkg/github/__toolsnaps__/request_cve_for_repository_security_advisory.snap new file mode 100644 index 0000000000..5d069b8259 --- /dev/null +++ b/pkg/github/__toolsnaps__/request_cve_for_repository_security_advisory.snap @@ -0,0 +1,29 @@ +{ + "annotations": { + "title": "Request CVE for repository security advisory" + }, + "description": "Request a CVE ID from GitHub for a draft repository security advisory.", + "inputSchema": { + "properties": { + "ghsaId": { + "description": "GitHub Security Advisory ID (format: GHSA-xxxx-xxxx-xxxx).", + "type": "string" + }, + "owner": { + "description": "The owner of the repository.", + "type": "string" + }, + "repo": { + "description": "The name of the repository.", + "type": "string" + } + }, + "required": [ + "owner", + "repo", + "ghsaId" + ], + "type": "object" + }, + "name": "request_cve_for_repository_security_advisory" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/update_repository_security_advisory.snap b/pkg/github/__toolsnaps__/update_repository_security_advisory.snap new file mode 100644 index 0000000000..97057570aa --- /dev/null +++ b/pkg/github/__toolsnaps__/update_repository_security_advisory.snap @@ -0,0 +1,163 @@ +{ + "annotations": { + "title": "Update repository security advisory" + }, + "description": "Update a repository security advisory, including publishing it.", + "inputSchema": { + "properties": { + "credits": { + "description": "Users credited for the advisory.", + "items": { + "properties": { + "login": { + "description": "The GitHub username of the credited user.", + "type": "string" + }, + "type": { + "description": "The credit type.", + "enum": [ + "analyst", + "finder", + "reporter", + "coordinator", + "remediation_developer", + "remediation_reviewer", + "remediation_verifier", + "tool", + "sponsor", + "other" + ], + "type": "string" + } + }, + "required": [ + "login", + "type" + ], + "type": "object" + }, + "type": "array" + }, + "cveId": { + "description": "The CVE ID to assign to the advisory.", + "type": "string" + }, + "cvssVectorString": { + "description": "The CVSS vector string for the advisory.", + "type": "string" + }, + "cweIds": { + "description": "Common Weakness Enumeration IDs (for example, [\"CWE-79\"]).", + "items": { + "type": "string" + }, + "type": "array" + }, + "description": { + "description": "A detailed description of the security advisory.", + "type": "string" + }, + "ghsaId": { + "description": "GitHub Security Advisory ID (format: GHSA-xxxx-xxxx-xxxx).", + "type": "string" + }, + "owner": { + "description": "The owner of the repository.", + "type": "string" + }, + "repo": { + "description": "The name of the repository.", + "type": "string" + }, + "severity": { + "description": "The severity of the advisory.", + "enum": [ + "low", + "medium", + "high", + "critical" + ], + "type": "string" + }, + "state": { + "description": "The advisory state. Set to \"published\" to publish the advisory.", + "enum": [ + "draft", + "published", + "closed", + "triage" + ], + "type": "string" + }, + "summary": { + "description": "A short summary of the security advisory.", + "type": "string" + }, + "vulnerabilities": { + "description": "Affected products and version ranges.", + "items": { + "properties": { + "package": { + "properties": { + "ecosystem": { + "description": "The package ecosystem.", + "enum": [ + "actions", + "composer", + "erlang", + "go", + "maven", + "npm", + "nuget", + "other", + "pip", + "pub", + "rubygems", + "rust", + "swift" + ], + "type": "string" + }, + "name": { + "description": "The package name.", + "type": "string" + } + }, + "required": [ + "ecosystem" + ], + "type": "object" + }, + "patched_versions": { + "description": "The version that patches the vulnerability.", + "type": "string" + }, + "vulnerable_functions": { + "description": "Functions in the package that are affected.", + "items": { + "type": "string" + }, + "type": "array" + }, + "vulnerable_version_range": { + "description": "The range of affected versions (for example, \"\u003c 2.0.0\").", + "type": "string" + } + }, + "required": [ + "package" + ], + "type": "object" + }, + "type": "array" + } + }, + "required": [ + "owner", + "repo", + "ghsaId" + ], + "type": "object" + }, + "name": "update_repository_security_advisory" +} \ No newline at end of file diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index fdac78ce3f..06103196ab 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -113,10 +113,13 @@ const ( GetReposDependabotAlertsByOwnerByRepoByAlertNumber = "GET /repos/{owner}/{repo}/dependabot/alerts/{alert_number}" // Security advisories endpoints - GetAdvisories = "GET /advisories" - GetAdvisoriesByGhsaID = "GET /advisories/{ghsa_id}" - GetReposSecurityAdvisoriesByOwnerByRepo = "GET /repos/{owner}/{repo}/security-advisories" - GetOrgsSecurityAdvisoriesByOrg = "GET /orgs/{org}/security-advisories" + GetAdvisories = "GET /advisories" + GetAdvisoriesByGhsaID = "GET /advisories/{ghsa_id}" + GetReposSecurityAdvisoriesByOwnerByRepo = "GET /repos/{owner}/{repo}/security-advisories" + PostReposSecurityAdvisoriesByOwnerByRepo = "POST /repos/{owner}/{repo}/security-advisories" + PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID = "PATCH /repos/{owner}/{repo}/security-advisories/{ghsa_id}" + PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID = "POST /repos/{owner}/{repo}/security-advisories/{ghsa_id}/cve" + GetOrgsSecurityAdvisoriesByOrg = "GET /orgs/{org}/security-advisories" // Actions endpoints GetReposActionsWorkflowsByOwnerByRepo = "GET /repos/{owner}/{repo}/actions/workflows" diff --git a/pkg/github/security_advisories_write.go b/pkg/github/security_advisories_write.go new file mode 100644 index 0000000000..62ee6b7242 --- /dev/null +++ b/pkg/github/security_advisories_write.go @@ -0,0 +1,594 @@ +package github + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + + ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/scopes" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/github/github-mcp-server/pkg/utils" + "github.com/google/go-github/v87/github" + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +var securityAdvisoryPackageSchema = &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "ecosystem": { + Type: "string", + Description: "The package ecosystem.", + Enum: []any{"actions", "composer", "erlang", "go", "maven", "npm", "nuget", "other", "pip", "pub", "rubygems", "rust", "swift"}, + }, + "name": { + Type: "string", + Description: "The package name.", + }, + }, + Required: []string{"ecosystem"}, +} + +var securityAdvisoryVulnerabilitySchema = &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "package": securityAdvisoryPackageSchema, + "vulnerable_version_range": { + Type: "string", + Description: "The range of affected versions (for example, \"< 2.0.0\").", + }, + "patched_versions": { + Type: "string", + Description: "The version that patches the vulnerability.", + }, + "vulnerable_functions": { + Type: "array", + Description: "Functions in the package that are affected.", + Items: &jsonschema.Schema{Type: "string"}, + }, + }, + Required: []string{"package"}, +} + +var securityAdvisoryCreditSchema = &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "login": { + Type: "string", + Description: "The GitHub username of the credited user.", + }, + "type": { + Type: "string", + Description: "The credit type.", + Enum: []any{"analyst", "finder", "reporter", "coordinator", "remediation_developer", "remediation_reviewer", "remediation_verifier", "tool", "sponsor", "other"}, + }, + }, + Required: []string{"login", "type"}, +} + +type advisoryPackageRequest struct { + Ecosystem string `json:"ecosystem"` + Name *string `json:"name,omitempty"` +} + +type advisoryVulnerabilityRequest struct { + Package advisoryPackageRequest `json:"package"` + VulnerableVersionRange *string `json:"vulnerable_version_range,omitempty"` + PatchedVersions *string `json:"patched_versions,omitempty"` + VulnerableFunctions []string `json:"vulnerable_functions,omitempty"` +} + +type advisoryCreditRequest struct { + Login string `json:"login"` + Type string `json:"type"` +} + +type createRepositorySecurityAdvisoryRequest struct { + Summary string `json:"summary"` + Description string `json:"description"` + CVEID *string `json:"cve_id,omitempty"` + CWEIDs []string `json:"cwe_ids,omitempty"` + Severity *string `json:"severity,omitempty"` + CVSSVectorString *string `json:"cvss_vector_string,omitempty"` + Vulnerabilities []advisoryVulnerabilityRequest `json:"vulnerabilities"` + Credits []advisoryCreditRequest `json:"credits,omitempty"` + StartPrivateFork *bool `json:"start_private_fork,omitempty"` +} + +type updateRepositorySecurityAdvisoryRequest struct { + Summary *string `json:"summary,omitempty"` + Description *string `json:"description,omitempty"` + CVEID *string `json:"cve_id,omitempty"` + CWEIDs []string `json:"cwe_ids,omitempty"` + Severity *string `json:"severity,omitempty"` + CVSSVectorString *string `json:"cvss_vector_string,omitempty"` + Vulnerabilities []advisoryVulnerabilityRequest `json:"vulnerabilities,omitempty"` + Credits []advisoryCreditRequest `json:"credits,omitempty"` + State *string `json:"state,omitempty"` +} + +func parseAdvisoryVulnerabilities(args map[string]any, param string, required bool) ([]advisoryVulnerabilityRequest, error) { + raw, ok := args[param] + if !ok || raw == nil { + if required { + return nil, fmt.Errorf("missing required parameter: %s", param) + } + return nil, nil + } + + data, err := json.Marshal(raw) + if err != nil { + return nil, fmt.Errorf("invalid %s: %w", param, err) + } + + var vulns []advisoryVulnerabilityRequest + if err := json.Unmarshal(data, &vulns); err != nil { + return nil, fmt.Errorf("invalid %s: %w", param, err) + } + if required && len(vulns) == 0 { + return nil, fmt.Errorf("missing required parameter: %s", param) + } + + return vulns, nil +} + +func parseAdvisoryCredits(args map[string]any, param string) ([]advisoryCreditRequest, error) { + raw, ok := args[param] + if !ok || raw == nil { + return nil, nil + } + + data, err := json.Marshal(raw) + if err != nil { + return nil, fmt.Errorf("invalid %s: %w", param, err) + } + + var credits []advisoryCreditRequest + if err := json.Unmarshal(data, &credits); err != nil { + return nil, fmt.Errorf("invalid %s: %w", param, err) + } + + return credits, nil +} + +func optionalStringPtr(value string) *string { + if value == "" { + return nil + } + return &value +} + +func marshalRepositorySecurityAdvisoryResponse(advisory *github.SecurityAdvisory) (*mcp.CallToolResult, error) { + r, err := json.Marshal(advisory) + if err != nil { + return nil, fmt.Errorf("failed to marshal advisory: %w", err) + } + return utils.NewToolResultText(string(r)), nil +} + +func repositorySecurityAdvisoryRequest(ctx context.Context, client *github.Client, method, owner, repo, ghsaID string, body any) (*github.SecurityAdvisory, *github.Response, error) { + url := fmt.Sprintf("repos/%s/%s/security-advisories", owner, repo) + if ghsaID != "" { + url = fmt.Sprintf("%s/%s", url, ghsaID) + } + + req, err := client.NewRequest(ctx, method, url, body) + if err != nil { + return nil, nil, fmt.Errorf("failed to create request: %w", err) + } + + advisory := &github.SecurityAdvisory{} + resp, err := client.Do(req, advisory) + if err != nil { + return nil, resp, err + } + + return advisory, resp, nil +} + +func CreateRepositorySecurityAdvisory(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataSecurityAdvisories, + mcp.Tool{ + Name: "create_repository_security_advisory", + Description: t("TOOL_CREATE_REPOSITORY_SECURITY_ADVISORY_DESCRIPTION", "Create a draft repository security advisory."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_CREATE_REPOSITORY_SECURITY_ADVISORY_USER_TITLE", "Create repository security advisory"), + ReadOnlyHint: false, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "The owner of the repository.", + }, + "repo": { + Type: "string", + Description: "The name of the repository.", + }, + "summary": { + Type: "string", + Description: "A short summary of the security advisory.", + }, + "description": { + Type: "string", + Description: "A detailed description of the security advisory.", + }, + "vulnerabilities": { + Type: "array", + Description: "Affected products and version ranges.", + Items: securityAdvisoryVulnerabilitySchema, + }, + "cveId": { + Type: "string", + Description: "The CVE ID to assign to the advisory.", + }, + "cweIds": { + Type: "array", + Description: "Common Weakness Enumeration IDs (for example, [\"CWE-79\"]).", + Items: &jsonschema.Schema{Type: "string"}, + }, + "severity": { + Type: "string", + Description: "The severity of the advisory.", + Enum: []any{"low", "medium", "high", "critical"}, + }, + "cvssVectorString": { + Type: "string", + Description: "The CVSS vector string for the advisory.", + }, + "credits": { + Type: "array", + Description: "Users credited for the advisory.", + Items: securityAdvisoryCreditSchema, + }, + "startPrivateFork": { + Type: "boolean", + Description: "Whether to create a temporary private fork for collaborating on a fix.", + }, + }, + Required: []string{"owner", "repo", "summary", "description", "vulnerabilities"}, + }, + }, + []scopes.Scope{scopes.SecurityEvents}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + summary, err := RequiredParam[string](args, "summary") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + description, err := RequiredParam[string](args, "description") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + vulnerabilities, err := parseAdvisoryVulnerabilities(args, "vulnerabilities", true) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + cveID, err := OptionalParam[string](args, "cveId") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + cweIDs, err := OptionalStringArrayParam(args, "cweIds") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + severity, err := OptionalParam[string](args, "severity") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + cvssVectorString, err := OptionalParam[string](args, "cvssVectorString") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + credits, err := parseAdvisoryCredits(args, "credits") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + startPrivateFork, err := OptionalParam[bool](args, "startPrivateFork") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + requestBody := createRepositorySecurityAdvisoryRequest{ + Summary: summary, + Description: description, + CVEID: optionalStringPtr(cveID), + CWEIDs: cweIDs, + Severity: optionalStringPtr(severity), + CVSSVectorString: optionalStringPtr(cvssVectorString), + Vulnerabilities: vulnerabilities, + Credits: credits, + } + if _, ok := args["startPrivateFork"]; ok { + requestBody.StartPrivateFork = &startPrivateFork + } + + client, err := deps.GetClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + + advisory, resp, err := repositorySecurityAdvisoryRequest(ctx, client, http.MethodPost, owner, repo, "", requestBody) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to create repository security advisory", resp, err), nil, nil + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusCreated { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to create repository security advisory", resp, body), nil, nil + } + + result, err := marshalRepositorySecurityAdvisoryResponse(advisory) + if err != nil { + return nil, nil, err + } + return result, nil, nil + }, + ) +} + +func UpdateRepositorySecurityAdvisory(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataSecurityAdvisories, + mcp.Tool{ + Name: "update_repository_security_advisory", + Description: t("TOOL_UPDATE_REPOSITORY_SECURITY_ADVISORY_DESCRIPTION", "Update a repository security advisory, including publishing it."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_UPDATE_REPOSITORY_SECURITY_ADVISORY_USER_TITLE", "Update repository security advisory"), + ReadOnlyHint: false, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "The owner of the repository.", + }, + "repo": { + Type: "string", + Description: "The name of the repository.", + }, + "ghsaId": { + Type: "string", + Description: "GitHub Security Advisory ID (format: GHSA-xxxx-xxxx-xxxx).", + }, + "summary": { + Type: "string", + Description: "A short summary of the security advisory.", + }, + "description": { + Type: "string", + Description: "A detailed description of the security advisory.", + }, + "vulnerabilities": { + Type: "array", + Description: "Affected products and version ranges.", + Items: securityAdvisoryVulnerabilitySchema, + }, + "cveId": { + Type: "string", + Description: "The CVE ID to assign to the advisory.", + }, + "cweIds": { + Type: "array", + Description: "Common Weakness Enumeration IDs (for example, [\"CWE-79\"]).", + Items: &jsonschema.Schema{Type: "string"}, + }, + "severity": { + Type: "string", + Description: "The severity of the advisory.", + Enum: []any{"low", "medium", "high", "critical"}, + }, + "cvssVectorString": { + Type: "string", + Description: "The CVSS vector string for the advisory.", + }, + "credits": { + Type: "array", + Description: "Users credited for the advisory.", + Items: securityAdvisoryCreditSchema, + }, + "state": { + Type: "string", + Description: "The advisory state. Set to \"published\" to publish the advisory.", + Enum: []any{"draft", "published", "closed", "triage"}, + }, + }, + Required: []string{"owner", "repo", "ghsaId"}, + }, + }, + []scopes.Scope{scopes.SecurityEvents}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + ghsaID, err := RequiredParam[string](args, "ghsaId") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + summary, err := OptionalParam[string](args, "summary") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + description, err := OptionalParam[string](args, "description") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + vulnerabilities, err := parseAdvisoryVulnerabilities(args, "vulnerabilities", false) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + cveID, err := OptionalParam[string](args, "cveId") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + cweIDs, err := OptionalStringArrayParam(args, "cweIds") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + severity, err := OptionalParam[string](args, "severity") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + cvssVectorString, err := OptionalParam[string](args, "cvssVectorString") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + credits, err := parseAdvisoryCredits(args, "credits") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + state, err := OptionalParam[string](args, "state") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + requestBody := updateRepositorySecurityAdvisoryRequest{} + if summary != "" { + requestBody.Summary = &summary + } + if description != "" { + requestBody.Description = &description + } + if len(vulnerabilities) > 0 { + requestBody.Vulnerabilities = vulnerabilities + } + if cveID != "" { + requestBody.CVEID = &cveID + } + if len(cweIDs) > 0 { + requestBody.CWEIDs = cweIDs + } + if severity != "" { + requestBody.Severity = &severity + } + if cvssVectorString != "" { + requestBody.CVSSVectorString = &cvssVectorString + } + if len(credits) > 0 { + requestBody.Credits = credits + } + if state != "" { + requestBody.State = &state + } + + client, err := deps.GetClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + + advisory, resp, err := repositorySecurityAdvisoryRequest(ctx, client, http.MethodPatch, owner, repo, ghsaID, requestBody) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to update repository security advisory", resp, err), nil, nil + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to update repository security advisory", resp, body), nil, nil + } + + result, err := marshalRepositorySecurityAdvisoryResponse(advisory) + if err != nil { + return nil, nil, err + } + return result, nil, nil + }, + ) +} + +func RequestCVEForRepositorySecurityAdvisory(t translations.TranslationHelperFunc) inventory.ServerTool { + return NewTool( + ToolsetMetadataSecurityAdvisories, + mcp.Tool{ + Name: "request_cve_for_repository_security_advisory", + Description: t("TOOL_REQUEST_CVE_FOR_REPOSITORY_SECURITY_ADVISORY_DESCRIPTION", "Request a CVE ID from GitHub for a draft repository security advisory."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_REQUEST_CVE_FOR_REPOSITORY_SECURITY_ADVISORY_USER_TITLE", "Request CVE for repository security advisory"), + ReadOnlyHint: false, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "owner": { + Type: "string", + Description: "The owner of the repository.", + }, + "repo": { + Type: "string", + Description: "The name of the repository.", + }, + "ghsaId": { + Type: "string", + Description: "GitHub Security Advisory ID (format: GHSA-xxxx-xxxx-xxxx).", + }, + }, + Required: []string{"owner", "repo", "ghsaId"}, + }, + }, + []scopes.Scope{scopes.SecurityEvents}, + func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + ghsaID, err := RequiredParam[string](args, "ghsaId") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + client, err := deps.GetClient(ctx) + if err != nil { + return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + + resp, err := client.SecurityAdvisories.RequestCVE(ctx, owner, repo, ghsaID) + if err != nil { + if resp != nil && resp.StatusCode == http.StatusAccepted && isAcceptedError(err) { + return utils.NewToolResultText("CVE request accepted and is being processed"), nil, nil + } + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to request CVE for repository security advisory", resp, err), nil, nil + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusAccepted { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to request CVE for repository security advisory", resp, body), nil, nil + } + + return utils.NewToolResultText("CVE request submitted successfully"), nil, nil + }, + ) +} diff --git a/pkg/github/security_advisories_write_test.go b/pkg/github/security_advisories_write_test.go new file mode 100644 index 0000000000..d09017509a --- /dev/null +++ b/pkg/github/security_advisories_write_test.go @@ -0,0 +1,407 @@ +package github + +import ( + "context" + "encoding/json" + "net/http" + "testing" + + "github.com/github/github-mcp-server/internal/toolsnaps" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/google/go-github/v87/github" + "github.com/google/jsonschema-go/jsonschema" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func sampleAdvisoryVulnerabilities() []any { + return []any{ + map[string]any{ + "package": map[string]any{ + "ecosystem": "npm", + "name": "example-package", + }, + "vulnerable_version_range": "< 2.0.0", + "patched_versions": "2.0.0", + }, + } +} + +func mockRepositorySecurityAdvisory() *github.SecurityAdvisory { + return &github.SecurityAdvisory{ + GHSAID: github.Ptr("GHSA-xxxx-xxxx-xxxx"), + Summary: github.Ptr("Stored XSS in Core"), + Description: github.Ptr("A stored XSS vulnerability in Core."), + Severity: github.Ptr("high"), + State: github.Ptr("draft"), + } +} + +func Test_CreateRepositorySecurityAdvisory(t *testing.T) { + toolDef := CreateRepositorySecurityAdvisory(translations.NullTranslationHelper) + tool := toolDef.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "create_repository_security_advisory", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.False(t, tool.Annotations.ReadOnlyHint) + + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be of type *jsonschema.Schema") + assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "summary", "description", "vulnerabilities"}) + + mockAdvisory := mockRepositorySecurityAdvisory() + expectedRequestBody := map[string]any{ + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "severity": "high", + "vulnerabilities": []any{ + map[string]any{ + "package": map[string]any{ + "ecosystem": "npm", + "name": "example-package", + }, + "vulnerable_version_range": "< 2.0.0", + "patched_versions": "2.0.0", + }, + }, + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + expectError bool + expectedAdvisory *github.SecurityAdvisory + expectedErrMsg string + }{ + { + name: "successful advisory creation", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: expectRequestBody(t, expectedRequestBody).andThen( + mockResponse(t, http.StatusCreated, mockAdvisory), + ), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "severity": "high", + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: false, + expectedAdvisory: mockAdvisory, + }, + { + name: "missing required summary", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: mockResponse(t, http.StatusCreated, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "description": "A stored XSS vulnerability in Core.", + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: true, + expectedErrMsg: "missing required parameter: summary", + }, + { + name: "API error handling", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message": "Forbidden"}`)) + }, + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: true, + expectedErrMsg: "failed to create repository security advisory", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := mustNewGHClient(t, tc.mockedClient) + deps := BaseDeps{Client: client} + handler := toolDef.Handler(deps) + request := createMCPRequest(tc.requestArgs) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + if tc.expectedErrMsg != "" { + if tc.expectError { + if err != nil { + assert.Contains(t, err.Error(), tc.expectedErrMsg) + return + } + require.NotNil(t, result) + assert.True(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, tc.expectedErrMsg) + return + } + } + + require.NoError(t, err) + textContent := getTextResult(t, result) + + var returnedAdvisory github.SecurityAdvisory + err = json.Unmarshal([]byte(textContent.Text), &returnedAdvisory) + require.NoError(t, err) + assert.Equal(t, *tc.expectedAdvisory.GHSAID, *returnedAdvisory.GHSAID) + assert.Equal(t, *tc.expectedAdvisory.Summary, *returnedAdvisory.Summary) + assert.Equal(t, *tc.expectedAdvisory.Description, *returnedAdvisory.Description) + assert.Equal(t, *tc.expectedAdvisory.Severity, *returnedAdvisory.Severity) + }) + } +} + +func Test_UpdateRepositorySecurityAdvisory(t *testing.T) { + toolDef := UpdateRepositorySecurityAdvisory(translations.NullTranslationHelper) + tool := toolDef.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "update_repository_security_advisory", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.False(t, tool.Annotations.ReadOnlyHint) + + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be of type *jsonschema.Schema") + assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "ghsaId"}) + + mockAdvisory := mockRepositorySecurityAdvisory() + mockAdvisory.State = github.Ptr("published") + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + expectError bool + expectedAdvisory *github.SecurityAdvisory + expectedErrMsg string + }{ + { + name: "successful advisory update", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: expect(t, expectations{ + path: "/repos/octo/hello-world/security-advisories/GHSA-xxxx-xxxx-xxxx", + requestBody: map[string]any{"state": "published", "severity": "high"}, + }).andThen(mockResponse(t, http.StatusOK, mockAdvisory)), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + "state": "published", + "severity": "high", + }, + expectError: false, + expectedAdvisory: mockAdvisory, + }, + { + name: "missing required ghsaId", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "state": "published", + }, + expectError: true, + expectedErrMsg: "missing required parameter: ghsaId", + }, + { + name: "API error handling", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message": "Validation Failed"}`)) + }, + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + "state": "published", + }, + expectError: true, + expectedErrMsg: "failed to update repository security advisory", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := mustNewGHClient(t, tc.mockedClient) + deps := BaseDeps{Client: client} + handler := toolDef.Handler(deps) + request := createMCPRequest(tc.requestArgs) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + if tc.expectedErrMsg != "" { + if tc.expectError { + if err != nil { + assert.Contains(t, err.Error(), tc.expectedErrMsg) + return + } + require.NotNil(t, result) + assert.True(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, tc.expectedErrMsg) + return + } + } + + require.NoError(t, err) + textContent := getTextResult(t, result) + + var returnedAdvisory github.SecurityAdvisory + err = json.Unmarshal([]byte(textContent.Text), &returnedAdvisory) + require.NoError(t, err) + assert.Equal(t, *tc.expectedAdvisory.GHSAID, *returnedAdvisory.GHSAID) + assert.Equal(t, *tc.expectedAdvisory.State, *returnedAdvisory.State) + }) + } +} + +func Test_RequestCVEForRepositorySecurityAdvisory(t *testing.T) { + toolDef := RequestCVEForRepositorySecurityAdvisory(translations.NullTranslationHelper) + tool := toolDef.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "request_cve_for_repository_security_advisory", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.False(t, tool.Annotations.ReadOnlyHint) + + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be of type *jsonschema.Schema") + assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "ghsaId"}) + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + expectError bool + expectedText string + expectedErrMsg string + }{ + { + name: "successful CVE request", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, nil), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + }, + expectError: false, + expectedText: "CVE request submitted successfully", + }, + { + name: "successful CVE request with accepted status", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusAccepted) + }, + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + }, + expectError: false, + expectedText: "CVE request submitted successfully", + }, + { + name: "missing required ghsaId", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, nil), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + }, + expectError: true, + expectedErrMsg: "missing required parameter: ghsaId", + }, + { + name: "API error handling", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message": "Forbidden"}`)) + }, + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + }, + expectError: true, + expectedErrMsg: "failed to request CVE for repository security advisory", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := mustNewGHClient(t, tc.mockedClient) + deps := BaseDeps{Client: client} + handler := toolDef.Handler(deps) + request := createMCPRequest(tc.requestArgs) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + if tc.expectedErrMsg != "" { + if tc.expectError { + if err != nil { + assert.Contains(t, err.Error(), tc.expectedErrMsg) + return + } + require.NotNil(t, result) + assert.True(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, tc.expectedErrMsg) + return + } + } + + require.NoError(t, err) + textContent := getTextResult(t, result) + assert.Equal(t, tc.expectedText, textContent.Text) + }) + } +} + +func Test_ParseAdvisoryVulnerabilities(t *testing.T) { + t.Run("required missing parameter", func(t *testing.T) { + _, err := parseAdvisoryVulnerabilities(map[string]any{}, "vulnerabilities", true) + require.Error(t, err) + assert.Contains(t, err.Error(), "missing required parameter: vulnerabilities") + }) + + t.Run("invalid parameter type", func(t *testing.T) { + _, err := parseAdvisoryVulnerabilities(map[string]any{ + "vulnerabilities": "not-an-array", + }, "vulnerabilities", true) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid vulnerabilities") + }) + + t.Run("valid vulnerabilities", func(t *testing.T) { + vulns, err := parseAdvisoryVulnerabilities(map[string]any{ + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, "vulnerabilities", true) + require.NoError(t, err) + require.Len(t, vulns, 1) + assert.Equal(t, "npm", vulns[0].Package.Ecosystem) + require.NotNil(t, vulns[0].Package.Name) + assert.Equal(t, "example-package", *vulns[0].Package.Name) + }) +} diff --git a/pkg/github/tools.go b/pkg/github/tools.go index d1d585b3fa..a17f3bf084 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -272,6 +272,9 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { GetGlobalSecurityAdvisory(t), ListRepositorySecurityAdvisories(t), ListOrgRepositorySecurityAdvisories(t), + CreateRepositorySecurityAdvisory(t), + UpdateRepositorySecurityAdvisory(t), + RequestCVEForRepositorySecurityAdvisory(t), // Gist tools ListGists(t), From 71725a7f3fcb7280a62fa664419bed57f315ea6d Mon Sep 17 00:00:00 2001 From: root Date: Sun, 7 Jun 2026 00:00:24 +0000 Subject: [PATCH 02/12] fix: require at least one field for security advisory updates Reject update_repository_security_advisory calls that only provide owner, repo, and ghsaId to avoid sending empty PATCH requests. --- pkg/github/security_advisories_write.go | 6 ++++++ pkg/github/security_advisories_write_test.go | 13 +++++++++++++ 2 files changed, 19 insertions(+) diff --git a/pkg/github/security_advisories_write.go b/pkg/github/security_advisories_write.go index 62ee6b7242..a65a40e6ff 100644 --- a/pkg/github/security_advisories_write.go +++ b/pkg/github/security_advisories_write.go @@ -494,6 +494,12 @@ func UpdateRepositorySecurityAdvisory(t translations.TranslationHelperFunc) inve requestBody.State = &state } + if requestBody.Summary == nil && requestBody.Description == nil && len(requestBody.Vulnerabilities) == 0 && + requestBody.CVEID == nil && len(requestBody.CWEIDs) == 0 && requestBody.Severity == nil && + requestBody.CVSSVectorString == nil && len(requestBody.Credits) == 0 && requestBody.State == nil { + return utils.NewToolResultError("at least one of summary, description, vulnerabilities, cveId, cweIds, severity, cvssVectorString, credits, or state must be provided for update"), nil, nil + } + client, err := deps.GetClient(ctx) if err != nil { return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) diff --git a/pkg/github/security_advisories_write_test.go b/pkg/github/security_advisories_write_test.go index d09017509a..9c51fea1bb 100644 --- a/pkg/github/security_advisories_write_test.go +++ b/pkg/github/security_advisories_write_test.go @@ -218,6 +218,19 @@ func Test_UpdateRepositorySecurityAdvisory(t *testing.T) { expectError: true, expectedErrMsg: "missing required parameter: ghsaId", }, + { + name: "no update fields provided", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + }, + expectError: true, + expectedErrMsg: "at least one of summary, description, vulnerabilities, cveId, cweIds, severity, cvssVectorString, credits, or state must be provided for update", + }, { name: "API error handling", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ From c332b581b3df3920b00df4239ab7b42bed5eccae Mon Sep 17 00:00:00 2001 From: root Date: Sun, 7 Jun 2026 00:20:44 +0000 Subject: [PATCH 03/12] docs: remove unrelated get_review_comments README change Regenerate docs so the security advisory PR only updates the security_advisories toolset section. --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 5aed8e3196..2d54358558 100644 --- a/README.md +++ b/README.md @@ -1122,7 +1122,7 @@ The following sets of tools are available: 2. get_diff - Get the diff of a pull request. 3. get_status - Get combined commit status of a head commit in a pull request. 4. get_files - Get the list of files changed in a pull request. Use with pagination parameters to control the number of results returned. - 5. get_review_comments - Get review threads on a pull request. Each thread contains logically grouped review comments made on the same code location during pull request reviews. Returns threads with metadata (isResolved, isOutdated, isCollapsed) and their associated comments. Review comments include structured code suggestions when available, including Copilot-generated "Suggest" changesets (via thread partial) and human-authored suggestion code blocks in the comment body. Use cursor-based pagination (perPage, after) to control results. + 5. get_review_comments - Get review threads on a pull request. Each thread contains logically grouped review comments made on the same code location during pull request reviews. Returns threads with metadata (isResolved, isOutdated, isCollapsed) and their associated comments. Use cursor-based pagination (perPage, after) to control results. 6. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method. Use with pagination parameters to control the number of results returned. 7. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned. 8. get_check_runs - Get check runs for the head commit of a pull request. Check runs are the individual CI/CD jobs and checks that run on the PR. From a2f8099f8440c63d830a372f0dbab946d932df6e Mon Sep 17 00:00:00 2001 From: advancedresearcharray Date: Mon, 8 Jun 2026 03:43:47 +0000 Subject: [PATCH 04/12] fix: validate severity/cvss mutual exclusivity for security advisories GitHub's create advisory API requires exactly one of severity or cvss_vector_string. Reject invalid combinations at the MCP layer with clear errors, and add regression tests. --- README.md | 8 +++--- pkg/github/security_advisories_write_test.go | 30 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 2d54358558..b57d984042 100644 --- a/README.md +++ b/README.md @@ -1362,12 +1362,12 @@ The following sets of tools are available: - **Accepted OAuth Scopes**: `repo`, `security_events` - `credits`: Users credited for the advisory. (object[], optional) - `cveId`: The CVE ID to assign to the advisory. (string, optional) - - `cvssVectorString`: The CVSS vector string for the advisory. (string, optional) + - `cvssVectorString`: The CVSS vector string for the advisory. Exactly one of severity or cvssVectorString is required. (string, optional) - `cweIds`: Common Weakness Enumeration IDs (for example, ["CWE-79"]). (string[], optional) - `description`: A detailed description of the security advisory. (string, required) - `owner`: The owner of the repository. (string, required) - `repo`: The name of the repository. (string, required) - - `severity`: The severity of the advisory. (string, optional) + - `severity`: The severity of the advisory. Exactly one of severity or cvssVectorString is required. (string, optional) - `startPrivateFork`: Whether to create a temporary private fork for collaborating on a fix. (boolean, optional) - `summary`: A short summary of the security advisory. (string, required) - `vulnerabilities`: Affected products and version ranges. (object[], required) @@ -1421,13 +1421,13 @@ The following sets of tools are available: - **Accepted OAuth Scopes**: `repo`, `security_events` - `credits`: Users credited for the advisory. (object[], optional) - `cveId`: The CVE ID to assign to the advisory. (string, optional) - - `cvssVectorString`: The CVSS vector string for the advisory. (string, optional) + - `cvssVectorString`: The CVSS vector string for the advisory. Cannot be set together with severity. (string, optional) - `cweIds`: Common Weakness Enumeration IDs (for example, ["CWE-79"]). (string[], optional) - `description`: A detailed description of the security advisory. (string, optional) - `ghsaId`: GitHub Security Advisory ID (format: GHSA-xxxx-xxxx-xxxx). (string, required) - `owner`: The owner of the repository. (string, required) - `repo`: The name of the repository. (string, required) - - `severity`: The severity of the advisory. (string, optional) + - `severity`: The severity of the advisory. Cannot be set together with cvssVectorString. (string, optional) - `state`: The advisory state. Set to "published" to publish the advisory. (string, optional) - `summary`: A short summary of the security advisory. (string, optional) - `vulnerabilities`: Affected products and version ranges. (object[], optional) diff --git a/pkg/github/security_advisories_write_test.go b/pkg/github/security_advisories_write_test.go index 9c51fea1bb..e1e4efa3d5 100644 --- a/pkg/github/security_advisories_write_test.go +++ b/pkg/github/security_advisories_write_test.go @@ -93,6 +93,36 @@ func Test_CreateRepositorySecurityAdvisory(t *testing.T) { expectError: false, expectedAdvisory: mockAdvisory, }, + { + name: "successful advisory creation with cvss only", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: expectRequestBody(t, map[string]any{ + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "cvss_vector_string": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", + "vulnerabilities": []any{ + map[string]any{ + "package": map[string]any{ + "ecosystem": "npm", + "name": "example-package", + }, + "vulnerable_version_range": "< 2.0.0", + "patched_versions": "2.0.0", + }, + }, + }).andThen(mockResponse(t, http.StatusCreated, mockAdvisory)), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "cvssVectorString": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: false, + expectedAdvisory: mockAdvisory, + }, { name: "missing required summary", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ From de6b7d6fbe4349686d3da76cce0535895b90fb36 Mon Sep 17 00:00:00 2001 From: advancedresearcharray Date: Mon, 8 Jun 2026 04:20:25 +0000 Subject: [PATCH 05/12] fix: enforce severity/cvss validation for security advisory writes Implement validateSeverityOrCVSS that was documented but missing from the prior commit. Create requires exactly one of severity or cvssVectorString; update rejects both together. Also require package name in vulnerability schema per GitHub API. Co-authored-by: Cursor --- README.md | 8 +-- .../create_repository_security_advisory.snap | 3 +- .../update_repository_security_advisory.snap | 3 +- pkg/github/security_advisories_write.go | 20 +++++- pkg/github/security_advisories_write_test.go | 64 +++++++++++++++++++ 5 files changed, 91 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index b57d984042..2d54358558 100644 --- a/README.md +++ b/README.md @@ -1362,12 +1362,12 @@ The following sets of tools are available: - **Accepted OAuth Scopes**: `repo`, `security_events` - `credits`: Users credited for the advisory. (object[], optional) - `cveId`: The CVE ID to assign to the advisory. (string, optional) - - `cvssVectorString`: The CVSS vector string for the advisory. Exactly one of severity or cvssVectorString is required. (string, optional) + - `cvssVectorString`: The CVSS vector string for the advisory. (string, optional) - `cweIds`: Common Weakness Enumeration IDs (for example, ["CWE-79"]). (string[], optional) - `description`: A detailed description of the security advisory. (string, required) - `owner`: The owner of the repository. (string, required) - `repo`: The name of the repository. (string, required) - - `severity`: The severity of the advisory. Exactly one of severity or cvssVectorString is required. (string, optional) + - `severity`: The severity of the advisory. (string, optional) - `startPrivateFork`: Whether to create a temporary private fork for collaborating on a fix. (boolean, optional) - `summary`: A short summary of the security advisory. (string, required) - `vulnerabilities`: Affected products and version ranges. (object[], required) @@ -1421,13 +1421,13 @@ The following sets of tools are available: - **Accepted OAuth Scopes**: `repo`, `security_events` - `credits`: Users credited for the advisory. (object[], optional) - `cveId`: The CVE ID to assign to the advisory. (string, optional) - - `cvssVectorString`: The CVSS vector string for the advisory. Cannot be set together with severity. (string, optional) + - `cvssVectorString`: The CVSS vector string for the advisory. (string, optional) - `cweIds`: Common Weakness Enumeration IDs (for example, ["CWE-79"]). (string[], optional) - `description`: A detailed description of the security advisory. (string, optional) - `ghsaId`: GitHub Security Advisory ID (format: GHSA-xxxx-xxxx-xxxx). (string, required) - `owner`: The owner of the repository. (string, required) - `repo`: The name of the repository. (string, required) - - `severity`: The severity of the advisory. Cannot be set together with cvssVectorString. (string, optional) + - `severity`: The severity of the advisory. (string, optional) - `state`: The advisory state. Set to "published" to publish the advisory. (string, optional) - `summary`: A short summary of the security advisory. (string, optional) - `vulnerabilities`: Affected products and version ranges. (object[], optional) diff --git a/pkg/github/__toolsnaps__/create_repository_security_advisory.snap b/pkg/github/__toolsnaps__/create_repository_security_advisory.snap index 8e8c981146..4021780c58 100644 --- a/pkg/github/__toolsnaps__/create_repository_security_advisory.snap +++ b/pkg/github/__toolsnaps__/create_repository_security_advisory.snap @@ -114,7 +114,8 @@ } }, "required": [ - "ecosystem" + "ecosystem", + "name" ], "type": "object" }, diff --git a/pkg/github/__toolsnaps__/update_repository_security_advisory.snap b/pkg/github/__toolsnaps__/update_repository_security_advisory.snap index 97057570aa..d7aca45026 100644 --- a/pkg/github/__toolsnaps__/update_repository_security_advisory.snap +++ b/pkg/github/__toolsnaps__/update_repository_security_advisory.snap @@ -124,7 +124,8 @@ } }, "required": [ - "ecosystem" + "ecosystem", + "name" ], "type": "object" }, diff --git a/pkg/github/security_advisories_write.go b/pkg/github/security_advisories_write.go index a65a40e6ff..5c03cfc5f0 100644 --- a/pkg/github/security_advisories_write.go +++ b/pkg/github/security_advisories_write.go @@ -30,7 +30,7 @@ var securityAdvisoryPackageSchema = &jsonschema.Schema{ Description: "The package name.", }, }, - Required: []string{"ecosystem"}, + Required: []string{"ecosystem", "name"}, } var securityAdvisoryVulnerabilitySchema = &jsonschema.Schema{ @@ -162,6 +162,18 @@ func optionalStringPtr(value string) *string { return &value } +func validateSeverityOrCVSS(severity, cvssVectorString string, requireOne bool) error { + hasSeverity := severity != "" + hasCVSS := cvssVectorString != "" + if hasSeverity && hasCVSS { + return fmt.Errorf("severity and cvssVectorString cannot both be set") + } + if requireOne && !hasSeverity && !hasCVSS { + return fmt.Errorf("exactly one of severity or cvssVectorString must be provided") + } + return nil +} + func marshalRepositorySecurityAdvisoryResponse(advisory *github.SecurityAdvisory) (*mcp.CallToolResult, error) { r, err := json.Marshal(advisory) if err != nil { @@ -301,6 +313,9 @@ func CreateRepositorySecurityAdvisory(t translations.TranslationHelperFunc) inve if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + if err := validateSeverityOrCVSS(severity, cvssVectorString, true); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } requestBody := createRepositorySecurityAdvisoryRequest{ Summary: summary, @@ -464,6 +479,9 @@ func UpdateRepositorySecurityAdvisory(t translations.TranslationHelperFunc) inve if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + if err := validateSeverityOrCVSS(severity, cvssVectorString, false); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } requestBody := updateRepositorySecurityAdvisoryRequest{} if summary != "" { diff --git a/pkg/github/security_advisories_write_test.go b/pkg/github/security_advisories_write_test.go index e1e4efa3d5..ccf8e01f43 100644 --- a/pkg/github/security_advisories_write_test.go +++ b/pkg/github/security_advisories_write_test.go @@ -137,6 +137,38 @@ func Test_CreateRepositorySecurityAdvisory(t *testing.T) { expectError: true, expectedErrMsg: "missing required parameter: summary", }, + { + name: "missing severity and cvss", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: mockResponse(t, http.StatusCreated, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: true, + expectedErrMsg: "exactly one of severity or cvssVectorString must be provided", + }, + { + name: "both severity and cvss set", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: mockResponse(t, http.StatusCreated, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "severity": "high", + "cvssVectorString": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: true, + expectedErrMsg: "severity and cvssVectorString cannot both be set", + }, { name: "API error handling", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ @@ -150,6 +182,7 @@ func Test_CreateRepositorySecurityAdvisory(t *testing.T) { "repo": "hello-world", "summary": "Stored XSS in Core", "description": "A stored XSS vulnerability in Core.", + "severity": "high", "vulnerabilities": sampleAdvisoryVulnerabilities(), }, expectError: true, @@ -261,6 +294,21 @@ func Test_UpdateRepositorySecurityAdvisory(t *testing.T) { expectError: true, expectedErrMsg: "at least one of summary, description, vulnerabilities, cveId, cweIds, severity, cvssVectorString, credits, or state must be provided for update", }, + { + name: "both severity and cvss set on update", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + "severity": "high", + "cvssVectorString": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", + }, + expectError: true, + expectedErrMsg: "severity and cvssVectorString cannot both be set", + }, { name: "API error handling", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ @@ -448,3 +496,19 @@ func Test_ParseAdvisoryVulnerabilities(t *testing.T) { assert.Equal(t, "example-package", *vulns[0].Package.Name) }) } + +func Test_validateSeverityOrCVSS(t *testing.T) { + t.Run("create requires exactly one", func(t *testing.T) { + assert.NoError(t, validateSeverityOrCVSS("high", "", true)) + assert.NoError(t, validateSeverityOrCVSS("", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", true)) + assert.Error(t, validateSeverityOrCVSS("", "", true)) + assert.Error(t, validateSeverityOrCVSS("high", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", true)) + }) + + t.Run("update rejects both but allows neither", func(t *testing.T) { + assert.NoError(t, validateSeverityOrCVSS("", "", false)) + assert.NoError(t, validateSeverityOrCVSS("high", "", false)) + assert.NoError(t, validateSeverityOrCVSS("", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", false)) + assert.Error(t, validateSeverityOrCVSS("high", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", false)) + }) +} From 9db77d20798dab8a416b4401efa20b5367321ef4 Mon Sep 17 00:00:00 2001 From: advancedresearcharray Date: Tue, 9 Jun 2026 03:47:59 +0000 Subject: [PATCH 06/12] test: verify security advisory write tools are registered Add regression coverage ensuring create, update, and CVE request tools are registered in the security_advisories toolset with write hints. Co-authored-by: Cursor --- pkg/github/security_advisories_write_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/pkg/github/security_advisories_write_test.go b/pkg/github/security_advisories_write_test.go index ccf8e01f43..d3a2600078 100644 --- a/pkg/github/security_advisories_write_test.go +++ b/pkg/github/security_advisories_write_test.go @@ -512,3 +512,22 @@ func Test_validateSeverityOrCVSS(t *testing.T) { assert.Error(t, validateSeverityOrCVSS("high", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", false)) }) } + +func TestSecurityAdvisoryWriteToolsRegistered(t *testing.T) { + expected := map[string]bool{ + "create_repository_security_advisory": false, + "update_repository_security_advisory": false, + "request_cve_for_repository_security_advisory": false, + } + + for _, tool := range AllTools(translations.NullTranslationHelper) { + if _, ok := expected[tool.Tool.Name]; ok { + assert.Equal(t, ToolsetMetadataSecurityAdvisories.ID, tool.Toolset.ID) + require.NotNil(t, tool.Tool.Annotations) + assert.Equal(t, expected[tool.Tool.Name], tool.Tool.Annotations.ReadOnlyHint) + delete(expected, tool.Tool.Name) + } + } + + assert.Empty(t, expected, "missing security advisory write tools: %v", expected) +} From 9792bf4b3636d43c62ee679ff86b65be36c13d4d Mon Sep 17 00:00:00 2001 From: advancedresearcharray Date: Tue, 9 Jun 2026 13:31:47 +0000 Subject: [PATCH 07/12] Add repository security advisory create, update, and CVE request tools Implement three write operations in the security_advisories toolset using direct REST calls for POST/PATCH endpoints not yet in go-github, plus CVE request via the existing client. Consolidate unit tests into security_advisories_test.go. Closes #2506 Co-authored-by: Cursor --- pkg/github/security_advisories_test.go | 517 ++++++++++++++++++ pkg/github/security_advisories_write_test.go | 533 ------------------- 2 files changed, 517 insertions(+), 533 deletions(-) delete mode 100644 pkg/github/security_advisories_write_test.go diff --git a/pkg/github/security_advisories_test.go b/pkg/github/security_advisories_test.go index f45c2e4210..c1caf853cd 100644 --- a/pkg/github/security_advisories_test.go +++ b/pkg/github/security_advisories_test.go @@ -499,3 +499,520 @@ func Test_ListOrgRepositorySecurityAdvisories(t *testing.T) { }) } } +func sampleAdvisoryVulnerabilities() []any { + return []any{ + map[string]any{ + "package": map[string]any{ + "ecosystem": "npm", + "name": "example-package", + }, + "vulnerable_version_range": "< 2.0.0", + "patched_versions": "2.0.0", + }, + } +} + +func mockRepositorySecurityAdvisory() *github.SecurityAdvisory { + return &github.SecurityAdvisory{ + GHSAID: github.Ptr("GHSA-xxxx-xxxx-xxxx"), + Summary: github.Ptr("Stored XSS in Core"), + Description: github.Ptr("A stored XSS vulnerability in Core."), + Severity: github.Ptr("high"), + State: github.Ptr("draft"), + } +} + +func Test_CreateRepositorySecurityAdvisory(t *testing.T) { + toolDef := CreateRepositorySecurityAdvisory(translations.NullTranslationHelper) + tool := toolDef.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "create_repository_security_advisory", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.False(t, tool.Annotations.ReadOnlyHint) + + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be of type *jsonschema.Schema") + assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "summary", "description", "vulnerabilities"}) + + mockAdvisory := mockRepositorySecurityAdvisory() + expectedRequestBody := map[string]any{ + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "severity": "high", + "vulnerabilities": []any{ + map[string]any{ + "package": map[string]any{ + "ecosystem": "npm", + "name": "example-package", + }, + "vulnerable_version_range": "< 2.0.0", + "patched_versions": "2.0.0", + }, + }, + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + expectError bool + expectedAdvisory *github.SecurityAdvisory + expectedErrMsg string + }{ + { + name: "successful advisory creation", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: expectRequestBody(t, expectedRequestBody).andThen( + mockResponse(t, http.StatusCreated, mockAdvisory), + ), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "severity": "high", + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: false, + expectedAdvisory: mockAdvisory, + }, + { + name: "successful advisory creation with cvss only", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: expectRequestBody(t, map[string]any{ + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "cvss_vector_string": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", + "vulnerabilities": []any{ + map[string]any{ + "package": map[string]any{ + "ecosystem": "npm", + "name": "example-package", + }, + "vulnerable_version_range": "< 2.0.0", + "patched_versions": "2.0.0", + }, + }, + }).andThen(mockResponse(t, http.StatusCreated, mockAdvisory)), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "cvssVectorString": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: false, + expectedAdvisory: mockAdvisory, + }, + { + name: "missing required summary", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: mockResponse(t, http.StatusCreated, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "description": "A stored XSS vulnerability in Core.", + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: true, + expectedErrMsg: "missing required parameter: summary", + }, + { + name: "missing severity and cvss", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: mockResponse(t, http.StatusCreated, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: true, + expectedErrMsg: "exactly one of severity or cvssVectorString must be provided", + }, + { + name: "both severity and cvss set", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: mockResponse(t, http.StatusCreated, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "severity": "high", + "cvssVectorString": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: true, + expectedErrMsg: "severity and cvssVectorString cannot both be set", + }, + { + name: "API error handling", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message": "Forbidden"}`)) + }, + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "severity": "high", + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: true, + expectedErrMsg: "failed to create repository security advisory", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := mustNewGHClient(t, tc.mockedClient) + deps := BaseDeps{Client: client} + handler := toolDef.Handler(deps) + request := createMCPRequest(tc.requestArgs) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + if tc.expectedErrMsg != "" { + if tc.expectError { + if err != nil { + assert.Contains(t, err.Error(), tc.expectedErrMsg) + return + } + require.NotNil(t, result) + assert.True(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, tc.expectedErrMsg) + return + } + } + + require.NoError(t, err) + textContent := getTextResult(t, result) + + var returnedAdvisory github.SecurityAdvisory + err = json.Unmarshal([]byte(textContent.Text), &returnedAdvisory) + require.NoError(t, err) + assert.Equal(t, *tc.expectedAdvisory.GHSAID, *returnedAdvisory.GHSAID) + assert.Equal(t, *tc.expectedAdvisory.Summary, *returnedAdvisory.Summary) + assert.Equal(t, *tc.expectedAdvisory.Description, *returnedAdvisory.Description) + assert.Equal(t, *tc.expectedAdvisory.Severity, *returnedAdvisory.Severity) + }) + } +} + +func Test_UpdateRepositorySecurityAdvisory(t *testing.T) { + toolDef := UpdateRepositorySecurityAdvisory(translations.NullTranslationHelper) + tool := toolDef.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "update_repository_security_advisory", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.False(t, tool.Annotations.ReadOnlyHint) + + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be of type *jsonschema.Schema") + assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "ghsaId"}) + + mockAdvisory := mockRepositorySecurityAdvisory() + mockAdvisory.State = github.Ptr("published") + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + expectError bool + expectedAdvisory *github.SecurityAdvisory + expectedErrMsg string + }{ + { + name: "successful advisory update", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: expect(t, expectations{ + path: "/repos/octo/hello-world/security-advisories/GHSA-xxxx-xxxx-xxxx", + requestBody: map[string]any{"state": "published", "severity": "high"}, + }).andThen(mockResponse(t, http.StatusOK, mockAdvisory)), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + "state": "published", + "severity": "high", + }, + expectError: false, + expectedAdvisory: mockAdvisory, + }, + { + name: "missing required ghsaId", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "state": "published", + }, + expectError: true, + expectedErrMsg: "missing required parameter: ghsaId", + }, + { + name: "no update fields provided", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + }, + expectError: true, + expectedErrMsg: "at least one of summary, description, vulnerabilities, cveId, cweIds, severity, cvssVectorString, credits, or state must be provided for update", + }, + { + name: "both severity and cvss set on update", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + "severity": "high", + "cvssVectorString": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", + }, + expectError: true, + expectedErrMsg: "severity and cvssVectorString cannot both be set", + }, + { + name: "API error handling", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message": "Validation Failed"}`)) + }, + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + "state": "published", + }, + expectError: true, + expectedErrMsg: "failed to update repository security advisory", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := mustNewGHClient(t, tc.mockedClient) + deps := BaseDeps{Client: client} + handler := toolDef.Handler(deps) + request := createMCPRequest(tc.requestArgs) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + if tc.expectedErrMsg != "" { + if tc.expectError { + if err != nil { + assert.Contains(t, err.Error(), tc.expectedErrMsg) + return + } + require.NotNil(t, result) + assert.True(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, tc.expectedErrMsg) + return + } + } + + require.NoError(t, err) + textContent := getTextResult(t, result) + + var returnedAdvisory github.SecurityAdvisory + err = json.Unmarshal([]byte(textContent.Text), &returnedAdvisory) + require.NoError(t, err) + assert.Equal(t, *tc.expectedAdvisory.GHSAID, *returnedAdvisory.GHSAID) + assert.Equal(t, *tc.expectedAdvisory.State, *returnedAdvisory.State) + }) + } +} + +func Test_RequestCVEForRepositorySecurityAdvisory(t *testing.T) { + toolDef := RequestCVEForRepositorySecurityAdvisory(translations.NullTranslationHelper) + tool := toolDef.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + assert.Equal(t, "request_cve_for_repository_security_advisory", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.False(t, tool.Annotations.ReadOnlyHint) + + schema, ok := tool.InputSchema.(*jsonschema.Schema) + require.True(t, ok, "InputSchema should be of type *jsonschema.Schema") + assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "ghsaId"}) + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + expectError bool + expectedText string + expectedErrMsg string + }{ + { + name: "successful CVE request", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, nil), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + }, + expectError: false, + expectedText: "CVE request submitted successfully", + }, + { + name: "successful CVE request with accepted status", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusAccepted) + }, + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + }, + expectError: false, + expectedText: "CVE request submitted successfully", + }, + { + name: "missing required ghsaId", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, nil), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + }, + expectError: true, + expectedErrMsg: "missing required parameter: ghsaId", + }, + { + name: "API error handling", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message": "Forbidden"}`)) + }, + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + }, + expectError: true, + expectedErrMsg: "failed to request CVE for repository security advisory", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := mustNewGHClient(t, tc.mockedClient) + deps := BaseDeps{Client: client} + handler := toolDef.Handler(deps) + request := createMCPRequest(tc.requestArgs) + + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + if tc.expectedErrMsg != "" { + if tc.expectError { + if err != nil { + assert.Contains(t, err.Error(), tc.expectedErrMsg) + return + } + require.NotNil(t, result) + assert.True(t, result.IsError) + assert.Contains(t, getTextResult(t, result).Text, tc.expectedErrMsg) + return + } + } + + require.NoError(t, err) + textContent := getTextResult(t, result) + assert.Equal(t, tc.expectedText, textContent.Text) + }) + } +} + +func Test_ParseAdvisoryVulnerabilities(t *testing.T) { + t.Run("required missing parameter", func(t *testing.T) { + _, err := parseAdvisoryVulnerabilities(map[string]any{}, "vulnerabilities", true) + require.Error(t, err) + assert.Contains(t, err.Error(), "missing required parameter: vulnerabilities") + }) + + t.Run("invalid parameter type", func(t *testing.T) { + _, err := parseAdvisoryVulnerabilities(map[string]any{ + "vulnerabilities": "not-an-array", + }, "vulnerabilities", true) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid vulnerabilities") + }) + + t.Run("valid vulnerabilities", func(t *testing.T) { + vulns, err := parseAdvisoryVulnerabilities(map[string]any{ + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, "vulnerabilities", true) + require.NoError(t, err) + require.Len(t, vulns, 1) + assert.Equal(t, "npm", vulns[0].Package.Ecosystem) + require.NotNil(t, vulns[0].Package.Name) + assert.Equal(t, "example-package", *vulns[0].Package.Name) + }) +} + +func Test_validateSeverityOrCVSS(t *testing.T) { + t.Run("create requires exactly one", func(t *testing.T) { + assert.NoError(t, validateSeverityOrCVSS("high", "", true)) + assert.NoError(t, validateSeverityOrCVSS("", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", true)) + assert.Error(t, validateSeverityOrCVSS("", "", true)) + assert.Error(t, validateSeverityOrCVSS("high", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", true)) + }) + + t.Run("update rejects both but allows neither", func(t *testing.T) { + assert.NoError(t, validateSeverityOrCVSS("", "", false)) + assert.NoError(t, validateSeverityOrCVSS("high", "", false)) + assert.NoError(t, validateSeverityOrCVSS("", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", false)) + assert.Error(t, validateSeverityOrCVSS("high", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", false)) + }) +} + +func TestSecurityAdvisoryWriteToolsRegistered(t *testing.T) { + expected := map[string]bool{ + "create_repository_security_advisory": false, + "update_repository_security_advisory": false, + "request_cve_for_repository_security_advisory": false, + } + + for _, tool := range AllTools(translations.NullTranslationHelper) { + if _, ok := expected[tool.Tool.Name]; ok { + assert.Equal(t, ToolsetMetadataSecurityAdvisories.ID, tool.Toolset.ID) + require.NotNil(t, tool.Tool.Annotations) + assert.Equal(t, expected[tool.Tool.Name], tool.Tool.Annotations.ReadOnlyHint) + delete(expected, tool.Tool.Name) + } + } + + assert.Empty(t, expected, "missing security advisory write tools: %v", expected) +} diff --git a/pkg/github/security_advisories_write_test.go b/pkg/github/security_advisories_write_test.go deleted file mode 100644 index d3a2600078..0000000000 --- a/pkg/github/security_advisories_write_test.go +++ /dev/null @@ -1,533 +0,0 @@ -package github - -import ( - "context" - "encoding/json" - "net/http" - "testing" - - "github.com/github/github-mcp-server/internal/toolsnaps" - "github.com/github/github-mcp-server/pkg/translations" - "github.com/google/go-github/v87/github" - "github.com/google/jsonschema-go/jsonschema" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func sampleAdvisoryVulnerabilities() []any { - return []any{ - map[string]any{ - "package": map[string]any{ - "ecosystem": "npm", - "name": "example-package", - }, - "vulnerable_version_range": "< 2.0.0", - "patched_versions": "2.0.0", - }, - } -} - -func mockRepositorySecurityAdvisory() *github.SecurityAdvisory { - return &github.SecurityAdvisory{ - GHSAID: github.Ptr("GHSA-xxxx-xxxx-xxxx"), - Summary: github.Ptr("Stored XSS in Core"), - Description: github.Ptr("A stored XSS vulnerability in Core."), - Severity: github.Ptr("high"), - State: github.Ptr("draft"), - } -} - -func Test_CreateRepositorySecurityAdvisory(t *testing.T) { - toolDef := CreateRepositorySecurityAdvisory(translations.NullTranslationHelper) - tool := toolDef.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - - assert.Equal(t, "create_repository_security_advisory", tool.Name) - assert.NotEmpty(t, tool.Description) - assert.False(t, tool.Annotations.ReadOnlyHint) - - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be of type *jsonschema.Schema") - assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "summary", "description", "vulnerabilities"}) - - mockAdvisory := mockRepositorySecurityAdvisory() - expectedRequestBody := map[string]any{ - "summary": "Stored XSS in Core", - "description": "A stored XSS vulnerability in Core.", - "severity": "high", - "vulnerabilities": []any{ - map[string]any{ - "package": map[string]any{ - "ecosystem": "npm", - "name": "example-package", - }, - "vulnerable_version_range": "< 2.0.0", - "patched_versions": "2.0.0", - }, - }, - } - - tests := []struct { - name string - mockedClient *http.Client - requestArgs map[string]any - expectError bool - expectedAdvisory *github.SecurityAdvisory - expectedErrMsg string - }{ - { - name: "successful advisory creation", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PostReposSecurityAdvisoriesByOwnerByRepo: expectRequestBody(t, expectedRequestBody).andThen( - mockResponse(t, http.StatusCreated, mockAdvisory), - ), - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "summary": "Stored XSS in Core", - "description": "A stored XSS vulnerability in Core.", - "severity": "high", - "vulnerabilities": sampleAdvisoryVulnerabilities(), - }, - expectError: false, - expectedAdvisory: mockAdvisory, - }, - { - name: "successful advisory creation with cvss only", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PostReposSecurityAdvisoriesByOwnerByRepo: expectRequestBody(t, map[string]any{ - "summary": "Stored XSS in Core", - "description": "A stored XSS vulnerability in Core.", - "cvss_vector_string": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", - "vulnerabilities": []any{ - map[string]any{ - "package": map[string]any{ - "ecosystem": "npm", - "name": "example-package", - }, - "vulnerable_version_range": "< 2.0.0", - "patched_versions": "2.0.0", - }, - }, - }).andThen(mockResponse(t, http.StatusCreated, mockAdvisory)), - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "summary": "Stored XSS in Core", - "description": "A stored XSS vulnerability in Core.", - "cvssVectorString": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", - "vulnerabilities": sampleAdvisoryVulnerabilities(), - }, - expectError: false, - expectedAdvisory: mockAdvisory, - }, - { - name: "missing required summary", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PostReposSecurityAdvisoriesByOwnerByRepo: mockResponse(t, http.StatusCreated, mockAdvisory), - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "description": "A stored XSS vulnerability in Core.", - "vulnerabilities": sampleAdvisoryVulnerabilities(), - }, - expectError: true, - expectedErrMsg: "missing required parameter: summary", - }, - { - name: "missing severity and cvss", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PostReposSecurityAdvisoriesByOwnerByRepo: mockResponse(t, http.StatusCreated, mockAdvisory), - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "summary": "Stored XSS in Core", - "description": "A stored XSS vulnerability in Core.", - "vulnerabilities": sampleAdvisoryVulnerabilities(), - }, - expectError: true, - expectedErrMsg: "exactly one of severity or cvssVectorString must be provided", - }, - { - name: "both severity and cvss set", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PostReposSecurityAdvisoriesByOwnerByRepo: mockResponse(t, http.StatusCreated, mockAdvisory), - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "summary": "Stored XSS in Core", - "description": "A stored XSS vulnerability in Core.", - "severity": "high", - "cvssVectorString": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", - "vulnerabilities": sampleAdvisoryVulnerabilities(), - }, - expectError: true, - expectedErrMsg: "severity and cvssVectorString cannot both be set", - }, - { - name: "API error handling", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PostReposSecurityAdvisoriesByOwnerByRepo: func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusForbidden) - _, _ = w.Write([]byte(`{"message": "Forbidden"}`)) - }, - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "summary": "Stored XSS in Core", - "description": "A stored XSS vulnerability in Core.", - "severity": "high", - "vulnerabilities": sampleAdvisoryVulnerabilities(), - }, - expectError: true, - expectedErrMsg: "failed to create repository security advisory", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - client := mustNewGHClient(t, tc.mockedClient) - deps := BaseDeps{Client: client} - handler := toolDef.Handler(deps) - request := createMCPRequest(tc.requestArgs) - - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - - if tc.expectedErrMsg != "" { - if tc.expectError { - if err != nil { - assert.Contains(t, err.Error(), tc.expectedErrMsg) - return - } - require.NotNil(t, result) - assert.True(t, result.IsError) - assert.Contains(t, getTextResult(t, result).Text, tc.expectedErrMsg) - return - } - } - - require.NoError(t, err) - textContent := getTextResult(t, result) - - var returnedAdvisory github.SecurityAdvisory - err = json.Unmarshal([]byte(textContent.Text), &returnedAdvisory) - require.NoError(t, err) - assert.Equal(t, *tc.expectedAdvisory.GHSAID, *returnedAdvisory.GHSAID) - assert.Equal(t, *tc.expectedAdvisory.Summary, *returnedAdvisory.Summary) - assert.Equal(t, *tc.expectedAdvisory.Description, *returnedAdvisory.Description) - assert.Equal(t, *tc.expectedAdvisory.Severity, *returnedAdvisory.Severity) - }) - } -} - -func Test_UpdateRepositorySecurityAdvisory(t *testing.T) { - toolDef := UpdateRepositorySecurityAdvisory(translations.NullTranslationHelper) - tool := toolDef.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - - assert.Equal(t, "update_repository_security_advisory", tool.Name) - assert.NotEmpty(t, tool.Description) - assert.False(t, tool.Annotations.ReadOnlyHint) - - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be of type *jsonschema.Schema") - assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "ghsaId"}) - - mockAdvisory := mockRepositorySecurityAdvisory() - mockAdvisory.State = github.Ptr("published") - - tests := []struct { - name string - mockedClient *http.Client - requestArgs map[string]any - expectError bool - expectedAdvisory *github.SecurityAdvisory - expectedErrMsg string - }{ - { - name: "successful advisory update", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: expect(t, expectations{ - path: "/repos/octo/hello-world/security-advisories/GHSA-xxxx-xxxx-xxxx", - requestBody: map[string]any{"state": "published", "severity": "high"}, - }).andThen(mockResponse(t, http.StatusOK, mockAdvisory)), - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "ghsaId": "GHSA-xxxx-xxxx-xxxx", - "state": "published", - "severity": "high", - }, - expectError: false, - expectedAdvisory: mockAdvisory, - }, - { - name: "missing required ghsaId", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, mockAdvisory), - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "state": "published", - }, - expectError: true, - expectedErrMsg: "missing required parameter: ghsaId", - }, - { - name: "no update fields provided", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, mockAdvisory), - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "ghsaId": "GHSA-xxxx-xxxx-xxxx", - }, - expectError: true, - expectedErrMsg: "at least one of summary, description, vulnerabilities, cveId, cweIds, severity, cvssVectorString, credits, or state must be provided for update", - }, - { - name: "both severity and cvss set on update", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, mockAdvisory), - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "ghsaId": "GHSA-xxxx-xxxx-xxxx", - "severity": "high", - "cvssVectorString": "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", - }, - expectError: true, - expectedErrMsg: "severity and cvssVectorString cannot both be set", - }, - { - name: "API error handling", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusUnprocessableEntity) - _, _ = w.Write([]byte(`{"message": "Validation Failed"}`)) - }, - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "ghsaId": "GHSA-xxxx-xxxx-xxxx", - "state": "published", - }, - expectError: true, - expectedErrMsg: "failed to update repository security advisory", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - client := mustNewGHClient(t, tc.mockedClient) - deps := BaseDeps{Client: client} - handler := toolDef.Handler(deps) - request := createMCPRequest(tc.requestArgs) - - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - - if tc.expectedErrMsg != "" { - if tc.expectError { - if err != nil { - assert.Contains(t, err.Error(), tc.expectedErrMsg) - return - } - require.NotNil(t, result) - assert.True(t, result.IsError) - assert.Contains(t, getTextResult(t, result).Text, tc.expectedErrMsg) - return - } - } - - require.NoError(t, err) - textContent := getTextResult(t, result) - - var returnedAdvisory github.SecurityAdvisory - err = json.Unmarshal([]byte(textContent.Text), &returnedAdvisory) - require.NoError(t, err) - assert.Equal(t, *tc.expectedAdvisory.GHSAID, *returnedAdvisory.GHSAID) - assert.Equal(t, *tc.expectedAdvisory.State, *returnedAdvisory.State) - }) - } -} - -func Test_RequestCVEForRepositorySecurityAdvisory(t *testing.T) { - toolDef := RequestCVEForRepositorySecurityAdvisory(translations.NullTranslationHelper) - tool := toolDef.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - - assert.Equal(t, "request_cve_for_repository_security_advisory", tool.Name) - assert.NotEmpty(t, tool.Description) - assert.False(t, tool.Annotations.ReadOnlyHint) - - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be of type *jsonschema.Schema") - assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "ghsaId"}) - - tests := []struct { - name string - mockedClient *http.Client - requestArgs map[string]any - expectError bool - expectedText string - expectedErrMsg string - }{ - { - name: "successful CVE request", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, nil), - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "ghsaId": "GHSA-xxxx-xxxx-xxxx", - }, - expectError: false, - expectedText: "CVE request submitted successfully", - }, - { - name: "successful CVE request with accepted status", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID: func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusAccepted) - }, - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "ghsaId": "GHSA-xxxx-xxxx-xxxx", - }, - expectError: false, - expectedText: "CVE request submitted successfully", - }, - { - name: "missing required ghsaId", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, nil), - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - }, - expectError: true, - expectedErrMsg: "missing required parameter: ghsaId", - }, - { - name: "API error handling", - mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID: func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusForbidden) - _, _ = w.Write([]byte(`{"message": "Forbidden"}`)) - }, - }), - requestArgs: map[string]any{ - "owner": "octo", - "repo": "hello-world", - "ghsaId": "GHSA-xxxx-xxxx-xxxx", - }, - expectError: true, - expectedErrMsg: "failed to request CVE for repository security advisory", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - client := mustNewGHClient(t, tc.mockedClient) - deps := BaseDeps{Client: client} - handler := toolDef.Handler(deps) - request := createMCPRequest(tc.requestArgs) - - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - - if tc.expectedErrMsg != "" { - if tc.expectError { - if err != nil { - assert.Contains(t, err.Error(), tc.expectedErrMsg) - return - } - require.NotNil(t, result) - assert.True(t, result.IsError) - assert.Contains(t, getTextResult(t, result).Text, tc.expectedErrMsg) - return - } - } - - require.NoError(t, err) - textContent := getTextResult(t, result) - assert.Equal(t, tc.expectedText, textContent.Text) - }) - } -} - -func Test_ParseAdvisoryVulnerabilities(t *testing.T) { - t.Run("required missing parameter", func(t *testing.T) { - _, err := parseAdvisoryVulnerabilities(map[string]any{}, "vulnerabilities", true) - require.Error(t, err) - assert.Contains(t, err.Error(), "missing required parameter: vulnerabilities") - }) - - t.Run("invalid parameter type", func(t *testing.T) { - _, err := parseAdvisoryVulnerabilities(map[string]any{ - "vulnerabilities": "not-an-array", - }, "vulnerabilities", true) - require.Error(t, err) - assert.Contains(t, err.Error(), "invalid vulnerabilities") - }) - - t.Run("valid vulnerabilities", func(t *testing.T) { - vulns, err := parseAdvisoryVulnerabilities(map[string]any{ - "vulnerabilities": sampleAdvisoryVulnerabilities(), - }, "vulnerabilities", true) - require.NoError(t, err) - require.Len(t, vulns, 1) - assert.Equal(t, "npm", vulns[0].Package.Ecosystem) - require.NotNil(t, vulns[0].Package.Name) - assert.Equal(t, "example-package", *vulns[0].Package.Name) - }) -} - -func Test_validateSeverityOrCVSS(t *testing.T) { - t.Run("create requires exactly one", func(t *testing.T) { - assert.NoError(t, validateSeverityOrCVSS("high", "", true)) - assert.NoError(t, validateSeverityOrCVSS("", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", true)) - assert.Error(t, validateSeverityOrCVSS("", "", true)) - assert.Error(t, validateSeverityOrCVSS("high", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", true)) - }) - - t.Run("update rejects both but allows neither", func(t *testing.T) { - assert.NoError(t, validateSeverityOrCVSS("", "", false)) - assert.NoError(t, validateSeverityOrCVSS("high", "", false)) - assert.NoError(t, validateSeverityOrCVSS("", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", false)) - assert.Error(t, validateSeverityOrCVSS("high", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", false)) - }) -} - -func TestSecurityAdvisoryWriteToolsRegistered(t *testing.T) { - expected := map[string]bool{ - "create_repository_security_advisory": false, - "update_repository_security_advisory": false, - "request_cve_for_repository_security_advisory": false, - } - - for _, tool := range AllTools(translations.NullTranslationHelper) { - if _, ok := expected[tool.Tool.Name]; ok { - assert.Equal(t, ToolsetMetadataSecurityAdvisories.ID, tool.Toolset.ID) - require.NotNil(t, tool.Tool.Annotations) - assert.Equal(t, expected[tool.Tool.Name], tool.Tool.Annotations.ReadOnlyHint) - delete(expected, tool.Tool.Name) - } - } - - assert.Empty(t, expected, "missing security advisory write tools: %v", expected) -} From 6a033642ab6272798783ea0361928d25c7f9612b Mon Sep 17 00:00:00 2001 From: advancedresearcharray Date: Tue, 9 Jun 2026 13:34:45 +0000 Subject: [PATCH 08/12] fix: validate vulnerability ecosystems and simplify CVE request handling Reject advisories missing package.ecosystem before calling GitHub, and remove unreachable AcceptedError handling since go-github already normalizes 202 responses from RequestCVE. Co-authored-by: Cursor --- pkg/github/security_advisories_test.go | 14 ++++++++++++++ pkg/github/security_advisories_write.go | 8 +++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/pkg/github/security_advisories_test.go b/pkg/github/security_advisories_test.go index c1caf853cd..e8b2046d0b 100644 --- a/pkg/github/security_advisories_test.go +++ b/pkg/github/security_advisories_test.go @@ -980,6 +980,20 @@ func Test_ParseAdvisoryVulnerabilities(t *testing.T) { require.NotNil(t, vulns[0].Package.Name) assert.Equal(t, "example-package", *vulns[0].Package.Name) }) + + t.Run("missing package ecosystem", func(t *testing.T) { + _, err := parseAdvisoryVulnerabilities(map[string]any{ + "vulnerabilities": []any{ + map[string]any{ + "package": map[string]any{ + "name": "example-package", + }, + }, + }, + }, "vulnerabilities", true) + require.Error(t, err) + assert.Contains(t, err.Error(), "vulnerabilities[0].package.ecosystem is required") + }) } func Test_validateSeverityOrCVSS(t *testing.T) { diff --git a/pkg/github/security_advisories_write.go b/pkg/github/security_advisories_write.go index 5c03cfc5f0..d194f16fc3 100644 --- a/pkg/github/security_advisories_write.go +++ b/pkg/github/security_advisories_write.go @@ -132,6 +132,11 @@ func parseAdvisoryVulnerabilities(args map[string]any, param string, required bo if required && len(vulns) == 0 { return nil, fmt.Errorf("missing required parameter: %s", param) } + for i, vuln := range vulns { + if vuln.Package.Ecosystem == "" { + return nil, fmt.Errorf("invalid %s: vulnerabilities[%d].package.ecosystem is required", param, i) + } + } return vulns, nil } @@ -597,9 +602,6 @@ func RequestCVEForRepositorySecurityAdvisory(t translations.TranslationHelperFun resp, err := client.SecurityAdvisories.RequestCVE(ctx, owner, repo, ghsaID) if err != nil { - if resp != nil && resp.StatusCode == http.StatusAccepted && isAcceptedError(err) { - return utils.NewToolResultText("CVE request accepted and is being processed"), nil, nil - } return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to request CVE for repository security advisory", resp, err), nil, nil } defer func() { _ = resp.Body.Close() }() From 58b77983ad796256878847cb4ab1a3d54c732797 Mon Sep 17 00:00:00 2001 From: Array Fleet Date: Tue, 9 Jun 2026 17:30:13 +0000 Subject: [PATCH 09/12] fix: validate vulnerability package names before API calls Reject advisories missing package.name alongside the existing ecosystem check so malformed vulnerability payloads fail fast with a clear error. Co-authored-by: Cursor --- pkg/github/security_advisories_test.go | 14 ++++++++++++++ pkg/github/security_advisories_write.go | 3 +++ 2 files changed, 17 insertions(+) diff --git a/pkg/github/security_advisories_test.go b/pkg/github/security_advisories_test.go index e8b2046d0b..99d2ef3f4b 100644 --- a/pkg/github/security_advisories_test.go +++ b/pkg/github/security_advisories_test.go @@ -994,6 +994,20 @@ func Test_ParseAdvisoryVulnerabilities(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "vulnerabilities[0].package.ecosystem is required") }) + + t.Run("missing package name", func(t *testing.T) { + _, err := parseAdvisoryVulnerabilities(map[string]any{ + "vulnerabilities": []any{ + map[string]any{ + "package": map[string]any{ + "ecosystem": "npm", + }, + }, + }, + }, "vulnerabilities", true) + require.Error(t, err) + assert.Contains(t, err.Error(), "vulnerabilities[0].package.name is required") + }) } func Test_validateSeverityOrCVSS(t *testing.T) { diff --git a/pkg/github/security_advisories_write.go b/pkg/github/security_advisories_write.go index d194f16fc3..323fe22784 100644 --- a/pkg/github/security_advisories_write.go +++ b/pkg/github/security_advisories_write.go @@ -136,6 +136,9 @@ func parseAdvisoryVulnerabilities(args map[string]any, param string, required bo if vuln.Package.Ecosystem == "" { return nil, fmt.Errorf("invalid %s: vulnerabilities[%d].package.ecosystem is required", param, i) } + if vuln.Package.Name == nil || *vuln.Package.Name == "" { + return nil, fmt.Errorf("invalid %s: vulnerabilities[%d].package.name is required", param, i) + } } return vulns, nil From 3c5110471b146755aa8ac5eefb5e4b4e57b2c48a Mon Sep 17 00:00:00 2001 From: Array Fleet Date: Tue, 9 Jun 2026 17:31:35 +0000 Subject: [PATCH 10/12] fix: validate advisory ecosystem and credit fields before API calls Reject invalid package ecosystems and malformed credit entries at the MCP layer so create/update advisory requests fail fast with clear errors. Co-authored-by: Cursor --- pkg/github/security_advisories_test.go | 69 +++++++++++++++++++++++++ pkg/github/security_advisories_write.go | 25 +++++++++ 2 files changed, 94 insertions(+) diff --git a/pkg/github/security_advisories_test.go b/pkg/github/security_advisories_test.go index 99d2ef3f4b..829e3435c7 100644 --- a/pkg/github/security_advisories_test.go +++ b/pkg/github/security_advisories_test.go @@ -1008,6 +1008,75 @@ func Test_ParseAdvisoryVulnerabilities(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "vulnerabilities[0].package.name is required") }) + + t.Run("invalid package ecosystem", func(t *testing.T) { + _, err := parseAdvisoryVulnerabilities(map[string]any{ + "vulnerabilities": []any{ + map[string]any{ + "package": map[string]any{ + "ecosystem": "invalid-ecosystem", + "name": "example-package", + }, + }, + }, + }, "vulnerabilities", true) + require.Error(t, err) + assert.Contains(t, err.Error(), `vulnerabilities[0].package.ecosystem "invalid-ecosystem" is invalid`) + }) +} + +func Test_ParseAdvisoryCredits(t *testing.T) { + t.Run("valid credits", func(t *testing.T) { + credits, err := parseAdvisoryCredits(map[string]any{ + "credits": []any{ + map[string]any{ + "login": "octocat", + "type": "finder", + }, + }, + }, "credits") + require.NoError(t, err) + require.Len(t, credits, 1) + assert.Equal(t, "octocat", credits[0].Login) + assert.Equal(t, "finder", credits[0].Type) + }) + + t.Run("missing login", func(t *testing.T) { + _, err := parseAdvisoryCredits(map[string]any{ + "credits": []any{ + map[string]any{ + "type": "finder", + }, + }, + }, "credits") + require.Error(t, err) + assert.Contains(t, err.Error(), "credits[0].login is required") + }) + + t.Run("missing type", func(t *testing.T) { + _, err := parseAdvisoryCredits(map[string]any{ + "credits": []any{ + map[string]any{ + "login": "octocat", + }, + }, + }, "credits") + require.Error(t, err) + assert.Contains(t, err.Error(), "credits[0].type is required") + }) + + t.Run("invalid type", func(t *testing.T) { + _, err := parseAdvisoryCredits(map[string]any{ + "credits": []any{ + map[string]any{ + "login": "octocat", + "type": "invalid-type", + }, + }, + }, "credits") + require.Error(t, err) + assert.Contains(t, err.Error(), `credits[0].type "invalid-type" is invalid`) + }) } func Test_validateSeverityOrCVSS(t *testing.T) { diff --git a/pkg/github/security_advisories_write.go b/pkg/github/security_advisories_write.go index 323fe22784..d451af39c9 100644 --- a/pkg/github/security_advisories_write.go +++ b/pkg/github/security_advisories_write.go @@ -17,6 +17,17 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" ) +var validAdvisoryEcosystems = map[string]struct{}{ + "actions": {}, "composer": {}, "erlang": {}, "go": {}, "maven": {}, "npm": {}, + "nuget": {}, "other": {}, "pip": {}, "pub": {}, "rubygems": {}, "rust": {}, "swift": {}, +} + +var validAdvisoryCreditTypes = map[string]struct{}{ + "analyst": {}, "finder": {}, "reporter": {}, "coordinator": {}, + "remediation_developer": {}, "remediation_reviewer": {}, "remediation_verifier": {}, + "tool": {}, "sponsor": {}, "other": {}, +} + var securityAdvisoryPackageSchema = &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -136,6 +147,9 @@ func parseAdvisoryVulnerabilities(args map[string]any, param string, required bo if vuln.Package.Ecosystem == "" { return nil, fmt.Errorf("invalid %s: vulnerabilities[%d].package.ecosystem is required", param, i) } + if _, ok := validAdvisoryEcosystems[vuln.Package.Ecosystem]; !ok { + return nil, fmt.Errorf("invalid %s: vulnerabilities[%d].package.ecosystem %q is invalid", param, i, vuln.Package.Ecosystem) + } if vuln.Package.Name == nil || *vuln.Package.Name == "" { return nil, fmt.Errorf("invalid %s: vulnerabilities[%d].package.name is required", param, i) } @@ -159,6 +173,17 @@ func parseAdvisoryCredits(args map[string]any, param string) ([]advisoryCreditRe if err := json.Unmarshal(data, &credits); err != nil { return nil, fmt.Errorf("invalid %s: %w", param, err) } + for i, credit := range credits { + if credit.Login == "" { + return nil, fmt.Errorf("invalid %s: credits[%d].login is required", param, i) + } + if credit.Type == "" { + return nil, fmt.Errorf("invalid %s: credits[%d].type is required", param, i) + } + if _, ok := validAdvisoryCreditTypes[credit.Type]; !ok { + return nil, fmt.Errorf("invalid %s: credits[%d].type %q is invalid", param, i, credit.Type) + } + } return credits, nil } From aa0a64db10107f09a312a57321353787dee780b2 Mon Sep 17 00:00:00 2001 From: Array Fleet Date: Tue, 9 Jun 2026 23:21:07 +0000 Subject: [PATCH 11/12] fix: validate advisory severity and state before API calls Reject invalid severity and state values client-side so create/update tools return clear validation errors instead of opaque GitHub API failures. Co-authored-by: Cursor --- pkg/github/security_advisories_test.go | 40 +++++++++++++++++++++++++ pkg/github/security_advisories_write.go | 36 ++++++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/pkg/github/security_advisories_test.go b/pkg/github/security_advisories_test.go index 829e3435c7..ad318e9338 100644 --- a/pkg/github/security_advisories_test.go +++ b/pkg/github/security_advisories_test.go @@ -654,6 +654,22 @@ func Test_CreateRepositorySecurityAdvisory(t *testing.T) { expectError: true, expectedErrMsg: "severity and cvssVectorString cannot both be set", }, + { + name: "invalid severity value", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: mockResponse(t, http.StatusCreated, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "severity": "urgent", + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: true, + expectedErrMsg: "severity must be one of: low, medium, high, critical", + }, { name: "API error handling", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ @@ -794,6 +810,20 @@ func Test_UpdateRepositorySecurityAdvisory(t *testing.T) { expectError: true, expectedErrMsg: "severity and cvssVectorString cannot both be set", }, + { + name: "invalid state value", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "GHSA-xxxx-xxxx-xxxx", + "state": "open", + }, + expectError: true, + expectedErrMsg: "state must be one of: draft, published, closed, triage", + }, { name: "API error handling", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ @@ -1093,6 +1123,16 @@ func Test_validateSeverityOrCVSS(t *testing.T) { assert.NoError(t, validateSeverityOrCVSS("", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", false)) assert.Error(t, validateSeverityOrCVSS("high", "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H", false)) }) + + t.Run("rejects invalid severity", func(t *testing.T) { + assert.Error(t, validateAdvisorySeverity("urgent")) + assert.NoError(t, validateAdvisorySeverity("critical")) + }) + + t.Run("rejects invalid state", func(t *testing.T) { + assert.Error(t, validateAdvisoryState("open")) + assert.NoError(t, validateAdvisoryState("published")) + }) } func TestSecurityAdvisoryWriteToolsRegistered(t *testing.T) { diff --git a/pkg/github/security_advisories_write.go b/pkg/github/security_advisories_write.go index d451af39c9..a2d5a68510 100644 --- a/pkg/github/security_advisories_write.go +++ b/pkg/github/security_advisories_write.go @@ -28,6 +28,14 @@ var validAdvisoryCreditTypes = map[string]struct{}{ "tool": {}, "sponsor": {}, "other": {}, } +var validAdvisorySeverities = map[string]struct{}{ + "low": {}, "medium": {}, "high": {}, "critical": {}, +} + +var validAdvisoryStates = map[string]struct{}{ + "draft": {}, "published": {}, "closed": {}, "triage": {}, +} + var securityAdvisoryPackageSchema = &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -195,9 +203,34 @@ func optionalStringPtr(value string) *string { return &value } +func validateAdvisorySeverity(severity string) error { + if severity == "" { + return nil + } + if _, ok := validAdvisorySeverities[severity]; !ok { + return fmt.Errorf("severity must be one of: low, medium, high, critical") + } + return nil +} + +func validateAdvisoryState(state string) error { + if state == "" { + return nil + } + if _, ok := validAdvisoryStates[state]; !ok { + return fmt.Errorf("state must be one of: draft, published, closed, triage") + } + return nil +} + func validateSeverityOrCVSS(severity, cvssVectorString string, requireOne bool) error { hasSeverity := severity != "" hasCVSS := cvssVectorString != "" + if hasSeverity { + if err := validateAdvisorySeverity(severity); err != nil { + return err + } + } if hasSeverity && hasCVSS { return fmt.Errorf("severity and cvssVectorString cannot both be set") } @@ -515,6 +548,9 @@ func UpdateRepositorySecurityAdvisory(t translations.TranslationHelperFunc) inve if err := validateSeverityOrCVSS(severity, cvssVectorString, false); err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + if err := validateAdvisoryState(state); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } requestBody := updateRepositorySecurityAdvisoryRequest{} if summary != "" { From 9f571cba285fa4b9287c0bf30db992121d7db977 Mon Sep 17 00:00:00 2001 From: Array Fleet Date: Tue, 9 Jun 2026 23:26:59 +0000 Subject: [PATCH 12/12] fix: address peer review for security advisory write tools Validate ghsaId format before REST path interpolation, add MCP safety annotations (OpenWorldHint/DestructiveHint), and drop unrelated #2605 confidence-parameter changes from this branch. Co-authored-by: Cursor --- docs/feature-flags.md | 3 +- .../create_repository_security_advisory.snap | 4 +- ..._cve_for_repository_security_advisory.snap | 1 + .../request_pull_request_reviewers.snap | 2 +- .../__toolsnaps__/set_issue_fields.snap | 11 +- .../__toolsnaps__/update_issue_labels.snap | 11 +- .../__toolsnaps__/update_issue_type.snap | 11 +- .../__toolsnaps__/update_pull_request.snap | 2 +- .../update_repository_security_advisory.snap | 2 + pkg/github/granular_tools_test.go | 321 ------------------ pkg/github/issues_granular.go | 96 ++---- pkg/github/security_advisories_test.go | 78 +++++ pkg/github/security_advisories_write.go | 38 ++- 13 files changed, 147 insertions(+), 433 deletions(-) diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 63fb28dc44..0b75a61bac 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -198,7 +198,6 @@ runtime behavior (such as output formatting) won't appear here. - **update_issue_type** - Update Issue Type - **Required OAuth Scopes**: `repo` - - `confidence`: How confident you are in this choice. Use 'high' for clear signal or explicit user request, 'medium' for reasonable inference with some ambiguity, 'low' for best guess with limited signal. (string, optional) - `is_suggestion`: If true, this issue type change is sent to the API as a suggestion (suggest:true) rather than an applied value. Whether the type is applied or recorded as a proposal is determined by the API. (boolean, optional) - `issue_number`: The issue number to update (number, required) - `issue_type`: The issue type to set (string, required) @@ -241,7 +240,7 @@ runtime behavior (such as output formatting) won't appear here. - `owner`: Repository owner (username or organization) (string, required) - `pullNumber`: The pull request number (number, required) - `repo`: Repository name (string, required) - - `reviewers`: GitHub usernames or ORG/team-slug team reviewers to request reviews from (string[], required) + - `reviewers`: GitHub usernames to request reviews from (string[], required) - **resolve_review_thread** - Resolve Review Thread - **Required OAuth Scopes**: `repo` diff --git a/pkg/github/__toolsnaps__/create_repository_security_advisory.snap b/pkg/github/__toolsnaps__/create_repository_security_advisory.snap index 4021780c58..60e342dd55 100644 --- a/pkg/github/__toolsnaps__/create_repository_security_advisory.snap +++ b/pkg/github/__toolsnaps__/create_repository_security_advisory.snap @@ -1,8 +1,10 @@ { "annotations": { + "destructiveHint": true, + "openWorldHint": true, "title": "Create repository security advisory" }, - "description": "Create a draft repository security advisory.", + "description": "Create a draft repository security advisory. When startPrivateFork is true, a temporary private fork is created for collaborating on a fix.", "inputSchema": { "properties": { "credits": { diff --git a/pkg/github/__toolsnaps__/request_cve_for_repository_security_advisory.snap b/pkg/github/__toolsnaps__/request_cve_for_repository_security_advisory.snap index 5d069b8259..e3f02b90ff 100644 --- a/pkg/github/__toolsnaps__/request_cve_for_repository_security_advisory.snap +++ b/pkg/github/__toolsnaps__/request_cve_for_repository_security_advisory.snap @@ -1,5 +1,6 @@ { "annotations": { + "openWorldHint": true, "title": "Request CVE for repository security advisory" }, "description": "Request a CVE ID from GitHub for a draft repository security advisory.", diff --git a/pkg/github/__toolsnaps__/request_pull_request_reviewers.snap b/pkg/github/__toolsnaps__/request_pull_request_reviewers.snap index 20f1ab62b6..7e6d33a274 100644 --- a/pkg/github/__toolsnaps__/request_pull_request_reviewers.snap +++ b/pkg/github/__toolsnaps__/request_pull_request_reviewers.snap @@ -37,4 +37,4 @@ "type": "object" }, "name": "request_pull_request_reviewers" -} \ No newline at end of file +} diff --git a/pkg/github/__toolsnaps__/set_issue_fields.snap b/pkg/github/__toolsnaps__/set_issue_fields.snap index e46febeeda..88c88fdc65 100644 --- a/pkg/github/__toolsnaps__/set_issue_fields.snap +++ b/pkg/github/__toolsnaps__/set_issue_fields.snap @@ -4,22 +4,13 @@ "openWorldHint": true, "title": "Set Issue Fields" }, - "description": "Set issue field values for an issue. Fields are organization-level custom fields (text, number, date, or single select). Use this to create or update field values on an issue. When setting values, include a confidence level (low, medium, or high) reflecting how certain you are about the choice.", + "description": "Set issue field values for an issue. Fields are organization-level custom fields (text, number, date, or single select). Use this to create or update field values on an issue.", "inputSchema": { "properties": { "fields": { "description": "Array of issue field values to set. Each element must have a 'field_id' (string, the GraphQL node ID of the field) and exactly one value field: 'text_value' for text fields, 'number_value' for number fields, 'date_value' (ISO 8601 date string) for date fields, or 'single_select_option_id' (the GraphQL node ID of the option) for single select fields. Set 'delete' to true to remove a field value.", "items": { "properties": { - "confidence": { - "description": "How confident you are in this choice. Use 'high' for clear signal or explicit user request, 'medium' for reasonable inference with some ambiguity, 'low' for best guess with limited signal.", - "enum": [ - "low", - "medium", - "high" - ], - "type": "string" - }, "date_value": { "description": "The value to set for a date field (ISO 8601 date string)", "type": "string" diff --git a/pkg/github/__toolsnaps__/update_issue_labels.snap b/pkg/github/__toolsnaps__/update_issue_labels.snap index 21f7fea6b6..3bdbdfc9ef 100644 --- a/pkg/github/__toolsnaps__/update_issue_labels.snap +++ b/pkg/github/__toolsnaps__/update_issue_labels.snap @@ -4,7 +4,7 @@ "openWorldHint": true, "title": "Update Issue Labels" }, - "description": "Update the labels of an existing issue. This replaces the current labels with the provided list. When setting values, include a confidence level (low, medium, or high) reflecting how certain you are about the choice.", + "description": "Update the labels of an existing issue. This replaces the current labels with the provided list.", "inputSchema": { "properties": { "issue_number": { @@ -22,15 +22,6 @@ }, { "properties": { - "confidence": { - "description": "How confident you are in this choice. Use 'high' for clear signal or explicit user request, 'medium' for reasonable inference with some ambiguity, 'low' for best guess with limited signal.", - "enum": [ - "low", - "medium", - "high" - ], - "type": "string" - }, "is_suggestion": { "description": "If true, this label is sent to the API as a suggestion (suggest:true) rather than an applied label. Whether the label is applied or recorded as a proposal is determined by the API.", "type": "boolean" diff --git a/pkg/github/__toolsnaps__/update_issue_type.snap b/pkg/github/__toolsnaps__/update_issue_type.snap index 2f39b2d3b8..da749cd466 100644 --- a/pkg/github/__toolsnaps__/update_issue_type.snap +++ b/pkg/github/__toolsnaps__/update_issue_type.snap @@ -4,18 +4,9 @@ "openWorldHint": true, "title": "Update Issue Type" }, - "description": "Update the type of an existing issue (e.g. 'bug', 'feature'). When setting values, include a confidence level (low, medium, or high) reflecting how certain you are about the choice.", + "description": "Update the type of an existing issue (e.g. 'bug', 'feature').", "inputSchema": { "properties": { - "confidence": { - "description": "How confident you are in this choice. Use 'high' for clear signal or explicit user request, 'medium' for reasonable inference with some ambiguity, 'low' for best guess with limited signal.", - "enum": [ - "low", - "medium", - "high" - ], - "type": "string" - }, "is_suggestion": { "description": "If true, this issue type change is sent to the API as a suggestion (suggest:true) rather than an applied value. Whether the type is applied or recorded as a proposal is determined by the API.", "type": "boolean" diff --git a/pkg/github/__toolsnaps__/update_pull_request.snap b/pkg/github/__toolsnaps__/update_pull_request.snap index 3d87fe75fe..640df79702 100644 --- a/pkg/github/__toolsnaps__/update_pull_request.snap +++ b/pkg/github/__toolsnaps__/update_pull_request.snap @@ -61,4 +61,4 @@ "type": "object" }, "name": "update_pull_request" -} \ No newline at end of file +} diff --git a/pkg/github/__toolsnaps__/update_repository_security_advisory.snap b/pkg/github/__toolsnaps__/update_repository_security_advisory.snap index d7aca45026..fa71065476 100644 --- a/pkg/github/__toolsnaps__/update_repository_security_advisory.snap +++ b/pkg/github/__toolsnaps__/update_repository_security_advisory.snap @@ -1,5 +1,7 @@ { "annotations": { + "destructiveHint": true, + "openWorldHint": true, "title": "Update repository security advisory" }, "description": "Update a repository security advisory, including publishing it.", diff --git a/pkg/github/granular_tools_test.go b/pkg/github/granular_tools_test.go index eb688a0b9f..ae34c1dd42 100644 --- a/pkg/github/granular_tools_test.go +++ b/pkg/github/granular_tools_test.go @@ -461,91 +461,6 @@ func TestGranularUpdateIssueLabelsInvalidRationale(t *testing.T) { } } -func TestGranularUpdateIssueLabelsConfidence(t *testing.T) { - tests := []struct { - name string - requestArgs map[string]any - expectedReq map[string]any - }{ - { - name: "label with confidence triggers object form", - requestArgs: map[string]any{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(1), - "labels": []any{ - map[string]any{"name": "bug", "confidence": "high"}, - }, - }, - expectedReq: map[string]any{ - "labels": []any{ - map[string]any{"name": "bug", "confidence": "high"}, - }, - }, - }, - { - name: "label with confidence and rationale", - requestArgs: map[string]any{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(1), - "labels": []any{ - map[string]any{"name": "bug", "rationale": "Reports a crash", "confidence": "medium"}, - }, - }, - expectedReq: map[string]any{ - "labels": []any{ - map[string]any{"name": "bug", "rationale": "Reports a crash", "confidence": "medium"}, - }, - }, - }, - { - name: "invalid confidence value", - requestArgs: map[string]any{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(1), - "labels": []any{ - map[string]any{"name": "bug", "confidence": "very_high"}, - }, - }, - expectedReq: nil, - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - if tc.expectedReq == nil { - // Error case - deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(nil))} - serverTool := GranularUpdateIssueLabels(translations.NullTranslationHelper) - handler := serverTool.Handler(deps) - - request := createMCPRequest(tc.requestArgs) - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - require.NoError(t, err) - - errorContent := getErrorResult(t, result) - assert.Contains(t, errorContent.Text, "confidence must be one of: low, medium, high") - return - } - - client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, tc.expectedReq). - andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(1)})), - })) - deps := BaseDeps{Client: client} - serverTool := GranularUpdateIssueLabels(translations.NullTranslationHelper) - handler := serverTool.Handler(deps) - - request := createMCPRequest(tc.requestArgs) - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - require.NoError(t, err) - assert.False(t, result.IsError) - }) - } -} - func TestGranularUpdateIssueMilestone(t *testing.T) { client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{ @@ -727,128 +642,6 @@ func TestGranularUpdateIssueTypeInvalidRationale(t *testing.T) { } } -func TestGranularUpdateIssueTypeConfidence(t *testing.T) { - tests := []struct { - name string - requestArgs map[string]any - expectedReq map[string]any - }{ - { - name: "type with confidence only", - requestArgs: map[string]any{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(1), - "issue_type": "bug", - "confidence": "high", - }, - expectedReq: map[string]any{ - "type": map[string]any{ - "value": "bug", - "confidence": "high", - }, - }, - }, - { - name: "type with confidence and rationale", - requestArgs: map[string]any{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(1), - "issue_type": "feature", - "rationale": "Asks for dark mode support", - "confidence": "medium", - }, - expectedReq: map[string]any{ - "type": map[string]any{ - "value": "feature", - "rationale": "Asks for dark mode support", - "confidence": "medium", - }, - }, - }, - { - name: "type with low confidence triggers object form", - requestArgs: map[string]any{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(1), - "issue_type": "bug", - "confidence": "low", - }, - expectedReq: map[string]any{ - "type": map[string]any{ - "value": "bug", - "confidence": "low", - }, - }, - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ - PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, tc.expectedReq). - andThen(mockResponse(t, http.StatusOK, &gogithub.Issue{Number: gogithub.Ptr(1)})), - })) - deps := BaseDeps{Client: client} - serverTool := GranularUpdateIssueType(translations.NullTranslationHelper) - handler := serverTool.Handler(deps) - - request := createMCPRequest(tc.requestArgs) - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - require.NoError(t, err) - assert.False(t, result.IsError) - }) - } -} - -func TestGranularUpdateIssueTypeInvalidConfidence(t *testing.T) { - tests := []struct { - name string - requestArgs map[string]any - expectedErrText string - }{ - { - name: "invalid confidence value", - requestArgs: map[string]any{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(1), - "issue_type": "bug", - "confidence": "very_high", - }, - expectedErrText: "confidence must be one of: low, medium, high", - }, - { - name: "confidence wrong type", - requestArgs: map[string]any{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(1), - "issue_type": "bug", - "confidence": float64(85), - }, - expectedErrText: "parameter confidence is not of type string", - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - deps := BaseDeps{Client: mustNewGHClient(t, MockHTTPClientWithHandlers(nil))} - serverTool := GranularUpdateIssueType(translations.NullTranslationHelper) - handler := serverTool.Handler(deps) - - request := createMCPRequest(tc.requestArgs) - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - require.NoError(t, err) - - errorContent := getErrorResult(t, result) - assert.Contains(t, errorContent.Text, tc.expectedErrText) - }) - } -} - func TestGranularUpdateIssueState(t *testing.T) { tests := []struct { name string @@ -1596,120 +1389,6 @@ func TestGranularSetIssueFields(t *testing.T) { assert.Contains(t, textContent.Text, "field rationale must be 280 characters or less") }) - t.Run("successful set with confidence", func(t *testing.T) { - confidence := "high" - matchers := []githubv4mock.Matcher{ - githubv4mock.NewQueryMatcher( - struct { - Repository struct { - Issue struct { - ID githubv4.ID - } `graphql:"issue(number: $issueNumber)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - }{}, - map[string]any{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "issueNumber": githubv4.Int(5), - }, - githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "issue": map[string]any{"id": "ISSUE_123"}, - }, - }), - ), - githubv4mock.NewMutationMatcher( - struct { - SetIssueFieldValue struct { - Issue struct { - ID githubv4.ID - Number githubv4.Int - URL githubv4.String - } - IssueFieldValues []struct { - TextValue struct { - Value string - } `graphql:"... on IssueFieldTextValue"` - SingleSelectValue struct { - Name string - } `graphql:"... on IssueFieldSingleSelectValue"` - DateValue struct { - Value string - } `graphql:"... on IssueFieldDateValue"` - NumberValue struct { - Value float64 - } `graphql:"... on IssueFieldNumberValue"` - } - } `graphql:"setIssueFieldValue(input: $input)"` - }{}, - SetIssueFieldValueInput{ - IssueID: githubv4.ID("ISSUE_123"), - IssueFields: []IssueFieldCreateOrUpdateInput{ - { - FieldID: githubv4.ID("FIELD_1"), - TextValue: githubv4.NewString(githubv4.String("hello")), - Confidence: &confidence, - }, - }, - }, - nil, - githubv4mock.DataResponse(map[string]any{ - "setIssueFieldValue": map[string]any{ - "issue": map[string]any{ - "id": "ISSUE_123", - "number": 5, - "url": "https://github.com/owner/repo/issues/5", - }, - }, - }), - ), - } - - gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matchers...)) - deps := BaseDeps{GQLClient: gqlClient} - serverTool := GranularSetIssueFields(translations.NullTranslationHelper) - handler := serverTool.Handler(deps) - - request := createMCPRequest(map[string]any{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(5), - "fields": []any{ - map[string]any{ - "field_id": "FIELD_1", - "text_value": "hello", - "confidence": "high", - }, - }, - }) - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - require.NoError(t, err) - assert.False(t, result.IsError) - }) - - t.Run("invalid confidence value returns error", func(t *testing.T) { - deps := BaseDeps{} - serverTool := GranularSetIssueFields(translations.NullTranslationHelper) - handler := serverTool.Handler(deps) - - request := createMCPRequest(map[string]any{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(5), - "fields": []any{ - map[string]any{ - "field_id": "FIELD_1", - "text_value": "hello", - "confidence": "very_high", - }, - }, - }) - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - require.NoError(t, err) - textContent := getTextResult(t, result) - assert.Contains(t, textContent.Text, "confidence must be one of: low, medium, high") - }) - t.Run("successful set with suggest flag", func(t *testing.T) { suggestTrue := githubv4.Boolean(true) matchers := []githubv4mock.Matcher{ diff --git a/pkg/github/issues_granular.go b/pkg/github/issues_granular.go index 22d26cc47f..73fa75413c 100644 --- a/pkg/github/issues_granular.go +++ b/pkg/github/issues_granular.go @@ -258,18 +258,17 @@ func GranularUpdateIssueAssignees(t translations.TranslationHelperFunc) inventor ) } -// labelWithIntent represents the object form of a label entry, allowing a -// rationale, confidence level, and/or suggest flag to be sent alongside the label name. -type labelWithIntent struct { - Name string `json:"name"` - Rationale string `json:"rationale,omitempty"` - Confidence string `json:"confidence,omitempty"` - Suggest bool `json:"suggest,omitempty"` +// labelWithRationale represents the object form of a label entry, allowing a +// rationale and/or suggest flag to be sent alongside the label name. +type labelWithRationale struct { + Name string `json:"name"` + Rationale string `json:"rationale,omitempty"` + Suggest bool `json:"suggest,omitempty"` } // labelsUpdateRequest is a custom request body for updating an issue's labels // where individual labels may optionally include a rationale. Each element of -// Labels is either a string (label name) or a labelWithIntent object. +// Labels is either a string (label name) or a labelWithRationale object. type labelsUpdateRequest struct { Labels []any `json:"labels"` } @@ -280,7 +279,7 @@ func GranularUpdateIssueLabels(t translations.TranslationHelperFunc) inventory.S ToolsetMetadataIssues, mcp.Tool{ Name: "update_issue_labels", - Description: t("TOOL_UPDATE_ISSUE_LABELS_DESCRIPTION", "Update the labels of an existing issue. This replaces the current labels with the provided list. When setting values, include a confidence level (low, medium, or high) reflecting how certain you are about the choice."), + Description: t("TOOL_UPDATE_ISSUE_LABELS_DESCRIPTION", "Update the labels of an existing issue. This replaces the current labels with the provided list."), Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_UPDATE_ISSUE_LABELS_USER_TITLE", "Update Issue Labels"), ReadOnlyHint: false, @@ -322,11 +321,6 @@ func GranularUpdateIssueLabels(t translations.TranslationHelperFunc) inventory.S "State the concrete signal (e.g. 'Reports a crash when saving' → bug).", MaxLength: jsonschema.Ptr(280), }, - "confidence": { - Type: "string", - Description: "How confident you are in this choice. Use 'high' for clear signal or explicit user request, 'medium' for reasonable inference with some ambiguity, 'low' for best guess with limited signal.", - Enum: []any{"low", "medium", "high"}, - }, "is_suggestion": { Type: "boolean", Description: "If true, this label is sent to the API as a suggestion (suggest:true) rather than an applied label. " + @@ -393,25 +387,18 @@ func GranularUpdateIssueLabels(t translations.TranslationHelperFunc) inventory.S if len([]rune(rationale)) > 280 { return utils.NewToolResultError("label rationale must be 280 characters or less"), nil, nil } - confidence, err := OptionalParam[string](v, "confidence") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - if confidence != "" && confidence != "low" && confidence != "medium" && confidence != "high" { - return utils.NewToolResultError("confidence must be one of: low, medium, high"), nil, nil - } isSuggestion, err := OptionalParam[bool](v, "is_suggestion") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } - if rationale == "" && !isSuggestion && confidence == "" { + if rationale == "" && !isSuggestion { payload = append(payload, name) } else { useObjectForm = true - payload = append(payload, labelWithIntent{Name: name, Rationale: rationale, Confidence: confidence, Suggest: isSuggestion}) + payload = append(payload, labelWithRationale{Name: name, Rationale: rationale, Suggest: isSuggestion}) } default: - return utils.NewToolResultError("each label must be a string or an object with 'name' and optional 'rationale', 'confidence', and/or 'is_suggestion'"), nil, nil + return utils.NewToolResultError("each label must be a string or an object with 'name' and optional 'rationale' and/or 'is_suggestion'"), nil, nil } } @@ -483,19 +470,18 @@ func GranularUpdateIssueMilestone(t translations.TranslationHelperFunc) inventor ) } -// issueTypeWithIntent represents the object form of the issue type field, -// allowing a rationale, confidence level, and/or suggest flag to be sent alongside the type name. -type issueTypeWithIntent struct { - Value string `json:"value"` - Rationale string `json:"rationale,omitempty"` - Confidence string `json:"confidence,omitempty"` - Suggest bool `json:"suggest,omitempty"` +// issueTypeWithRationale represents the object form of the issue type field, +// allowing a rationale and/or suggest flag to be sent alongside the type name. +type issueTypeWithRationale struct { + Value string `json:"value"` + Rationale string `json:"rationale,omitempty"` + Suggest bool `json:"suggest,omitempty"` } // issueTypeUpdateRequest is a custom request body for updating an issue type -// with optional intent metadata, using the object form that the REST API accepts. +// with an optional rationale, using the object form that the REST API accepts. type issueTypeUpdateRequest struct { - Type issueTypeWithIntent `json:"type"` + Type issueTypeWithRationale `json:"type"` } // GranularUpdateIssueType creates a tool to update an issue's type. @@ -504,7 +490,7 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser ToolsetMetadataIssues, mcp.Tool{ Name: "update_issue_type", - Description: t("TOOL_UPDATE_ISSUE_TYPE_DESCRIPTION", "Update the type of an existing issue (e.g. 'bug', 'feature'). When setting values, include a confidence level (low, medium, or high) reflecting how certain you are about the choice."), + Description: t("TOOL_UPDATE_ISSUE_TYPE_DESCRIPTION", "Update the type of an existing issue (e.g. 'bug', 'feature')."), Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_UPDATE_ISSUE_TYPE_USER_TITLE", "Update Issue Type"), ReadOnlyHint: false, @@ -537,11 +523,6 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser "State the concrete signal (e.g. 'Reports a crash when saving' → bug, 'Asks for dark mode support' → feature).", MaxLength: jsonschema.Ptr(280), }, - "confidence": { - Type: "string", - Description: "How confident you are in this choice. Use 'high' for clear signal or explicit user request, 'medium' for reasonable inference with some ambiguity, 'low' for best guess with limited signal.", - Enum: []any{"low", "medium", "high"}, - }, "is_suggestion": { Type: "boolean", Description: "If true, this issue type change is sent to the API as a suggestion (suggest:true) rather than an applied value. " + @@ -577,13 +558,6 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser if len([]rune(rationale)) > 280 { return utils.NewToolResultError("parameter rationale must be 280 characters or less"), nil, nil } - confidence, err := OptionalParam[string](args, "confidence") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - if confidence != "" && confidence != "low" && confidence != "medium" && confidence != "high" { - return utils.NewToolResultError("confidence must be one of: low, medium, high"), nil, nil - } isSuggestion, err := OptionalParam[bool](args, "is_suggestion") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil @@ -595,13 +569,12 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser } var body any - if rationale != "" || isSuggestion || confidence != "" { + if rationale != "" || isSuggestion { body = &issueTypeUpdateRequest{ - Type: issueTypeWithIntent{ - Value: issueType, - Rationale: rationale, - Confidence: confidence, - Suggest: isSuggestion, + Type: issueTypeWithRationale{ + Value: issueType, + Rationale: rationale, + Suggest: isSuggestion, }, } } else { @@ -914,7 +887,6 @@ type IssueFieldCreateOrUpdateInput struct { SingleSelectOptionID *githubv4.ID `json:"singleSelectOptionId,omitempty"` Delete *githubv4.Boolean `json:"delete,omitempty"` Rationale *githubv4.String `json:"rationale,omitempty"` - Confidence *string `json:"confidence,omitempty"` Suggest *githubv4.Boolean `json:"suggest,omitempty"` } @@ -924,7 +896,7 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv ToolsetMetadataIssues, mcp.Tool{ Name: "set_issue_fields", - Description: t("TOOL_SET_ISSUE_FIELDS_DESCRIPTION", "Set issue field values for an issue. Fields are organization-level custom fields (text, number, date, or single select). Use this to create or update field values on an issue. When setting values, include a confidence level (low, medium, or high) reflecting how certain you are about the choice."), + Description: t("TOOL_SET_ISSUE_FIELDS_DESCRIPTION", "Set issue field values for an issue. Fields are organization-level custom fields (text, number, date, or single select). Use this to create or update field values on an issue."), Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_SET_ISSUE_FIELDS_USER_TITLE", "Set Issue Fields"), ReadOnlyHint: false, @@ -984,11 +956,6 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv "State the concrete signal (e.g. 'Reports a crash when saving' → high priority).", MaxLength: jsonschema.Ptr(280), }, - "confidence": { - Type: "string", - Description: "How confident you are in this choice. Use 'high' for clear signal or explicit user request, 'medium' for reasonable inference with some ambiguity, 'low' for best guess with limited signal.", - Enum: []any{"low", "medium", "high"}, - }, "is_suggestion": { Type: "boolean", Description: "If true, this field value is sent to the API as a suggestion (suggest:true) rather than an applied value. " + @@ -1106,17 +1073,6 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv } } - confidence, err := OptionalParam[string](fieldMap, "confidence") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - if confidence != "" && confidence != "low" && confidence != "medium" && confidence != "high" { - return utils.NewToolResultError("confidence must be one of: low, medium, high"), nil, nil - } - if confidence != "" { - input.Confidence = &confidence - } - isSuggestion, err := OptionalParam[bool](fieldMap, "is_suggestion") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil diff --git a/pkg/github/security_advisories_test.go b/pkg/github/security_advisories_test.go index ad318e9338..d1460574e8 100644 --- a/pkg/github/security_advisories_test.go +++ b/pkg/github/security_advisories_test.go @@ -530,6 +530,10 @@ func Test_CreateRepositorySecurityAdvisory(t *testing.T) { assert.Equal(t, "create_repository_security_advisory", tool.Name) assert.NotEmpty(t, tool.Description) assert.False(t, tool.Annotations.ReadOnlyHint) + require.NotNil(t, tool.Annotations.OpenWorldHint) + assert.True(t, *tool.Annotations.OpenWorldHint) + require.NotNil(t, tool.Annotations.DestructiveHint) + assert.True(t, *tool.Annotations.DestructiveHint) schema, ok := tool.InputSchema.(*jsonschema.Schema) require.True(t, ok, "InputSchema should be of type *jsonschema.Schema") @@ -608,6 +612,38 @@ func Test_CreateRepositorySecurityAdvisory(t *testing.T) { expectError: false, expectedAdvisory: mockAdvisory, }, + { + name: "successful advisory creation with startPrivateFork", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesByOwnerByRepo: expectRequestBody(t, map[string]any{ + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "severity": "high", + "start_private_fork": true, + "vulnerabilities": []any{ + map[string]any{ + "package": map[string]any{ + "ecosystem": "npm", + "name": "example-package", + }, + "vulnerable_version_range": "< 2.0.0", + "patched_versions": "2.0.0", + }, + }, + }).andThen(mockResponse(t, http.StatusCreated, mockAdvisory)), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "summary": "Stored XSS in Core", + "description": "A stored XSS vulnerability in Core.", + "severity": "high", + "startPrivateFork": true, + "vulnerabilities": sampleAdvisoryVulnerabilities(), + }, + expectError: false, + expectedAdvisory: mockAdvisory, + }, { name: "missing required summary", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ @@ -735,6 +771,10 @@ func Test_UpdateRepositorySecurityAdvisory(t *testing.T) { assert.Equal(t, "update_repository_security_advisory", tool.Name) assert.NotEmpty(t, tool.Description) assert.False(t, tool.Annotations.ReadOnlyHint) + require.NotNil(t, tool.Annotations.OpenWorldHint) + assert.True(t, *tool.Annotations.OpenWorldHint) + require.NotNil(t, tool.Annotations.DestructiveHint) + assert.True(t, *tool.Annotations.DestructiveHint) schema, ok := tool.InputSchema.(*jsonschema.Schema) require.True(t, ok, "InputSchema should be of type *jsonschema.Schema") @@ -769,6 +809,20 @@ func Test_UpdateRepositorySecurityAdvisory(t *testing.T) { expectError: false, expectedAdvisory: mockAdvisory, }, + { + name: "invalid ghsaId format", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposSecurityAdvisoriesByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, mockAdvisory), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "invalid/../../path", + "state": "published", + }, + expectError: true, + expectedErrMsg: "invalid ghsaId format: must match GHSA-xxxx-xxxx-xxxx", + }, { name: "missing required ghsaId", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ @@ -885,6 +939,9 @@ func Test_RequestCVEForRepositorySecurityAdvisory(t *testing.T) { assert.Equal(t, "request_cve_for_repository_security_advisory", tool.Name) assert.NotEmpty(t, tool.Description) assert.False(t, tool.Annotations.ReadOnlyHint) + require.NotNil(t, tool.Annotations.OpenWorldHint) + assert.True(t, *tool.Annotations.OpenWorldHint) + assert.Nil(t, tool.Annotations.DestructiveHint) schema, ok := tool.InputSchema.(*jsonschema.Schema) require.True(t, ok, "InputSchema should be of type *jsonschema.Schema") @@ -926,6 +983,19 @@ func Test_RequestCVEForRepositorySecurityAdvisory(t *testing.T) { expectError: false, expectedText: "CVE request submitted successfully", }, + { + name: "invalid ghsaId format", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PostReposSecurityAdvisoriesCveByOwnerByRepoByGhsaID: mockResponse(t, http.StatusOK, nil), + }), + requestArgs: map[string]any{ + "owner": "octo", + "repo": "hello-world", + "ghsaId": "not-a-valid-ghsa", + }, + expectError: true, + expectedErrMsg: "invalid ghsaId format: must match GHSA-xxxx-xxxx-xxxx", + }, { name: "missing required ghsaId", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ @@ -1135,6 +1205,14 @@ func Test_validateSeverityOrCVSS(t *testing.T) { }) } +func Test_validateGHSAID(t *testing.T) { + assert.NoError(t, validateGHSAID("GHSA-xxxx-xxxx-xxxx")) + assert.NoError(t, validateGHSAID("ghsa-abcd-1234-5678")) + assert.Error(t, validateGHSAID("invalid-ghsa-id")) + assert.Error(t, validateGHSAID("GHSA-xxxx-xxxx-xxxx/extra")) + assert.Error(t, validateGHSAID("../etc/passwd")) +} + func TestSecurityAdvisoryWriteToolsRegistered(t *testing.T) { expected := map[string]bool{ "create_repository_security_advisory": false, diff --git a/pkg/github/security_advisories_write.go b/pkg/github/security_advisories_write.go index a2d5a68510..f9d205eea5 100644 --- a/pkg/github/security_advisories_write.go +++ b/pkg/github/security_advisories_write.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "regexp" ghErrors "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/inventory" @@ -36,6 +37,15 @@ var validAdvisoryStates = map[string]struct{}{ "draft": {}, "published": {}, "closed": {}, "triage": {}, } +var ghsaIDPattern = regexp.MustCompile(`(?i)^GHSA-[a-z0-9]{4}-[a-z0-9]{4}-[a-z0-9]{4}$`) + +func validateGHSAID(ghsaID string) error { + if !ghsaIDPattern.MatchString(ghsaID) { + return fmt.Errorf("invalid ghsaId format: must match GHSA-xxxx-xxxx-xxxx") + } + return nil +} + var securityAdvisoryPackageSchema = &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -251,6 +261,9 @@ func marshalRepositorySecurityAdvisoryResponse(advisory *github.SecurityAdvisory func repositorySecurityAdvisoryRequest(ctx context.Context, client *github.Client, method, owner, repo, ghsaID string, body any) (*github.SecurityAdvisory, *github.Response, error) { url := fmt.Sprintf("repos/%s/%s/security-advisories", owner, repo) if ghsaID != "" { + if err := validateGHSAID(ghsaID); err != nil { + return nil, nil, err + } url = fmt.Sprintf("%s/%s", url, ghsaID) } @@ -273,10 +286,12 @@ func CreateRepositorySecurityAdvisory(t translations.TranslationHelperFunc) inve ToolsetMetadataSecurityAdvisories, mcp.Tool{ Name: "create_repository_security_advisory", - Description: t("TOOL_CREATE_REPOSITORY_SECURITY_ADVISORY_DESCRIPTION", "Create a draft repository security advisory."), + Description: t("TOOL_CREATE_REPOSITORY_SECURITY_ADVISORY_DESCRIPTION", "Create a draft repository security advisory. When startPrivateFork is true, a temporary private fork is created for collaborating on a fix."), Annotations: &mcp.ToolAnnotations{ - Title: t("TOOL_CREATE_REPOSITORY_SECURITY_ADVISORY_USER_TITLE", "Create repository security advisory"), - ReadOnlyHint: false, + Title: t("TOOL_CREATE_REPOSITORY_SECURITY_ADVISORY_USER_TITLE", "Create repository security advisory"), + ReadOnlyHint: false, + OpenWorldHint: jsonschema.Ptr(true), + DestructiveHint: jsonschema.Ptr(true), }, InputSchema: &jsonschema.Schema{ Type: "object", @@ -432,8 +447,10 @@ func UpdateRepositorySecurityAdvisory(t translations.TranslationHelperFunc) inve Name: "update_repository_security_advisory", Description: t("TOOL_UPDATE_REPOSITORY_SECURITY_ADVISORY_DESCRIPTION", "Update a repository security advisory, including publishing it."), Annotations: &mcp.ToolAnnotations{ - Title: t("TOOL_UPDATE_REPOSITORY_SECURITY_ADVISORY_USER_TITLE", "Update repository security advisory"), - ReadOnlyHint: false, + Title: t("TOOL_UPDATE_REPOSITORY_SECURITY_ADVISORY_USER_TITLE", "Update repository security advisory"), + ReadOnlyHint: false, + OpenWorldHint: jsonschema.Ptr(true), + DestructiveHint: jsonschema.Ptr(true), }, InputSchema: &jsonschema.Schema{ Type: "object", @@ -509,6 +526,9 @@ func UpdateRepositorySecurityAdvisory(t translations.TranslationHelperFunc) inve if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + if err := validateGHSAID(ghsaID); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } summary, err := OptionalParam[string](args, "summary") if err != nil { return utils.NewToolResultError(err.Error()), nil, nil @@ -622,8 +642,9 @@ func RequestCVEForRepositorySecurityAdvisory(t translations.TranslationHelperFun Name: "request_cve_for_repository_security_advisory", Description: t("TOOL_REQUEST_CVE_FOR_REPOSITORY_SECURITY_ADVISORY_DESCRIPTION", "Request a CVE ID from GitHub for a draft repository security advisory."), Annotations: &mcp.ToolAnnotations{ - Title: t("TOOL_REQUEST_CVE_FOR_REPOSITORY_SECURITY_ADVISORY_USER_TITLE", "Request CVE for repository security advisory"), - ReadOnlyHint: false, + Title: t("TOOL_REQUEST_CVE_FOR_REPOSITORY_SECURITY_ADVISORY_USER_TITLE", "Request CVE for repository security advisory"), + ReadOnlyHint: false, + OpenWorldHint: jsonschema.Ptr(true), }, InputSchema: &jsonschema.Schema{ Type: "object", @@ -658,6 +679,9 @@ func RequestCVEForRepositorySecurityAdvisory(t translations.TranslationHelperFun if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + if err := validateGHSAID(ghsaID); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } client, err := deps.GetClient(ctx) if err != nil {