Certain paths should be reserved for paths in the file system such as includes, misc, modules, profiles, scripts, sites, and themes. When you try and add one of these in 'url path settings' it will allow the entry and redirect to the 404 page not found; if clean urls are allowed.

I'm not sure what the correct handling of these urls should be.

I'm re-submitting this to Core. I accidentally posted this under the old path module at http://drupal.org/node/118769

Files: 
CommentFileSizeAuthor
#39 drupal.sanitize_aliases_121362-39.patch4.57 KBJoshuaRogers
PASSED: [[SimpleTest]]: [MySQL] 56,081 pass(es).
[ View ]
#41 drupal.sanitize_aliases_121362-39.patch4.57 KBJoshuaRogers
PASSED: [[SimpleTest]]: [MySQL] 56,498 pass(es).
[ View ]
#37 drupal.sanitize_aliases_121362-37.patch5.59 KBJoshuaRogers
FAILED: [[SimpleTest]]: [MySQL] 55,927 pass(es), 26 fail(s), and 0 exception(s).
[ View ]
#31 drupal.sanitize_aliases_121362-31.patch3.2 KBJoshuaRogers
PASSED: [[SimpleTest]]: [MySQL] 35,227 pass(es).
[ View ]
#27 drupal.sanitize_aliases_121362-27.patch3.61 KBJoshuaRogers
PASSED: [[SimpleTest]]: [MySQL] 35,101 pass(es).
[ View ]
#22 drupal.sanitize_aliases_121362-22.patch3.55 KBJoshuaRogers
PASSED: [[SimpleTest]]: [MySQL] 35,937 pass(es).
[ View ]
#21 drupal.sanitize_aliases_121362-21.patch3.55 KBJoshuaRogers
PASSED: [[SimpleTest]]: [MySQL] 35,934 pass(es).
[ View ]
#15 drupal.sanitize_aliases_121362-15.patch1.94 KBJoshuaRogers
PASSED: [[SimpleTest]]: [MySQL] 35,794 pass(es).
[ View ]

Comments

Category:bug» feature

I'm not sure precisely what feature you're after from the first paragraph - do you want it to be impossible to set those paths, or do you want path.module to set them aside somehow (to define them seperately beforehand)?

Regardless, the standard Apache mod_rewrite config prevents Drupal from intercepting requests to existing files. Apache checks to see if a system file or directory exists: if it does, Drupal never gets a look-in.

I'm changing this to feature request as I don't see any buggy behaviour here: if you set a path to an existing file, the existing file is still served up by the webserver, isn't it?

Yes, I'd consider it a feature request at this point. It would be nice if path.module wouldn't allow a user to use a url of a known "reserved" path used by the product.

Just to make it clear, this is reserved as in, "won't work due to how apache handles rewrites" and not reserved as in "a core system URL" like admin, feed, etc. The first one I think would be a great addition, since I have run into that problem once before myself. The second wouldn't not be a good idea, since I have often needed to replace core URLs with my own pages as needed.

So, it's a +/-1 :)

Robin

@Robin, right.. I had to use the second method several places as well.

Title:Reserved paths with clean urlsDo not allow aliases to be created for existing or reserved paths
Version:5.1» 7.x-dev

Bumping to 7.x for a feature request. Also marking #366275: 403 on alias 'sites' as a duplicate of this issue.

Title:Do not allow aliases to be created for existing or reserved pathsDo not allow existing or reserved paths as aliases

Version:7.x-dev» 8.x-dev

Bumping to D8 since its too late for D7.

subscribing. #22336: Move all core Drupal files under a /core folder to improve usability and upgrades should help with making Drupal's path less likely to be used as URL aliases.

This issue is also there in D6. Will be nice if a solution is provided. I have run into this issue while creating a French site. There is a page titled "Thèmes" which is the French word for "Issues". The path for the page became "themes" which conflicts with the Drupal themes directory name and I am getting a "403 Forbidden error". Since clients want the path to remain the same (as we are migrating their old site keeping the paths same) it has become a big problem for me.

Merging in the following issues as duplicates:
#803382: Manually entered path can override another Drupal internal path
#1018960: Add hook_path_validate() API
#757732: Overriding Drupal paths

Not really part of the original issue but goes straight along with it is we want to limit the ability for users with just the 'create url aliases' permission to make URL alises for system paths like 'node', 'user', or 'admin'.

I've drafted up a D7 module Path restrict which essentially uses a method equivalent to hook_admin_paths() to determine if a URL alias should be allowed.

Edit: Created a sandbox project on Drupal for the code (instead of the GitHub repository that was previously linked).

We still want to make sure we can get this into D8 core as well.

I was really surprised when I discovered this issue. Isn't this a big security implication? The only required permission to mess up a Drupal site big time is "Create and edit URL aliases" (which are often given to simple content editors etc).

Then it's just to start hijacking critical core paths like "admin/config", "user/login" with e.g. a link to a malicious site.

I don't know what would be the ideal solution but perhaps protecting existing system paths, router items (like "node/%") and the files directory. Btw, isn't it weird that aliases are prioritized higher than system paths when a page request is made?

Assigned:Unassigned» JoshuaRogers
Status:Active» Needs review
StatusFileSize
new1.94 KB
PASSED: [[SimpleTest]]: [MySQL] 35,794 pass(es).
[ View ]

I've added some validation logic to the path module. It checks that the alias does not already map to another location, does not refer to a file on the filesystem, and does not begin with 'node'.

Is it a "file system path" or an "item on the file system"? When I think of path I think more of directory. Also; should your wildcard check also apply to users?

Just thinking aloud; the rest of the patch looks good on visual inspection.

/Robin

An item on the file system is definitely more accurate; it checks against both files and folders since either one would preempt Drupal.

You're right about users. Possibly taxonomy as well. Maybe I should make it check against the installed modules rather than keep a list? That would make sure that it matched taxonomy/, user/, node/, etc.

Yeah; the ultimate solution to this would be to be smart enough to parse though hook_menu; that could make a substantial performance hit, however.

I would recommend creating this check in its own function that then can be called from the form validation function. This then would allow unit testing (rather than a web test) and it also can be used when entities are created via code (like Feeds).

Status:Needs review» Needs work

Sounds good. I'll work on breaking that code out, adding some unit tests, and addressing the issues from #16.

Status:Needs work» Needs review
StatusFileSize
new3.55 KB
PASSED: [[SimpleTest]]: [MySQL] 35,934 pass(es).
[ View ]

Okay, I think that I've addressed the issues that were brought up. I ended up using a DrupalWebTestCase instead of a Unit test, however, since part of the validation logic does require the database.

StatusFileSize
new3.55 KB
PASSED: [[SimpleTest]]: [MySQL] 35,937 pass(es).
[ View ]

Rerolling

Status:Needs review» Needs work

<?php
} else {
    
// If it isn't a duplicate, it still might be invalid.
   
$valid = path_alias_is_valid($path['alias']);
?>

should be

<?php
}
else {
   
// If it isn't a duplicate, it still might be invalid.
   
$valid = path_alias_is_valid($path['alias']);
?>

This would be nice as a variable to allow sites to reverse additional name spaces for other entities

<?php
$restricted_paths
= array('node', 'taxonomy', 'user');
?>

Again this would be nice as a variable in case a site has an additional standalone file that bootstraps drupal etc

<?php
$restricted_files
= array('install.php', 'cron.php', 'update.php');
?>

The variable approach does seem sound; especially if you have forum software or the like living under Drupal's base domain scope and want to avoid conflicts. It would need to be documented in the relevant places for inclusion in a site-specific settings.php file.

That makes sense. I'll try to roll that up this evening.

Another quick niggle; I believe all of those form errors /should/ end with a period to make them correct structurally.

/Robin

Status:Needs work» Needs review
StatusFileSize
new3.61 KB
PASSED: [[SimpleTest]]: [MySQL] 35,101 pass(es).
[ View ]

Well, it seems that my concept of "this evening" really meant "two nights from now." ;) Either way, I've taken the advice from #23 and #26. I have not yet added the documentation to the patch, but intend to do that soon. In the mean time, here is the rerolled patch.

Status:Needs review» Needs work

+++ b/core/modules/path/path.moduleundefined
@@ -179,10 +179,58 @@ function path_form_element_validate($element, &$form_state, $complete_form) {
+  ¶

white space

Status:Needs work» Needs review

back to NR

Status:Needs review» Needs work

+++ b/core/modules/path/path.moduleundefined
@@ -179,10 +179,58 @@ function path_form_element_validate($element, &$form_state, $complete_form) {
+ * @return
+ *   TRUE if the alias is valid, otherwise the error message.
+ */

I think this interface of returning TRUE if it is valid and a string (which converts to TRUE) if it is not valid is incredibly confusing.

That is:

if (path_alias_is_valid($alias)) {
  // do something... always ?!
}

Status:Needs work» Needs review
StatusFileSize
new3.2 KB
PASSED: [[SimpleTest]]: [MySQL] 35,227 pass(es).
[ View ]

Rerolled with #28 and #30.

Status:Needs review» Reviewed & tested by the community

This is looking good to me.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/path/path.module
@@ -179,7 +179,55 @@ function path_form_element_validate($element, &$form_state, $complete_form) {
+  // We could create an alias that would conflict with another node/user/taxonomy
+  // later on, so we need to take account of that. (e.g. Create node 7
+  // with the alias 'node/9/delete'. It will make it impossible to delete
+  // node 9 later on without modifying node 7 first.) We pull it from configuration
+  // so that the user can modify it in their settings.php.
+  $restricted_paths = variable_get('restricted_path_aliases', '/(?:node|taxonomy|user)(?:\/|$)/i');
+  if (preg_match($restricted_paths, $alias)) {
+     return FALSE;
+  }

This breaks a built-in feature of the URL alias functionality: It is perfectly valid to create an alias node/123 for a new node ID 500. Especially useful when node/123 existed before, but got deleted and did not have an alias. Thus, node/500 exists on node/500 as well as on the former node/123 path (but the latter merely as an alias).

+++ b/core/modules/path/path.module
@@ -179,7 +179,55 @@ function path_form_element_validate($element, &$form_state, $complete_form) {
+  // Verify that no other module has reserved this address
+  $router_item = menu_get_item($alias);
+  if (!empty($router_item)) {
+    return FALSE;
   }

This breaks a built-in feature of the URL alias functionality: Intentionally overriding a router path through a URL alias.

+++ b/core/modules/path/path.module
@@ -179,7 +179,55 @@ function path_form_element_validate($element, &$form_state, $complete_form) {
+  // Before invoking Drupal, the webserver will first check the local
+  // filesystem for files/folders that match the path, so Drupal cannot
+  // match any path that would match an item on the filesystem.
+  // Additionally, .htaccess redirects some D7 paths to their D8 counterparts
+  // (e.g. cron.php). Those have to be enumerated and checked seperately.
+  $restricted_files = array('install.php', 'cron.php', 'update.php');
+  if (file_exists($alias) || in_array($alias, $restricted_files)) {
+    return FALSE;
+  }

I don't think this is necessary.

@sun You make some good points. The first two use cases showed why an administrator might want to create /node/7 or /user/login. It still might be problematic to give those rights to a site contributor though. Since aliases are enabled by default, a disgruntled contributor could effectively make the site unusable for a period by aliasing /user and /user/login. An admin without experience building HTTP requests or eiting databases might not know how to login to recover the site.

Would it be worth considering another permission that would be required for aliases that could break the site?

As for the last bit of code listed, I created a node with an alias "myfile.txt". When I attempted to view the node, the .htaccess file sent me to the file on my local filesystem instead. In this case it seems a bit misleading to let the user use the alias.

Thanks for the review @sun. :)

Subscribe; would love to have the path_alias_is_valid() handy, as I basically had to write it on my own for a project today. Useful for more than just core stuff.

Category:feature» bug

The problem with the existing functionality is that any user can create a page that take precedence over one of the default pages if they have the permission "Create and edit URL aliases". If that permission were marked with a warning of any type, it might not be a problem. As it is though, a novice administrator could find most of their users unable to login simply by giving a seemingly safe permission to a restricted role.

If we are to leave this functionality in place, could we split the "Create and edit URL aliases" permission into "Create and edit URL aliases" and "Override core URL aliases"? That would still provide the functionality while allowing a more granular set of permissions for users.

-- Edit --

So, I completely forgot (and overlooked) that I had already replied to this one. Rerolling a new patch. /embarassed.

-- Edited Again --

So, I had read the "I don't think this is necessary" as applying to the whole patch. I'm a mostly asleep idiot ATM. #drupalcon

Status:Needs work» Needs review
StatusFileSize
new5.59 KB
FAILED: [[SimpleTest]]: [MySQL] 55,927 pass(es), 26 fail(s), and 0 exception(s).
[ View ]

Rerolled using all of the advice from @sun.

Status:Needs review» Needs work

The last submitted patch, drupal.sanitize_aliases_121362-37.patch, failed testing.

Status:Needs review» Needs work
StatusFileSize
new4.57 KB
PASSED: [[SimpleTest]]: [MySQL] 56,081 pass(es).
[ View ]

Hmm. @sun didn't give the advice that the tests should fail though. That was all my idea.

Status:Needs work» Needs review

StatusFileSize
new4.57 KB
PASSED: [[SimpleTest]]: [MySQL] 56,498 pass(es).
[ View ]

The last submitted patch, drupal.sanitize_aliases_121362-39.patch, failed testing.

Status:Needs work» Needs review