-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH Require sudo mode to edit the site config form #186
ENH Require sudo mode to edit the site config form #186
Conversation
b028158
to
494c0b2
Compare
494c0b2
to
7e9783e
Compare
@@ -78,6 +78,8 @@ class SiteConfig extends DataObject implements PermissionProvider, TemplateGloba | |||
'CMS_ACCESS_LeftAndMain' | |||
]; | |||
|
|||
private static bool $require_sudo_mode = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we talked about this, we agreed that this would only be for the "access" tab, or for the access controls in that tab. There's already permissions controlling access to this model as a whole.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My memory of this was "try and get it to work for just a single tab, and if that proves to be too difficult just put it on everything", and it quickly proved to be too difficult. I'm assuming that editing siteconfig is very infrequent operation so the UX downside here should be very low
The permissions controlling access to the model is irrelevant, the whole point of this is to prevent XSS style attacks on users who do have access
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting the discussion about tab vs model aside for now, if we're not protecting the access controls in SiteTree
and File
, we shouldn't protect them here (continuing the discussion from silverstripe/silverstripe-admin#1898 (comment))
It's sending mixed messages - "we think access controls are privilege escalation" in siteconfig but "we think access controls are not privilege escalation" in the individual records, when they control the same thing.
An XSS attack could give basically the same level of access to pages and files by targetting the records directly as it can via siteconfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. Will close PR.
Issue silverstripe/silverstripe-admin#1898