Repository navigation
Refuse to export a demo instance - #846
Merged
Merged
Conversation
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
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
Every way of getting an installation out of a demo instance refuses — except the web export, both
halves of it.
config/exportxmlExport(create)downloadExport(fetch)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. What the export contains is 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
(
XmlExport::appendNode()only encrypts when a password is supplied; established in #840).#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 I was comparing 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.
Test
ConfigBackupTestgains both refusals. Mutation-checked: revertingsrc/fails exactly those two.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
ResponseType::CALLBACK, so arefusal that is not a callable renders as nothing. That blank page is a pre-existing wart the three
share — I verified it against the untouched
downloadBackupApprather than assuming — and it isleft alone here. What the test pins is the part that matters: the export is not handed over.
PHPStan level 6 and PHPCS clean.
Correcting the record on #841
CLAUDE.mdis corrected, and the correction is the substantive part of this PR's docs.#841 said any signed-in user could delete another user's notification, because
Aclreturnstruefor the notification actions unconditionally.
NOTIFICATION_VIEW,NOTIFICATION_SEARCHandNOTIFICATION_CHECKare in that list —NOTIFICATION_DELETEis not in the switch at all, so itfalls through to the deny at the end, and only
isAdminAppever reached those methods.The ownership check that PR added is still right — as defence in depth, and for consistency with
setCheckedById()— but it was not the reachable hole it was described as. The lesson recordedalongside it: read the ACL arm for the exact action, not for its neighbours.