Gate GET /subject-labels by per-page read permission

The subject-labels autocomplete endpoint returned the labels of Subjects on
pages the caller cannot read, and accepted an uncapped limit, so a per-result
permission check would have been unbounded work.

- Cap limit at 50 (PARAM_MAX), matching the layouts endpoint.
- Traverse Page-[:HasSubject]->Subject, scoped to the current wiki on both the
  page and the subject, so the page id reaches PHP and resolves within this
  wiki. Drop rows the caller cannot read via PageReadAuthorizer, mirroring
  DatabaseSchemaNameLookup. Denied Subjects are simply absent, never an error,
  so restricted pages cannot be enumerated.
- Over-fetch, then authorize row by row and stop at the limit, so read
  restrictions tend not to shorten the result while the expensive per-page
  check stays bounded to the rows actually needed.

The cross-wiki wiki_id leak and Subject-node identity the issue also raised are
already handled elsewhere (5d802e6 and ADR 22 respectively).

For #1060

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: alistair3149

docs/api/rest-api.md

@@ -15,11 +15,11 @@ Every endpoint is also published as a complete [OpenAPI 3.0 description](#full-s
 
 ## Permissions
 
-The Subject, page-subjects, Schema, Layout, RDF export, and entity-dereference read endpoints enforce the caller's
-per-page `read` permission; page protection and `$wgNamespaceProtection` do not restrict them, because MediaWiki's
-`read` action ignores both. When you may not read a page they respond as if the data were absent — a `null` value, an
-empty list, or a `404` — never a `403`. `GET /subject-labels` is the exception: it filters only by wiki and Schema, not
-by per-page `read`.
+The Subject, page-subjects, subject-labels, Schema, Layout, RDF export, and entity-dereference read endpoints enforce
+the caller's per-page `read` permission; page protection and `$wgNamespaceProtection` do not restrict them, because
+MediaWiki's `read` action ignores both. When you may not read a page they respond as if the data were absent — a `null`
+value, an empty list, or a `404` — never a `403`. `GET /subject-labels` omits the labels of Subjects whose page you
+cannot read; because that filter runs per result, it caps `limit` at 50.
 
 Subject write endpoints require per-page `edit` permission and answer `403` on denial.
 

src/Application/SubjectLabelLookup.php

@@ -7,6 +7,9 @@
 interface SubjectLabelLookup {
 
 	/**
+	 * @param int $limit Maximum results to return. Callers must cap this to a small value: an
+	 *   implementation may over-fetch and run a per-result permission check, so an unbounded limit
+	 *   is unbounded work. The REST entry point caps it at 50.
 	 * @return SubjectLabelLookupResult[]
 	 */
 	public function getSubjectLabelsMatching( string $search, int $limit, string $schemaName ): array;

src/EntryPoints/REST/GetSubjectLabelsApi.php

@@ -10,6 +10,7 @@
 use ProfessionalWiki\NeoWiki\Application\SubjectLabelLookupResult;
 use ProfessionalWiki\NeoWiki\NeoWikiExtension;
 use Wikimedia\ParamValidator\ParamValidator;
+use Wikimedia\ParamValidator\TypeDef\IntegerDef;
 
 class GetSubjectLabelsApi extends SimpleHandler {
 
@@ -57,6 +58,8 @@ public function getParamSettings(): array {
 				ParamValidator::PARAM_TYPE => 'integer',
 				ParamValidator::PARAM_REQUIRED => false,
 				ParamValidator::PARAM_DEFAULT => 10,
+				IntegerDef::PARAM_MIN => 1,
+				IntegerDef::PARAM_MAX => 50,
 				self::PARAM_DESCRIPTION => 'Maximum number of items to return.',
 			],
 		];

src/GraphDatabasePlugins/Neo4j/Persistence/Neo4jSubjectLabelLookup.php

@@ -7,14 +7,24 @@
 use Laudis\Neo4j\Contracts\ClientInterface;
 use Laudis\Neo4j\Contracts\TransactionInterface;
 use Laudis\Neo4j\Databags\SummarizedResult;
+use ProfessionalWiki\NeoWiki\Application\PageReadAuthorizer;
 use ProfessionalWiki\NeoWiki\Application\SubjectLabelLookup;
 use ProfessionalWiki\NeoWiki\Application\SubjectLabelLookupResult;
+use ProfessionalWiki\NeoWiki\Domain\Page\PageId;
 
 class Neo4jSubjectLabelLookup implements SubjectLabelLookup {
 
+	/**
+	 * Rows are read past the requested limit so that dropping Subjects on pages the caller cannot
+	 * read still tends to fill it. A heuristic, not a guarantee: a run of unreadable Subjects longer
+	 * than the extra headroom can still shorten the result.
+	 */
+	private const int OVERFETCH_FACTOR = 4;
+
 	public function __construct(
 		private readonly ClientInterface $client,
 		private readonly string $wikiId,
+		private readonly PageReadAuthorizer $readAuthorizer,
 	) {
 	}
 
@@ -26,36 +36,63 @@ public function getSubjectLabelsMatching( string $search, int $limit, string $sc
 			return [];
 		}
 
+		$results = [];
+
+		// Authorize rows one at a time and stop once the limit is filled, so when readable rows are
+		// plentiful the expensive per-page check runs about `limit` times rather than across the
+		// whole over-fetched window. A mostly-unreadable search still checks the whole window.
+		foreach ( $this->fetchLabels( $search, $limit * self::OVERFETCH_FACTOR, $schemaName ) as $row ) {
+			if ( count( $results ) >= $limit ) {
+				break;
+			}
+
+			if ( $this->readAuthorizer->authorizeReadByPageId( new PageId( $row['pageId'] ) ) ) {
+				$results[] = new SubjectLabelLookupResult( id: $row['id'], label: $row['name'] );
+			}
+		}
+
+		return $results;
+	}
+
+	/**
+	 * The Subject is reached through a Page owned by the current wiki, so the caller can check
+	 * per-page read access and the returned page id always resolves within this wiki (page ids
+	 * are unique only per wiki).
+	 *
+	 * @return list<array{id: string, name: string, pageId: int}>
+	 */
+	private function fetchLabels( string $search, int $limit, string $schemaName ): array {
 		return $this->client->readTransaction(
 			function ( TransactionInterface $transaction ) use ( $search, $limit, $schemaName ): array {
 				/**
 				 * @var SummarizedResult $result
 				 */
 				$result = $transaction->run(
-					"MATCH (n:Subject)
+					"MATCH (page:Page { wiki_id: \$wikiId })-[:HasSubject]->(n:Subject)
 					 WHERE toLower(n.name) STARTS WITH toLower(\$search)
 					 AND \$schemaName IN labels(n)
 					 AND n.wiki_id = \$wikiId
-					 RETURN n.id AS id, n.name AS name
+					 RETURN n.id AS id, n.name AS name, page.id AS pageId
 					 ORDER BY n.name
 					 LIMIT \$limit",
 					[
 						'search' => $search,
-						'limit' => (int)$limit,
+						'limit' => $limit,
 						'schemaName' => $schemaName,
 						'wikiId' => $this->wikiId,
 					]
 				);
 
-				$subjects = [];
+				$rows = [];
 				foreach ( $result as $row ) {
-					$subjects[] = new SubjectLabelLookupResult(
-						id: $row->get( 'id' ),
-						label: $row->get( 'name' )
-					);
+					$rows[] = [
+						'id' => $row->get( 'id' ),
+						'name' => $row->get( 'name' ),
+						'pageId' => (int)$row->get( 'pageId' ),
+					];
 				}
 
-				return $subjects;
+				return $rows;
 			}
 		);
 	}

src/NeoWikiExtension.php

@@ -997,6 +997,7 @@ public function getSubjectLabelLookup(): SubjectLabelLookup {
 		return new Neo4jSubjectLabelLookup(
 			client: $this->getReadOnlyNeo4jClient(),
 			wikiId: $this->config->wikiId,
+			readAuthorizer: $this->newPageReadAuthorizer( $this->getRequestAuthority() ),
 		);
 	}
 

tests/phpunit/EntryPoints/REST/GetSubjectLabelsApiTest.php

@@ -0,0 +1,35 @@
+<?php
+
+declare( strict_types = 1 );
+
+namespace ProfessionalWiki\NeoWiki\Tests\EntryPoints\REST;
+
+use MediaWiki\Rest\HttpException;
+use MediaWiki\Rest\RequestData;
+use MediaWiki\Tests\Rest\Handler\HandlerTestTrait;
+use ProfessionalWiki\NeoWiki\EntryPoints\REST\GetSubjectLabelsApi;
+use ProfessionalWiki\NeoWiki\Tests\NeoWikiIntegrationTestCase;
+
+/**
+ * @covers \ProfessionalWiki\NeoWiki\EntryPoints\REST\GetSubjectLabelsApi
+ * @group Database
+ */
+class GetSubjectLabelsApiTest extends NeoWikiIntegrationTestCase {
+	use HandlerTestTrait;
+
+	public function testRejectsLimitAboveMaximum(): void {
+		// The endpoint runs a per-result read-permission check, so an uncapped limit is an
+		// unbounded-work vector. The cap is enforced before the handler runs any query (#1060).
+		$this->expectException( HttpException::class );
+		$this->expectExceptionCode( 400 );
+
+		$this->executeHandler(
+			new GetSubjectLabelsApi(),
+			new RequestData( [
+				'method' => 'GET',
+				'queryParams' => [ 'schema' => 'Person', 'limit' => '100000' ],
+			] )
+		);
+	}
+
+}

tests/phpunit/GraphDatabasePlugins/Neo4j/Persistence/Neo4jSubjectLabelLookupTest.php

@@ -5,6 +5,7 @@
 namespace ProfessionalWiki\NeoWiki\Tests\GraphDatabasePlugins\Neo4j\Persistence;
 
 use Laudis\Neo4j\Contracts\ClientInterface;
+use ProfessionalWiki\NeoWiki\Application\PageReadAuthorizer;
 use ProfessionalWiki\NeoWiki\Application\SubjectLabelLookupResult;
 use ProfessionalWiki\NeoWiki\Domain\GraphDatabase\GraphDatabasePlugin;
 use ProfessionalWiki\NeoWiki\Domain\Schema\SchemaName;
@@ -19,6 +20,8 @@
 use ProfessionalWiki\NeoWiki\Tests\Data\TestSubject;
 use ProfessionalWiki\NeoWiki\Tests\NeoWikiIntegrationTestCase;
 use ProfessionalWiki\NeoWiki\Tests\TestDoubles\InMemorySchemaLookup;
+use ProfessionalWiki\NeoWiki\Tests\TestDoubles\SelectivePageReadAuthorizer;
+use ProfessionalWiki\NeoWiki\Tests\TestDoubles\StubPageReadAuthorizer;
 
 /**
  * @covers \ProfessionalWiki\NeoWiki\GraphDatabasePlugins\Neo4j\Persistence\Neo4jSubjectLabelLookup
@@ -142,6 +145,77 @@ public function testExcludesSubjectsWithoutWikiId(): void {
 		);
 	}
 
+	public function testExcludesForeignSubjectsReachableThroughALocalPage(): void {
+		$this->saveSubjects( new SubjectMap(
+			TestSubject::build( id: self::SUBJECT_ID_1, label: new SubjectLabel( 'Apple Pie' ) ),
+		) );
+
+		// A cross-wiki-shared Subject id (ADR 22) can leave a local Page holding a Subject stamped
+		// for another wiki. The Subject filter must still withhold it so its foreign label, and a
+		// page id that resolves against the wrong wiki, cannot surface.
+		$this->getClient()->run(
+			'CREATE (:Page { id: 500, wiki_id: $wikiId })-[:HasSubject { isMain: false }]->'
+				. '(:Subject:' . Cypher::escape( TestSubject::DEFAULT_SCHEMA_ID )
+				. ' { id: "sTestSLL4444441", name: "Apple Tart", wiki_id: $otherWikiId })',
+			[ 'wikiId' => $this->currentWikiId(), 'otherWikiId' => $this->currentWikiId() . '-other' ]
+		);
+
+		$this->assertEquals(
+			[ new SubjectLabelLookupResult( self::SUBJECT_ID_1, 'Apple Pie' ) ],
+			$this->getSubjectLabelsMatching( 'Apple' )
+		);
+	}
+
+	public function testReturnsALocallyOwnedSubjectOnceWhenAForeignPageAlsoReferencesIt(): void {
+		$this->saveSubjectOnPage( pageId: 7, subjectId: self::SUBJECT_ID_1, label: 'Apple Pie' );
+
+		// In a shared graph the same Subject node (ADR 22 keeps it by bare id) can be referenced by
+		// a foreign wiki's Page too. Only the local owning Page is followed, so the label appears
+		// once and the page id used for the read check resolves within this wiki, not the foreign
+		// one whose colliding page id would gate an unrelated local page.
+		$this->getClient()->run(
+			'MATCH (subject:Subject { id: $subjectId }) '
+				. 'CREATE (:Page { id: 7, wiki_id: $otherWikiId })-[:HasSubject { isMain: false }]->(subject)',
+			[ 'subjectId' => self::SUBJECT_ID_1, 'otherWikiId' => $this->currentWikiId() . '-other' ]
+		);
+
+		$this->assertEquals(
+			[ new SubjectLabelLookupResult( self::SUBJECT_ID_1, 'Apple Pie' ) ],
+			$this->getSubjectLabelsMatching( 'Apple' )
+		);
+	}
+
+	public function testSubjectsOnUnreadablePagesAreOmitted(): void {
+		$this->saveSubjects( new SubjectMap(
+			TestSubject::build( id: self::SUBJECT_ID_1, label: new SubjectLabel( 'Apple Pie' ) ),
+		) );
+
+		$this->assertSame(
+			[],
+			$this->newLookup( readAuthorizer: new StubPageReadAuthorizer( allowed: false ) )
+				->getSubjectLabelsMatching( 'Apple', 10, TestSubject::DEFAULT_SCHEMA_ID )
+		);
+	}
+
+	public function testOverFetchesPastUnreadableRowsToFillTheLimit(): void {
+		$this->saveSubjectOnPage( pageId: 1, subjectId: 'sTestSLL1111141', label: 'Apple 1' );
+		$this->saveSubjectOnPage( pageId: 2, subjectId: 'sTestSLL1111142', label: 'Apple 2' );
+		$this->saveSubjectOnPage( pageId: 3, subjectId: 'sTestSLL1111143', label: 'Apple 3' );
+
+		// Page 2 is hidden and sorts between the two readable rows, so a naive "LIMIT 2 then
+		// filter" would return only Apple 1. Over-fetching then re-limiting must still yield two.
+		$results = $this->newLookup( readAuthorizer: new SelectivePageReadAuthorizer( deniedPageIds: [ 2 ] ) )
+			->getSubjectLabelsMatching( 'Apple', 2, TestSubject::DEFAULT_SCHEMA_ID );
+
+		$this->assertEquals(
+			[
+				new SubjectLabelLookupResult( 'sTestSLL1111141', 'Apple 1' ),
+				new SubjectLabelLookupResult( 'sTestSLL1111143', 'Apple 3' ),
+			],
+			$results
+		);
+	}
+
 	private function saveSubjects( SubjectMap $subjects ): void {
 		$this->newProjectionStore()->savePage( TestPage::build(
 			id: 1,
@@ -150,6 +224,16 @@ private function saveSubjects( SubjectMap $subjects ): void {
 		) );
 	}
 
+	private function saveSubjectOnPage( int $pageId, string $subjectId, string $label ): void {
+		$this->newProjectionStore()->savePage( TestPage::build(
+			id: $pageId,
+			properties: TestPageProperties::build( title: 'Page ' . $pageId ),
+			childSubjects: new SubjectMap(
+				TestSubject::build( id: $subjectId, label: new SubjectLabel( $label ) )
+			)
+		) );
+	}
+
 	protected function newProjectionStore(): GraphDatabasePlugin {
 		return NeoWikiExtension::getInstance()->newNeo4jProjectionStore(
 			new InMemorySchemaLookup(
@@ -165,10 +249,14 @@ private function getSubjectLabelsMatching( string $search, int $limit = 10 ): ar
 		return $this->newLookup()->getSubjectLabelsMatching( $search, $limit, TestSubject::DEFAULT_SCHEMA_ID );
 	}
 
-	private function newLookup( ClientInterface $client = null ): Neo4jSubjectLabelLookup {
+	private function newLookup(
+		ClientInterface $client = null,
+		?PageReadAuthorizer $readAuthorizer = null
+	): Neo4jSubjectLabelLookup {
 		return new Neo4jSubjectLabelLookup(
 			client: $client ?? $this->getClient(),
 			wikiId: $this->currentWikiId(),
+			readAuthorizer: $readAuthorizer ?? new StubPageReadAuthorizer( allowed: true ),
 		);
 	}
 
@@ -179,17 +267,24 @@ private function currentWikiId(): string {
 	/**
 	 * Creates a Subject node directly in the graph, bypassing the projection store, so a test can
 	 * plant a node with a chosen wiki_id (or none at all, as written before wiki_id stamping existed).
+	 *
+	 * The node is attached to a Page carrying the same wiki_id via a HasSubject edge, so it is
+	 * reachable by the lookup's Page->Subject traversal. Without the Page it would be dropped for
+	 * having no Page at all, which would make the wiki filter these tests target untested.
 	 */
 	private function createSubjectNode( string $id, string $name, ?string $wikiId ): void {
-		$properties = [ 'id' => $id, 'name' => $name ];
+		$subjectProperties = [ 'id' => $id, 'name' => $name ];
+		$pageProperties = [ 'id' => 0 ];
 
 		if ( $wikiId !== null ) {
-			$properties['wiki_id'] = $wikiId;
+			$subjectProperties['wiki_id'] = $wikiId;
+			$pageProperties['wiki_id'] = $wikiId;
 		}
 
 		$this->getClient()->run(
-			'CREATE (n:Subject:' . Cypher::escape( TestSubject::DEFAULT_SCHEMA_ID ) . ' $properties)',
-			[ 'properties' => $properties ]
+			'CREATE (:Page $pageProperties)-[:HasSubject { isMain: false }]->'
+				. '(:Subject:' . Cypher::escape( TestSubject::DEFAULT_SCHEMA_ID ) . ' $subjectProperties)',
+			[ 'pageProperties' => $pageProperties, 'subjectProperties' => $subjectProperties ]
 		);
 	}
 

tests/phpunit/TestDoubles/SelectivePageReadAuthorizer.php

@@ -0,0 +1,34 @@
+<?php
+
+declare( strict_types=1 );
+
+namespace ProfessionalWiki\NeoWiki\Tests\TestDoubles;
+
+use MediaWiki\Title\Title;
+use ProfessionalWiki\NeoWiki\Application\PageReadAuthorizer;
+use ProfessionalWiki\NeoWiki\Domain\Page\PageId;
+
+/**
+ * Authorizes every page read except for a fixed set of denied page ids. Lets a list-filter test
+ * hide specific pages without touching the MediaWiki database, so the caller's own filtering and
+ * over-fetch logic can be exercised in isolation.
+ */
+class SelectivePageReadAuthorizer implements PageReadAuthorizer {
+
+	/**
+	 * @param int[] $deniedPageIds
+	 */
+	public function __construct(
+		private array $deniedPageIds
+	) {
+	}
+
+	public function authorizeReadByPageId( PageId $pageId ): bool {
+		return !in_array( $pageId->id, $this->deniedPageIds, true );
+	}
+
+	public function authorizeReadByPageTitle( Title $title ): bool {
+		return !in_array( $title->getId(), $this->deniedPageIds, true );
+	}
+
+}