feat(core): add owasp zap containers test - #100
Open
RVANDO12 wants to merge 2 commits into
Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces on-demand Dynamic Application Security Testing (DAST) for IDP-Core using OWASP ZAP, implemented as a JUnit 5 security-tagged integration test executed via Testcontainers, with a companion GitHub Actions workflow and contributor documentation.
Changes:
- Adds
ZapSecurityIntegrationTest(@Tag("security")) to run ZAP’szap-api-scan.pyagainst the app’s/v3/api-docs. - Adds a GitHub Actions workflow to trigger the scan via
workflow_dispatchor a/run-owasp-zapPR comment. - Updates contributing docs to describe the security scanning strategy and usage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/test/java/com/decathlon/idp_core/security/ZapSecurityIntegrationTest.java |
Adds a Testcontainers-based OWASP ZAP scan integration test targeting /v3/api-docs. |
src/main/java/com/decathlon/idp_core/infrastructure/adapters/api/auth/mock/MockSecurityConfiguration.java |
Adjusts mock security filter chain (notably CSRF handling) used during the scan. |
pom.xml |
Attempts to exclude the security group (currently placed under the wrong plugin). |
docs/src/contributing/testing.md |
Expands testing strategy docs to include DAST security tests and links to the ZAP page. |
docs/src/contributing/owasp-zap-security.md |
Adds a new documentation page explaining OWASP ZAP scanning, usage, and interpretation. |
.github/workflows/zap_security_check.yml |
Adds an on-demand workflow to run the security-tagged tests and upload/report results. |
Comments suppressed due to low confidence (3)
.github/workflows/zap_security_check.yml:67
- The job posts a hard-coded findings summary (0/1/3) which can be misleading because it does not reflect the actual ZAP results. Either compute real counts from the generated report/logs or change the message to avoid publishing fake numbers.
message: |
### 🛡️ OWASP ZAP Scan Findings
- **High Risk:** 0
- **Medium Risk:** 1
- **Low Risk:** 3
📥 *Download the complete HTML report from the [Workflow Run Artifacts](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}).*
.github/workflows/zap_security_check.yml:53
- The workflow uploads
target/zap-reports/, butupload-artifactwill currently error/warn if the directory does not exist (and the new test currently does not generate files there). Consider settingif-no-files-foundexplicitly to avoid surprising workflow failures while keeping artifacts optional.
- name: Upload ZAP Report Artifact
if: always()
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
with:
name: zap-scan-report
path: target/zap-reports/
retention-days: 1
docs/src/contributing/owasp-zap-security.md:275
- The page ends without the standard
---+## Next Stepssection, and it links tocode/security.mdwhich does not exist in this docs tree. Update the link to an existing page and add a small Next Steps section so navigation remains consistent.
## Related Resources
- **[OWASP ZAP Documentation](https://www.zaproxy.org/docs/desktop/start/features/autoscan/)**
- **[OWASP Top 10](https://owasp.org/www-project-top-ten/)**
- **[Spring Security Best Practices](code/security.md)**
- **[CI/CD Workflows](ci-workflow.md)**
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (5)
.github/workflows/zap_security_report.yml:42
- The PR comment links to
${{ github.run_id }}, which is the run ID of this report workflow (it has no artifacts). Link to the triggering scan run instead (github.event.workflow_run.id) so the artifact URL is correct.
📥 *Download the HTML report from the [Workflow Run Artifacts](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}).*
.github/workflows/zap_security_report.yml:35
github.event.workflow_run.pull_requests != nullcan still be an empty array. Referencing[0].numberwill then fail. Guard against empty arrays before indexing.
if: github.event.workflow_run.event == 'issue_comment' && github.event.workflow_run.pull_requests != null
docs/src/contributing/owasp-zap-security.md:193
- This section describes the unprivileged workflow as posting PR comments, but the preceding "two-workflow" hardening section says PR commenting should be done from the privileged
workflow_runworkflow. Update this step description so the documentation matches the intended architecture (and the workflows).
5. Posts comment results on the PR
docs/src/contributing/owasp-zap-security.md:356
- This page links to
code/security.md, but that file doesn’t exist underdocs/src/, so the link will be broken. Also, docs pages indocs/src/are expected to end with a "Next Steps" section; consider adding it here to match the documentation layout used elsewhere.
- **[OWASP ZAP Documentation](https://www.zaproxy.org/docs/desktop/start/features/autoscan/)**
- **[OWASP Top 10](https://owasp.org/www-project-top-ten/)**
- **[Spring Security Best Practices](code/security.md)**
- **[CI/CD Workflows](ci-workflow.md)**
docs/src/contributing/testing.md:32
- This describes the scan as "Automated" on every PR, but the workflow is currently triggered manually or via an explicit
/run-owasp-zapcomment. Adjust the wording (and consider adding a "Next Steps" section at the end to match the docs page structure used elsewhere).
* **Scope**: API security scanning, vulnerability detection.
* **Tool**: OWASP ZAP via Testcontainers for Dynamic Application Security Testing.
* **Rule**: Automated scanning on every PR via `/run-owasp-zap` comment.
→ See **[OWASP ZAP Security Scanning](owasp-zap-security.md)** for complete details.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
.github/workflows/zap_security_check.yml:70
- The workflow explicitly does not grant
pull-requests: write, but it still tries to post a PR comment viamarocchino/sticky-pull-request-comment. This step will fail with insufficient permissions and also contradicts the intended two-workflow hardening model (commenting should be done only in the privilegedworkflow_runworkflow).
# Step E: Post Result Comment on the PR
- name: Comment PR with ZAP Findings
if: always() && github.event.issue.pull_request
uses: marocchino/sticky-pull-request-comment@5770ad5eb8f42dd2c4f34da00c94c5381e49af88 # v3.0.5
with:
docs/src/contributing/owasp-zap-security.md:100
- The documented ZAP exit code meanings don’t match the test behavior (
ZapSecurityIntegrationTestallows exit codes 0 and 1, but this page says 1 is a FAIL and 2 is a WARN). Align the documentation with the expected behavior so contributors interpret CI results correctly.
**Exit codes:**
- `0`: Success (no vulnerabilities found)
- `1`: At least one FAIL (critical vulnerability)
- `2`: At least one WARN (medium-risk vulnerability)
docs/src/contributing/owasp-zap-security.md:231
- The "Reference" link formatting is broken (nested bold markers), which renders incorrectly in Markdown and may trip markdownlint/vale rules.
**Reference:
** [GitHub Actions Security Hardening Guide](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injection)
RVANDO12
force-pushed
the
feat/seycurity/owasp-zap
branch
from
July 24, 2026 06:59
9cd51e7 to
374622d
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (3)
docs/src/contributing/owasp-zap-security.md:134
- This step says a scan results summary is posted to the PR, but the reporting workflow currently posts only a link to the workflow run/artifacts. Please align the doc with the implemented behavior (or update the workflow to actually summarize findings).
6. **PR Comment**: Scan results summary posted on the PR
docs/src/contributing/owasp-zap-security.md:195
- The unprivileged workflow description says it posts PR comment results, but zap_security_check.yml intentionally does not have pull-requests: write and does not post comments. This should describe artifact upload only, with commenting handled by zap_security_report.yml.
1. Checks out PR branch code
2. Builds and starts the application
3. Runs OWASP ZAP security tests
4. Uploads artifacts
5. Posts comment results on the PR
**Security benefit:** Untrusted code cannot access elevated permissions to modify repository or secrets.
.github/workflows/zap_security_report.yml:45
- PR description and docs describe posting a findings summary comment, but this workflow currently posts only a link to the workflow run. If a findings summary is required, it likely needs an additional machine-readable report (for example JSON) produced by the scan and parsed here.
message: |
### 🛡️ OWASP ZAP Scan Report
*Download the HTML report from the [Workflow Run Artifacts](${{ github.event.workflow_run.html_url }}).*
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (3)
pom.xml:339
- The Surefire configuration uses
<groups>/<excludedGroups>, but the new security test is tagged with JUnit 5@Tag("security"). Surefire’s group filtering does not apply to JUnit 5 tags, so this will not reliably exclude the ZAP test from the default build (and it may start running in normal CI, requiring Docker). Configure JUnit Platform tag include/exclude viaconfigurationParametersinstead.
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<groups>${test.groups}</groups>
<excludedGroups>${test.excludedGroups}</excludedGroups>
</configuration>
docs/src/contributing/owasp-zap-security.md:193
zap_security_check.ymlintentionally does not post PR comments (it has nopull-requests: writepermission). The PR comment is posted byzap_security_report.ymlafter downloading artifacts. Update this step to match the actual workflow behavior.
5. Posts comment results on the PR
docs/src/contributing/owasp-zap-security.md:143
- The example PR comment output shows risk counts (High/Medium/Low), but the implemented workflow comment only links to the artifacts and does not extract findings from the report. Please align the example with what
zap_security_report.ymlactually posts (or extend the workflow to generate a summary).
### 🛡️ OWASP ZAP Scan Findings
- **High Risk:** 0
- **Medium Risk:** 1
- **Low Risk:** 3
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (5)
.github/workflows/zap_security_report.yml:24
- The comment says this job only posts results when the scan completed successfully, but the condition currently allows any conclusion except "skipped" (including failures). Update the comment to match the actual behavior to avoid confusion during incident/debugging.
# Only post results if the scan workflow completed successfully
if: github.event.workflow_run.conclusion != 'skipped'
.github/workflows/zap_security_report.yml:44
- The PR description says the workflow posts a findings summary comment, but this workflow currently only posts a link to the run artifacts (no parsing/summarization of ZAP output). Either add a step to extract counts/severity from the report and include them in the comment, or adjust the PR description/docs to match the implemented behavior.
message: |
### 🛡️ OWASP ZAP Scan Report
*Download the HTML report from the [Workflow Run Artifacts](${{ github.event.workflow_run.html_url }}).*
docs/src/contributing/owasp-zap-security.md:134
- This step claims a scan findings summary is posted on the PR, but the current report workflow only posts a link to the run artifacts. Update this line to match the current behavior (or implement the summary generation).
6. **PR Comment**: Scan results summary posted on the PR
docs/src/contributing/owasp-zap-security.md:143
- The example PR comment shows per-severity counts, but the workflow currently posts only a header and a link. Update the example so readers don't expect a summary that isn't produced.
- **High Risk:** 0
- **Medium Risk:** 1
- **Low Risk:** 3
docs/src/contributing/owasp-zap-security.md:355
- The horizontal rule has a leading space, which can prevent Markdown from rendering it as a rule and may violate markdownlint rules. Use an unindented
---.
---
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
docs/src/contributing/owasp-zap-security.md:146
- The "Example PR Comment Output" shows risk counts, but the current
zap_security_report.ymlworkflow only posts a link to the workflow run artifacts. Update the example so it matches what the automation actually posts (or implement parsing + summary posting in the workflow).
### Example PR Comment Output
```markdown
### 🛡️ OWASP ZAP Scan Findings
- **High Risk:** 0
- **Medium Risk:** 1
- **Low Risk:** 3
📥 Download the complete HTML report from the [Workflow Run Artifacts](https://github.com/...).
</details>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
.github/workflows/zap_security_report.yml:36
- The PR description and docs describe posting a findings summary (High/Medium/Low counts) as a PR comment, but this workflow currently posts only a static message with a link to the workflow run. Either implement artifact download + report parsing to include a summary, or update the docs/expectations to match the current behavior.
- name: Comment PR with ZAP Findings
if: github.event.workflow_run.event == 'issue_comment' && github.event.workflow_run.pull_requests[0] != null
uses: marocchino/sticky-pull-request-comment@5770ad5eb8f42dd2c4f34da00c94c5381e49af88 # v3.0.5
with:
header: zap-scan-results
number: ${{ github.event.workflow_run.pull_requests[0].number }}
message: |
### 🛡️ OWASP ZAP Scan Report
*Download the HTML report from the [Workflow Run Artifacts](${{ github.event.workflow_run.html_url }}).*
docs/src/contributing/owasp-zap-security.md:146
- This example PR comment output doesn’t match what
zap_security_report.ymlactually posts (it currently posts only a header + link, without severity counts). Update the example so it reflects the current workflow output, or implement summary generation in the workflow.
### Example PR Comment Output
```markdown
### 🛡️ OWASP ZAP Scan Findings
- **High Risk:** 0
- **Medium Risk:** 1
- **Low Risk:** 3
📥 Download the complete HTML report from the [Workflow Run Artifacts](https://github.com/...).
</details>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
docs/src/contributing/owasp-zap-security.md:142
- The docs show a PR comment that includes High/Medium/Low counts, but the implemented
zap_security_report.ymlworkflow currently posts only a link to the workflow run artifacts (no findings summary). Either update this example/output description to match the current behavior, or extend the workflow to parse the report and include counts.
```markdown
### 🛡️ OWASP ZAP Scan Findings
- **High Risk:** 0
- **Medium Risk:** 1
.github/workflows/zap_security_report.yml:36
- This workflow posts only a static message with a link to the run page. The PR description/documentation claims the workflow posts a findings summary (counts by severity), but there’s no step here that downloads/parses the uploaded report artifact to generate that summary. Consider either (1) enhancing this workflow to download
zap-scan-reportand extract counts for the PR comment, or (2) updating the PR description/docs to clarify that only a report link is posted.
message: |
### 🛡️ OWASP ZAP Scan Report
*Download the HTML report from the [Workflow Run Artifacts](${{ github.event.workflow_run.html_url }}).*
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
docs/src/contributing/owasp-zap-security.md:346
- This command will not run the ZAP test with the current Surefire defaults because
pom.xmlexcludes thesecuritytag by default (test.excludedGroups=security). Even if-Dtest=ZapSecurityIntegrationTestselects the class, Surefire will still filter it out unless you also cleartest.excludedGroups(and/or include thesecuritytag).
- **Run Locally Before PR**: Execute `mvn test -Dtest=ZapSecurityIntegrationTest` locally to catch issues early
- **Review Reports Carefully**: Not all findings are actionable; prioritize high/medium severity
- **Keep Dependencies Updated**: Regularly update ZAP container and security libraries
- **Integrate with CI/CD**: Make ZAP scans a required check before merge
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pom.xml:36
- The PR description’s “How to test” section uses
mvn test -Dgroups=security, but the build is wired totest.groups/test.excludedGroups. As-is, someone following the PR description won’t run the intended tagged tests; update the PR description to use-Dtest.groups=security -Dtest.excludedGroups=(or adjust the build to acceptgroupsas an alias).
<test.groups/>
<test.excludedGroups>security</test.excludedGroups>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
.github/workflows/zap_security_report.yml:56
- Even with the parsing fix, it’s still possible for
pr_numberto be empty (for example, if the check workflow failed before saving it). In that case, this step should be skipped to avoid failing the whole report workflow.
- name: Comment PR with ZAP Findings
if: github.event.workflow_run.event == 'issue_comment'
uses: marocchino/sticky-pull-request-comment@5770ad5eb8f42dd2c4f34da00c94c5381e49af88 # v3.0.5
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
.github/workflows/zap_security_report.yml:56
pr_numberis already read conditionally earlier in this script, but it is read again unconditionally here. If the artifact is missingpr_number.txtthis will fail the job even though the workflow already handles missing values.
pr_number=$(cat zap-reports/pr_number.txt)
feat(core): update code without break process
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (3)
pom.xml:339
maven-surefire-pluginis configured with<groups>/<excludedGroups>, but this project runs on JUnit 5 (@Tag("security")). Surefire’sgroups/excludedGroupsfiltering does not apply to JUnit 5 tags, so the security test may run in the defaultmvn testsuite (and-Dtest.excludedGroups=securitymay not actually exclude it). Configure tag filtering via JUnit Platform tag parameters instead sosecurityis excluded by default.
<configuration>
<groups>${test.groups}</groups>
<excludedGroups>${test.excludedGroups}</excludedGroups>
</configuration>
.github/workflows/zap_security_report.yml:55
- This unguarded
cat zap-reports/pr_number.txtwill fail the step if the artifact is missingpr_number.txt(for example, if the upstream workflow exits early before saving it or the download step didn’t fetch the expected artifact). The file is already read above when present, so keep this line from causing a hard failure.
pr_number=$(cat zap-reports/pr_number.txt)
.github/workflows/zap_security_report.yml:66
- If
pr_numberis empty (missing artifact / parse failure), this step will still run and the action will fail becausenumberis not a valid PR number. Guard the step so it only runs when a PR number was successfully extracted.
if: github.event.workflow_run.event == 'issue_comment'
RVANDO12
force-pushed
the
feat/seycurity/owasp-zap
branch
from
July 24, 2026 12:25
f82f24b to
7fff886
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
.github/workflows/zap_security_report.yml:56
- The parse step reads
pr_numberconditionally (when the file exists) but then unconditionally re-reads it withcat zap-reports/pr_number.txt. If the artifact is missingpr_number.txt(e.g., scan ran viaworkflow_dispatchor artifact upload failed), thiscatwill fail the step and prevent posting the report even whenzap-report.jsonexists.
high=$(jq '[.site[].alerts[]? | select(.riskcode == "3" or .riskcode == 3)] | length' zap-reports/zap-report.json)
medium=$(jq '[.site[].alerts[]? | select(.riskcode == "2" or .riskcode == 2)] | length' zap-reports/zap-report.json)
low=$(jq '[.site[].alerts[]? | select(.riskcode == "1" or .riskcode == 1)] | length' zap-reports/zap-report.json)
pr_number=$(cat zap-reports/pr_number.txt)
.github/workflows/zap_security_report.yml:70
- This step will run even when
steps.zap-summary.outputs.pr_numberis empty, which can cause the sticky comment action to fail (and makes the workflow noisy) when the PR number artifact is missing. Gate the comment step on a non-empty PR number.
- name: Comment PR with ZAP Findings
if: github.event.workflow_run.event == 'issue_comment'
uses: marocchino/sticky-pull-request-comment@5770ad5eb8f42dd2c4f34da00c94c5381e49af88 # v3.0.5
with: # FIX: Indented the following keys properly
header: zap-scan-results
number: ${{ steps.zap-summary.outputs.pr_number }}
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Description
What this PR Provides
Adds dynamic security scanning (DAST) support using OWASP ZAP via Testcontainers.
Introduces the
ZapSecurityIntegrationTestclass tagged with@Tag("security")to perform automated API security scans against the OpenAPI spec (/v3/api-docs).Adds a GitHub Actions workflow (
zap.yml) allowing manual triggers (workflow_dispatch) or PR comment triggers (/run-owasp-zap).Includes documentation detailing how OWASP ZAP integration works, local execution instructions, report interpretation, and testing strategy updates.
Fixes
N/A
Review
The reviewer must double-check these points:
How to test
Initial State
Docker daemon must be running.
Java 25 and Maven 3.8.0+ installed.
What and How to Test
Option 1: Local Test Execution
mvn test -Dgroups=securityzaproxy/zap-stable:2.17.0, and the scan runs against[http://host.testcontainers.internal](http://host.testcontainers.internal):<port>/v3/api-docs.Option 2: PR Comment Trigger
Open a pull request.
Comment
/run-owasp-zapon the PR.Verify that the
OWASP ZAP Security Scanworkflow is triggered, runsmvn test -Dgroups=security, posts the findings summary comment, and uploads the artifact report.Expected Results
Security tests complete with exit code 0 or 1.
Standard output logs from OWASP ZAP are captured in the test output/CI logs.
Reports are generated in
target/zap-reports/and uploaded as workflow artifacts.Breaking Changes (if any)