Repository navigation
Check who a notification belongs to before deleting it - #841
Merged
Merged
Conversation
Any signed-in user could delete any other user's notification by its id. The rule was already written down and named: checkUserAccess(), whose docblock says admins may reach any notification and regular users only their own, and which answers "not found" so ids cannot be enumerated by the difference. It was applied to getById() and setCheckedById(), and not to delete() or deleteByIdBatch(), where the repository's only condition is sticky = 0. And NOTIFICATION_DELETE is not an administrator's permission — Acl returns true for the notification actions unconditionally, with no profile bit — so every authenticated user holds it. The REST door was safe by accident: it reads the row to build its event message, and that read carries the check. The web's DeleteController calls through for a non-admin, single and batch alike, which is the door that was open. Both delete paths now read the notification first, in the service where both doors reach it rather than in the controller that happened to be at fault. The batch checks every id before removing any of them, since checking afterwards would mean having already deleted somebody else's. deleteAdmin() and deleteAdminBatch() are untouched: deleting any notification is what they are for. Two refusals are added, modelled on the setCheckedById one already there. Four unit tests and three integration tests needed the ownership read stubbed, having been written when these methods went straight to the repository; the integration ones also needed getUserDataDto() memoised, since the generator mints a fresh random user on each call and they now have to know who is signed in. The class docblock said the delete methods never call getById and needed no ownership resolver, which is no longer true and now says so. Reading was already guarded and is left alone. The linked-account parentId path was swept in the same pass and is sound: a linked account resolves through the parent's id, and getPasswordForId() builds through AccountFilterUser.
blaipr
added a commit
that referenced
this pull request
Aug 23, 2026
Every way of getting an installation out of a demo refuses — the backup and both of its downloads, the config import, the encryption save and refresh, and the REST export since #831 — except the web export, both halves of it. A demo publishes its administrator's credentials by design, so the ACL in front of these stops nobody and the demo check is the whole boundary. The export holds the same installation the backup is guarded for: every account's encrypted secret and its key, and, when no export password is given, the name, login, URL and notes of every account in the clear. #831 fixed the REST door by comparing it against the five web config actions that had the guard. The web export was not among those five, so comparing against them could not reveal it: the gap was in the list being compared to. Both halves are guarded, because guarding only the creation leaves an export made before the instance became a demo fetchable, which is a way around it. The download refusal asserts an empty body rather than a message, which is what the backup downloads beside it already answer on a demo: these actions are typed CALLBACK, so a refusal that is not a callable renders as nothing. That blank page is a pre-existing wart the three share, verified against the untouched downloadBackupApp rather than assumed, and left alone. What the test pins is that the export is not handed over. CLAUDE.md corrects #841, which said any signed-in user could delete another user's notification on the grounds that Acl returns true for the notification actions unconditionally. NOTIFICATION_VIEW, NOTIFICATION_SEARCH and NOTIFICATION_CHECK are in that list; NOTIFICATION_DELETE is not in the switch at all and falls through to the deny, so only isAdminApp ever reached those methods. The ownership check remains right as defence in depth, but it was not the reachable hole it was described as.
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
1 participant
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.
The defect
Any signed-in user could delete any other user's notification by its id.
Notificationalready has the rule written down and named —checkUserAccess(), with a docblocksaying admins may reach any notification and regular users only their own, answering "not found" so
ids cannot be enumerated by the difference. It was applied to two of the four operations that need
it:
getById()setCheckedById()delete()deleteByIdBatch()Below them the repository's only condition is
sticky = 0— nothing about ownership. AndNOTIFICATION_DELETEis not an administrator's permission:Aclreturnstruefor thenotification actions unconditionally, with no profile bit behind them, so every authenticated user
holds it.
The REST door was safe by accident — it reads the row to build its event message, and that read
carries the check. The web's
DeleteControllercallsdelete()/deleteByIdBatch()straightthrough for a non-admin, so that is the door that was open, single and batch alike.
The fix
Both delete paths read the notification first, which applies the existing check. In the service,
where both doors reach it, rather than in the web controller that happens to be the one at fault.
The batch checks every id before removing any of them — checking afterwards would mean having
already deleted somebody else's.
deleteAdmin()/deleteAdminBatch()are untouched: deleting any notification is what they are for.Test
NotificationTestgains the two refusals, modelled on thesetCheckedByIdone that was alreadythere: a notification owned by another user is refused for
delete(), and a selection containingone id belonging to somebody else is refused as a whole with nothing removed. Mutation-checked:
removing the guards fails exactly those two.
Four existing unit tests and three integration tests needed the ownership read stubbed — they were
written when these methods went straight to the repository, and the integration ones needed
getUserDataDto()memoised, since the generator mints a fresh random user on every call and thesenow have to know who is signed in. The class docblock said in as many words that "the delete service
methods go directly to the repository without calling getById, so no ownership resolver is needed";
that is no longer true and now says so.
PHPStan level 6 and PHPCS clean.
Checked and left alone
Reading is already guarded —
getById()calls the check — so?r=notification/view/{id}does notleak. I had assumed otherwise from where the guard is defined and had to read the method to find
out;
CLAUDE.mdrecords that, because the same mistake in the other direction is what left thesetwo unguarded.
The linked-account feature (
parentId) was swept in the same pass and is sound: the client requeststhe parent's id for a linked account, and
getPasswordForId()builds throughAccountFilterUser,so a link pointing at an account the caller cannot see refuses at view time rather than borrowing
its password.