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..1447a3373 100644 --- a/app/Models/Party.php +++ b/app/Models/Party.php @@ -818,27 +818,34 @@ 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(); if ($volunteer->volunteer) { - $volunteer['volunteer'] = $volunteer->volunteer; + $user = $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; + $userSkills = $user->userSkills->all(); + $volunteer['profilePath'] = '/uploads/thumbnail_'.$user->getProfile($user->id)->path; - foreach ($volunteer['userSkills'] as $skill) { - // Force expansion - $skill->skillName->skill_name; - } + foreach ($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, + // 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' => $userSkills, + ]; } $ret[] = $volunteer; 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', ]; /** diff --git a/tests/Feature/Events/PublicEventDataLeakTest.php b/tests/Feature/Events/PublicEventDataLeakTest.php new file mode 100644 index 000000000..b2da715ca --- /dev/null +++ b/tests/Feature/Events/PublicEventDataLeakTest.php @@ -0,0 +1,171 @@ +create(['approved' => true]); + $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.50741234, + 'longitude' => -0.12781234, + 'recovery' => 'SENSITIVE_RECOVERY_TOKEN', + 'recovery_expires' => now()->addHour(), + 'username' => 'SENSITIVE_USERNAME', + 'external_user_id' => 'SENSITIVE_EXT_ID', + ], $userOverrides)); + + EventsUsers::create([ + 'event' => $event->idevents, + 'user' => $volunteer->id, + 'status' => 1, + 'role' => Role::RESTARTER, + ]); + + return [$event, $volunteer]; + } + + 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 . '"', + $decoded, + "Sensitive field '$field' found in response" + ); + } + + $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'); + } + + /** + * 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); + + // 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', 'user_skills'], + $keys, + 'Volunteer object must be limited to the {id, name, email, user_skills} 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, $volunteer] = $this->createEventWithVolunteer(); + + $response = $this->get('/party/view/' . $event->idevents); + + $response->assertStatus(200); + $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 + { + [$event] = $this->createEventWithVolunteer(); + + $admin = $this->createUserWithToken(Role::ADMINISTRATOR); + $this->actingAs($admin); + + $response = $this->get('/api/events/' . $event->idevents . '/volunteers'); + + $response->assertStatus(200); + $content = $response->getContent(); + + $this->assertNoSensitiveFields($content); + $this->assertVolunteerObjectsAllowlistOnly($content); + } +}