Fix: Ensure Event/Volunteer Responses only Include Intended User Fields#54
Merged
Conversation
The User model only hid password and remember_token during serialization, leaving api_token, calendar_hash, recovery tokens, coordinates, and login activity exposed wherever the model was json_encoded — including public event pages accessible without authentication.
… makeHidden
expandVolunteers() previously assigned the full User Eloquent model
to volunteer data that gets json_encoded into public HTML. Replacing
it with an explicit {id, name, email} allowlist prevents future
fields or makeVisible() calls from re-exposing data. The ad-hoc
makeHidden calls in ApiController and UserController for fields now
covered by User::$hidden are also removed.
Asserts that public event pages and the volunteers API endpoint do not expose api_token, calendar_hash, recovery tokens, or coordinates in their responses.
…pplies The expandVolunteers() allowlist added in 58a2e8e was a no-op: $volunteer is an EventsUsers model with the `volunteer` relation already loaded, so during toArray() Eloquent's relationsToArray() is merged after the attributes and overwrites $volunteer['volunteer'] with the full related User model. As a result public event pages still serialized the entire user record to anonymous visitors — username, external_user_id, location, age, gender, country_code, consent timestamps, biography, etc. — and the $showEmails gating was bypassed, leaking volunteer emails too. Only the fields in User::$hidden (api_token, calendar_hash, recovery, coordinates, login activity) were actually protected. Calling unsetRelation('volunteer') before assigning the allowlist array removes the loaded relation so the {id, name, email} array survives serialization. Also harden PublicEventDataLeakTest: - approve the group, otherwise the anonymous request 404s before the page renders and the public-page assertions never run (vacuous pass) - extract every embedded "volunteer" object and assert it is limited to the {id, name, email} allowlist - assert volunteer email is not exposed to anonymous viewers - add username/external_user_id sentinels Verified: test fails on the pre-fix code and passes with the fix.
…render
The {id, name, email} allowlist introduced for expandVolunteers dropped
user_skills from the serialized volunteer object, but the event page reads
attendee.volunteer.user_skills (EventAttendee.vue) to show each volunteer's
skill count and list. Without it every attendee rendered as "0 skills".
Re-add user_skills to the allowlist (skill associations are not sensitive)
and extend the regression test to assert the volunteer object is exactly
{id, name, email, user_skills}.
Collaborator
Author
|
QA 👍 checked against the staging instance. |
sctice-ifixit
left a comment
sctice-ifixit
left a comment
Member
There was a problem hiding this comment.
Claude Code findings.
unsetRelation('volunteer') is what makes the allowlist take effect, and EventsUsers has only the one user-bearing relation, so that closes the leak. The allowlist preserves everything EventAttendee.vue reads (volunteer.name, volunteer.user_skills, plus top-level fullName/profilePath).
Three inline notes: a Zapier heads-up (no action), a test-coverage gap, and a redundant-payload nit.
| 'recovery_expires', | ||
| 'latitude', | ||
| 'longitude', | ||
| 'last_login_at', |
Member
There was a problem hiding this comment.
Heads-up: latitude, longitude, and last_login_at weren't hidden in the Zapier changes path (mapUserAndAuditToUserChange hid only number_of_logins and the token fields), so they were present in that feed. They're now hidden globally. No evidence anything consumes them; flagging in case a Zap maps those fields.
The field-name checks ran against the raw response, but on the public page path Blade escapes the JSON quotes ("), so the key checks were no-ops there. Decode HTML entities before the key checks. latitude/longitude are legitimate public fields on the event itself, so the bare key can't distinguish event location from a leaked user location. Guard the volunteer's coordinates with distinctive numeric value sentinels instead, and drop them from the key-name allowlist.
…nteers userSkills was serialized twice: at the top level of each volunteer and nested under volunteer.user_skills. Only the nested copy is consumed (EventAttendee.vue reads volunteer.user_skills); the top-level copy was dead weight in the payload. Use a local accumulator so the skills are serialized once, under the volunteer allowlist only.
Collaborator
Author
|
QA 👍 |
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
Event pages and the volunteers API previously serialized full
Usermodels into their responses. This restricts them to an explicit, minimal set of fields (id,name, and—where appropriate—email) and hardens model serialization so only intended fields are ever included.Changes
Usermodel: expand the serialization$hiddenset so internal/account fields are never included by default (defence-in-depth for any path that serializes a user).expandVolunteers(): return an explicit{id, name, email}allowlist. A prior allowlist wasn't taking effect because a loaded Eloquent relation overrode the array duringtoArray(); the relation is now cleared first so the allowlist (and the existing email-visibility rule) applies.makeHidden()calls in the API controllers (covered by the model$hiddenset).