Harden Subject-write authorization on the server and align the frontend hints#1003
Merged
Merged
Conversation
alistair3149
force-pushed
the
harden/subject-write-authorize-write
branch
from
July 7, 2026 21:04
bfcc705 to
e5a6224
Compare
The Subject write actions (create, replace, delete, set main, set ordering) authorized their writes with `Authority::definitelyCan( 'edit', $title )`. `definitelyCan` reads permissions from a replica (RIGOR_FULL) and checks the edit rate limit without counting the hit, so these writes never consume the edit throttle. API- and agent-driven bulk writes therefore bypass rate limiting, and a block or protection applied moments earlier can be missed due to replication lag. `SubjectAuthorizer::authorizeEdit()` now performs the write authorization via `Authority::authorizeWrite( 'edit', $title )`, which uses RIGOR_SECURE (primary reads) and counts the write against the edit rate limit. All five write actions use it. Because every Subject write is an edit of the owning page, the previous per-operation create-main/create-child authorization distinction collapses into this single check. `Special:NeoJson` edits Subject content directly rather than through these actions, so its `onSubmit` write now authorizes via `authorizeEdit` as well; its `execute` keeps the side-effect-free `canEditSubject` for early denial before the form is rendered. No Subject write path is left on `definitelyCan`. The `can*` checks remain for UI hints (page tools, the RedHerb sidebar) and that early `Special:NeoJson` gate, so ordinary page views neither enforce nor consume the edit rate limit. The now-unused `canDeleteSubject` is removed. Each write action's authorization test pins it to `authorizeEdit`: a revert to a `can*` hint fails the test. Follows-up to #986 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alistair3149
force-pushed
the
harden/subject-write-authorize-write
branch
from
July 9, 2026 00:33
e5a6224 to
e36458a
Compare
alistair3149
marked this pull request as ready for review
July 9, 2026 00:35
JeroenDeDauw
reviewed
Jul 9, 2026
Member
|
There is an interface segregation issue now. Interestingly, Fable 5 max didn't catch it when looking from multiple angles, but it did when I asked "isn't it better to have two interfaces?". Got an Opus session running now with the implementation which will be pushed here after running review skills. |
The single SubjectAuthorizer interface mixed two contracts: side-effect-free capability queries used to paint UI affordances, and the binding authorization that performs a write. That mixing is what let the write actions authorize with a `can*` hint in the first place. Only a docblock and a verb distinguished the two, so nothing but review could stop a write path from gating on a hint, or a render path from consuming rate-limit allowance. The interface is now split along the line its callers already follow: * `SubjectPermissionHints` — `canCreateMainSubject`, `canCreateChildSubject`, `canEditSubject`. Side-effect-free, advisory, callable from anywhere. * `SubjectWriteAuthorizer` — `authorize`. Called once per write attempt, at the point of writing, and only when a write will actually be attempted. The five write actions now depend on `SubjectWriteAuthorizer` alone, so passing them a hint no longer type checks. The hint callers (page tools, the RedHerb sidebar, the early denial in `Special:NeoJson`) depend on `SubjectPermissionHints` alone, so a render path cannot consume the edit rate limit even by accident. `Special:NeoJson` is the one legitimate consumer of both, and takes each where it belongs. `AuthorityBasedSubjectAuthorizer` implements both interfaces, and is the only place that names `definitelyCan`, `authorizeWrite`, or the rate limit. `NeoWikiExtension` exposes `newSubjectPermissionHints()` and `newSubjectWriteAuthorizer()`. Only the former is documented for extensions: extensions get pure queries, and the binding gate stays with the write actions. Rate-limit accounting is a MediaWiki policy effect, not a guarantee of the port: a `noratelimit` holder consumes nothing, and a wiki can configure the limit away. So the interface documents that policy effects attach to `authorize()` and to no other call, and the mechanism is described on the adapter that implements it. `SpecialNeoJsonTest` gains a test that a POST consumes the poster's edit rate limit allowance. Its `onSubmit` write authorization previously had no test of its own: the existing denial test is satisfied by the hint gate in `execute()` alone, so removing the write authorization, or downgrading it back to the hint, broke nothing. The new test fails under both. Also removes `SchemaAuthorizer`, which had no production caller. Schema and Layout writes go through MediaWiki's normal edit machinery, which authorizes and rate limits them; Subjects need their own port only because their writes bypass that path. The now-unused `SucceedingSubjectAuthorizer` and `SpySubjectAuthorizer` collapse into a single `SpySubjectWriteAuthorizer`. Follows-up #1003
The frontend permission hints disagreed with the authorization the server applies. `canDeleteSubject` hinted on the `delete` right, and the two creation hints on `createpage` plus `edit`, while the server authorizes every Subject write as an `edit` of the page that holds the Subject. The `delete` right is granted to sysops by default, so the delete affordance in the subjects manager was hidden from ordinary editors who are in fact allowed to delete a Subject. The creation hints were wrong in the same direction: adding a Subject to an existing page creates no page, so `createpage` should not be required. All four hints now check the `edit` right, mirroring `AuthorityBasedSubjectAuthorizer` on the server. They remain hints: they check the wiki-global right, whereas the server checks `edit` on the specific page, so a user who may edit globally but not the page at hand is still offered affordances the server rejects. That gap is recorded as a TODO rather than fixed here, since closing it means feeding the server's per-page answer to the frontend. Follows-up #1003
The frontend has no binding authorization and cannot have any: the browser decides nothing, the server authorizes every write. Naming these classes `Authorizer` implied a security control that does not exist here, which is the same conflation the backend split removed. Extension authors read these names off the public API, so the name is the documentation. `SubjectAuthorizer`, `SchemaAuthorizer` and `LayoutAuthorizer` become `SubjectPermissionHints`, `SchemaPermissionHints` and `LayoutPermissionHints`, with their `RightsBased*` implementations and the service and factory names following. `NeoWikiExtension.newSubjectPermissionHints()` now matches the PHP factory of the same name. Each interface documents that a positive answer means "offer the affordance", not "the write will succeed". The backend split is not mirrored: there is no frontend counterpart to `SubjectWriteAuthorizer`, because there is nothing for it to authorize. `canDeleteSubject` is kept rather than folded into `canEditSubject`. Both now resolve to the same right, but the hints name the question the caller asks, which is also why the backend keeps three hints that all resolve to `canEditPage`. Follows-up #1003
JeroenDeDauw
approved these changes
Jul 10, 2026
This was referenced Jul 16, 2026
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.
Follows-up #986
Stacked on #1002
Hardens Subject-write authorization on both the server and the frontend. The server change is the substance; the frontend commits (merged in from #1019) align the browser-side permission hints with it.
Server
The Subject write actions (create, replace, delete, set main, set ordering) authorized their writes with
Authority::definitelyCan('edit', $title).definitelyCanreads permissions from a replica (RIGOR_FULL) and checks the edit rate limit without counting the hit, so these writes never consume the edit throttle. API- and agent-driven bulk writes therefore bypass rate limiting, and a block or protection applied moments earlier can be missed due to replication lag.SubjectAuthorizer::authorizeEdit()now performs the write authorization viaAuthority::authorizeWrite('edit', $title), which uses RIGOR_SECURE (primary reads) and counts the write against the edit rate limit. All five write actions use it. Because every Subject write is an edit of the owning page, the previous per-operation create-main/create-child authorization distinction collapses into this single check.Special:NeoJsonedits Subject content directly rather than through these actions, so itsonSubmitwrite now authorizes viaauthorizeEditas well; itsexecutekeeps the side-effect-freecanEditSubjectfor early denial before the form is rendered. No Subject write path is left ondefinitelyCan.The
can*checks remain for UI hints (page tools, the RedHerb sidebar) and that earlySpecial:NeoJsongate, so ordinary page views neither enforce nor consume the edit rate limit. The now-unusedcanDeleteSubjectis removed. Each write action's authorization test pins it toauthorizeEdit: reverting an action to acan*hint fails its test.SubjectAuthorizeris also split along the line its callers already follow:SubjectPermissionHints(side-effect-free capability queries) andSubjectWriteAuthorizer(the binding, rate-limit-counting write gate). Passing a write action a hint no longer type checks, and a render path can no longer consume rate-limit allowance by accident.AuthorityBasedSubjectAuthorizerimplements both. The unusedSchemaAuthorizeris removed, since Schema and Layout writes go through MediaWiki's normal edit machinery.Part of the permission audit tracked in #101.
Frontend
The frontend permission hints disagreed with the server:
canDeleteSubjecthinted on thedeleteright and the two creation hints oncreatepageplusedit, while the server authorizes every Subject write as anedit. Thedeleteright is sysop-only by default, so the delete affordance in the subjects manager was hidden from ordinary editors who are in fact allowed to delete a Subject. All four hints now checkedit, mirroringAuthorityBasedSubjectAuthorizer.Because the frontend has no binding authorization and cannot have any, the
SubjectAuthorizer,SchemaAuthorizerandLayoutAuthorizerinterfaces (with theirRightsBased*implementations, services, factories, and public-API exports) become*PermissionHints. The name is the documentation extension authors read off the public API: a positive answer means "offer the affordance", never "the write will succeed" — the same conflation the server split removes.The hints remain wiki-global rather than per-page (a documented TODO): a user who may edit globally but not the page at hand is still offered affordances the server then rejects.
Verification
AuthorityBasedSubjectAuthorizerTestpinsauthorizeEdittoauthorizeWrite(and thecan*hints todefinitelyCan); each write action's authorization test fails if the action is reverted to acan*hint.Confirmed end-to-end against a running wiki, with the
editrate limit set to one edit per window — two back-to-back Subject writes via the REST API, as the same rate-limited user, on the same page:POST /neowiki/v0/page/{id}/childSubjects201 Created— consumes the edit allowance403 Forbidden— throttled by the edit rate limitOn the previous
definitelyCanpath, write #2 would also have returned201, since that check never counts the throttle.Frontend:
make tsciis green — 99 test files, 1007 tests, withvue-tscand eslint/stylelint clean.RightsBasedSubjectPermissionHintsTestpins the corrected rights: revertingcanDeleteSubjecttodelete, or the creation hints tocreatepage, fails its assertions.