Fixes five findings from the security review, one commit each (plus a regenerated AGENTS.md).
#3 — The staff key no longer appears in URLs (818c979)
Before: the Engagement Report tab put STAFF_REPORT_KEY into the iframe URL for every user with "access CiviCRM", without checking contact permissions. Anyone could copy it from the page and open any org's report and files, getting around Civi's ACLs. The key never expired and ended up in access logs.
Extension 0.4.0: Tab.php checks Contact_Permission::allow(VIEW), then mints sig = base64url(HMAC-SHA256(key, "webform-mw.staff-report.v1:<org>:<exp>")), valid for 8 hours.
App: lib/staff-token.mjs checks the signature in constant time. The org must match, and tokens that are expired or valid for more than 12 hours are rejected. /staff/report, /api/staff/report and /api/staff/file share staffAuthQuery().
Transitional: legacy ?key= is still accepted (compared in constant time, with a warning logged) so the app can deploy before the extension. A follow-up PR will remove it.
#6 — Fail-closed verifyChecksum (c9b6105)
Removed the Contact.get fallback. Our API key is privileged, so that fallback would have accepted any checksum if Civi ever ignored the unknown parameter. Now any API error means "invalid."
#5 — Extension upload endpoint hardened (0a19b57)
It now requires X-Requested-With: XMLHttpRequest (blocks CSRF), EDIT permission on the target contact or activity, and the same mime allowlist as /api/upload.
#4 — File ids are verified on submit (df471e0)
A file id from the client is accepted only if it's anchored to the submitting contact (/api/upload stores uploads that way) or matches the org's current value. Otherwise the submit returns 400.
#8 — /api/preview-link gets its own Bearer secret (526422a)
PREVIEW_ADMIN_TOKEN, when set, is required as Authorization: Bearer and is never accepted from the URL.
Until it's set, HEALTH_TOKEN keeps working, so the EMAIL_DELIVERY.md runbook isn't broken.
Token checks in preview-link and /api/health are now constant-time, and redact() covers the new secrets.
Rollout order (important)
Merge. Amplify deploys, and the existing tab keeps working through the legacy ?key=.
Install extension 0.4.0 on client.crm.fci.coop and crm.fci.coop.
Then the follow-up PR to remove legacy ?key= support.
Optional: add PREVIEW_ADMIN_TOKEN in Amplify Secrets.
Test plan
npm test: 36/36 pass (9 new token tests, including a PHP-generated vector that pins PHP/Node compatibility)
npm run build succeeds on Node 24. Lint shows the same 3 errors as main, with one warning fewer.
php -l passes on all extension pages
Staff auth against a stub-mode production build: valid token → 200. Wrong org, tampered exp, expired, over-long TTL, wrong key, no auth → 404. Legacy key → 200 + warning. Page HTML doesn't contain the secret.
preview-link auth checked across all combinations on a dev server
After merge: the Engagement Report tab still loads (legacy path), and /api/health?token=… still works
After installing extension 0.4.0: the tab loads, the iframe URL shows exp=&sig= with no key=, the attachment lightbox and downloads work, and a user without view access to an org gets "permission denied"
Form file upload + submit still work end to end.#5 requires the app's Civi API user to have edit permission on the form-filler's contact, and #4 runs Attachment.get on each submitted file
Re-submitting the form with an existing attachment left unchanged still succeeds
Not covered locally: #4, #5 and #6 need a live CiviCRM
## Summary
Fixes five findings from the security review, one commit each (plus a regenerated `AGENTS.md`).
**#3 — The staff key no longer appears in URLs** (`818c979`)
- Before: the Engagement Report tab put `STAFF_REPORT_KEY` into the iframe URL for every user with "access CiviCRM", without checking contact permissions. Anyone could copy it from the page and open any org's report and files, getting around Civi's ACLs. The key never expired and ended up in access logs.
- Extension **0.4.0**: `Tab.php` checks `Contact_Permission::allow(VIEW)`, then mints `sig = base64url(HMAC-SHA256(key, "webform-mw.staff-report.v1:<org>:<exp>"))`, valid for 8 hours.
- App: `lib/staff-token.mjs` checks the signature in constant time. The org must match, and tokens that are expired or valid for more than 12 hours are rejected. `/staff/report`, `/api/staff/report` and `/api/staff/file` share `staffAuthQuery()`.
- **Transitional:** legacy `?key=` is still accepted (compared in constant time, with a warning logged) so the app can deploy before the extension. A follow-up PR will remove it.
**#6 — Fail-closed `verifyChecksum`** (`c9b6105`)
Removed the `Contact.get` fallback. Our API key is privileged, so that fallback would have accepted any checksum if Civi ever ignored the unknown parameter. Now any API error means "invalid."
**#5 — Extension upload endpoint hardened** (`0a19b57`)
It now requires `X-Requested-With: XMLHttpRequest` (blocks CSRF), EDIT permission on the target contact or activity, and the same mime allowlist as `/api/upload`.
**#4 — File ids are verified on submit** (`df471e0`)
A file id from the client is accepted only if it's anchored to the submitting contact (`/api/upload` stores uploads that way) or matches the org's current value. Otherwise the submit returns 400.
**#8 — `/api/preview-link` gets its own Bearer secret** (`526422a`)
- `PREVIEW_ADMIN_TOKEN`, when set, is required as `Authorization: Bearer` and is never accepted from the URL.
- Until it's set, `HEALTH_TOKEN` keeps working, so the `EMAIL_DELIVERY.md` runbook isn't broken.
- Token checks in `preview-link` and `/api/health` are now constant-time, and `redact()` covers the new secrets.
## Rollout order (important)
1. Merge. Amplify deploys, and the existing tab keeps working through the legacy `?key=`.
2. Install extension **0.4.0** on client.crm.fci.coop and crm.fci.coop.
3. Then the follow-up PR to remove legacy `?key=` support.
4. Optional: add `PREVIEW_ADMIN_TOKEN` in Amplify Secrets.
## Test plan
- [x] `npm test`: 36/36 pass (9 new token tests, including a PHP-generated vector that pins PHP/Node compatibility)
- [x] `npm run build` succeeds on Node 24. Lint shows the same 3 errors as `main`, with one warning fewer.
- [x] `php -l` passes on all extension pages
- [x] Staff auth against a stub-mode production build: valid token → 200. Wrong org, tampered exp, expired, over-long TTL, wrong key, no auth → 404. Legacy key → 200 + warning. Page HTML doesn't contain the secret.
- [x] preview-link auth checked across all combinations on a dev server
- [ ] After merge: the Engagement Report tab still loads (legacy path), and `/api/health?token=…` still works
- [ ] After installing extension 0.4.0: the tab loads, the iframe URL shows `exp=&sig=` with no `key=`, the attachment lightbox and downloads work, and a user without view access to an org gets "permission denied"
- [ ] **Form file upload + submit still work end to end.** #5 requires the app's Civi API user to have edit permission on the form-filler's contact, and #4 runs `Attachment.get` on each submitted file
- [ ] Re-submitting the form with an existing attachment left unchanged still succeeds
- [ ] Not covered locally: #4, #5 and #6 need a live CiviCRM
🤖 Generated with [Claude Code](https://claude.com/claude-code)
The Engagement Report tab wrote STAFF_REPORT_KEY into the iframe src for
every user with "access CiviCRM", without checking contact permissions.
Anyone could copy it from the DOM and read any org's report and files,
bypassing Civi ACLs; the key never expired and landed in access logs.
- Extension (0.4.0): Tab.php checks Contact_Permission::allow(VIEW) and
mints sig = base64url(HMAC-SHA256(key, "webform-mw.staff-report.v1:
<org>:<exp>")) with an 8h exp. The secret no longer leaves the server.
- App: lib/staff-token.mjs verifies the token (constant-time, org must
match, rejects expired or >12h tokens). /staff/report, /api/staff/report
and /api/staff/file share staffAuthQuery(); client links carry exp/sig.
- Legacy ?key= is still accepted (constant-time compare, logs a warning)
so the app can deploy before the extension. Remove it once every Civi
install runs extension 0.4.0.
- PHP/Node signature compatibility is pinned by a PHP-generated vector.
If Contact.validateChecksum threw for any reason, verifyChecksum retried
with Contact.get plus an unknown `checksum` param. Our API key is
privileged, so if Civi ever ignored that param the probe would resolve
the contact and accept any checksum. Treat errors as invalid instead,
and reject non-positive/non-integer contact ids up front.
civicrm/webform-mw/upload accepted any session-authenticated POST, called
civicrm_api3('Attachment','create') (which skips permission checks from
PHP) against any contact/activity id, and stored any client mime type.
- Require X-Requested-With: XMLHttpRequest (already sent by the app), so
cross-site form posts can't ride a logged-in staff session.
- Require EDIT permission on the target contact/activity.
- Enforce the same mime allowlist as the app's /api/upload route.
/api/submit wrote whatever {id} the client sent into file custom fields.
A link holder could point their org at any civicrm_file id, learn its
filename via /api/report, and make the staff file proxy treat it as
theirs. Each file id must now either be anchored to the submitting
contact (how /api/upload stores uploads) or equal the org's current value
for that field; otherwise the submit is rejected with a 400.
preview-link mints working form links for any contact, but was gated by
HEALTH_TOKEN (the diagnostics secret) passed in the query string, where
it lands in access logs. README already documented PREVIEW_ADMIN_TOKEN
as a Bearer header; make the code match.
- PREVIEW_ADMIN_TOKEN, when set, is required as Authorization: Bearer
and is never accepted from the URL.
- Until it is set, HEALTH_TOKEN still works (Bearer or legacy ?token=)
so the support runbook in EMAIL_DELIVERY.md keeps working.
- preview-link and /api/health compare tokens in constant time.
- redact() also scrubs PREVIEW_ADMIN_TOKEN and STAFF_REPORT_KEY.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Summary
Fixes five findings from the security review, one commit each (plus a regenerated
AGENTS.md).#3 — The staff key no longer appears in URLs (
818c979)STAFF_REPORT_KEYinto the iframe URL for every user with "access CiviCRM", without checking contact permissions. Anyone could copy it from the page and open any org's report and files, getting around Civi's ACLs. The key never expired and ended up in access logs.Tab.phpchecksContact_Permission::allow(VIEW), then mintssig = base64url(HMAC-SHA256(key, "webform-mw.staff-report.v1:<org>:<exp>")), valid for 8 hours.lib/staff-token.mjschecks the signature in constant time. The org must match, and tokens that are expired or valid for more than 12 hours are rejected./staff/report,/api/staff/reportand/api/staff/filesharestaffAuthQuery().?key=is still accepted (compared in constant time, with a warning logged) so the app can deploy before the extension. A follow-up PR will remove it.#6 — Fail-closed
verifyChecksum(c9b6105)Removed the
Contact.getfallback. Our API key is privileged, so that fallback would have accepted any checksum if Civi ever ignored the unknown parameter. Now any API error means "invalid."#5 — Extension upload endpoint hardened (
0a19b57)It now requires
X-Requested-With: XMLHttpRequest(blocks CSRF), EDIT permission on the target contact or activity, and the same mime allowlist as/api/upload.#4 — File ids are verified on submit (
df471e0)A file id from the client is accepted only if it's anchored to the submitting contact (
/api/uploadstores uploads that way) or matches the org's current value. Otherwise the submit returns 400.#8 —
/api/preview-linkgets its own Bearer secret (526422a)PREVIEW_ADMIN_TOKEN, when set, is required asAuthorization: Bearerand is never accepted from the URL.HEALTH_TOKENkeeps working, so theEMAIL_DELIVERY.mdrunbook isn't broken.preview-linkand/api/healthare now constant-time, andredact()covers the new secrets.Rollout order (important)
?key=.?key=support.PREVIEW_ADMIN_TOKENin Amplify Secrets.Test plan
npm test: 36/36 pass (9 new token tests, including a PHP-generated vector that pins PHP/Node compatibility)npm run buildsucceeds on Node 24. Lint shows the same 3 errors asmain, with one warning fewer.php -lpasses on all extension pages/api/health?token=…still worksexp=&sig=with nokey=, the attachment lightbox and downloads work, and a user without view access to an org gets "permission denied"Attachment.geton each submitted file🤖 Generated with Claude Code
civicrm/webform-mw/upload accepted any session-authenticated POST, called civicrm_api3('Attachment','create') (which skips permission checks from PHP) against any contact/activity id, and stored any client mime type. - Require X-Requested-With: XMLHttpRequest (already sent by the app), so cross-site form posts can't ride a logged-in staff session. - Require EDIT permission on the target contact/activity. - Enforce the same mime allowlist as the app's /api/upload route./api/submit wrote whatever {id} the client sent into file custom fields. A link holder could point their org at any civicrm_file id, learn its filename via /api/report, and make the staff file proxy treat it as theirs. Each file id must now either be anchored to the submitting contact (how /api/upload stores uploads) or equal the org's current value for that field; otherwise the submit is rejected with a 400.