Fix volunteer PII leak on public event pages

app/Http/Controllers/API/EventController.php

@@ -244,8 +244,8 @@ public function listVolunteers(Request $request, $idevents): JsonResponse
         // Get the user that the API has been authenticated as.
         $user = auth('api')->user();
 
-        // Emails are sensitive.
-        $showEmails = $user && !Fixometer::userHasEditPartyPermission($idevents, $user->id);
+        // Only show emails to users who have edit permission on this event.
+        $showEmails = $user && Fixometer::userHasEditPartyPermission($idevents, $user->id);
         $volunteers = $party->expandVolunteers($party->allConfirmedVolunteers()->get(), $showEmails);
 
         return response()->json([

app/Http/Controllers/API/UserController.php

@@ -73,6 +73,8 @@ protected static function mapUserAndAuditToUserChange($user, $audit)
             'access_group_tag_id',
             'mediawiki',
             'wiki_sync_status',
+            'latitude',
+            'longitude',
         ]);
 
         $userChange = $user->toArray();

app/Http/Controllers/ApiController.php

@@ -178,7 +178,9 @@ public static function getUserInfo(): JsonResponse
     {
         $user = Auth::user();
 
-        $user->makeHidden('api_token');
+        // api_token and other credentials are in User::$hidden; makeHidden() is
+        // a belt-and-suspenders guard for any future $hidden regression.
+        $user->makeHidden(['api_token', 'calendar_hash', 'recovery', 'recovery_expires', 'mediawiki', 'latitude', 'longitude']);
 
         return response()->json($user->toArray());
     }

app/Party.php

@@ -819,21 +819,34 @@ public static function expandVolunteers($volunteers, $showEmails) {
             $volunteer['fullName'] = $volunteer->getFullName();
 
             if ($volunteer->volunteer) {
-                $volunteer['volunteer'] = $volunteer->volunteer;
+                $user = $volunteer->volunteer;
 
-                if (!$showEmails) {
-                    $volunteer['volunteer']['email'] = NULL;
-                }
+                $safeUser = [
+                    'id'     => $user->id,
+                    'name'   => $user->name,
+                    'email'  => $showEmails ? $user->email : null,
+                    'user_skills' => [],
+                ];
 
-                if (! empty($volunteer->volunteer)) {
-                    $volunteer['userSkills'] = $volunteer->volunteer->userSkills->all();
-                    $volunteer['profilePath'] = '/uploads/thumbnail_'.$volunteer->volunteer->getProfile($volunteer->volunteer->id)->path;
+                $volunteer['profilePath'] = '/uploads/thumbnail_'.$user->getProfile($user->id)->path;
 
-                    foreach ($volunteer['userSkills'] as $skill) {
-                        // Force expansion
-                        $skill->skillName->skill_name;
+                foreach ($user->userSkills as $skill) {
+                    // Guard against orphaned FK rows (deleted skill record).
+                    if (!$skill->skillName) {
+                        continue;
                     }
+                    // Emit only the shape the frontend expects: { skill_name: { skill_name: "…" } }
+                    $safeUser['user_skills'][] = [
+                        'skill_name' => ['skill_name' => $skill->skillName->skill_name],
+                    ];
                 }
+
+                // Unset the loaded relation before assigning the safe array.
+                // Eloquent's toArray() merges relationsToArray() LAST, so without this
+                // the full User model from the 'volunteer' HasOne would overwrite $safeUser
+                // when the blade calls json_encode($expanded_attended).
+                $volunteer->unsetRelation('volunteer');
+                $volunteer['volunteer'] = $safeUser;
             }
 
             $ret[] = $volunteer;

app/User.php

@@ -54,6 +54,9 @@ class User extends Authenticatable implements Auditable, HasLocalePreference
      */
     protected $hidden = [
         'password', 'remember_token',
+        // Credentials and PII that must never appear in serialised API responses or page HTML.
+        'api_token', 'calendar_hash', 'recovery', 'recovery_expires',
+        'mediawiki', 'latitude', 'longitude',
     ];
 
     /**
@@ -65,6 +68,13 @@ class User extends Authenticatable implements Auditable, HasLocalePreference
         'number_of_logins',
         'last_login_at',
         'remember_token',
+        'password',
+        'api_token',
+        'calendar_hash',
+        'recovery',
+        'recovery_expires',
+        'latitude',
+        'longitude',
     ];
 
     /**

tests/Feature/Events/VolunteerPiiTest.php

@@ -0,0 +1,144 @@
+<?php
+
+namespace Tests\Feature\Events;
+
+use App\EventsUsers;
+use App\Group;
+use App\Party;
+use App\User;
+use Tests\TestCase;
+
+/**
+ * Regression tests for the volunteer PII leak:
+ * Party::expandVolunteers() previously serialised the full User Eloquent model
+ * into the event page HTML, exposing api_token, calendar_hash, lat/lng, and
+ * recovery tokens to anonymous visitors.
+ */
+class VolunteerPiiTest extends TestCase
+{
+    /** Sensitive field values injected into the volunteer user. */
+    private string $apiToken     = 'SENTINEL_API_TOKEN_SECRET';
+    private string $calendarHash = 'SENTINEL_CALENDAR_HASH';
+    private string $recovery     = 'SENTINEL_RECOVERY_TOKEN';
+    private float  $latitude     = 51.123456;
+    private float  $longitude    = -0.123456;
+
+    private function makeEventWithVolunteer(): array
+    {
+        $group     = Group::factory()->create(['approved' => true]);
+        $event     = Party::factory()->create([
+            'group'           => $group,
+            'event_start_utc' => '2130-01-01T10:00:00+00:00',
+            'event_end_utc'   => '2130-01-01T12:00:00+00:00',
+        ]);
+        $volunteer = User::factory()->restarter()->create();
+
+        // Bypass $fillable to inject the sentinel values directly.
+        \DB::table('users')->where('id', $volunteer->id)->update([
+            'api_token'     => $this->apiToken,
+            'calendar_hash' => $this->calendarHash,
+            'recovery'      => $this->recovery,
+            'latitude'      => $this->latitude,
+            'longitude'     => $this->longitude,
+        ]);
+
+        // Confirmed attendance record (status 1 = confirmed).
+        EventsUsers::create([
+            'event'  => $event->idevents,
+            'user'   => $volunteer->id,
+            'status' => 1,
+            'role'   => 3, // Restarter
+        ]);
+
+        return [$event, $volunteer];
+    }
+
+    public function testEventPageDoesNotLeakVolunteerCredentials(): void
+    {
+        [$event, $volunteer] = $this->makeEventWithVolunteer();
+
+        // Anonymous GET — this is the attack surface.
+        $response = $this->get('/party/view/' . $event->idevents);
+        $response->assertSuccessful();
+        $html = $response->getContent();
+
+        // Sensitive fields must not appear anywhere in the page.
+        $this->assertStringNotContainsString($this->apiToken,     $html, 'api_token must not appear in event page HTML');
+        $this->assertStringNotContainsString($this->calendarHash, $html, 'calendar_hash must not appear in event page HTML');
+        $this->assertStringNotContainsString($this->recovery,     $html, 'recovery token must not appear in event page HTML');
+        $this->assertStringNotContainsString((string) $this->latitude,  $html, 'latitude must not appear in event page HTML');
+        $this->assertStringNotContainsString((string) $this->longitude, $html, 'longitude must not appear in event page HTML');
+
+        // Sanity: volunteer's name must still be present (attendee list is not broken).
+        $this->assertStringContainsString($volunteer->name, $html, 'Volunteer name must still appear on event page');
+    }
+
+    public function testUserModelHiddenFieldsExcludeCredentials(): void
+    {
+        $user = User::factory()->restarter()->create();
+
+        \DB::table('users')->where('id', $user->id)->update([
+            'api_token'       => $this->apiToken,
+            'calendar_hash'   => $this->calendarHash,
+            'recovery'        => $this->recovery,
+            'recovery_expires'=> '2099-01-01',
+            'latitude'        => $this->latitude,
+            'longitude'       => $this->longitude,
+        ]);
+
+        $user->refresh();
+        $array = $user->toArray();
+
+        $this->assertArrayNotHasKey('api_token',        $array, 'api_token must be in $hidden');
+        $this->assertArrayNotHasKey('calendar_hash',    $array, 'calendar_hash must be in $hidden');
+        $this->assertArrayNotHasKey('recovery',         $array, 'recovery must be in $hidden');
+        $this->assertArrayNotHasKey('recovery_expires', $array, 'recovery_expires must be in $hidden');
+        $this->assertArrayNotHasKey('latitude',         $array, 'latitude must be in $hidden');
+        $this->assertArrayNotHasKey('longitude',        $array, 'longitude must be in $hidden');
+        $this->assertArrayNotHasKey('mediawiki',        $array, 'mediawiki must be in $hidden');
+
+        // Non-sensitive fields must still be present.
+        $this->assertArrayHasKey('name',  $array);
+        $this->assertArrayHasKey('email', $array);
+    }
+
+    public function testExpandVolunteersEmitsSafeShape(): void
+    {
+        [$event, $volunteer] = $this->makeEventWithVolunteer();
+
+        $volunteers = $event->allConfirmedVolunteers()->get();
+        $expanded   = Party::expandVolunteers($volunteers, false);
+
+        $this->assertNotEmpty($expanded, 'expandVolunteers must return the volunteer');
+
+        // Decode via json_encode/decode — this is the actual attack surface (blade page HTML).
+        // PHP array-access reads $attributes, but json_encode calls toArray() which merges
+        // relationsToArray() LAST, so a still-loaded 'volunteer' HasOne relation would
+        // silently overwrite the safe array.  Decoding the JSON catches that bypass.
+        $decoded = json_decode(json_encode($expanded), true);
+        $ev = $decoded[0];
+
+        // The nested 'volunteer' key must be an array (not an Eloquent model).
+        $this->assertIsArray($ev['volunteer'], "'volunteer' must be a plain array, not an Eloquent model");
+
+        // Safe fields present.
+        $this->assertArrayHasKey('id',   $ev['volunteer']);
+        $this->assertArrayHasKey('name', $ev['volunteer']);
+
+        // Sensitive fields absent — checked in the JSON-decoded output (real attack surface).
+        $this->assertArrayNotHasKey('api_token',     $ev['volunteer'], 'api_token must not be in expanded volunteer JSON');
+        $this->assertArrayNotHasKey('calendar_hash', $ev['volunteer'], 'calendar_hash must not be in expanded volunteer JSON');
+        $this->assertArrayNotHasKey('recovery',      $ev['volunteer'], 'recovery must not be in expanded volunteer JSON');
+        $this->assertArrayNotHasKey('latitude',      $ev['volunteer'], 'latitude must not be in expanded volunteer JSON');
+        $this->assertArrayNotHasKey('longitude',     $ev['volunteer'], 'longitude must not be in expanded volunteer JSON');
+
+        // Sentinel values must not appear anywhere in the raw JSON string.
+        $json = json_encode($expanded);
+        $this->assertStringNotContainsString($this->apiToken,     $json, 'api_token sentinel must not appear in JSON output');
+        $this->assertStringNotContainsString($this->calendarHash, $json, 'calendar_hash sentinel must not appear in JSON output');
+        $this->assertStringNotContainsString($this->recovery,     $json, 'recovery sentinel must not appear in JSON output');
+
+        // Email must be null when showEmails=false.
+        $this->assertNull($ev['volunteer']['email'], 'email must be null when showEmails is false');
+    }
+}

tests/Feature/Users/UserAuditExcludeTest.php

@@ -0,0 +1,65 @@
+<?php
+
+namespace Tests\Feature\Users;
+
+use App\User;
+use Tests\TestCase;
+
+/**
+ * Regression test: credential and PII fields must never be written to the
+ * audit log, regardless of whether they change on a user update.
+ */
+class UserAuditExcludeTest extends TestCase
+{
+    private array $sensitiveFields = [
+        'api_token',
+        'calendar_hash',
+        'recovery',
+        'recovery_expires',
+        'latitude',
+        'longitude',
+        'password',
+    ];
+
+    public function testCredentialFieldsAreNotWrittenToAuditLog(): void
+    {
+        $user = User::factory()->restarter()->create();
+
+        // Trigger an audit by updating a sensitive field alongside a benign one.
+        \DB::table('users')->where('id', $user->id)->update([
+            'api_token'     => 'NEW_API_TOKEN',
+            'calendar_hash' => 'NEW_CALENDAR_HASH',
+            'recovery'      => 'NEW_RECOVERY',
+            'name'          => 'Updated Name',
+        ]);
+
+        // Re-fetch via Eloquent so the auditing trait fires on save.
+        $user->refresh();
+        $user->name = 'Triggered Audit';
+        $user->save();
+
+        $audits = \OwenIt\Auditing\Models\Audit::where('auditable_type', User::class)
+            ->where('auditable_id', $user->id)
+            ->get();
+
+        $this->assertNotEmpty($audits, 'At least one audit record should exist');
+
+        foreach ($audits as $audit) {
+            $oldValues = $audit->old_values ?? [];
+            $newValues = $audit->new_values ?? [];
+
+            foreach ($this->sensitiveFields as $field) {
+                $this->assertArrayNotHasKey(
+                    $field,
+                    $oldValues,
+                    "Field '$field' must not appear in audit old_values"
+                );
+                $this->assertArrayNotHasKey(
+                    $field,
+                    $newValues,
+                    "Field '$field' must not appear in audit new_values"
+                );
+            }
+        }
+    }
+}