Problem/Motivation

In Drupal 8 and earlier, it is possible for a user to create a path alias that clobbers an essential core route such as "admin" or "user/login". For example, a malicious user with permission to create content using an unfiltered text format and to "Create and edit URL aliases" could override the "user/login" path with a node crafted to look like it but with an HTML form that posted the submission to another URL that harvested the login credentials. Of course less serious abuses can be easily imagined, such as merely rendering certain admin pages inaccessible.

Proposed resolution

  • Adds a setting for restricted path alias patterns
  • Adds a validation constraint to enforce it
  • Adds a permission to bypass it that admins can use

Remaining tasks

  • Decide if this is a bug or a task, the presence of new API makes me think its a task
  • Reroll the patch at #67 as an MR
  • Document the new setting in default.settings.php
  • Add test coverage
  • Review
  • Commit

User interface changes

None

API changes

New validation constraint and permission

Release notes snippet

Todo

Issue fork drupal-121362

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jp.stacey’s picture

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?

JHeffner’s picture

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.

Robin Monks’s picture

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

JHeffner’s picture

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

Dave Reid’s picture

Title: Reserved paths with clean urls » Do 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.

Dave Reid’s picture

Title: Do not allow aliases to be created for existing or reserved paths » Do not allow existing or reserved paths as aliases
Dave Reid’s picture

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

Bumping to D8 since its too late for D7.

scor’s picture

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.

a6hiji7’s picture

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.

Dave Reid’s picture

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'.

bfroehle’s picture

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).

Robin Monks’s picture

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

betamos’s picture

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?

Dave Reid’s picture

Anonymous’s picture

Assigned: Unassigned »
Status: Active » Needs review
FileSize
1.94 KB

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'.

Robin Monks’s picture

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

Anonymous’s picture

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.

Robin Monks’s picture

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.

Lars Toomre’s picture

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).

Anonymous’s picture

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.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
3.55 KB

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.

Anonymous’s picture

Rerolling

marcingy’s picture

Status: Needs review » Needs work
} else {
     // If it isn't a duplicate, it still might be invalid.
    $valid = path_alias_is_valid($path['alias']);

should be

} 
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

$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

$restricted_files = array('install.php', 'cron.php', 'update.php');
Robin Monks’s picture

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.

Anonymous’s picture

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

Robin Monks’s picture

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

/Robin

Anonymous’s picture

Status: Needs work » Needs review
FileSize
3.61 KB

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.

attiks’s picture

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

attiks’s picture

Status: Needs work » Needs review

back to NR

bfroehle’s picture

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 ?!
}
Anonymous’s picture

Status: Needs work » Needs review
FileSize
3.2 KB

Rerolled with #28 and #30.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

This is looking good to me.

sun’s picture

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.

Anonymous’s picture

@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. :)

geerlingguy’s picture

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.

Anonymous’s picture

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

Anonymous’s picture

Status: Needs work » Needs review
FileSize
5.59 KB

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.

Anonymous’s picture

Status: Needs review » Needs work
FileSize
4.57 KB

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

Anonymous’s picture

Status: Needs work » Needs review
Anonymous’s picture

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

Anonymous’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

scor’s picture

Issue summary: View changes
TravisCarden’s picture

Assigned: » Unassigned
Issue summary: View changes
Related issues: +#118769: Reserved paths with clean urls

The security team has discussed this internally and come to the consensus that the issue can be handled publicly, since any phishing or access escalation vectors that would be exacerbated by this behavior would already be exploitable without it. The chief concern therefore is one of stability--that users either unwittingly or maliciously could create aliases that render certain core functionality unusable. Here are the questions I can think of that still need to be answered:

  • What should the system do when a user tries to create an alias at a taken path?
  • What constitutes a taken path (e.g., core routes? module routes? directories? physical files?), and how will they be detected?
  • What about an alias that preexists a new route, such as an alias that conflicts with a later added contrib module route when that module is enabled?
  • And finally, to @geerlingguy's point in #35, can/should we expose the test via an API function that contrib modules can use, too?

Note: I'm unassigning the issue since it's been silent for two years.

scor’s picture

Issue tags: +Security improvements
mbaynton’s picture

I think the main use case for aliases doesn't require them to have the overarching capabilities to override things that they have now.

The suggestion in #36 to have a 'safe' url alias permission more suitable for untrusted content authors, and a restricted 'override' URL alias permission makes sense to me as a way to retain backwards compatibility while fixing the security problems. But, I wonder if attempting to validate whether a particular alias requires the 'override' permission is the right approach. If aliases are stored in a way that the permission used to create them is recorded, I think it's just a matter of when in the "routing" process which aliases are applied. Current aliases, and any aliases built using the 'override' permission continue to impact the path before the fist attempt to find the matching route. New aliases built using the 'safe' permission are only applied when no route/controller was found on the first pass, one step before returning a 404. If a 'safe' alias matches at this stage, the route matching/controller locating logic is run again on the de-aliased path.

Here are the questions I can think of that still need to be answered:

What should the system do when a user tries to create an alias at a taken path?

For 'override' aliases, happily allow as is done today. For 'safe' aliases, apply the same logic used to decide if a path matches something when a request is received, and if the new alias already matches something, fail the form validation so the user can choose another one.

What about an alias that preexists a new route, such as an alias that conflicts with a later added contrib module route when that module is enabled?

This is a strength of this approach; pre-existing 'safe' aliases are superseded by the new route. There could always be path conflicts in cases like adding a module, but I think this is a better resolution strategy than we have today.

And finally, to @geerlingguy's point in #35, can/should we expose the test via an API function that contrib modules can use, too?

The only test for 'safe' aliases should be whether a request to the path would return a 404 (alias is available), or something other than a 404, (alias is not available).

The UI on content creation would change to create 'safe' aliases; legacy aliases with overarching power to override things would be relegated to the administrative aliases UI (and hopefully, would have precious few items on new sites.) This plan would align very well with #1553358-40: Figure out a new implementation for url aliases, which also suggests moving towards a new alias storage model, with the old one left intact but becoming uncommon, and having most alias-creating UI manipulating the new alias entities.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

osman’s picture

security vs. convenience...

I am having a similar issue with @a6hiji7 #121362-9: Do not allow existing or reserved paths as aliases

I wanted to have a page called Themes, with an alias of /themes. It automatically saved as /themes-0

Are there any workarounds?

TravisCarden’s picture

Sorry, @osman. /themes isn't even a reserved path--it's a physical directory. So you're not fighting Drupal but Apache. It might be possible to do something in .htaccess, but it would be a complicated, non-standard hack.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Priority: Normal » Major

Discussed this with other committers on a triage call in relation to #2203573: Path alias validation and tests duplicative, inconsistent and decided since there's a number of duplicate issues of this opened over the years, we should raise the priority to 'major'.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

johnwebdev’s picture

Old, but still an existing issue. I built a sandbox https://www.drupal.org/sandbox/johndevman/3038709 where I added a new constraint "ExistingPath" to the PathItem. Still needs some more test coverage, but seems to do the trick so far.

johnwebdev’s picture

icgurney’s picture

This is still an issue, and I hope one of these patches gets applied soon.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

anrikun’s picture

I'm bumping this as it is really a major issue.

xjm’s picture

Everyone who's hoping that this can get committed could help by updating the patch to work on 9.3.x. :) (The code is very different than it was in 2013 so it would not merely be a reroll.) Thanks!

eiriksm’s picture

Assigned: Unassigned » eiriksm

Working on a re-roll

eiriksm’s picture

Assigned: eiriksm » Unassigned
FileSize
5.2 KB

Hm, re-read the comments a bit, and probably a re-roll is not what we want?

Quick re-roll except the test is here. And did a change discussed below.

Have we reached a consensus about the following?

- Should we disallow node/33 alias for a node when a user does not have a new "safe" permission?
- Should we disallow other paths than a hardcoded list of paths?
- Should we make it possible to configure the list of paths? I think we should, just a slight change to the latest patch anyway.

Also: If we allow it to be configured, maybe we should allow it to be configured a bit more flexible than just the first part of the path?

Unassigning since I do not have time to address my own feedback right now. But here's a (re)start.

bkosborne’s picture

Oof, just ran into this when I was debugging an issue where the media library widget on a site stopped working! After lots of xdebugging, turns out that it was because the AJAX request the widget makes to /media-library was overwritten by a page on the site with the same URL!

quietone’s picture

Version: 8.9.x-dev » 9.3.x-dev
Issue summary: View changes
Issue tags: -Needs reroll +Needs issue summary update

The proposed resolution section of the IS doesn't make it clear what changes are being implemented. Can someone update that.

This needs to be on 9.3.x now.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mxr576’s picture

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tgoeg’s picture

Just lost my (lengthy) comment because of a Firefox crash, I'll keep it shorter, ask for details if needed, please!

I do think this should be handled as a security issue.
With a respective setup for example, even unauthenticated users might take over core paths on sites where some comment functionality or similar might create aliases (e.g. Path Auto setup to make human readable aliases of comments' URLs by using the title or first few words as pattern (this is a common feature you see on the Internet). I also don't see how an editor can create a legit looking /user/login site without this "feature".

Ran into this with a site using permissions by term. A node with alias /batch had been created. Afterwards, a user was created, permissions by term then drops all privileges and rebuilds them. In batches :). Guess what happened. No rebuilt permissions as the alias caught the request to /batch&id=132, site completely broken.

The security issue alone should warrant a "critical" priority I think, even more now that I see this is postponed to 11.x which means this issue that's already 6 years old will have to wait a few more for a fix.

As stated above, we need to come to a consensus how this should be solved before updating the IS. Reading through this whole thread shows a few different use-cases I try to cover.
Here's my suggestion:

  1. Provide a separate permission for a manual override, as suggested in https://www.drupal.org/project/drupal/issues/121362#comment-7437458, e.g. "Override core URL aliases", though I'd call it "Override existing, whitelisted URLs (created by core/modules) with aliases". Add a link to the config page.
  2. Provide another permission, "Override any existing URL with an alias", meant only for admins. This way, a granular configuration of what editors may override can be performed without restricting admins that may need to override even more.
  3. Build a list of found core/module(/file, see below) paths, updated probably in cron runs and module/core installation steps. I would not hardcode this, as it can change quickly.
  4. Provide a config page as can be seen in this mockup. Shamelessly copied from the config_split module; there might also be code there that can be reused for the list/form: Mockup showing config options
    1. String-based whitelist for aliases anticipated but not existing yet. Or for specific ones like /node/123 (which should not be part of the automatic list)
    2. Blacklist might not be necessary, as everything not on the whitelist should be blacklisted, anyway. Might however be practical to reduce whitelist-writing efforts, e.g. whitelist /node/* but blacklist /node/1. This would be tedious with whitelist only.
  5. Show a warning when saving a page that overrides an alias.Mockup showing a warning that the alias overrides an existing URL.
  6. Show an error when trying to save a page and permissions/white-/blacklists do not allow overriding as in https://www.drupal.org/project/drupal/issues/121362#comment-13013100
  7. The whitelist/blacklist should also apply to modules like e.g. pathauto. If a module tries to assign an alias that is not allowed, at least log that to the DB log. Better yet, show it in the status page and the top of the admin interface, so people notice something is going wrong. Unsure whether this should be part of core's path functionality or every module itself. Sounds more like the former, as it will catch all errors.
    This, together with the dynamically built list, also solves the problem of existing white-/blacklists and URLs that get added afterwards.
  8. Unsure how to handle aliases that resolve to filesystem paths. Those should be forbidden by default. Probably also determine them in the list-building process and explicitly add them to the blacklist. Or make a separate list for them.

This should implicitly answer all the questions in https://www.drupal.org/project/drupal/issues/121362#comment-14103203 with "Yes, configurable as outlined in the mockup".

A completely different strategy might be to explicitly trigger a request to the alias that shall be added and determine whether it is free by the answer received.

catch’s picture

Priority: Major » Critical

@tgoeg "I do think this should be handled as a security issue."

Handling this as a security issue would mean unpublishing this issue, opening a new issue in the private security tracker, and then any solution having to go through the core security release process. Given this issue has been open for 16 years in the public queue that is not going to stop anyone finding out about it. It's also very hard/impossible to do security releases with significant behaviour changes like a new configuration page, which is something that would normally only be introduced in a minor release, not to (at this time) 9.4.x

I think there's an argument this allows users in certain configurations to DOS a site though, so agreed with making it a critical bug.

tgoeg’s picture

The 'net does not forget, as they say.

I totally agree with you. I did not want to change the issue administration-wise, but reflect that there definitely is a security aspect to it, which should not be taken lightheartedly (which I felt was the case, reading the above comments).
If a critical priority reduces the time to fix this, this should be enough. Knowing that 9.x and even 10.x will remain without a fix however feels pretty uncomfortable. I think we should at least strive for a patch that applies to 10.x if it really is too late for any changes in this major series..

catch’s picture

The change could land in a minor release (i.e. the next one would be 10.2.x now that 10.1.0 is at rc stage), it just won't land in a patch release if it's adding new configuration and a UI for it.

larowlan’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

This is now the oldest open bug in core.

Taking a look at it for Bug Smash Initiative.

What is being added here looks like a new API - so perhaps this should be a task.

In that case we would call it 'Add a restricted patterns setting to limit what can be used as path aliases'

The current patch does the following:

* Adds a setting for restricted path alias patterns
* Adds a validation constraint to enforce it
* Adds a permission to bypass it that admins can use

I think this is a reasonable approach to getting something in - this issue has been open for 17 years.

The next steps are:
* Reroll the patch as an MR
* Add test coverage
* Document the setting in the default.settings.php file.

Does anyone feel strongly about keeping this as a bug?

kim.pepper made their first commit to this issue’s fork.

pameeela’s picture

I came here to see whether there was any discussion of bug vs task :)

Oddly enough, just last week we had a use case to redirect /user/register because they are about to launch a new site and wanted to pause registrations. A redirect to a content page explaining the situation was by far the easiest way to achieve this, because it was linked from many places across the site.

So this combined with the fact that it generally feels like an add-on leads me to task.

Edit: Oops, realised this applies specifically to aliases, not sure whether it changes the equation...

catch’s picture

Category: Bug report » Task

Yeah I bumped this to critical in #75, because I think the example given that certain combination of permissions are able to do worse things due to this issue than they otherwise would be is a good one, but I also think it's a task. There's no functional bug as such, this was intended behaviour when it was added, it's just that this issue wants to change that behaviour to make things more locked down.