Conversation
|
Great job! No new security vulnerabilities introduced in this pull requestUse @Checkmarx to interact with Checkmarx PR Assistant. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for “standalone” Import/Export exporter types (not backed by message history) and uses that capability to integrate Client Map export into the standard Import/Export automation export job (type clientmap).
Changes:
- Add
supportsStandaloneExport()/export(Writer)toExporterTypeand invoke standalone exports fromExporter. - Introduce a Client add-on sub-extension that registers a
clientmapexporter with the Import/Export add-on. - Add tests/docs/changelog entries for the new standalone-export pathway and the Client Map export type.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| addOns/exim/src/main/java/org/zaproxy/addon/exim/Exporter.java | Executes standalone exporters when message export is unsupported. |
| addOns/exim/src/main/java/org/zaproxy/addon/exim/ExporterType.java | Adds standalone-export capability API to exporter types. |
| addOns/exim/src/test/java/org/zaproxy/addon/exim/ExporterUnitTest.java | Adds coverage ensuring standalone export is invoked for applicable types. |
| addOns/exim/CHANGELOG.md | Documents standalone export type support in Unreleased notes. |
| addOns/client/src/main/java/org/zaproxy/addon/client/exim/ExtensionClientExim.java | Registers/unregisters the Client Map exporter with Import/Export when installed. |
| addOns/client/src/main/java/org/zaproxy/addon/client/exim/ClientMapExporter.java | Implements the clientmap standalone exporter type. |
| addOns/client/src/main/java/org/zaproxy/addon/client/ExtensionClientIntegration.java | Adds writer-based Client Map export entrypoint for re-use by Import/Export. |
| addOns/client/src/test/java/org/zaproxy/addon/client/exim/ClientMapExporterUnitTest.java | Unit tests for ClientMapExporter behavior and delegation. |
| addOns/client/src/main/resources/org/zaproxy/addon/client/resources/Messages.properties | Adds i18n strings for the new Import/Export integration UI text. |
| addOns/client/src/main/javahelp/org/zaproxy/addon/client/resources/help/contents/automation.html | Documents how to export the Client Map via the automation export job. |
| addOns/client/client.gradle.kts | Registers the new sub-extension and adds build dependency on the exim add-on. |
| addOns/client/CHANGELOG.md | Notes the new automation export support for the Client Map. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } else { | ||
| if (optionsType != null && !optionsType.supportsMessageExport()) { | ||
| String sourceName = | ||
| Constant.messages.getString( | ||
| "exim.exporter.source." + options.getSource().getId()); | ||
| result.addError( | ||
| Constant.messages.getString( | ||
| "exim.exporter.error.type.messages", sourceName)); | ||
| if (optionsType.supportsStandaloneExport()) { | ||
| optionsType.createForExport().export(writer); | ||
| } else { |
There was a problem hiding this comment.
Standalone exporters are only executed in the non-SITESTREE branch. If a standalone type is selected while source is SITESTREE, it will currently be treated as a Sites Tree export and fail the YAML-only validation. Consider checking supportsStandaloneExport() before the Source.SITESTREE special-case (or explicitly allowing standalone types in that branch) so that source can be safely ignored for standalone exports.
There was a problem hiding this comment.
YAML is the only valid option forSites Tree, so I'm fine with the code as is
b490843 to
64a2c35
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| The output is in the same YAML format as the manual Client Map export. The <code>context</code> parameter is not applicable and is ignored. | ||
| <pre> | ||
| - type: export # Exports the Client Map | ||
| parameters: | ||
| type: yaml # YAML is the only supported format for the Client Map | ||
| source: clientmap # Export the Client Map |
There was a problem hiding this comment.
The help text says the export job’s context parameter “is not applicable and is ignored” and that the output is the same YAML as the manual Client Map export, but the implementation passes the context through and the new writer path outputs a different (no-components) structure including url. Please update this documentation to match actual behavior (either document that context is supported for filtering and that the export omits components / differs from the manual export, or change the implementation to match what’s documented).
| The output is in the same YAML format as the manual Client Map export. The <code>context</code> parameter is not applicable and is ignored. | |
| <pre> | |
| - type: export # Exports the Client Map | |
| parameters: | |
| type: yaml # YAML is the only supported format for the Client Map | |
| source: clientmap # Export the Client Map | |
| The <code>context</code> parameter can be used to filter the exported data. The YAML produced by this job uses the export writer format and differs from the manual Client Map export, for example it includes <code>url</code> fields and omits <code>components</code>. | |
| <pre> | |
| - type: export # Exports the Client Map | |
| parameters: | |
| type: yaml # YAML is the only supported format for the Client Map | |
| source: clientmap # Export the Client Map | |
| context: # String: Optional context name used to filter the exported data |
| } else if (Source.CLIENTMAP.equals(options.getSource())) { | ||
| if (!YamlExporter.isYamlExporter(options.getType())) { | ||
| result.addError( | ||
| Constant.messages.getString( | ||
| "exim.exporter.error.type.messages", sourceName)); | ||
| "exim.exporter.error.type.clientmap", options.getType())); | ||
| } else { | ||
| SourceExporter sourceExporter = SOURCE_EXPORTERS.get(Source.CLIENTMAP); | ||
| if (sourceExporter != null) { | ||
| sourceExporter.export(writer, options); |
There was a problem hiding this comment.
registerSourceExporter(source, …) stores exporters keyed by the provided source, but the export path only ever looks up SOURCE_EXPORTERS.get(Source.CLIENTMAP) (hard-coded). This makes the public API misleading: registering for any other source is a no-op. Consider either (a) restricting registration to Source.CLIENTMAP (validate and reject others) or (b) generalizing the export implementation to delegate to SOURCE_EXPORTERS.get(options.getSource()) for any supported pluggable source before falling back to built-in handling.
| } else if (Source.CLIENTMAP.equals(this.parameters.getSource()) | ||
| && !YamlExporter.ID.equalsIgnoreCase(this.parameters.getType())) { | ||
| String typeId = | ||
| this.parameters.getType() != null | ||
| ? this.parameters.getType().toLowerCase(Locale.ROOT) | ||
| : HarExporter.ID; | ||
| progress.error( | ||
| Constant.messages.getString( | ||
| "exim.automation.export.error.clientmap.type", this.getName(), typeId)); |
There was a problem hiding this comment.
verifyParameters() adds a new invalid-combination check for source: clientmap with non-YAML types, but there’s no corresponding unit test (the existing ExportJobUnitTest already covers the analogous SitesTree and non-SitesTree/YAML cases). Please add a test that sets source: clientmap with a non-YAML type and asserts the expected progress error message.
64a2c35 to
ec0f089
Compare
cd08654 to
835be63
Compare
835be63 to
4c8bff1
Compare
Signed-off-by: Simon Bennetts <psiinon@gmail.com>
4c8bff1 to
7738753
Compare
|
Thank you! |

No description provided.