getgrav / grav

Modern, Crazy Fast, Ridiculously Easy and Amazingly Powerful Flat-File CMS powered by PHP, Markdown, Twig, and Symfony
https://getgrav.org
MIT License
14.59k stars 1.41k forks source link

Task URLs Interact Poorly With Casing Redirect Rules #3151

Open ScottHamper opened 3 years ago

ScottHamper commented 3 years ago

Summary

The admin plugin sends multiple AJAX requests on every page load in the format of, /admin/task:<methodName> (e.g., /admin/task:getNotifications). Additionally, a common practice (at least in the Windows/IIS world) is to enforce lowercase URL paths by redirecting requests with uppercase characters to equivalent all-lowercase URLs, leaving filenames and query strings unchanged.

Some examples:

The current URL structure of the admin plugin AJAX requests is such that it triggers a redirect when a web server is configured in the above way. For example, /admin/task:getNotifications gets redirected to /admin/task:getnotifications. When the lowercased URL is requested, Grav returns an error message, causing a "Fetch failed" box to appear on every single page in the admin web interface.

Steps to Reproduce

  1. Using URL Rewrite in IIS, or Mod Rewrite in Apache, configure a rule that will redirect visitors to a lowercased version of a URL with uppercase characters. See below for example configuration. Place this rule before the rewrite rules for the index page.
  2. Open up the Dashboard page in the admin web interface.

Example IIS rewrite rule:

<rule name="lower_case_except_filename" stopProcessing="true">
    <match url="^([^.]*?[A-Z][^.]*?)(\/[^/]+\.[^/]+)?$" ignoreCase="false" />
    <action type="Redirect" url="{ToLower:{R:1}}{R:2}" />
</rule>

Example Apache rewrite rule:

RewriteCond %{REQUEST_URI} ^([^.]*?[A-Z][^.]*?)(\/[^/]+\.[^/]+)?$
RewriteRule .* ${tolower:%1}%2 [R=301,L]

Expected Result

The Dashboard loads without displaying any error messages, and notifications are displayed.

Actual Result

A "Fetch failed" error message appears on the page, and the notifications pane gets stuck in a loading animation.

Possible Resolutions

  1. Remove web server configuration that redirects requests to enforce lowercasing. This resolution does not align with how I would like my site to function, in that it will trigger 404 error pages for pages with uppercase characters in the URL instead of taking visitors to where they want to go (recognizing that most visitors do not anticipate a difference between an /About or /about URL, for example).
  2. Leave the burden to me (and others like me) to add web server configuration that specifically knows about the task:<methodName> URLs and does not trigger a redirect for them. This seems like a brittle solution, as these URLs could change in the future, or more could be added. Along the same lines, the /admin part of the path is simply the default, and is something that Grav offers configuration for. By adding additional rewrite rules in the webserver configuration, it creates two places where that path name is configured instead of one, leading to a potential "gotcha" should there ever be a want or need to change this path in the future.
  3. Update the Grav Admin plugin to supply the method name via query string. E.g., /task:getNotifications becomes /task?m=getNotifications or something similar. It appears as if the colon character is currently acting like a custom implementation of a query string, so let's simply switch it to use the standard mechanism instead of rolling our own.
  4. Update the Grav Admin plugin to be case-insensitive for method names.
  5. Update the Grav Admin plugin to only use lowercase characters in its URLs. This potentially triggers the same issue, but in reverse (however, I think it is extraordinarily uncommon for a website to enforce any uppercase characters rather than lowercase).
  6. Update the main Grav project to include an option to redirect visitors to enforce lowercase URLs. This would remove the need for configuration at the web server level, and would allow Grav/Grav Admin to manage its own exceptions to the rewrite rule (moving the burden of the brittleness described in option 2 to the admin plugin itself, where it can be managed more effectively).
ScottHamper commented 3 years ago

Correction: the task:getNotifications URL is the only one that follows the format of /admin/task:<methodName>. The other similar URLs are /admin/update.json/task:getUpdates and admin/ajax.json/task:getNewsFeed. These latter two URLs do not trigger a redirect given the example configuration I provided in the opening post due to issues with the rewrite rule regex (the .json makes the regex think that everything after it is part of a filename extension, and thus is not rewritten).

Regardless, these latter two examples further indicate to me that the third possible resolution suggested in the opening post could be a better way of handling things. The three URLs could change to:

mahagr commented 3 years ago

Tasks are case insensitive so they will not break, the mistake you did was to redirect POST into GET. POST requests should never be altered.

Grav uses /foo:bar format for parameters that look like query parameters, and those can have case sensitive values. So you should ignore all sections which have : in them. Also NEVER change cases for the values of query parameters, just paths. You will break forms which use GET instead of POST as you will break user sent data.

The best aproach here would be to just let Grav to handle redirecting. It is pretty easy to write similar logic to trailing slash removal.

mahagr commented 3 years ago

@ScottHamper Moved the issue to proper project.

ScottHamper commented 3 years ago

Thanks, @mahagr ! Your explanation makes sense. What's the reasoning behind having these be POST requests instead of GET requests? At face value, it seems like it would be more semantic to use GET (the URLs even have the word "get" in their path!).

While I agree that adding a feature to Grav for handling case redirecting is worthwhile, I still think the existing task approach should also be modified. It's easy to write a redirect rule in IIS or mod rewrite to enforce lower case URLs while not affecting query string parameters. However, having to support custom ":" syntax is much more complicated and error prone. HTTP has query strings as a first class feature, web servers understand them, and they work the same regardless of page/application, but that is not the case for Grav's use of ":". From a web server's perspective, a ":" character is just another character in the URL path, and not something that has special meaning in some contexts.

Perhaps the issue is that we're trying to use an overly generalized system and it breaks down in particular situations? Is the concept of a "task" something that's generalized for many pages, such that sometimes it makes sense to have the request be an HTTP POST?

It seems like it would be more ideal to rely on "normal"/semantic HTTP mechanisms: