From b0da2cdeccf40f6d168d2619b792c5097b0ffbdd Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Sat, 26 Sep 2026 02:19:13 +0200 Subject: [PATCH] fix: restrict media-search to the query, not just editable() after it I1: query the attachments themselves author-restricted when the user lacks edit_others_posts (PostGuard::ownPostsOnly, extracted from ContentQueryAbility so content-query reuses the exact same rule) instead of relying solely on a post-query editable() filter, which could leak the existence of other users' attachments via an empty page plus next_cursor. editable() stays as a backstop. Also: M1 document that only status=inherit attachments are listed; M2 tie-break ordering by ID (content-query and media-search) for stable pagination; M3 no next_cursor once the last candidate was examined; M4 reject an explicit non-image mime with missing_alt; M5 additional coverage (posts_per_page, cursor offset pass-through, invalid cursor, post_status/date/mime args); mime maxLength 100. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7 --- src/Integrations/Media/MediaSearchAbility.php | 73 +++++++-- .../WordPress/Content/ContentQueryAbility.php | 11 +- src/Integrations/WordPress/PostGuard.php | 22 +++ .../Media/MediaSearchAbilityTest.php | 153 +++++++++++++++++- .../Content/ContentQueryAbilityTest.php | 3 +- .../Integrations/WordPress/PostGuardTest.php | 21 +++ 6 files changed, 262 insertions(+), 21 deletions(-) diff --git a/src/Integrations/Media/MediaSearchAbility.php b/src/Integrations/Media/MediaSearchAbility.php index b71081f..ce1c7f3 100644 --- a/src/Integrations/Media/MediaSearchAbility.php +++ b/src/Integrations/Media/MediaSearchAbility.php @@ -7,6 +7,7 @@ namespace LIT\MCP\Integrations\Media; use LIT\MCP\Abilities\Ability; use LIT\MCP\Abilities\Cursor; use LIT\MCP\Abilities\Errors; +use LIT\MCP\Integrations\WordPress\PostGuard; use WP_Error; use WP_Post; @@ -40,12 +41,13 @@ final class MediaSearchAbility implements Ability public function description(): string { return 'Sucht in der Mediathek nach Text (Titel, Dateiname, Beschreibung), MIME-Typ (z. B. image oder ' - . 'application/pdf) und Zeitraum. Nur Medien, die der Nutzer bearbeiten darf; Papierkorb ausgeschlossen. ' - . 'missing_alt: Bilder ohne Alt-Text. unused: Medien ohne gefundene Verwendung – "keine Verwendung ' - . 'gefunden" ist keine Garantie (Details je Medium über lit/media-get); als "möglicherweise verwendet" ' - . 'erkannte Medien zählen bereits als verwendet und werden nicht gelistet. Prüft dafür je Aufruf ' - . 'höchstens ' . self::MAX_SCAN . ' Medien; wird die Prüfung dadurch unterbrochen, folgt eine Warnung ' - . 'und mit next_cursor kann fortgesetzt werden.'; + . 'application/pdf) und Zeitraum. Nur Medien mit Status inherit (reguläre Mediathek-Einträge, kein ' + . 'Papierkorb), die der Nutzer bearbeiten darf. missing_alt: Bilder ohne Alt-Text (mime muss dabei leer ' + . 'oder ein Bild-Typ sein). unused: Medien ohne gefundene Verwendung – "keine Verwendung gefunden" ist ' + . 'keine Garantie (Details je Medium über lit/media-get); als "möglicherweise verwendet" erkannte ' + . 'Medien zählen bereits als verwendet und werden nicht gelistet. Prüft dafür je Aufruf höchstens ' + . self::MAX_SCAN . ' Medien; wird die Prüfung dadurch unterbrochen, folgt eine Warnung und mit ' + . 'next_cursor kann fortgesetzt werden.'; } public function inputSchema(): array @@ -54,7 +56,7 @@ final class MediaSearchAbility implements Ability 'type' => 'object', 'properties' => array_merge([ 'search' => ['type' => 'string', 'maxLength' => 200], - 'mime' => ['type' => 'string', 'pattern' => '^[a-z]+(/[a-z0-9.+-]+)?$'], + 'mime' => ['type' => 'string', 'maxLength' => 100, 'pattern' => '^[a-z]+(/[a-z0-9.+-]+)?$'], 'date_after' => ['type' => 'string', 'pattern' => self::DATE_PATTERN], 'date_before' => ['type' => 'string', 'pattern' => self::DATE_PATTERN], 'missing_alt' => ['type' => 'boolean'], @@ -101,6 +103,11 @@ final class MediaSearchAbility implements Ability return Cursor::invalid(); } + $mimeError = $this->rejectNonImageMimeForMissingAlt($input); + if ($mimeError !== null) { + return $mimeError; + } + $limit = Cursor::limit($input['limit'] ?? null); $args = $this->queryArgs($input); @@ -118,6 +125,27 @@ final class MediaSearchAbility implements Ability ]; } + /** + * missing_alt only makes sense for images; an explicit non-image mime would otherwise + * be silently ignored (post_mime_type overwritten) and quietly return nothing. + * + * @param array $input + */ + private function rejectNonImageMimeForMissingAlt(array $input): ?WP_Error + { + $mime = isset($input['mime']) ? (string) $input['mime'] : ''; + $isImage = $mime === '' || $mime === 'image' || str_starts_with($mime, 'image/'); + if (($input['missing_alt'] ?? false) !== true || $isImage) { + return null; + } + + return Errors::invalidInput( + 'missing_alt gilt nur für Bilder.', + ['mime' => 'kein Bild-MIME-Typ'], + 'mime weglassen, auf "image" setzen oder einen "image/…"-Subtyp verwenden.' + ); + } + /** * @param array $input * @return array @@ -127,12 +155,22 @@ final class MediaSearchAbility implements Ability $args = [ 'post_type' => 'attachment', 'post_status' => 'inherit', - 'orderby' => 'date', - 'order' => 'DESC', + // A secondary ID tie-breaker keeps paging stable when several attachments share + // the exact same date (common with bulk uploads). + 'orderby' => ['date' => 'DESC', 'ID' => 'DESC'], 'no_found_rows' => true, 'suppress_filters' => false, ]; + // Without edit_others_posts, restrict the query itself to the user's own uploads – + // filtering with editable() afterwards is not enough on its own: an unrestricted + // query could otherwise come back empty (or short) with a next_cursor that quietly + // reveals other users' attachments exist without ever showing them. + $ownerId = PostGuard::ownPostsOnly('attachment'); + if ($ownerId !== null) { + $args['author'] = $ownerId; + } + if (isset($input['search']) && $input['search'] !== '') { $args['s'] = (string) $input['search']; } @@ -174,6 +212,9 @@ final class MediaSearchAbility implements Ability $page = Cursor::page(array_values($rows), $limit, $offset); return [ + // editable() stays as a backstop even though the query is already author-restricted + // above: e.g. a per-attachment cap denial (a filter, or the author losing rights + // after upload) must still drop the row instead of exposing it. 'items' => array_values(array_filter($page['items'], [self::class, 'editable'])), 'next_cursor' => $page['next_cursor'], 'warnings' => [], @@ -197,6 +238,7 @@ final class MediaSearchAbility implements Ability $items = []; $position = $offset; $examined = 0; + $reachedLimit = false; foreach ($candidates as $attachment) { if ($examined >= self::MAX_SCAN) { @@ -209,15 +251,22 @@ final class MediaSearchAbility implements Ability if (self::editable($attachment) && ! $this->finder->isUsed((int) $attachment->ID)) { $items[] = $attachment; if (count($items) === $limit) { - return ['items' => $items, 'next_cursor' => Cursor::encode($position), 'warnings' => []]; + $reachedLimit = true; + break; } } } - if (count($candidates) <= self::MAX_SCAN) { + // The extra (MAX_SCAN + 1)th candidate, when present, was deliberately never examined – + // its mere presence is enough to know more candidates remain either way. + if ($examined >= count($candidates)) { return ['items' => $items, 'next_cursor' => null, 'warnings' => []]; } + if ($reachedLimit) { + return ['items' => $items, 'next_cursor' => Cursor::encode($position), 'warnings' => []]; + } + return [ 'items' => $items, 'next_cursor' => Cursor::encode($position), @@ -228,7 +277,7 @@ final class MediaSearchAbility implements Ability ]; } - /** Only attachments the current user may edit are ever listed; trashed ones never reach here. */ + /** Backstop: editable() is checked even though the query is already author-restricted. */ private static function editable(WP_Post $attachment): bool { return current_user_can('edit_post', (int) $attachment->ID); diff --git a/src/Integrations/WordPress/Content/ContentQueryAbility.php b/src/Integrations/WordPress/Content/ContentQueryAbility.php index a99c284..464511e 100644 --- a/src/Integrations/WordPress/Content/ContentQueryAbility.php +++ b/src/Integrations/WordPress/Content/ContentQueryAbility.php @@ -176,8 +176,10 @@ final class ContentQueryAbility implements Ability // Without edit_others_posts, even a status the user may edit in principle must // stay limited to their own posts – otherwise the query would leak drafts etc. // of other authors that happen to pass the per-status gate below. - $restrictedToOwnPosts = ! current_user_can($this->capability($caps, 'edit_others_posts')); + $ownerId = PostGuard::ownPostsOnly($postType); + $restrictedToOwnPosts = $ownerId !== null; + $order = strtoupper((string) ($input['order'] ?? 'desc')); $args = [ 'post_type' => $postType, 'post_status' => $this->allowedStatuses( @@ -185,15 +187,16 @@ final class ContentQueryAbility implements Ability $caps, $restrictedToOwnPosts ), - 'orderby' => (string) ($input['orderby'] ?? 'date'), - 'order' => strtoupper((string) ($input['order'] ?? 'desc')), + // A secondary ID tie-breaker keeps paging stable when several entries share the + // exact same orderby value (e.g. bulk-imported posts with an identical date). + 'orderby' => [(string) ($input['orderby'] ?? 'date') => $order, 'ID' => $order], 'no_found_rows' => true, 'ignore_sticky_posts' => true, 'suppress_filters' => false, ]; if ($restrictedToOwnPosts) { - $args['author'] = get_current_user_id(); + $args['author'] = $ownerId; } if (isset($input['search']) && $input['search'] !== '') { diff --git a/src/Integrations/WordPress/PostGuard.php b/src/Integrations/WordPress/PostGuard.php index 3aa842e..b711ad9 100644 --- a/src/Integrations/WordPress/PostGuard.php +++ b/src/Integrations/WordPress/PostGuard.php @@ -119,6 +119,28 @@ final class PostGuard return $object; } + /** + * Whether a list/search query for this post type must stay restricted to the current + * user's own entries because they lack edit_others_posts. Filtering per row after the + * query (e.g. current_user_can('edit_post', $id)) is not enough on its own: without this, + * a restricted user's query still runs unrestricted, so a page can come back empty (or + * short) with a next_cursor that quietly reveals other users' entries exist without ever + * showing them – the same defect ContentQueryAbility guards against for content-query. + * + * @return int|null The current user id when restricted, null when the query may run + * unrestricted (author filter must then be left out entirely). + */ + public static function ownPostsOnly(string $postType): ?int + { + $typeObject = get_post_type_object($postType); + $caps = is_object($typeObject) && isset($typeObject->cap) ? $typeObject->cap : null; + $capability = $caps !== null && isset($caps->edit_others_posts) + ? (string) $caps->edit_others_posts + : 'edit_others_posts'; + + return current_user_can($capability) ? null : get_current_user_id(); + } + public static function id(mixed $value): ?int { if (is_int($value)) { diff --git a/tests/Unit/Integrations/Media/MediaSearchAbilityTest.php b/tests/Unit/Integrations/Media/MediaSearchAbilityTest.php index ee667b4..13651f7 100644 --- a/tests/Unit/Integrations/Media/MediaSearchAbilityTest.php +++ b/tests/Unit/Integrations/Media/MediaSearchAbilityTest.php @@ -29,9 +29,16 @@ final class MediaSearchAbilityTest extends TestCase return array_map(static fn (int $id): WP_Post => new WP_Post(['ID' => $id, 'post_type' => 'attachment']), $ids); } + /** No edit_others_posts cap mapped: PostGuard::ownPostsOnly() falls back to the raw name. */ + private function unrestrictedPostType(): void + { + Functions\when('get_post_type_object')->justReturn((object) ['cap' => (object) []]); + } + public function testMissingAltLimitsToImagesWithoutAltText(): void { Functions\when('current_user_can')->justReturn(true); + $this->unrestrictedPostType(); Functions\expect('get_posts')->once()->with(Mockery::on(static fn (array $args): bool => $args['post_type'] === 'attachment' && $args['post_mime_type'] === 'image' @@ -52,10 +59,62 @@ final class MediaSearchAbilityTest extends TestCase self::assertSame([], $result['warnings']); } + public function testMissingAltRejectsNonImageMime(): void + { + Functions\when('current_user_can')->justReturn(true); + $this->unrestrictedPostType(); + + $result = (new MediaSearchAbility($this->presenter(), Mockery::mock(MediaUsageFinder::class))) + ->execute(['missing_alt' => true, 'mime' => 'application/pdf']); + + self::assertSame('lit_validation_failed', $result->get_error_code()); + } + + public function testMissingAltAllowsAnImageSubtypeMime(): void + { + Functions\when('current_user_can')->justReturn(true); + $this->unrestrictedPostType(); + Functions\when('get_posts')->justReturn([]); + + $result = (new MediaSearchAbility($this->presenter(), Mockery::mock(MediaUsageFinder::class))) + ->execute(['missing_alt' => true, 'mime' => 'image/png']); + + self::assertSame([], $result['items']); + } + + public function testQueryArgsCoverStatusDateAndMime(): void + { + Functions\when('current_user_can')->justReturn(true); + $this->unrestrictedPostType(); + Functions\expect('get_posts')->once()->with(Mockery::on(static fn (array $args): bool => + $args['post_status'] === 'inherit' + && $args['post_mime_type'] === 'application/pdf' + && $args['date_query'] === [['after' => '2026-01-01', 'before' => '2026-02-01', 'inclusive' => true]] + && $args['posts_per_page'] === 6 + && $args['offset'] === 3))->andReturn([]); + + (new MediaSearchAbility($this->presenter(), Mockery::mock(MediaUsageFinder::class)))->execute([ + 'mime' => 'application/pdf', + 'date_after' => '2026-01-01', + 'date_before' => '2026-02-01', + 'limit' => 5, + 'cursor' => Cursor::encode(3), + ]); + } + + public function testInvalidCursorIsRejected(): void + { + $result = (new MediaSearchAbility($this->presenter(), Mockery::mock(MediaUsageFinder::class))) + ->execute(['cursor' => '???']); + + self::assertSame('lit_validation_failed', $result->get_error_code()); + } + public function testExcludesAttachmentsTheUserCannotEdit(): void { Functions\when('get_posts')->justReturn($this->attachments(1, 2)); - Functions\when('current_user_can')->alias(static fn (string $cap, int $id): bool => $id !== 2); + $this->unrestrictedPostType(); + Functions\when('current_user_can')->alias(static fn (string $cap, int $id = 0): bool => $id !== 2); $result = (new MediaSearchAbility($this->presenter(), Mockery::mock(MediaUsageFinder::class))) ->execute([]); @@ -63,10 +122,38 @@ final class MediaSearchAbilityTest extends TestCase self::assertSame([['id' => 1]], $result['items']); } + public function testSearchIsRestrictedToOwnAttachmentsWithoutEditOthersPosts(): void + { + Functions\when('get_post_type_object')->justReturn( + (object) ['cap' => (object) ['edit_others_posts' => 'edit_others_posts']] + ); + Functions\when('current_user_can')->alias(static fn (string $cap): bool => $cap !== 'edit_others_posts'); + Functions\when('get_current_user_id')->justReturn(7); + Functions\expect('get_posts')->once()->with( + Mockery::on(static fn (array $args): bool => ($args['author'] ?? null) === 7) + )->andReturn([]); + + (new MediaSearchAbility($this->presenter(), Mockery::mock(MediaUsageFinder::class)))->execute([]); + } + + public function testSearchIsNotRestrictedToAuthorWithEditOthersPosts(): void + { + Functions\when('get_post_type_object')->justReturn( + (object) ['cap' => (object) ['edit_others_posts' => 'edit_others_posts']] + ); + Functions\when('current_user_can')->justReturn(true); + Functions\expect('get_posts')->once()->with( + Mockery::on(static fn (array $args): bool => ! array_key_exists('author', $args)) + )->andReturn([]); + + (new MediaSearchAbility($this->presenter(), Mockery::mock(MediaUsageFinder::class)))->execute([]); + } + public function testUnusedFiltersThroughTheUsageFinder(): void { Functions\when('get_posts')->justReturn($this->attachments(1, 2, 3)); Functions\when('current_user_can')->justReturn(true); + $this->unrestrictedPostType(); $finder = Mockery::mock(MediaUsageFinder::class); $finder->allows('isUsed')->andReturnUsing(static fn (int $id): bool => $id === 2); @@ -81,6 +168,7 @@ final class MediaSearchAbilityTest extends TestCase { Functions\when('get_posts')->justReturn($this->attachments(1, 2, 3)); Functions\when('current_user_can')->justReturn(true); + $this->unrestrictedPostType(); $finder = Mockery::mock(MediaUsageFinder::class); $finder->allows('isUsed')->andReturn(false); @@ -90,10 +178,25 @@ final class MediaSearchAbilityTest extends TestCase self::assertSame(1, Cursor::decode($result['next_cursor'])); } + public function testUnusedHasNoCursorWhenTheLastCandidateWasExaminedAtTheLimit(): void + { + Functions\when('get_posts')->justReturn($this->attachments(1)); + Functions\when('current_user_can')->justReturn(true); + $this->unrestrictedPostType(); + $finder = Mockery::mock(MediaUsageFinder::class); + $finder->allows('isUsed')->andReturn(false); + + $result = (new MediaSearchAbility($this->presenter(), $finder))->execute(['unused' => true, 'limit' => 1]); + + self::assertSame([['id' => 1]], $result['items']); + self::assertNull($result['next_cursor']); + } + public function testUnusedSkipsAttachmentsTheUserCannotEdit(): void { Functions\when('get_posts')->justReturn($this->attachments(1, 2, 3)); - Functions\when('current_user_can')->alias(static fn (string $cap, int $id): bool => $id !== 2); + $this->unrestrictedPostType(); + Functions\when('current_user_can')->alias(static fn (string $cap, int $id = 0): bool => $id !== 2); $finder = Mockery::mock(MediaUsageFinder::class); $finder->allows('isUsed')->andReturn(false); @@ -105,8 +208,11 @@ final class MediaSearchAbilityTest extends TestCase public function testUnusedWarnsAndPagesWhenTheScanCapIsReached(): void { - Functions\when('get_posts')->justReturn($this->attachments(...range(1, 21))); Functions\when('current_user_can')->justReturn(true); + $this->unrestrictedPostType(); + Functions\expect('get_posts')->once()->with( + Mockery::on(static fn (array $args): bool => $args['posts_per_page'] === 21 && $args['offset'] === 0) + )->andReturn($this->attachments(...range(1, 21))); $finder = Mockery::mock(MediaUsageFinder::class); $finder->allows('isUsed')->andReturn(true); @@ -117,6 +223,47 @@ final class MediaSearchAbilityTest extends TestCase self::assertNotSame([], $result['warnings']); } + public function testUnusedCursorOffsetIsPassedThrough(): void + { + Functions\when('current_user_can')->justReturn(true); + $this->unrestrictedPostType(); + Functions\expect('get_posts')->once()->with( + Mockery::on(static fn (array $args): bool => $args['offset'] === 5) + )->andReturn([]); + $finder = Mockery::mock(MediaUsageFinder::class); + + (new MediaSearchAbility($this->presenter(), $finder))->execute(['unused' => true, 'cursor' => Cursor::encode(5)]); + } + + public function testUnusedIsRestrictedToOwnAttachmentsWithoutEditOthersPosts(): void + { + Functions\when('get_post_type_object')->justReturn( + (object) ['cap' => (object) ['edit_others_posts' => 'edit_others_posts']] + ); + Functions\when('current_user_can')->alias(static fn (string $cap): bool => $cap !== 'edit_others_posts'); + Functions\when('get_current_user_id')->justReturn(7); + Functions\expect('get_posts')->once()->with( + Mockery::on(static fn (array $args): bool => ($args['author'] ?? null) === 7) + )->andReturn([]); + + (new MediaSearchAbility($this->presenter(), Mockery::mock(MediaUsageFinder::class))) + ->execute(['unused' => true]); + } + + public function testUnusedIsNotRestrictedToAuthorWithEditOthersPosts(): void + { + Functions\when('get_post_type_object')->justReturn( + (object) ['cap' => (object) ['edit_others_posts' => 'edit_others_posts']] + ); + Functions\when('current_user_can')->justReturn(true); + Functions\expect('get_posts')->once()->with( + Mockery::on(static fn (array $args): bool => ! array_key_exists('author', $args)) + )->andReturn([]); + + (new MediaSearchAbility($this->presenter(), Mockery::mock(MediaUsageFinder::class))) + ->execute(['unused' => true]); + } + public function testRequiresUploadRights(): void { Functions\when('current_user_can')->justReturn(false); diff --git a/tests/Unit/Integrations/WordPress/Content/ContentQueryAbilityTest.php b/tests/Unit/Integrations/WordPress/Content/ContentQueryAbilityTest.php index 6eccf66..e610bf2 100644 --- a/tests/Unit/Integrations/WordPress/Content/ContentQueryAbilityTest.php +++ b/tests/Unit/Integrations/WordPress/Content/ContentQueryAbilityTest.php @@ -37,8 +37,7 @@ final class ContentQueryAbilityTest extends TestCase && $args['s'] === 'Anna' && $args['posts_per_page'] === 3 && $args['offset'] === 4 - && $args['orderby'] === 'title' - && $args['order'] === 'ASC' + && $args['orderby'] === ['title' => 'ASC', 'ID' => 'ASC'] && $args['tax_query'] === [ 'relation' => 'OR', ['taxonomy' => 'category', 'field' => 'term_id', 'terms' => [4]], diff --git a/tests/Unit/Integrations/WordPress/PostGuardTest.php b/tests/Unit/Integrations/WordPress/PostGuardTest.php index a06d948..f36fd87 100644 --- a/tests/Unit/Integrations/WordPress/PostGuardTest.php +++ b/tests/Unit/Integrations/WordPress/PostGuardTest.php @@ -85,6 +85,27 @@ final class PostGuardTest extends TestCase self::assertNull(PostGuard::statusStable(new WP_Post(['post_status' => 'publish']))); } + public function testOwnPostsOnlyReturnsTheCurrentUserWithoutEditOthersPosts(): void + { + Functions\when('get_post_type_object')->justReturn( + (object) ['cap' => (object) ['edit_others_posts' => 'edit_others_posts']] + ); + Functions\when('current_user_can')->alias(static fn (string $cap): bool => $cap !== 'edit_others_posts'); + Functions\when('get_current_user_id')->justReturn(7); + + self::assertSame(7, PostGuard::ownPostsOnly('page')); + } + + public function testOwnPostsOnlyReturnsNullWithEditOthersPosts(): void + { + Functions\when('get_post_type_object')->justReturn( + (object) ['cap' => (object) ['edit_others_posts' => 'edit_others_posts']] + ); + Functions\when('current_user_can')->justReturn(true); + + self::assertNull(PostGuard::ownPostsOnly('page')); + } + public function testPostTypeChecksAvailabilityAndCapability(): void { $object = (object) ['cap' => (object) ['create_posts' => 'edit_pages']];