Gate GET /subject-labels by per-page read permission#1126
Merged
Conversation
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>
alistair3149
force-pushed
the
fix/1060-subject-labels-read-gate
branch
from
July 21, 2026 20:43
9b33399 to
b1ca656
Compare
alistair3149
marked this pull request as ready for review
July 21, 2026 20:44
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1060
What
GET /neowiki/v0/subject-labels(the Subject autocomplete) returned the labels of Subjects onpages the caller cannot read, and accepted an uncapped
limit. This gates it by per-pagereadpermission and bounds the work, mirroring the read-gate work from #1058 / PR #1058.
Fixes the leak described in #1060.
Changes
(:Page)-[:HasSubject]->(:Subject)so the owning page idreaches PHP, and each row is dropped unless
PageReadAuthorizer::authorizeReadByPageIdallows it —the same
authorizeRead('read', $title)binding check the other read endpoints use. DeniedSubjects are simply absent (never a
403), so restricted pages cannot be enumerated. Thismirrors
DatabaseSchemaNameLookup::filterReadable.limitcap.PARAM_MAX => 50(andPARAM_MIN => 1), matchingGET /layouts. Without it aper-result permission check was an unbounded-work vector (
?limit=100000was accepted).limit × 4), then rows are authorized one at atime and collection stops at
limit, so read restrictions tend not to shorten the result whilethe expensive per-page check runs only for the rows actually needed, not the whole over-fetched
window.
(
(:Page { wiki_id })) in addition to the existing subject scope (n.wiki_id). The page scopekeeps the returned page id resolvable within this wiki (page ids are unique only per wiki, so a
foreign page id would gate an unrelated local page); the subject scope keeps 5d802e6's ownership
guarantee. Both are independently mutation-verified below.
Testing
Neo4jSubjectLabelLookupTest(14): adds a denial test, an over-fetch/re-limit test, and twowiki-scope tests (a foreign subject on a local page; a local subject also referenced by a foreign
page). The two pre-existing cross-wiki isolation tests were made non-vacuous — their fixtures now
create a Page +
HasSubjectedge so exclusion is attributable to the wiki filter, not to amissing page.
n.wiki_idfails exactlytestExcludesForeignSubjectsReachableThroughALocalPage; removing the page{ wiki_id }scopefails exactly
testReturnsALocallyOwnedSubjectOnceWhenAForeignPageAlsoReferencesIt.GetSubjectLabelsApiTest(new):?limit=100000→400.NoGraphBackendTest,RestApiDocsCoverageTest,ModuleSpecHandlerNeoWikiTest,DocsPathReferencesTestgreen;make cs(phpcs + phpstan) clean.RestSubjectLabelSearch.ts) sends nolimit, soPARAM_MIN/PARAM_MAXcannot break it; no frontend change.
Smoke tested on a live wiki
Verified end-to-end on the dev wiki (
/w/rest.php), beyond the automated suite:limitcap:?limit=100000and?limit=51→400("must be between 1 and 50");?limit=50→200;?limit=0→400; missingschema→400.getUserPermissionsErrorshook,GET /neowiki/v0/subject-labels?schema=Product&search=adropped that Subject's label —[Acme Anvil, Acme Rocket, Acme TNT]→[Acme Anvil, Acme TNT]— while other Subjects and other schemas were unaffected; lifting the restriction restored it. Denied Subjects are absent, identical to a Subject that does not exist.