Project: Drupal Version: cvs Component: path.module Category: bug reports Priority: critical Assigned to: Anonymous Reported by: adrian Updated by: Boris Mann Status: patch When adrian said "user", he meant a site admin who is using one of our hosted installs. The issue remains: anyone can alias "admin" or any other existing path, which will override existing pages. Boris Mann Previous comments: ------------------------------------------------------------------------ February 15, 2005 - 12:51 : adrian Attachment: http://drupal.org/files/issues/path_menu_exists.diff (2.27 KB) One of our users recently tried to link to /admin , using the path module. And the link was allowed to be created. This meant his admin menu was disabled, which should not be able to happen. Attached is a function added to menu.inc that steps through all the menu items, and finds out if there is a callback registered for a specific path, unfortunately there still exists a few problems : Only explicitly registered callbacks are detected. For instance: 'node' will be picked up, but 'node/12' will be allowed. This is not as easy to test for, because of how pages default back to the top most callback, and pass the rest of the fields as paramaters. The only way to work around this is to explicitly define all callbacks that use parameters with a 'paramaters'=> true in the item. When an additional module gets enabled, any already specified links will take precedence over the new links created by it. ------------------------------------------------------------------------ February 15, 2005 - 14:47 : Axel I think just not allow to users change paths - this instrument is for site admins/editors only. Additional checks will slow down path module. If site admin want to allow users change paths to their own manner then such job must doing by separate module, which will offer interface for change paths with restrictions and additional checks. -- View: http://drupal.org/node/17386 Edit: http://drupal.org/project/comments/add/17386

Project: Drupal Version: cvs Component: path.module Category: bug reports Priority: critical Assigned to: Anonymous Reported by: adrian Updated by: Anonymous Status: patch administrators are users too. =) And this stops the most common abuse, but there are still special cases I mentioned. Also, there is exactly 1 for loop extra and an extra if statement, only upon submission of a new path. I hardly think it is a performance issue. Anonymous Previous comments: ------------------------------------------------------------------------ February 15, 2005 - 20:51 : adrian Attachment: http://drupal.org/files/issues/path_menu_exists.diff (2.27 KB) One of our users recently tried to link to /admin , using the path module. And the link was allowed to be created. This meant his admin menu was disabled, which should not be able to happen. Attached is a function added to menu.inc that steps through all the menu items, and finds out if there is a callback registered for a specific path, unfortunately there still exists a few problems : Only explicitly registered callbacks are detected. For instance: 'node' will be picked up, but 'node/12' will be allowed. This is not as easy to test for, because of how pages default back to the top most callback, and pass the rest of the fields as paramaters. The only way to work around this is to explicitly define all callbacks that use parameters with a 'paramaters'=> true in the item. When an additional module gets enabled, any already specified links will take precedence over the new links created by it. ------------------------------------------------------------------------ February 15, 2005 - 22:47 : Axel I think just not allow to users change paths - this instrument is for site admins/editors only. Additional checks will slow down path module. If site admin want to allow users change paths to their own manner then such job must doing by separate module, which will offer interface for change paths with restrictions and additional checks. ------------------------------------------------------------------------ February 15, 2005 - 23:18 : Boris Mann When adrian said "user", he meant a site admin who is using one of our hosted installs. The issue remains: anyone can alias "admin" or any other existing path, which will override existing pages. ------------------------------------------------------------------------ February 16, 2005 - 01:50 : Bèr Kessels " Additional checks will slow down path module." True. But IMO thats non-issue in the rare case of a path added. How will adding 100 aliasees a day (not sure if any site out there makes that) affect performance? Performance issues on adminstrative functions should have a very low priority. Below all others like safety, useability and all. For the rest: I agree upon a separate advanced path module. We have more "unsafe" issues, such as people being able to add PHP, block themselves, block Superuser, etc. A line of documentation on this might suffice? -- View: http://drupal.org/node/17386 Edit: http://drupal.org/project/comments/add/17386

Project: Drupal Version: cvs Component: path.module Category: bug reports Priority: critical Assigned to: Anonymous Reported by: adrian Updated by: javanaut Status: patch How about setting a priority level as an attribute of the path item? Path module could use a lower priority than the default and thus "lose" any conflicts with existing paths. There are modules that need to override other paths and redefine the actions/access/etc for them. This would provide a means to support both of these needs. This would also remove the current dependence on alphabetic module name preference for conflicting paths (later modules currently overwrite previous modules' path entries). javanaut Previous comments: ------------------------------------------------------------------------ February 15, 2005 - 14:51 : adrian Attachment: http://drupal.org/files/issues/path_menu_exists.diff (2.27 KB) One of our users recently tried to link to /admin , using the path module. And the link was allowed to be created. This meant his admin menu was disabled, which should not be able to happen. Attached is a function added to menu.inc that steps through all the menu items, and finds out if there is a callback registered for a specific path, unfortunately there still exists a few problems : Only explicitly registered callbacks are detected. For instance: 'node' will be picked up, but 'node/12' will be allowed. This is not as easy to test for, because of how pages default back to the top most callback, and pass the rest of the fields as paramaters. The only way to work around this is to explicitly define all callbacks that use parameters with a 'paramaters'=> true in the item. When an additional module gets enabled, any already specified links will take precedence over the new links created by it. ------------------------------------------------------------------------ February 15, 2005 - 16:47 : Axel I think just not allow to users change paths - this instrument is for site admins/editors only. Additional checks will slow down path module. If site admin want to allow users change paths to their own manner then such job must doing by separate module, which will offer interface for change paths with restrictions and additional checks. ------------------------------------------------------------------------ February 15, 2005 - 17:18 : Boris Mann When adrian said "user", he meant a site admin who is using one of our hosted installs. The issue remains: anyone can alias "admin" or any other existing path, which will override existing pages. ------------------------------------------------------------------------ February 15, 2005 - 19:50 : Bèr Kessels " Additional checks will slow down path module." True. But IMO thats non-issue in the rare case of a path added. How will adding 100 aliasees a day (not sure if any site out there makes that) affect performance? Performance issues on adminstrative functions should have a very low priority. Below all others like safety, useability and all. For the rest: I agree upon a separate advanced path module. We have more "unsafe" issues, such as people being able to add PHP, block themselves, block Superuser, etc. A line of documentation on this might suffice? ------------------------------------------------------------------------ February 15, 2005 - 22:31 : Anonymous administrators are users too. =) And this stops the most common abuse, but there are still special cases I mentioned. Also, there is exactly 1 for loop extra and an extra if statement, only upon submission of a new path. I hardly think it is a performance issue. ------------------------------------------------------------------------ February 16, 2005 - 01:00 : andremolnar +1 for adding the menu_item_exists function even with its limitations that adrian has pointed out. "* When an additional module gets enabled, any already specified links will take precedence over the new links created by it. " Could you include a similar check when enabling a module. i.e. a function called from module.module that would look at the menu callbacks of the newly enabled module, check them against the menu array and post an warning on the admin/module page and add an entry to watchdog. Admins would at least be notified about the problem and could take appropriate action. e.g. Warning: A path alias exists for $path. This path is in conflict with a registered path in the newly enabled module $module. This conflict will not allow module $module to function properly. The problem can be corrected by renaming $path to something else from linktoadmin/alias. and watchdog could have something like a user error with a short description "path conflict" and a detailed message similar to the example above. andre -- View: http://drupal.org/node/17386 Edit: http://drupal.org/project/comments/add/17386

Project: Drupal Version: cvs Component: path.module Category: bug reports Priority: critical Assigned to: Anonymous Reported by: adrian Updated by: Anonymous Status: patch I, for one, would prefer that the ability to override paths stay intact. I have used it to override certain Drupal-generated pages, such as the user information page on my blog. If this patch were to land, I would have to do this with the URL rewrite functionality, which would undoubted have slower performance and be more difficult. Perhaps rather than strictly prohibiting this behavior, we could instead present an extra confirmation screen noting that "This will prevent access to the non-aliased path. Are you sure you want to do this?" -1 for the patch, as-is... you interpret this as a bug; I see it as a feature. +1 iff we double-check with the user instead of strictly prohibiting. Anonymous Previous comments: ------------------------------------------------------------------------ February 15, 2005 - 20:51 : adrian Attachment: http://drupal.org/files/issues/path_menu_exists.diff (2.27 KB) One of our users recently tried to link to /admin , using the path module. And the link was allowed to be created. This meant his admin menu was disabled, which should not be able to happen. Attached is a function added to menu.inc that steps through all the menu items, and finds out if there is a callback registered for a specific path, unfortunately there still exists a few problems : Only explicitly registered callbacks are detected. For instance: 'node' will be picked up, but 'node/12' will be allowed. This is not as easy to test for, because of how pages default back to the top most callback, and pass the rest of the fields as paramaters. The only way to work around this is to explicitly define all callbacks that use parameters with a 'paramaters'=> true in the item. When an additional module gets enabled, any already specified links will take precedence over the new links created by it. ------------------------------------------------------------------------ February 15, 2005 - 22:47 : Axel I think just not allow to users change paths - this instrument is for site admins/editors only. Additional checks will slow down path module. If site admin want to allow users change paths to their own manner then such job must doing by separate module, which will offer interface for change paths with restrictions and additional checks. ------------------------------------------------------------------------ February 15, 2005 - 23:18 : Boris Mann When adrian said "user", he meant a site admin who is using one of our hosted installs. The issue remains: anyone can alias "admin" or any other existing path, which will override existing pages. ------------------------------------------------------------------------ February 16, 2005 - 01:50 : Bèr Kessels " Additional checks will slow down path module." True. But IMO thats non-issue in the rare case of a path added. How will adding 100 aliasees a day (not sure if any site out there makes that) affect performance? Performance issues on adminstrative functions should have a very low priority. Below all others like safety, useability and all. For the rest: I agree upon a separate advanced path module. We have more "unsafe" issues, such as people being able to add PHP, block themselves, block Superuser, etc. A line of documentation on this might suffice? ------------------------------------------------------------------------ February 16, 2005 - 04:31 : Anonymous administrators are users too. =) And this stops the most common abuse, but there are still special cases I mentioned. Also, there is exactly 1 for loop extra and an extra if statement, only upon submission of a new path. I hardly think it is a performance issue. ------------------------------------------------------------------------ February 16, 2005 - 07:00 : andremolnar +1 for adding the menu_item_exists function even with its limitations that adrian has pointed out. "* When an additional module gets enabled, any already specified links will take precedence over the new links created by it. " Could you include a similar check when enabling a module. i.e. a function called from module.module that would look at the menu callbacks of the newly enabled module, check them against the menu array and post an warning on the admin/module page and add an entry to watchdog. Admins would at least be notified about the problem and could take appropriate action. e.g. Warning: A path alias exists for $path. This path is in conflict with a registered path in the newly enabled module $module. This conflict will not allow module $module to function properly. The problem can be corrected by renaming $path to something else from linktoadmin/alias. and watchdog could have something like a user error with a short description "path conflict" and a detailed message similar to the example above. andre ------------------------------------------------------------------------ February 16, 2005 - 16:11 : javanaut How about setting a priority level as an attribute of the path item? Path module could use a lower priority than the default and thus "lose" any conflicts with existing paths. There are modules that need to override other paths and redefine the actions/access/etc for them. This would provide a means to support both of these needs. This would also remove the current dependence on alphabetic module name preference for conflicting paths (later modules currently overwrite previous modules' path entries). ------------------------------------------------------------------------ February 16, 2005 - 17:42 : javanaut Attachment: http://drupal.org/files/issues/menu_prioritize.patch (1.36 KB) I tested this patch and it appears to work. It avoids having to iterate through the menu any more than necessary, and typically only adds one extra comparison to the process. Modules would add an $item[n]['priority'] = -1; to the menu hook to keep from overwriting menu items with a default of 0. The only oddity that I've seen is that there are conflicts between cached and non-cached instances of a given path, though that could have just been from my testing. -- View: http://drupal.org/node/17386 Edit: http://drupal.org/project/comments/add/17386

Project: Drupal Version: cvs Component: path.module Category: bug reports Priority: critical Assigned to: Anonymous Reported by: adrian Updated by: Steven Status: patch I agree with the Anonymous user above: this is a useful feature and we should put in a warning instead. Steven Previous comments: ------------------------------------------------------------------------ February 15, 2005 - 21:51 : adrian Attachment: http://drupal.org/files/issues/path_menu_exists.diff (2.27 KB) One of our users recently tried to link to /admin , using the path module. And the link was allowed to be created. This meant his admin menu was disabled, which should not be able to happen. Attached is a function added to menu.inc that steps through all the menu items, and finds out if there is a callback registered for a specific path, unfortunately there still exists a few problems : Only explicitly registered callbacks are detected. For instance: 'node' will be picked up, but 'node/12' will be allowed. This is not as easy to test for, because of how pages default back to the top most callback, and pass the rest of the fields as paramaters. The only way to work around this is to explicitly define all callbacks that use parameters with a 'paramaters'=> true in the item. When an additional module gets enabled, any already specified links will take precedence over the new links created by it. ------------------------------------------------------------------------ February 15, 2005 - 23:47 : Axel I think just not allow to users change paths - this instrument is for site admins/editors only. Additional checks will slow down path module. If site admin want to allow users change paths to their own manner then such job must doing by separate module, which will offer interface for change paths with restrictions and additional checks. ------------------------------------------------------------------------ February 16, 2005 - 00:18 : Boris Mann When adrian said "user", he meant a site admin who is using one of our hosted installs. The issue remains: anyone can alias "admin" or any other existing path, which will override existing pages. ------------------------------------------------------------------------ February 16, 2005 - 02:50 : Bèr Kessels " Additional checks will slow down path module." True. But IMO thats non-issue in the rare case of a path added. How will adding 100 aliasees a day (not sure if any site out there makes that) affect performance? Performance issues on adminstrative functions should have a very low priority. Below all others like safety, useability and all. For the rest: I agree upon a separate advanced path module. We have more "unsafe" issues, such as people being able to add PHP, block themselves, block Superuser, etc. A line of documentation on this might suffice? ------------------------------------------------------------------------ February 16, 2005 - 05:31 : Anonymous administrators are users too. =) And this stops the most common abuse, but there are still special cases I mentioned. Also, there is exactly 1 for loop extra and an extra if statement, only upon submission of a new path. I hardly think it is a performance issue. ------------------------------------------------------------------------ February 16, 2005 - 08:00 : andremolnar +1 for adding the menu_item_exists function even with its limitations that adrian has pointed out. "* When an additional module gets enabled, any already specified links will take precedence over the new links created by it. " Could you include a similar check when enabling a module. i.e. a function called from module.module that would look at the menu callbacks of the newly enabled module, check them against the menu array and post an warning on the admin/module page and add an entry to watchdog. Admins would at least be notified about the problem and could take appropriate action. e.g. Warning: A path alias exists for $path. This path is in conflict with a registered path in the newly enabled module $module. This conflict will not allow module $module to function properly. The problem can be corrected by renaming $path to something else from linktoadmin/alias. and watchdog could have something like a user error with a short description "path conflict" and a detailed message similar to the example above. andre ------------------------------------------------------------------------ February 16, 2005 - 17:11 : javanaut How about setting a priority level as an attribute of the path item? Path module could use a lower priority than the default and thus "lose" any conflicts with existing paths. There are modules that need to override other paths and redefine the actions/access/etc for them. This would provide a means to support both of these needs. This would also remove the current dependence on alphabetic module name preference for conflicting paths (later modules currently overwrite previous modules' path entries). ------------------------------------------------------------------------ February 16, 2005 - 18:42 : javanaut Attachment: http://drupal.org/files/issues/menu_prioritize.patch (1.36 KB) I tested this patch and it appears to work. It avoids having to iterate through the menu any more than necessary, and typically only adds one extra comparison to the process. Modules would add an $item[n]['priority'] = -1; to the menu hook to keep from overwriting menu items with a default of 0. The only oddity that I've seen is that there are conflicts between cached and non-cached instances of a given path, though that could have just been from my testing. ------------------------------------------------------------------------ February 17, 2005 - 20:17 : Anonymous I, for one, would prefer that the ability to override paths stay intact. I have used it to override certain Drupal-generated pages, such as the user information page on my blog. If this patch were to land, I would have to do this with the URL rewrite functionality, which would undoubted have slower performance and be more difficult. Perhaps rather than strictly prohibiting this behavior, we could instead present an extra confirmation screen noting that "This will prevent access to the non-aliased path. Are you sure you want to do this?" -1 for the patch, as-is... you interpret this as a bug; I see it as a feature. +1 iff we double-check with the user instead of strictly prohibiting. ------------------------------------------------------------------------ February 17, 2005 - 23:25 : Axel "When adrian said "user", he meant a site admin who is using one of our hosted installs. " I think only if the administrator has drunk a lot of beer, him can ovewrite 'admin' path. *Don't drive if you drunken!* - 1 for patch which add uneccessary complexity to path module. +1 for adding checkbox in site logon: "You are drunk today?" (instead of not worked "remember me"). This option will disable some "dangerous" functions like access to admin pages and so on. -- View: http://drupal.org/node/17386 Edit: http://drupal.org/project/comments/add/17386