Pin denial on a warm Schema and Mapping cache#1073
Draft
malberts wants to merge 15 commits into
Draft
Conversation
Third interface on AuthorityBasedSubjectAuthorizer, delegating to Authority::authorizeRead( 'read', $title ): the full per-title check including the expensive ACL hook that probablyCan skips. Reads have no global-right fallback - an unresolvable page denies. Denials log to the NeoWiki channel. Groundwork for #1046. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GET /page/{id}/rdf read the Subject slot and page properties with no
per-title check, exporting any page's data to anyone holding the
wiki-global read right. Denials reuse the exact no-data 404 so
unreadable pages are indistinguishable from pages without NeoWiki data.
CLI dumps (maintenance/DumpRdf.php) stay unfiltered. Part of #1046.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GET /page/{id}/subjects returned every Subject on any page to anyone
holding the wiki-global read right. A denied page now takes exactly the
no-Subjects path (200 + empty map), and referenced Subjects on
unreadable pages are omitted, so page ids cannot be swept to map
restricted pages. Part of #1046.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GET /subject/{id} returned Subject data from unreadable pages. A denied
Subject now takes exactly the absent-Subject path (200 {"subject":null}),
a denied revision page answers like a nonexistent revision, and denied
referenced Subjects are omitted. One page resolve now serves both the
gate and expand=page, removing the former duplicate graph query.
Part of #1046.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
POST /subject/{id}/validate oracled Subject existence (404 vs 200) and
validated against the Subject's Schema regardless of page readability.
A denied Subject now throws the same SubjectNotFoundException as an
absent one, so the two are byte-identical. The query is built at
request time so it can carry the caller's Authority. Part of #1046.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WikiPageLayoutLookup relied on the revision audience check, which
filters revision deletion only and never checks read on the title. It
now gates with authorizeRead before fetching; a denied Layout is null,
identical to an absent one, on both GET /layout/{name} and the
GET /layouts item list. Part of #1046.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The empty-search branch of DatabaseSchemaNameLookup listed NS_SCHEMA straight from the page table with no permission check, and the search branch depended on engine-specific visibility rules. Both branches now share a binding per-title read filter; filtered names are absent, like Schemas that do not exist. getSchemaCount() is a known, deferred count oracle. Part of #1046. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
probablyCan is documented by core as unfit for access control: it skips the expensive permission hook that ACL extensions use, so on a permission-filtered wiki the gate silently passed users it should deny. Also corrects the false comment claiming the inner lookup applies a per-user read check - the revision audience check filters revision deletion only. Part of #1046. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Name the Cypher endpoint as a second per-page-filter exception, stop claiming write denials reveal nothing (page-id-keyed writes are not yet enumeration-hardened), and make three gate comments state their actual situation and accepted trade-offs. Part of #1046. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add the missing verb/action pin test for DatabaseSchemaNameLookup (the one gate a probablyCan regression would not have failed), resolve the validate endpoint's owning page once instead of twice (no slot content is loaded for denied Subjects anymore), and drop the redundant 'NeoWiki:' prefix from denial-log messages - the channel already names the extension. Part of #1046. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GetPageSubjectsQuery only ever receives Subjects from the graph-backed repository, which cannot return a Subject without resolving its page, so the null case is unreachable today. It previously followed the sibling queries' null-means-readable rule for uniformity; that default would silently serve ungated data if a non-graph SubjectLookup were ever wired in. Unresolvable now means omitted - the endpoint's normal absence shape. GetSubjectQuery deliberately keeps null readable: its revision branch reaches null for handler-authorized pages. Part of #1046. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CachingMappingLookupTest created a real Mapping page via createMapping() without declaring the page table used or clearing the graph, so its committed rows and advanced revision ids leaked into later @group Database tests in the same process and made their graph projection resolve stale revisions - deterministically breaking MediaWikiSubjectRepositoryTest::testGetSubjectReturnsSubject downstream (green in isolation, red in the full suite; CI-only). Rewritten as a pure unit test mirroring its schema twin CachingSchemaLookupTest::testGateUsesBindingAuthorizeRead: mocked Title/TitleFactory/Authority, hash-backed cache, no DB. It still pins the binding authorizeRead verb, the 'read' action string, and the denial log. Part of #1046.
Drop the enumeration rationale, the wiki-global-shortcut framing, the per-endpoint denial-shape table, and the temporary not-yet caveats. Keep the one fact a client author needs: a denied read looks like absent data, so treat not-found as maybe-not-permitted. Part of #1046.
All prior denial tests for CachingSchemaLookup and CachingMappingLookup ran against a cold cache, so a refactor that consulted the shared revision-keyed cache before the read gate would serve cached content to a denied caller with the whole suite staying green. Each new test warms the cache through a lookup built with an allowing authority, then builds a second lookup sharing the same cache with a denying authority and asserts it still gets null. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
2 participants
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.
Follows-up to #1058
Review fix from an AI review of that PR, complementing the fixes in
#1071: the gate-before-cache ordering in
CachingSchemaLookupandCachingMappingLookupwas only ever tested against a cold cache, so arefactor that consults the shared revision-keyed cache before the gate would serve cached content to
denied callers with the suite staying green. The new tests warm the cache as an allowed caller and
assert a denied caller still gets null.
Both tests were proven load-bearing by mutation: moving the gate after the cache read turns them red
while the pre-existing tests stay green.
Production notes
Review and finding verification by
Fable 5 (max)with adversarial verifier subagents; implementation byOpus 4.8 (max). Context: the full PR #1058 diff, the NeoWiki test suite, and a live dev-stackreproduction of the PR's denial matrix.