Fix volunteer PII leak on public event pages#872
Closed
edwh wants to merge 12 commits into
Closed
Conversation
PR #744 changed GroupsTable's prop API from `groups` (an array of group objects) to `groupids` (an array of group ids), but the migration of GroupsRequiringModeration was left half-finished: the template had a literal 'TODO' string visible to admins / NCs and the original `<GroupsTable :groups="groups" approve />` was commented out next to it. Render the table with `:groupids="groupIds" :approve="true"` and add a computed `groupIds` derived from the moderation store (which uses `idgroups`, but fall back to `id` for safety). Tests cover: - renders GroupsTable (not literal TODO) when groups exist - renders nothing when there are no groups - filters by the networks prop Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…le comment Three follow-ups to the GroupsRequiringModeration fix: 1. GroupsPage forwards showTags / networks / allGroupTags through to its inner GroupsTable on the 'Your groups' tab so tag badges actually render there (they didn't — the props were received from the blade but dropped on the floor). Also fixes a `your-area="yourArea"` literal-string bug (now `:your-area="yourArea"`). Removes three genuinely dead props (yourLat, yourLng, userId) and the matching blade attributes. Resolves the TODO at GroupsPage.vue:118. Added GroupsPage.test.js with 2 cases. 2. groups/fetch action now de-dups concurrent in-flight requests for the same (id, includeStats) key — GroupsPage's mounted() loop fires one fetch per yourGroup in parallel, so without this the same group could be in flight twice. Implemented as a module-scope Map; entry is cleared in a `finally` so a rejection doesn't poison the slot. Resolves the TODO at groups.js:227. Added groups.test.js with 5 cases (concurrent same-id, sequential, concurrent different-ids, error-then-retry, different includeStats). 3. Removed the stale 'See TODO below to chase the underlying reactivity bug separately' comment in grouptags.test.js — the reactivity bug WAS chased (unhandled async lifecycle rejections leaking Vue 2's scheduler `pending` flag) and the fix shipped in this PR (try/catch on async mounted hooks + flushRender() helper). Test infra: jest.config.json gets resources/js/store as a third root, plus moduleNameMapper stubs for leaflet-control-geocoder and vue-awesome so component tests can mount GroupsPage without resolving its transitive Leaflet/Icon deps. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…revert; keep PR content) # Conflicts: # resources/js/components/GroupsPage.vue # resources/js/components/GroupsRequiringModeration.vue # resources/js/store/groups.js # tests/Integration/grouptags.test.js
The userId prop was removed from GroupsPage.vue and its blade view as genuinely dead code while clearing PR #744 review TODOs. Update the test contract to match.
GroupInfoModal.groupImage guarded on group.image but built the URL from group.group_image, which the Group API resource never sets — so any group with an image rendered /uploads/mid_undefined. Use group.image (the back-filled groups.image column the resource exposes). Found by adversarial review of PR #862.
The `hosts` and `restarters` columns rendered their text labels ("Hosts" /
"Restarters") instead of icons, while every other column showed an icon. The
b-table header-override slots were named `head(all_confirmed_hosts_count)` /
`head(all_confirmed_restarters_count)`, but the field keys are `hosts` /
`restarters`, so the slots never matched and b-table fell back to the labels.
Rename the slots to `head(hosts)` / `head(restarters)`.
Test (GroupsTable.test.js): asserts the host/restarter column headers render
the user / volunteer icons rather than text.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When GroupMap is created inside a hidden tab (the groups page renders the map tab eagerly), its Leaflet container is 0x0. On switching to the tab the container resizes but nothing told Leaflet to re-measure, so tiles never filled the now-visible area and most of the map showed as grey. Observe the container with a ResizeObserver and call `invalidateSize()` (then re-run `idle()` to recompute in-bounds groups) when it changes size. Test (GroupMap.test.js): firing the resize observer calls invalidateSize(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The groups table filter bar (shown on the network page) referenced groups.search_name_placeholder, search_location_placeholder, search_country_placeholder, search_tags_placeholder, show_filters and hide_filters, none of which existed in the lang files, so the UI rendered the raw keys. Add them for en, fr and fr-BE. Test (GroupFilterTranslationsTest.php): asserts the keys exist and are non-empty in all three locales. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Party::expandVolunteers() was serialising the full User Eloquent model into the page HTML (via json_encode in the Blade template). Because User::$hidden only contained password and remember_token, any visitor could read api_token, calendar_hash, latitude, longitude, recovery, and mediawiki tokens directly from the page source. Fixes: 1. User::$hidden — add api_token, calendar_hash, recovery, recovery_expires, mediawiki, latitude, longitude (defence-in-depth; protects every serialisation point). 2. Party::expandVolunteers() — replace the full User model assignment with an explicit whitelist of only the fields the frontend needs (id, name, email when showEmails, user_skills shape). Never put an Eloquent model directly into page-embedded JSON. 3. ApiController::getUserInfo() — extend the belt-and-suspenders makeHidden() call to cover all credential fields, not just api_token. Tests (VolunteerPiiTest.php): - testEventPageDoesNotLeakVolunteerCredentials: loads the public event page HTML and asserts sentinel values are absent while volunteer name is still present. - testUserModelHiddenFieldsExcludeCredentials: asserts every sensitive field is in $hidden via User::toArray(). - testExpandVolunteersEmitsSafeShape: asserts expandVolunteers() returns a plain array (not an Eloquent model) and contains no sensitive keys. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| <!-- Styles --> | ||
| @vite(['resources/sass/app.scss', 'resources/global/css/app.scss']) | ||
| <link rel="stylesheet" href="https://unpkg.com/leaflet-control-geocoder/dist/Control.Geocoder.css" type="text/css" /> |
| <!-- Styles --> | ||
| @vite(['resources/sass/app.scss', 'resources/global/css/app.scss']) | ||
| <link rel="stylesheet" href="https://unpkg.com/leaflet-control-geocoder/dist/Control.Geocoder.css" type="text/css" /> | ||
| <link rel="stylesheet" href="//unpkg.com/leaflet-gesture-handling/dist/leaflet-gesture-handling.min.css" type="text/css"> |
…ints
- Call unsetRelation('volunteer') before assigning the safe array; without
this, Eloquent's toArray() merges relationsToArray() last, silently
overwriting the safe array with the full User model in JSON output
- Add null guard for orphaned skill FK rows (deleted skillName record)
- Fix inverted $showEmails in EventController::listVolunteers — was
negated so non-hosts received emails and hosts did not
- Add latitude/longitude to makeHidden() in ApiController::getUserInfo()
and UserController::mapUserAndAuditToUserChange() (Zapier endpoint)
- Update testExpandVolunteersEmitsSafeShape to assert via json_encode so
the Eloquent relation-bypass is actually caught (PHP array-access was
a false pass — it reads $attributes, not the serialised output)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add api_token, calendar_hash, recovery, recovery_expires, latitude, longitude, and password to User::$auditExclude so they are never written to the audits table on any model update. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
Closing in favour of a clean branch rebased directly onto develop — the original branch was cut from a feature branch, so the diff included ~100 unrelated files. Replaced by #879 |
|
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.
Summary
Party::expandVolunteers()was serialising the fullUserEloquent model into page HTML viajson_encode. Any visitor to a public event page could readapi_token,calendar_hash,latitude,longitude,recovery, andmediawikitokens directly from the page source — and use theapi_tokento authenticate as that volunteer.User::$hiddenonly containedpasswordandremember_token, leaving all other sensitive fields unprotected in any serialisation.ApiController::getUserInfo()was callingmakeHidden('api_token')but leavingcalendar_hash,recovery, etc. visible.Changes
app/User.php— extend$hiddento includeapi_token,calendar_hash,recovery,recovery_expires,mediawiki,latitude,longitude. Defence-in-depth: protects every serialisation point, not just the one we know about.app/Party.phpexpandVolunteers()— replace the fullUsermodel assignment with an explicit whitelist of only the fields the frontend needs:id,name, optionallyemail, and theuser_skillsshape{ skill_name: { skill_name: "…" } }. The frontend only accessesvolunteer.nameandvolunteer.user_skills— no other User fields are consumed.app/Http/Controllers/ApiController.php— extend the belt-and-suspendersmakeHidden()call to cover all credential fields.Tests
tests/Feature/Events/VolunteerPiiTest.php(3 tests, 26 assertions):testEventPageDoesNotLeakVolunteerCredentials— loads the public event page HTML with a known sentinelapi_tokenand asserts it is absent, while the volunteer's name is present (sanity check that volunteers are still shown).testUserModelHiddenFieldsExcludeCredentials— asserts every sensitive field key is absent fromUser::toArray().testExpandVolunteersEmitsSafeShape— assertsexpandVolunteers()returns a plain array (not an Eloquent model) and contains none of the sensitive keys.Test plan
VolunteerPiiTesttests pass (OK (3 tests, 26 assertions)confirmed locally)api_token,calendar_hash, coordinates visible🤖 Generated with Claude Code