Incorrect use of check_plain() makes unwanted changes to the permission configuration [#2126775]

The administration form uses check_plain() on default values for text fields, which is incorrect use.

The text in text fields does not need to be escaped because text fields don't render HTML that is typed within them, so there is no security risk from useing the exact value given by the user previously.

The problem with using check plain is that if you have characters that get escaped, like & being changed to &, then every time you save that form the value of that field is changed and potentially the name of your permission is changed, which means it will technically be a new permission and no roles will have access until you go and configure the permissions again.

This is not a security issue because it causes users to not have access to pages they should have access to instead of the other way around.

I have now noticed that there is also check_plain() used on the machine name of the permissions when in hook_permission().
This is also bad because it means that incorrect (escaped) machine names are being stored to the database.

For example, I have a permission called "news & events page" and in my database after a while for my permissions I have all these:

news &amp; events page (Boolean) TRUE
news &amp;amp; events page (Boolean) TRUE
news &amp;amp;amp; events page (Boolean) TRUE

Notice that none of them are the real value.

This shouldn't be escaped like this.

We will also need a database update to try to fix this when we change it to work without the escaping.
It will also need a notice to users to double check their permissions for all their custom perms.

Comment	File	Size	Author
#4	config_perms-check_plain-2126775-4.patch	6.2 KB	rooby
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

rooby CreditAttribution: rooby commented 4 November 2013 at 04:49

Issue summary:

View changes

Adding code tag around some html entities and adding note about permission machine names being stored wrongly.

Comment #2

rooby CreditAttribution: rooby commented 4 November 2013 at 06:19

Assigned:

Unassigned

» rooby

Assigning to myself as I am working on this.

Comment #3

rooby CreditAttribution: rooby commented 5 November 2013 at 03:18

Status:	Active	» Needs review
Related issues:		+#2127635: Cached versions are not cleared appropriately when using ctools

Here is a patch that addresses this.

It removes the calls to check_plain() and includes a database update script to fix any existing config perms in the case that some had escaped characters.

If you have the ctools module enabled this patch also requires the patch at #2127635: Cached versions are not cleared appropriately when using ctools or the update script will fail if there are any exising config perms to fix.