From 310569d764e185b6360da1356a308e2fb6f535c0 Mon Sep 17 00:00:00 2001 From: smarcet Date: Fri, 7 Aug 2026 11:05:35 -0300 Subject: [PATCH 1/7] feat(auth): implement Facebook data deletion callback and backfill command Adds the Data Deletion Request Callback Facebook requires apps to implement (https://developers.facebook.com/documentation/development/create-an-app/app-dashboard/data-deletion-callback): - POST /api/public/v1/facebook/data-deletion verifies the HMAC-SHA256 signed_request, unlinks the matching user's Facebook identity (external_id/external_provider/external_pic), and returns the required {url, confirmation_code} JSON. - GET /api/public/v1/facebook/data-deletion/status/{code} renders a human-readable status page, per Facebook's spec. - idp:facebook-data-deletion-backfill processes the CSV of pending app-scoped IDs already queued in the app dashboard. FacebookDataDeletionService::processDeletionRequest() is idempotent: a (provider, external_id) unique constraint plus a re-select-on-collision fallback ensure a duplicate submission (Facebook retry, or an ID already handled by the live callback later appearing in a CSV backfill) never re-processes the user or creates a duplicate audit row. All facebook_deletion_requests reads/writes route through the same Doctrine connection the entity flush uses, resolved fresh per call, so the audit row and the user unlink commit or roll back together. --- .../Commands/FacebookDataDeletionBackfill.php | 103 +++++++++++ app/Console/Kernel.php | 1 + .../Api/FacebookDataDeletionController.php | 91 ++++++++++ app/Repositories/DoctrineUserRepository.php | 18 ++ .../Auth/FacebookDataDeletionService.php | 165 ++++++++++++++++++ .../Auth/IFacebookDataDeletionService.php | 37 ++++ app/Services/ServicesProvider.php | 4 + app/libs/Auth/FacebookSignedRequestParser.php | 58 ++++++ app/libs/Auth/Models/User.php | 16 +- .../Auth/Repositories/IUserRepository.php | 7 + database/migrations/Version20260806190000.php | 56 ++++++ .../facebook_data_deletion_status.blade.php | 22 +++ routes/api_public.php | 9 + tests/FacebookDataDeletionApiTest.php | 150 ++++++++++++++++ ...acebookDataDeletionBackfillCommandTest.php | 95 ++++++++++ .../FacebookDataDeletionServiceRaceTest.php | 80 +++++++++ .../unit/FacebookSignedRequestParserTest.php | 95 ++++++++++ 17 files changed, 999 insertions(+), 8 deletions(-) create mode 100644 app/Console/Commands/FacebookDataDeletionBackfill.php create mode 100644 app/Http/Controllers/Api/FacebookDataDeletionController.php create mode 100644 app/Services/Auth/FacebookDataDeletionService.php create mode 100644 app/Services/Auth/IFacebookDataDeletionService.php create mode 100644 app/libs/Auth/FacebookSignedRequestParser.php create mode 100644 database/migrations/Version20260806190000.php create mode 100644 resources/views/auth/facebook_data_deletion_status.blade.php create mode 100644 tests/FacebookDataDeletionApiTest.php create mode 100644 tests/FacebookDataDeletionBackfillCommandTest.php create mode 100644 tests/unit/FacebookDataDeletionServiceRaceTest.php create mode 100644 tests/unit/FacebookSignedRequestParserTest.php diff --git a/app/Console/Commands/FacebookDataDeletionBackfill.php b/app/Console/Commands/FacebookDataDeletionBackfill.php new file mode 100644 index 00000000..24580733 --- /dev/null +++ b/app/Console/Commands/FacebookDataDeletionBackfill.php @@ -0,0 +1,103 @@ +service = $service; + } + + /** + * @return int + */ + public function handle() + { + $path = $this->argument('path'); + + if (!is_readable($path)) { + $this->error(sprintf("File %s is not readable.", $path)); + return 1; + } + + $matched = 0; + $not_found = 0; + $skipped = 0; + + $handle = fopen($path, 'r'); + while (($line = fgets($handle)) !== false) { + $external_id = trim($line, " \t\n\r\0\x0B\"'"); + + if ($external_id === '') { + $skipped++; + continue; + } + + $result = $this->service->processDeletionRequest($external_id); + + if ($result['status'] === 'completed') { + $matched++; + } else { + $not_found++; + } + + Log::debug(sprintf("FacebookDataDeletionBackfill::handle processed %s -> %s", $external_id, $result['confirmation_code'])); + } + fclose($handle); + + $this->info(sprintf("Processed CSV: %d matched, %d not found, %d skipped blank lines.", $matched, $not_found, $skipped)); + return 0; + } +} diff --git a/app/Console/Kernel.php b/app/Console/Kernel.php index 89bf376a..cdabd3d2 100644 --- a/app/Console/Kernel.php +++ b/app/Console/Kernel.php @@ -29,6 +29,7 @@ class Kernel extends ConsoleKernel Commands\CleanOAuth2StaleData::class, Commands\CleanOpenIdStaleData::class, Commands\CreateSuperAdmin::class, + Commands\FacebookDataDeletionBackfill::class, Commands\SpammerProcess\RebuildUserSpammerEstimator::class, Commands\SpammerProcess\UserSpammerProcessor::class, ]; diff --git a/app/Http/Controllers/Api/FacebookDataDeletionController.php b/app/Http/Controllers/Api/FacebookDataDeletionController.php new file mode 100644 index 00000000..f66135d9 --- /dev/null +++ b/app/Http/Controllers/Api/FacebookDataDeletionController.php @@ -0,0 +1,91 @@ +service = $service; + } + + /** + * @param Request $request + * @return \Illuminate\Http\JsonResponse + */ + public function handle(Request $request) + { + $signed_request = $request->input('signed_request'); + + if (empty($signed_request)) { + Log::warning("FacebookDataDeletionController::handle missing signed_request"); + return response()->json([ + 'error' => ['code' => 'invalid_request', 'message' => 'signed_request is required.'] + ], 400); + } + + $secret = Config::get('services.facebook.client_secret'); + $data = FacebookSignedRequestParser::parse($signed_request, $secret); + + if (is_null($data)) { + Log::warning("FacebookDataDeletionController::handle invalid signed_request"); + return response()->json([ + 'error' => ['code' => 'invalid_signature', 'message' => 'Invalid signed_request.'] + ], 400); + } + + $result = $this->service->processDeletionRequest((string)$data['user_id']); + + return response()->json([ + 'url' => $result['url'], + 'confirmation_code' => $result['confirmation_code'], + ], 200); + } + + /** + * @param Request $request + * @param string $confirmation_code + * @return \Illuminate\Contracts\View\View + */ + public function status(Request $request, string $confirmation_code) + { + $result = $this->service->getStatus($confirmation_code); + + if (is_null($result)) { + abort(404); + } + + return view('auth.facebook_data_deletion_status', ['result' => $result]); + } +} diff --git a/app/Repositories/DoctrineUserRepository.php b/app/Repositories/DoctrineUserRepository.php index f6f8e8e3..408a3f15 100644 --- a/app/Repositories/DoctrineUserRepository.php +++ b/app/Repositories/DoctrineUserRepository.php @@ -140,4 +140,22 @@ public function getByIdWithGroups(int $id): ?User return $qb->getQuery()->getOneOrNullResult(); } + + /** + * @param string $provider + * @param string $external_id + * @return User|null + */ + public function getByExternalId(string $provider, string $external_id): ?User + { + return $this->getEntityManager() + ->createQueryBuilder() + ->select("e") + ->from($this->getBaseEntity(), "e") + ->Where("e.external_provider = :provider AND e.external_id = :external_id") + ->setParameter("provider", $provider) + ->setParameter("external_id", $external_id) + ->getQuery() + ->getOneOrNullResult(); + } } \ No newline at end of file diff --git a/app/Services/Auth/FacebookDataDeletionService.php b/app/Services/Auth/FacebookDataDeletionService.php new file mode 100644 index 00000000..1153b4d2 --- /dev/null +++ b/app/Services/Auth/FacebookDataDeletionService.php @@ -0,0 +1,165 @@ +user_repository = $user_repository; + } + + /** + * Resolved fresh on every call (never cached on the instance), matching + * DoctrineRepository::getEntityManager() - DoctrineTransactionService::transaction() + * can swap in a new EntityManager mid-request on a retryable connection error + * (Registry::resetManager()), and a cached reference would then point at a + * closed connection. + * @return EntityManagerInterface + */ + private function getEntityManager(): EntityManagerInterface + { + return Registry::getManager(BaseEntity::EntityManager); + } + + /** + * @inheritDoc + */ + public function processDeletionRequest(string $external_id, string $provider = 'facebook'): array + { + return $this->tx_service->transaction(function () use ($external_id, $provider) { + + $existing = $this->findRequest($provider, $external_id); + if (!is_null($existing)) { + return $this->toResult($existing); + } + + $user = $this->user_repository->getByExternalId($provider, $external_id); + + $status = self::StatusNotFound; + $user_id = null; + + if (!is_null($user)) { + $user->setExternalId(null); + $user->setExternalProvider(null); + $user->setExternalPic(null); + $user_id = $user->getId(); + $status = self::StatusCompleted; + } + + $confirmation_code = (new RandomGenerator())->randomToken('sha256'); + + // Inserted through the same Doctrine DBAL connection the user-entity + // flush below uses (via $this->tx_service->transaction()), so both + // writes commit or roll back together - using Laravel's separately + // autocommitting `DB` facade connection here would let this row + // persist even if the flush that actually nulls the user's fields + // fails afterward. + try { + $this->getEntityManager()->getConnection()->insert(self::Table, [ + 'provider' => $provider, + 'external_id' => $external_id, + 'confirmation_code' => $confirmation_code, + 'status' => $status, + 'user_id' => $user_id, + 'created_at' => now()->format('Y-m-d H:i:s'), + 'updated_at' => now()->format('Y-m-d H:i:s'), + ]); + } catch (UniqueConstraintViolationException $ex) { + // a concurrent request already inserted the row for this + // (provider, external_id) pair before this one committed - + // two independent statements racing the same unique index. + // Treat this as the idempotent path instead of failing the request. + $existing = $this->findRequest($provider, $external_id); + if (!is_null($existing)) { + return $this->toResult($existing); + } + throw $ex; + } + + return $this->toResult((object)[ + 'confirmation_code' => $confirmation_code, + 'status' => $status, + ]); + }); + } + + /** + * @inheritDoc + */ + public function getStatus(string $confirmation_code): ?array + { + $row = $this->getEntityManager()->getConnection()->fetchAssociative( + 'SELECT * FROM ' . self::Table . ' WHERE confirmation_code = ?', + [$confirmation_code] + ); + return $row === false ? null : $row; + } + + /** + * @param string $provider + * @param string $external_id + * @return object|null + */ + private function findRequest(string $provider, string $external_id) + { + $row = $this->getEntityManager()->getConnection()->fetchAssociative( + 'SELECT * FROM ' . self::Table . ' WHERE provider = ? AND external_id = ?', + [$provider, $external_id] + ); + return $row === false ? null : (object)$row; + } + + /** + * @param object $row + * @return array + */ + private function toResult(object $row): array + { + return [ + 'confirmation_code' => $row->confirmation_code, + 'status' => $row->status, + 'url' => URL::route('facebook_data_deletion_status', ['confirmation_code' => $row->confirmation_code]), + ]; + } +} diff --git a/app/Services/Auth/IFacebookDataDeletionService.php b/app/Services/Auth/IFacebookDataDeletionService.php new file mode 100644 index 00000000..e8e8744b --- /dev/null +++ b/app/Services/Auth/IFacebookDataDeletionService.php @@ -0,0 +1,37 @@ +external_provider = $external_provider; } @@ -2207,25 +2207,25 @@ public function getExternalPic(): ?string } /** - * @param string $external_pic + * @param string|null $external_pic */ - public function setExternalPic(string $external_pic): void + public function setExternalPic(?string $external_pic): void { $this->external_pic = $external_pic; } /** - * @return string + * @return string|null */ - public function getExternalId(): string + public function getExternalId(): ?string { return $this->external_id; } /** - * @param string $external_id + * @param string|null $external_id */ - public function setExternalId(string $external_id): void + public function setExternalId(?string $external_id): void { $this->external_id = $external_id; } diff --git a/app/libs/Auth/Repositories/IUserRepository.php b/app/libs/Auth/Repositories/IUserRepository.php index acf4d316..0127cec9 100644 --- a/app/libs/Auth/Repositories/IUserRepository.php +++ b/app/libs/Auth/Repositories/IUserRepository.php @@ -45,4 +45,11 @@ public function getByIdentifier($user_identifier):?User; public function getByVerificationEmailToken(string $token):?User; public function getByIdWithGroups(int $id): ?User; + + /** + * @param string $provider + * @param string $external_id + * @return User|null + */ + public function getByExternalId(string $provider, string $external_id): ?User; } \ No newline at end of file diff --git a/database/migrations/Version20260806190000.php b/database/migrations/Version20260806190000.php new file mode 100644 index 00000000..fea3b60a --- /dev/null +++ b/database/migrations/Version20260806190000.php @@ -0,0 +1,56 @@ +hasTable("facebook_deletion_requests")) { + $builder->create("facebook_deletion_requests", function (Table $table) { + $table->increments('id'); + $table->timestamps(); + $table->string("provider")->setNotnull(true); + $table->string("external_id")->setNotnull(true); + $table->string("confirmation_code")->setNotnull(true); + $table->string("status")->setNotnull(true); + $table->integer("user_id")->setNotnull(false); + $table->unique(["provider", "external_id"]); + $table->unique("confirmation_code"); + }); + } + } + + /** + * @param Schema $schema + */ + public function down(Schema $schema):void + { + $builder = new Builder($schema); + + $builder->dropIfExists("facebook_deletion_requests"); + } +} diff --git a/resources/views/auth/facebook_data_deletion_status.blade.php b/resources/views/auth/facebook_data_deletion_status.blade.php new file mode 100644 index 00000000..cfa8d53d --- /dev/null +++ b/resources/views/auth/facebook_data_deletion_status.blade.php @@ -0,0 +1,22 @@ +@extends('layout') +@section('title') + Welcome to {{ Config::get("app.app_name") }} - Facebook Data Deletion Status +@append +@section('scripts') + +@append + +@section('content') +
+
+

Facebook Data Deletion Request Status

+

Confirmation code: {{ $result['confirmation_code'] }}

+ @if($result['status'] === 'completed') +

Your Facebook-linked data has been deleted.

+ @else +

No data was found for this identifier.

+ @endif +
+
+@endsection diff --git a/routes/api_public.php b/routes/api_public.php index c3fcd7b5..82b4b2c0 100644 --- a/routes/api_public.php +++ b/routes/api_public.php @@ -12,6 +12,7 @@ * limitations under the License. **/ +use App\Http\Controllers\Api\FacebookDataDeletionController; use Illuminate\Support\Facades\Route; /* @@ -48,3 +49,11 @@ return response()->json($versionData, 200); }); + +Route::post('/facebook/data-deletion', [FacebookDataDeletionController::class, 'handle']) + ->middleware('throttle:60,1') + ->name('facebook_data_deletion_callback'); + +Route::get('/facebook/data-deletion/status/{confirmation_code}', [FacebookDataDeletionController::class, 'status']) + ->middleware('throttle:60,1') + ->name('facebook_data_deletion_status'); diff --git a/tests/FacebookDataDeletionApiTest.php b/tests/FacebookDataDeletionApiTest.php new file mode 100644 index 00000000..e51f27b5 --- /dev/null +++ b/tests/FacebookDataDeletionApiTest.php @@ -0,0 +1,150 @@ +delete(); + } + + private function buildSignedRequest(string $user_id): string + { + $secret = Config::get('services.facebook.client_secret'); + $payload = ['algorithm' => 'HMAC-SHA256', 'user_id' => $user_id]; + $encoded_payload = $this->base64UrlEncode(json_encode($payload)); + $sig = hash_hmac('sha256', $encoded_payload, $secret, true); + $encoded_sig = $this->base64UrlEncode($sig); + return $encoded_sig . '.' . $encoded_payload; + } + + private function base64UrlEncode(string $input): string + { + return rtrim(strtr(base64_encode($input), '+/', '-_'), '='); + } + + private function linkSeededUserToFacebook(string $asid): User + { + $user_repository = EntityManager::getRepository(User::class); + $user = $user_repository->findOneBy(["identifier" => 'sebastian.marcet']); + $user->setExternalId($asid); + $user->setExternalProvider('facebook'); + $user->setExternalPic('https://graph.facebook.com/pic.jpg'); + EntityManager::persist($user); + EntityManager::flush(); + return $user; + } + + public function testMatchedUserIsUnlinkedAndReturnsConfirmation(): void + { + $asid = '218471001'; + $user = $this->linkSeededUserToFacebook($asid); + $sr = $this->buildSignedRequest($asid); + + $response = $this->post(self::CallbackUri, ['signed_request' => $sr]); + + $this->assertResponseStatus(200); + $json = json_decode($response->response->getContent(), true); + $this->assertNotEmpty($json['confirmation_code']); + $this->assertNotEmpty($json['url']); + + $reloaded = EntityManager::getRepository(User::class)->getById($user->getId()); + $this->assertNull($reloaded->getExternalId()); + $this->assertNull($reloaded->getExternalProvider()); + $this->assertNull($reloaded->getExternalPic()); + + $row = DB::table('facebook_deletion_requests')->where('external_id', $asid)->first(); + $this->assertNotNull($row); + $this->assertSame('completed', $row->status); + $this->assertSame($user->getId(), $row->user_id); + } + + public function testUnmatchedAsidReturns200AndPersistsNotFoundRow(): void + { + $asid = 'no-such-user-999999'; + $sr = $this->buildSignedRequest($asid); + + $response = $this->post(self::CallbackUri, ['signed_request' => $sr]); + + $this->assertResponseStatus(200); + $json = json_decode($response->response->getContent(), true); + $this->assertNotEmpty($json['confirmation_code']); + + $row = DB::table('facebook_deletion_requests')->where('external_id', $asid)->first(); + $this->assertNotNull($row); + $this->assertSame('not_found', $row->status); + $this->assertNull($row->user_id); + } + + public function testTamperedSignatureReturns400(): void + { + $sr = $this->buildSignedRequest('218471001'); + [, $payload] = explode('.', $sr, 2); + $tampered = 'AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA.' . $payload; + + $response = $this->post(self::CallbackUri, ['signed_request' => $tampered]); + + $this->assertResponseStatus(400); + } + + public function testStatusPageForRealConfirmationCodeReturns200(): void + { + $asid = '218471002'; + $sr = $this->buildSignedRequest($asid); + $response = $this->post(self::CallbackUri, ['signed_request' => $sr]); + $json = json_decode($response->response->getContent(), true); + $confirmation_code = $json['confirmation_code']; + + $status_response = $this->get(self::CallbackUri . '/status/' . $confirmation_code); + + $this->assertResponseStatus(200); + $status_response->see($confirmation_code); + } + + public function testStatusPageForUnknownCodeReturns404(): void + { + $this->get(self::CallbackUri . '/status/does-not-exist'); + + $this->assertResponseStatus(404); + } + + public function testDuplicateSubmissionIsIdempotent(): void + { + $asid = '218471003'; + $sr = $this->buildSignedRequest($asid); + + $first = $this->post(self::CallbackUri, ['signed_request' => $sr]); + $first_json = json_decode($first->response->getContent(), true); + + $second = $this->post(self::CallbackUri, ['signed_request' => $sr]); + $second_json = json_decode($second->response->getContent(), true); + + $this->assertResponseStatus(200); + $this->assertSame($first_json['confirmation_code'], $second_json['confirmation_code']); + + $count = DB::table('facebook_deletion_requests')->where('external_id', $asid)->count(); + $this->assertSame(1, $count); + } +} diff --git a/tests/FacebookDataDeletionBackfillCommandTest.php b/tests/FacebookDataDeletionBackfillCommandTest.php new file mode 100644 index 00000000..b7b6f1e8 --- /dev/null +++ b/tests/FacebookDataDeletionBackfillCommandTest.php @@ -0,0 +1,95 @@ +delete(); + } + + private function linkSeededUserToFacebook(string $asid): User + { + $user_repository = EntityManager::getRepository(User::class); + $user = $user_repository->findOneBy(["identifier" => 'sebastian.marcet']); + $user->setExternalId($asid); + $user->setExternalProvider('facebook'); + EntityManager::persist($user); + EntityManager::flush(); + return $user; + } + + private function writeCsvFixture(array $lines): string + { + $path = tempnam(sys_get_temp_dir(), 'fb_deletion_csv_'); + file_put_contents($path, implode("\n", $lines) . "\n"); + return $path; + } + + public function testBackfillUnlinksMatchedAndRecordsUnmatched(): void + { + $matched_asid = '555000111'; + $unmatched_asid = '555000222'; + $user = $this->linkSeededUserToFacebook($matched_asid); + + $csv_path = $this->writeCsvFixture([$matched_asid, $unmatched_asid, '']); + + $exit_code = Artisan::call(self::Command, ['path' => $csv_path]); + + $this->assertSame(0, $exit_code); + + $reloaded = EntityManager::getRepository(User::class)->getById($user->getId()); + $this->assertNull($reloaded->getExternalId()); + + $matched_row = DB::table('facebook_deletion_requests')->where('external_id', $matched_asid)->first(); + $this->assertSame('completed', $matched_row->status); + + $unmatched_row = DB::table('facebook_deletion_requests')->where('external_id', $unmatched_asid)->first(); + $this->assertSame('not_found', $unmatched_row->status); + + unlink($csv_path); + } + + public function testBackfillIsIdempotentAcrossRuns(): void + { + $asid = '555000333'; + $csv_path = $this->writeCsvFixture([$asid]); + + Artisan::call(self::Command, ['path' => $csv_path]); + Artisan::call(self::Command, ['path' => $csv_path]); + + $count = DB::table('facebook_deletion_requests')->where('external_id', $asid)->count(); + $this->assertSame(1, $count); + + unlink($csv_path); + } + + public function testUnreadablePathReturnsNonZeroExitCode(): void + { + $exit_code = Artisan::call(self::Command, ['path' => '/no/such/file.csv']); + + $this->assertNotSame(0, $exit_code); + } +} diff --git a/tests/unit/FacebookDataDeletionServiceRaceTest.php b/tests/unit/FacebookDataDeletionServiceRaceTest.php new file mode 100644 index 00000000..a180efca --- /dev/null +++ b/tests/unit/FacebookDataDeletionServiceRaceTest.php @@ -0,0 +1,80 @@ +createMock(IUserRepository::class); + $user_repository->method('getByExternalId')->willReturn(null); + + $connection = $this->createMock(Connection::class); + + $lookup_calls = 0; + $connection->method('fetchAssociative') + ->willReturnCallback(function () use (&$lookup_calls) { + $lookup_calls++; + if ($lookup_calls === 1) { + // first check: no row yet, so processDeletionRequest proceeds to insert + return false; + } + // second check (inside the catch): a concurrent writer already + // committed the row for this (provider, external_id) pair + return [ + 'provider' => 'facebook', + 'external_id' => 'racing-asid', + 'confirmation_code' => 'winner-confirmation-code', + 'status' => 'not_found', + 'user_id' => null, + ]; + }); + + $connection->method('insert') + ->willThrowException($this->createMock(UniqueConstraintViolationException::class)); + + $em = $this->createMock(EntityManagerInterface::class); + $em->method('getConnection')->willReturn($connection); + + $registry = $this->createMock(ManagerRegistry::class); + $registry->method('getManager')->willReturn($em); + Registry::swap($registry); + + $tx_service = $this->createMock(ITransactionService::class); + $tx_service->method('transaction')->willReturnCallback(fn($callback) => $callback()); + + $service = new FacebookDataDeletionService($user_repository, $tx_service); + + $result = $service->processDeletionRequest('racing-asid'); + + $this->assertSame('winner-confirmation-code', $result['confirmation_code']); + $this->assertSame('not_found', $result['status']); + } +} diff --git a/tests/unit/FacebookSignedRequestParserTest.php b/tests/unit/FacebookSignedRequestParserTest.php new file mode 100644 index 00000000..9bff6853 --- /dev/null +++ b/tests/unit/FacebookSignedRequestParserTest.php @@ -0,0 +1,95 @@ +base64UrlEncode(json_encode($payload)); + $sig = hash_hmac('sha256', $encoded_payload, $secret, true); + $encoded_sig = $this->base64UrlEncode($sig); + return $encoded_sig . '.' . $encoded_payload; + } + + private function base64UrlEncode(string $input): string + { + return rtrim(strtr(base64_encode($input), '+/', '-_'), '='); + } + + public function testValidSignedRequestReturnsPayloadWithUserId(): void + { + $sr = $this->buildSignedRequest(['user_id' => '218471'], self::Secret); + + $data = FacebookSignedRequestParser::parse($sr, self::Secret); + + $this->assertIsArray($data); + $this->assertSame('218471', $data['user_id']); + } + + public function testLowercaseAlgorithmIsStillAccepted(): void + { + $sr = $this->buildSignedRequest(['user_id' => '218471'], self::Secret, 'hmac-sha256'); + + $data = FacebookSignedRequestParser::parse($sr, self::Secret); + + $this->assertIsArray($data); + $this->assertSame('218471', $data['user_id']); + } + + public function testTamperedPayloadReturnsNull(): void + { + $sr = $this->buildSignedRequest(['user_id' => '218471'], self::Secret); + [$sig, $payload] = explode('.', $sr, 2); + + $tampered_payload = $this->base64UrlEncode(json_encode(['user_id' => '999999', 'algorithm' => 'HMAC-SHA256'])); + $tampered_sr = $sig . '.' . $tampered_payload; + + $this->assertNull(FacebookSignedRequestParser::parse($tampered_sr, self::Secret)); + } + + public function testWrongSecretReturnsNull(): void + { + $sr = $this->buildSignedRequest(['user_id' => '218471'], self::Secret); + + $this->assertNull(FacebookSignedRequestParser::parse($sr, 'wrong-secret')); + } + + public function testUnsupportedAlgorithmReturnsNull(): void + { + $sr = $this->buildSignedRequest(['user_id' => '218471'], self::Secret, 'MD5'); + + $this->assertNull(FacebookSignedRequestParser::parse($sr, self::Secret)); + } + + public function testMalformedStringWithNoDotReturnsNull(): void + { + $this->assertNull(FacebookSignedRequestParser::parse('not-a-valid-signed-request', self::Secret)); + } + + public function testMissingUserIdReturnsNull(): void + { + $sr = $this->buildSignedRequest([], self::Secret); + + $this->assertNull(FacebookSignedRequestParser::parse($sr, self::Secret)); + } +} From 477109d61e99fa846c2bde3a73ad4a25f9467021 Mon Sep 17 00:00:00 2001 From: smarcet Date: Fri, 7 Aug 2026 12:38:08 -0300 Subject: [PATCH 2/7] debug: dump response body on CI 500 failure for FacebookDataDeletionApiTest --- tests/FacebookDataDeletionApiTest.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/FacebookDataDeletionApiTest.php b/tests/FacebookDataDeletionApiTest.php index e51f27b5..56d46916 100644 --- a/tests/FacebookDataDeletionApiTest.php +++ b/tests/FacebookDataDeletionApiTest.php @@ -65,6 +65,10 @@ public function testMatchedUserIsUnlinkedAndReturnsConfirmation(): void $response = $this->post(self::CallbackUri, ['signed_request' => $sr]); + if ($response->response->getStatusCode() !== 200) { + fwrite(STDERR, "\n===DEBUG RESPONSE BODY===\n" . $response->response->getContent() . "\n===END DEBUG===\n"); + } + $this->assertResponseStatus(200); $json = json_decode($response->response->getContent(), true); $this->assertNotEmpty($json['confirmation_code']); From c8daaa07da1822749a8a63346e22a2ee1ea398d2 Mon Sep 17 00:00:00 2001 From: smarcet Date: Fri, 7 Aug 2026 12:50:15 -0300 Subject: [PATCH 3/7] fix(auth): reject null/empty secret cleanly, configure CI Facebook creds Root cause of the CI failure on testMatchedUserIsUnlinkedAndReturnsConfirmation: .env.testing (which supplies FACEBOOK_CLIENT_SECRET locally) is gitignored, and none of the three GitHub Actions workflows ever set FACEBOOK_CLIENT_ID/ FACEBOOK_CLIENT_SECRET - a pre-existing gap this PR was the first to depend on. Config::get('services.facebook.client_secret') resolved to null in CI, and FacebookSignedRequestParser::parse()'s non-nullable string $secret parameter turned that into a fatal TypeError (500) instead of a clean rejection. - Widen parse()'s $secret to ?string and reject immediately on null/'' - a webhook endpoint should fail closed with 400 on missing server config, never 500. - Add FACEBOOK_CLIENT_ID/FACEBOOK_CLIENT_SECRET/FACEBOOK_REDIRECT_URI (same dummy test values as .env.testing) to all three CI workflows. - Two new parser tests lock in the defensive null/empty-secret behavior. Reverts the temporary response-body-dump instrumentation from 477109d6, which was used to capture the real TypeError from CI's APP_DEBUG=true error page. --- .github/workflows/nightly_unit_tests.yml | 3 +++ .github/workflows/pull_request_unit_tests.yml | 3 +++ .github/workflows/push.yml | 3 +++ app/libs/Auth/FacebookSignedRequestParser.php | 6 ++++-- tests/FacebookDataDeletionApiTest.php | 4 ---- tests/unit/FacebookSignedRequestParserTest.php | 14 ++++++++++++++ 6 files changed, 27 insertions(+), 6 deletions(-) diff --git a/.github/workflows/nightly_unit_tests.yml b/.github/workflows/nightly_unit_tests.yml index 01dd2112..5e5671b0 100644 --- a/.github/workflows/nightly_unit_tests.yml +++ b/.github/workflows/nightly_unit_tests.yml @@ -32,6 +32,9 @@ jobs: SSL_ENABLED: false SESSION_DRIVER: redis PHP_VERSION: 8.3 + FACEBOOK_CLIENT_ID: 214242500242860 + FACEBOOK_CLIENT_SECRET: e62fa81aa898699d8cebf14bf5e586aa + FACEBOOK_REDIRECT_URI: /auth/login/facebook/callback services: mysql: image: mysql:8.0 diff --git a/.github/workflows/pull_request_unit_tests.yml b/.github/workflows/pull_request_unit_tests.yml index 462317c3..c4a35288 100644 --- a/.github/workflows/pull_request_unit_tests.yml +++ b/.github/workflows/pull_request_unit_tests.yml @@ -37,6 +37,9 @@ jobs: PHP_VERSION: 8.3 OTEL_SDK_DISABLED: true OTEL_SERVICE_ENABLED: false + FACEBOOK_CLIENT_ID: 214242500242860 + FACEBOOK_CLIENT_SECRET: e62fa81aa898699d8cebf14bf5e586aa + FACEBOOK_REDIRECT_URI: /auth/login/facebook/callback services: mysql: image: mysql:8.0 diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index ad2ede65..001d6d0a 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -33,6 +33,9 @@ jobs: PHP_VERSION: 8.3 OTEL_SDK_DISABLED: true OTEL_SERVICE_ENABLED: false + FACEBOOK_CLIENT_ID: 214242500242860 + FACEBOOK_CLIENT_SECRET: e62fa81aa898699d8cebf14bf5e586aa + FACEBOOK_REDIRECT_URI: /auth/login/facebook/callback services: mysql: image: mysql:8.0 diff --git a/app/libs/Auth/FacebookSignedRequestParser.php b/app/libs/Auth/FacebookSignedRequestParser.php index 3ae742c6..85746ea8 100644 --- a/app/libs/Auth/FacebookSignedRequestParser.php +++ b/app/libs/Auth/FacebookSignedRequestParser.php @@ -23,11 +23,13 @@ final class FacebookSignedRequestParser { /** * @param string $signed_request - * @param string $secret + * @param string|null $secret * @return array|null */ - public static function parse(string $signed_request, string $secret): ?array + public static function parse(string $signed_request, ?string $secret): ?array { + if ($secret === null || $secret === '') return null; + $parts = explode('.', $signed_request, 2); if (count($parts) !== 2) return null; diff --git a/tests/FacebookDataDeletionApiTest.php b/tests/FacebookDataDeletionApiTest.php index 56d46916..e51f27b5 100644 --- a/tests/FacebookDataDeletionApiTest.php +++ b/tests/FacebookDataDeletionApiTest.php @@ -65,10 +65,6 @@ public function testMatchedUserIsUnlinkedAndReturnsConfirmation(): void $response = $this->post(self::CallbackUri, ['signed_request' => $sr]); - if ($response->response->getStatusCode() !== 200) { - fwrite(STDERR, "\n===DEBUG RESPONSE BODY===\n" . $response->response->getContent() . "\n===END DEBUG===\n"); - } - $this->assertResponseStatus(200); $json = json_decode($response->response->getContent(), true); $this->assertNotEmpty($json['confirmation_code']); diff --git a/tests/unit/FacebookSignedRequestParserTest.php b/tests/unit/FacebookSignedRequestParserTest.php index 9bff6853..66bf327f 100644 --- a/tests/unit/FacebookSignedRequestParserTest.php +++ b/tests/unit/FacebookSignedRequestParserTest.php @@ -92,4 +92,18 @@ public function testMissingUserIdReturnsNull(): void $this->assertNull(FacebookSignedRequestParser::parse($sr, self::Secret)); } + + public function testNullSecretReturnsNullInsteadOfThrowing(): void + { + $sr = $this->buildSignedRequest(['user_id' => '218471'], self::Secret); + + $this->assertNull(FacebookSignedRequestParser::parse($sr, null)); + } + + public function testEmptySecretReturnsNullInsteadOfThrowing(): void + { + $sr = $this->buildSignedRequest(['user_id' => '218471'], self::Secret); + + $this->assertNull(FacebookSignedRequestParser::parse($sr, '')); + } } From 86c4b352722a748ec480329c5b8b79d9e3d2c23e Mon Sep 17 00:00:00 2001 From: smarcet Date: Fri, 7 Aug 2026 15:21:58 -0300 Subject: [PATCH 4/7] fix(auth): reject non-string signed_request with 400 instead of TypeError Request::input('signed_request') can return an array (e.g. array-notation POST body signed_request[]=x), which empty() treats as non-empty and passes through to FacebookSignedRequestParser::parse(string $signed_request, ...). That non-nullable string param throws an uncaught TypeError on an array argument, turning a malformed request to this public unauthenticated endpoint into a 500 instead of the existing clean 400 response. --- .../Controllers/Api/FacebookDataDeletionController.php | 2 +- tests/FacebookDataDeletionApiTest.php | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/Http/Controllers/Api/FacebookDataDeletionController.php b/app/Http/Controllers/Api/FacebookDataDeletionController.php index f66135d9..a64bddcc 100644 --- a/app/Http/Controllers/Api/FacebookDataDeletionController.php +++ b/app/Http/Controllers/Api/FacebookDataDeletionController.php @@ -48,7 +48,7 @@ public function handle(Request $request) { $signed_request = $request->input('signed_request'); - if (empty($signed_request)) { + if (!is_string($signed_request) || $signed_request === '') { Log::warning("FacebookDataDeletionController::handle missing signed_request"); return response()->json([ 'error' => ['code' => 'invalid_request', 'message' => 'signed_request is required.'] diff --git a/tests/FacebookDataDeletionApiTest.php b/tests/FacebookDataDeletionApiTest.php index e51f27b5..31ec4576 100644 --- a/tests/FacebookDataDeletionApiTest.php +++ b/tests/FacebookDataDeletionApiTest.php @@ -123,6 +123,13 @@ public function testStatusPageForRealConfirmationCodeReturns200(): void $status_response->see($confirmation_code); } + public function testArraySignedRequestReturns400InsteadOf500(): void + { + $response = $this->post(self::CallbackUri, ['signed_request' => ['a', 'b']]); + + $this->assertResponseStatus(400); + } + public function testStatusPageForUnknownCodeReturns404(): void { $this->get(self::CallbackUri . '/status/does-not-exist'); From 06df951122725a2f4032a123ff5f57101d18637e Mon Sep 17 00:00:00 2001 From: smarcet Date: Fri, 7 Aug 2026 15:24:11 -0300 Subject: [PATCH 5/7] fix(db): add FK, index and correct column width to facebook_deletion_requests.user_id Every other user_id/performer_id column in this codebase (users_deleted, users_email_changed, banned_ips, oauth2_client, ...) is bigInteger() ->setUnsigned(true) with an explicit index() and foreign() constraint. This table's user_id was a plain, unindexed, un-constrained integer: no referential integrity, a full table scan on lookups by user, and a type mismatch against users.id (bigint unsigned). ON DELETE SET NULL (not CASCADE) preserves the audit row if a user is later hard-deleted, matching oauth2_client_user_id_foreign's precedent for the same case. Verified by rolling the migration down/up against idp-db-local's idp_test database and inspecting the resulting SHOW CREATE TABLE output. --- database/migrations/Version20260806190000.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/database/migrations/Version20260806190000.php b/database/migrations/Version20260806190000.php index fea3b60a..01d22401 100644 --- a/database/migrations/Version20260806190000.php +++ b/database/migrations/Version20260806190000.php @@ -37,7 +37,9 @@ public function up(Schema $schema):void $table->string("external_id")->setNotnull(true); $table->string("confirmation_code")->setNotnull(true); $table->string("status")->setNotnull(true); - $table->integer("user_id")->setNotnull(false); + $table->bigInteger("user_id")->setUnsigned(true)->setNotnull(false); + $table->index("user_id", "user_id"); + $table->foreign("users", "user_id", "id", ["onDelete" => "SET NULL"]); $table->unique(["provider", "external_id"]); $table->unique("confirmation_code"); }); From e7cbe3db468cea672d55a0d593061faa0b9987b0 Mon Sep 17 00:00:00 2001 From: smarcet Date: Fri, 7 Aug 2026 15:25:18 -0300 Subject: [PATCH 6/7] fix(auth): stop logging the Facebook app-scoped id in backfill debug log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Log::debug wrote the raw external_id being unlinked, which — if LOG_LEVEL is ever raised above the default 'error' — writes the very identifier the deletion request exists to purge into log files, undermining the point of the deletion. Log the outcome status instead; the confirmation_code/status are already the durable audit trail via facebook_deletion_requests. --- .../Commands/FacebookDataDeletionBackfill.php | 2 +- .../FacebookDataDeletionBackfillCommandTest.php | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/Console/Commands/FacebookDataDeletionBackfill.php b/app/Console/Commands/FacebookDataDeletionBackfill.php index 24580733..e167a8e5 100644 --- a/app/Console/Commands/FacebookDataDeletionBackfill.php +++ b/app/Console/Commands/FacebookDataDeletionBackfill.php @@ -93,7 +93,7 @@ public function handle() $not_found++; } - Log::debug(sprintf("FacebookDataDeletionBackfill::handle processed %s -> %s", $external_id, $result['confirmation_code'])); + Log::debug(sprintf("FacebookDataDeletionBackfill::handle processed request with status %s", $result['status'])); } fclose($handle); diff --git a/tests/FacebookDataDeletionBackfillCommandTest.php b/tests/FacebookDataDeletionBackfillCommandTest.php index b7b6f1e8..c78f1388 100644 --- a/tests/FacebookDataDeletionBackfillCommandTest.php +++ b/tests/FacebookDataDeletionBackfillCommandTest.php @@ -14,6 +14,7 @@ use Auth\User; use Illuminate\Support\Facades\Artisan; use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Log; use LaravelDoctrine\ORM\Facades\EntityManager; /** @@ -86,6 +87,21 @@ public function testBackfillIsIdempotentAcrossRuns(): void unlink($csv_path); } + public function testDoesNotLogExternalId(): void + { + Log::spy(); + $asid = '555000444'; + $csv_path = $this->writeCsvFixture([$asid]); + + Artisan::call(self::Command, ['path' => $csv_path]); + + Log::shouldNotHaveReceived('debug', function (string $message) use ($asid) { + return str_contains($message, $asid); + }); + + unlink($csv_path); + } + public function testUnreadablePathReturnsNonZeroExitCode(): void { $exit_code = Artisan::call(self::Command, ['path' => '/no/such/file.csv']); From 5b72374c6f6561c8f0d718d9748d6345726f8baa Mon Sep 17 00:00:00 2001 From: smarcet Date: Fri, 7 Aug 2026 15:26:58 -0300 Subject: [PATCH 7/7] fix(auth): fail cleanly when fopen() fails after is_readable() passes is_readable() passing doesn't guarantee fopen() succeeds (fd exhaustion, file removed mid-check). An unchecked fopen() failure previously crashed the command (PHP escalates the fopen() warning to an ErrorException, or fgets() throws TypeError on a false stream) instead of using the command's own clean $this->error(...); return 1; pattern already used for the is_readable check two lines above. Test uses a minimal custom stream wrapper (url_stat succeeds, stream_open returns false) to deterministically reproduce the gap without depending on a real filesystem race. --- .../Commands/FacebookDataDeletionBackfill.php | 7 +++- ...acebookDataDeletionBackfillCommandTest.php | 36 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/app/Console/Commands/FacebookDataDeletionBackfill.php b/app/Console/Commands/FacebookDataDeletionBackfill.php index e167a8e5..0d39afb2 100644 --- a/app/Console/Commands/FacebookDataDeletionBackfill.php +++ b/app/Console/Commands/FacebookDataDeletionBackfill.php @@ -76,7 +76,12 @@ public function handle() $not_found = 0; $skipped = 0; - $handle = fopen($path, 'r'); + $handle = @fopen($path, 'r'); + if ($handle === false) { + $this->error(sprintf("Unable to open file %s.", $path)); + return 1; + } + while (($line = fgets($handle)) !== false) { $external_id = trim($line, " \t\n\r\0\x0B\"'"); diff --git a/tests/FacebookDataDeletionBackfillCommandTest.php b/tests/FacebookDataDeletionBackfillCommandTest.php index c78f1388..a56ae29a 100644 --- a/tests/FacebookDataDeletionBackfillCommandTest.php +++ b/tests/FacebookDataDeletionBackfillCommandTest.php @@ -108,4 +108,40 @@ public function testUnreadablePathReturnsNonZeroExitCode(): void $this->assertNotSame(0, $exit_code); } + + public function testFopenFailureAfterReadableCheckReturnsNonZeroExitCode(): void + { + stream_wrapper_register('fbfailtest', FacebookDataDeletionBackfillFailingStreamWrapper::class); + + try { + $exit_code = Artisan::call(self::Command, ['path' => 'fbfailtest://fake.csv']); + $this->assertNotSame(0, $exit_code); + } finally { + stream_wrapper_unregister('fbfailtest'); + } + } +} + +/** + * Reports the path as readable (url_stat succeeds) but fails to open it + * (stream_open returns false) - reproduces the fopen()-fails-after- + * is_readable()-passes gap without depending on real filesystem races. + */ +final class FacebookDataDeletionBackfillFailingStreamWrapper +{ + public $context; + + public function url_stat(string $path, int $flags) + { + return [ + 'dev' => 0, 'ino' => 0, 'mode' => 0100644, 'nlink' => 1, + 'uid' => 0, 'gid' => 0, 'rdev' => 0, 'size' => 0, + 'atime' => 0, 'mtime' => 0, 'ctime' => 0, 'blksize' => -1, 'blocks' => -1, + ]; + } + + public function stream_open(string $path, string $mode, int $options, ?string &$opened_path): bool + { + return false; + } }