Extend ZAP scan to provider users and state API M2M (#1467, #1468)#1470
Open
jlkravitz wants to merge 19 commits into
Open
Extend ZAP scan to provider users and state API M2M (#1467, #1468)#1470jlkravitz wants to merge 19 commits into
jlkravitz wants to merge 19 commits into
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 47 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds multi-token authentication to ZAP scans (staff, provider, state M2M): CI and local scripts now obtain three tokens, pass them into the scanner, route requests to the appropriate token by hostname/path, add optional ECDSA request signing for state API requests, extend OpenAPI enrichment, and update workflows, scripts, and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions
participant Staff as Staff Cognito
participant Provider as Provider Cognito
participant M2M as State Auth (OAuth2)
participant ZAP as ZAP Scanner
participant API as Backend APIs
GHA->>Staff: Request staff auth (username/password)
Staff-->>GHA: Return staff accessToken
GHA->>Provider: Request provider auth (username/password)
Provider-->>GHA: Return provider idToken
GHA->>M2M: Request M2M token (client_credentials)
M2M-->>GHA: Return state access_token
GHA->>ZAP: Start scan with tokens (staff, provider, state)
ZAP->>API: Request to state-api.test... (uses state token + optional ECDSA headers)
API-->>ZAP: Response
ZAP->>API: Request to /v1/provider-users/* (uses provider token)
API-->>ZAP: Response
ZAP->>API: Other endpoints (uses staff token)
API-->>ZAP: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jlkravitz
force-pushed
the
feat/zap-provider-state-scan
branch
from
9a39ff6 to
d1acb19
Compare
Route bearer tokens by URL in bearer-token.js: state-api host gets the M2M token, /v1/provider-users/* + /v1/purchases/* + GET attestations-by-id get the provider token, everything else gets the staff token. Workflow runs three auth steps and exposes each token as a distinct env var. main.js takes --mode=staff|provider so the same script handles both user-pool sign-ins from a single unified .env. Requires new GitHub secrets for provider user creds and state auth M2M client; see owasp-zap/README.md for provisioning. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jlkravitz
force-pushed
the
feat/zap-provider-state-scan
branch
from
d1acb19 to
80ac360
Compare
Renames TEST_STATE_AUTH_CLIENT_ID/_SECRET/_SCOPES to TEST_ZAP_STATE_AUTH_* for clarity (these are ZAP-specific, not general test credentials). Also pins the staff userId in enrich-oas-for-zap.py to a dedicated test user to avoid ZAP mutating the account it's authenticating with. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the post-creation update-user-pool-client patch (which is a full-replacement API and easy to get wrong) with a one-shot temporary edit to BASE_CLIENT_CONFIG in create_app_client.py. Also clarifies the full scope list to enter at the prompts so future rotations don't miss aslp/readGeneral. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lets us observe the scan end-to-end from GitHub's UI before merge. Revert this commit before the branch lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
owasp-zap/authenticator/main.js (1)
8-14: Consider validatingmodeagainst the allowed set.Any value (e.g.,
--mode=stafff) silently becomes a prefix and surfaces only via the generic "Missing environment variables" message on line 22, which is confusing. A small whitelist check would produce a clearer error and fail fast.♻️ Proposed fix
-const modeArg = process.argv.find((arg) => arg.startsWith('--mode=')); -const mode = modeArg ? modeArg.split('=')[1] : ''; -if (!mode) { - console.error('Missing --mode=staff|provider'); - process.exit(1); -} -const prefix = `${mode.toUpperCase()}_`; +const ALLOWED_MODES = ['staff', 'provider']; +const modeArg = process.argv.find((arg) => arg.startsWith('--mode=')); +const mode = modeArg ? modeArg.split('=')[1] : ''; +if (!ALLOWED_MODES.includes(mode)) { + console.error(`Missing or invalid --mode. Expected one of: ${ALLOWED_MODES.join('|')}`); + process.exit(1); +} +const prefix = `${mode.toUpperCase()}_`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owasp-zap/authenticator/main.js` around lines 8 - 14, Validate the parsed mode value against an explicit whitelist instead of accepting any non-empty string: after computing mode (from modeArg) check it is one of the allowed values (e.g., 'staff' or 'provider'); if not, log a clear error like "Invalid --mode value: must be 'staff' or 'provider'" and exit with process.exit(1). Update the logic around modeArg/mode/prefix to perform this whitelist check before computing prefix and before any environment-variable checks so invalid modes fail fast and produce a helpful message.owasp-zap/README.md (1)
51-63: Consider replacing the “temporary source edit” provisioning step with a script argument.As written, this is workable but error-prone (missed revert / accidental commit). A small enhancement to
create_app_client.py(e.g.,--access-token-validity 60) would remove the risky manual edit/revert path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owasp-zap/README.md` around lines 51 - 63, Add a CLI flag to backend/compact-connect/app_clients/bin/create_app_client.py to avoid manual edits: add an argument like --access-token-validity (default 15) in the argument parser, use its value to override BASE_CLIENT_CONFIG['AccessTokenValidity'] before client creation, and ensure any validation/coercion to an int is applied; update the script usage/help and tests if present so you can run the one-shot `--access-token-validity 60` flow instead of editing the source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/zap-scan-test.yml:
- Around line 4-5: Temporary pull_request trigger is present in the workflow
(the pull_request: key) and must be removed before merge; remove the
pull_request: line from the YAML (or replace it with a safer trigger such as
workflow_dispatch: for manual runs or a path/label-gated pull_request variant)
to avoid running expensive ZAP scans on external-fork PRs (which lack secrets)
and the live test environment; if you intend to keep automated PR runs,
implement path filters or a label check (e.g., add paths: or a label-based if
condition) instead of the unconditional pull_request trigger.
In `@owasp-zap/authenticator/.env.example`:
- Line 20: The STATE_AUTH_SCOPES line in .env.example contains unquoted
space-separated values which breaks sourcing (manual-scan.sh uses set -a; .
"$env_file"; set +a and set -e), so update the STATE_AUTH_SCOPES entry to be a
single quoted string (e.g., wrap the entire scope list in double quotes) so
sourcing sets the full value instead of treating subsequent tokens as commands;
modify the .env.example STATE_AUTH_SCOPES line accordingly and verify
manual-scan.sh still reads it correctly.
In `@owasp-zap/authenticator/get-m2m-token.sh`:
- Around line 19-30: The curl call that populates the response variable can
abort the script under set -e, preventing the script from printing the HTTP
body; modify the curl invocation so it cannot trigger an immediate exit (e.g.,
append "|| true" or run curl and capture its exit status into a variable like
curl_rc), then check curl_rc and/or the extracted token variable (token) and if
curl_rc is non-zero or token is empty, print the full response and exit with a
non-zero code; update the block that sets response and token to first capture
response and curl exit status, then use that status and the token check to
surface the response body on errors.
In `@owasp-zap/manual-scan.sh`:
- Around line 24-50: fetch_user_token can yield the literal string "null"
because jq -r '.accessToken' outputs "null" when the key is missing; update
fetch_user_token (and token assignment logic for STAFF_TOKEN and PROVIDER_TOKEN)
to treat "null" as empty by using a jq expression that returns empty when
.accessToken is null (or explicitly post-process the output and set tokens to
empty if they equal "null"), so that STAFF_TOKEN/PROVIDER_TOKEN/STATE_TOKEN are
empty rather than the string "null" and the subsequent -z check correctly aborts
when no real tokens exist.
In `@owasp-zap/README.md`:
- Around line 30-35: Update the README fenced code blocks to include language
identifiers: add "text" to blocks listing environment variable names (e.g., the
blocks containing TEST_COGNITO_USER_POOL_ID_STAFF, TEST_ZAP_PASSWORD_PROVIDER,
and TEST_COGNITO_STATE_AUTH_DOMAIN) and add "bash" to the block containing the
python3 backend/compact-connect/app_clients/bin/create_app_client.py command;
ensure every triple-backtick fence in the file (including the ones around the
lines noted at 39-44, 56-58, and 66-71) has the appropriate language label.
---
Nitpick comments:
In `@owasp-zap/authenticator/main.js`:
- Around line 8-14: Validate the parsed mode value against an explicit whitelist
instead of accepting any non-empty string: after computing mode (from modeArg)
check it is one of the allowed values (e.g., 'staff' or 'provider'); if not, log
a clear error like "Invalid --mode value: must be 'staff' or 'provider'" and
exit with process.exit(1). Update the logic around modeArg/mode/prefix to
perform this whitelist check before computing prefix and before any
environment-variable checks so invalid modes fail fast and produce a helpful
message.
In `@owasp-zap/README.md`:
- Around line 51-63: Add a CLI flag to
backend/compact-connect/app_clients/bin/create_app_client.py to avoid manual
edits: add an argument like --access-token-validity (default 15) in the argument
parser, use its value to override BASE_CLIENT_CONFIG['AccessTokenValidity']
before client creation, and ensure any validation/coercion to an int is applied;
update the script usage/help and tests if present so you can run the one-shot
`--access-token-validity 60` flow instead of editing the source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0df9ff2d-a26b-4fb9-b253-0ec93d850bd9
📒 Files selected for processing (9)
.github/workflows/zap-scan-test.ymlowasp-zap/README.mdowasp-zap/authenticator/.env.exampleowasp-zap/authenticator/get-m2m-token.showasp-zap/authenticator/main.jsowasp-zap/data/bearer-token.jsowasp-zap/data/test-automation.ymlowasp-zap/enrich-oas-for-zap.pyowasp-zap/manual-scan.sh
Bumps zaproxy/action-af from v0.1.0 to v0.2.0, which adds a docker_env_vars input for forwarding env vars into the ZAP docker container. Without this, the three new ZAP_AUTH_*_TOKEN vars set in $GITHUB_ENV never reached the container, so bearer-token.js got null from System.getenv and every authenticated request returned 401. The staff-only predecessor worked by piggybacking on ZAP_AUTH_HEADER_VALUE, which action-af hardcoded to forward; the multi-auth rewrite renamed the vars without a corresponding passthrough mechanism. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Provider Lambda handlers extract claims (email/cognito:username) that only exist on the ID token; the access token was getting rejected by the authorizer/handlers, so every authenticated provider-endpoint request came back 401. Matches the pattern in backend/compact-connect/tests/smoke/smoke_common.py:174. Staff endpoints continue to use the access token. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ZAP's active scan was hitting these read-only POST /providers/query endpoints (staff, public, state) with invalid bodies and getting 400s on the baseline request — so fuzzing never saw the stored-search path. Adding a minimal valid example per endpoint gives ZAP a 2xx baseline to fuzz around, exercising the query/pagination handling. The three endpoints account for ~2.5k of the 400s in the last run. Only read-only endpoints are listed; mutation endpoints like POST /licenses would flood the test DB with junk records. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The handler rejects date ranges wider than 7 days (lambdas/python/common/cc_common/data_model/schema/provider/api.py:331-332). My initial 2024-01-01 → 2025-01-01 example was 365 days, producing 400 on every baseline request. A 7-day window in April 2026 stays inside the limit so ZAP gets a real 2xx baseline to fuzz around. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
owasp-zap/enrich-oas-for-zap.py (1)
124-143: Report newly inserted examples instead of all examples present.This post-enrichment scan counts pre-existing examples too, so
Enriched {param_count}...can overstate what the script changed. Consider incrementing counters at the insertion points on Lines 78 and 88, then returning them fromenrich_spec().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owasp-zap/enrich-oas-for-zap.py` around lines 124 - 143, The post-scan counts currently tally all examples in spec instead of only newly inserted ones; modify enrich_spec() to maintain param_count and body_count counters that are incremented exactly at the example insertion sites (the places inside enrich_spec() where you add path parameter examples and requestBody/application/json examples), return those two counts from enrich_spec(), and update the final print to use the returned param_count and body_count so the message reports only examples your script inserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@owasp-zap/enrich-oas-for-zap.py`:
- Around line 80-88: The enrichment currently adds json_content["example"] when
a body_example exists but only checks for the singular "example"; update the
guard in the block that computes body_example (using BODY_EXAMPLES and
json_content derived from operation.get("requestBody", {}).get("content",
{}).get("application/json")) to skip adding anything if either "example" or
"examples" already exists on json_content and ensure json_content is a mapping
before checking keys; only assign json_content["example"] = body_example when
json_content is a dict-like object and neither "example" nor "examples" are
present.
---
Nitpick comments:
In `@owasp-zap/enrich-oas-for-zap.py`:
- Around line 124-143: The post-scan counts currently tally all examples in spec
instead of only newly inserted ones; modify enrich_spec() to maintain
param_count and body_count counters that are incremented exactly at the example
insertion sites (the places inside enrich_spec() where you add path parameter
examples and requestBody/application/json examples), return those two counts
from enrich_spec(), and update the final print to use the returned param_count
and body_count so the message reports only examples your script inserted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 918d0f96-5fb0-4694-8e37-f168b44c2c01
📒 Files selected for processing (1)
owasp-zap/enrich-oas-for-zap.py
Revert before merge. Investigating why the body example we added for state API isn't producing 2xx baselines like the public/staff equivalents do. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
owasp-zap/data/bearer-token.js (1)
18-22: Tokens are snapshotted at script load — consider resolving per-request.
TOKENSis populated once when ZAP loads the httpsender script. If the script is loaded in a context where an env var is not yet set (or the process env differs from what the workflow exports), the relevant kind will be permanentlyundefinedand every matching request will fall through to theNo <kind> token availablebranch with no Authorization header. ReadingSystem.getenv(...)lazily insidesendingRequest(or at least on first use per kind) makes the error surface at the first request and is more robust to env-timing differences between localmanual-scan.shand the workflow. Optional if you've verified env is always present at load time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owasp-zap/data/bearer-token.js` around lines 18 - 22, The TOKENS object is populated at module load using System.getenv, which can snapshot missing env vars; change token resolution to be lazy by reading System.getenv inside sendingRequest (or on first use per kind) instead of at module scope: replace direct TOKENS lookups with a function or getter invoked from sendingRequest (referencing TOKENS, sendingRequest and System.getenv) that fetches System.getenv('ZAP_AUTH_*_TOKEN') when handling a request so the Authorization header uses the current env value and the code logs the real-time absence of a token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@owasp-zap/data/bearer-token.js`:
- Around line 56-63: Remove the temporary diagnostic block that prints truncated
request/response bodies for state-api /providers/query 400s: delete the if block
that checks statusCode === 400 and uri host/path (which uses variables uri,
statusCode, path and msg and calls print), or wrap it behind an explicit opt-in
flag (e.g., read an env var like ENABLE_STATE_QUERY_DIAG) so the prints won't
run in CI/main; update any related comment TODOs in bearer-token.js accordingly.
- Around line 25-32: The classifyRequest function incorrectly assigns provider
tokens for all HTTP methods on the attestations path because it only checks
path; update classifyRequest to accept the HTTP method (add a method parameter)
and only return 'provider' for the PROVIDER_ATTESTATION_PATH when method ===
'GET'; otherwise fall through to return 'staff' (or allow other provider checks
like PROVIDER_PATH_PREFIX to remain unchanged). Change all call sites that
invoke classifyRequest(host, path) to pass the request method (e.g.,
classifyRequest(host, path, method)) and ensure any tests/mock callers are
updated accordingly.
---
Nitpick comments:
In `@owasp-zap/data/bearer-token.js`:
- Around line 18-22: The TOKENS object is populated at module load using
System.getenv, which can snapshot missing env vars; change token resolution to
be lazy by reading System.getenv inside sendingRequest (or on first use per
kind) instead of at module scope: replace direct TOKENS lookups with a function
or getter invoked from sendingRequest (referencing TOKENS, sendingRequest and
System.getenv) that fetches System.getenv('ZAP_AUTH_*_TOKEN') when handling a
request so the Authorization header uses the current env value and the code logs
the real-time absence of a token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe83ca18-9a73-4336-a076-148213c22a1e
📒 Files selected for processing (1)
owasp-zap/data/bearer-token.js
Refining earlier debug — previous filter caught path-traversal-fuzzed URLs that CloudFront rejected with HTML 400s. Narrowing to the exact clean URL with Content-Type + body + response, to see whether ZAP ever sends the baseline example at all. Revert before merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Skip injecting example when application/json already has examples (plural) or isn't a mapping. Addresses CodeRabbit review feedback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
State-api /providers/query and /providers/{id} require an ECDSA-SHA256
signature (X-Key-Id + X-Signature headers) in addition to the M2M
bearer token. Without it they 401 at the signature-validation layer
with "Missing required X-Key-Id header", leaving ZAP unable to probe
the PII-heavy handler code paths these endpoints expose.
Adds request signing in bearer-token.js:
- Loads a PKCS#8 PEM from ZAP_STATE_SIGNATURE_PRIVATE_KEY
- For every state-api request, attaches X-Algorithm / X-Timestamp /
X-Nonce / X-Key-Id / X-Signature per the canonical string format in
backend/compact-connect/docs/client_signature_auth.md
- Signing is skipped when the env vars aren't set, so existing test
environments continue to work (with the same signature-gated coverage
gap they had before).
Wires the two new secrets through the workflow's action-af step via
docker_env_vars, mirrors the same env vars in manual-scan.sh, and
documents the full key-provisioning flow (openssl keypair →
manage_signature_keys.py registration → GitHub secret) as a new step 5
in owasp-zap/README.md.
Also removes the TEMP debug logging that was added to diagnose why
state-api /providers/query was 401ing; this change is the fix.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
owasp-zap/authenticator/.env.example (1)
20-20:⚠️ Potential issue | 🟠 MajorQuote the multi-scope value before users copy this into
.env.Line 20 is sourced by
manual-scan.sh; unquoted spaces make Bash treatky/aslp.writeas a command instead of part ofSTATE_AUTH_SCOPES.🛡️ Suggested fix
-STATE_AUTH_SCOPES=aslp/readGeneral ky/aslp.write oh/aslp.write +STATE_AUTH_SCOPES="aslp/readGeneral ky/aslp.write oh/aslp.write"Verification:
#!/bin/bash set -euo pipefail tmp="$(mktemp)" cat > "$tmp" <<'EOF' STATE_AUTH_SCOPES=aslp/readGeneral ky/aslp.write oh/aslp.write EOF # Expected: this fails by attempting to execute `ky/aslp.write`. set -a . "$tmp" set +a🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owasp-zap/authenticator/.env.example` at line 20, The STATE_AUTH_SCOPES value in .env.example contains unquoted spaces which cause Bash to treat subsequent tokens as commands when the file is sourced (used by manual-scan.sh); fix by wrapping the entire scope string in quotes (e.g., STATE_AUTH_SCOPES="aslp/readGeneral ky/aslp.write oh/aslp.write") or escape the spaces so the whole value is assigned to STATE_AUTH_SCOPES when sourced.owasp-zap/README.md (1)
30-35:⚠️ Potential issue | 🟡 MinorAdd language identifiers to the remaining fenced code blocks.
These fences still trigger markdownlint MD040. Use
textfor secret/env-var lists andbashfor commands.📝 Suggested doc patch
- ``` + ```text TEST_COGNITO_USER_POOL_ID_STAFF TEST_WEBROOT_COGNITO_CLIENT_ID_STAFF TEST_ZAP_USERNAME_STAFF TEST_ZAP_PASSWORD_STAFF @@ - ``` + ```text TEST_COGNITO_USER_POOL_ID_PROVIDER TEST_COGNITO_CLIENT_ID_PROVIDER TEST_ZAP_USERNAME_PROVIDER TEST_ZAP_PASSWORD_PROVIDER @@ - ``` + ```bash python3 backend/compact-connect/app_clients/bin/create_app_client.py -u <state-auth-pool-id> @@ - ``` + ```text TEST_COGNITO_STATE_AUTH_DOMAIN (e.g. compact-connect-state-auth-test.auth.us-east-1.amazoncognito.com) TEST_ZAP_STATE_AUTH_CLIENT_ID TEST_ZAP_STATE_AUTH_CLIENT_SECRET TEST_ZAP_STATE_AUTH_SCOPES (space-separated, e.g. "aslp/readGeneral ky/aslp.write oh/aslp.write") @@ - ``` + ```text TEST_ZAP_STATE_SIGNATURE_KEY_ID (e.g. zap-test-v1) TEST_ZAP_STATE_SIGNATURE_PRIVATE_KEY (paste the full PKCS#8 PEM contents, including the BEGIN/END lines)Also applies to: 39-44, 56-58, 66-71, 98-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owasp-zap/README.md` around lines 30 - 35, Update the README fenced code blocks to include language identifiers so markdownlint MD040 is satisfied: add ```text to all environment/secret lists (e.g., the blocks containing TEST_COGNITO_* and TEST_ZAP_* variables and the block listing TEST_ZAP_STATE_SIGNATURE_PRIVATE_KEY) and change the command block that runs the Python script to ```bash (the block with the python3 backend/compact-connect/app_clients/bin/create_app_client.py -u <state-auth-pool-id>). Ensure every remaining unlabelled triple-backtick block in the file (including the ones noted around the staff/provider/state auth sections) is annotated accordingly..github/workflows/zap-scan-test.yml (1)
4-5:⚠️ Potential issue | 🟡 MinorRemove the temporary PR trigger before merge.
Line 4 explicitly says this is temporary. Keeping it runs the full authenticated ZAP scan on PR pushes, which is expensive and will fail for forked PRs without secrets.
🧹 Suggested cleanup
on: - # TEMP: trigger on PR pushes so we can validate the scan end-to-end. Revert before merge. - pull_request: - # Weekly scheduled scan of the deployed test environment schedule:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/zap-scan-test.yml around lines 4 - 5, Remove the temporary pull request trigger by deleting or reverting the "pull_request:" trigger added to the workflow; specifically remove the "pull_request:" line in the zap-scan workflow so the file no longer runs full authenticated ZAP scans on PR pushes and restore the intended triggers (e.g., push to protected branches or scheduled runs) that were used before the TEMP change.owasp-zap/manual-scan.sh (1)
24-53:⚠️ Potential issue | 🟡 MinorTreat
jq’s literalnullas an empty token.Line 38 can still emit
nullwhen the selected token field is missing, and Line 53 treats that as a real token. This can sendAuthorization: Bearer nullinto ZAP.🛡️ Suggested fix
fetch_user_token() { local mode="$1" local prefix="$2" local pool_id_var="${prefix}_COGNITO_USER_POOL_ID" @@ if [[ "$mode" == 'provider' ]]; then token_field='.idToken' fi - (cd "$authenticator_dir" && node main.js --mode="$mode") | jq -r "$token_field" + local token + token=$((cd "$authenticator_dir" && node main.js --mode="$mode") | jq -r "${token_field} // empty") + if [[ -z "$token" ]]; then + echo "Failed to obtain $mode token" >&2 + return 1 + fi + printf '%s' "$token" }Verification:
#!/bin/bash set -euo pipefail printf '{}\n' | jq -r '.accessToken' printf '{"accessToken":null}\n' | jq -r '.accessToken' printf '{}\n' | jq -r '.accessToken // empty' printf '{"accessToken":null}\n' | jq -r '.accessToken // empty'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owasp-zap/manual-scan.sh` around lines 24 - 53, fetch_user_token can return the literal string "null" when the selected JSON field is missing, causing AUTH headers like "Bearer null"; update the jq invocation in fetch_user_token to treat JSON null as empty by using the alternative operator (e.g. change jq -r "$token_field" to jq -r "$token_field // empty") so missing or null tokens yield an empty string, and keep the STAFF_TOKEN/PROVIDER_TOKEN assignments as-is so the subsequent check ([[ -z "$STAFF_TOKEN$PROVIDER_TOKEN$STATE_TOKEN" ]]) correctly treats null as missing; locate the jq call in the fetch_user_token function and apply the "$token_field // empty" change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/zap-scan-test.yml:
- Around line 4-5: Remove the temporary pull request trigger by deleting or
reverting the "pull_request:" trigger added to the workflow; specifically remove
the "pull_request:" line in the zap-scan workflow so the file no longer runs
full authenticated ZAP scans on PR pushes and restore the intended triggers
(e.g., push to protected branches or scheduled runs) that were used before the
TEMP change.
In `@owasp-zap/authenticator/.env.example`:
- Line 20: The STATE_AUTH_SCOPES value in .env.example contains unquoted spaces
which cause Bash to treat subsequent tokens as commands when the file is sourced
(used by manual-scan.sh); fix by wrapping the entire scope string in quotes
(e.g., STATE_AUTH_SCOPES="aslp/readGeneral ky/aslp.write oh/aslp.write") or
escape the spaces so the whole value is assigned to STATE_AUTH_SCOPES when
sourced.
In `@owasp-zap/manual-scan.sh`:
- Around line 24-53: fetch_user_token can return the literal string "null" when
the selected JSON field is missing, causing AUTH headers like "Bearer null";
update the jq invocation in fetch_user_token to treat JSON null as empty by
using the alternative operator (e.g. change jq -r "$token_field" to jq -r
"$token_field // empty") so missing or null tokens yield an empty string, and
keep the STAFF_TOKEN/PROVIDER_TOKEN assignments as-is so the subsequent check
([[ -z "$STAFF_TOKEN$PROVIDER_TOKEN$STATE_TOKEN" ]]) correctly treats null as
missing; locate the jq call in the fetch_user_token function and apply the
"$token_field // empty" change.
In `@owasp-zap/README.md`:
- Around line 30-35: Update the README fenced code blocks to include language
identifiers so markdownlint MD040 is satisfied: add ```text to all
environment/secret lists (e.g., the blocks containing TEST_COGNITO_* and
TEST_ZAP_* variables and the block listing TEST_ZAP_STATE_SIGNATURE_PRIVATE_KEY)
and change the command block that runs the Python script to ```bash (the block
with the python3 backend/compact-connect/app_clients/bin/create_app_client.py -u
<state-auth-pool-id>). Ensure every remaining unlabelled triple-backtick block
in the file (including the ones noted around the staff/provider/state auth
sections) is annotated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 313fb010-0022-425b-bf2f-69ace8ccdced
📒 Files selected for processing (6)
.github/workflows/zap-scan-test.ymlowasp-zap/README.mdowasp-zap/authenticator/.env.exampleowasp-zap/data/bearer-token.jsowasp-zap/enrich-oas-for-zap.pyowasp-zap/manual-scan.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- owasp-zap/enrich-oas-for-zap.py
- owasp-zap/data/bearer-token.js
The activeScan job stops at maxScanDurationInMins even mid-exploration, so coverage on large runs is partial — note this inline. Also re-triggers the ZAP scan to validate the multi-auth + ECDSA changes against current main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The 45-min cap was truncating the active scan mid-exploration (the last run hit exactly 00:45:00). Bump to 330 min so a full scan can complete; stays under the GitHub-hosted runner's 6h job limit with headroom for the surrounding steps. Will lower once we measure how long a full scan needs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- activeScan cap 330 -> 75 min (a full scan was measured at ~58 min, so 330 needlessly held a runner slot). - Replace the brittle per-URL SQL-injection (ruleId 40018) false-positive filters with a single global filter; CompactConnect is DynamoDB-only so 40018 is a known FP class, but the rule stays enabled for any future non-DynamoDB sink. - Emit a traditional-json report alongside the HTML and make exitStatus fail the job on any High alert (warn on Medium) instead of silently passing via warnExitValue: 0. - Add notify/slack_summary.py + a workflow step that posts the ZAP alert summary to Slack (ZAP_SLACK_CHANNEL_ID); no-op if the secret is unset. - Note the state M2M token's 60-min expiry vs scan length in bearer-token.js so a future cap increase doesn't silently 401. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Drop the Slack summary step and notify/slack_summary.py. Scan results are triaged via a GitHub issue (label: security) instead. The exitStatus fail-on-High behavior remains the in-CI gate. - Keep the traditional-json report as an uploaded artifact for review. - Remove the TEMP pull_request trigger now that the scan is validated; the workflow runs on the weekly schedule and manual dispatch only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- .env.example: quote STATE_AUTH_SCOPES so the space-separated value sources correctly under `set -a; .` (unquoted, it tried to exec the trailing scopes as commands and aborted under set -e). - get-m2m-token.sh: capture curl's exit status instead of letting set -e abort before the diagnostic response body can print. - manual-scan.sh: jq `// empty` so a missing token field yields "" rather than the literal "null", which defeated the -z no-tokens check. - bearer-token.js: make classifyRequest method-aware — only GET on the attestations-by-id path uses the provider token (other methods are staff). - main.js: validate --mode against an explicit staff|provider whitelist. - README: add language identifiers to fenced blocks; correct the stale 45-minute cap reference (now 75) to the measured ~57-min scan duration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@landonshumway-ia This is ready for your review! |
landonshumway-ia
approved these changes
Jun 15, 2026
landonshumway-ia
left a comment
landonshumway-ia
left a comment
Collaborator
There was a problem hiding this comment.
Looks good, just one clarifying question to make sure the public key is set up correctly.
| # State API requires ECDSA signatures on top of the M2M bearer token. | ||
| # See backend/compact-connect/docs/client_signature_auth.md. | ||
| ZAP_STATE_SIGNATURE_PRIVATE_KEY: ${{ secrets.TEST_ZAP_STATE_SIGNATURE_PRIVATE_KEY }} | ||
| ZAP_STATE_SIGNATURE_KEY_ID: ${{ secrets.TEST_ZAP_STATE_SIGNATURE_KEY_ID }} |
Collaborator
There was a problem hiding this comment.
Did you insert the public key into the test environment's compact configuration table as documented in: https://github.com/csg-org/CompactConnect/blob/main/backend/compact-connect/app_clients/bin/manage_signature_keys.py ?
Collaborator
Author
There was a problem hiding this comment.
I did this a while ago, but I confirmed that the compact configuration table in the test environment contains two public keys for KY and OH for ZAP. The app client registry has also been updated: https://docs.google.com/spreadsheets/d/1HzVoDJubwNlUm0vHK7_W9igkQG4gq2afNiukZwL0n8Y/edit?gid=0#gid=0
I assume that's sufficient but let me know if not.
Collaborator
There was a problem hiding this comment.
Yes as long as the public keys have been uploaded with the expected key id then that should be ready to use.
Comment on lines
+7
to
+8
| * - Provider users pool for /v1/provider-users/*, /v1/purchases/*, and the GET on | ||
| * /v1/compacts/{compact}/attestations/{attestationId} |
Collaborator
There was a problem hiding this comment.
Seems a little brittle to branch based off a specific section of the path in the sense that new endpoints might not be picked up, but I can't think of a better solution for this.
Collaborator
Author
There was a problem hiding this comment.
I agree, but that was sort of my thought here. Unless this is somehow better specified in the OAS or postman spec?
Collaborator
There was a problem hiding this comment.
The only way I can think of parsing the specific endpoints would be to look in the OpenAPI spec, and for every endpoint that has the 'security' field, match any endpoints that have a ProviderUsersPoolAuthorizer substring for the authorizer name. For example in the /v1/purchases/privileges endpoint definition:
"security": [
{
"SandboxAPIStackLicenseApiProviderUsersPoolAuthorizerEB7523BA": []
}
]
The ProviderUsersPoolAuthorizer is specifically used by all of our authenticated endpoints that require an token from a provider account, rather than a staff user account.
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
2 participants
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.
Extends the ZAP scan from staff-only to all three auth tiers (Staff, Provider, State M2M) and hardens the scan's configuration and result-handling.
Implements #1467 (provider-user scanning) and #1468 (state-api scanning).
Multi-tier authentication
bearer-token.jsroutes each request to the correct token by host + path: the state-api host gets the M2M token plus a per-request ECDSA-SHA256 signature (perdocs/client_signature_auth.md);/v1/provider-users/*,/v1/purchases/*, andGET attestations-by-idget the provider ID token; everything else gets the staff token.docker_env_vars(zaproxy/action-af@v0.2.0).main.jstakes--mode=staff|provider;get-m2m-token.shperforms the client-credentials grant.enrich-oas-for-zap.pyinjects valid path/body examples so ZAP gets 2xx baselines to fuzz around instead of 400-ing on every request.Scan hardening
exitStatusfails the build on any High alert (Medium is reported but non-fatal) instead of silently passing viawarnExitValue: 0.ZAP Reportartifact.bearer-token.jsso a future cap increase doesn't silently 401.Result review
Findings are triaged via a
security-labelled GitHub issue rather than a chat notification. Latest validated run: 27373665498 — 0 High / 6 Medium / 20 Low / 13 Info, with all three auth tiers producing clean 2xx baselines. Tracked for review in #1644.Testing
GET /v1/provider-users/me→ 200, state-apiPOST .../providers/query→ 200.Notes
pull_requesttrigger used to validate the scan during development has been removed.TEMP:commits will be collapsed on squash-merge.Closes #1467
Closes #1468
Summary by CodeRabbit
Release Notes