Extend ZAP scan to provider users and state API M2M (#1467, #1468)#1470
Open
Extend ZAP scan to provider users and state API M2M (#1467, #1468)#1470
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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 docstrings
🧪 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
Comment on lines
+4
to
+5
| # TEMP: trigger on PR pushes so we can validate the scan end-to-end. Revert before merge. | ||
| pull_request: |
Contributor
There was a problem hiding this comment.
Reminder: revert the temporary pull_request trigger before merge.
Per the PR description, this trigger is intended to be removed prior to merge. Two additional notes while it's present:
pull_requestevents from forked repos do not have access to repository secrets, so the three authentication steps (and thus the whole job) will hard-fail for any external contributor PR.- Running the full ZAP active scan on every PR push is expensive and will target the live test environment — consider path filters or a manual
workflow_dispatchgate if this is ever made permanent.
Want me to draft a follow-up commit that removes the pull_request: trigger (or replaces it with a path-filtered, label-gated variant) once the end-to-end validation is complete?
🤖 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, 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.
| COGNITO_STATE_AUTH_DOMAIN=compact-connect-state-auth-test.auth.us-east-1.amazoncognito.com | ||
| STATE_AUTH_CLIENT_ID=xxxxxxxxxxxxxxxxxxxxxxxxxx | ||
| STATE_AUTH_CLIENT_SECRET=yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy | ||
| STATE_AUTH_SCOPES=aslp/readGeneral aslp/write ky/aslp.write oh/aslp.write |
Contributor
There was a problem hiding this comment.
Quote STATE_AUTH_SCOPES — unquoted multi-word value breaks source-ing the file.
manual-scan.sh loads .env via set -a; . "$env_file"; set +a. In bash, VAR=a b c is a one-shot env assignment followed by running b c as a command, so this line will try to execute aslp/write ky/aslp.write oh/aslp.write after setting STATE_AUTH_SCOPES=aslp/readGeneral. Combined with set -e at the top of manual-scan.sh, sourcing the user's copied .env will abort the script (and at best leave STATE_AUTH_SCOPES set to only the first scope). Anyone following the template will hit this.
🛡️ Proposed fix
-STATE_AUTH_SCOPES=aslp/readGeneral aslp/write ky/aslp.write oh/aslp.write
+STATE_AUTH_SCOPES="aslp/readGeneral aslp/write ky/aslp.write oh/aslp.write"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| STATE_AUTH_SCOPES=aslp/readGeneral aslp/write ky/aslp.write oh/aslp.write | |
| STATE_AUTH_SCOPES="aslp/readGeneral aslp/write ky/aslp.write oh/aslp.write" |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 20-20: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🤖 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 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.
Comment on lines
+19
to
+30
| response=$(curl -sS --fail-with-body \ | ||
| -X POST "https://${COGNITO_STATE_AUTH_DOMAIN}/oauth2/token" \ | ||
| -u "${STATE_AUTH_CLIENT_ID}:${STATE_AUTH_CLIENT_SECRET}" \ | ||
| -H "Content-Type: application/x-www-form-urlencoded" \ | ||
| --data-urlencode "grant_type=client_credentials" \ | ||
| --data-urlencode "scope=${STATE_AUTH_SCOPES}") | ||
|
|
||
| token=$(jq -r '.access_token // empty' <<<"$response") | ||
| if [[ -z "$token" ]]; then | ||
| echo "Failed to obtain M2M token. Response: $response" >&2 | ||
| exit 1 | ||
| fi |
Contributor
There was a problem hiding this comment.
set -e makes --fail-with-body ineffective on HTTP errors.
With set -euo pipefail, if curl returns non-zero (e.g., HTTP 4xx from bad scopes or misconfigured client), the response=$(...) command substitution fails and the script aborts before reaching the jq/error block at lines 26–30. The body captured by --fail-with-body is never surfaced to the operator, leaving only curl's terse -sS stderr. Consider separating success/failure paths so the response body is always reported on error.
🛡️ Suggested fix
-response=$(curl -sS --fail-with-body \
- -X POST "https://${COGNITO_STATE_AUTH_DOMAIN}/oauth2/token" \
- -u "${STATE_AUTH_CLIENT_ID}:${STATE_AUTH_CLIENT_SECRET}" \
- -H "Content-Type: application/x-www-form-urlencoded" \
- --data-urlencode "grant_type=client_credentials" \
- --data-urlencode "scope=${STATE_AUTH_SCOPES}")
-
-token=$(jq -r '.access_token // empty' <<<"$response")
-if [[ -z "$token" ]]; then
- echo "Failed to obtain M2M token. Response: $response" >&2
- exit 1
-fi
-printf '%s' "$token"
+http_status=0
+response=$(curl -sS -w '\n%{http_code}' \
+ -X POST "https://${COGNITO_STATE_AUTH_DOMAIN}/oauth2/token" \
+ -u "${STATE_AUTH_CLIENT_ID}:${STATE_AUTH_CLIENT_SECRET}" \
+ -H "Content-Type: application/x-www-form-urlencoded" \
+ --data-urlencode "grant_type=client_credentials" \
+ --data-urlencode "scope=${STATE_AUTH_SCOPES}") || true
+http_status="${response##*$'\n'}"
+body="${response%$'\n'*}"
+
+if [[ "$http_status" != 2* ]]; then
+ echo "Failed to obtain M2M token (HTTP $http_status). Response: $body" >&2
+ exit 1
+fi
+
+token=$(jq -r '.access_token // empty' <<<"$body")
+if [[ -z "$token" ]]; then
+ echo "Token missing from response: $body" >&2
+ exit 1
+fi
+printf '%s' "$token"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@owasp-zap/authenticator/get-m2m-token.sh` around lines 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.
Comment on lines
+24
to
+50
| fetch_user_token() { | ||
| local mode="$1" | ||
| local prefix="$2" | ||
| local pool_id_var="${prefix}_COGNITO_USER_POOL_ID" | ||
| if [[ -z "${!pool_id_var:-}" ]]; then | ||
| echo "Skipping $mode token: ${prefix}_COGNITO_* vars not set in $env_file" >&2 | ||
| return 1 | ||
| fi | ||
| (cd "$authenticator_dir" && node main.js --mode="$mode") | jq -r '.accessToken' | ||
| } | ||
|
|
||
| fetch_m2m_token() { | ||
| if [[ -z "${COGNITO_STATE_AUTH_DOMAIN:-}" ]]; then | ||
| echo "Skipping state M2M token: COGNITO_STATE_AUTH_DOMAIN not set in $env_file" >&2 | ||
| return 1 | ||
| fi | ||
| (cd "$authenticator_dir" && ./get-m2m-token.sh) | ||
| } | ||
|
|
||
| STAFF_TOKEN=$(fetch_user_token staff STAFF || true) | ||
| PROVIDER_TOKEN=$(fetch_user_token provider PROVIDER || true) | ||
| STATE_TOKEN=$(fetch_m2m_token || true) | ||
|
|
||
| if [[ -z "$STAFF_TOKEN$PROVIDER_TOKEN$STATE_TOKEN" ]]; then | ||
| echo "No tokens obtained; aborting." >&2 | ||
| exit 1 | ||
| fi |
Contributor
There was a problem hiding this comment.
Treat jq's literal null as empty.
If node main.js prints nothing or a payload without .accessToken, jq -r '.accessToken' emits the literal string null. STAFF_TOKEN/PROVIDER_TOKEN then contains "null", which passes the -z "$STAFF_TOKEN$PROVIDER_TOKEN$STATE_TOKEN" guard on line 47 and is forwarded to ZAP as Authorization: Bearer null. The GitHub workflow already guards against this ("$token" == "null"); mirroring that here keeps the two paths consistent.
🛡️ Suggested fix
fetch_user_token() {
local mode="$1"
local prefix="$2"
local pool_id_var="${prefix}_COGNITO_USER_POOL_ID"
if [[ -z "${!pool_id_var:-}" ]]; then
echo "Skipping $mode token: ${prefix}_COGNITO_* vars not set in $env_file" >&2
return 1
fi
- (cd "$authenticator_dir" && node main.js --mode="$mode") | jq -r '.accessToken'
+ local token
+ token=$((cd "$authenticator_dir" && node main.js --mode="$mode") | jq -r '.accessToken')
+ if [[ -z "$token" || "$token" == "null" ]]; then
+ echo "Failed to obtain $mode token" >&2
+ return 1
+ fi
+ printf '%s' "$token"
}🤖 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 - 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.
Comment on lines
+30
to
+35
| ``` | ||
| TEST_COGNITO_USER_POOL_ID_STAFF | ||
| TEST_WEBROOT_COGNITO_CLIENT_ID_STAFF | ||
| TEST_ZAP_USERNAME_STAFF | ||
| TEST_ZAP_PASSWORD_STAFF | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (markdownlint MD040).
These code fences should specify a language to satisfy linting and keep docs consistent.
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")
```Also applies to: 39-44, 56-58, 66-71
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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: 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.
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
Comment on lines
+25
to
+32
| const PROVIDER_ATTESTATION_PATH = /^\/v1\/compacts\/[^\/]+\/attestations\/[^\/]+$/; | ||
|
|
||
| function classifyRequest(host, path) { | ||
| if (host.indexOf('state-api.') === 0) return 'state'; | ||
| if (PROVIDER_PATH_PREFIX.test(path)) return 'provider'; | ||
| if (PROVIDER_ATTESTATION_PATH.test(path)) return 'provider'; | ||
| return 'staff'; | ||
| } |
Contributor
There was a problem hiding this comment.
Attestation route ignores HTTP method — non-GET requests will be mis-authenticated.
The PR objective states only the GET on /v1/compacts/{compact}/attestations/{attestationId} should receive the provider token; other methods on that path (e.g. staff-uploaded attestation PUT/POST) should use the staff token. classifyRequest has no access to the method here and will route every request matching PROVIDER_ATTESTATION_PATH to the provider token, which will cause 401/403s on staff-initiated attestation mutations during the scan and skew findings.
Consider threading the method through and gating the attestation rule on GET only:
🔧 Proposed fix
-function classifyRequest(host, path) {
- if (host.indexOf('state-api.') === 0) return 'state';
- if (PROVIDER_PATH_PREFIX.test(path)) return 'provider';
- if (PROVIDER_ATTESTATION_PATH.test(path)) return 'provider';
- return 'staff';
-}
+function classifyRequest(host, path, method) {
+ if (host.indexOf('state-api.') === 0) return 'state';
+ if (PROVIDER_PATH_PREFIX.test(path)) return 'provider';
+ if (method === 'GET' && PROVIDER_ATTESTATION_PATH.test(path)) return 'provider';
+ return 'staff';
+}And at the call site:
- const kind = classifyRequest(String(uri.getHost()), String(uri.getPath()));
+ const method = String(msg.getRequestHeader().getMethod()).toUpperCase();
+ const kind = classifyRequest(String(uri.getHost()), String(uri.getPath()), method);🤖 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 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.
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
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
1 participant
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.
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.
Requirements List
Description List
Testing List
yarn test:unit:allshould run without errors or warningsyarn serveshould run without errors or warningsyarn buildshould run without errors or warningsbackend/compact-connect/tests/unit/test_api.pyrun compact-connect/bin/download_oas30.pyCloses #
Summary by CodeRabbit
New Features
Documentation
Chores