Conversation
|
Great job! No new security vulnerabilities introduced in this pull requestUse @Checkmarx to interact with Checkmarx PR Assistant. |
aeae296 to
25ffa9d
Compare
There was a problem hiding this comment.
Pull request overview
Adds initial “MCP Import” capability plus Automation Framework support for configuring the MCP server and importing external MCP servers, along with a JSON-RPC variant to structure/import MCP traffic into the Sites Tree and expose fuzzable parameters.
Changes:
- Introduces MCP JSON-RPC
Variantfor Sites Tree pathing and parameter extraction/mutation. - Adds external MCP server importer (GUI dialog + Automation
mcp-importjob) and MCP server config Automation job (mcp-config). - Expands help/docs and i18n strings; adds Automation YAML templates.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| addOns/mcp/src/test/java/org/zaproxy/addon/mcp/importer/VariantJsonRpcUnitTest.java | Adds unit tests for the new JSON-RPC Variant behavior. |
| addOns/mcp/src/main/resources/org/zaproxy/addon/mcp/resources/Messages.properties | Adds new UI/job strings for config/import/automation and options panel. |
| addOns/mcp/src/main/resources/org/zaproxy/addon/mcp/resources/mcp-import-min.yaml | Adds minimal Automation template for mcp-import. |
| addOns/mcp/src/main/resources/org/zaproxy/addon/mcp/resources/mcp-import-max.yaml | Adds maximal Automation template for mcp-import (includes securityKey). |
| addOns/mcp/src/main/resources/org/zaproxy/addon/mcp/resources/mcp-config-min.yaml | Adds minimal Automation template for mcp-config. |
| addOns/mcp/src/main/resources/org/zaproxy/addon/mcp/resources/mcp-config-max.yaml | Adds maximal Automation template for mcp-config (security key options). |
| addOns/mcp/src/main/javahelp/org/zaproxy/addon/mcp/resources/help/toc.xml | Adds Automation section to the add-on help TOC. |
| addOns/mcp/src/main/javahelp/org/zaproxy/addon/mcp/resources/help/map.jhm | Maps new Automation help targets to HTML pages. |
| addOns/mcp/src/main/javahelp/org/zaproxy/addon/mcp/resources/help/index.xml | Adds help index entries for Automation pages. |
| addOns/mcp/src/main/javahelp/org/zaproxy/addon/mcp/resources/help/contents/mcp.html | Expands overview docs (importing servers, Sites Tree structure, Automation). |
| addOns/mcp/src/main/javahelp/org/zaproxy/addon/mcp/resources/help/contents/mcp-options.html | Documents new “Enable MCP Server” option and updates “See Also”. |
| addOns/mcp/src/main/javahelp/org/zaproxy/addon/mcp/resources/help/contents/mcp-automation.html | New Automation overview help page. |
| addOns/mcp/src/main/javahelp/org/zaproxy/addon/mcp/resources/help/contents/mcp-automation-import.html | New help page for mcp-import job. |
| addOns/mcp/src/main/javahelp/org/zaproxy/addon/mcp/resources/help/contents/mcp-automation-config.html | New help page for mcp-config job. |
| addOns/mcp/src/main/java/org/zaproxy/addon/mcp/McpParam.java | Adds persisted enabled setting for MCP server. |
| addOns/mcp/src/main/java/org/zaproxy/addon/mcp/McpOptionsPanel.java | Adds “Enable MCP Server” toggle and enables/disables other options accordingly. |
| addOns/mcp/src/main/java/org/zaproxy/addon/mcp/importer/VariantMcpJsonRpc.java | New JSON-RPC variant for Sites Tree pathing + fuzzable param extraction/mutation. |
| addOns/mcp/src/main/java/org/zaproxy/addon/mcp/importer/McpImporter.java | New importer that probes an external MCP server and records traffic into history/tree. |
| addOns/mcp/src/main/java/org/zaproxy/addon/mcp/importer/ImportMcpServerDialog.java | New GUI dialog to run the importer. |
| addOns/mcp/src/main/java/org/zaproxy/addon/mcp/ExtensionMcp.java | Registers variant, automation jobs, import menu item; updates server start behavior. |
| addOns/mcp/src/main/java/org/zaproxy/addon/mcp/automation/McpConfigJobDialog.java | New dialog for mcp-config Automation job. |
| addOns/mcp/src/main/java/org/zaproxy/addon/mcp/automation/McpConfigJob.java | New mcp-config Automation job that updates MCP server settings. |
| addOns/mcp/src/main/java/org/zaproxy/addon/mcp/automation/ImportMcpServerJobDialog.java | New dialog for mcp-import Automation job. |
| addOns/mcp/src/main/java/org/zaproxy/addon/mcp/automation/ImportMcpServerJob.java | New mcp-import Automation job that runs the importer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
af33769 to
bc0b3a9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (value.isObject()) { | ||
| // Flatten one level: "arguments": {"target": "..."} → "target" | ||
| value.properties() | ||
| .forEach( | ||
| inner -> | ||
| addParamOrTemplateVars( | ||
| inner.getKey(), | ||
| inner.getValue().asText(), | ||
| key + "." + inner.getKey())); | ||
| } else { | ||
| addParamOrTemplateVars(key, value.asText(), key); | ||
| } |
There was a problem hiding this comment.
setMessage uses JsonNode#asText() for all non-object values (and for flattened inner values). For array/object nodes asText() returns an empty string, which would silently create empty-value parameters (or miss template vars) for tools/prompts that legitimately take arrays/objects. Consider handling non-scalar nodes explicitly (e.g. value.isValueNode() ? value.asText() : value.toString() or skipping non-scalars) so parameter extraction reflects the actual JSON structure.
| @@ -184,8 +211,23 @@ private void optionsChanged(OptionsParam optionsParam) { | |||
| } | |||
| } | |||
There was a problem hiding this comment.
optionsChanged only reacts to port changes. If the user disables/enables the MCP server in the options panel, the listener will not stop/start the server, leaving the running state out of sync with param.isEnabled(). Consider delegating to applyServerConfig() (or at least handling param.isEnabled() changes) from this listener so enable/disable takes effect immediately after saving options.
026f6af to
82571fc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private JTextField getSecurityKeyField() { | ||
| if (fieldSecurityKey == null) { | ||
| fieldSecurityKey = new JTextField(30); | ||
| } | ||
| return fieldSecurityKey; | ||
| } |
There was a problem hiding this comment.
The import dialog uses a plain JTextField for the Authorization/security key, which exposes tokens in clear text. Consider switching this to a JPasswordField (or similar masked input) to reduce accidental disclosure (e.g., screen sharing).
| } else if (queryParamLocations.containsKey(param)) { | ||
| String[] location = queryParamLocations.get(param); | ||
| String fieldName = location[0]; | ||
| String currentUri = paramsObj.get(fieldName).asText(); | ||
| String newUri = | ||
| currentUri.replaceAll( | ||
| "([?&]" + Pattern.quote(param) + "=)[^&]*", | ||
| "$1" + Matcher.quoteReplacement(value)); | ||
| paramsObj.put(fieldName, newUri); |
There was a problem hiding this comment.
extractUriParams() supports query params without an '=' (e.g. "?flag"), but setParameter() only replaces patterns of the form "?name=..."/"&name=...". For params extracted from a "?flag" query, setParameter() will not update the URI. Consider handling the no-'=' case (e.g. convert "?flag" to "?flag=" or replace whole-token matches).
Signed-off-by: Simon Bennetts <psiinon@gmail.com>
|
Ready for (human) review |

No description provided.