From a4c176d52006ce893d227860345bcbf97e84316b Mon Sep 17 00:00:00 2001 From: Daniel Supernault Date: Mon, 21 Sep 2026 06:37:10 -0600 Subject: [PATCH] Fix deletes, again --- app/Http/Controllers/FederationController.php | 8 +- app/Jobs/InboxPipeline/DeleteWorker.php | 31 ++-- .../DirectMessageBackfillTest.php | 33 ++++ .../DirectMessageInboundTest.php | 52 ++++++- .../DirectMessageSignedDeleteTest.php | 145 ++++++++++++++++++ tests/Feature/DirectMessage/helpers.php | 24 +-- 6 files changed, 262 insertions(+), 31 deletions(-) create mode 100644 tests/Feature/DirectMessage/DirectMessageSignedDeleteTest.php diff --git a/app/Http/Controllers/FederationController.php b/app/Http/Controllers/FederationController.php index c621688f3..095f3d4ed 100644 --- a/app/Http/Controllers/FederationController.php +++ b/app/Http/Controllers/FederationController.php @@ -191,7 +191,7 @@ class FederationController extends Controller if (isset($obj['object']) && isset($obj['object']['type']) && isset($obj['object']['id'])) { if ($obj['object']['type'] === 'Person') { if (Profile::whereRemoteUrl($obj['object']['id'])->exists()) { - dispatch(new DeleteWorker($headers, $payload))->onQueue('inbox'); + dispatch(new DeleteWorker($headers, $payload, $request->getPathInfo()))->onQueue('inbox'); return; } @@ -199,14 +199,14 @@ class FederationController extends Controller if ($obj['object']['type'] === 'Tombstone') { if ($this->isKnownTombstone($obj['object']['id'])) { - dispatch(new DeleteWorker($headers, $payload))->onQueue('delete'); + dispatch(new DeleteWorker($headers, $payload, $request->getPathInfo()))->onQueue('delete'); return; } } if ($obj['object']['type'] === 'Story') { - dispatch(new DeleteWorker($headers, $payload))->onQueue('story'); + dispatch(new DeleteWorker($headers, $payload, $request->getPathInfo()))->onQueue('story'); return; } @@ -269,7 +269,7 @@ class FederationController extends Controller } if ($obj['object']['type'] === 'Tombstone') { - if (Status::whereObjectUrl($obj['object']['id'])->exists()) { + if ($this->isKnownTombstone($obj['object']['id'])) { dispatch(new DeleteWorker($headers, $payload))->onQueue('delete'); return; diff --git a/app/Jobs/InboxPipeline/DeleteWorker.php b/app/Jobs/InboxPipeline/DeleteWorker.php index bb9c7ca62..e281d9b57 100644 --- a/app/Jobs/InboxPipeline/DeleteWorker.php +++ b/app/Jobs/InboxPipeline/DeleteWorker.php @@ -21,6 +21,8 @@ class DeleteWorker implements ShouldQueue protected $payload; + protected $inboxPath = '/f/inbox'; + public $timeout = 300; public $tries = 1; @@ -32,10 +34,13 @@ class DeleteWorker implements ShouldQueue * * @return void */ - public function __construct($headers, $payload) + public function __construct($headers, $payload, ?string $inboxPath = null) { $this->headers = $headers; $this->payload = $payload; + if ($inboxPath) { + $this->inboxPath = $inboxPath; + } } /** @@ -77,14 +82,15 @@ class DeleteWorker implements ShouldQueue return; } - if ($payload['type'] === 'Delete' && + if ( + $payload['type'] === 'Delete' && ((is_string($payload['object']) && $payload['object'] === $payload['actor']) || - (is_array($payload['object']) && - isset($payload['object']['id'], $payload['object']['type']) && - $payload['object']['type'] === 'Person' && - $payload['actor'] === $payload['object']['id'] - )) + (is_array($payload['object']) && + isset($payload['object']['id'], $payload['object']['type']) && + $payload['object']['type'] === 'Person' && + $payload['actor'] === $payload['object']['id'] + )) ) { $actor = $payload['actor']; if ($this->verifySignature($headers, $payload) == true) { @@ -132,8 +138,9 @@ class DeleteWorker implements ShouldQueue if (! $date) { return false; } - if (! now()->parse($date)->gt(now()->subDays(1)) || - ! now()->parse($date)->lt(now()->addDays(1)) + if ( + ! now()->parse($date)->gt(now()->subDays(1)) || + ! now()->parse($date)->lt(now()->addDays(1)) ) { return false; } @@ -148,7 +155,8 @@ class DeleteWorker implements ShouldQueue $keyDomain = parse_url($keyId, PHP_URL_HOST); $idDomain = parse_url($id, PHP_URL_HOST); $actorDomain = parse_url($bodyDecoded['actor'] ?? '', PHP_URL_HOST); - if (isset($bodyDecoded['object']) + if ( + isset($bodyDecoded['object']) && is_array($bodyDecoded['object']) && isset($bodyDecoded['object']['attributedTo']) ) { @@ -189,8 +197,7 @@ class DeleteWorker implements ShouldQueue if (! $pkey) { return false; } - $inboxPath = '/f/inbox'; - [$verified, $headers] = HttpSignature::verify($pkey, $signatureData, $headers, $inboxPath, $body); + [$verified, $headers] = HttpSignature::verify($pkey, $signatureData, $headers, $this->inboxPath, $body); if ($verified == 1) { return true; } diff --git a/tests/Feature/DirectMessage/DirectMessageBackfillTest.php b/tests/Feature/DirectMessage/DirectMessageBackfillTest.php index 005d7fe46..1134ff662 100644 --- a/tests/Feature/DirectMessage/DirectMessageBackfillTest.php +++ b/tests/Feature/DirectMessage/DirectMessageBackfillTest.php @@ -1,5 +1,6 @@ 1, 'snowflake.worker_id' => 1]); }); @@ -146,6 +150,35 @@ describe('dm:backfill-conversations', function () { }); }); +describe('remote deletes of converted messages', function () { + it('hands a converted message back to the status path so the legacy status goes too', function () { + $bob = dmProfile(dmLocalUser()); + $carol = dmRemoteProfile('carol'); + + dmLegacy($carol, $bob, 'old one', [], ['uri' => $carol->remote_url.'/statuses/9']); + $this->artisan('dm:backfill-conversations', ['--force' => true])->assertSuccessful(); + + $handler = app(DirectMessageHandler::class); + + expect($handler->handleDelete($carol, $carol->remote_url.'/statuses/9'))->toBeFalse() + ->and(DmMessage::count())->toBe(0); + }); + + it('fully handles a message that never was a status', function () { + $bob = dmProfile(dmLocalUser()); + $carol = dmRemoteProfile('carol'); + + $service = app(DirectMessageService::class); + $service->storeMessage($service->findOrCreateDm($carol, $bob), $carol, [ + 'body' => 'new one', + 'ap_object_uri' => $carol->remote_url.'/statuses/10', + ]); + + expect(app(DirectMessageHandler::class)->handleDelete($carol, $carol->remote_url.'/statuses/10'))->toBeTrue() + ->and(DmMessage::count())->toBe(0); + }); +}); + describe('cleanup', function () { it('removes the message when the status behind it is deleted', function () { $alice = dmProfile(dmLocalUser()); diff --git a/tests/Feature/DirectMessage/DirectMessageInboundTest.php b/tests/Feature/DirectMessage/DirectMessageInboundTest.php index 10c4b572f..9f9b4fadf 100644 --- a/tests/Feature/DirectMessage/DirectMessageInboundTest.php +++ b/tests/Feature/DirectMessage/DirectMessageInboundTest.php @@ -2,6 +2,7 @@ use App\Federation\Handlers\DirectMessageHandler; use App\Federation\Validators\DirectMessageValidator; +use App\Jobs\InboxPipeline\DeleteWorker; use App\Jobs\MediaPipeline\MediaDeletePipeline; use App\Models\DmConversation; use App\Models\DmConversationParticipant; @@ -40,11 +41,13 @@ beforeEach(function () { Queue::fake(); Http::fake(); + // Ids minted in the same millisecond only sort by creation order when the + // worker bits are fixed. Left unset they are random for every id. + config(['snowflake.datacenter_id' => 1, 'snowflake.worker_id' => 1]); + config([ 'instance.enable_cc' => false, 'federation.activitypub.enabled' => true, - 'snowflake.datacenter_id' => 1, - 'snowflake.worker_id' => 1, ]); }); @@ -467,10 +470,16 @@ describe('groups', function () { describe('conversation identity', function () { it('hashes the same people to the same conversation whatever order they come in', function () { + // Snowflake ids minted in the same millisecond differ only in their + // last bits, which a float comparison cannot see $a = 1007571621538590723; $b = $a + 1; $c = $a + 2; - expect(DmConversation::dmHash($a, $b))->toBe(DmConversation::dmHash($b, $a))->and(DmConversation::participantsHash([$a, $b, $c]))->toBe(DmConversation::participantsHash([$c, $a, $b]))->and(DmConversation::participantsHash([$a, $b, $c]))->toBe(DmConversation::participantsHash([(string) $b, $c, $a, $a]))->and(DmConversation::dmHash($a, $b))->not->toBe(DmConversation::dmHash($a, $c)); + + expect(DmConversation::dmHash($a, $b))->toBe(DmConversation::dmHash($b, $a)) + ->and(DmConversation::participantsHash([$a, $b, $c]))->toBe(DmConversation::participantsHash([$c, $a, $b])) + ->and(DmConversation::participantsHash([$a, $b, $c]))->toBe(DmConversation::participantsHash([(string) $b, $c, $a, $a])) + ->and(DmConversation::dmHash($a, $b))->not->toBe(DmConversation::dmHash($a, $c)); }); it('keeps one conversation when two participants have neighbouring ids', function () { @@ -621,6 +630,43 @@ describe('deletes', function () { Queue::assertPushed(MediaDeletePipeline::class, 1); }); + it('lets a delete for a direct message through the inbox endpoints', function (string $endpoint) { + $bobUser = dmLocalUser(); + $bob = dmProfile($bobUser); + $alice = dmRemoteProfile(); + dmSeedHosts(); + + dmDeliver($alice, dmNote($alice, '1', [$bob])); + + // What Loops and Mastodon send + $delete = [ + '@context' => 'https://www.w3.org/ns/activitystreams', + 'id' => $alice->remote_url.'/statuses/1#delete', + 'type' => 'Delete', + 'actor' => $alice->remote_url, + 'to' => [$bob->permalink()], + 'object' => ['id' => $alice->remote_url.'/statuses/1', 'type' => 'Tombstone'], + ]; + + $this->postJson(str_replace('{username}', $bobUser->username, $endpoint), $delete)->assertOk(); + + Queue::assertPushed(DeleteWorker::class, 1); + })->with(['/f/inbox', '/users/{username}/inbox']); + + it('still drops a delete for something this server has never seen', function () { + $alice = dmRemoteProfile(); + dmSeedHosts(); + + $this->postJson('/f/inbox', [ + 'id' => $alice->remote_url.'/statuses/404#delete', + 'type' => 'Delete', + 'actor' => $alice->remote_url, + 'object' => ['id' => $alice->remote_url.'/statuses/404', 'type' => 'Tombstone'], + ])->assertOk(); + + Queue::assertNotPushed(DeleteWorker::class); + }); + it('ignores a delete from someone who did not write the message', function () { $bob = dmProfile(dmLocalUser()); $alice = dmRemoteProfile(); diff --git a/tests/Feature/DirectMessage/DirectMessageSignedDeleteTest.php b/tests/Feature/DirectMessage/DirectMessageSignedDeleteTest.php new file mode 100644 index 000000000..b9e627820 --- /dev/null +++ b/tests/Feature/DirectMessage/DirectMessageSignedDeleteTest.php @@ -0,0 +1,145 @@ + 1, + 'snowflake.worker_id' => 1, + 'instance.enable_cc' => false, + 'federation.activitypub.enabled' => true, + ]); +}); + +function dmSignedPost($test, Profile $actor, $privateKey, string $path, array $activity) +{ + $body = json_encode($activity, JSON_UNESCAPED_SLASHES); + $host = parse_url(config('app.url'), PHP_URL_HOST); + $date = now()->toRfc7231String(); + $digest = 'SHA-256='.base64_encode(hash('sha256', $body, true)); + + $signingString = implode("\n", [ + '(request-target): post '.$path, + 'host: '.$host, + 'date: '.$date, + 'digest: '.$digest, + ]); + + openssl_sign($signingString, $signature, $privateKey, OPENSSL_ALGO_SHA256); + + return $test->call('POST', $path, [], [], [], [ + 'HTTP_HOST' => $host, + 'HTTP_DATE' => $date, + 'HTTP_DIGEST' => $digest, + 'HTTP_SIGNATURE' => 'keyId="'.$actor->key_id.'",algorithm="rsa-sha256",headers="(request-target) host date digest",signature="'.base64_encode($signature).'"', + 'CONTENT_TYPE' => 'application/activity+json', + 'HTTP_ACCEPT' => 'application/activity+json', + ], $body); +} + +it('deletes a direct message from a signed Delete', function (string $endpoint) { + $key = openssl_pkey_new(['private_key_bits' => 2048, 'private_key_type' => OPENSSL_KEYTYPE_RSA]); + + $bobUser = dmLocalUser(); + $bob = dmProfile($bobUser); + $alice = dmRemoteProfile(); + $alice->public_key = openssl_pkey_get_details($key)['key']; + $alice->save(); + dmSeedHosts(); + + dmDeliver($alice, dmNote($alice, '1', [$bob])); + expect(DmMessage::count())->toBe(1); + + $response = dmSignedPost($this, $alice, $key, str_replace('{username}', $bobUser->username, $endpoint), [ + '@context' => 'https://www.w3.org/ns/activitystreams', + 'id' => $alice->remote_url.'/statuses/1#delete', + 'type' => 'Delete', + 'actor' => $alice->remote_url, + 'to' => [$bob->permalink()], + 'object' => ['id' => $alice->remote_url.'/statuses/1', 'type' => 'Tombstone'], + ]); + + $response->assertOk(); + + // The endpoint queued it + $worker = Queue::pushed(DeleteWorker::class)->first(); + expect($worker)->not->toBeNull(); + + // The signature verified against the path it was delivered to + $worker->handle(); + $handler = Queue::pushed(ActivityHandler::class)->first(); + expect($handler)->not->toBeNull(); + + // And the inbox removed the message + $handler->handle(); + expect(DmMessage::count())->toBe(0); +})->with(['/f/inbox', '/users/{username}/inbox']); + +it('rejects a Delete whose signature was made for a different inbox', function () { + $key = openssl_pkey_new(['private_key_bits' => 2048, 'private_key_type' => OPENSSL_KEYTYPE_RSA]); + + $bobUser = dmLocalUser(); + $bob = dmProfile($bobUser); + $alice = dmRemoteProfile(); + $alice->public_key = openssl_pkey_get_details($key)['key']; + $alice->save(); + dmSeedHosts(); + + dmDeliver($alice, dmNote($alice, '1', [$bob])); + + $activity = [ + 'id' => $alice->remote_url.'/statuses/1#delete', + 'type' => 'Delete', + 'actor' => $alice->remote_url, + 'object' => ['id' => $alice->remote_url.'/statuses/1', 'type' => 'Tombstone'], + ]; + + // Signed for the shared inbox, replayed at a personal one + $body = json_encode($activity, JSON_UNESCAPED_SLASHES); + $host = parse_url(config('app.url'), PHP_URL_HOST); + $date = now()->toRfc7231String(); + $digest = 'SHA-256='.base64_encode(hash('sha256', $body, true)); + openssl_sign("(request-target): post /f/inbox\nhost: {$host}\ndate: {$date}\ndigest: {$digest}", $signature, $key, OPENSSL_ALGO_SHA256); + + $this->call('POST', '/users/'.$bobUser->username.'/inbox', [], [], [], [ + 'HTTP_HOST' => $host, + 'HTTP_DATE' => $date, + 'HTTP_DIGEST' => $digest, + 'HTTP_SIGNATURE' => 'keyId="'.$alice->key_id.'",algorithm="rsa-sha256",headers="(request-target) host date digest",signature="'.base64_encode($signature).'"', + 'CONTENT_TYPE' => 'application/activity+json', + ], $body)->assertOk(); + + Queue::pushed(DeleteWorker::class)->first()->handle(); + + Queue::assertNotPushed(ActivityHandler::class); + expect(DmMessage::count())->toBe(1); +}); diff --git a/tests/Feature/DirectMessage/helpers.php b/tests/Feature/DirectMessage/helpers.php index a886c6ad0..cba608a5f 100644 --- a/tests/Feature/DirectMessage/helpers.php +++ b/tests/Feature/DirectMessage/helpers.php @@ -43,18 +43,6 @@ if (! function_exists('dmLocalUser')) { ]); } - /** - * Give $b the id right after $a, the way two profiles created in the same - * millisecond end up. Ids that close are equal once compared as floats, - * so anything that orders or matches ids has to cope with it. - */ - function dmNeighbour(Profile $a, Profile $b): Profile - { - DB::table('profiles')->where('id', $b->id)->update(['id' => $a->id + 1]); - - return Profile::findOrFail($a->id + 1); - } - /** * Seed the DNS and banned-domain caches so URL validation passes without * a network lookup. Call after factories, the lazy refresh can flush the @@ -71,6 +59,18 @@ if (! function_exists('dmLocalUser')) { Cache::put('instances:banned:domains', [], 1209600); } + /** + * Give $b the id right after $a, the way two profiles created in the same + * millisecond end up. Ids that close are equal once compared as floats, + * so anything that orders or matches ids has to cope with it. + */ + function dmNeighbour(Profile $a, Profile $b): Profile + { + DB::table('profiles')->where('id', $b->id)->update(['id' => $a->id + 1]); + + return Profile::findOrFail($a->id + 1); + } + function dmFollow(Profile $follower, Profile $target): void { Follower::create([