Report unreadable relation targets as not found - #1385
Open
alistair3149 wants to merge 4 commits into
Open
Conversation
Fixes #1266 SubjectValidator told `relation-target-not-found` from `relation-target-schema-mismatch` by resolving the target through a lookup that applied no read authorization, and the mismatch violation names the target's own Schema. Through the validate endpoints that pair was an oracle: a caller could learn that a Subject id exists, and which Schema it uses, on a page they may not read. The validator now resolves relation targets through ReadAuthorizedSubjectLookup, a SubjectLookup that drops ids hosted by pages the caller may not read, so such a target reports as `relation-target-not-found` - the same warning an id nobody has minted produces. Ids are filtered before the fetch, so no revision is loaded and no slot deserialized for a page the caller may not read. Building the validator now takes an Authority, which threads through the proposed Subject validator into the create, replace and update-statement actions, and through both validate queries. ValidateSubjectApi builds its query in run() from the request authority, as ValidateSubjectUpdateApi already did. A relation target whose Schema mismatches now goes unreported when the writer cannot read its page, so an enforced-validation write that used to be blocked lands. That is the trade the read gate makes: nothing about a page you cannot read may reach you, a blocking error included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KFsam1RUQGa9Sk4MNxuXP6
Follows the in-session review of the previous commit. - The gate now withholds a Subject whose hosting page does not resolve rather than passing it to the wrapped lookup. Both answers are behaviour-identical against the index-backed repository, and this is the one GetPageSubjectsQuery already chose and wrote down. - Permission is asked once per hosting page rather than once per Subject. Each ask is an uncached page-row load plus the full permission hook, several Subjects share a page, and a save validates twice. - ValidateSubjectApiTest now validates the same target page under two authorities. The old pair denied the page to everyone, so it passed with the request authority replaced by RequestContext's - the exact wiring the fix adds. - ReplaceSubjectApiTest covers the write path, which shares the validator and had no test for the gate. - newSubjectValidator / newProposedSubjectValidator: `get` marks the memoizable factories here, and memoizing these would pin one caller's read rights into a shared validator. - The not-found message no longer asserts the target does not exist, which is false for one it withholds. - rest-api.md's Permissions section covers the validate endpoints, which validation-codes.md links to for exactly that fact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KFsam1RUQGa9Sk4MNxuXP6
The project allows one class per file, so the spy that pins the per-page dedupe cannot live beside the test using it. Folding the recording into SelectivePageReadAuthorizer avoids a near-duplicate double and gives the other read-gate tests the same handle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KFsam1RUQGa9Sk4MNxuXP6
The entry asserted the id resolves to no existing Subject, which the read gate makes false: an importer reading that concludes the id is free to mint, and creating with it answers 409. Fixing the definition says it once instead of appending a caveat. Drops the rest-api.md paragraph with it. That section already states that a page you may not read answers as if the data were absent, and a not-found violation is that shape, so it restated the rule for one more surface - and half of it described the validate endpoint's 404, which predates this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KFsam1RUQGa9Sk4MNxuXP6
alistair3149
marked this pull request as ready for review
September 9, 2026 02:57
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 #1266
SubjectValidatortoldrelation-target-not-foundfromrelation-target-schema-mismatchby resolving the target through a lookup that applied no read authorization, and the mismatch violation's args name the target's own Schema. A caller could therefore learn that a Subject id exists, and which Schema it uses, on a page they may not read.The new
ReadAuthorizedSubjectLookupwithholds Subjects hosted by pages the caller may not read, so such a target reports asrelation-target-not-found— the same warning an id nobody has minted produces. Ids are filtered before the fetch, so no revision is loaded and no slot deserialized for a page the caller may not read.The scope is wider than the issue's title.
CreateSubjectAction,ReplaceSubjectActionandUpdateStatementActionreturn their violations regardless of the enforcement setting, so the three Subject write endpoints leaked the same pair. Gating where the validator is built covers all five surfaces at once.Consequences
A relation target whose Schema mismatches now goes unreported when the writer cannot read its page, so a write that enforcement used to block lands. That is the trade the read gate makes: nothing about a page you cannot read may reach you, a blocking error included.
This does not make restricted Subject data unreachable on a default wiki.
neowiki-queryis granted to*and the Cypher endpoint does no per-page trimming (#1342), so the same data is already readable wholesale there. The fix matters for wikis that revoke that right, for no-graph deployments under ADR 32, and for the ADR 27 invariant that a denied read is indistinguishable from absent data.Design notes
GetSubjectQuery, which filters the Subjects after it. Nothing about a page the caller may not read is read at all.GetPageSubjectsQueryrather thanGetSubjectQuery's allow-unresolved branch. Behaviour-identical against the index-backed repository, and a lookup that later bypasses the index cannot escape the gate.newSubjectValidator/newProposedSubjectValidatorwere renamed fromget*, which marks the memoizable factories inNeoWikiExtension. Memoizing these would pin one caller's read rights into a shared validator.Manual Browser Check
The reworded warning is the only user-visible change; the gate itself needs a restricted page to see.
s00000000000000.Considered, omitted
statements. ADR 27 asks surfaces doing per-row permission checks to bound their result sizes, andValidateSubjectApihas noPARAM_MAX. With the per-page memoization the added cost stays below the per-page revision loads validation already paid, so capping the endpoint is a contract change that belongs in its own issue rather than in a security fix.neowiki_subject_pagequery. The decorator resolves hosting pages andMediaWikiSubjectRepositoryresolves them again. Avoiding it means decoratingPageIdentifiersLookupinstead, which is a larger change than this fix warrants.ViolationDiff. Its(propertyName, code, valuePartIndex)identity makes a re-indexed survivor look new, so removing one relation target can block a write that only improved. Pre-existing — a mixed valid/invalid target list already triggers it — and this change adds one narrow new trigger. Wants its own issue.