From 44a71f236143329e5b82fbfa51cbe18984a759d3 Mon Sep 17 00:00:00 2001 From: Julien BIANCHI Date: Sat, 31 Oct 2026 16:02:37 +0100 Subject: [PATCH 1/4] feat: tell API clients which reply continues a thread When an author replies to their own commit, the web shows their earliest such reply as the next commit of their thread, not among the replies, and replies_count already leaves it out. The API loaded that reply's id as continuation_id on commit pages, reply lists and log, profile and search cards, but CommitResource never sent it, so apps saw a replies count one short with no way to tell why or to draw the thread. CommitResource now sends continuation_id to signed requests wherever it was loaded: the id of that reply, or null when there is none the user may see. Ancestors and notifications don't load it, so they come without it, as they do without starred. It adds no query. --- app/Http/Resources/CommitResource.php | 12 ++++++++---- .../Controllers/Api/CommitControllerTest.php | 19 +++++++++++++++++++ .../Api/CommitReplyControllerTest.php | 13 +++++++++++++ .../Controllers/Api/LogControllerTest.php | 1 + .../Api/UserCommitControllerTest.php | 13 +++++++++++++ 5 files changed, 54 insertions(+), 4 deletions(-) diff --git a/app/Http/Resources/CommitResource.php b/app/Http/Resources/CommitResource.php index f8340df0c..ec06cc5e8 100644 --- a/app/Http/Resources/CommitResource.php +++ b/app/Http/Resources/CommitResource.php @@ -24,10 +24,13 @@ * and how many pastéis it was given, which the app only shows in pastéis mode, as the web does. Like the web's cards, * it also has preview_segments, the excerpt a list shows of a long commit, the poll, with what the user may see of * its votes, and the commit it quotes, or null once the user may no longer see that one, while quoted_commit_id still - * says it quoted something. In a list, reply_preview is the newest reply or quote the web shows under it. As on the - * web, the counts, link_preview, images, starred, parent, poll, quoted_commit and reply_preview are only - * there when they were loaded, so ancestors and the commits of notifications come without some of them. The images of - * a commit that isn't published yet have temporary URLs, since the app has no web session to load them with. + * says it quoted something. In a list, reply_preview is the newest reply or quote the web shows under it. + * continuation_id is the author's earliest reply to their own commit, which the web shows as the next commit of their + * thread rather than among the replies, so replies_count leaves it out; it is null when there is none the user may see. + * As on the web, the counts, link_preview, images, starred, continuation_id, parent, poll, quoted_commit and + * reply_preview are only there when they were loaded, so ancestors and the commits of notifications come without some + * of them. The images of a commit that isn't published yet have temporary URLs, since the app has no web session to + * load them with. * * A deleted commit that still has replies stays in its thread as a tombstone: its id, parent_id, deleted_at and * replies_count, but never its author or body. Only signed requests are ever sent one. @@ -81,6 +84,7 @@ public function toArray(Request $request): array 'starred' => $this->whenHas('starred'), 'pasteis_count' => $this->pasteis_count, 'deleted_at' => null, + 'continuation_id' => $this->whenHas('continuation_id'), 'parent' => $this->whenLoaded('parent', fn (Commit $parent): self => self::make($parent)), 'quoted_commit_id' => $this->quoted_commit_id, 'quoted_commit' => $this->whenLoaded('quotedCommit', fn (Commit $quoted): self => self::make($quoted)), diff --git a/tests/Feature/Controllers/Api/CommitControllerTest.php b/tests/Feature/Controllers/Api/CommitControllerTest.php index f811fcac9..df3b92198 100644 --- a/tests/Feature/Controllers/Api/CommitControllerTest.php +++ b/tests/Feature/Controllers/Api/CommitControllerTest.php @@ -291,6 +291,7 @@ 'starred' => true, 'pasteis_count' => 4, 'deleted_at' => null, + 'continuation_id' => null, 'quoted_commit_id' => null, 'quoted_commit' => null, 'poll' => null, @@ -322,6 +323,24 @@ ]); }); +it('tells a signed request which reply continues the thread of its author, and leaves it out of the replies count', function (): void { + $keyPair = apiKeyPairFor(User::factory()->create()); + $author = User::factory()->create(); + $commit = Commit::factory()->for($author)->create(['created_at' => now()->subMinutes(4)]); + $continuation = Commit::factory()->for($author)->for($commit, 'parent')->create(['created_at' => now()->subMinutes(3)]); + Commit::factory()->for($author)->for($commit, 'parent')->create(['created_at' => now()->subMinutes(2)]); + Commit::factory()->for($commit, 'parent')->create(['created_at' => now()->subMinute()]); + + signedApiRequest($keyPair, 'GET', route('api-commit.show', $commit)) + ->assertOk() + ->assertJsonPath('data.continuation_id', $continuation->id) + ->assertJsonPath('data.replies_count', 2); + + $this->getJson(route('api-commit.show', $commit)) + ->assertOk() + ->assertJsonMissingPath('data.continuation_id'); +}); + it('shows a signed request the poll of a commit as the web does, with each share once the votes show', function (): void { $user = User::factory()->create(); $keyPair = apiKeyPairFor($user); diff --git a/tests/Feature/Controllers/Api/CommitReplyControllerTest.php b/tests/Feature/Controllers/Api/CommitReplyControllerTest.php index adfc109fc..92164f7ab 100644 --- a/tests/Feature/Controllers/Api/CommitReplyControllerTest.php +++ b/tests/Feature/Controllers/Api/CommitReplyControllerTest.php @@ -36,6 +36,19 @@ ->assertJsonPath('meta.next_cursor', null); }); +it('tells which reply continues the thread of each reply', function (): void { + $parent = Commit::factory()->create(); + $author = User::factory()->create(); + $threaded = Commit::factory()->for($author)->for($parent, 'parent')->create(['created_at' => now()->subMinutes(3)]); + $continuation = Commit::factory()->for($author)->for($threaded, 'parent')->create(['created_at' => now()->subMinutes(2)]); + Commit::factory()->for($parent, 'parent')->create(['created_at' => now()->subMinute()]); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-reply.index', $parent)) + ->assertOk() + ->assertJsonPath('data.0.continuation_id', $continuation->id) + ->assertJsonPath('data.1.continuation_id', null); +}); + it('shows the stars and saves of each reply as the user sees them', function (): void { $parent = Commit::factory()->create(); $starred = Commit::factory()->for($parent, 'parent')->create(['created_at' => now()->subMinutes(2)]); diff --git a/tests/Feature/Controllers/Api/LogControllerTest.php b/tests/Feature/Controllers/Api/LogControllerTest.php index 984afa307..76940da8c 100644 --- a/tests/Feature/Controllers/Api/LogControllerTest.php +++ b/tests/Feature/Controllers/Api/LogControllerTest.php @@ -82,6 +82,7 @@ 'starred' => false, 'pasteis_count' => 2, 'deleted_at' => null, + 'continuation_id' => null, 'quoted_commit_id' => null, 'quoted_commit' => null, 'poll' => null, diff --git a/tests/Feature/Controllers/Api/UserCommitControllerTest.php b/tests/Feature/Controllers/Api/UserCommitControllerTest.php index bf8c346ed..210fc0c1e 100644 --- a/tests/Feature/Controllers/Api/UserCommitControllerTest.php +++ b/tests/Feature/Controllers/Api/UserCommitControllerTest.php @@ -271,3 +271,16 @@ ->assertOk() ->assertJsonMissingPath('data.0.reply_preview'); }); + +it('tells a signed request which reply continues the thread of each commit it lists', function (): void { + $keyPair = apiKeyPairFor(User::factory()->create()); + $user = User::factory()->create(); + $commit = Commit::factory()->for($user)->create(['created_at' => now()->subMinutes(3)]); + $continuation = Commit::factory()->for($user)->for($commit, 'parent')->create(['created_at' => now()->subMinutes(2)]); + Commit::factory()->for($user)->create(['created_at' => now()->subMinutes(4)]); + + signedApiRequest($keyPair, 'GET', route('api-user-commit.index', $user)) + ->assertOk() + ->assertJsonPath('data.0.continuation_id', $continuation->id) + ->assertJsonPath('data.1.continuation_id', null); +}); From 0da3de79fc2bf9ab2bf42c77b9a174193a93cf14 Mon Sep 17 00:00:00 2001 From: Julien BIANCHI Date: Sat, 31 Oct 2026 17:34:43 +0100 Subject: [PATCH 2/4] feat: let API clients leave a reply out of a commit's replies The web asks for a commit's replies without the one it already shows elsewhere, such as the continuation of the author's thread, which replies_count leaves out too. The API had no way to do that, so an app showing a thread got its continuation twice. The replies endpoint now takes the same except parameter as the web: the id of a reply to leave out. --- .../Controllers/Api/CommitReplyController.php | 2 +- .../Requests/ListApiCommitRepliesRequest.php | 9 +++++++++ .../Api/CommitReplyControllerTest.php | 16 ++++++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/app/Http/Controllers/Api/CommitReplyController.php b/app/Http/Controllers/Api/CommitReplyController.php index 7506a4a07..327595a67 100644 --- a/app/Http/Controllers/Api/CommitReplyController.php +++ b/app/Http/Controllers/Api/CommitReplyController.php @@ -16,6 +16,6 @@ { public function index(ListApiCommitRepliesRequest $request, #[CurrentUser] User $user, Commit $commit, CommitRepliesQuery $replies): AnonymousResourceCollection { - return CommitResource::collection($replies->paginate($commit, $user, $request->boolean('newest'))->withQueryString()); + return CommitResource::collection($replies->paginate($commit, $user, $request->boolean('newest'), except: $request->exceptedReplyId())->withQueryString()); } } diff --git a/app/Http/Requests/ListApiCommitRepliesRequest.php b/app/Http/Requests/ListApiCommitRepliesRequest.php index 359a8f9b0..707892f88 100644 --- a/app/Http/Requests/ListApiCommitRepliesRequest.php +++ b/app/Http/Requests/ListApiCommitRepliesRequest.php @@ -25,6 +25,14 @@ public function authorize(): Response return $commit->isVisibleTo($user) ? Response::allow() : Response::denyAsNotFound(); } + /** + * The reply the client shows elsewhere, such as the continuation of the commit, as on the web. + */ + public function exceptedReplyId(): ?string + { + return $this->string('except')->value() ?: null; + } + /** * Newest first continues the next_cursor of a reply preview, as on the web. * @@ -34,6 +42,7 @@ public function rules(): array { return [ 'newest' => ['sometimes', 'boolean'], + 'except' => ['sometimes', 'nullable', 'uuid'], ]; } } diff --git a/tests/Feature/Controllers/Api/CommitReplyControllerTest.php b/tests/Feature/Controllers/Api/CommitReplyControllerTest.php index 92164f7ab..6c5090532 100644 --- a/tests/Feature/Controllers/Api/CommitReplyControllerTest.php +++ b/tests/Feature/Controllers/Api/CommitReplyControllerTest.php @@ -49,6 +49,22 @@ ->assertJsonPath('data.1.continuation_id', null); }); +it('leaves out the reply the client shows elsewhere, such as the continuation, as the web does', function (): void { + $parent = Commit::factory()->create(['created_at' => now()->subMinutes(3)]); + $continuation = Commit::factory()->for($parent->user)->for($parent, 'parent')->create(['created_at' => now()->subMinutes(2)]); + $reply = Commit::factory()->for($parent, 'parent')->create(['created_at' => now()->subMinute()]); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-reply.index', [$parent, 'except' => $continuation->id])) + ->assertOk() + ->assertJsonPath('data.*.id', [$reply->id]); +}); + +it('validates the reply to leave out', function (): void { + signedApiRequest($this->keyPair, 'GET', route('api-commit-reply.index', [Commit::factory()->create(), 'except' => 'not-a-uuid'])) + ->assertUnprocessable() + ->assertJsonValidationErrors('except'); +}); + it('shows the stars and saves of each reply as the user sees them', function (): void { $parent = Commit::factory()->create(); $starred = Commit::factory()->for($parent, 'parent')->create(['created_at' => now()->subMinutes(2)]); From c607fc5b1e08616e87f5f02bf24cc1058ed991f9 Mon Sep 17 00:00:00 2001 From: Julien BIANCHI Date: Sat, 31 Oct 2026 17:34:49 +0100 Subject: [PATCH 3/4] feat: read the thread an author writes as replies to themselves through the API The web reads such a thread with commits/{commit}/continuation, but the API had no equivalent: an app had to fetch the thread one commit at a time, with no way to show where a commit stands in it. GET /api/v1/commits/{commit}/continuation lists the commits that follow a commit in its author's thread, with the same contract as its replies: signed requests only, not found for a commit the user may not see, and cursor pagination with meta.next_cursor and links.next. The cursor continues from the last commit of a page, which the user must still see. It reuses the web's query, so the thread stops at the first commit the user may not see and comes 10 commits a page. meta also has position, the place of the commit a page continues from, length, how many commits follow it (counted up to 100, as on the web), and is_capped, so position + length is the size of the thread. Each commit says which one continues it, which explains its replies_count. --- KNOWLEDGE_BASE.md | 2 +- .../Api/CommitContinuationController.php | 47 ++++++ .../ListApiCommitContinuationRequest.php | 64 +++++++ routes/api.php | 3 + .../Api/CommitContinuationControllerTest.php | 159 ++++++++++++++++++ 5 files changed, 274 insertions(+), 1 deletion(-) create mode 100644 app/Http/Controllers/Api/CommitContinuationController.php create mode 100644 app/Http/Requests/ListApiCommitContinuationRequest.php create mode 100644 tests/Feature/Controllers/Api/CommitContinuationControllerTest.php diff --git a/KNOWLEDGE_BASE.md b/KNOWLEDGE_BASE.md index 4f575f267..376278dcd 100644 --- a/KNOWLEDGE_BASE.md +++ b/KNOWLEDGE_BASE.md @@ -539,7 +539,7 @@ An app can use the same `/api/v1` for you with signed requests. It keeps an SSH A signed request sees what the web shows you, such as your own pending commits. It can also, with the web's validation, moderation, and rate limits: -- read your account, logs, replies, notifications (including grouped activity), and starred commits; +- read your account, logs, replies, threads authors write as replies to themselves, notifications (including grouped activity), and starred commits; - commit, reply, and quote, with images, and delete your commits, with the same undo window as the web; - preview commit drafts without publishing them; - follow or unfollow people and organizations, mute or unmute, and block or unblock, and list who you muted and blocked; diff --git a/app/Http/Controllers/Api/CommitContinuationController.php b/app/Http/Controllers/Api/CommitContinuationController.php new file mode 100644 index 000000000..19fb12f42 --- /dev/null +++ b/app/Http/Controllers/Api/CommitContinuationController.php @@ -0,0 +1,47 @@ +get($request->continuedCommit() ?? $commit, $user); + $commits = $thread->commits->values(); + $nextIds = $commits->skip(1)->pluck('id')->push($thread->nextId)->values(); + + foreach ($commits as $index => $listed) { + $listed->setAttribute('continuation_id', $nextIds[$index]); + } + + // The paginator only links to a next page when it is given one item more than a page. + $page = new CursorPaginator( + $thread->nextId === null ? $commits : $commits->concat([new Commit]), + CommitContinuationQuery::PER_PAGE, + options: ['path' => $request->url(), 'parameters' => ['id']], + ); + + return CommitResource::collection($page)->additional(['meta' => [ + 'position' => $thread->position, + 'length' => $thread->length, + 'is_capped' => $thread->isCapped, + ]]); + } +} diff --git a/app/Http/Requests/ListApiCommitContinuationRequest.php b/app/Http/Requests/ListApiCommitContinuationRequest.php new file mode 100644 index 000000000..43d0e1d91 --- /dev/null +++ b/app/Http/Requests/ListApiCommitContinuationRequest.php @@ -0,0 +1,64 @@ +user(); + assert($user instanceof User); + + $commit = $this->route('commit'); + assert($commit instanceof Commit); + + return $commit->isVisibleTo($user) && $this->continuedCommit()?->isVisibleTo($user) + ? Response::allow() + : Response::denyAsNotFound(); + } + + /** + * The commit whose next commits this page lists: the commit itself, or the one the cursor continues from. + */ + public function continuedCommit(): ?Commit + { + if ($this->continued instanceof Commit) { + return $this->continued; + } + + $commit = $this->route('commit'); + assert($commit instanceof Commit); + + $cursor = $this->query('cursor'); + $id = is_string($cursor) ? Cursor::fromEncoded($cursor)?->toArray()['id'] ?? null : null; + + if ($id === null) { + return $this->continued = $commit; + } + + return $this->continued = is_string($id) && Str::isUuid($id) ? Commit::query()->find($id) : null; + } + + /** + * @return array{} + */ + public function rules(): array + { + return []; + } +} diff --git a/routes/api.php b/routes/api.php index 824068436..fc4ec4047 100644 --- a/routes/api.php +++ b/routes/api.php @@ -4,6 +4,7 @@ use App\Exceptions\ApiException; use App\Http\Controllers\Api\CallController; +use App\Http\Controllers\Api\CommitContinuationController; use App\Http\Controllers\Api\CommitController as ApiCommitController; use App\Http\Controllers\Api\CommitPastelController; use App\Http\Controllers\Api\CommitPinController; @@ -260,6 +261,8 @@ Route::get('commits/{commit}/replies', [CommitReplyController::class, 'index']) ->withTrashed() ->name('api-commit-reply.index'); + Route::get('commits/{commit}/continuation', [CommitContinuationController::class, 'index']) + ->name('api-commit-continuation.index'); // Commit Report: a commit that doesn't exist or was deleted gets the same answer as one the user may not see... Route::post('commits/{commit}/reports', [CommitReportController::class, 'store']) diff --git a/tests/Feature/Controllers/Api/CommitContinuationControllerTest.php b/tests/Feature/Controllers/Api/CommitContinuationControllerTest.php new file mode 100644 index 000000000..dd625fd76 --- /dev/null +++ b/tests/Feature/Controllers/Api/CommitContinuationControllerTest.php @@ -0,0 +1,159 @@ +user = User::factory()->create(); + $this->keyPair = apiKeyPairFor($this->user); +}); + +it('lists the next commits of the author thread as the API shows commits', function (): void { + $author = User::factory()->create(); + $commit = Commit::factory()->for($author)->create(['created_at' => now()->subMinutes(5)]); + $first = Commit::factory()->for($author)->for($commit, 'parent')->create(['created_at' => now()->subMinutes(4)]); + $second = Commit::factory()->for($author)->for($first, 'parent')->create(['created_at' => now()->subMinutes(3)]); + Commit::factory()->for($author)->for($commit, 'parent')->create(['created_at' => now()->subMinutes(2)]); + Commit::factory()->for($first, 'parent')->create(['created_at' => now()->subMinute()]); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $commit)) + ->assertOk() + ->assertJsonPath('data.*.id', [$first->id, $second->id]) + ->assertJsonPath('data.0.author.handle', $author->handle) + ->assertJsonPath('data.0.continuation_id', $second->id) + ->assertJsonPath('data.0.replies_count', 1) + ->assertJsonPath('data.1.continuation_id', null) + ->assertJsonPath('meta.position', 1) + ->assertJsonPath('meta.length', 2) + ->assertJsonPath('meta.is_capped', false) + ->assertJsonPath('meta.next_cursor', null) + ->assertJsonPath('links.next', null); +}); + +it('lists the thread 10 commits at a time with a cursor to the next page', function (): void { + $author = User::factory()->create(); + $commits = [Commit::factory()->for($author)->create()]; + + foreach (range(1, CommitContinuationQuery::PER_PAGE + 2) as $ignored) { + $commits[] = Commit::factory()->for($author)->for(end($commits), 'parent')->create(); + } + + $first = signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $commits[0])); + + $first->assertOk() + ->assertJsonPath('data.*.id', array_map(fn (Commit $commit): string => $commit->id, array_slice($commits, 1, 10))) + ->assertJsonPath('data.9.continuation_id', $commits[11]->id) + ->assertJsonPath('meta.position', 1) + ->assertJsonPath('meta.length', 12); + + $second = signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commits[0], 'cursor' => $first->json('meta.next_cursor')])); + + $second->assertOk() + ->assertJsonPath('data.*.id', [$commits[11]->id, $commits[12]->id]) + ->assertJsonPath('meta.position', 11) + ->assertJsonPath('meta.length', 2) + ->assertJsonPath('meta.next_cursor', null); + + signedApiRequest($this->keyPair, 'GET', (string) $first->json('links.next')) + ->assertOk() + ->assertJsonPath('data.*.id', [$commits[11]->id, $commits[12]->id]); +}); + +it('does not query each commit of the thread, its author or its stars', function (): void { + Queue::fake([DetectCommitLanguage::class]); + $author = User::factory()->create(); + $commits = [Commit::factory()->for($author)->create()]; + + $queries = function () use ($commits): int { + DB::flushQueryLog(); + DB::enableQueryLog(); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $commits[0]))->assertOk(); + + return count(DB::getQueryLog()); + }; + + $create = function (int $count) use (&$commits, $author): void { + foreach (range(1, $count) as $ignored) { + $commits[] = $commit = Commit::factory()->for($author)->for(end($commits), 'parent')->create(); + $commit->stars()->attach([$this->user->id, User::factory()->create()->id]); + } + }; + + $create(2); + + // The first signed request also syncs the user's keys with GitHub, and the first cards load the features... + $queries(); + + $few = $queries(); + + $create(8); + + expect($queries())->toBe($few); +}); + +it('lists the first page for a cursor that is not one', function (mixed $cursor): void { + $commit = Commit::factory()->create(); + $next = Commit::factory()->for($commit->user)->for($commit, 'parent')->create(); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commit, 'cursor' => $cursor])) + ->assertOk() + ->assertJsonPath('data.*.id', [$next->id]); +})->with([ + 'malformed' => ['not-a-cursor'], + 'array' => [['not-a-cursor']], +]); + +it('returns not found for a cursor on something that is not a commit', function (): void { + $commit = Commit::factory()->create(); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commit, 'cursor' => new Cursor(['id' => 'not-a-uuid'])->encode()])) + ->assertNotFound(); +}); + +it('stops the thread at the first commit the user may not see', function (): void { + $author = User::factory()->create(); + $commit = Commit::factory()->for($author)->create(); + $visible = Commit::factory()->for($author)->for($commit, 'parent')->create(); + $pending = Commit::factory()->for($author)->for($visible, 'parent')->pending()->create(); + Commit::factory()->for($author)->for($pending, 'parent')->create(); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $commit)) + ->assertOk() + ->assertJsonPath('data.*.id', [$visible->id]) + ->assertJsonPath('data.0.continuation_id', null) + ->assertJsonPath('meta.length', 1); +}); + +it('returns not found for the thread of a commit the user may not see', function (): void { + $commit = Commit::factory()->pending()->create(); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $commit))->assertNotFound(); +}); + +it('returns not found for the thread of a commit across a block', function (): void { + $commit = Commit::factory()->create(); + Commit::factory()->for($commit->user)->for($commit, 'parent')->create(); + $this->user->blocking()->attach($commit->user); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $commit))->assertNotFound(); +}); + +it('returns not found for a cursor on a commit the user may not see', function (): void { + $commit = Commit::factory()->create(); + $hidden = Commit::factory()->pending()->create(); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commit, 'cursor' => new Cursor(['id' => $hidden->id])->encode()])) + ->assertNotFound(); +}); + +it('rejects requests that are not signed', function (): void { + $this->getJson(route('api-commit-continuation.index', Commit::factory()->create()))->assertUnauthorized(); +}); From 8add84386e0435689d3c931b6ab389d35b5cec82 Mon Sep 17 00:00:00 2001 From: Julien BIANCHI Date: Sat, 31 Oct 2026 18:30:03 +0100 Subject: [PATCH 4/4] fix: tie continuation cursors to the commit they page through A cursor only held the commit a page continues from, so a cursor from one thread sent to the continuation of another commit listed the first thread under the second one's URL. Nothing leaked, since the user had to see that commit, but the commit in the URL no longer scoped the results, unlike the replies. Cursors now also hold the commit in the URL as thread_id, and a cursor given for another commit is not found, like one on a commit the user may not see. --- .../Api/CommitContinuationController.php | 9 +++++---- .../ListApiCommitContinuationRequest.php | 9 ++++++--- .../Api/CommitContinuationControllerTest.php | 19 +++++++++++++++++-- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/app/Http/Controllers/Api/CommitContinuationController.php b/app/Http/Controllers/Api/CommitContinuationController.php index 19fb12f42..99a26bba0 100644 --- a/app/Http/Controllers/Api/CommitContinuationController.php +++ b/app/Http/Controllers/Api/CommitContinuationController.php @@ -17,9 +17,9 @@ { /** * The commits that follow the commit in its author's thread, as the web reads it, paged like its replies: the - * cursor continues from the last commit of a page. position is the place in the thread of the commit a page - * continues from and length how many commits follow it, so position + length is the size of the thread, and - * is_capped says it goes on past that. + * cursor continues the commit's thread from the last commit of a page. position is the place in the thread of the + * commit a page continues from and length how many commits follow it, so position + length is the size of the + * thread, and is_capped says it goes on past that. */ public function index(ListApiCommitContinuationRequest $request, #[CurrentUser] User $user, Commit $commit, CommitContinuationQuery $continuation): AnonymousResourceCollection { @@ -29,13 +29,14 @@ public function index(ListApiCommitContinuationRequest $request, #[CurrentUser] foreach ($commits as $index => $listed) { $listed->setAttribute('continuation_id', $nextIds[$index]); + $listed->setAttribute('thread_id', $commit->id); } // The paginator only links to a next page when it is given one item more than a page. $page = new CursorPaginator( $thread->nextId === null ? $commits : $commits->concat([new Commit]), CommitContinuationQuery::PER_PAGE, - options: ['path' => $request->url(), 'parameters' => ['id']], + options: ['path' => $request->url(), 'parameters' => ['thread_id', 'id']], ); return CommitResource::collection($page)->additional(['meta' => [ diff --git a/app/Http/Requests/ListApiCommitContinuationRequest.php b/app/Http/Requests/ListApiCommitContinuationRequest.php index 43d0e1d91..0ae54e0dd 100644 --- a/app/Http/Requests/ListApiCommitContinuationRequest.php +++ b/app/Http/Requests/ListApiCommitContinuationRequest.php @@ -17,7 +17,7 @@ final class ListApiCommitContinuationRequest extends FormRequest /** * The cursor of a page after the first continues the thread from the last commit of the page before, so the user - * must still see that commit too. + * must still see that commit too, and it only continues the thread of the commit it was given for. */ public function authorize(): Response { @@ -45,13 +45,16 @@ public function continuedCommit(): ?Commit assert($commit instanceof Commit); $cursor = $this->query('cursor'); - $id = is_string($cursor) ? Cursor::fromEncoded($cursor)?->toArray()['id'] ?? null : null; + $parameters = is_string($cursor) ? Cursor::fromEncoded($cursor)?->toArray() ?? [] : []; + $id = $parameters['id'] ?? null; if ($id === null) { return $this->continued = $commit; } - return $this->continued = is_string($id) && Str::isUuid($id) ? Commit::query()->find($id) : null; + return $this->continued = ($parameters['thread_id'] ?? null) === $commit->id && is_string($id) && Str::isUuid($id) + ? Commit::query()->find($id) + : null; } /** diff --git a/tests/Feature/Controllers/Api/CommitContinuationControllerTest.php b/tests/Feature/Controllers/Api/CommitContinuationControllerTest.php index dd625fd76..7d1ce77c9 100644 --- a/tests/Feature/Controllers/Api/CommitContinuationControllerTest.php +++ b/tests/Feature/Controllers/Api/CommitContinuationControllerTest.php @@ -114,10 +114,25 @@ it('returns not found for a cursor on something that is not a commit', function (): void { $commit = Commit::factory()->create(); - signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commit, 'cursor' => new Cursor(['id' => 'not-a-uuid'])->encode()])) + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commit, 'cursor' => new Cursor(['thread_id' => $commit->id, 'id' => 'not-a-uuid'])->encode()])) ->assertNotFound(); }); +it('returns not found for a cursor from the thread of another commit', function (): void { + $commit = Commit::factory()->create(); + Commit::factory()->for($commit->user)->for($commit, 'parent')->create(); + $other = [Commit::factory()->create()]; + + foreach (range(1, CommitContinuationQuery::PER_PAGE + 1) as $ignored) { + $other[] = Commit::factory()->for($other[0]->user)->for(end($other), 'parent')->create(); + } + + $cursor = signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', $other[0]))->json('meta.next_cursor'); + + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$other[0], 'cursor' => $cursor]))->assertOk(); + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commit, 'cursor' => $cursor]))->assertNotFound(); +}); + it('stops the thread at the first commit the user may not see', function (): void { $author = User::factory()->create(); $commit = Commit::factory()->for($author)->create(); @@ -150,7 +165,7 @@ $commit = Commit::factory()->create(); $hidden = Commit::factory()->pending()->create(); - signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commit, 'cursor' => new Cursor(['id' => $hidden->id])->encode()])) + signedApiRequest($this->keyPair, 'GET', route('api-commit-continuation.index', [$commit, 'cursor' => new Cursor(['thread_id' => $commit->id, 'id' => $hidden->id])->encode()])) ->assertNotFound(); });