Do not allow existing or reserved paths as aliases

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 📌 Do not allow existing or reserved paths as aliases Needs work , 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:
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.
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 📌 Do not allow existing or reserved paths as aliases Needs work
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 📌 Do not allow existing or reserved paths as aliases Needs work 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.

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

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

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.

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.

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.

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.

Looking at the most recent patch -- #67 I see that it adds `override url aliases` as a new permission. Which is, I think, correct. However, this does mean that, hypothetically, this could break for a user who is relying upon this ability. The classic "using a bug as a feature" scenario.

Since this is just a validation method, existing revisions would be unaffected. I think this makes sense. Does it make sense to reject this pattern on a new revision of existing content?

@bkosborne's comment 📌 Do not allow existing or reserved paths as aliases Needs work made me realize thatt his issue can apply to any system route, so instead of using the proposed pattern matching in #67, it verifies if the proposed alias matches any of the currently available routes. This also means that no configuration changes are needed.

Still need to determine appropriate unit tests.

Here's a patch that applies to D10.4.x or D10.3.x, using annotations instead of PHP attributes.

I also noticed that one of the functional tests had unnecessarily failed; I re-ran it and it passed. Interesting that it happened in a relevant test, but it was on an assertion about caching that doesn't look to me like it should have failed. I think caching is the kind of thing that can intermittently misbehave in the test runner?

I'm doing some local testing, but this is a promising solution. I had wondered about creating a contrib module to address the problem, but I'm reluctant to do so if that might make it less likely for a solution to arrive in core! Unless anyone can suggest why it might be better/worse to address this in contrib?

Argh, sorry, that last patch for D10 causes an error due to a missing trailing comma. This one is better!

> I had wondered about creating a contrib module to address the problem

I'm not sure that a contrib module is the appropriate solution to this, as this isn't a problem you're going to be aware of until it happens, the very definition of a bug. This is exactly what @bkosborne mentioned in #68 📌 Do not allow existing or reserved paths as aliases Needs work .

However, as other people have mentioned 📌 Do not allow existing or reserved paths as aliases Needs work , there certainly could be a use case for overriding a system path with an alias. There also could be a case where someone has inadvertently used this without realizing the implications. To address this, I added the permission. If this gets accepted into core, it should be clearly noted in the release notes.