From 753fc89edc21bfad05fee52d586751a929ee1c83 Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Fri, 5 Jun 2026 09:28:24 -0700 Subject: [PATCH 1/7] fix(model): add sensitive fields to User $hidden array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- app/Models/User.php | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/app/Models/User.php b/app/Models/User.php index 8851e13f3..b7989d13c 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -54,7 +54,16 @@ class User extends Authenticatable implements Auditable, HasLocalePreference * @var array */ protected $hidden = [ - 'password', 'remember_token', + 'password', + 'remember_token', + 'api_token', + 'calendar_hash', + 'recovery', + 'recovery_expires', + 'latitude', + 'longitude', + 'last_login_at', + 'number_of_logins', ]; /** From 58a2e8e58db92e636380bbc8713e244f14addc9c Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Fri, 5 Jun 2026 09:28:45 -0700 Subject: [PATCH 2/7] fix(security): use allowlist in expandVolunteers and remove redundant 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. --- app/Http/Controllers/API/UserController.php | 6 ----- app/Http/Controllers/ApiController.php | 2 -- app/Models/Party.php | 26 ++++++++++----------- 3 files changed, 12 insertions(+), 22 deletions(-) diff --git a/app/Http/Controllers/API/UserController.php b/app/Http/Controllers/API/UserController.php index fc0291979..304b9e6a5 100644 --- a/app/Http/Controllers/API/UserController.php +++ b/app/Http/Controllers/API/UserController.php @@ -57,15 +57,9 @@ protected static function getUserAudits($dateFrom = null) protected static function mapUserAndAuditToUserChange($user, $audit) { - // Hide fields not relevant for Zapier. $user->makeHidden([ 'updated_at', 'deleted_at', - 'api_token', - 'recovery', - 'recovery_expires', - 'calendar_hash', - 'number_of_logins', 'consent_past_data', 'consent_gdpr', 'consent_future_data', diff --git a/app/Http/Controllers/ApiController.php b/app/Http/Controllers/ApiController.php index 554034364..3a56e153c 100644 --- a/app/Http/Controllers/ApiController.php +++ b/app/Http/Controllers/ApiController.php @@ -164,8 +164,6 @@ public static function getUserInfo(): JsonResponse { $user = Auth::user(); - $user->makeHidden('api_token'); - return response()->json($user->toArray()); } diff --git a/app/Models/Party.php b/app/Models/Party.php index e72a36a3b..c0963ca4b 100644 --- a/app/Models/Party.php +++ b/app/Models/Party.php @@ -824,20 +824,18 @@ public static function expandVolunteers($volunteers, $showEmails) { $volunteer['fullName'] = $volunteer->getFullName(); if ($volunteer->volunteer) { - $volunteer['volunteer'] = $volunteer->volunteer; - - if (!$showEmails) { - $volunteer['volunteer']['email'] = NULL; - } - - if (! empty($volunteer->volunteer)) { - $volunteer['userSkills'] = $volunteer->volunteer->userSkills->all(); - $volunteer['profilePath'] = '/uploads/thumbnail_'.$volunteer->volunteer->getProfile($volunteer->volunteer->id)->path; - - foreach ($volunteer['userSkills'] as $skill) { - // Force expansion - $skill->skillName->skill_name; - } + $user = $volunteer->volunteer; + $volunteer['volunteer'] = [ + 'id' => $user->id, + 'name' => $user->name, + 'email' => $showEmails ? $user->email : null, + ]; + + $volunteer['userSkills'] = $user->userSkills->all(); + $volunteer['profilePath'] = '/uploads/thumbnail_'.$user->getProfile($user->id)->path; + + foreach ($volunteer['userSkills'] as $skill) { + $skill->skillName->skill_name; } } From 75cb3a34acce354011183339dfc111f804ed75cd Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Fri, 5 Jun 2026 09:29:04 -0700 Subject: [PATCH 3/7] test(security): add regression test for sensitive field leakage Asserts that public event pages and the volunteers API endpoint do not expose api_token, calendar_hash, recovery tokens, or coordinates in their responses. --- .../Events/PublicEventDataLeakTest.php | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 tests/Feature/Events/PublicEventDataLeakTest.php diff --git a/tests/Feature/Events/PublicEventDataLeakTest.php b/tests/Feature/Events/PublicEventDataLeakTest.php new file mode 100644 index 000000000..718084a7c --- /dev/null +++ b/tests/Feature/Events/PublicEventDataLeakTest.php @@ -0,0 +1,95 @@ +create(); + $network = Network::factory()->create(); + $network->addGroup($group); + + $event = Party::factory()->create([ + 'group' => $group, + 'event_start_utc' => '2130-01-01T12:13:00+00:00', + 'event_end_utc' => '2130-01-01T13:14:00+00:00', + ]); + + $volunteer = User::factory()->create(array_merge([ + 'api_token' => 'SENSITIVE_TOKEN_VALUE', + 'calendar_hash' => 'SENSITIVE_CAL_HASH', + 'latitude' => 51.5074, + 'longitude' => -0.1278, + 'recovery' => 'SENSITIVE_RECOVERY_TOKEN', + 'recovery_expires' => now()->addHour(), + ], $userOverrides)); + + EventsUsers::create([ + 'event' => $event->idevents, + 'user' => $volunteer->id, + 'status' => 1, + 'role' => Role::RESTARTER, + ]); + + return [$event, $volunteer]; + } + + private function assertNoSensitiveFields(string $content): void + { + foreach ($this->sensitiveFields as $field) { + $this->assertStringNotContainsString( + '"' . $field . '"', + $content, + "Sensitive field '$field' found in response" + ); + } + + $this->assertStringNotContainsString('SENSITIVE_TOKEN_VALUE', $content); + $this->assertStringNotContainsString('SENSITIVE_CAL_HASH', $content); + $this->assertStringNotContainsString('SENSITIVE_RECOVERY_TOKEN', $content); + } + + public function testPublicEventPageDoesNotLeakSensitiveUserFields(): void + { + [$event] = $this->createEventWithVolunteer(); + + $response = $this->get('/party/view/' . $event->idevents); + + $response->assertStatus(200); + $this->assertNoSensitiveFields($response->getContent()); + } + + public function testVolunteersApiDoesNotLeakSensitiveUserFields(): void + { + [$event] = $this->createEventWithVolunteer(); + + $admin = $this->createUserWithToken(Role::ADMINISTRATOR); + $this->actingAs($admin); + + $response = $this->get('/api/events/' . $event->idevents . '/volunteers'); + + $response->assertStatus(200); + $this->assertNoSensitiveFields($response->getContent()); + } +} From 5f8069281116af27260d2d663c3090f6e64805ef Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Fri, 5 Jun 2026 10:57:48 -0700 Subject: [PATCH 4/7] fix(security): drop loaded volunteer relation so allowlist actually applies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The expandVolunteers() allowlist added in 58a2e8e5 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. --- app/Models/Party.php | 16 +++-- .../Events/PublicEventDataLeakTest.php | 67 +++++++++++++++++-- 2 files changed, 74 insertions(+), 9 deletions(-) diff --git a/app/Models/Party.php b/app/Models/Party.php index c0963ca4b..d8cdf3939 100644 --- a/app/Models/Party.php +++ b/app/Models/Party.php @@ -825,11 +825,6 @@ public static function expandVolunteers($volunteers, $showEmails) { if ($volunteer->volunteer) { $user = $volunteer->volunteer; - $volunteer['volunteer'] = [ - 'id' => $user->id, - 'name' => $user->name, - 'email' => $showEmails ? $user->email : null, - ]; $volunteer['userSkills'] = $user->userSkills->all(); $volunteer['profilePath'] = '/uploads/thumbnail_'.$user->getProfile($user->id)->path; @@ -837,6 +832,17 @@ public static function expandVolunteers($volunteers, $showEmails) { foreach ($volunteer['userSkills'] as $skill) { $skill->skillName->skill_name; } + + // Drop the loaded Eloquent relation before overwriting it with the + // allowlist. Otherwise relationsToArray() re-injects the full User + // model during serialization, overriding the array set below and + // leaking the entire user record to public event pages. + $volunteer->unsetRelation('volunteer'); + $volunteer['volunteer'] = [ + 'id' => $user->id, + 'name' => $user->name, + 'email' => $showEmails ? $user->email : null, + ]; } $ret[] = $volunteer; diff --git a/tests/Feature/Events/PublicEventDataLeakTest.php b/tests/Feature/Events/PublicEventDataLeakTest.php index 718084a7c..50fd50da5 100644 --- a/tests/Feature/Events/PublicEventDataLeakTest.php +++ b/tests/Feature/Events/PublicEventDataLeakTest.php @@ -26,7 +26,10 @@ private function createEventWithVolunteer(array $userOverrides = []): array { Queue::fake(); - $group = Group::factory()->create(); + // The group must be approved, otherwise events on it are not visible to + // anonymous visitors (PartyController::view aborts with 404) and the + // public-page leak assertions below would never execute. + $group = Group::factory()->create(['approved' => true]); $network = Network::factory()->create(); $network->addGroup($group); @@ -43,6 +46,8 @@ private function createEventWithVolunteer(array $userOverrides = []): array 'longitude' => -0.1278, 'recovery' => 'SENSITIVE_RECOVERY_TOKEN', 'recovery_expires' => now()->addHour(), + 'username' => 'SENSITIVE_USERNAME', + 'external_user_id' => 'SENSITIVE_EXT_ID', ], $userOverrides)); EventsUsers::create([ @@ -70,14 +75,65 @@ private function assertNoSensitiveFields(string $content): void $this->assertStringNotContainsString('SENSITIVE_RECOVERY_TOKEN', $content); } + /** + * Assert that every embedded "volunteer" object is limited to the + * {id, name, email} allowlist. The full User model used to leak here + * because a loaded Eloquent relation overrides the allowlist array + * during serialization, so checking only the $hidden fields above is + * not enough — username, external ids, location, age, consent dates, + * etc. would still be exposed. + */ + private function assertVolunteerObjectsAllowlistOnly(string $content): void + { + // Props are json_encode()d then HTML-escaped by Blade ({{ }}), so + // decode entities before extracting the embedded JSON. + $decoded = html_entity_decode($content, ENT_QUOTES); + + $this->assertMatchesRegularExpression( + '/"volunteer":\{/', + $decoded, + 'No volunteer object was rendered; the allowlist assertions would be vacuous' + ); + + preg_match_all('/"volunteer":(\{[^}]*\})/', $decoded, $matches); + + foreach ($matches[1] as $json) { + $object = json_decode($json, true); + $this->assertIsArray($object, "Could not decode volunteer object: $json"); + + $keys = array_keys($object); + sort($keys); + + $this->assertSame( + ['email', 'id', 'name'], + $keys, + 'Volunteer object exposes more than the {id, name, email} allowlist: ' . $json + ); + } + + // Sentinel PII values that must never reach any serialized response. + $this->assertStringNotContainsString('SENSITIVE_USERNAME', $content); + $this->assertStringNotContainsString('SENSITIVE_EXT_ID', $content); + } + public function testPublicEventPageDoesNotLeakSensitiveUserFields(): void { - [$event] = $this->createEventWithVolunteer(); + [$event, $volunteer] = $this->createEventWithVolunteer(); $response = $this->get('/party/view/' . $event->idevents); $response->assertStatus(200); - $this->assertNoSensitiveFields($response->getContent()); + $content = $response->getContent(); + + $this->assertNoSensitiveFields($content); + $this->assertVolunteerObjectsAllowlistOnly($content); + + // Anonymous viewers must not see volunteer email addresses. + $this->assertStringNotContainsString( + $volunteer->email, + html_entity_decode($content, ENT_QUOTES), + 'Volunteer email address exposed to an anonymous viewer' + ); } public function testVolunteersApiDoesNotLeakSensitiveUserFields(): void @@ -90,6 +146,9 @@ public function testVolunteersApiDoesNotLeakSensitiveUserFields(): void $response = $this->get('/api/events/' . $event->idevents . '/volunteers'); $response->assertStatus(200); - $this->assertNoSensitiveFields($response->getContent()); + $content = $response->getContent(); + + $this->assertNoSensitiveFields($content); + $this->assertVolunteerObjectsAllowlistOnly($content); } } From 6897d60e2b1bd9f5b50f1a9f54b6f280628edb08 Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Fri, 5 Jun 2026 11:42:46 -0700 Subject: [PATCH 5/7] fix(events): keep user_skills in volunteer allowlist so skills still 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}. --- app/Models/Party.php | 4 ++++ tests/Feature/Events/PublicEventDataLeakTest.php | 6 ++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/Models/Party.php b/app/Models/Party.php index d8cdf3939..843b9f85d 100644 --- a/app/Models/Party.php +++ b/app/Models/Party.php @@ -842,6 +842,10 @@ public static function expandVolunteers($volunteers, $showEmails) { 'id' => $user->id, 'name' => $user->name, 'email' => $showEmails ? $user->email : null, + // The event page reads volunteer.user_skills (see + // EventAttendee.vue); keep it in the allowlist so the skills + // list still renders. Skill associations are not sensitive. + 'user_skills' => $volunteer['userSkills'], ]; } diff --git a/tests/Feature/Events/PublicEventDataLeakTest.php b/tests/Feature/Events/PublicEventDataLeakTest.php index 50fd50da5..54c885109 100644 --- a/tests/Feature/Events/PublicEventDataLeakTest.php +++ b/tests/Feature/Events/PublicEventDataLeakTest.php @@ -104,10 +104,12 @@ private function assertVolunteerObjectsAllowlistOnly(string $content): void $keys = array_keys($object); sort($keys); + // The test volunteer has no skills, so user_skills is an empty array + // and the volunteer object stays flat (the regex above assumes that). $this->assertSame( - ['email', 'id', 'name'], + ['email', 'id', 'name', 'user_skills'], $keys, - 'Volunteer object exposes more than the {id, name, email} allowlist: ' . $json + 'Volunteer object must be limited to the {id, name, email, user_skills} allowlist: ' . $json ); } From d0147dcfc2b65f859105726412c1e506cfe27eb1 Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Mon, 8 Jun 2026 08:18:15 -0700 Subject: [PATCH 6/7] test(security): close lat/long leak gap in PublicEventDataLeakTest 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. --- .../Events/PublicEventDataLeakTest.php | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/tests/Feature/Events/PublicEventDataLeakTest.php b/tests/Feature/Events/PublicEventDataLeakTest.php index 54c885109..b2da715ca 100644 --- a/tests/Feature/Events/PublicEventDataLeakTest.php +++ b/tests/Feature/Events/PublicEventDataLeakTest.php @@ -13,13 +13,16 @@ class PublicEventDataLeakTest extends TestCase { + // Field-name sentinels for keys that are user-only and must never be + // serialized. latitude/longitude are deliberately NOT listed here: they + // are legitimate public fields on the event itself (Party model), so the + // bare key appears in responses for the event's location. The volunteer's + // own coordinates are guarded by the numeric value sentinels below instead. private array $sensitiveFields = [ 'api_token', 'calendar_hash', 'recovery', 'recovery_expires', - 'latitude', - 'longitude', ]; private function createEventWithVolunteer(array $userOverrides = []): array @@ -42,8 +45,8 @@ private function createEventWithVolunteer(array $userOverrides = []): array $volunteer = User::factory()->create(array_merge([ 'api_token' => 'SENSITIVE_TOKEN_VALUE', 'calendar_hash' => 'SENSITIVE_CAL_HASH', - 'latitude' => 51.5074, - 'longitude' => -0.1278, + 'latitude' => 51.50741234, + 'longitude' => -0.12781234, 'recovery' => 'SENSITIVE_RECOVERY_TOKEN', 'recovery_expires' => now()->addHour(), 'username' => 'SENSITIVE_USERNAME', @@ -62,10 +65,16 @@ private function createEventWithVolunteer(array $userOverrides = []): array private function assertNoSensitiveFields(string $content): void { + // Props are json_encode()d then HTML-escaped by Blade ({{ }}) on the + // public page path, so the raw content contains "api_token" + // rather than "api_token". Decode entities first, otherwise these + // field-name checks are no-ops there. + $decoded = html_entity_decode($content, ENT_QUOTES); + foreach ($this->sensitiveFields as $field) { $this->assertStringNotContainsString( '"' . $field . '"', - $content, + $decoded, "Sensitive field '$field' found in response" ); } @@ -73,6 +82,12 @@ private function assertNoSensitiveFields(string $content): void $this->assertStringNotContainsString('SENSITIVE_TOKEN_VALUE', $content); $this->assertStringNotContainsString('SENSITIVE_CAL_HASH', $content); $this->assertStringNotContainsString('SENSITIVE_RECOVERY_TOKEN', $content); + + // latitude/longitude have no string token to match on, so assert on + // their distinctive numeric values. These guard against the coordinates + // leaking even if the allowlist assertion is ever weakened. + $this->assertStringNotContainsString('51.50741234', $content, 'Volunteer latitude leaked'); + $this->assertStringNotContainsString('0.12781234', $content, 'Volunteer longitude leaked'); } /** From 854bb5614192446eb2d34e10dd58fb0adcee33b3 Mon Sep 17 00:00:00 2001 From: Angel de la Torre Date: Mon, 8 Jun 2026 08:21:47 -0700 Subject: [PATCH 7/7] refactor(events): drop redundant top-level userSkills from expandVolunteers 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. --- app/Models/Party.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/Models/Party.php b/app/Models/Party.php index 843b9f85d..1447a3373 100644 --- a/app/Models/Party.php +++ b/app/Models/Party.php @@ -818,7 +818,6 @@ public static function expandVolunteers($volunteers, $showEmails) { $ret = []; foreach ($volunteers as $volunteer) { - $volunteer['userSkills'] = []; $volunteer['confirmed'] = intval($volunteer->status) === 1; $volunteer['profilePath'] = '/uploads/thumbnail_placeholder.png'; $volunteer['fullName'] = $volunteer->getFullName(); @@ -826,10 +825,10 @@ public static function expandVolunteers($volunteers, $showEmails) { if ($volunteer->volunteer) { $user = $volunteer->volunteer; - $volunteer['userSkills'] = $user->userSkills->all(); + $userSkills = $user->userSkills->all(); $volunteer['profilePath'] = '/uploads/thumbnail_'.$user->getProfile($user->id)->path; - foreach ($volunteer['userSkills'] as $skill) { + foreach ($userSkills as $skill) { $skill->skillName->skill_name; } @@ -845,7 +844,7 @@ public static function expandVolunteers($volunteers, $showEmails) { // The event page reads volunteer.user_skills (see // EventAttendee.vue); keep it in the allowlist so the skills // list still renders. Skill associations are not sensitive. - 'user_skills' => $volunteer['userSkills'], + 'user_skills' => $userSkills, ]; }