Skip to content

Restore the documented status-code contract of DELETE /subject/{id} - #1380

Merged
JeroenDeDauw merged 1 commit into
masterfrom
fix/1312-delete-subject-read-gate
Sep 9, 2026
Merged

Restore the documented status-code contract of DELETE /subject/{id}#1380
JeroenDeDauw merged 1 commit into
masterfrom
fix/1312-delete-subject-read-gate

Conversation

@alistair3149

@alistair3149 alistair3149 commented Sep 9, 2026

Copy link
Copy Markdown
Member

Fixes #1312

DELETE /neowiki/v0/subject/{subjectId} broke the status-code contract thatdocs/api/rest-api.md documents and ADR 027 governs, in four ways.

The read gate

The action resolved the Subject to its page and authorized the write against that page, but never
checked the caller may read it — unlike ReplaceSubjectAction and UpdateStatementAction. Two
consequences: a Subject on an unreadable page answered 403 where the contract promises 404, so
403 versus 404 told a caller which Subject ids exist; and a caller holding edit but not read
could delete Subjects there.

The action now takes a PageReadAuthorizer and answers the not-found path before reaching the write
check — the shape UpdateStatementAction::getPageOfSubjectToEdit() already uses. A page the caller
can read still answers 403 when they cannot edit it.

Three further breaches of the same contract

A malformed id answered 500. new SubjectId( 'notavalidid' ) throws
InvalidArgumentException, a LogicException, which no catch arm matched. It now answers 400, as
every sibling Subject-keyed handler does.

Internal failures answered 403 carrying their own message. The handler caught every
RuntimeException and reported it as a permission denial, so a DBError raised inside the
authorizers surfaced as 403 {"message": "<internal error text>"} — and the read gate added here
runs the getUserPermissionsErrors hook, which is arbitrary ACL-extension code. The delete denial
now raises the typed SubjectEditNotAuthorizedException, keeping its own message, and an internal
error is a 500.

A delete that removed nothing answered 200. SubjectRepository::deleteSubject() returned
void and the implementation discarded the write's outcome. It now returns
PageContentSavingStatus, as savePageSubjects() already does, and the action answers not-found
unless a revision was created. The repository asks the slot whether it still holds the Subject
rather than inferring removal from the save: mutatePageSubjects re-serializes the slot whatever
the mutation did, so a slot written by anything other than the serializer — an import, or
Special:NeoJson — yields new bytes and a real revision even when nothing was removed.

Open for review

A write the wiki refuses — read-only mode, an edit-filter abort — now answers 404 Subject not found while the Subject still exists. CreateSubjectAction, SetMainSubjectAction,
SetSubjectsOrderingAction and MoveSubjectAction all map ERROR to a not-found presentation, so
the shape is the established one, and it replaces a false 200. Giving ERROR its own answer is
the alternative, and the one that would keep the documented 404 causes exact. Left as it is here
because the choice belongs to all five endpoints at once, not to this one.

Considered, omitted

Sharing the resolve-then-read-gate half with Application\Rdf\SubjectHostingPageResolver. Adopting
it here alone would leave the three Subject-id-keyed write actions in three different shapes, and
moving it out of the Rdf namespace reaches past this fix. Worth doing across all of them at once.

Not fixed here

#1383Special:NeoJson reads and
writes Subject content with no read gate at all, the same threat model on a path that is not a REST
route. It is unregistered unless $wgNeoWikiEnableDevelopmentUI is set, which defaults to false.

AI-authored — Claude Code, Opus 5 (1M context); one-line ask from @alistair3149, who then widened the scope to the endpoint's whole contract; diff not yet human-reviewed; the read gate verified failing against five mutations of it with no survivors, the later guards not mutation-tested (the local runner became unstable); make cs clean, non-Database suite (2002 tests) plus the DeleteSubject, MediaWikiSubjectRepository and NoGraphBackend classes green locally, full suite green on CI.

@alistair3149
alistair3149 force-pushed the fix/1312-delete-subject-read-gate branch from 1426c48 to 650f2fc Compare September 9, 2026 00:13
@alistair3149
alistair3149 force-pushed the fix/1312-delete-subject-read-gate branch from 650f2fc to b46b02b Compare September 9, 2026 00:23
@alistair3149
alistair3149 force-pushed the fix/1312-delete-subject-read-gate branch from b46b02b to dcc34e6 Compare September 9, 2026 00:36
@alistair3149 alistair3149 changed the title Gate DELETE /subject/{id} on per-page read permission Restore the documented status-code contract of DELETE /subject/{id} Sep 9, 2026
@alistair3149
alistair3149 force-pushed the fix/1312-delete-subject-read-gate branch 3 times, most recently from a31f842 to 60adeea Compare September 9, 2026 01:46
Fixes #1312

`DELETE /neowiki/v0/subject/{subjectId}` broke the status-code contract that
[`docs/api/rest-api.md`](https://github.com/ProfessionalWiki/NeoWiki/blob/master/docs/api/rest-api.md)
documents and [ADR 027](https://github.com/ProfessionalWiki/NeoWiki/blob/master/docs/adr/027-access-control.md)
governs, in four ways.

## The read gate

The action resolved the Subject to its page and authorized the write against that page, but never
checked the caller may read it — unlike `ReplaceSubjectAction` and `UpdateStatementAction`. Two
consequences: a Subject on an unreadable page answered `403` where the contract promises `404`, so
`403` versus `404` told a caller which Subject ids exist; and a caller holding `edit` but not `read`
could delete Subjects there.

The action now takes a `PageReadAuthorizer` and answers the not-found path before reaching the write
check — the shape `UpdateStatementAction::getPageOfSubjectToEdit()` already uses. A page the caller
can read still answers `403` when they cannot edit it.

## Three further breaches of the same contract

**A malformed id answered `500`.** `new SubjectId( 'notavalidid' )` throws
`InvalidArgumentException`, a `LogicException`, which no catch arm matched. It now answers `400`, as
every sibling Subject-keyed handler does.

**Internal failures answered `403` carrying their own message.** The handler caught every
`RuntimeException` and reported it as a permission denial, so a `DBError` raised inside the
authorizers surfaced as `403 {"message": "<internal error text>"}` — and the read gate added here
runs the `getUserPermissionsErrors` hook, which is arbitrary ACL-extension code. The delete denial
now raises the typed `SubjectEditNotAuthorizedException`, keeping its own message, and an internal
error is a `500`.

**A delete that removed nothing answered `200`.** `SubjectRepository::deleteSubject()` returned
`void` and the implementation discarded the write's outcome. It now returns
`PageContentSavingStatus`, as `savePageSubjects()` already does, and the action answers not-found
unless a revision was created. The repository asks the slot whether it still holds the Subject
rather than inferring removal from the save: `mutatePageSubjects` re-serializes the slot whatever
the mutation did, so a slot written by anything other than the serializer — an import, or
`Special:NeoJson` — yields new bytes and a real revision even when nothing was removed.

## Open for review

A write the wiki refuses — read-only mode, an edit-filter abort — now answers `404 Subject not
found` while the Subject still exists. `CreateSubjectAction`, `SetMainSubjectAction`,
`SetSubjectsOrderingAction` and `MoveSubjectAction` all map `ERROR` to a not-found presentation, so
the shape is the established one, and it replaces a false `200`. Giving `ERROR` its own answer is
the alternative, and the one that would keep the documented `404` causes exact. Left as it is here
because the choice belongs to all five endpoints at once, not to this one.

## Considered, omitted

Sharing the resolve-then-read-gate half with `Application\Rdf\SubjectHostingPageResolver`. Adopting
it here alone would leave the three Subject-id-keyed write actions in three different shapes, and
moving it out of the `Rdf` namespace reaches past this fix. Worth doing across all of them at once.

## Not fixed here

[#1383](#1383) — `Special:NeoJson` reads and
writes Subject content with no read gate at all, the same threat model on a path that is not a REST
route. It is unregistered unless `$wgNeoWikiEnableDevelopmentUI` is set, which defaults to false.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AfaXavUCWB237b7NmErXjg
@alistair3149
alistair3149 force-pushed the fix/1312-delete-subject-read-gate branch from 60adeea to 17b951c Compare September 9, 2026 01:48
@alistair3149
alistair3149 marked this pull request as ready for review September 9, 2026 01:56
@JeroenDeDauw
JeroenDeDauw merged commit 00e6daf into master Sep 9, 2026
14 checks passed
@JeroenDeDauw
JeroenDeDauw deleted the fix/1312-delete-subject-read-gate branch September 9, 2026 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DELETE /subject/{id} has no per-page read gate, unlike the other Subject write endpoints

2 participants