Issue status update for http://drupal.org/node/27949 Post a follow up: http://drupal.org/project/comments/add/27949 Project: Drupal Version: cvs Component: profile.module Category: bug reports Priority: normal Assigned to: robertDouglass Reported by: robertDouglass Updated by: robertDouglass Status: patch (code needs review) patch still applies. Anyone care to review? robertDouglass Previous comments: ------------------------------------------------------------------------ Mon, 01 Aug 2005 12:32:07 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/profile_fix_acces_control_in_theme.txt (2.36 KB) The two theme functions in profile.module both violate good theming practice by running user control logic in the middle of them. Worse yet, this isn't immediately visible since it happens in yet another function. Thus themers overriding these functions to style profile pages[1] inadvertently break access control, thus leading to the misperception that overriding theme functions is inherently dangerous[2]. [1] http://drupal.org/node/16011 [2] http://drupal.org/node/16821

Issue status update for http://drupal.org/node/27949 Post a follow up: http://drupal.org/project/comments/add/27949 Project: Drupal Version: cvs Component: profile.module Category: bug reports Priority: normal Assigned to: robertDouglass Reported by: robertDouglass Updated by: chx Status: patch (code needs review) Just by looking at the code: -1. Never write $user we use $account in these places. There is a global $user and you do not want accidental mixup. If you change that, I think I'll like it :) chx Previous comments: ------------------------------------------------------------------------ Mon, 01 Aug 2005 12:32:07 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/profile_fix_acces_control_in_theme.txt (2.36 KB) The two theme functions in profile.module both violate good theming practice by running user control logic in the middle of them. Worse yet, this isn't immediately visible since it happens in yet another function. Thus themers overriding these functions to style profile pages[1] inadvertently break access control, thus leading to the misperception that overriding theme functions is inherently dangerous[2]. [1] http://drupal.org/node/16011 [2] http://drupal.org/node/16821 ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:37:40 +0000 : robertDouglass patch still applies. Anyone care to review? ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:41:46 +0000 : Dublin Drupaller Would like to review and help Robert, but I don't have a CVS version of drupal installed..will the patch work with 4.6.x? Dub

Issue status update for http://drupal.org/node/27949 Post a follow up: http://drupal.org/project/comments/add/27949 Project: Drupal Version: cvs Component: profile.module Category: bug reports Priority: normal Assigned to: robertDouglass Reported by: robertDouglass Updated by: chx Status: patch (code needs review) feel free to overwrite, it's common with nodes to do similar. chx Previous comments: ------------------------------------------------------------------------ Mon, 01 Aug 2005 12:32:07 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/profile_fix_acces_control_in_theme.txt (2.36 KB) The two theme functions in profile.module both violate good theming practice by running user control logic in the middle of them. Worse yet, this isn't immediately visible since it happens in yet another function. Thus themers overriding these functions to style profile pages[1] inadvertently break access control, thus leading to the misperception that overriding theme functions is inherently dangerous[2]. [1] http://drupal.org/node/16011 [2] http://drupal.org/node/16821 ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:37:40 +0000 : robertDouglass patch still applies. Anyone care to review? ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:41:46 +0000 : Dublin Drupaller Would like to review and help Robert, but I don't have a CVS version of drupal installed..will the patch work with 4.6.x? Dub ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:45:25 +0000 : chx Just by looking at the code: -1. Never write $user we use $account in these places. There is a global $user and you do not want accidental mixup. If you change that, I think I'll like it :) ------------------------------------------------------------------------ Thu, 18 Aug 2005 13:43:33 +0000 : robertDouglass chx: $account is already in use and I didn't want to replace it ($account = user_load()) - or do you think that would be ok in this case?

Issue status update for http://drupal.org/node/27949 Post a follow up: http://drupal.org/project/comments/add/27949 Project: Drupal Version: cvs Component: profile.module Category: bug reports Priority: normal Assigned to: robertDouglass Reported by: robertDouglass Updated by: killes@www.drop.org -Status: patch (code needs review) +Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/profile_2.patch (3.09 KB) I've re-rolled th epatch from Drupal root dir and without DOS line endings. I have no objections against this patch and I think the intention is a good one.. killes@www.drop.org Previous comments: ------------------------------------------------------------------------ Mon, 01 Aug 2005 12:32:07 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/profile_fix_acces_control_in_theme.txt (2.36 KB) The two theme functions in profile.module both violate good theming practice by running user control logic in the middle of them. Worse yet, this isn't immediately visible since it happens in yet another function. Thus themers overriding these functions to style profile pages[1] inadvertently break access control, thus leading to the misperception that overriding theme functions is inherently dangerous[2]. [1] http://drupal.org/node/16011 [2] http://drupal.org/node/16821 ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:37:40 +0000 : robertDouglass patch still applies. Anyone care to review? ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:41:46 +0000 : Dublin Drupaller Would like to review and help Robert, but I don't have a CVS version of drupal installed..will the patch work with 4.6.x? Dub ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:45:25 +0000 : chx Just by looking at the code: -1. Never write $user we use $account in these places. There is a global $user and you do not want accidental mixup. If you change that, I think I'll like it :) ------------------------------------------------------------------------ Thu, 18 Aug 2005 13:43:33 +0000 : robertDouglass chx: $account is already in use and I didn't want to replace it ($account = user_load()) - or do you think that would be ok in this case? ------------------------------------------------------------------------ Thu, 18 Aug 2005 20:26:20 +0000 : chx feel free to overwrite, it's common with nodes to do similar. ------------------------------------------------------------------------ Fri, 19 Aug 2005 15:10:39 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/profile_fix_acces_control_in_theme2.txt (2.95 KB) updated with $account

Issue status update for http://drupal.org/node/27949 Post a follow up: http://drupal.org/project/comments/add/27949 Project: Drupal Version: cvs Component: profile.module Category: bug reports Priority: normal Assigned to: robertDouglass Reported by: robertDouglass Updated by: robertDouglass Status: patch (ready to be committed) PS why didn't your follow up send a mail to the devel-list, Dries? robertDouglass Previous comments: ------------------------------------------------------------------------ Mon, 01 Aug 2005 12:32:07 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/profile_fix_acces_control_in_theme.txt (2.36 KB) The two theme functions in profile.module both violate good theming practice by running user control logic in the middle of them. Worse yet, this isn't immediately visible since it happens in yet another function. Thus themers overriding these functions to style profile pages[1] inadvertently break access control, thus leading to the misperception that overriding theme functions is inherently dangerous[2]. [1] http://drupal.org/node/16011 [2] http://drupal.org/node/16821 ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:37:40 +0000 : robertDouglass patch still applies. Anyone care to review? ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:41:46 +0000 : Dublin Drupaller Would like to review and help Robert, but I don't have a CVS version of drupal installed..will the patch work with 4.6.x? Dub ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:45:25 +0000 : chx Just by looking at the code: -1. Never write $user we use $account in these places. There is a global $user and you do not want accidental mixup. If you change that, I think I'll like it :) ------------------------------------------------------------------------ Thu, 18 Aug 2005 13:43:33 +0000 : robertDouglass chx: $account is already in use and I didn't want to replace it ($account = user_load()) - or do you think that would be ok in this case? ------------------------------------------------------------------------ Thu, 18 Aug 2005 20:26:20 +0000 : chx feel free to overwrite, it's common with nodes to do similar. ------------------------------------------------------------------------ Fri, 19 Aug 2005 15:10:39 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/profile_fix_acces_control_in_theme2.txt (2.95 KB) updated with $account ------------------------------------------------------------------------ Fri, 02 Sep 2005 16:12:21 +0000 : killes@www.drop.org Attachment: http://drupal.org/files/issues/profile_2.patch (3.09 KB) I've re-rolled th epatch from Drupal root dir and without DOS line endings. I have no objections against this patch and I think the intention is a good one.. ------------------------------------------------------------------------ Fri, 02 Sep 2005 22:28:38 +0000 : Dries I don't see why a theme function can't call helper functions? At least you can by-pass the helper function to theme things differently. If you pass pre-rendered fields to the theme function, the themer has less flexibility. Please re-open if I missed the point. ------------------------------------------------------------------------ Sat, 03 Sep 2005 07:41:49 +0000 : robertDouglass The helper function in quetion, profile_view_field($user, $field)) , implements all of the access permission checks for the entire module's output. It does two things; it sees whether the current user is allowed to see the field, and it builds the standard output for the field. Building the ouput usually means calling functions like check_plain() or l(). The entire point of the patch can be seen here: Before   foreach ($fields as $field) {     if ($value = profile_view_field($user, $field)) {       $output .= " <div class=\"field\">$value</div>\n";     }   } In this code, the profile_view_field($user, $field) call filters the $field array based on user permissions. It returns a non-themable, pre built piece of output called $value. After   foreach ($fields as $field) {     if ($field->value) {       $output .= " <div class=\"field\">$value</div>\n";     }   } The $fields array has already been filtered using profile_view_field so that only the fields that the current user is allowed to see are in it. What does a themer do with a theme function like this? In the first version, they take the $fields array and start extracting values from it to display the unthemable information as desired. Oops, user access goes out the window. In the second version, the themer extracts *the same*, unthemable information from the $fields array to display it as desired. No problems with access control. People who were theming this using the current theme functions were ending up with broken permissions and were, as a result, re-programming them at the theme level and writing in the handbook and forums that PHPTemplate doesn't respect permissions.

Issue status update for http://drupal.org/node/27949 Post a follow up: http://drupal.org/project/comments/add/27949 Project: Drupal Version: cvs Component: profile.module Category: bug reports Priority: normal Assigned to: robertDouglass Reported by: robertDouglass Updated by: Bèr Kessels Status: patch (ready to be committed) To add my voice to adrians: Security must never be an opt-out system, where, in the end, you decide what people may not see, and then remove that. It must always be opt-in, nothing is shown, unless explicitly told to do otherwise. The current system is an opt-out system, where the theme is supposed to remove the parts that may not be seen. Simply bad architecture. Bèr Kessels Previous comments: ------------------------------------------------------------------------ Mon, 01 Aug 2005 12:32:07 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/profile_fix_acces_control_in_theme.txt (2.36 KB) The two theme functions in profile.module both violate good theming practice by running user control logic in the middle of them. Worse yet, this isn't immediately visible since it happens in yet another function. Thus themers overriding these functions to style profile pages[1] inadvertently break access control, thus leading to the misperception that overriding theme functions is inherently dangerous[2]. [1] http://drupal.org/node/16011 [2] http://drupal.org/node/16821 ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:37:40 +0000 : robertDouglass patch still applies. Anyone care to review? ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:41:46 +0000 : Dublin Drupaller Would like to review and help Robert, but I don't have a CVS version of drupal installed..will the patch work with 4.6.x? Dub ------------------------------------------------------------------------ Thu, 18 Aug 2005 12:45:25 +0000 : chx Just by looking at the code: -1. Never write $user we use $account in these places. There is a global $user and you do not want accidental mixup. If you change that, I think I'll like it :) ------------------------------------------------------------------------ Thu, 18 Aug 2005 13:43:33 +0000 : robertDouglass chx: $account is already in use and I didn't want to replace it ($account = user_load()) - or do you think that would be ok in this case? ------------------------------------------------------------------------ Thu, 18 Aug 2005 20:26:20 +0000 : chx feel free to overwrite, it's common with nodes to do similar. ------------------------------------------------------------------------ Fri, 19 Aug 2005 15:10:39 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/profile_fix_acces_control_in_theme2.txt (2.95 KB) updated with $account ------------------------------------------------------------------------ Fri, 02 Sep 2005 16:12:21 +0000 : killes@www.drop.org Attachment: http://drupal.org/files/issues/profile_2.patch (3.09 KB) I've re-rolled th epatch from Drupal root dir and without DOS line endings. I have no objections against this patch and I think the intention is a good one.. ------------------------------------------------------------------------ Fri, 02 Sep 2005 22:28:38 +0000 : Dries I don't see why a theme function can't call helper functions? At least you can by-pass the helper function to theme things differently. If you pass pre-rendered fields to the theme function, the themer has less flexibility. Please re-open if I missed the point. ------------------------------------------------------------------------ Sat, 03 Sep 2005 07:41:49 +0000 : robertDouglass The helper function in quetion, profile_view_field($user, $field)) , implements all of the access permission checks for the entire module's output. It does two things; it sees whether the current user is allowed to see the field, and it builds the standard output for the field. Building the ouput usually means calling functions like check_plain() or l(). The entire point of the patch can be seen here: Before   foreach ($fields as $field) {     if ($value = profile_view_field($user, $field)) {       $output .= " <div class=\"field\">$value</div>\n";     }   } In this code, the profile_view_field($user, $field) call filters the $field array based on user permissions. It returns a non-themable, pre built piece of output called $value. After   foreach ($fields as $field) {     if ($field->value) {       $output .= " <div class=\"field\">$value</div>\n";     }   } The $fields array has already been filtered using profile_view_field so that only the fields that the current user is allowed to see are in it. What does a themer do with a theme function like this? In the first version, they take the $fields array and start extracting values from it to display the unthemable information as desired. Oops, user access goes out the window. In the second version, the themer extracts *the same*, unthemable information from the $fields array to display it as desired. No problems with access control. People who were theming this using the current theme functions were ending up with broken permissions and were, as a result, re-programming them at the theme level and writing in the handbook and forums that PHPTemplate doesn't respect permissions. ------------------------------------------------------------------------ Sat, 03 Sep 2005 07:45:14 +0000 : robertDouglass PS why didn't your follow up send a mail to the devel-list, Dries? ------------------------------------------------------------------------ Sat, 03 Sep 2005 08:40:21 +0000 : adrian The problem is that we pass all the fields to the theme layer, wether the user is allowed to see them or not. This would be like passing all nodes on the page, and expecting the theme to check node_access itself. There's no problem with helper functions, but these functions are testing to see wether or not the user has access to view this field. Permissions should never be done in the theme layer.