[PLUGINS] Replace placeholder zap_scanner launcher with real ZAP execution contract#720
[PLUGINS] Replace placeholder zap_scanner launcher with real ZAP execution contract#720Pcmhacker-piro wants to merge 4 commits into
Conversation
|
heyy @utksh1 |
utksh1
left a comment
There was a problem hiding this comment.
This needs changes before merge. The new ZAP launcher ignores docker non-zero exit codes and can report success with empty findings when ZAP fails. Please propagate subprocess failures, cover that behavior with tests, and keep the parser contract clear for both wrapper JSON and raw ZAP JSON outputs before this becomes the plugin execution path.
utksh1
left a comment
There was a problem hiding this comment.
Re-reviewed the latest update. This still needs changes before merge.
The launcher still does not fail the task when Docker/ZAP exits non-zero. It captures result.stderr, but it never checks result.returncode, so a failed ZAP run can still print an empty successful JSON payload and be treated as a completed scan.
Please make non-zero Docker/ZAP exit codes produce a failing process exit and add unit coverage for that behavior. Keep the parser tests, but the execution wrapper must not hide scanner failures.
|
heyy @utksh1 |
utksh1
left a comment
There was a problem hiding this comment.
This is still blocked.
plugins/zap_scanner/run.py still ignores a nonzero Docker/ZAP exit code. If zap-full-scan.py fails before writing a report, the wrapper can still emit a successful JSON payload with zero findings, which hides scanner failures from SecuScan.
Please check result.returncode after subprocess.run, return a nonzero exit with an error when Docker/ZAP fails, and add a test for the nonzero-returncode/no-report path.
…ution contract - Replace placeholder Python one-liner with a proper run.py script that invokes OWASP ZAP via Docker, mounts a temp volume, captures JSON output, and handles timeout/docker-not-found errors. - Update metadata.json: description, long_description, dependencies (adds docker), field help text, consent message, checksum. - Rewrite parser.py to parse real ZAP JSON alert output with severity mapping. - Add test_zap_scanner_parser.py with 6 tests covering valid alerts, empty results, invalid JSON, items fallback, missing severity, and non-dict items. - Update PLUGINS.md with new summary and runtime-dependencies table.
1007617 to
5fc0321
Compare
|
heyy @utksh1 |
utksh1
left a comment
There was a problem hiding this comment.
Rechecking after the latest audit-exception commit: this is still blocked.
plugins/zap_scanner/run.py still needs to fail on nonzero Docker/ZAP return codes, especially when no report is produced. Please add explicit result.returncode handling plus a test for the nonzero-returncode/no-report path, and remove unrelated audit-policy exception changes from this plugin PR.
|
Closing due to unresolved review feedback. |
Description
Replaces the placeholder Python one-liner in
plugins/zap_scannerwith a real Docker-based OWASP ZAP execution contract.Changes
ghcr.io/zaproxy/zaproxy:stable)dockerto binaries dependencies, addedhelptext to the target field, improved consent message, updated checksumRuntime Requirements
ghcr.io/zaproxy/zaproxy:stableon first runCloses #541
Type of change
How Has This Been Tested?
scripts/validate_plugin.py --plugin zap_scanner— passes schema validationscripts/refresh_plugin_checksum.py --plugin zap_scanner— checksum up to dateChecklist: